fix(frontend): fixes library health pinging even when the user is not authenticated#385
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughWebsocket subscription is started once auth initialization completes; a separate reactive effect triggers an explicit HTTP health fetch only when Changes
Sequence Diagram(s)sequenceDiagram
actor Browser/App
participant AuthInit as AuthInitializationService
participant Auth as AuthService
participant AppComp as AppComponent
participant LHS as LibraryHealthService
participant WS as WebSocket (/topic/library-health)
participant QC as QueryClient
Browser/App ->> AuthInit: boot
AuthInit -->> AppComp: initialized() (auth ready)
AppComp ->> LHS: initWebsocket()
LHS ->> WS: subscribe()
WS -->> LHS: message(payload)
LHS ->> QC: setQueryData(LIBRARY_HEALTH_QUERY_KEY, payload.libraryHealth)
Note over Auth,AppComp: reactive authenticated effect
Auth -->> AppComp: isAuthenticated() -> true
AppComp ->> LHS: fetchHealth()
LHS ->> QC: fetchQuery(health)
QC -->> LHS: health response
LHS ->> QC: setQueryData(LIBRARY_HEALTH_QUERY_KEY, response)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
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/features/book/service/library-health.service.ts (1)
35-40:⚠️ Potential issue | 🟡 MinorWebSocket subscription is not tracked for cleanup.
The subscription returned by
this.rxStompService.watch(...).subscribe(...)is not stored, so it cannot be unsubscribed when the service is destroyed. While root-provided services typically live for the app's lifetime, this pattern can lead to memory leaks if the service is ever recreated or in testing scenarios.🛡️ Proposed fix to track the subscription
+import {computed, DestroyRef, inject, Injectable} from '@angular/core'; +import {takeUntilDestroyed} from '@angular/core/rxjs-interop'; ... `@Injectable`({providedIn: 'root'}) export class LibraryHealthService { private readonly url = `${API_CONFIG.BASE_URL}/api/v1/libraries/health`; private http = inject(HttpClient); private rxStompService = inject(RxStompService); private queryClient = inject(QueryClient); + private destroyRef = inject(DestroyRef); ... initialize(): void { - this.rxStompService.watch('/topic/library-health').subscribe(msg => { + this.rxStompService.watch('/topic/library-health').pipe( + takeUntilDestroyed(this.destroyRef) + ).subscribe(msg => { const payload = JSON.parse(msg.body); this.queryClient.setQueryData(LIBRARY_HEALTH_QUERY_KEY, payload.libraryHealth); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/features/book/service/library-health.service.ts` around lines 35 - 40, The WebSocket subscription created in initialize() via this.rxStompService.watch(...).subscribe(...) is not stored for cleanup; capture the returned Subscription (e.g., a private field like rxStompSubscription: Subscription) when calling initialize(), import Subscription from 'rxjs', and then unsubscribe in a destroy lifecycle method (implement ngOnDestroy() or a public destroy() that calls this.rxStompSubscription.unsubscribe() if set) to avoid memory leaks; update initialize(), add the private field, import, and the unsubscribe logic referencing initialize(), rxStompService.watch, and rxStompSubscription.
🤖 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/features/book/service/library-health.service.ts`:
- Around line 31-33: The test expects initialize() to perform the HTTP fetch but
after refactor initialize() only sets up the websocket subscription and
fetchHealth() does the HTTP call; update the spec so after calling
initializeService() it also calls libraryHealthService.fetchHealth() (or replace
the assertion to only verify websocket subscription behavior) and assert that
httpGetSpy is called by that fetchHealth() invocation; reference the service
methods initialize() and fetchHealth() and the test spies (e.g., httpGetSpy /
initializeService()) when making the change.
---
Outside diff comments:
In `@frontend/src/app/features/book/service/library-health.service.ts`:
- Around line 35-40: The WebSocket subscription created in initialize() via
this.rxStompService.watch(...).subscribe(...) is not stored for cleanup; capture
the returned Subscription (e.g., a private field like rxStompSubscription:
Subscription) when calling initialize(), import Subscription from 'rxjs', and
then unsubscribe in a destroy lifecycle method (implement ngOnDestroy() or a
public destroy() that calls this.rxStompSubscription.unsubscribe() if set) to
avoid memory leaks; update initialize(), add the private field, import, and the
unsubscribe logic referencing initialize(), rxStompService.watch, and
rxStompSubscription.
🪄 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: d23ffec7-ab7c-4680-b0bd-d565f165ac55
📒 Files selected for processing (4)
frontend/src/app/app.component.spec.tsfrontend/src/app/app.component.tsfrontend/src/app/features/book/service/library-health.service.tsfrontend/src/app/shared/service/auth.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). (3)
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Analyze (java-kotlin)
🧰 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/app.component.tsfrontend/src/app/shared/service/auth.service.tsfrontend/src/app/features/book/service/library-health.service.tsfrontend/src/app/app.component.spec.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/app.component.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/app.component.tsfrontend/src/app/shared/service/auth.service.tsfrontend/src/app/features/book/service/library-health.service.tsfrontend/src/app/app.component.spec.ts
frontend/src/**/*.{test,spec}.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use Vitest for tests in the frontend
Files:
frontend/src/app/app.component.spec.ts
🔇 Additional comments (4)
frontend/src/app/shared/service/auth.service.ts (1)
24-25: LGTM!Clean addition of
isAuthenticatedas a computed signal derived fromtoken. This reactive approach integrates well with Angular's signal-based architecture and enables theauthenticatedEffectinAppComponentto respond to authentication changes.frontend/src/app/app.component.ts (1)
60-65: LGTM!The
authenticatedEffectcorrectly gates thefetchHealth()call behind an authentication check, which directly addresses issue#378by preventing health endpoint pings when unauthenticated. The reactive effect pattern ensuresfetchHealth()is called whenever the user becomes authenticated (including after re-login).frontend/src/app/app.component.spec.ts (2)
149-155: Good test coverage for the unauthenticated scenario.This test directly validates the fix for issue
#378by confirming thatfetchHealth()is not called when the user is unauthenticated, whileinitialize()(websocket setup) still runs. This prevents the infinite crash loop described in the issue.
64-67: Clean test configuration refactor.The
authparameter structure{ ready, authenticated }cleanly separates initialization state from authentication state, making tests more expressive and easier to maintain.
b9b1c87 to
fe20205
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/src/app/features/book/service/library-health.service.spec.ts (1)
40-44: Optional: simplifyinitWebsocket()helper to a direct call.
initWebsocket()is currently wrapped with a spy/await pattern, but the helper can stay synchronous and clearer.♻️ Suggested simplification
- async function initWebsocket(): Promise<void> { - const initializeSpy = vi.spyOn(service, 'initWebsocket'); - service.initWebsocket(); - await initializeSpy.mock.results[0]?.value; - } + function initWebsocket(): void { + service.initWebsocket(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/features/book/service/library-health.service.spec.ts` around lines 40 - 44, The test helper initWebsocket currently creates a spy (initializeSpy = vi.spyOn(service, 'initWebsocket')) and awaits the spy.mock.results[0]?.value; simplify it by removing the spy and directly invoking and awaiting the real method: replace the spy/await pattern with a direct call to service.initWebsocket() (e.g., await service.initWebsocket() if the method returns a Promise) and remove initializeSpy and any mock.results usage to keep the helper synchronous/clear.
🤖 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/features/book/service/library-health.service.spec.ts`:
- Around line 40-44: The test helper initWebsocket currently creates a spy
(initializeSpy = vi.spyOn(service, 'initWebsocket')) and awaits the
spy.mock.results[0]?.value; simplify it by removing the spy and directly
invoking and awaiting the real method: replace the spy/await pattern with a
direct call to service.initWebsocket() (e.g., await service.initWebsocket() if
the method returns a Promise) and remove initializeSpy and any mock.results
usage to keep the helper synchronous/clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0cb2a41f-6852-4aa2-b089-6fcdc02fb3bb
📒 Files selected for processing (4)
frontend/src/app/app.component.spec.tsfrontend/src/app/app.component.tsfrontend/src/app/features/book/service/library-health.service.spec.tsfrontend/src/app/features/book/service/library-health.service.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/app/app.component.ts
- frontend/src/app/features/book/service/library-health.service.ts
- frontend/src/app/app.component.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). (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/features/book/service/library-health.service.spec.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/features/book/service/library-health.service.spec.ts
frontend/src/**/*.{test,spec}.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use Vitest for tests in the frontend
Files:
frontend/src/app/features/book/service/library-health.service.spec.ts
🔇 Additional comments (3)
frontend/src/app/features/book/service/library-health.service.spec.ts (3)
34-38: Good synchronization forfetchHealth()assertions.Waiting on the
fetchQuerypromise before asserting state is a solid way to avoid timing-related test flakes.
46-77: Nice update of health-path tests to usefetchHealth().These cases now correctly validate the new split API behavior while preserving expected health/unhealthy outcomes.
79-87: Websocket state-update coverage looks correct after method split.Calling websocket init before emitting test messages keeps this scenario aligned with runtime behavior.
fe20205 to
bec7392
Compare
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/app.component.ts`:
- Around line 60-65: The authenticatedEffect calls
libraryHealthService.fetchHealth() solely based on
authService.isAuthenticated(), but it must also wait for auth bootstrap to
complete; update authenticatedEffect to gate the call with
authInit.initialized() (same pattern used in syncAuthInitializationEffect) so
that fetchHealth() runs only when both authInit.initialized() is true and
isAuthenticated is true; locate and modify the authenticatedEffect function to
include the initialization check alongside the existing isAuthenticated
condition.
- Around line 55-56: initWebsocket() currently creates an internal subscription
to /topic/library-health but returns void so the component can't track or tear
it down; update LibraryHealthService.initWebsocket() to return the Subscription
object (rxjs.Subscription) instead of void, then in AppComponent where you call
this.libraryHealthService.initWebsocket() capture that return and push it into
the existing subscriptions array (the same array unsubscribed in ngOnDestroy());
this ensures the websocket listener is tracked and will be unsubscribed when the
component is destroyed.
🪄 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: add42bf1-ef18-4132-9b93-5e34a0879b38
📒 Files selected for processing (4)
frontend/src/app/app.component.spec.tsfrontend/src/app/app.component.tsfrontend/src/app/features/book/service/library-health.service.spec.tsfrontend/src/app/features/book/service/library-health.service.ts
✅ Files skipped from review due to trivial changes (1)
- frontend/src/app/features/book/service/library-health.service.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/app/features/book/service/library-health.service.ts
- frontend/src/app/app.component.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: Analyze (java-kotlin)
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Test Suite / Backend Tests
🧰 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/app.component.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/app.component.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/app.component.ts
zachyale
left a comment
There was a problem hiding this comment.
@NamLe029 Let me know if you plan on modifying this PR further to handle subscription teardown for the LibraryHealthService now, or if you plan on making a follow up ticket.
If the PR is feature complete now, I'm happy to approve as-is and merge.
|
should be done rn. ready to merge |
… authenticated (grimmory-tools#385) * fix(frontend): library health pings even not authenticated * test * authInitialized guard * subscription
… authenticated (grimmory-tools#385) * fix(frontend): library health pings even not authenticated * test * authInitialized guard * subscription
… authenticated (#385) * fix(frontend): library health pings even not authenticated * test * authInitialized guard * subscription
… authenticated (#385) * fix(frontend): library health pings even not authenticated * test * authInitialized guard * subscription
… authenticated (grimmory-tools#385) * fix(frontend): library health pings even not authenticated * test * authInitialized guard * subscription
Description
Fixes library health pinging even when the user is not authenticated.
Linked Issue: Fixes #378
Changes
initialize()inLibraryHealthServiceinto two separate methods:initialize()for WebSocket subscription setup, andfetchHealth()for the actual health queryauthenticatedEffectinAppComponentthat callsfetchHealth()reactively, only when the user is authenticatedisAuthenticatedcomputed signal toAuthServicederived from the existingtokensignalSummary by CodeRabbit
Refactor
Tests