Angular Project Structure & Code Reviews

1️⃣ Feature-based Folder Structure (Non-Negotiable)

❌ Red flag (junior mindset)

/components
/services
/models
/utils

➡️ Files are grouped by type, not by business meaning.

✅ Lead-approved structure

/features
  /orders
    orders.routes.ts
    orders.component.ts
    orders.service.ts
    orders.store.ts
    orders.model.ts
  /payments
    ...

🔍 PR review checklist

❓ Can I delete a feature folder without breaking others?
❓ Does this feature own its UI + logic + state?
❌ Reject PRs adding files to generic /shared/services by default

📌 Rule: Features should be isolated, portable, and cohesive.

2️⃣ Shared vs Core — Most PRs Get This Wrong

🧠 Core Module (App-wide, once)

What belongs here

Auth service
HTTP interceptors
Global error handler
App configuration
Logging

/core
  auth.service.ts
  error-handler.ts
  http.interceptor.ts

🚨 PR Red Flags

CoreModule imported into a lazy module ❌
Stateful services inside SharedModule ❌

🧠 Shared Module (Stateless & reusable)

What belongs here

UI components (buttons, modals)
Pipes
Directives

/shared
  /ui
    button.component.ts
  /pipes

📌 Rule:

Shared = reusable & stateless
Core = singleton & global

3️⃣ Lazy Loading — Architectural Decision, Not Performance Hype

Lazy Loading in Angular — what it REALLY means

Lazy loading in Angular is NOT a performance trick.

It is an architectural decision about feature isolation.

Performance improvement is just a side-effect.

❌ Wrong Angular thinking (very common in PRs)

“I lazy-loaded this module because performance”

🚫 This tells a reviewer:

You don’t understand why this module exists
You’re cargo-culting Angular features

✅ Correct Angular thinking

“This feature is isolated, optional at startup, and has its own navigation boundary — therefore it is lazy-loaded.”

What lazy loading ACTUALLY does in Angular

When you lazy load:

{
  path: 'admin',
  loadChildren: () =>
    import('./admin/admin.routes').then(m => m.ADMIN_ROUTES)
}

Angular:

❌ Does not load admin components
❌ Does not register admin routes
❌ Does not create admin injectors

until the user navigates to /admin

👉 This is isolation, not speed.

When should a feature be lazy loaded? (Angular rules)

1️⃣ Feature is NOT part of the initial route

Ask:

Can the user complete their main task without this feature?

If yes → lazy load

Examples:

Reports
Settings
Admin
History
Analytics

❌ Dashboard, Login, Shell → usually eager

2️⃣ Feature has its OWN routing

If a feature has:

/admin
/admin/users
/admin/roles

That’s a routing boundary → perfect for lazy loading.

If it doesn’t have its own routes → don’t force lazy loading.

3️⃣ Feature has its OWN state

If the feature owns:

Facade
Effects
Signals store
API calls

Lazy loading means:

State is created when entering
State is destroyed when leaving

This is CLEAN.

❌ Eager loading keeps dead state forever.

4️⃣ Feature is role-based

Angular guard example:

{
  path: 'admin',
  canMatch: [adminGuard],
  loadChildren: () => import('./admin/admin.routes')
}

👉 Angular won’t even download the admin bundle if the role fails.

This is security + architecture, not performance.

PR Review Questions — what a lead REALLY checks

❓ Is this reachable from the initial route?

If yes:

Why is it lazy?
Are we delaying critical UX?

❓ Does lazy loading simplify or complicate state?

Good lazy loading:

Feature-local services
No cross-feature injections

Bad lazy loading:

Shared services hacked through CoreModule
Tight coupling

❓ Is preloading used correctly?

Bad:

PreloadAllModules

Good:

data: { preload: true }

👉 Only preload what is important but not initial.

The golden Angular rule (remember this)

Lazy loading is about isolation first, performance second.

If a feature:

Can stand alone
Has its own routes
Has its own state
Is not needed at startup

➡️ Lazy load it.

One-line test (lead-level)

“If I delete this feature folder, will the rest of the app still make sense?”

If yes → lazy load
If no → eager load

❌ Bad PR justification

“I lazy-loaded it because performance”

✅ Lead thinking

Lazy load when:

Feature is not needed on app start
Feature is large
Feature is role-based
Feature has independent navigation

🔍 PR review questions

Is this feature reachable from the initial route?
Does lazy loading simplify or complicate state sharing?
Is preloading strategy used correctly?

