Implement Login [DRAFT]#8
Open
ElbekDeveloper wants to merge 1 commit intomasterfrom
Open
Conversation
Co-authored-by: Copilot <copilot@github.com>
📝 WalkthroughWalkthroughThis PR implements a comprehensive login UI feature with route guards, session-based authentication, and complete specifications. It adds a LoginComponent with reactive form validation, SessionService for token management, auth guards for route protection, and updates app routing accordingly. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant Router as Router
participant AuthGuard as Auth Guards
participant LoginComponent as Login Component
participant SessionService as Session Service
participant SessionStorage as sessionStorage
User->>Router: Navigate to /login
Router->>AuthGuard: Check authRedirectGuard
alt Has Token
AuthGuard->>Router: Redirect to /chat
AuthGuard-->>User: Block /login access
else No Token
AuthGuard-->>Router: Allow activation
Router->>LoginComponent: Activate component
LoginComponent->>User: Render login form
end
User->>LoginComponent: Submit form (email/password)
LoginComponent->>LoginComponent: Validate form
alt Form Invalid
LoginComponent->>User: Mark fields touched, show errors
else Form Valid
LoginComponent->>LoginComponent: Set isSubmitting=true
LoginComponent->>SessionService: setToken(mock_token_*)
SessionService->>SessionStorage: Store auth_token
LoginComponent->>Router: Navigate to /chat
Router->>AuthGuard: Check authGuard
AuthGuard->>SessionService: hasToken()
SessionService->>SessionStorage: Read auth_token
AuthGuard-->>Router: Allow activation
Router->>User: Activate /chat route
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related Issues
Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (8)
src/app/features/auth/guards/auth.guard.ts (1)
5-15: ⚡ Quick winReturn a
UrlTreefrom the guard instead of callingnavigate().On Line 13, side-effect navigation inside a guard can cause extra navigation churn. Prefer returning a redirect
UrlTreedirectly.Proposed refactor
export const authGuard: CanActivateFn = () => { const sessionService = inject(SessionService); const router = inject(Router); - if (sessionService.hasToken()) { - return true; - } - - router.navigate(['/login']); - return false; + return sessionService.hasToken() ? true : router.createUrlTree(['/login']); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/features/auth/guards/auth.guard.ts` around lines 5 - 15, The guard authGuard currently calls router.navigate(['/login']) as a side effect; change it to return a redirect UrlTree instead: if SessionService.hasToken() return true, otherwise return router.createUrlTree(['/login']); update the return paths in authGuard so it returns boolean | UrlTree and remove the navigate() call to avoid extra navigation churn.src/app/features/auth/guards/auth-redirect.guard.ts (1)
5-15: ⚡ Quick winUse
UrlTreeredirect instead of imperative navigation in guard.At Line 10, prefer returning
router.createUrlTree(['/chat'])to keep guard behavior declarative and avoid side-effect navigation.Proposed refactor
export const authRedirectGuard: CanActivateFn = () => { const sessionService = inject(SessionService); - const router = inject(Router); + const router = inject(Router); if (sessionService.hasToken()) { - router.navigate(['/chat']); - return false; + return router.createUrlTree(['/chat']); } return true; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/features/auth/guards/auth-redirect.guard.ts` around lines 5 - 15, The guard authRedirectGuard currently calls router.navigate and returns false when SessionService.hasToken() is true; change it to return router.createUrlTree(['/chat']) instead of performing imperative navigation so the guard returns a UrlTree declaratively; update the branch that checks sessionService.hasToken() to return the UrlTree and keep the final return true for allowed access, referencing authRedirectGuard, SessionService.hasToken, and Router.createUrlTree.src/app/features/auth/services/session.service.spec.ts (1)
11-12: ⚡ Quick winPrefer key-scoped cleanup instead of
sessionStorage.clear().Using
clear()removes unrelated keys and can create hidden coupling with other tests. Remove onlyauth_tokenfor tighter test isolation.Also applies to: 15-17
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/features/auth/services/session.service.spec.ts` around lines 11 - 12, Tests call sessionStorage.clear() which removes unrelated keys and couples tests; replace uses of sessionStorage.clear() (e.g., in afterEach / beforeEach blocks) with sessionStorage.removeItem('auth_token') so only the auth_token key is cleaned up; update all occurrences referenced (the calls around sessionStorage.clear() at the start and the later block) to remove only 'auth_token' to ensure tighter test isolation.src/app/features/auth/login/login.component.ts (1)
57-62: ⚡ Quick winAlign DI style with project lint rule (
prefer-inject).Current constructor parameter injection is flagged by lint in this repo configuration. Migrating to
inject()will clear the rule and keep style consistent.Angular-style DI migration sketch
-import { Component, OnInit, ChangeDetectionStrategy, signal } from '@angular/core'; +import { Component, OnInit, ChangeDetectionStrategy, signal, inject } from '@angular/core'; @@ - constructor( - private fb: FormBuilder, - private sessionService: SessionService, - private router: Router, - private title: Title, - ) { + private fb = inject(FormBuilder); + private sessionService = inject(SessionService); + private router = inject(Router); + private title = inject(Title); + + constructor() { this.initializeForm(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/features/auth/login/login.component.ts` around lines 57 - 62, The constructor-based DI in LoginComponent should be converted to use Angular's inject() to satisfy the prefer-inject lint rule: remove the constructor and instead declare class properties using inject() for FormBuilder, SessionService, Router and Title (e.g. a private fb = inject(FormBuilder), sessionService = inject(SessionService), router = inject(Router), title = inject(Title)); also add import { inject } from '@angular/core' if missing and adjust any references to the former constructor properties to the new class properties.src/app/features/auth/login/login.component.spec.ts (2)
243-256: ⚡ Quick winAssert
isSubmittingreturns to false after async completionThis test currently checks only the transient
truestate. Add a final assertion aftertick(1500)to verify the loading state clears deterministically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/features/auth/login/login.component.spec.ts` around lines 243 - 256, The test currently asserts component.isSubmitting() becomes true but doesn't verify it returns to false after the async work; update the spec for the onSubmit() flow (in the test using component.onSubmit(), router.navigate mock and tick(1500)) to add a final assertion after tick(1500) that component.isSubmitting() is false, ensuring the loading state clears after the async completion.
166-176: ⚡ Quick winAdd DOM assertions for spinner and disabled button during submission
For the sign-in UX path, assert rendered behavior (spinner visible + button non-interactive) in addition to signal state to lock FR-003 at template level.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/features/auth/login/login.component.spec.ts` around lines 166 - 176, The test currently only asserts the submission signal state; extend the spec to assert rendered UX by querying the template after calling component.onSubmit(): call fixture.detectChanges() immediately after onSubmit(), verify the spinner element (e.g., `.spinner` or `mat-progress-spinner`) is present/visible and the submit button (e.g., `button[type="submit"]` or `.submit-button`) is disabled or has the non-interactive class while component.isSubmitting() is true, then use tick(1500) + fixture.detectChanges() and assert the spinner is gone and the button is enabled again; reference component.onSubmit(), component.isSubmitting(), and the template spinner/submit button selectors to locate where to add these DOM assertions.specs/003-login-ui/spec.md (1)
74-75: ⚡ Quick winClarify FR-006 to target protected routes only
Current wording (“all routes other than
/login”) is over-broad and can block future public pages. Reword to “all protected routes” to avoid unintended routing constraints.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/003-login-ui/spec.md` around lines 74 - 75, Update FR-006 to narrow its scope: change the phrase "all routes other than `/login`" to "all protected routes" and ensure the spec clarifies what constitutes a protected route (e.g., routes requiring an authenticated session); keep FR-007 as-is referencing `/login` and `/chat`, and ensure the "route guard" behavior is consistently described in relation to the new "protected routes" definition.specs/003-login-ui/tasks.md (1)
102-105: ⚡ Quick winResolve eager-vs-lazy routing contradiction in T010
component: LoginComponentis eager route config, but the task also requires lazy loading. Pick one explicit approach (preferloadComponentif lazy-loading is intended).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/003-login-ui/tasks.md` around lines 102 - 105, T010 currently mixes eager and lazy routing by specifying component: LoginComponent while requiring lazy loading; update the route for path 'login' to use the lazy syntax (prefer loadComponent) and remove the component property—e.g., replace the route object that references LoginComponent with { path: 'login', loadComponent: () => import('...').then(m => m.LoginComponent) } so the LoginComponent is dynamically imported (or alternatively make it fully eager by keeping component: LoginComponent and removing any loadComponent usage); ensure the exported symbol LoginComponent is referenced in the dynamic import.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@specs/003-login-ui/contracts/auth-guard.md`:
- Around line 23-27: The contract currently checks token truthiness using
sessionToken (if (sessionToken)) but the actual guard uses key presence via
hasToken() which treats an empty string as present; update the pseudocode to
match the guard semantics by either calling hasToken() directly (to mirror the
guard) or by checking presence with sessionStorage.getItem('auth_token') !==
null (so "" is considered present); apply the same change to the second
occurrence referenced at lines 44-48 to keep both contract snippets consistent
with hasToken().
In `@specs/003-login-ui/data-model.md`:
- Around line 148-150: Update the Relationship section to reference the actual
method used by the guard: replace any mention of SessionService.getToken() with
SessionService.hasToken() so the model reflects the implemented guard behavior
(e.g., where it currently lists "Calls: SessionService.getToken()", change to
"Calls: SessionService.hasToken()").
- Around line 26-28: Multiple fenced code blocks in the data model doc lack a
language tag (e.g. the state diagram block "[Not Present]
--sign-in-successful--> [Present in sessionStorage] ..."); update each of the
mentioned fenced blocks by adding an appropriate fence language (use mermaid for
diagrams/state charts, text for plain ascii/state lists, and typescript for any
code snippets) for the blocks at the ranges called out (around the shown state
diagram and the other ranges 54-56, 84-90, 110-117, 123-153, 170-208) so
markdownlint MD040 is satisfied.
In `@specs/003-login-ui/plan.md`:
- Line 3: Update the broken spec link and contract filename references: replace
the path-prefixed spec link "specs/003-login-ui/spec.md" with the correct
relative link "./spec.md" (or the repo-correct relative path to the spec file)
and change the contract reference "login-guard.md" to the current filename
"auth-guard.md" so the plan.md correctly links to the spec and matching contract
document.
In `@specs/003-login-ui/quickstart.md`:
- Around line 44-47: Update the "Route Protection" quickstart to match the
actual implementation: describe that two separate guards are used (authGuard and
authRedirectGuard) rather than a single guard, show routes using loadComponent
for lazy component loading, and change any example state property names to
isSubmitting (not isSubmitting$); ensure the redirect behavior is described for
both guards (authGuard protects /chat and redirects to /login when no session
token, authRedirectGuard prevents authenticated users from accessing /login and
redirects them to /chat).
- Around line 53-76: The markdown code fence in the quickstart architecture
snippet lacks a language tag, causing MD040; update the opening triple-backtick
for the block that shows the src/app tree (the block starting with ``` and
containing "src/app/ ├── features/") to include a language identifier such as
text (e.g., change ``` to ```text) so markdownlint passes and the tree renders
correctly.
In `@specs/003-login-ui/research.md`:
- Around line 230-240: The example uses aria-invalid directly on
<app-text-input> and an invalid inline `@if` block; instead update the usage to
pass the error text via the TextInputComponent's errorMessage input (e.g.,
<app-text-input [errorMessage]="...">) and move the error alert outside the
component as a sibling element that uses role="alert" and the same id referenced
by aria-describedby (the component will set aria-invalid/aria-describedby
internally when errorMessage is provided). Locate examples referencing
<app-text-input>, replace direct aria bindings with errorMessage, and render the
conditional alert as a separate sibling element.
- Around line 193-203: Replace the unsupported functional-style LoginComponent
declaration using component({...}) with a standard Angular `@Component` class: add
an `@Component`({ selector: 'app-login', template: `...` }) decorator and export a
class LoginComponent that defines the input property redirectUrl with `@Input`()
redirectUrl: string = ''; and the output signIn as `@Output`() signIn = new
EventEmitter<{ email: string; password: string }>();; update any references to
the previous inputs/outputs accordingly so selector, template, redirectUrl and
signIn symbols remain consistent.
In `@specs/003-login-ui/tasks.md`:
- Around line 299-309: The fenced code block containing the phase diagram (the
triple-backtick block that starts with "Phase 1: Setup" and includes "Phase 2:
Foundational (SessionService) ← BLOCKING" etc.) is missing a language
identifier; update the opening fence from ``` to ```text (or ```markdown) so the
block has a language tag and satisfies MD040 linting. Ensure only the opening
fence is changed and the block content remains identical.
- Around line 187-198: The task should instruct updating the login template
(login.component.html) to stop binding `[aria-invalid]` on the app-text-input
and instead pass the field-specific error via the TextInputComponent's input
prop `errorMessage`; for the `app-text-input` instances for `email` and
`password` set `[errorMessage]="(email.invalid && email.touched) ? 'Email is
required' : ''"` and `[errorMessage]="(password.invalid && password.touched) ?
'Password is required' : ''"` respectively (remove the explicit `[aria-invalid]`
bindings), and also add the conditional alert markup only if you still need
visible error elements; rely on TextInputComponent to manage
aria-invalid/aria-describedby when `errorMessage` is provided.
In `@src/app/features/auth/login/login.component.ts`:
- Around line 110-128: The submit handler currently sets
this.isSubmitting.set(true) but only clears it in the catch block, so successful
paths (including router.navigate returning false) can leave the form disabled;
wrap the try/catch in a try/finally or add a finally block that always calls
this.isSubmitting.set(false), or explicitly check the result of await
this.router.navigate(['/chat']) and call this.isSubmitting.set(false) on
failure—update the method containing simulateSignIn, sessionService.setToken,
this.form.reset and router.navigate to ensure this.isSubmitting.set(false) is
executed for all exit paths.
In `@src/app/features/auth/services/session.service.spec.ts`:
- Around line 60-63: The test currently assumes an empty string token is
authenticated; update the test expectation to expect false and modify
SessionService.hasToken() to return true only when the stored 'auth_token' value
is a non-empty, non-whitespace string (e.g., check that
sessionStorage.getItem('auth_token') is not null and .trim().length > 0). Ensure
hasToken() handles null/undefined safely and the spec in session.service.spec.ts
asserts false for '' so guards won't treat empty values as valid tokens.
In `@src/app/features/auth/services/session.service.ts`:
- Around line 20-22: The current token handling treats empty strings/whitespace
as valid sessions; update the session logic so tokens that are '' or only
whitespace are treated as unauthenticated. In the session service (methods
referencing TOKEN_KEY such as setToken, getToken and any
isAuthenticated/getStoredToken logic) trim the retrieved value and consider it
invalid if null/undefined or trimmed length === 0, returning null or false for
unauthenticated checks and not unlocking guarded routes. Ensure setToken still
writes the raw token but downstream retrieval/validation uses the
trimmed-nonempty rule.
---
Nitpick comments:
In `@specs/003-login-ui/spec.md`:
- Around line 74-75: Update FR-006 to narrow its scope: change the phrase "all
routes other than `/login`" to "all protected routes" and ensure the spec
clarifies what constitutes a protected route (e.g., routes requiring an
authenticated session); keep FR-007 as-is referencing `/login` and `/chat`, and
ensure the "route guard" behavior is consistently described in relation to the
new "protected routes" definition.
In `@specs/003-login-ui/tasks.md`:
- Around line 102-105: T010 currently mixes eager and lazy routing by specifying
component: LoginComponent while requiring lazy loading; update the route for
path 'login' to use the lazy syntax (prefer loadComponent) and remove the
component property—e.g., replace the route object that references LoginComponent
with { path: 'login', loadComponent: () => import('...').then(m =>
m.LoginComponent) } so the LoginComponent is dynamically imported (or
alternatively make it fully eager by keeping component: LoginComponent and
removing any loadComponent usage); ensure the exported symbol LoginComponent is
referenced in the dynamic import.
In `@src/app/features/auth/guards/auth-redirect.guard.ts`:
- Around line 5-15: The guard authRedirectGuard currently calls router.navigate
and returns false when SessionService.hasToken() is true; change it to return
router.createUrlTree(['/chat']) instead of performing imperative navigation so
the guard returns a UrlTree declaratively; update the branch that checks
sessionService.hasToken() to return the UrlTree and keep the final return true
for allowed access, referencing authRedirectGuard, SessionService.hasToken, and
Router.createUrlTree.
In `@src/app/features/auth/guards/auth.guard.ts`:
- Around line 5-15: The guard authGuard currently calls
router.navigate(['/login']) as a side effect; change it to return a redirect
UrlTree instead: if SessionService.hasToken() return true, otherwise return
router.createUrlTree(['/login']); update the return paths in authGuard so it
returns boolean | UrlTree and remove the navigate() call to avoid extra
navigation churn.
In `@src/app/features/auth/login/login.component.spec.ts`:
- Around line 243-256: The test currently asserts component.isSubmitting()
becomes true but doesn't verify it returns to false after the async work; update
the spec for the onSubmit() flow (in the test using component.onSubmit(),
router.navigate mock and tick(1500)) to add a final assertion after tick(1500)
that component.isSubmitting() is false, ensuring the loading state clears after
the async completion.
- Around line 166-176: The test currently only asserts the submission signal
state; extend the spec to assert rendered UX by querying the template after
calling component.onSubmit(): call fixture.detectChanges() immediately after
onSubmit(), verify the spinner element (e.g., `.spinner` or
`mat-progress-spinner`) is present/visible and the submit button (e.g.,
`button[type="submit"]` or `.submit-button`) is disabled or has the
non-interactive class while component.isSubmitting() is true, then use
tick(1500) + fixture.detectChanges() and assert the spinner is gone and the
button is enabled again; reference component.onSubmit(),
component.isSubmitting(), and the template spinner/submit button selectors to
locate where to add these DOM assertions.
In `@src/app/features/auth/login/login.component.ts`:
- Around line 57-62: The constructor-based DI in LoginComponent should be
converted to use Angular's inject() to satisfy the prefer-inject lint rule:
remove the constructor and instead declare class properties using inject() for
FormBuilder, SessionService, Router and Title (e.g. a private fb =
inject(FormBuilder), sessionService = inject(SessionService), router =
inject(Router), title = inject(Title)); also add import { inject } from
'@angular/core' if missing and adjust any references to the former constructor
properties to the new class properties.
In `@src/app/features/auth/services/session.service.spec.ts`:
- Around line 11-12: Tests call sessionStorage.clear() which removes unrelated
keys and couples tests; replace uses of sessionStorage.clear() (e.g., in
afterEach / beforeEach blocks) with sessionStorage.removeItem('auth_token') so
only the auth_token key is cleaned up; update all occurrences referenced (the
calls around sessionStorage.clear() at the start and the later block) to remove
only 'auth_token' to ensure tighter test isolation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9bfcbe76-17a9-494a-8d49-f28258ef2e1c
📒 Files selected for processing (21)
.specify/feature.json.vscode/settings.jsonCLAUDE.mdspecs/003-login-ui/checklists/requirements.mdspecs/003-login-ui/contracts/auth-guard.mdspecs/003-login-ui/data-model.mdspecs/003-login-ui/plan.mdspecs/003-login-ui/quickstart.mdspecs/003-login-ui/research.mdspecs/003-login-ui/spec.mdspecs/003-login-ui/tasks.mdsrc/app/app.routes.tssrc/app/features/auth/guards/auth-redirect.guard.tssrc/app/features/auth/guards/auth.guard.tssrc/app/features/auth/index.tssrc/app/features/auth/login/login.component.htmlsrc/app/features/auth/login/login.component.scsssrc/app/features/auth/login/login.component.spec.tssrc/app/features/auth/login/login.component.tssrc/app/features/auth/services/session.service.spec.tssrc/app/features/auth/services/session.service.ts
Comment on lines
+23
to
+27
| const sessionToken = sessionStorage.getItem('auth_token'); | ||
|
|
||
| // If token exists, user is authenticated | ||
| if (sessionToken) { | ||
| return true; // Allow navigation to protected route |
There was a problem hiding this comment.
Align token-check semantics with the actual guard implementation.
The contract pseudocode uses truthiness (if (sessionToken)), but current guard behavior is based on key presence (hasToken()), which treats "" as present. Please make the contract explicit and consistent to avoid conflicting test expectations.
Also applies to: 44-48
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/003-login-ui/contracts/auth-guard.md` around lines 23 - 27, The
contract currently checks token truthiness using sessionToken (if
(sessionToken)) but the actual guard uses key presence via hasToken() which
treats an empty string as present; update the pseudocode to match the guard
semantics by either calling hasToken() directly (to mirror the guard) or by
checking presence with sessionStorage.getItem('auth_token') !== null (so "" is
considered present); apply the same change to the second occurrence referenced
at lines 44-48 to keep both contract snippets consistent with hasToken().
Comment on lines
+26
to
+28
| ``` | ||
| [Not Present] --sign-in-successful--> [Present in sessionStorage] --tab-close-or-logout--> [Not Present] | ||
| ``` |
There was a problem hiding this comment.
Add fence languages to satisfy markdownlint (MD040).
Several fenced blocks are missing a language tag; adding text/mermaid/typescript as appropriate will clear lint noise and keep docs CI clean.
Also applies to: 54-56, 84-90, 110-117, 123-153, 170-208
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 26-26: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/003-login-ui/data-model.md` around lines 26 - 28, Multiple fenced code
blocks in the data model doc lack a language tag (e.g. the state diagram block
"[Not Present] --sign-in-successful--> [Present in sessionStorage] ..."); update
each of the mentioned fenced blocks by adding an appropriate fence language (use
mermaid for diagrams/state charts, text for plain ascii/state lists, and
typescript for any code snippets) for the blocks at the ranges called out
(around the shown state diagram and the other ranges 54-56, 84-90, 110-117,
123-153, 170-208) so markdownlint MD040 is satisfied.
Comment on lines
+148
to
+150
| ├─ Calls: SessionService.getToken() | ||
| │ | ||
| ├─ Returns: boolean (canActivate) |
There was a problem hiding this comment.
Relationship section references the wrong SessionService method.
This section says guards call SessionService.getToken(), while the guard implementation uses SessionService.hasToken(). Please update the model to match actual behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/003-login-ui/data-model.md` around lines 148 - 150, Update the
Relationship section to reference the actual method used by the guard: replace
any mention of SessionService.getToken() with SessionService.hasToken() so the
model reflects the implemented guard behavior (e.g., where it currently lists
"Calls: SessionService.getToken()", change to "Calls:
SessionService.hasToken()").
| @@ -0,0 +1,100 @@ | |||
| # Implementation Plan: Login UI | |||
|
|
|||
| **Branch**: `003-login-ui` | **Date**: 2026-04-30 | **Spec**: [specs/003-login-ui/spec.md](specs/003-login-ui/spec.md) | |||
There was a problem hiding this comment.
Fix stale/broken spec references.
The spec link on Line 3 is path-prefixed in a way that likely resolves incorrectly from this file, and the contract filename on Line 50 (login-guard.md) doesn’t match the current auth-guard.md. Updating both avoids dead links and spec drift.
Also applies to: 50-50
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/003-login-ui/plan.md` at line 3, Update the broken spec link and
contract filename references: replace the path-prefixed spec link
"specs/003-login-ui/spec.md" with the correct relative link "./spec.md" (or the
repo-correct relative path to the spec file) and change the contract reference
"login-guard.md" to the current filename "auth-guard.md" so the plan.md
correctly links to the spec and matching contract document.
Comment on lines
+44
to
+47
| ### 5. **Route Protection**: Single Auth Guard | ||
| - Check for session token in guard logic | ||
| - Redirect unauthenticated users to `/login` | ||
| - Redirect authenticated users away from `/login` to `/chat` |
There was a problem hiding this comment.
Quickstart is out of sync with the implemented routing/guard model.
This section describes a single guard and component-based routes, but implementation uses two guards (authGuard + authRedirectGuard) and loadComponent. Also, state naming should match isSubmitting (not isSubmitting$).
Also applies to: 91-91, 113-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/003-login-ui/quickstart.md` around lines 44 - 47, Update the "Route
Protection" quickstart to match the actual implementation: describe that two
separate guards are used (authGuard and authRedirectGuard) rather than a single
guard, show routes using loadComponent for lazy component loading, and change
any example state property names to isSubmitting (not isSubmitting$); ensure the
redirect behavior is described for both guards (authGuard protects /chat and
redirects to /login when no session token, authRedirectGuard prevents
authenticated users from accessing /login and redirects them to /chat).
Comment on lines
+187
to
+198
| - [ ] T018 [US3] Update login component template `src/app/features/auth/login/login.component.html` to add validation error display: | ||
| - Add conditional error message for email field using `@if`: | ||
| - Show when: `email.invalid && email.touched` | ||
| - Message: "Email is required" | ||
| - Markup: `<div role="alert" class="error-message">{message}</div>` | ||
| - Add conditional error message for password field using `@if`: | ||
| - Show when: `password.invalid && password.touched` | ||
| - Message: "Password is required" | ||
| - Markup: `<div role="alert" class="error-message">{message}</div>` | ||
| - Add `[aria-invalid]="email.invalid && email.touched"` binding to email TextInput | ||
| - Add `[aria-invalid]="password.invalid && password.touched"` binding to password TextInput | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify TextInput public API and accessibility wiring.
rg -n "readonly\\s+\\w+\\s*=\\s*input" src/app/shared/ui/text-input/text-input.component.ts
rg -n "errorMessage|aria-invalid|aria-describedby" src/app/shared/ui/text-input/text-input.component.ts src/app/shared/ui/text-input/text-input.component.htmlRepository: ElbekDeveloper/lexico
Length of output: 821
T018 should use errorMessage binding, not [aria-invalid] on app-text-input
This task currently directs an API usage that does not match the shared input component's contract. The TextInputComponent exposes an errorMessage input property; the component internally handles aria-invalid and aria-describedby attributes based on whether an error message is present. Update the task to pass the field-specific error message instead of binding [aria-invalid] directly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/003-login-ui/tasks.md` around lines 187 - 198, The task should instruct
updating the login template (login.component.html) to stop binding
`[aria-invalid]` on the app-text-input and instead pass the field-specific error
via the TextInputComponent's input prop `errorMessage`; for the `app-text-input`
instances for `email` and `password` set `[errorMessage]="(email.invalid &&
email.touched) ? 'Email is required' : ''"` and
`[errorMessage]="(password.invalid && password.touched) ? 'Password is required'
: ''"` respectively (remove the explicit `[aria-invalid]` bindings), and also
add the conditional alert markup only if you still need visible error elements;
rely on TextInputComponent to manage aria-invalid/aria-describedby when
`errorMessage` is provided.
Comment on lines
+299
to
+309
| ``` | ||
| Phase 1: Setup | ||
| ↓ | ||
| Phase 2: Foundational (SessionService) ← BLOCKING | ||
| ↓ | ||
| Phase 3: User Story 1 (Sign In) ← depends on SessionService | ||
| ├─ Phase 4: User Story 2 (Route Guard) ← can run in parallel with US1 (different files) | ||
| ├─ Phase 5: User Story 3 (Validation) ← depends on US1 component existing | ||
| ↓ | ||
| Phase 6: Polish & Cross-Cutting Concerns | ||
| ``` |
There was a problem hiding this comment.
Add a language identifier to the fenced code block
This block is missing a fence language (MD040), which can fail markdown lint checks.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 299-299: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/003-login-ui/tasks.md` around lines 299 - 309, The fenced code block
containing the phase diagram (the triple-backtick block that starts with "Phase
1: Setup" and includes "Phase 2: Foundational (SessionService) ← BLOCKING" etc.)
is missing a language identifier; update the opening fence from ``` to ```text
(or ```markdown) so the block has a language tag and satisfies MD040 linting.
Ensure only the opening fence is changed and the block content remains
identical.
Comment on lines
+110
to
+128
| this.isSubmitting.set(true); | ||
|
|
||
| try { | ||
| // Simulate async authentication with delay | ||
| await this.simulateSignIn(); | ||
|
|
||
| // Store token in session storage | ||
| const mockToken = `mock_token_${Date.now()}`; | ||
| this.sessionService.setToken(mockToken); | ||
|
|
||
| // Reset form after successful submission | ||
| this.form.reset(); | ||
|
|
||
| // Redirect to protected page | ||
| await this.router.navigate(['/chat']); | ||
| } catch (error) { | ||
| console.error('Sign-in failed:', error); | ||
| this.isSubmitting.set(false); | ||
| } |
There was a problem hiding this comment.
Finalize isSubmitting on all submit outcomes.
On the success path, isSubmitting is never reset. If navigation returns false without throwing, the form can stay disabled indefinitely. Move reset into a finally (or handle navigate result explicitly).
Suggested control-flow fix
async onSubmit(): Promise<void> {
@@
this.isSubmitting.set(true);
try {
@@
- await this.router.navigate(['/chat']);
+ await this.router.navigate(['/chat']);
} catch (error) {
console.error('Sign-in failed:', error);
- this.isSubmitting.set(false);
+ } finally {
+ this.isSubmitting.set(false);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| this.isSubmitting.set(true); | |
| try { | |
| // Simulate async authentication with delay | |
| await this.simulateSignIn(); | |
| // Store token in session storage | |
| const mockToken = `mock_token_${Date.now()}`; | |
| this.sessionService.setToken(mockToken); | |
| // Reset form after successful submission | |
| this.form.reset(); | |
| // Redirect to protected page | |
| await this.router.navigate(['/chat']); | |
| } catch (error) { | |
| console.error('Sign-in failed:', error); | |
| this.isSubmitting.set(false); | |
| } | |
| this.isSubmitting.set(true); | |
| try { | |
| // Simulate async authentication with delay | |
| await this.simulateSignIn(); | |
| // Store token in session storage | |
| const mockToken = `mock_token_${Date.now()}`; | |
| this.sessionService.setToken(mockToken); | |
| // Reset form after successful submission | |
| this.form.reset(); | |
| // Redirect to protected page | |
| await this.router.navigate(['/chat']); | |
| } catch (error) { | |
| console.error('Sign-in failed:', error); | |
| } finally { | |
| this.isSubmitting.set(false); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/features/auth/login/login.component.ts` around lines 110 - 128, The
submit handler currently sets this.isSubmitting.set(true) but only clears it in
the catch block, so successful paths (including router.navigate returning false)
can leave the form disabled; wrap the try/catch in a try/finally or add a
finally block that always calls this.isSubmitting.set(false), or explicitly
check the result of await this.router.navigate(['/chat']) and call
this.isSubmitting.set(false) on failure—update the method containing
simulateSignIn, sessionService.setToken, this.form.reset and router.navigate to
ensure this.isSubmitting.set(false) is executed for all exit paths.
Comment on lines
+60
to
+63
| it('should return true even if token is empty string', () => { | ||
| sessionStorage.setItem('auth_token', ''); | ||
| expect(service.hasToken()).toBe(true); | ||
| }); |
There was a problem hiding this comment.
Don’t treat empty token as authenticated.
This test codifies a weak auth invariant ("" => authenticated). That can allow guard bypass when the key exists but has no usable value. Update expectation to false and align SessionService.hasToken() accordingly.
Suggested test + service alignment
- it('should return true even if token is empty string', () => {
+ it('should return false when token is empty string', () => {
sessionStorage.setItem('auth_token', '');
- expect(service.hasToken()).toBe(true);
+ expect(service.hasToken()).toBe(false);
});// src/app/features/auth/services/session.service.ts
hasToken(): boolean {
- return sessionStorage.getItem(this.TOKEN_KEY) !== null;
+ const token = sessionStorage.getItem(this.TOKEN_KEY);
+ return token !== null && token.trim().length > 0;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('should return true even if token is empty string', () => { | |
| sessionStorage.setItem('auth_token', ''); | |
| expect(service.hasToken()).toBe(true); | |
| }); | |
| it('should return false when token is empty string', () => { | |
| sessionStorage.setItem('auth_token', ''); | |
| expect(service.hasToken()).toBe(false); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/features/auth/services/session.service.spec.ts` around lines 60 - 63,
The test currently assumes an empty string token is authenticated; update the
test expectation to expect false and modify SessionService.hasToken() to return
true only when the stored 'auth_token' value is a non-empty, non-whitespace
string (e.g., check that sessionStorage.getItem('auth_token') is not null and
.trim().length > 0). Ensure hasToken() handles null/undefined safely and the
spec in session.service.spec.ts asserts false for '' so guards won't treat empty
values as valid tokens.
Comment on lines
+20
to
+22
| setToken(token: string): void { | ||
| sessionStorage.setItem(this.TOKEN_KEY, token); | ||
| } |
There was a problem hiding this comment.
Treat empty tokens as unauthenticated.
Line 37 only checks for null, so '' (or whitespace) is considered a valid session and can incorrectly unlock guarded routes.
Proposed fix
setToken(token: string): void {
- sessionStorage.setItem(this.TOKEN_KEY, token);
+ const normalized = token.trim();
+ if (normalized.length === 0) {
+ sessionStorage.removeItem(this.TOKEN_KEY);
+ return;
+ }
+ sessionStorage.setItem(this.TOKEN_KEY, normalized);
}
@@
hasToken(): boolean {
- return sessionStorage.getItem(this.TOKEN_KEY) !== null;
+ const token = sessionStorage.getItem(this.TOKEN_KEY);
+ return token !== null && token.trim().length > 0;
}Also applies to: 36-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/features/auth/services/session.service.ts` around lines 20 - 22, The
current token handling treats empty strings/whitespace as valid sessions; update
the session logic so tokens that are '' or only whitespace are treated as
unauthenticated. In the session service (methods referencing TOKEN_KEY such as
setToken, getToken and any isAuthenticated/getStoredToken logic) trim the
retrieved value and consider it invalid if null/undefined or trimmed length ===
0, returning null or false for unauthenticated checks and not unlocking guarded
routes. Ensure setToken still writes the raw token but downstream
retrieval/validation uses the trimmed-nonempty rule.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
It looks bad
Summary by CodeRabbit
Release Notes
New Features
Documentation