Repository navigation
fix(auth): show logout's own notification, not login's - #936
Merged
Merged
Conversation
Bundle sizeShared by all pages: 629.6 kB (+0.2 kB) ⚪ No route changed by more than 1 kB. ✅ First load = polyfills + shared |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #936 +/- ##
========================================
- Coverage 57.4% 55.1% -2.3%
========================================
Files 338 374 +36
Lines 10646 11462 +816
Branches 2338 2514 +176
========================================
+ Hits 6107 6305 +198
- Misses 3917 4526 +609
- Partials 622 631 +9
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved test incompatibility and logout navigation/notification issues block approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates logout navigation to display dedicated logout notifications instead of replaying the login notification.
Changes:
- Rewrites successful logout targets to
account-logout-success. - Corrects the
account-logout-failednotification ID. - Adds logout behavior tests.
File summaries
| File | Review summary |
|---|---|
src/store/slices/notification.ts |
Corrects the failed-logout notification identifier. |
src/lib/useSession.ts |
Adds logout notification navigation. Moderate findings: assign preserves stale history (2 votes); protected routes can hide the logout confirmation (1 vote); failed logout can replay the login notification (2 votes). |
src/lib/useSession.test.tsx |
Adds logout tests. Critical finding (2 votes): redefining window.location is incompatible with jsdom 26 and causes the suite to fail before running. |
Review details
Suppressed comments (1)
src/lib/useSession.ts:31
- High impact (confidence: high): When logout is triggered from
/user/settings,/user/libraries, or/user/notifications, this target is still a protected route. The middleware redirects the unauthenticated reload to the login page and replacesnotifywithlogin-required(src/middleware.ts:286-308), so the new logout confirmation is not shown for those common logout locations. Navigate to a public destination after logout or preserve the logout notification through that redirect.
window.location.assign(`${url.pathname}${url.search}${url.hash}`);
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+31
to
+34
| Object.defineProperty(window, 'location', { | ||
| configurable: true, | ||
| value: { ...window.location, href, assign: vi.fn() }, | ||
| }); |
| // redirect, which replays on reload. | ||
| const url = new URL(window.location.href); | ||
| url.searchParams.set('notify', 'account-logout-success' satisfies NotificationId); | ||
| window.location.assign(`${url.pathname}${url.search}${url.hash}`); |
| // redirect, which replays on reload. | ||
| const url = new URL(window.location.href); | ||
| url.searchParams.set('notify', 'account-logout-success' satisfies NotificationId); | ||
| window.location.assign(`${url.pathname}${url.search}${url.hash}`); |
thostetler
changed the title
thostetler
force-pushed
the
fix/logout-notification
branch
from
September 23, 2026 13:13
c775b29 to
f66ad1d
Compare
shinyichen
approved these changes
Sep 25, 2026
Member
|
@thostetler Not sure if this would be an issue. After logging out (now has param notify=account-logout-success), and login again, the param is still there. |
…n one Logging out reloaded the current URL, which still carried notify=account-login-success from the login redirect, so the toast read "You have been logged in." Nothing ever emitted account-logout-success, so logout had no confirmation of its own. - Rewrites notify to account-logout-success on the logout reload target - Corrects the account-logout-failed entry, which declared itself with id account-logout-success
A failed logout reloaded the current URL, which still carried the login redirect's notify param, so the user saw the login toast and account-logout-failed was never emitted. Both logout outcomes now reload through one helper that overwrites notify with the matching id, using replace() rather than assign() so Back cannot return to the stale URL.
thostetler
force-pushed
the
fix/logout-notification
branch
from
September 29, 2026 20:00
f66ad1d to
e9d438b
Compare
notify is consumed during hydration, but nothing removed it, so the toast replayed on reload, on Back, and through any link built from the current path. Logging out and back in showed the logout toast, because the login next param carried the logout notify along. The param is now stripped from the URL after hydration, from the next param the account dropdown builds, and from the target login redirects to.
thostetler
force-pushed
the
fix/logout-notification
branch
from
September 29, 2026 20:03
e9d438b to
f00e31e
Compare
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
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.
Logging out reloaded the page with the login redirect's
?notify=account-login-success still in the URL, so it showed "You have
been logged in." after a logout. account-logout-success existed but
nothing ever emitted it.
via window.location.replace(), so Back can't land on the stale URL
account-logout-failed (previously replayed the login toast)
id: 'account-logout-success'