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/servicesby 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
CoreModuleimported 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
CoreModuleTight 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
catchErroreverywhereconsole.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
UtilsServiceCommonServiceHelperServiceSharedDataService
🧠Rule
If a service has no clear business name, it’s a design failure.
✅ Replace with
OrderPricingServicePaymentAuthorizationFacadeUserSessionStore
🔍 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
takeUntilDestroyedSignals 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
✅ 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
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
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 configShow good vs bad lazy loading PR examples
Explain lazy loading + signals + facades
Explain why some features should NEVER be lazy
