fix(ui): logout needs access token before clearing session

[imnotjames]

Description

The logout button clears the user's session so the auth token is removed before we make the request. This causes the logout request to fail, because the logout request requires the login token.

We can actually call the logout endpoint properly by waiting to clear the session until after the request completes

Linked Issue

fixes #2339

Changes

moves clearSession to after logout request completes

Manual Testing Steps

1. Logout
2. See request complete with 200

Screenshots (Optional)

Additional Context (Optional)

It's possible the endpoint isn't working as expected but a log out request requiring a valid access token makes sense to me.

AI Disclosure

None.

Checklist

• This PR links and implements an accepted issue.
• This PR is a single focused change.
• There are new or updated tests validating this change.
• I ran just ui check and just api check.
• I have added screenshots if there were any UI changes.
• I have disclosed any AI usage as per the organization AI Policy above.
• I understand all of my submitted changes.

Summary by CodeRabbit

• Bug Fixes
  • Improved logout handling by clearing the local session whether the logout request succeeds or fails.
  • Ensured users are redirected to the login screen after logout.
  • Updated refresh-session behavior to handle logout responses in the correct order.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 80c8ecf5-45e6-4df2-9ecd-0163ad4e18ef

📥 Commits

Reviewing files that changed from the base of the PR and between ef041dd and 9bdea5a.

📒 Files selected for processing (2)

• frontend/src/app/shared/service/auth.service.spec.ts
• frontend/src/app/shared/service/auth.service.ts

🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

• grimmory-tools/grimmory-docs (manual)

📜 Recent review details

⏰ Context from checks skipped due to timeout. (1)

• GitHub Check: Packaging Smoke Test

🧰 Additional context used

📓 Path-based instructions (3)

**/*

⚙️ CodeRabbit configuration file

**/*: This project is being developed using current and future-facing technologies:

• Java 25 with --enable-preview (preview features are INTENTIONAL and encouraged)
• Spring Boot 4 (latest major version, check APIs accordingly)
• Jackson 3 (new package: tools.jackson.* instead of com.fasterxml.jackson.*)
• Hibernate 7.3.x (Jakarta Persistence 3.2, new APIs; avoid deprecated Hibernate 5/6 patterns)
• Angular 21 (signals-based reactivity, no NgModules unless legacy)

Grimmory Internal Tools

• epub4j and pdfium4j are our own internal tools developed by the Grimmory team.
• Always verify behavior and API changes against the upstream repositories:
  • epub4j: https://github.com/grimmory-tools/epub4j
  • pdfium4j: https://github.com/grimmory-tools/pdfium4j
• If you encounter issues with these libraries, check if a fix exists in the upstream grimmory-tools organization.

Metadata Standards and Compliance

• For all metadata writing and parsing logic, double-check against Dublin Core and ANSI standards to ensure perfect official compliance.
• We strictly follow the widespread and official XML-compliant methods for EPUB2, EPUB3, CBX, and PDF formats.

General Java and Spring rules

• ALWAYS prefer modern, idiomatic Java 25 constructs over legacy patterns.
• Preview features (--enable-preview) are enabled and intentional; do NOT flag them as risky unless there is a concrete runtime issue.
• Prefer: records, sealed classes/interfaces, pattern matching (switch expressions, instanceof), structured concurrency (StructuredTaskScope), scoped values, string templates, unnamed patterns/variables.
• Prefer virtual threads (Thread.ofVirtual(), Executors.newVirtualThreadPerTaskExecutor()) over platform threads for I/O-bound work.
• Prefer the new Sequenced Collections API (SequencedCollection, SequencedMap) where applicable.
• Prefer var for local variables when the type is obvious from context.
• Use stream().toList() instead of stream().collect(Collectors.toList()) for imm...

Files:

• frontend/src/app/shared/service/auth.service.spec.ts
• frontend/src/app/shared/service/auth.service.ts

**/*.spec.ts

⚙️ CodeRabbit configuration file

**/*.spec.ts: Angular 21 test review:

• Flag tests with no expect() calls.
• Flag hardcoded async timeouts; prefer fakeAsync/tick or signal-based testing.
• Flag missing fixture.detectChanges() after state mutations.

Files:

• frontend/src/app/shared/service/auth.service.spec.ts

**/*.service.ts

⚙️ CodeRabbit configuration file

**/*.service.ts: Angular 21 service review:

