Repository navigation
Auth0 is removed - #117
Auth0 is removed#117
Conversation
WalkthroughThis PR transitions the application from Auth0/OIDC authentication to a custom token-based system with dedicated API endpoints, introduces M2M client management in the admin module, adds user security features (email verification, account lockout), implements new authentication pages for login/register/password recovery, and updates styling with theme variables. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant AuthCallback as Auth Callback<br/>Component
participant AuthService
participant TokenStorage as Token Storage<br/>Service
participant Backend
participant Router
Browser->>AuthCallback: Redirect from OAuth provider<br/>with tokens in URL
AuthCallback->>AuthCallback: Parse tokens from query/fragment
AuthCallback->>AuthService: handleOAuthTokens(accessToken,<br/>refreshToken, expiresIn)
AuthService->>TokenStorage: Store tokens & expiry
TokenStorage->>TokenStorage: Save to localStorage
AuthService->>Backend: Fetch current user
Backend-->>AuthService: User data
AuthService->>AuthService: Check if MFA required
alt MFA Required
AuthCallback->>Browser: Show MFA UI
else MFA Not Required
AuthService->>Router: Navigate to /me
Router->>Browser: Redirect to dashboard
end
sequenceDiagram
participant Admin
participant M2MCreate as M2M Create<br/>Component
participant M2MService
participant Backend
participant Admin2 as Admin<br/>Display
Admin->>M2MCreate: Navigate to create page
Admin->>M2MCreate: Fill form (name, scopes,<br/>rate limits)
Admin->>M2MCreate: Submit form
M2MCreate->>M2MService: create(request)
M2MService->>Backend: POST /api/m2m-clients
Backend-->>M2MService: Created client with secret
M2MService-->>M2MCreate: M2mClientCreateResponse
M2MCreate->>M2MCreate: Display secret (copy button)
Admin->>M2MCreate: View client details
M2MCreate->>M2MService: getById(clientId)
M2MService->>Backend: GET /api/m2m-clients/:id
Backend-->>M2MService: Client details
M2MService-->>Admin2: Render detail view
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. 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: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app/modules/home/components/auth-callback/auth-callback.component.ts (1)
231-245: Logic bug: Success state is immediately overwritten.Lines 242-244 execute unconditionally after the
if (result.result)block, overwriting the success state. This meansshowTotpInvalidwill always betrueandshowInfoblockwill always befalseafter the subscription callback runs, regardless of verification success.Additionally, this subscription lacks
untilDestroyed(this)for proper cleanup.🐛 Proposed fix
validateTotp(): void { if (this.totpCode == null || this.totpCode.length !== 6) { this.showTotpInvalid = true; this.totpCodeSent = false; return; } - this.totpService.verifyTotp(this.totpCode).subscribe((result) => { + this.totpService.verifyTotp(this.totpCode) + .pipe(untilDestroyed(this)) + .subscribe((result) => { if (result.result) { this.showTotpInvalid = false; this.showInfoblock = true; this.totpCodeSent = true; - this.authService.getCurrentUserFromBackend().subscribe((user) => { + this.authService.getCurrentUserFromBackend() + .pipe(untilDestroyed(this)) + .subscribe((user) => { this.redirectToMainPageOrUrl(); }); + } else { + this.showInfoblock = false; + this.showTotpInvalid = true; + this.totpCodeSent = false; } - - this.showInfoblock = false; - this.showTotpInvalid = true; - this.totpCodeSent = false; }); }src/app/shared/services/auth/auth.service.ts (1)
72-81: Only clear the session on auth-related errors ingetCurrentUserFromBackend.The current
catchErrorclears the session for all errors (network failures, 5xx, etc.), which logs out users on transient failures. Clear the session only for 401 (invalid token) and 403 (forbidden) status codes.Suggested fix
+import { HttpErrorResponse } from "@angular/common/http"; - catchError(() => { - this.clearSession(); - return of(null); - }), + catchError((error: HttpErrorResponse) => { + if (error.status === 401 || error.status === 403) { + this.clearSession(); + } + return of(null); + }),
🤖 Fix all issues with AI agents
In `@src/app/modules/admin/components/m2m-clients/m2m-client-create.component.ts`:
- Around line 56-78: Before performing the create request in onSubmit, reset the
previous create state so stale secrets/flags aren't shown: clear
this.createdClient (set to null/undefined) and reset any "copied" flag/state
(e.g., this.copied = false) and ensure isLoading is set appropriately; then
proceed with validation and call this.m2mClientsService.create(...). Also ensure
error and success branches still set isLoading and createdClient as before so
state stays consistent.
In `@src/app/modules/admin/components/m2m-clients/m2m-client-detail.component.ts`:
- Around line 46-68: Validate the route id in ngOnInit/loadClient to avoid NaN
lookups and ensure isLoading is cleared on error: in ngOnInit parse the id from
this.route.snapshot.params["id"] and if it is not a finite number call
this.alertService.error(...) and navigate back to "/admin/m2m-clients" without
calling loadClient; in loadClient ensure this.isLoading is set to true at start
and set to false in both the subscribe.next and subscribe.error handlers (before
navigating) so the spinner is cleared even when the lookup fails; reference
ngOnInit, loadClient, isLoading, this.route.snapshot.params, and the subscribe
error handler to locate changes.
- Around line 38-43: The editForm's rate limit fields lack the same validators
as the create form; update the FormGroup created in editForm (this.editForm =
this.fb.group(...)) to add Validators.min(1) and Validators.max(10000) to
rateLimitPerMinute and Validators.min(1) and Validators.max(1000000) to
rateLimitPerDay so validation matches the create page (keep existing
required/maxlength rules for name/description).
In
`@src/app/modules/admin/components/m2m-clients/m2m-clients-list.component.html`:
- Around line 87-113: The three icon-only action buttons in the m2m-clients-list
template (the buttons that call viewClient(client), toggleStatus(client) and
deleteClient(client)) need accessible labels: add an appropriate aria-label
attribute to each button (e.g., aria-label="View/Edit client",
aria-label="Deactivate client" / "Activate client" based on client.isActive, and
aria-label="Delete client") and mark the inner <i> icons as decorative by adding
aria-hidden="true" (or role="img" with aria-hidden) so screen readers announce
the button action instead of the icon; ensure the toggle button's aria-label
updates based on client.isActive to match the title text.
In
`@src/app/modules/admin/components/users/users-admin-page/users-admin-page.component.html`:
- Around line 168-215: The icon-only action buttons in the users admin row (the
buttons that call openUserRolesForm(item), unlockUser(item),
resetFailedAttempts(item), resendVerification(item), and forceVerifyEmail(item))
lack accessible names; add aria-label attributes to each button (e.g.,
aria-label="Change role", "Unlock user", "Reset failed attempts", "Resend
verification email", "Force verify email") matching the button's action/title so
screen readers can announce them.
In `@src/app/modules/home/components/auth-callback/auth-callback.component.ts`:
- Around line 32-39: Remove all direct console.log calls in
auth-callback.component.ts (e.g., the debug lines in AuthCallbackComponent that
log window.location.href, this.route.snapshot.queryParams,
this.route.snapshot.fragment and similar console statements across the file at
the listed ranges). Replace them with a proper logging mechanism or gated debug
logging: either call the app logger service (e.g., this.logger.debug(...)) or
wrap logs behind an environment/debug flag so nothing sensitive is printed in
production; ensure you update all occurrences referenced (around the ngOnInit
and other methods that use console.log) rather than leaving any stray console
statements.
- Around line 162-174: The parseFragment function currently uses const [key,
value] = pair.split("=") which truncates values containing '=' (e.g., base64
tokens); change the split logic in parseFragment to find the first '=' (use
pair.indexOf('=')) and slice the key as pair.slice(0, idx) and the value as
pair.slice(idx + 1) so the value retains any '=' characters, then
decodeURIComponent both key and value and handle missing '=' (skip or treat
value as empty) accordingly.
In
`@src/app/modules/home/components/forgot-password-page/forgot-password-page.component.html`:
- Around line 24-33: The email input in forgot-password-page.component (the
<input> with id="email" and [(ngModel)]="email") uses an English placeholder;
update it to Russian or add i18n metadata per guidelines — either replace
placeholder="Enter your email address" with the appropriate Russian string or
add an i18n attribute (e.g.,
i18n-placeholder="@@forgotPassword.emailPlaceholder") and provide the localized
text in the translation files so the placeholder matches the Russian UI.
In `@src/app/modules/home/components/login-page/login-page.component.html`:
- Around line 85-89: The loading text "Signing in..." is not localized; update
the template around the isLoading span (referencing the isLoading conditional,
spinner-wrapper span and spinner-border) to use the app's i18n approach: either
replace the hardcoded English with the Russian equivalent or add i18n metadata
(e.g., i18n attribute) or use the translation key (e.g., a translate pipe or
i18n key like LOGIN.SIGNING_IN) so the loading text is localized consistently
with the rest of the page.
- Around line 62-71: The password visibility toggle button is currently removed
from keyboard focus with tabindex="-1"; remove that attribute so the button is
focusable, and add accessible state/label bindings: bind aria-pressed to
showPassword and set an aria-label that changes with showPassword (e.g., "Show
password"/"Hide password") so screen readers get context; keep the existing
(click)="togglePasswordVisibility()" handler (and class "password-toggle" and
icon logic using showPassword) unchanged.
In `@src/app/modules/home/components/login-page/login-page.component.ts`:
- Around line 55-81: Replace the nested subscribe pattern in the login flow by
flattening the observables: call authService.loginWithEmail(this.email,
this.password).pipe(untilDestroyed(this), switchMap(() =>
authService.getCurrentUserFromBackend()), untilDestroyed(this)).subscribe({...})
so that login errors are handled in the single subscription error handler (using
alertService.error and setting this.isLoading = false) and successful completion
(or error from getCurrentUserFromBackend if you still want to ignore it)
navigates to "/me" via router.navigate; ensure you preserve setting
this.isLoading = false on both success and error paths and keep references to
the methods loginWithEmail and getCurrentUserFromBackend and the alertService
and router usage.
- Around line 37-44: The queryParams subscription in login-page.component.ts
(this.route.queryParams.subscribe) leaks because it isn't cleaned up; update the
component to unsubscribe on destroy by either piping the observable through
untilDestroyed(this) (or a takeUntil(this.destroy$) pattern) before subscribing,
or store the Subscription and call unsubscribe in ngOnDestroy; ensure you
reference the existing this.route.queryParams subscription and add/implement
ngOnDestroy (or destroy$) to reliably dispose the subscription when the
component is destroyed.
In `@src/app/modules/home/components/register-page/register-page.component.html`:
- Around line 104-109: Remove the tabindex="-1" from the password visibility
toggle buttons so they remain keyboard-focusable, and add an appropriate
aria-label (e.g., aria-label="Show password" / "Hide password" managed by the
component) to each button; update the click handler usage of
togglePasswordVisibility() in the register-page component to also set or reflect
the accessible label/state (and apply the same changes to the second
password-toggle button occurrence).
- Around line 49-70: Fix typos and mixed-language strings in the registration
template by updating the visible Russian text for the form fields: correct the
first name input placeholder bound to firstName from "Выше имя" to "Ваше имя"
and the last name input placeholder bound to lastName from "Ваша файмилия" to
"Ваша фамилия"; then scan the other inputs referenced in this template (e.g.,
ids/names like email, password, confirmPassword or any placeholders/labels
around lines mentioned) and replace any English/misspelled strings with proper
Russian equivalents so all labels/placeholders are consistent.
In
`@src/app/modules/home/components/reset-password-page/reset-password-page.component.html`:
- Around line 43-53: Add accessible labels to the password visibility toggle
buttons by adding an aria-label that reflects the current action (e.g., "Show
password" when showPassword is false and "Hide password" when showPassword is
true). Update the <button> elements that call togglePasswordVisibility() (the
password-toggle buttons referencing the showPassword property and
bi-eye/bi-eye-slash classes) to include an [attr.aria-label] binding that
toggles between the two labels, and apply the same change to the second toggle
button used elsewhere on the page.
In
`@src/app/modules/home/components/reset-password-page/reset-password-page.component.ts`:
- Around line 68-70: The setTimeout used to call
this.router.navigate(["/login"]) can fire after the component is destroyed; add
a class property (e.g., private redirectTimeoutId: any) to store the return
value of setTimeout where it is invoked (in the success handler that currently
calls setTimeout), and implement ngOnDestroy() to
clearTimeout(this.redirectTimeoutId) and null it to avoid the callback running
after destruction; ensure you still call router.navigate only when the timeout
exists and component is alive.
In
`@src/app/modules/home/components/verify-email-page/verify-email-page.component.ts`:
- Around line 17-23: The component VerifyEmailPageComponent currently uses the
raw token in URLs and starts a delayed navigation that can fire after the
component is destroyed; update any place that builds a URL or route with the
token (e.g., where token is read in ngOnInit and where resend or verify links
are constructed) to wrap the token with encodeURIComponent(token) to avoid URL
path issues, and add a class property (e.g., redirectTimer: any) to store the
timer ID and implement ngOnDestroy to call clearTimeout(this.redirectTimer) so
the pending navigation is cancelled when the component is torn down; ensure any
places that set the timer (setTimeout) assign to this.redirectTimer.
In `@src/app/services/auth-api.service.ts`:
- Around line 104-106: The getDeviceInfo method directly accesses
navigator.userAgent which will throw during SSR; update getDeviceInfo to guard
access (e.g., check typeof navigator !== 'undefined' or typeof window !==
'undefined' or use globalThis) and return a safe fallback string (like empty
string or 'server') when navigator is not present so server rendering won't
error; locate and modify the getDeviceInfo() function to implement this guard
and fallback.
🧹 Nitpick comments (39)
src/app/shared/components/dialogs/confirm-dialog/confirm-dialog.component.scss (1)
6-10: Good use of CSS variable for theming.The switch to
var(--warm-modal-overlay)aligns well with the warm theme changes across the PR. Consider adding a fallback value for resilience if the variable is undefined in some context:.modal { - background-color: var(--warm-modal-overlay); + background-color: var(--warm-modal-overlay, rgba(0, 0, 0, 0.6)); -webkit-animation: fadein 0.2s; animation: fadein 0.2s; }src/app/modules/home/components/auth-callback/auth-callback.component.ts (2)
186-222: Refactor nested subscription using RxJS operators.Nested subscriptions are an anti-pattern that makes the code harder to test and manage. Use higher-order mapping operators like
switchMapinstead.♻️ Proposed refactor
+import { switchMap, filter } from "rxjs/operators"; +import { of } from "rxjs"; ... private handleOAuthTokens( accessToken: string, refreshToken: string, expiresIn: number, ): void { this.authService .handleOAuthCallback(accessToken, refreshToken, expiresIn) .pipe( untilDestroyed(this), + switchMap((user) => { + if (!user) { + this.showErrorBlock = true; + this.showInfoblock = false; + return of(null); + } + return this.authService.completeAuthentication(); + }), ) - .subscribe({ - next: (user) => { - if (user) { - this.authService - .completeAuthentication() - .pipe(untilDestroyed(this)) - .subscribe((totpCheck) => { - if (totpCheck.isMfaEnabled) { - this.showMfaBlock = true; - this.showInfoblock = false; - } else { - this.redirectToMainPageOrUrl(); - } - }); - } else { - this.showErrorBlock = true; - this.showInfoblock = false; - } - }, + .subscribe({ + next: (totpCheck) => { + if (!totpCheck) return; + if (totpCheck.isMfaEnabled) { + this.showMfaBlock = true; + this.showInfoblock = false; + } else { + this.redirectToMainPageOrUrl(); + } + }, error: (err) => { console.error("[AuthCallback] handleOAuthCallback error:", err); this.showErrorBlock = true; this.showInfoblock = false; }, }); }
53-56: Clarify the test detection logic.The comment suggests this check is for detecting test execution, but
queryParams == nullusing loose equality is an unusual way to detect tests. If Angular's test utilities set up routes differently, consider using a more explicit test detection mechanism or environment flag.src/environments/environment.staging.ts (1)
1-12: Consider deriving auth redirect URIs frombaseUrlto prevent drift.
Keeps staging config consistent if the host changes later.♻️ Suggested refactor
-export const environment = { +const baseUrl = "http://localhost:4200"; + +export const environment = { production: true, staging: true, isUnderMaintenance: false, type: "staging", - baseUrl: "http://localhost:4200", + baseUrl, resourceApiURI: "https://api.techinterview.space", name: "staging", auth: { - redirectUri: "http://localhost:4200/auth-callback", - postLogoutRedirectUri: "http://localhost:4200/logout-callback", + redirectUri: `${baseUrl}/auth-callback`, + postLogoutRedirectUri: `${baseUrl}/logout-callback`, },src/app/services/user-admin.service.ts (1)
50-76: Consider DRY-ing the repeated action POSTs.
These four methods share the same URL pattern and response shape; a small private helper would reduce duplication and centralize path changes.♻️ Example refactor
+ private postUserAction( + userId: number, + action: "unlock" | "reset-failed-attempts" | "resend-verification" | "force-verify-email", + ): Observable<{ message: string }> { + return this.api.post<{ message: string }>(`${this.apiUrl}${userId}/${action}`, {}); + } + unlockUser(userId: number): Observable<{ message: string }> { - return this.api.post<{ message: string }>( - `${this.apiUrl}${userId}/unlock`, - {}, - ); + return this.postUserAction(userId, "unlock"); } @@ resetFailedAttempts(userId: number): Observable<{ message: string }> { - return this.api.post<{ message: string }>( - `${this.apiUrl}${userId}/reset-failed-attempts`, - {}, - ); + return this.postUserAction(userId, "reset-failed-attempts"); } @@ resendVerification(userId: number): Observable<{ message: string }> { - return this.api.post<{ message: string }>( - `${this.apiUrl}${userId}/resend-verification`, - {}, - ); + return this.postUserAction(userId, "resend-verification"); } @@ forceVerifyEmail(userId: number): Observable<{ message: string }> { - return this.api.post<{ message: string }>( - `${this.apiUrl}${userId}/force-verify-email`, - {}, - ); + return this.postUserAction(userId, "force-verify-email"); }src/app/modules/admin/components/m2m-clients/m2m-clients-list.component.html (1)
37-68: Add trackBy functions and extract complex template expressions into component methods.The
*ngForloops on clients (line 38) and scopes (line 61) lack trackBy functions, causing unnecessary DOM recreation during change detection. Additionally, the inline ternary operator withslice()(lines 50–52), theclientId.slice(0, 12)(line 57), andscopes.slice(0, 3)(line 61) violate the coding guideline requiring templates to avoid complex expressions.Create three component methods—
trackByClientId(),displayDescription(),displayClientId(), andvisibleScopes()—and update the template to use them. This improves performance, readability, and maintainability.♻️ Suggested template adjustments
- <tr - *ngFor="let client of clients" - [class.inactive-row]="!client.isActive" - > + <tr + *ngFor="let client of clients; trackBy: trackByClientId" + [class.inactive-row]="!client.isActive" + > @@ - <div class="client-description" *ngIf="client.description"> - {{ - client.description.length > 50 - ? client.description.slice(0, 50) + "..." - : client.description - }} - </div> + <div class="client-description" *ngIf="client.description"> + {{ displayDescription(client) }} + </div> @@ - <code class="client-id">{{ client.clientId.slice(0, 12) }}...</code> + <code class="client-id">{{ displayClientId(client) }}</code> @@ - <span - *ngFor="let scope of client.scopes.slice(0, 3)" - class="badge badge-scope" - > + <span + *ngFor="let scope of visibleScopes(client); trackBy: trackByScope" + class="badge badge-scope" + >src/app/modules/admin/components/salaries/historical-data-templates/historical-data-templates.component.scss (1)
32-77: Consider alternatives to::ng-deep(deprecated).
::ng-deepis deprecated and may be removed in future Angular versions. While it's currently necessary for styling third-party components likeng-select, consider:
- Using the component's native theming/styling API if available
- Moving these styles to a global stylesheet with proper scoping
For now, this usage is acceptable given the need to style
ng-selectinternals.src/app/modules/salaries/components/add-salary/add-salary.component.scss (1)
76-79: Duplicate responsive rules for.form-container.Lines 76-79 and 108-110 both define responsive styles for
.form-containerat themax-width: 768pxbreakpoint with slightly different padding values (1.5remvs1.5rem 1rem). The second rule (lines 108-110) will override the first.Consider consolidating these into a single media query block for clarity.
Suggested consolidation
`@media` (max-width: 768px) { - padding: 1.5rem; + padding: 1.5rem 1rem; border-radius: 12px; }Then remove lines 107-115.
Also applies to: 107-115
src/app/modules/companies/components/company-review-block/company-review-block.component.scss (1)
186-194: Consider using CSS variables for hardcoded rgba values.The tag background colors use hardcoded rgba values which won't adapt to theme changes. Consider defining these as CSS variables in the theme file for consistency.
Suggested approach
&.positive { - background-color: rgba(129, 178, 154, 0.15); + background-color: var(--warm-success-bg-subtle); color: var(--warm-success); } &.negative { - background-color: rgba(217, 85, 80, 0.15); + background-color: var(--warm-danger-bg-subtle); color: var(--warm-danger); }Define the corresponding variables in your theme file.
src/app/modules/companies/components/add-review-page/add-review-page.component.scss (1)
141-145: Consider using a CSS variable for the focus ring color.The hardcoded
rgba(224, 122, 95, 0.15)won't adapt to theme changes. This pattern appears in multiple files.Suggested approach
&:focus { border-color: var(--warm-accent-primary); - box-shadow: 0 0 0 3px rgba(224, 122, 95, 0.15); + box-shadow: 0 0 0 3px var(--warm-input-focus-ring); outline: none; }src/app/modules/admin/components/m2m-clients/m2m-client-detail.component.scss (2)
35-44: Consider using a CSS variable for the alert background.The hardcoded
rgba(255, 193, 7, 0.15)won't adapt to theme changes.Suggested approach
.secret-alert { display: flex; justify-content: space-between; align-items: flex-start; - background-color: rgba(255, 193, 7, 0.15); + background-color: var(--warm-warning-bg-subtle); border: 1px solid var(--warm-warning);
71-74: Consider defining a CSS variable for the monospace font stack.The font-family
"JetBrains Mono", monospaceis repeated multiple times (lines 72, 123, 226, 275). A CSS variable would improve maintainability.Suggested approach
Define in your theme file:
--warm-font-mono: "JetBrains Mono", monospace;Then use throughout:
.secret-input { - font-family: "JetBrains Mono", monospace; + font-family: var(--warm-font-mono); font-size: 0.9rem; }src/app/modules/home/components/register-page/register-page.component.scss (3)
37-43: Consider using a CSS variable for the heading font.The font-family is hardcoded. If
--warm-font-headingis already defined (as seen in other components), use it here for consistency.Suggested approach
.auth-title { - font-family: "Lora", Georgia, serif; + font-family: var(--warm-font-heading); font-size: 1.75rem;
50-67: Consider using a CSS variable for the success background.The hardcoded
rgba(107, 142, 35, 0.1)won't adapt to theme changes, similar to other alert backgrounds in this PR.Suggested approach
.success-message { display: flex; align-items: flex-start; gap: 1rem; padding: 1.25rem; - background-color: rgba(107, 142, 35, 0.1); + background-color: var(--warm-success-bg-subtle); border: 1px solid var(--warm-success);
170-178: Consider using a CSS variable for the focus ring.Same pattern as other files - hardcoded
rgba(224, 122, 95, 0.15)could bevar(--warm-input-focus-ring)for consistency.src/app/shared/services/auth/token-storage.service.ts (1)
24-37: Consider wrapping localStorage operations in try-catch.While the browser check is good for SSR,
localStorageoperations can throw exceptions in certain scenarios (storage quota exceeded, private browsing mode in some browsers, security restrictions). This could cause runtime errors.💡 Optional: Add defensive error handling
setTokens( accessToken: string, refreshToken: string, expiresIn: number, ): void { if (!this.isBrowser) { return; } - const expiresAt = Date.now() + expiresIn * 1000; - localStorage.setItem(this.ACCESS_TOKEN_KEY, accessToken); - localStorage.setItem(this.REFRESH_TOKEN_KEY, refreshToken); - localStorage.setItem(this.EXPIRES_AT_KEY, expiresAt.toString()); + try { + const expiresAt = Date.now() + expiresIn * 1000; + localStorage.setItem(this.ACCESS_TOKEN_KEY, accessToken); + localStorage.setItem(this.REFRESH_TOKEN_KEY, refreshToken); + localStorage.setItem(this.EXPIRES_AT_KEY, expiresAt.toString()); + } catch (e) { + console.error('Failed to store tokens:', e); + } }src/app/shared/services/auth/auth.session.service.ts (3)
21-30: Redundant assertion inside truthy block.On line 23,
Assertion.notNull(authInfo, "authInfo")is called after already checkingif (authInfo)on line 22. The assertion will never fail sinceauthInfois guaranteed to be truthy at that point.♻️ Remove redundant assertion
set auth(authInfo: AuthInfo | null) { if (authInfo) { - Assertion.notNull(authInfo, "authInfo"); this.session.setItem(this.authorizationStorageSessionKey, authInfo); this.session.setItem(this.authorizationTimestampSessionKey, Date.now()); } else { this.session.removeItem(this.authorizationStorageSessionKey); this.session.removeItem(this.authorizationTimestampSessionKey); } }
32-41: Redundant null coalescing operator.On line 40,
authInfo ?? nullis unnecessary because line 36 already returnsnullifauthInfo == null. At line 40,authInfois guaranteed to be non-null.♻️ Simplify return statement
get auth(): AuthInfo | null { const authInfo = this.session.getItem<AuthInfo>( this.authorizationStorageSessionKey, ); if (authInfo == null || this.sessionExpired) { return null; } - return authInfo ?? null; + return authInfo; }
43-50: Same redundant assertion pattern in applicationUser setter.The
Assertion.notNull(user, "user")on line 45 is redundant sinceuseris already confirmed truthy by theif (user)check.♻️ Remove redundant assertion
set applicationUser(user: ApplicationUser | null) { if (user) { - Assertion.notNull(user, "user"); this.session.setItem(this.applicationUserStorageSessionKey, user); } else { this.session.removeItem(this.applicationUserStorageSessionKey); } }src/app/modules/admin/services/m2m-clients.service.ts (1)
1-19: UseApiServicewrapper for consistency with the codebase pattern.This service uses
HttpClientdirectly, while 16+ other services in the codebase use theApiServicewrapper. Consider refactoring to useApiServiceto leverage centralized request handling (base URL, headers, error transformation, and retry strategies).src/app/modules/admin/components/m2m-clients/m2m-client-create.component.html (1)
48-63: AddtrackByfor scope lists to prevent DOM churn.Without
trackBy, Angular recreates items on each array update, which can reset checkbox focus/selection. Please verify and addtrackByhandlers in the component (e.g., byscope.value).♻️ Suggested template update
- <div *ngFor="let scope of availableScopes" class="scope-item"> + <div + *ngFor="let scope of availableScopes; trackBy: trackByScope" + class="scope-item" + > - <span - *ngFor="let scope of createdClient.scopes" - class="badge badge-scope" - > + <span + *ngFor="let scope of createdClient.scopes; trackBy: trackByScopeValue" + class="badge badge-scope" + >Also applies to: 163-168
src/app/modules/home/components/register-page/register-page.component.ts (1)
96-106: Keep submit disabled when passwords mismatch.
isFormValid()doesn't include the confirmation match, so the submit button can enable even when the passwords differ. Folding the check into validation avoids the extra warning step.♻️ Suggested update
- this.confirmPassword.trim() !== "" && + this.confirmPassword.trim() !== "" && + this.password === this.confirmPassword &&src/app/modules/home/components/reset-password-page/reset-password-page.component.ts (3)
31-31: Consider moving title setup tongOnInitfor consistency.Setting the title in the constructor works, but moving it to
ngOnInitwould be more consistent with Angular lifecycle patterns and align with howngOnDestroyresets the title.
98-117: Duplicated password validation logic betweenisPasswordStrong()andgetPasswordStrength().Both methods compute
hasLower,hasUpper, andhasDigitindependently. Consider consolidating this logic to avoid duplication and ensure consistency.♻️ Proposed refactor
+ private getPasswordCriteria(): { hasLower: boolean; hasUpper: boolean; hasDigit: boolean; hasSpecial: boolean } { + return { + hasLower: /[a-z]/.test(this.newPassword), + hasUpper: /[A-Z]/.test(this.newPassword), + hasDigit: /\d/.test(this.newPassword), + hasSpecial: /[!@#$%^&*(),.?":{}|<>]/.test(this.newPassword), + }; + } + isPasswordStrong(): boolean { - const hasLower = /[a-z]/.test(this.newPassword); - const hasUpper = /[A-Z]/.test(this.newPassword); - const hasDigit = /\d/.test(this.newPassword); - return hasLower && hasUpper && hasDigit; + const { hasLower, hasUpper, hasDigit } = this.getPasswordCriteria(); + return hasLower && hasUpper && hasDigit; } getPasswordStrength(): "weak" | "medium" | "strong" { if (this.newPassword.length < 8) return "weak"; - const hasLower = /[a-z]/.test(this.newPassword); - const hasUpper = /[A-Z]/.test(this.newPassword); - const hasDigit = /\d/.test(this.newPassword); - const hasSpecial = /[!@#$%^&*(),.?":{}|<>]/.test(this.newPassword); + const { hasLower, hasUpper, hasDigit, hasSpecial } = this.getPasswordCriteria(); const score = [hasLower, hasUpper, hasDigit, hasSpecial].filter(Boolean).length; if (score >= 4 && this.newPassword.length >= 12) return "strong"; if (score >= 3) return "medium"; return "weak"; }
8-13: Consider addingchangeDetection: ChangeDetectionStrategy.OnPushfor better performance.Since this component uses explicit state management with properties like
isLoading,successMessage, anderrorMessage, it's a good candidate for OnPush change detection.src/app/modules/home/components/reset-password-page/reset-password-page.component.scss (1)
55-55: Consider using CSS variables for the rgba background colors.The hardcoded
rgba(192, 80, 77, 0.1)andrgba(107, 142, 35, 0.1)values may not adapt well to theme changes. Consider defining CSS variables for these semi-transparent backgrounds or deriving them from the existing--warm-dangerand--warm-successtokens.Also applies to: 75-75
src/app/modules/home/components/reset-password-page/reset-password-page.component.html (1)
55-60: Repeated method calls in template may impact performance.
getPasswordStrength()is called twice in the template (lines 56, 57-58), andisFormValid()is called on every change detection cycle (line 106). Consider caching these values in the component or using getter properties with OnPush change detection.Also applies to: 105-107
src/app/modules/home/components/verify-email-page/verify-email-page.component.html (1)
37-62: Consider wrapping the resend section in a<form>element.The email input and submit button work without a form wrapper, but wrapping them in
<form>would enable keyboard submission (Enter key) and improve semantic structure.♻️ Proposed refactor
<!-- Resend Form --> <div *ngIf="canResend && !successMessage" class="resend-section"> <p class="resend-info"> Enter your email address to receive a new verification link. </p> + <form (ngSubmit)="resendVerification()"> <div class="form-group"> <label for="email" class="form-label">Email</label> <input type="email" id="email" class="form-control" [(ngModel)]="email" name="email" placeholder="Enter your email address" [disabled]="isResending" autocomplete="email" /> </div> <button - type="button" + type="submit" class="btn-submit" - (click)="resendVerification()" [disabled]="isResending || !isEmailValid()" > <span *ngIf="!isResending">Resend Verification Email</span> <span *ngIf="isResending" class="spinner-wrapper"> <span class="spinner-border spinner-border-sm" role="status"></span> Sending... </span> </button> + </form> </div>src/app/modules/admin/components/m2m-clients/m2m-client-create.component.scss (1)
75-137: Add small-screen stacking for flex rows/actions.On narrow screens,
.form-rowand.form-actionsstay in a single row and can overflow. Consider stacking them below 600px like.scopes-griddoes. As per coding guidelines.♻️ Suggested responsive tweaks
.form-row { display: flex; gap: 1rem; + flex-wrap: wrap; .half { flex: 1; } + + `@media` (max-width: 600px) { + flex-direction: column; + } } @@ .form-actions { display: flex; gap: 0.75rem; margin-top: 1.5rem; + + `@media` (max-width: 600px) { + flex-direction: column; + } }src/app/modules/home/components/forgot-password-page/forgot-password-page.component.ts (1)
54-58: Consider extracting the shared email validation regex.The email regex
/^[^\s@]+@[^\s@]+\.[^\s@]+$/is duplicated acrossLoginPageComponent,RegisterPageComponent, and this component. Consider extracting it to a shared utility or validator.♻️ Suggested refactor
Create a shared validation utility:
// src/app/shared/utils/validators.ts export const EMAIL_REGEX = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; export function isValidEmail(email: string): boolean { return email.trim() !== '' && EMAIL_REGEX.test(email); }Then use in components:
+import { isValidEmail } from '@shared/utils/validators'; isEmailValid(): boolean { - return ( - this.email.trim() !== "" && /^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(this.email) - ); + return isValidEmail(this.email); }src/app/modules/admin/components/m2m-clients/m2m-clients-list.component.ts (3)
72-79: Consider using the app's confirm dialog component instead of nativeconfirm().The codebase has a
confirm-dialogcomponent (referenced insrc/app/shared/components/dialogs/confirm-dialog/confirm-dialog.component.ts). Using it would provide a consistent UX across the application.
55-70: Direct state mutation may cause inconsistency.Line 62 directly mutates
client.isActivebased on the assumed toggle result rather than using the API response. If the server returns a different state, the UI will be out of sync.♻️ Suggested improvement
toggleStatus(client: M2mClientModel): void { const action = client.isActive ? this.m2mClientsService.deactivate(client.id) : this.m2mClientsService.activate(client.id); action.pipe(untilDestroyed(this)).subscribe({ - next: () => { - client.isActive = !client.isActive; + next: (updatedClient) => { + const index = this.clients.findIndex(c => c.id === client.id); + if (index !== -1) { + this.clients[index] = updatedClient; + } this.alertService.success( - `Client ${client.isActive ? "activated" : "deactivated"}`, + `Client ${updatedClient.isActive ? "activated" : "deactivated"}`, ); }, error: (err) => this.alertService.error(err.error?.message || "Action failed"), }); }This assumes the API returns the updated client. If it doesn't, the current approach is acceptable but should reload from the server for correctness.
28-28: EmptyngOnDestroyis intentional.The empty body is required for the
untilDestroyedoperator to function correctly. Consider adding a brief comment to clarify this for future maintainers.- ngOnDestroy(): void {} + ngOnDestroy(): void { + // Required for untilDestroyed operator + }src/app/modules/admin/components/m2m-clients/m2m-client-detail.component.html (3)
59-63: AddtrackByfunction for*ngForto improve performance.When iterating over arrays, using
trackByhelps Angular identify which items changed, reducing unnecessary DOM updates.- *ngFor="let scope of generatedToken.scopes" + *ngFor="let scope of generatedToken.scopes; trackBy: trackByScope"Add to component:
trackByScope(index: number, scope: string): string { return scope; }
182-196: AddtrackByfunction for available scopes iteration.Same as above, consider adding
trackByfor theavailableScopesloop.- <div *ngFor="let scope of availableScopes" class="scope-toggle"> + <div *ngFor="let scope of availableScopes; trackBy: trackByScopeValue" class="scope-toggle">
26-31: Security consideration: Secret input should use appropriate input type.The secret is displayed in a
type="text"input. While it's intentional to show the secret for copying, consider if masking with a toggle would be more appropriate to prevent shoulder-surfing.src/app/modules/home/components/login-page/login-page.component.ts (1)
103-105: Add a comment notingngOnDestroyis required foruntilDestroyed.Similar to other components, it helps maintainability to document why
ngOnDestroyexists.src/app/modules/admin/components/m2m-clients/m2m-client-create.component.ts (1)
81-87: Handle clipboard failures instead of assuming success.
navigator.clipboard.writeTextis async and can reject (permissions/HTTP); consider handling failure and only toggling the copied state after success.♻️ Suggested hardening
copySecret(): void { - if (this.createdClient) { - navigator.clipboard.writeText(this.createdClient.clientSecret); - this.secretCopied = true; - setTimeout(() => (this.secretCopied = false), 2000); - } + if (!this.createdClient) { + return; + } + if (!navigator.clipboard) { + this.alertService.error("Clipboard is not available in this browser."); + return; + } + navigator.clipboard + .writeText(this.createdClient.clientSecret) + .then(() => { + this.secretCopied = true; + setTimeout(() => (this.secretCopied = false), 2000); + }) + .catch(() => this.alertService.error("Failed to copy client secret.")); }src/app/modules/home/components/verify-email-page/verify-email-page.component.ts (1)
103-107: Consider centralizing the email regex used across auth pages.The same pattern appears in
src/app/modules/home/components/forgot-password-page/forgot-password-page.component.tsandsrc/app/modules/home/components/login-page/login-page.component.ts. A shared validator/helper reduces drift.
| onSubmit(): void { | ||
| if (this.form.invalid) { | ||
| this.form.markAllAsTouched(); | ||
| return; | ||
| } | ||
|
|
||
| this.isLoading = true; | ||
| this.m2mClientsService | ||
| .create(this.form.value) | ||
| .pipe(untilDestroyed(this)) | ||
| .subscribe({ | ||
| next: (response) => { | ||
| this.isLoading = false; | ||
| this.createdClient = response; | ||
| this.alertService.success("M2M client created successfully"); | ||
| }, | ||
| error: (err) => { | ||
| this.isLoading = false; | ||
| this.alertService.error( | ||
| err.error?.message || "Failed to create client", | ||
| ); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Reset create state before new submit to avoid stale secrets.
If a previous creation succeeded, a later failed submit can leave the old createdClient (and “copied” state) visible.
🛠️ Suggested fix
onSubmit(): void {
if (this.form.invalid) {
this.form.markAllAsTouched();
return;
}
this.isLoading = true;
+ this.createdClient = null;
+ this.secretCopied = false;
this.m2mClientsService
.create(this.form.value)
.pipe(untilDestroyed(this))
.subscribe({🤖 Prompt for AI Agents
In `@src/app/modules/admin/components/m2m-clients/m2m-client-create.component.ts`
around lines 56 - 78, Before performing the create request in onSubmit, reset
the previous create state so stale secrets/flags aren't shown: clear
this.createdClient (set to null/undefined) and reset any "copied" flag/state
(e.g., this.copied = false) and ensure isLoading is set appropriately; then
proceed with validation and call this.m2mClientsService.create(...). Also ensure
error and success branches still set isLoading and createdClient as before so
state stays consistent.
| this.editForm = this.fb.group({ | ||
| name: ["", [Validators.required, Validators.maxLength(200)]], | ||
| description: ["", Validators.maxLength(1000)], | ||
| rateLimitPerMinute: [null], | ||
| rateLimitPerDay: [null], | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find files related to m2m-clients component
find . -path "*/m2m-clients*" -type f -name "*.ts" | head -20Repository: Techinterview-space/frontend
Length of output: 360
🏁 Script executed:
#!/bin/bash
# List all TypeScript files in the m2m-clients directory
ls -la src/app/modules/admin/components/m2m-clients/ 2>/dev/null || echo "Directory not found, searching..."
find . -name "*m2m*" -type f | grep -E "\.(ts|component)$"Repository: Techinterview-space/frontend
Length of output: 1249
🏁 Script executed:
#!/bin/bash
# Look for the create component file
find . -name "*m2m-client-create*" -o -name "*m2m-clients-create*" | head -10Repository: Techinterview-space/frontend
Length of output: 309
🏁 Script executed:
#!/bin/bash
# Read the m2m-client-create component to see the form group definition
cat -n src/app/modules/admin/components/m2m-clients/m2m-client-create.component.tsRepository: Techinterview-space/frontend
Length of output: 3804
🏁 Script executed:
#!/bin/bash
# Also check the detail component to see the exact lines mentioned
cat -n src/app/modules/admin/components/m2m-clients/m2m-client-detail.component.tsRepository: Techinterview-space/frontend
Length of output: 8098
Align rate‑limit validation with the create form.
The create page enforces min(1)–max(10000) for rateLimitPerMinute and min(1)–max(1000000) for rateLimitPerDay; the edit form currently has no validators. Add these constraints to maintain consistency.
🛠️ Suggested fix
name: ["", [Validators.required, Validators.maxLength(200)]],
description: ["", Validators.maxLength(1000)],
- rateLimitPerMinute: [null],
- rateLimitPerDay: [null],
+ rateLimitPerMinute: [null, [Validators.min(1), Validators.max(10000)]],
+ rateLimitPerDay: [null, [Validators.min(1), Validators.max(1000000)]],
});📝 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.
| this.editForm = this.fb.group({ | |
| name: ["", [Validators.required, Validators.maxLength(200)]], | |
| description: ["", Validators.maxLength(1000)], | |
| rateLimitPerMinute: [null], | |
| rateLimitPerDay: [null], | |
| }); | |
| this.editForm = this.fb.group({ | |
| name: ["", [Validators.required, Validators.maxLength(200)]], | |
| description: ["", Validators.maxLength(1000)], | |
| rateLimitPerMinute: [null, [Validators.min(1), Validators.max(10000)]], | |
| rateLimitPerDay: [null, [Validators.min(1), Validators.max(1000000)]], | |
| }); |
🤖 Prompt for AI Agents
In `@src/app/modules/admin/components/m2m-clients/m2m-client-detail.component.ts`
around lines 38 - 43, The editForm's rate limit fields lack the same validators
as the create form; update the FormGroup created in editForm (this.editForm =
this.fb.group(...)) to add Validators.min(1) and Validators.max(10000) to
rateLimitPerMinute and Validators.min(1) and Validators.max(1000000) to
rateLimitPerDay so validation matches the create page (keep existing
required/maxlength rules for name/description).
| ngOnInit(): void { | ||
| const id = +this.route.snapshot.params["id"]; | ||
| this.loadClient(id); | ||
| } | ||
|
|
||
| ngOnDestroy(): void {} | ||
|
|
||
| loadClient(id: number): void { | ||
| this.isLoading = true; | ||
| this.m2mClientsService | ||
| .getById(id) | ||
| .pipe(untilDestroyed(this)) | ||
| .subscribe({ | ||
| next: (client) => { | ||
| this.client = client; | ||
| this.editForm.patchValue(client); | ||
| this.isLoading = false; | ||
| }, | ||
| error: () => { | ||
| this.alertService.error("Failed to load client"); | ||
| this.router.navigate(["/admin/m2m-clients"]); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Guard against invalid route IDs and clear loading on error.
Prevents NaN lookups and avoids leaving the spinner on if navigation is blocked.
🛠️ Suggested fix
ngOnInit(): void {
- const id = +this.route.snapshot.params["id"];
+ const id = Number(this.route.snapshot.params["id"]);
+ if (!Number.isFinite(id)) {
+ this.alertService.error("Invalid client id.");
+ this.router.navigate(["/admin/m2m-clients"]);
+ return;
+ }
this.loadClient(id);
}
@@
error: () => {
+ this.isLoading = false;
this.alertService.error("Failed to load client");
this.router.navigate(["/admin/m2m-clients"]);
},🤖 Prompt for AI Agents
In `@src/app/modules/admin/components/m2m-clients/m2m-client-detail.component.ts`
around lines 46 - 68, Validate the route id in ngOnInit/loadClient to avoid NaN
lookups and ensure isLoading is cleared on error: in ngOnInit parse the id from
this.route.snapshot.params["id"] and if it is not a finite number call
this.alertService.error(...) and navigate back to "/admin/m2m-clients" without
calling loadClient; in loadClient ensure this.isLoading is set to true at start
and set to false in both the subscribe.next and subscribe.error handlers (before
navigating) so the spinner is cleared even when the lookup fails; reference
ngOnInit, loadClient, isLoading, this.route.snapshot.params, and the subscribe
error handler to locate changes.
| <button | ||
| class="btn btn-outline-primary" | ||
| (click)="viewClient(client)" | ||
| title="View/Edit" | ||
| > | ||
| <i class="bi bi-pencil"></i> | ||
| </button> | ||
| <button | ||
| class="btn" | ||
| [ngClass]=" | ||
| client.isActive | ||
| ? 'btn-outline-warning' | ||
| : 'btn-outline-success' | ||
| " | ||
| (click)="toggleStatus(client)" | ||
| [title]="client.isActive ? 'Deactivate' : 'Activate'" | ||
| > | ||
| <i | ||
| class="bi" | ||
| [ngClass]="client.isActive ? 'bi-pause' : 'bi-play'" | ||
| ></i> | ||
| </button> | ||
| <button | ||
| class="btn btn-outline-danger" | ||
| (click)="deleteClient(client)" | ||
| title="Delete" | ||
| > |
There was a problem hiding this comment.
Add accessible labels for icon-only action buttons.
Icon-only buttons are not announced by screen readers, which blocks keyboard/screen-reader users from understanding actions. Add aria-label on the buttons and mark icons as decorative. As per coding guidelines, ensure accessibility in templates.
✅ Suggested fix
- <button
- class="btn btn-outline-primary"
- (click)="viewClient(client)"
- title="View/Edit"
- >
- <i class="bi bi-pencil"></i>
- </button>
+ <button
+ class="btn btn-outline-primary"
+ (click)="viewClient(client)"
+ title="View/Edit"
+ aria-label="View or edit client"
+ >
+ <i class="bi bi-pencil" aria-hidden="true"></i>
+ </button>
@@
- <button
- class="btn"
- [ngClass]="
- client.isActive
- ? 'btn-outline-warning'
- : 'btn-outline-success'
- "
- (click)="toggleStatus(client)"
- [title]="client.isActive ? 'Deactivate' : 'Activate'"
- >
- <i
- class="bi"
- [ngClass]="client.isActive ? 'bi-pause' : 'bi-play'"
- ></i>
- </button>
+ <button
+ class="btn"
+ [ngClass]="
+ client.isActive
+ ? 'btn-outline-warning'
+ : 'btn-outline-success'
+ "
+ (click)="toggleStatus(client)"
+ [title]="client.isActive ? 'Deactivate' : 'Activate'"
+ [attr.aria-label]="client.isActive ? 'Deactivate client' : 'Activate client'"
+ >
+ <i
+ class="bi"
+ [ngClass]="client.isActive ? 'bi-pause' : 'bi-play'"
+ aria-hidden="true"
+ ></i>
+ </button>
@@
- <button
- class="btn btn-outline-danger"
- (click)="deleteClient(client)"
- title="Delete"
- >
- <i class="bi bi-trash"></i>
- </button>
+ <button
+ class="btn btn-outline-danger"
+ (click)="deleteClient(client)"
+ title="Delete"
+ aria-label="Delete client"
+ >
+ <i class="bi bi-trash" aria-hidden="true"></i>
+ </button>📝 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.
| <button | |
| class="btn btn-outline-primary" | |
| (click)="viewClient(client)" | |
| title="View/Edit" | |
| > | |
| <i class="bi bi-pencil"></i> | |
| </button> | |
| <button | |
| class="btn" | |
| [ngClass]=" | |
| client.isActive | |
| ? 'btn-outline-warning' | |
| : 'btn-outline-success' | |
| " | |
| (click)="toggleStatus(client)" | |
| [title]="client.isActive ? 'Deactivate' : 'Activate'" | |
| > | |
| <i | |
| class="bi" | |
| [ngClass]="client.isActive ? 'bi-pause' : 'bi-play'" | |
| ></i> | |
| </button> | |
| <button | |
| class="btn btn-outline-danger" | |
| (click)="deleteClient(client)" | |
| title="Delete" | |
| > | |
| <button | |
| class="btn btn-outline-primary" | |
| (click)="viewClient(client)" | |
| title="View/Edit" | |
| aria-label="View or edit client" | |
| > | |
| <i class="bi bi-pencil" aria-hidden="true"></i> | |
| </button> | |
| <button | |
| class="btn" | |
| [ngClass]=" | |
| client.isActive | |
| ? 'btn-outline-warning' | |
| : 'btn-outline-success' | |
| " | |
| (click)="toggleStatus(client)" | |
| [title]="client.isActive ? 'Deactivate' : 'Activate'" | |
| [attr.aria-label]="client.isActive ? 'Deactivate client' : 'Activate client'" | |
| > | |
| <i | |
| class="bi" | |
| [ngClass]="client.isActive ? 'bi-pause' : 'bi-play'" | |
| aria-hidden="true" | |
| ></i> | |
| </button> | |
| <button | |
| class="btn btn-outline-danger" | |
| (click)="deleteClient(client)" | |
| title="Delete" | |
| aria-label="Delete client" | |
| > | |
| <i class="bi bi-trash" aria-hidden="true"></i> | |
| </button> |
🤖 Prompt for AI Agents
In `@src/app/modules/admin/components/m2m-clients/m2m-clients-list.component.html`
around lines 87 - 113, The three icon-only action buttons in the
m2m-clients-list template (the buttons that call viewClient(client),
toggleStatus(client) and deleteClient(client)) need accessible labels: add an
appropriate aria-label attribute to each button (e.g., aria-label="View/Edit
client", aria-label="Deactivate client" / "Activate client" based on
client.isActive, and aria-label="Delete client") and mark the inner <i> icons as
decorative by adding aria-hidden="true" (or role="img" with aria-hidden) so
screen readers announce the button action instead of the icon; ensure the toggle
button's aria-label updates based on client.isActive to match the title text.
| <div class="btn-group btn-group-sm" role="group"> | ||
| <button | ||
| type="button" | ||
| class="btn btn-outline-primary btn-sm" | ||
| (click)="openUserRolesForm(item)" | ||
| title="Изменить роль" | ||
| > | ||
| <i class="bi bi-person-gear"></i> | ||
| </button> | ||
| <button | ||
| *ngIf="item.isLockedOut" | ||
| type="button" | ||
| class="btn btn-outline-success btn-sm" | ||
| (click)="unlockUser(item)" | ||
| title="Разблокировать" | ||
| > | ||
| <i class="bi bi-unlock"></i> | ||
| </button> | ||
| <button | ||
| *ngIf=" | ||
| item.failedLoginAttempts && item.failedLoginAttempts > 0 | ||
| " | ||
| type="button" | ||
| class="btn btn-outline-warning btn-sm" | ||
| (click)="resetFailedAttempts(item)" | ||
| title="Сбросить попытки" | ||
| > | ||
| <i class="bi bi-arrow-counterclockwise"></i> | ||
| </button> | ||
| <button | ||
| *ngIf="!item.emailConfirmed" | ||
| type="button" | ||
| class="btn btn-outline-info btn-sm" | ||
| (click)="resendVerification(item)" | ||
| title="Отправить письмо" | ||
| > | ||
| <i class="bi bi-envelope"></i> | ||
| </button> | ||
| <button | ||
| *ngIf="!item.emailConfirmed" | ||
| type="button" | ||
| class="btn btn-outline-success btn-sm" | ||
| (click)="forceVerifyEmail(item)" | ||
| title="Подтвердить email" | ||
| > | ||
| <i class="bi bi-check-circle"></i> | ||
| </button> | ||
| </div> |
There was a problem hiding this comment.
Add accessible labels to icon-only action buttons.
Icon-only buttons should expose an accessible name so screen readers can announce the action. As per coding guidelines, please add aria-label attributes.
🔧 Suggested update
- <button
+ <button
type="button"
class="btn btn-outline-primary btn-sm"
(click)="openUserRolesForm(item)"
title="Изменить роль"
+ aria-label="Изменить роль"
>
<i class="bi bi-person-gear"></i>
</button>
<button
*ngIf="item.isLockedOut"
type="button"
class="btn btn-outline-success btn-sm"
(click)="unlockUser(item)"
title="Разблокировать"
+ aria-label="Разблокировать"
>
<i class="bi bi-unlock"></i>
</button>
<button
*ngIf="
item.failedLoginAttempts && item.failedLoginAttempts > 0
"
type="button"
class="btn btn-outline-warning btn-sm"
(click)="resetFailedAttempts(item)"
title="Сбросить попытки"
+ aria-label="Сбросить попытки"
>
<i class="bi bi-arrow-counterclockwise"></i>
</button>
<button
*ngIf="!item.emailConfirmed"
type="button"
class="btn btn-outline-info btn-sm"
(click)="resendVerification(item)"
title="Отправить письмо"
+ aria-label="Отправить письмо"
>
<i class="bi bi-envelope"></i>
</button>
<button
*ngIf="!item.emailConfirmed"
type="button"
class="btn btn-outline-success btn-sm"
(click)="forceVerifyEmail(item)"
title="Подтвердить email"
+ aria-label="Подтвердить email"
>
<i class="bi bi-check-circle"></i>
</button>📝 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.
| <div class="btn-group btn-group-sm" role="group"> | |
| <button | |
| type="button" | |
| class="btn btn-outline-primary btn-sm" | |
| (click)="openUserRolesForm(item)" | |
| title="Изменить роль" | |
| > | |
| <i class="bi bi-person-gear"></i> | |
| </button> | |
| <button | |
| *ngIf="item.isLockedOut" | |
| type="button" | |
| class="btn btn-outline-success btn-sm" | |
| (click)="unlockUser(item)" | |
| title="Разблокировать" | |
| > | |
| <i class="bi bi-unlock"></i> | |
| </button> | |
| <button | |
| *ngIf=" | |
| item.failedLoginAttempts && item.failedLoginAttempts > 0 | |
| " | |
| type="button" | |
| class="btn btn-outline-warning btn-sm" | |
| (click)="resetFailedAttempts(item)" | |
| title="Сбросить попытки" | |
| > | |
| <i class="bi bi-arrow-counterclockwise"></i> | |
| </button> | |
| <button | |
| *ngIf="!item.emailConfirmed" | |
| type="button" | |
| class="btn btn-outline-info btn-sm" | |
| (click)="resendVerification(item)" | |
| title="Отправить письмо" | |
| > | |
| <i class="bi bi-envelope"></i> | |
| </button> | |
| <button | |
| *ngIf="!item.emailConfirmed" | |
| type="button" | |
| class="btn btn-outline-success btn-sm" | |
| (click)="forceVerifyEmail(item)" | |
| title="Подтвердить email" | |
| > | |
| <i class="bi bi-check-circle"></i> | |
| </button> | |
| </div> | |
| <div class="btn-group btn-group-sm" role="group"> | |
| <button | |
| type="button" | |
| class="btn btn-outline-primary btn-sm" | |
| (click)="openUserRolesForm(item)" | |
| title="Изменить роль" | |
| aria-label="Изменить роль" | |
| > | |
| <i class="bi bi-person-gear"></i> | |
| </button> | |
| <button | |
| *ngIf="item.isLockedOut" | |
| type="button" | |
| class="btn btn-outline-success btn-sm" | |
| (click)="unlockUser(item)" | |
| title="Разблокировать" | |
| aria-label="Разблокировать" | |
| > | |
| <i class="bi bi-unlock"></i> | |
| </button> | |
| <button | |
| *ngIf=" | |
| item.failedLoginAttempts && item.failedLoginAttempts > 0 | |
| " | |
| type="button" | |
| class="btn btn-outline-warning btn-sm" | |
| (click)="resetFailedAttempts(item)" | |
| title="Сбросить попытки" | |
| aria-label="Сбросить попытки" | |
| > | |
| <i class="bi bi-arrow-counterclockwise"></i> | |
| </button> | |
| <button | |
| *ngIf="!item.emailConfirmed" | |
| type="button" | |
| class="btn btn-outline-info btn-sm" | |
| (click)="resendVerification(item)" | |
| title="Отправить письмо" | |
| aria-label="Отправить письмо" | |
| > | |
| <i class="bi bi-envelope"></i> | |
| </button> | |
| <button | |
| *ngIf="!item.emailConfirmed" | |
| type="button" | |
| class="btn btn-outline-success btn-sm" | |
| (click)="forceVerifyEmail(item)" | |
| title="Подтвердить email" | |
| aria-label="Подтвердить email" | |
| > | |
| <i class="bi bi-check-circle"></i> | |
| </button> | |
| </div> |
🤖 Prompt for AI Agents
In
`@src/app/modules/admin/components/users/users-admin-page/users-admin-page.component.html`
around lines 168 - 215, The icon-only action buttons in the users admin row (the
buttons that call openUserRolesForm(item), unlockUser(item),
resetFailedAttempts(item), resendVerification(item), and forceVerifyEmail(item))
lack accessible names; add aria-label attributes to each button (e.g.,
aria-label="Change role", "Unlock user", "Reset failed attempts", "Resend
verification email", "Force verify email") matching the button's action/title so
screen readers can announce them.
| <button | ||
| type="button" | ||
| class="password-toggle" | ||
| (click)="togglePasswordVisibility()" | ||
| tabindex="-1" | ||
| > |
There was a problem hiding this comment.
Keep password visibility toggles keyboard-accessible.
tabindex="-1" removes the buttons from the tab order; remove it and add aria-label for screen readers.
♿ Suggested accessibility tweak
- tabindex="-1"
+ aria-label="Показать или скрыть пароль"
- tabindex="-1"
+ aria-label="Показать или скрыть подтверждение пароля"Also applies to: 143-148
🤖 Prompt for AI Agents
In `@src/app/modules/home/components/register-page/register-page.component.html`
around lines 104 - 109, Remove the tabindex="-1" from the password visibility
toggle buttons so they remain keyboard-focusable, and add an appropriate
aria-label (e.g., aria-label="Show password" / "Hide password" managed by the
component) to each button; update the click handler usage of
togglePasswordVisibility() in the register-page component to also set or reflect
the accessible label/state (and apply the same changes to the second
password-toggle button occurrence).
| <button | ||
| type="button" | ||
| class="password-toggle" | ||
| (click)="togglePasswordVisibility()" | ||
| tabindex="-1" | ||
| > | ||
| <i | ||
| class="bi" | ||
| [ngClass]="showPassword ? 'bi-eye-slash' : 'bi-eye'" | ||
| ></i> | ||
| </button> |
There was a problem hiding this comment.
Add aria-label to password toggle buttons for accessibility.
Screen reader users won't understand the purpose of these buttons without accessible labels.
♿ Proposed fix for the first toggle (lines 43-53)
<button
type="button"
class="password-toggle"
(click)="togglePasswordVisibility()"
tabindex="-1"
+ [attr.aria-label]="showPassword ? 'Hide password' : 'Show password'"
>♿ Proposed fix for the second toggle (lines 82-92)
<button
type="button"
class="password-toggle"
(click)="toggleConfirmPasswordVisibility()"
tabindex="-1"
+ [attr.aria-label]="showConfirmPassword ? 'Hide password' : 'Show password'"
>Also applies to: 82-92
🤖 Prompt for AI Agents
In
`@src/app/modules/home/components/reset-password-page/reset-password-page.component.html`
around lines 43 - 53, Add accessible labels to the password visibility toggle
buttons by adding an aria-label that reflects the current action (e.g., "Show
password" when showPassword is false and "Hide password" when showPassword is
true). Update the <button> elements that call togglePasswordVisibility() (the
password-toggle buttons referencing the showPassword property and
bi-eye/bi-eye-slash classes) to include an [attr.aria-label] binding that
toggles between the two labels, and apply the same change to the second toggle
button used elsewhere on the page.
| setTimeout(() => { | ||
| this.router.navigate(["/login"]); | ||
| }, 2000); |
There was a problem hiding this comment.
Potential memory leak with setTimeout if component is destroyed early.
If the user navigates away before the 2-second delay completes, the setTimeout callback will still fire and attempt navigation. Consider storing the timeout reference and clearing it in ngOnDestroy.
🔧 Proposed fix
Add a property to track the timeout:
+ private redirectTimeout: ReturnType<typeof setTimeout> | null = null;Update the success handler:
- setTimeout(() => {
- this.router.navigate(["/login"]);
- }, 2000);
+ this.redirectTimeout = setTimeout(() => {
+ this.router.navigate(["/login"]);
+ }, 2000);Clear in ngOnDestroy:
ngOnDestroy(): void {
+ if (this.redirectTimeout) {
+ clearTimeout(this.redirectTimeout);
+ }
this.titleService.resetTitle();
}🤖 Prompt for AI Agents
In
`@src/app/modules/home/components/reset-password-page/reset-password-page.component.ts`
around lines 68 - 70, The setTimeout used to call
this.router.navigate(["/login"]) can fire after the component is destroyed; add
a class property (e.g., private redirectTimeoutId: any) to store the return
value of setTimeout where it is invoked (in the success handler that currently
calls setTimeout), and implement ngOnDestroy() to
clearTimeout(this.redirectTimeoutId) and null it to avoid the callback running
after destruction; ensure you still call router.navigate only when the timeout
exists and component is alive.
| isLoading = true; | ||
| successMessage: string | null = null; | ||
| errorMessage: string | null = null; | ||
| email = ""; | ||
| canResend = false; | ||
| isResending = false; | ||
|
|
There was a problem hiding this comment.
Encode the token and clear the redirect timer on destroy.
This avoids URL path issues and prevents a delayed navigation after the component is torn down.
🛠️ Suggested fix
isLoading = true;
successMessage: string | null = null;
errorMessage: string | null = null;
email = "";
canResend = false;
isResending = false;
+ private redirectTimeoutId: ReturnType<typeof setTimeout> | null = null;
@@
this.http
.get<{ success: boolean; message: string }>(
- `${environment.resourceApiURI}/api/auth/verify-email/${token}`,
+ `${environment.resourceApiURI}/api/auth/verify-email/${encodeURIComponent(token)}`,
)
.pipe(untilDestroyed(this))
.subscribe({
next: () => {
this.isLoading = false;
this.successMessage =
"Email verified successfully! Redirecting to sign in...";
- setTimeout(() => {
+ this.redirectTimeoutId = setTimeout(() => {
this.router.navigate(["/login"], {
queryParams: { verified: "true" },
});
}, 2000);
},
@@
ngOnDestroy(): void {
+ if (this.redirectTimeoutId) {
+ clearTimeout(this.redirectTimeoutId);
+ this.redirectTimeoutId = null;
+ }
this.titleService.resetTitle();
}Also applies to: 46-70, 109-111
🤖 Prompt for AI Agents
In
`@src/app/modules/home/components/verify-email-page/verify-email-page.component.ts`
around lines 17 - 23, The component VerifyEmailPageComponent currently uses the
raw token in URLs and starts a delayed navigation that can fire after the
component is destroyed; update any place that builds a URL or route with the
token (e.g., where token is read in ngOnInit and where resend or verify links
are constructed) to wrap the token with encodeURIComponent(token) to avoid URL
path issues, and add a class property (e.g., redirectTimer: any) to store the
timer ID and implement ngOnDestroy to call clearTimeout(this.redirectTimer) so
the pending navigation is cancelled when the component is torn down; ensure any
places that set the timer (setTimeout) assign to this.redirectTimer.
| private getDeviceInfo(): string { | ||
| return navigator.userAgent; | ||
| } |
There was a problem hiding this comment.
Potential SSR issue with navigator.userAgent.
Direct access to navigator will throw a ReferenceError during server-side rendering since navigator is a browser-only global. Consider adding a platform check or providing a fallback.
🛠️ Proposed fix with SSR guard
+import { Injectable, PLATFORM_ID, Inject } from "@angular/core";
+import { isPlatformBrowser } from "@angular/common";
import { Observable } from "rxjs";
import { ApiService } from "./api.service";
// ... interfaces remain the same ...
`@Injectable`({
providedIn: "root",
})
export class AuthApiService {
- constructor(private readonly api: ApiService) {}
+ private isBrowser: boolean;
+
+ constructor(
+ private readonly api: ApiService,
+ `@Inject`(PLATFORM_ID) platformId: Object,
+ ) {
+ this.isBrowser = isPlatformBrowser(platformId);
+ }
// ... other methods remain the same ...
private getDeviceInfo(): string {
- return navigator.userAgent;
+ return this.isBrowser ? navigator.userAgent : 'SSR';
}
}🤖 Prompt for AI Agents
In `@src/app/services/auth-api.service.ts` around lines 104 - 106, The
getDeviceInfo method directly accesses navigator.userAgent which will throw
during SSR; update getDeviceInfo to guard access (e.g., check typeof navigator
!== 'undefined' or typeof window !== 'undefined' or use globalThis) and return a
safe fallback string (like empty string or 'server') when navigator is not
present so server rendering won't error; locate and modify the getDeviceInfo()
function to implement this guard and fallback.
Summary by CodeRabbit
Release Notes
New Features
Refactor
Style
✏️ Tip: You can customize this high-level summary in your review settings.