fix(auth): add global error handler and PWA update service, and update auth logout to force full page reload (prevent reload-hell)#741
Merged
Conversation
…e auth logout to force full page reload (prevent reload-hell)
Contributor
📝 WalkthroughWalkthroughAdds guarded global chunk/module-load error handling and a PWA update service that trigger controlled page reloads; refactors auth state to Angular signals with logout re-entrancy guards and a redirect helper; excludes Changes
Sequence Diagram(s)sequenceDiagram
participant App as Angular App
participant GEH as GlobalErrorHandler
participant Storage as localStorage
participant Browser as Browser/Window
App->>GEH: Error occurs (e.g. chunk/module load)
activate GEH
GEH->>GEH: Normalize error (name/message)
alt Chunk/Module Load Error
GEH->>Storage: Read `last_chunk_error_reload`
alt No recent reload (missing or >10s)
GEH->>Storage: Write current timestamp
GEH->>Browser: window.location.reload()
else Recent reload (<10s)
GEH->>GEH: Log — reload suppressed
end
else Other Error
GEH->>GEH: Log unexpected error (no reload)
end
deactivate GEH
sequenceDiagram
participant SwUpdate as SwUpdate
participant Pwa as PwaUpdateService
participant Storage as sessionStorage
participant Browser as Browser/Window
SwUpdate->>Pwa: Emit VERSION_READY / unrecoverable event
activate Pwa
Pwa->>Pwa: Compute guard key (hash or reason)
Pwa->>Storage: Check `pwa_reload_guard` entry
alt Allow reload (no recent entry)
Pwa->>Storage: Write guard entry with timestamp
Pwa->>Browser: window.location.reload()
else Suppress reload (recent entry)
Pwa->>Pwa: Log — reload suppressed
end
deactivate Pwa
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
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 unit tests (beta)
✨ Simplify code
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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/app/shared/service/auth.service.ts (1)
141-143:⚠️ Potential issue | 🔴 CriticalCritical: signal truthiness check will never run
handleSuccessfulAuth().After the refactor,
this.postLoginInitializedis aSignal<boolean>(a function reference), not a boolean.!this.postLoginInitializedis therefore alwaysfalse, sohandleSuccessfulAuth()is never invoked frominitializeWebSocketConnection(). That breaks the bootstrap path where a user already has a valid token (e.g., page refresh viawebsocketInitializer) —postLoginInitializer.initialize()will no longer run outside of an explicit login. This also appears to contradict the existing spec atauth.service.spec.tslines 167–176 which expectspostLoginInitializer.initializeto have been called once afterinitializeWebSocketConnection().🐛 Proposed fix
- if (!this.postLoginInitialized) { - this.handleSuccessfulAuth(); - } + if (!this._postLoginInitialized()) { + this.handleSuccessfulAuth(); + }(
handleSuccessfulAuth()is already idempotent via its own signal guard, so this simply restores the intended behavior.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/shared/service/auth.service.ts` around lines 141 - 143, The condition in initializeWebSocketConnection is checking the Signal object itself (this.postLoginInitialized) instead of its boolean value, so handleSuccessfulAuth() never runs; update the check to read the signal value (e.g., call this.postLoginInitialized() or use .value depending on your Signal API) so that if the signal is false the code calls handleSuccessfulAuth(); ensure you only change the conditional in initializeWebSocketConnection (referencing postLoginInitialized and handleSuccessfulAuth) so postLoginInitializer.initialize behavior expected by the tests (auth.service.spec.ts) is restored.
🧹 Nitpick comments (1)
frontend/src/app/core/services/pwa-update.service.ts (1)
13-23: Bind the subscriptions to the service lifetime.Even for a root service, using
takeUntilDestroyed()keeps teardown behavior explicit and consistent with the project’s Angular patterns.♻️ Suggested teardown pattern
-import { inject, Injectable } from '@angular/core'; +import { DestroyRef, inject, Injectable } from '@angular/core'; +import { takeUntilDestroyed } from '@angular/core/rxjs-interop'; import { SwUpdate, VersionReadyEvent } from '@angular/service-worker'; import { filter } from 'rxjs/operators'; export class PwaUpdateService { - private swUpdate = inject(SwUpdate); + private swUpdate = inject(SwUpdate); + private destroyRef = inject(DestroyRef); constructor() { if (this.swUpdate.isEnabled) { this.swUpdate.versionUpdates - .pipe(filter((evt): evt is VersionReadyEvent => evt.type === 'VERSION_READY')) + .pipe( + filter((evt): evt is VersionReadyEvent => evt.type === 'VERSION_READY'), + takeUntilDestroyed(this.destroyRef), + ) .subscribe(() => { console.info('New version available. Refreshing...'); window.location.reload(); }); - this.swUpdate.unrecoverable.subscribe(event => { - console.error('Service Worker entered unrecoverable state:', event.reason); - window.location.reload(); - }); + this.swUpdate.unrecoverable + .pipe(takeUntilDestroyed(this.destroyRef)) + .subscribe(event => { + console.error('Service Worker entered unrecoverable state:', event.reason); + window.location.reload(); + }); } } }Based on learnings: Prefer
DestroyRefwithtakeUntilDestroyed(destroyRef)for teardown in Angular v16+ instead of manually tracking subscriptions and cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/core/services/pwa-update.service.ts` around lines 13 - 23, The two ServiceWorker subscriptions (swUpdate.versionUpdates and swUpdate.unrecoverable in PwaUpdateService) must be bound to the service lifetime using Angular's takeUntilDestroyed pattern: inject DestroyRef into the service constructor (e.g., destroyRef) and change the observables to call .pipe(takeUntilDestroyed(destroyRef), filter(...)) for versionUpdates and .pipe(takeUntilDestroyed(destroyRef)) for unrecoverable before subscribing so the subscriptions are automatically torn down with the service.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/app/core/errors/global-error-handler.ts`:
- Around line 3-29: This file uses 4-space indentation; convert all indentation
to 2 spaces throughout the GlobalErrorHandler class (including the handleError
method, conditional blocks, and inner scopes) so it matches project style for
TypeScript files; update leading spaces on every line in this file to 2-space
indents and ensure alignment for the regex declarations, if/else blocks, and
localStorage/reload logic to preserve current code structure while changing only
indentation.
- Around line 5-24: The handleError method should be made defensive so recovery
never throws: ensure you safely coerce the incoming error (handle non-Error
types) before reading message/name, guard the
chunkFailedMessage/moduleFailedMessage regex tests by using a string fallback,
and wrap all localStorage.getItem/setItem and window.location.reload calls in
try/catch; when reading last_reload use Number(...) or parseInt with a NaN check
and fallback to 0 so non-numeric values don’t break logic, and always swallow
any exceptions during the reload-prevention logic so the original handler
completes without throwing (references: handleError, chunkFailedMessage,
moduleFailedMessage, localStorage.getItem, localStorage.setItem,
window.location.reload).
- Around line 10-13: Update the dynamic-import failure detection in
global-error-handler.ts: extend the existing chunkFailedMessage and
moduleFailedMessage checks (symbols: chunkFailedMessage, moduleFailedMessage,
and the name === 'ChunkLoadError' branch) to also match native browser messages
"Failed to fetch dynamically imported module" and "Importing a module script
failed" so those errors are caught, and reformat the file to follow project
style (use 2-space indentation throughout) so the new checks and surrounding
code use 2-space indentation.
In `@frontend/src/app/core/services/pwa-update.service.ts`:
- Around line 5-26: The file uses 4-space indentation; reformat to 2-space
indentation throughout this TypeScript file. Update indentation for the
PwaUpdateService class, the private swUpdate = inject(SwUpdate); declaration,
the constructor block, and the nested if (this.swUpdate.isEnabled) { ... } body
including the versionUpdates.pipe(...) subscribe and
unrecoverable.subscribe(...) callbacks so every level uses 2 spaces per indent
to match the project's frontend/src/**/*.{ts,tsx,html,scss} style rule.
- Around line 13-23: Replace the unconditional reloads and 4-space indentation:
for the VERSION_READY handler (swUpdate.versionUpdates piped to filter for
VersionReadyEvent) read the event.latestVersion.hash and use a sessionStorage
key like "pwa-reload:<hash>" to only call window.location.reload() once per new
hash; for swUpdate.unrecoverable subscribe, build a sessionStorage key based on
event.reason (e.g., "pwa-unrecoverable:<reason>") and check/set that key with a
short TTL-like timestamp or flag to prevent immediate repeated reloads before
calling window.location.reload(); also reformat the block to 2-space indentation
to match project style.
In `@frontend/src/app/shared/service/auth.service.ts`:
- Around line 104-111: The re-entrancy guard in forceLogout makes it a no-op:
change forceLogout to (1) stop resetting this._logoutInProgress to false so the
flag remains set (remove the trailing set(false) line), and (2) ensure
forceLogout always performs the redirect/clearSession even if
this._logoutInProgress() is true (i.e., don't early-return silently); keep using
clearSession() and
window.location.replace(`/login?reason=${encodeURIComponent(reason)}`) so a
concurrent logout() in-flight won't prevent the forced redirect — update the
logic around this._logoutInProgress(), forceLogout(reason), and interactions
with logout() accordingly.
---
Outside diff comments:
In `@frontend/src/app/shared/service/auth.service.ts`:
- Around line 141-143: The condition in initializeWebSocketConnection is
checking the Signal object itself (this.postLoginInitialized) instead of its
boolean value, so handleSuccessfulAuth() never runs; update the check to read
the signal value (e.g., call this.postLoginInitialized() or use .value depending
on your Signal API) so that if the signal is false the code calls
handleSuccessfulAuth(); ensure you only change the conditional in
initializeWebSocketConnection (referencing postLoginInitialized and
handleSuccessfulAuth) so postLoginInitializer.initialize behavior expected by
the tests (auth.service.spec.ts) is restored.
---
Nitpick comments:
In `@frontend/src/app/core/services/pwa-update.service.ts`:
- Around line 13-23: The two ServiceWorker subscriptions
(swUpdate.versionUpdates and swUpdate.unrecoverable in PwaUpdateService) must be
bound to the service lifetime using Angular's takeUntilDestroyed pattern: inject
DestroyRef into the service constructor (e.g., destroyRef) and change the
observables to call .pipe(takeUntilDestroyed(destroyRef), filter(...)) for
versionUpdates and .pipe(takeUntilDestroyed(destroyRef)) for unrecoverable
before subscribing so the subscriptions are automatically torn down with the
service.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 15c2a415-9e73-40ed-bf6a-0a63c22e2a08
📒 Files selected for processing (8)
frontend/src/app/core/errors/global-error-handler.tsfrontend/src/app/core/security/auth-interceptor.service.spec.tsfrontend/src/app/core/security/auth-interceptor.service.tsfrontend/src/app/core/services/pwa-update.service.tsfrontend/src/app/features/author-browser/components/author-detail/author-detail.component.tsfrontend/src/app/shared/service/auth.service.spec.tsfrontend/src/app/shared/service/auth.service.tsfrontend/src/main.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Frontend Lint Threshold Check
🧰 Additional context used
📓 Path-based instructions (4)
frontend/src/**/*.{ts,tsx,html,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Use 2-space indentation in TypeScript, HTML, and SCSS files
Files:
frontend/src/app/core/security/auth-interceptor.service.tsfrontend/src/app/features/author-browser/components/author-detail/author-detail.component.tsfrontend/src/app/shared/service/auth.service.spec.tsfrontend/src/app/core/security/auth-interceptor.service.spec.tsfrontend/src/app/core/services/pwa-update.service.tsfrontend/src/app/shared/service/auth.service.tsfrontend/src/main.tsfrontend/src/app/core/errors/global-error-handler.ts
frontend/src/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
frontend/src/app/**/*.{ts,tsx}: Preferinject()over constructor injection
Followfrontend/eslint.config.js: component selectors useapp-*, directive selectors useapp*, andanyis disallowed
Files:
frontend/src/app/core/security/auth-interceptor.service.tsfrontend/src/app/features/author-browser/components/author-detail/author-detail.component.tsfrontend/src/app/shared/service/auth.service.spec.tsfrontend/src/app/core/security/auth-interceptor.service.spec.tsfrontend/src/app/core/services/pwa-update.service.tsfrontend/src/app/shared/service/auth.service.tsfrontend/src/app/core/errors/global-error-handler.ts
frontend/src/app/**/*.component.ts
📄 CodeRabbit inference engine (AGENTS.md)
Keep Angular code on standalone components. Do not add NgModules
Files:
frontend/src/app/features/author-browser/components/author-detail/author-detail.component.ts
frontend/src/**/*.{test,spec}.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use Vitest for tests in the frontend
Files:
frontend/src/app/shared/service/auth.service.spec.tsfrontend/src/app/core/security/auth-interceptor.service.spec.ts
🧠 Learnings (6)
📚 Learning: 2026-04-05T21:16:01.715Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 385
File: frontend/src/app/app.component.ts:55-56
Timestamp: 2026-04-05T21:16:01.715Z
Learning: When reviewing code in the Grimmory frontend (Angular), prefer modern Angular patterns. Specifically: (1) Prefer `DestroyRef` with `takeUntilDestroyed(destroyRef)` for teardown in Angular v16+ instead of manually tracking `Subscription` arrays and calling `unsubscribe()` in `ngOnDestroy()`. (2) Prefer `inject()` for dependency injection over constructor injection where appropriate. (3) Prefer Angular signals (e.g., `signal`, `computed`) over `BehaviorSubject`/`Observable` for state where signals/computed values fit the use case. Flag older patterns when they can be replaced with these modern equivalents without changing behavior.
Applied to files:
frontend/src/app/core/security/auth-interceptor.service.tsfrontend/src/app/features/author-browser/components/author-detail/author-detail.component.tsfrontend/src/app/shared/service/auth.service.spec.tsfrontend/src/app/core/security/auth-interceptor.service.spec.tsfrontend/src/app/core/services/pwa-update.service.tsfrontend/src/app/shared/service/auth.service.tsfrontend/src/main.tsfrontend/src/app/core/errors/global-error-handler.ts
📚 Learning: 2026-04-07T09:28:09.587Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 393
File: frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts:255-263
Timestamp: 2026-04-07T09:28:09.587Z
Learning: In this Angular frontend (under frontend/src/app/), flag manual resource management/cleanup patterns when there is an Angular v16+ automatic alternative. Examples to prefer: (1) Instead of manually pairing document/window event listeners with stored cleanup functions (e.g., add/removeEventListener with mouseMoveCleanup/documentClickCleanup/keydownCleanup/touchCleanup fields), register teardown via DestroyRef.onDestroy(cleanupFn) (or equivalent Angular v16+ teardown mechanism). (2) Instead of storing Subscriptions in fields and explicitly unsubscribing in ngOnDestroy (e.g., annotationSaveSubscription/annotationCacheSubscription), use takeUntilDestroyed(destroyRef) (piped into the observable) or other Angular v16+ primitives. (3) If teardown is lifecycle-coupled and can be automated via DestroyRef/takeUntilDestroyed/signals (or other Angular v16+ mechanisms), prefer the automated approach over manual ngOnDestroy cleanup. Raise a review finding for the manual pattern and recommend the aut...
Applied to files:
frontend/src/app/core/security/auth-interceptor.service.tsfrontend/src/app/features/author-browser/components/author-detail/author-detail.component.tsfrontend/src/app/shared/service/auth.service.spec.tsfrontend/src/app/core/security/auth-interceptor.service.spec.tsfrontend/src/app/core/services/pwa-update.service.tsfrontend/src/app/shared/service/auth.service.tsfrontend/src/app/core/errors/global-error-handler.ts
📚 Learning: 2026-04-11T03:55:57.229Z
Learnt from: zachyale
Repo: grimmory-tools/grimmory PR: 439
File: frontend/src/app/features/series-browser/components/series-browser/series-browser.component.ts:178-196
Timestamp: 2026-04-11T03:55:57.229Z
Learning: In this Angular frontend (frontend/src/app/), prefer the team’s reactive i18n “signal/computed” pattern: (1) For individual reactive translated strings, use `translateSignal()` from `jsverse/transloco`. (2) For option/label arrays that must update on language switch, create a single `activeLang` signal with `toSignal(t.langChanges$, { initialValue: t.getActiveLang() })`, then derive the arrays as `computed()` signals that read `activeLang()`. This should avoid manual `langChanges$` subscriptions and any `ngOnDestroy` subscription cleanup; prefer this over subscribing in `ngOnInit` when implementing reactive localization.
Applied to files:
frontend/src/app/core/security/auth-interceptor.service.tsfrontend/src/app/features/author-browser/components/author-detail/author-detail.component.tsfrontend/src/app/shared/service/auth.service.spec.tsfrontend/src/app/core/security/auth-interceptor.service.spec.tsfrontend/src/app/core/services/pwa-update.service.tsfrontend/src/app/shared/service/auth.service.tsfrontend/src/app/core/errors/global-error-handler.ts
📚 Learning: 2026-03-26T01:46:48.863Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-26T01:46:48.863Z
Learning: Applies to frontend/src/app/**/*.component.ts : Keep Angular code on standalone components. Do not add NgModules
Applied to files:
frontend/src/app/features/author-browser/components/author-detail/author-detail.component.ts
📚 Learning: 2026-03-26T01:46:48.863Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-26T01:46:48.863Z
Learning: Applies to frontend/src/**/*.{test,spec}.ts : Use Vitest for tests in the frontend
Applied to files:
frontend/src/app/shared/service/auth.service.spec.tsfrontend/src/app/core/security/auth-interceptor.service.spec.ts
📚 Learning: 2026-03-25T19:09:09.638Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 189
File: booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java:113-116
Timestamp: 2026-03-25T19:09:09.638Z
Learning: In `booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java`, when deciding whether to forward the `Authorization` header to the upstream Kobo server proxy on the `/v1/library/sync` endpoint, a simple null check (`request.getHeader(HttpHeaders.AUTHORIZATION) != null`) is the correct guard. The header is either absent or present — if present it may or may not be a valid token, but validity is determined by the upstream Kobo server, not by Grimmory. Blank/whitespace intermediate states do not occur in practice, and pre-validating the token value before forwarding is not the responsibility of this service.
Applied to files:
frontend/src/app/core/security/auth-interceptor.service.spec.ts
🔇 Additional comments (5)
frontend/src/app/core/security/auth-interceptor.service.ts (1)
25-25: LGTM!Adding
/api/v1/public-settingsto the excluded auth paths is consistent with the endpoint definition inapp-settings.service.ts(publicApiUrl = ${API_CONFIG.BASE_URL}/api/v1/public-settings) and with the pre-auth fetch inauth-initializer.ts. Exact-match comparison onurlPath(line 28) correctly prevents prefix collisions.frontend/src/app/core/security/auth-interceptor.service.spec.ts (1)
215-219: LGTM!Test correctly asserts that
GET /api/v1/public-settingsbypasses the Authorization header injection even when a token is available, matching the new interceptor exclusion. Consistent with the surrounding exclusion tests in the samedescribeblock.frontend/src/app/features/author-browser/components/author-detail/author-detail.component.ts (1)
1-24: LGTM: Formatting-only changes to import statements.These changes only reformatted whitespace in import declarations without modifying any imported symbols, module paths, or component behavior. The formatting is consistent and improves readability. The existing component code already follows all modern Angular patterns (standalone components,
inject(), signals,DestroyRef).frontend/src/app/shared/service/auth.service.ts (1)
79-102: I'm ready to verify and rewrite a review comment. However, I don't see a review comment provided in the<review_comment>tags. Please provide the original review comment you'd like me to verify and rewrite, and I'll process it according to the required format.frontend/src/main.ts (1)
2-2: LGTM.The global error handler and PWA update service are wired into bootstrap cleanly, and the service worker provider is registered before the update service is initialized.
Also applies to: 16-17, 85-89
… reload guards for chunk errors and PWA updates
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/app/shared/service/auth.service.spec.ts`:
- Around line 53-54: Replace the cast-to-any when spying on the protected
redirectTo by declaring a narrow test-only type (e.g. type TestableAuthService =
Pick<AuthService, 'redirectTo'> or an interface with redirectTo(): void) and
cast the service to that type when calling vi.spyOn; update the three other spy
sites (lines ~117, ~133, ~144) to use the same TestableAuthService cast so no
any is used while still allowing spying on the protected redirectTo method.
In `@frontend/src/app/shared/service/auth.service.ts`:
- Around line 91-96: The remote logout path uses
this.redirectTo(response.logoutUrl) without setting the replace flag, leaving
the authenticated page in history; update the handling in auth.service.ts so
that when response.logoutUrl is present you call
this.redirectTo(response.logoutUrl, true) (same replace behavior as the
local/error logout branch) to replace the history entry and prevent the stale
page from remaining in browser history.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b4604d51-1b42-4344-bc03-07263f84d330
📒 Files selected for processing (4)
frontend/src/app/core/errors/global-error-handler.tsfrontend/src/app/core/services/pwa-update.service.tsfrontend/src/app/shared/service/auth.service.spec.tsfrontend/src/app/shared/service/auth.service.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/app/core/errors/global-error-handler.ts
- frontend/src/app/core/services/pwa-update.service.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (java-kotlin)
🧰 Additional context used
📓 Path-based instructions (3)
frontend/src/**/*.{ts,tsx,html,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Use 2-space indentation in TypeScript, HTML, and SCSS files
Files:
frontend/src/app/shared/service/auth.service.spec.tsfrontend/src/app/shared/service/auth.service.ts
frontend/src/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
frontend/src/app/**/*.{ts,tsx}: Preferinject()over constructor injection
Followfrontend/eslint.config.js: component selectors useapp-*, directive selectors useapp*, andanyis disallowed
Files:
frontend/src/app/shared/service/auth.service.spec.tsfrontend/src/app/shared/service/auth.service.ts
frontend/src/**/*.{test,spec}.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use Vitest for tests in the frontend
Files:
frontend/src/app/shared/service/auth.service.spec.ts
🧠 Learnings (4)
📚 Learning: 2026-03-26T01:46:48.863Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-26T01:46:48.863Z
Learning: Applies to frontend/src/**/*.{test,spec}.ts : Use Vitest for tests in the frontend
Applied to files:
frontend/src/app/shared/service/auth.service.spec.ts
📚 Learning: 2026-04-05T21:16:01.715Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 385
File: frontend/src/app/app.component.ts:55-56
Timestamp: 2026-04-05T21:16:01.715Z
Learning: When reviewing code in the Grimmory frontend (Angular), prefer modern Angular patterns. Specifically: (1) Prefer `DestroyRef` with `takeUntilDestroyed(destroyRef)` for teardown in Angular v16+ instead of manually tracking `Subscription` arrays and calling `unsubscribe()` in `ngOnDestroy()`. (2) Prefer `inject()` for dependency injection over constructor injection where appropriate. (3) Prefer Angular signals (e.g., `signal`, `computed`) over `BehaviorSubject`/`Observable` for state where signals/computed values fit the use case. Flag older patterns when they can be replaced with these modern equivalents without changing behavior.
Applied to files:
frontend/src/app/shared/service/auth.service.spec.tsfrontend/src/app/shared/service/auth.service.ts
📚 Learning: 2026-04-07T09:28:09.587Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 393
File: frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts:255-263
Timestamp: 2026-04-07T09:28:09.587Z
Learning: In this Angular frontend (under frontend/src/app/), flag manual resource management/cleanup patterns when there is an Angular v16+ automatic alternative. Examples to prefer: (1) Instead of manually pairing document/window event listeners with stored cleanup functions (e.g., add/removeEventListener with mouseMoveCleanup/documentClickCleanup/keydownCleanup/touchCleanup fields), register teardown via DestroyRef.onDestroy(cleanupFn) (or equivalent Angular v16+ teardown mechanism). (2) Instead of storing Subscriptions in fields and explicitly unsubscribing in ngOnDestroy (e.g., annotationSaveSubscription/annotationCacheSubscription), use takeUntilDestroyed(destroyRef) (piped into the observable) or other Angular v16+ primitives. (3) If teardown is lifecycle-coupled and can be automated via DestroyRef/takeUntilDestroyed/signals (or other Angular v16+ mechanisms), prefer the automated approach over manual ngOnDestroy cleanup. Raise a review finding for the manual pattern and recommend the aut...
Applied to files:
frontend/src/app/shared/service/auth.service.spec.tsfrontend/src/app/shared/service/auth.service.ts
📚 Learning: 2026-04-11T03:55:57.229Z
Learnt from: zachyale
Repo: grimmory-tools/grimmory PR: 439
File: frontend/src/app/features/series-browser/components/series-browser/series-browser.component.ts:178-196
Timestamp: 2026-04-11T03:55:57.229Z
Learning: In this Angular frontend (frontend/src/app/), prefer the team’s reactive i18n “signal/computed” pattern: (1) For individual reactive translated strings, use `translateSignal()` from `jsverse/transloco`. (2) For option/label arrays that must update on language switch, create a single `activeLang` signal with `toSignal(t.langChanges$, { initialValue: t.getActiveLang() })`, then derive the arrays as `computed()` signals that read `activeLang()`. This should avoid manual `langChanges$` subscriptions and any `ngOnDestroy` subscription cleanup; prefer this over subscribing in `ngOnInit` when implementing reactive localization.
Applied to files:
frontend/src/app/shared/service/auth.service.spec.tsfrontend/src/app/shared/service/auth.service.ts
…ty for redirectTo spy
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/src/app/shared/service/auth.service.ts (1)
104-116: Remove the no-opforceLogout()branch.
isStandardLogoutInFlightonly feeds an emptyifblock, and the comments mention a manual reset that never happens. Since the current behavior is to keep the flag set until page unload, simplify this to the actual intent.♻️ Proposed cleanup
forceLogout(reason: string): void { - const isStandardLogoutInFlight = this._logoutInProgress(); this._logoutInProgress.set(true); this.clearSession(); this.redirectTo(`/login?reason=${encodeURIComponent(reason)}`, true); - - // If a standard logout was NOT in flight, we need to reset the flag manually - // because forceLogout doesn't have an async pipeline to reset it. - // However, if we don't reset it, it protects against re-entry before page unloads. - if (isStandardLogoutInFlight) { - // We'll let the standard logout pipeline finish resetting it if it's still around - } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/shared/service/auth.service.ts` around lines 104 - 116, Remove the dead code in forceLogout: delete the unnecessary isStandardLogoutInFlight variable and the empty if (isStandardLogoutInFlight) block, and simplify the method to just set this._logoutInProgress (via this._logoutInProgress.set(true)), call clearSession() and redirectTo(...); update or remove the outdated comment about manual reset since the flag intentionally remains set until page unload. Ensure references are to forceLogout, this._logoutInProgress, clearSession, and redirectTo so the change is applied in the correct method.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/src/app/shared/service/auth.service.ts`:
- Around line 104-116: Remove the dead code in forceLogout: delete the
unnecessary isStandardLogoutInFlight variable and the empty if
(isStandardLogoutInFlight) block, and simplify the method to just set
this._logoutInProgress (via this._logoutInProgress.set(true)), call
clearSession() and redirectTo(...); update or remove the outdated comment about
manual reset since the flag intentionally remains set until page unload. Ensure
references are to forceLogout, this._logoutInProgress, clearSession, and
redirectTo so the change is applied in the correct method.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 838a514c-8fc5-4613-b15b-b9ab80267c14
📒 Files selected for processing (2)
frontend/src/app/shared/service/auth.service.spec.tsfrontend/src/app/shared/service/auth.service.ts
✅ Files skipped from review due to trivial changes (1)
- frontend/src/app/shared/service/auth.service.spec.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Analyze (java-kotlin)
🧰 Additional context used
📓 Path-based instructions (2)
frontend/src/**/*.{ts,tsx,html,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Use 2-space indentation in TypeScript, HTML, and SCSS files
Files:
frontend/src/app/shared/service/auth.service.ts
frontend/src/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
frontend/src/app/**/*.{ts,tsx}: Preferinject()over constructor injection
Followfrontend/eslint.config.js: component selectors useapp-*, directive selectors useapp*, andanyis disallowed
Files:
frontend/src/app/shared/service/auth.service.ts
🧠 Learnings (3)
📚 Learning: 2026-04-05T21:16:01.715Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 385
File: frontend/src/app/app.component.ts:55-56
Timestamp: 2026-04-05T21:16:01.715Z
Learning: When reviewing code in the Grimmory frontend (Angular), prefer modern Angular patterns. Specifically: (1) Prefer `DestroyRef` with `takeUntilDestroyed(destroyRef)` for teardown in Angular v16+ instead of manually tracking `Subscription` arrays and calling `unsubscribe()` in `ngOnDestroy()`. (2) Prefer `inject()` for dependency injection over constructor injection where appropriate. (3) Prefer Angular signals (e.g., `signal`, `computed`) over `BehaviorSubject`/`Observable` for state where signals/computed values fit the use case. Flag older patterns when they can be replaced with these modern equivalents without changing behavior.
Applied to files:
frontend/src/app/shared/service/auth.service.ts
📚 Learning: 2026-04-07T09:28:09.587Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 393
File: frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts:255-263
Timestamp: 2026-04-07T09:28:09.587Z
Learning: In this Angular frontend (under frontend/src/app/), flag manual resource management/cleanup patterns when there is an Angular v16+ automatic alternative. Examples to prefer: (1) Instead of manually pairing document/window event listeners with stored cleanup functions (e.g., add/removeEventListener with mouseMoveCleanup/documentClickCleanup/keydownCleanup/touchCleanup fields), register teardown via DestroyRef.onDestroy(cleanupFn) (or equivalent Angular v16+ teardown mechanism). (2) Instead of storing Subscriptions in fields and explicitly unsubscribing in ngOnDestroy (e.g., annotationSaveSubscription/annotationCacheSubscription), use takeUntilDestroyed(destroyRef) (piped into the observable) or other Angular v16+ primitives. (3) If teardown is lifecycle-coupled and can be automated via DestroyRef/takeUntilDestroyed/signals (or other Angular v16+ mechanisms), prefer the automated approach over manual ngOnDestroy cleanup. Raise a review finding for the manual pattern and recommend the aut...
Applied to files:
frontend/src/app/shared/service/auth.service.ts
📚 Learning: 2026-04-11T03:55:57.229Z
Learnt from: zachyale
Repo: grimmory-tools/grimmory PR: 439
File: frontend/src/app/features/series-browser/components/series-browser/series-browser.component.ts:178-196
Timestamp: 2026-04-11T03:55:57.229Z
Learning: In this Angular frontend (frontend/src/app/), prefer the team’s reactive i18n “signal/computed” pattern: (1) For individual reactive translated strings, use `translateSignal()` from `jsverse/transloco`. (2) For option/label arrays that must update on language switch, create a single `activeLang` signal with `toSignal(t.langChanges$, { initialValue: t.getActiveLang() })`, then derive the arrays as `computed()` signals that read `activeLang()`. This should avoid manual `langChanges$` subscriptions and any `ngOnDestroy` subscription cleanup; prefer this over subscribing in `ngOnInit` when implementing reactive localization.
Applied to files:
frontend/src/app/shared/service/auth.service.ts
zachyale
requested changes
Apr 22, 2026
zachyale
approved these changes
Apr 22, 2026
imnotjames
approved these changes
Apr 22, 2026
zachyale
pushed a commit
that referenced
this pull request
Apr 22, 2026
…e auth logout to force full page reload (prevent reload-hell) (#741)
dsmouse
pushed a commit
to dsmouse/grimmory
that referenced
this pull request
May 11, 2026
…e auth logout to force full page reload (prevent reload-hell) (grimmory-tools#741)
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
3 participants
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.
Description
Linked Issue: Fixes #
Changes
This pull request introduces several improvements to error handling, authentication, and service worker update management in the frontend application. The key changes include the addition of a global error handler for chunk/module loading failures, enhanced service worker update logic for automatic reloads, and refactoring of the authentication service to improve logout handling and state management. It also updates tests to reflect these changes and ensures certain API endpoints are correctly excluded from authentication.
Error Handling and Service Worker Updates
GlobalErrorHandler(global-error-handler.ts) to catch and handle chunk/module loading errors by forcing a reload, with safeguards to prevent reload loops.PwaUpdateServiceto detect new service worker versions or unrecoverable states and automatically reload the page, ensuring users always have the latest version.Authentication Service Refactoring
AuthServiceto use Angular signals forpostLoginInitializedandlogoutInProgressstate, improving reactivity and preventing concurrent logouts.window.location.replacefor navigation, simplifying the logout flow and ensuring a clean redirect.Authentication Interceptor Improvements
/api/v1/public-settingsto the list of endpoints excluded from authentication inAuthInterceptorService./api/v1/public-settingsare not intercepted for authentication.Summary by CodeRabbit
New Features
Bug Fixes
Tests