• Prefer providedIn: 'root' unless scope is intentionally limited.
• Prefer inject() over constructor DI.
• Prefer Signals or lightweight RxJS (takeUntilDestroyed) over manual subscription management.
• Flag unsubscribed Observables (missing takeUntilDestroyed or explicit unsubscribe).
• Prefer typed HttpClient responses with explicit error handling (catchError).
• Flag any state mutation outside a defined signal or BehaviorSubject.

Files:

• frontend/src/app/shared/service/auth.service.ts

🧠 Learnings (5)

📚 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.ts
• 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.spec.ts
• 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.spec.ts
• frontend/src/app/shared/service/auth.service.ts

📚 Learning: 2026-05-18T14:54:39.422Z

Learnt from: alexhb1
Repo: grimmory-tools/grimmory PR: 1379
File: frontend/src/assets/styles/tailwind.css:3-4
Timestamp: 2026-05-18T14:54:39.422Z
Learning: In the grimmory-tools/grimmory repository, Biome is not used for linting/formatting (no `biome.json` and no Biome dependency in `package.json`). During code reviews, do not raise Biome-related issues or recommend adding/changing `biome.json`/Biome dependencies for formatting or linting in this project.

Applied to files:

• frontend/src/app/shared/service/auth.service.spec.ts
• frontend/src/app/shared/service/auth.service.ts

📚 Learning: 2026-06-30T01:30:43.644Z

Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1881
File: frontend/src/app/shared/components/icon-picker/icon-picker-component.ts:116-119
Timestamp: 2026-06-30T01:30:43.644Z
Learning: In the Grimmory Angular frontend (Angular 21), do not flag `[(ngModel)]` two-way bindings that are bound directly to a `WritableSignal`. Angular supports two-way binding to writable signals, so usages like `[(ngModel)]="svgSearchText"` should be treated as valid and should not be considered an incorrect overwrite of the signal with a plain value. Only flag if the binding target is not a `WritableSignal`.

Applied to files:

• frontend/src/app/shared/service/auth.service.spec.ts
• frontend/src/app/shared/service/auth.service.ts

🔇 Additional comments (2)

frontend/src/app/shared/service/auth.service.ts (1)

186-196: LGTM!

frontend/src/app/shared/service/auth.service.spec.ts (1)

189-189: LGTM!

---

Walkthrough

AuthService.logout() now clears the local session after both successful and failed logout requests. The stale-refresh test now completes the logout response before the delayed refresh response.

Changes

Logout session cleanup

Layer / File(s)	Summary
Logout flow and validation frontend/src/app/shared/service/auth.service.ts, frontend/src/app/shared/service/auth.service.spec.ts	logout() clears the session before remote URL handling on success and before redirecting on failure. The stale-refresh test flushes the logout response first.

Estimated code review effort: 2 (Simple) | ~5 minutes

Merge Risk: ⚪ Minimal · up to 9bdea

This localized logout change defers session clearing until the authenticated request completes; no actionable merge-blocking risk remains after normal checks and review.

Possibly related PRs

• grimmory-tools/grimmory#1534: Modifies AuthService logout and session-clearing behavior.

Suggested labels: frontend, enhancement

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title follows conventional commit format and accurately describes the logout token handling change.
Description check	✅ Passed	The description includes the required sections, linked issue, change summary, testing steps, AI disclosure, and completed checklist.
Linked Issues check	✅ Passed	The changes keep the access token available until the logout request completes, addressing issue #2339 and its HTTP 401 failure.
Out of Scope Changes check	✅ Passed	The changes are limited to logout session handling and its related test, with no unrelated scope identified.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

✨ Simplify code

• Create PR with simplified 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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[alexhb1]

Would it be worth adding a standard RXJS timeout to trigger clearSession() in the case of a hung / bad connection? Those would have no error state but could leave logout as unresponsive and unable to retry after this change.

[imnotjames]

Would it be worth adding a standard RXJS timeout to trigger clearSession() in the case of a hung / bad connection? Those would have no error state but could leave logout as unresponsive and unable to retry after this change.

They should be able to log out again, shouldn't they?

I feel like an error shouldn't even clear the session because the logout request needs to actually happen for the frontend to know it has to do a redirect to do an IDP logout.. at least if we want to be correct.

[alexhb1]

Makes sense yeah. No issues anyway, just a random thought. fine to merge :)