📌 Rule: Lazy loading is about isolation first, performance second.

4️⃣ Global Error Handling (No try/catch soup)

❌ Red flags in PR

catchError everywhere
console.log(error)
Silent failures

✅ Lead-approved pattern

Centralized error handling
User-friendly messages
Logged + observable-safe

@Injectable()
export class GlobalErrorHandler implements ErrorHandler {
  handleError(error: unknown) {
    // log
    // notify user
    // fail gracefully
  }
}

🔍 Review checklist

Are HTTP errors normalized?
Are UI errors surfaced properly?
Is business logic polluted with error UI logic?

📌 Rule: Errors are architecture, not implementation details.

5️⃣ Reviewing PRs for Design (This is where leads shine)

🧠 Don’t ask:

“Why didn’t you use map?”
“Why not signals instead of RxJS?”

🧠 DO ask:

❓ Is responsibility clear?
❓ Is state leaking across layers?
❓ Can this scale to 10x complexity?
❓ Is this testable without the UI?
❓ Does this follow existing patterns?

6️⃣ PR Review Mental Model (Lead Lens)

Area	Lead Focus
Components	Dumb UI vs Smart containers
Services	Single responsibility
State	Ownership & lifecycle
Modules	Boundaries respected
Imports	Directional (no cycles)
Naming	Business-driven

📌 Golden Rule:

If a new dev joins, can they understand this feature in 15 minutes?

7️⃣ Common PR Comments You Should Start Writing

“This logic belongs to the feature, not shared.”
“This component is doing too much — consider a container.”
“Why is this service global?”
“This breaks lazy-loading isolation.”
“What owns this state?”

👆 These comments signal senior/lead thinking.

8️⃣ Graduation Test (Self-check)

Reject a PR even if it “works”
Explain why structure matters
Protect boundaries without being dogmatic
Optimize for future developers, not today’s speed

Below are more lead-level PR review tips that most coders never learn 👇

9️⃣ Watch for Hidden Coupling (Silent Architecture Killers)

❌ PR smell

this.router.navigate(['/orders', order.id]);
this.sharedService.selectedOrder = order;

➡️ Feature A silently depends on Feature B’s internal behavior.

✅ Lead thinking

State must be explicit
Navigation should not carry hidden context
Use route params / resolvers / facades

🔍 PR question

“If I remove Feature B, will Feature A still compile?”

📌 Coupling doesn’t show in tests — it shows in production.

🔟 Kill “Convenience Services”

❌ Red flag names

UtilsService
CommonService
HelperService
SharedDataService

🧠Rule

If a service has no clear business name, it’s a design failure.

✅ Replace with

OrderPricingService
PaymentAuthorizationFacade
UserSessionStore

🔍 PR comment

“What business capability does this service represent?”

1️⃣1️⃣ Guard Against Smart Components Drift

❌ PR pattern

ngOnInit() {
  this.route.params.subscribe(...)
  this.service.getData().subscribe(...)
  this.store.update(...)
}

➡️ Component becoming a mini-controller.

✅ Lead correction

Move orchestration to:
- Facade
- Store
- Container component

📌 Rule

Components render state, not decide state.

1️⃣2️⃣ Naming Is Architecture (Not Cosmetics)

❌ Red flag

handleData()
processStuff()
doAction()

✅ Lead-approved

loadOrders()
submitPayment()
confirmDelivery()

🔍 PR check

Method names must reflect business verbs
Avoid technical verbs (handle, process, manage)

📌 If naming is vague, design is vague.

1️⃣3️⃣ Lifecycle Ownership Review

❌ PR smell

ngOnInit() {
  this.interval = setInterval(...)
}

No cleanup.

🧠 Lead questions

Who owns this subscription?
When should it stop?
What happens on navigation?

✅ Expect

takeUntilDestroyed
Signals auto cleanup
Store-driven lifecycles

📌 Memory leaks are leadership failures.

1️⃣4️⃣ Watch for State Duplication

❌ PR smell State calculated in multiple places.

this.items$ = this.store.items$;
this.total = this.items.reduce((a, b) => a + b.price, 0);

❌Derived state lives next to source

@Component(...)
export class CartComponent {
  items$ = this.store.items$;
  total$ = this.items$.pipe(
    map(items => items.reduce((a, b) => a + b.price, 0))
  );
}

Why wrong:
Component now owns business logic
Logic is duplicated if reused
Hard to test

🧠 Lead response

“Where is the single source of truth?”

✅ Correct place: Store / Facade

