PR #2400 — Code Review Issues¶
Temporary planning document — delete after PR #2400 is merged.
Issues identified during automated code review of the impersonation UX improvements. All scored 50-75 confidence (below the 80 threshold for auto-commenting on the PR).
Issue 1: effect() writes to signal — Refactored to resource()¶
Type: Improvement
Status: ✅ Resolved
File: src/services/web/src/app/admin/impersonation/impersonation.component.ts
Original concern: The indexEffect wrote to a signal inside effect(). While this is actually allowed by default in Angular 19+ (allowSignalWrites was deprecated), the pattern is not idiomatic for async derived state.
Resolution: Replaced the effect() + signal() combo with Angular 21's resource() API, which is the idiomatic primitive for async derived state. The resource automatically rebuilds the lunr index when allInvestigators() changes, with no manual signal writes needed.
Research findings: allowSignalWrites was deprecated in Angular 19 and is a no-op in Angular 21. NG0600 only applies to computed(), not effect(). However, the Angular team recommends resource() for async operations and computed()/linkedSignal() for synchronous derived state — effect() should be reserved for side effects against non-signal APIs.
Issue 2: Audit logging — replaced email with name¶
Type: Design concern
Status: ✅ Resolved
File: src/libs/webhostconfig/SyRF.WebHostConfig.Common/Infrastructure/UserImpersonationMiddleware.cs
Resolution: Replaced {ImpersonatedEmail} with {ImpersonatedName} (the fullname FullName object) in the structured log message. This provides a human-readable identifier for quick log scanning while removing raw PII (email). The user ID remains in the log for precise lookup.
Note: Per-request logging volume was left as-is for now. The log still fires on every request carrying impersonation headers. A future improvement could track session state and log only on start/stop, but the name-only change addresses the primary GDPR concern.
Issue 3: Inline styles extracted to .component.scss files¶
Type: CLAUDE.md violation
Status: ✅ Resolved
Files:
- confirm-impersonation-dialog.component.scss (new)
- impersonation-banner.component.scss (new)
Resolution: Extracted inline styles: [...] blocks to separate .component.scss files. Updated @Component decorators to use styleUrl (singular, for standalone components). The banner SCSS also uses nesting (&:hover, mat-icon) which is a natural improvement over the flat CSS original.
Issue 4: Planning doc — kept, PR description already accurate¶
Type: CLAUDE.md compliance Status: ⏭️ Skipped
Decision: The planning doc (docs/planning/improve-impersonation-ux.md) is kept. The PR title (feat(web,api): improve user impersonation UX) and description accurately reflect the work. The PR body should be updated to reflect:
- Audit logging now logs name instead of email
- Lunr index uses resource() instead of effect() + signal()
- Dialog has deduplication guard
Issue 5: Dialog deduplication guard added¶
Type: Bug (edge case)
Status: ✅ Resolved
File: src/services/web/src/app/admin/impersonation/impersonation.component.ts
Resolution: Added id: 'confirm-impersonation' to the dialog config and a getDialogById() guard before opening. This uses Angular Material's built-in deduplication API — no boolean flags or manual cleanup needed.
Follow-up: Created docs/planning/unguarded-mat-dialog-usages.md documenting 6 other call sites across the app that could benefit from the same pattern in a future PR.
Issue 6: mkdocs.yml nav sections — false positive¶
Type: Possible regression Status: ⏭️ Skipped (monitor CI)
Assessment unchanged: The nav generator correctly removes references to symlinked directories that don't exist in the worktree. CI runs from the main worktree where symlinks exist. No action needed unless CI fails.
Issue 7: Unused imports removed¶
Type: Code quality
Status: ✅ Resolved
File: src/services/web/src/app/admin/impersonation/impersonation.component.spec.ts
Resolution: Removed fakeAsync, tick, and MatDialogRef imports. Added getDialogById to the dialog spy type to match the new guard.
Summary¶
| # | Issue | Resolution | Status |
|---|---|---|---|
| 1 | Signal writes in effect | Refactored to resource() |
✅ |
| 2 | PII in audit logs | Log name instead of email | ✅ |
| 3 | Inline styles | Extracted to .component.scss |
✅ |
| 4 | Planning doc | Kept; update PR description | ⏭️ |
| 5 | Dialog deduplication | id + getDialogById() guard |
✅ |
| 6 | mkdocs nav | False positive, monitor CI | ⏭️ |
| 7 | Unused imports | Removed | ✅ |