@Injectable()

export class CartStore {

private itemsSubject = new BehaviorSubject<Item[]>([]);

items$ = this.itemsSubject.asObservable();

total$ = this.items$.pipe(

map(items => items.reduce((a, b) => a + b.price, 0))

);

}

Now:

items = source
total = derived next to it
Components stay dumb

✅ Same rule with Angular Signals

@Injectable()

export class CartStore {

readonly items = signal<Item[]>([]);

readonly total = computed(() =>

this.items().reduce((a, b) => a + b.price, 0)

);

}

✅ Rule

Derived state lives next to source
Never recalc in components
If state is derived in more than one place, your design is already broken.

📌 Duplicated state = guaranteed bugs.

1️⃣5️⃣ Review Imports Like a Hawk 🦅

❌ Red flags

../../../../shared/utils

➡️ Module boundary violation.

Why `../../../../` is a problem (not aesthetics)

This is not about ugly paths.

It means:

The file escaped its own module
Reached into another layer
Bypassed the architecture

That’s a directional dependency violation.

🧠 Lead rule

Imports must flow downward

App

├─ Core

├─ Shared

└─ Features

├─ Orders

├─ Payments

└─ Reports

Features don’t import from other features

Allowed imports ✅

Feature → Shared
Feature → Core
App → everything below

Forbidden imports ❌

Feature → Feature
Shared → Feature
Core → Feature

🔍 PR rejection reason

“This breaks directional dependency.”

Golden rule (memorize this)

If you need ../../../../, you are in the wrong layer.

1️⃣6️⃣ Async Code Smell Radar

❌ PR smell

subscribe(() => {
  subscribe(() => {
    subscribe(() => {})
  })
})

🧠 Lead response

Flatten streams
One subscription per component
Prefer effects / stores

📌 Nested subscriptions = junior panic coding.

1️⃣7️⃣ Flags, Booleans & Explosion Risk

❌ PR smell

isLoading
isSaving
isEditing
isUpdating

🧠 Lead solution

Replace booleans with:

state: 'idle' | 'loading' | 'saving' | 'error'

📌 Boolean hell grows exponentially.

1️⃣8️⃣ Review Tests for Meaning, Not Coverage

❌ Useless test

it('should create component')

🧠 Lead expectation

Tests describe business behavior
Test state transitions
Test edge cases

🔍 PR comment

“What behavior would break if this test fails?”

1️⃣9️⃣ Beware of “Just One More Input()”

❌ PR smell

Component with 8+ @Input()s

@Component({...})

export class OrderCardComponent {

@Input() order!: Order;

@Input() isSelected = false;

@Input() showActions = true;

@Input() canEdit = false;

@Input() canCancel = false;

@Input() highlight = false;

@Input() currency!: string;

@Input() locale!: string;

}

🧠 Lead insight

Too many inputs = leaky abstraction
Signals broken responsibility

✅ Fix

Pass a ViewModel

@Input() vm!: OrderCardVM;

export interface OrderCardVM {

order: Order;

isSelected: boolean;

canEdit: boolean;

canCancel: boolean;

currency: string;

locale: string;

}

Or split component
ask:
- ❓ Can this component be rendered with a single object?
- ❓ Are these inputs flags or real data?
- ❓ Do multiple parents need different combinations?
- ❓ Is this UI or business logic leaking?
- ❓ Would removing one input break unrelated behavior?

📌 If a component needs too much context, it’s too big.

2️⃣0️⃣ Watch How Errors Are Named

❌ PR smell

throw new Error('Something went wrong');

Why this is bad:
❌ No meaning
❌ Cannot be handled differently
❌ UI can only show a generic message
❌ Breaks refactoring safety
❌ Error string becomes a hidden contract


PR review checklist
ask:
❓ Is this error meaningful to the domain?
❓ Can UI react differently to each error?
❓ Is the error name stable over time?
❓ Does this leak backend / HTTP details?
❓ Is this string-based logic hiding a contract?

🧠 Lead expectation

Domain-specific errors
Typed error handling

throw new PaymentDeclinedError();

📌 Errors are part of your API.

🧠 PR Review Checklist

Before approving:

Does this change add a new pattern?
Does it respect existing architecture?
Will this age well in 1 year?
Is the complexity proportional?
Could a junior maintain this safely?

=Review a real Angular route config
Show good vs bad lazy loading PR examples
Explain lazy loading + signals + facades
Explain why some features should NEVER be lazy

Tuesday, January 13, 2026