fix(ux/session): anchor sidebar nav + share session across tabs - #487
Merged
cristim merged 2 commits intoMay 19, 2026
Merged
Conversation
Closes #462. Two related QA findings, fixed together because both touch the same nav surface and the same auth bootstrap path: (a) Sidebar nav items were `<button>` so right-click "Open Link in New Tab" was missing. Converted each to `<a href="https://github.com/{tab}">` with the existing role=tab + data-tab attributes preserved. The SPA click handler now preventDefaults only for un-modified left clicks; middle-clicks and Ctrl/Cmd/Shift-clicks fall through so the browser handles open-in-new-tab natively. CSS gets text-decoration:none + :visited override so the rendered look matches the previous button rows. (b) Auth tokens lived in sessionStorage (tab-scoped), so opening a second tab on the same origin forced a re-login. Moved authToken/apiKey/csrfToken to localStorage and added a `storage` event listener that propagates logout across tabs (clearing the keys in one tab reloads the others into the login modal). Reverse migration copies any leftover sessionStorage values into localStorage on first init after upgrade so users stay signed in. The XSS-exposure-window widening is documented in a SECURITY block at the top of client.ts with the rationale and mitigations. Tests: new __tests__/sidebar-anchors.test.ts asserts each of the six sidebar items renders as `<a href="https://github.com/{tab}">` (so middle/right-click works), and that the click handler preventDefaults only for un-modified left clicks (lets the browser handle Ctrl/Cmd/Shift/middle clicks). api.test.ts gains a fresh-tab bootstrap assertion (existing localStorage token => isAuthenticated() === true) and the sessionStorage → localStorage reverse-migration assertion. Existing api.test.ts storage assertions moved from sessionStorageMock to localStorageMock.
Member
Author
|
@coderabbitai review |
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR addresses issue ChangesSession Management and Navigation Link Improvements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
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)
Comment |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/src/__tests__/sidebar-anchors.test.ts (1)
135-146: ⚡ Quick winAdd an explicit
metaKey(Cmd-click) test case.The current case validates
ctrlKeyonly, so a regression in themetaKeybranch could slip through on macOS.Proposed test addition
test('Ctrl/Cmd-click falls through to the browser (no preventDefault, no switchTab)', () => { const link = document.querySelector('a.tab-btn') as HTMLAnchorElement; const evt = new MouseEvent('click', { bubbles: true, cancelable: true, button: 0, ctrlKey: true, }); link.dispatchEvent(evt); expect(evt.defaultPrevented).toBe(false); expect(switchTab).not.toHaveBeenCalled(); }); + + test('Cmd-click (metaKey) falls through to the browser', () => { + const link = document.querySelector('a.tab-btn') as HTMLAnchorElement; + const evt = new MouseEvent('click', { + bubbles: true, + cancelable: true, + button: 0, + metaKey: true, + }); + link.dispatchEvent(evt); + expect(evt.defaultPrevented).toBe(false); + expect(switchTab).not.toHaveBeenCalled(); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/__tests__/sidebar-anchors.test.ts` around lines 135 - 146, Add a parallel test for Cmd-click by creating a MouseEvent with metaKey: true (similar to the existing Ctrl-click test) in frontend/src/__tests__/sidebar-anchors.test.ts: reuse the same query for the anchor (link), dispatch the event (evt with bubbles, cancelable, button: 0, metaKey: true), and assert evt.defaultPrevented is false and switchTab has not been called; you can copy the existing "Ctrl/Cmd-click falls through..." test or add a separate test case to ensure the metaKey branch is covered.frontend/src/__tests__/api.test.ts (1)
137-150: 💤 Low valueOptional: Add missing assertion for
csrfTokenremoval from sessionStorage.The implementation iterates over all
STORAGE_KEYSand removes each from both localStorage and sessionStorage. The test verifiessessionStorageMock.removeItemforauthTokenandapiKeybut omitscsrfToken.Proposed fix
expect(sessionStorageMock.removeItem).toHaveBeenCalledWith('authToken'); expect(sessionStorageMock.removeItem).toHaveBeenCalledWith('apiKey'); + expect(sessionStorageMock.removeItem).toHaveBeenCalledWith('csrfToken'); expect(isAuthenticated()).toBe(false);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/__tests__/api.test.ts` around lines 137 - 150, The test for clearAuth is missing an assertion that sessionStorage.removeItem is called for 'csrfToken'; update the 'clearAuth' describe block test to also expect(sessionStorageMock.removeItem).toHaveBeenCalledWith('csrfToken') so the test verifies removal of all keys in STORAGE_KEYS from both storages (functions referenced: clearAuth, setAuthToken, setApiKey, isAuthenticated; mocks: sessionStorageMock/removeItem, localStorageMock/removeItem).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/__tests__/sidebar-anchors.test.ts`:
- Around line 111-121: The beforeEach currently appends an anchor and calls
setupEventListeners without clearing prior DOM or listeners; update the test
setup to reset the DOM and avoid listener accumulation by clearing document.body
(e.g., document.body.innerHTML = '') or removing existing .tab-btn anchors
before creating the new <a>, and ensure any global listeners bound by
setupEventListeners are not duplicated (remove/rebind or call a teardown like
removeEventListeners in afterEach). Specifically modify the beforeEach (and/or
add afterEach) surrounding the creation of the anchor element and the call to
setupEventListeners so jest.clearAllMocks remains but the DOM and event
listeners are reset between tests.
---
Nitpick comments:
In `@frontend/src/__tests__/api.test.ts`:
- Around line 137-150: The test for clearAuth is missing an assertion that
sessionStorage.removeItem is called for 'csrfToken'; update the 'clearAuth'
describe block test to also
expect(sessionStorageMock.removeItem).toHaveBeenCalledWith('csrfToken') so the
test verifies removal of all keys in STORAGE_KEYS from both storages (functions
referenced: clearAuth, setAuthToken, setApiKey, isAuthenticated; mocks:
sessionStorageMock/removeItem, localStorageMock/removeItem).
In `@frontend/src/__tests__/sidebar-anchors.test.ts`:
- Around line 135-146: Add a parallel test for Cmd-click by creating a
MouseEvent with metaKey: true (similar to the existing Ctrl-click test) in
frontend/src/__tests__/sidebar-anchors.test.ts: reuse the same query for the
anchor (link), dispatch the event (evt with bubbles, cancelable, button: 0,
metaKey: true), and assert evt.defaultPrevented is false and switchTab has not
been called; you can copy the existing "Ctrl/Cmd-click falls through..." test or
add a separate test case to ensure the metaKey branch is covered.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: cbe79c01-b993-46e2-8443-da4e0899cdf7
📒 Files selected for processing (6)
frontend/src/__tests__/api.test.tsfrontend/src/__tests__/sidebar-anchors.test.tsfrontend/src/api/client.tsfrontend/src/app.tsfrontend/src/index.htmlfrontend/src/styles/layout.css
- sidebar-anchors.test.ts: explicitly clear document.body in beforeEach of the click-handling describe so prior anchors do not accumulate click listeners across runs (CR actionable). - sidebar-anchors.test.ts: add a separate metaKey (Cmd-click on macOS) fall-through test so a regression on either of ctrlKey or metaKey cannot slip through (CR nit). - api.test.ts: assert clearAuth() removes csrfToken from sessionStorage alongside authToken and apiKey to cover the full STORAGE_KEYS sweep (CR nit).
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
This was referenced May 19, 2026
Closed
cristim
added a commit
that referenced
this pull request
May 19, 2026
* test(frontend/auth): cover cross-tab logout sync (closes #493) PR #487 added installStorageListener() in api/client.ts as the compensating control for moving tokens off sessionStorage. The behaviour is implemented but had no automated regression test; if the listener's key filter, newValue check, or idempotency guard ever regresses, the SECURITY claim in client.ts becomes silently false. Add crosstab-session.test.ts covering: - cleared auth keys (authToken, apiKey, csrfToken) zero in-memory state and call window.location.reload - non-auth keys are ignored - partial updates (newValue !== null, e.g. token refresh) are ignored - initAuth() is idempotent: storage listener installs exactly once Each test loads api/client in jest.isolateModules() so the storageListenerInstalled guard is fresh per test. * test(frontend/auth): patch only window.location.reload (CR nit) Capture the original window.location reference before tests and restore it in an afterEach, so each test starts from a clean Location object. This implements CR's teardown recommendation while keeping the Object.defineProperty(window, 'location', ...) approach required by jsdom (window.location.reload is non-configurable and cannot be patched directly).
This was referenced May 20, 2026
cristim
added a commit
that referenced
this pull request
Jun 9, 2026
* test(frontend/auth): cover cross-tab logout sync (closes #493) PR #487 added installStorageListener() in api/client.ts as the compensating control for moving tokens off sessionStorage. The behaviour is implemented but had no automated regression test; if the listener's key filter, newValue check, or idempotency guard ever regresses, the SECURITY claim in client.ts becomes silently false. Add crosstab-session.test.ts covering: - cleared auth keys (authToken, apiKey, csrfToken) zero in-memory state and call window.location.reload - non-auth keys are ignored - partial updates (newValue !== null, e.g. token refresh) are ignored - initAuth() is idempotent: storage listener installs exactly once Each test loads api/client in jest.isolateModules() so the storageListenerInstalled guard is fresh per test. * test(frontend/auth): patch only window.location.reload (CR nit) Capture the original window.location reference before tests and restore it in an afterEach, so each test starts from a clean Location object. This implements CR's teardown recommendation while keeping the Object.defineProperty(window, 'location', ...) approach required by jsdom (window.location.reload is non-configurable and cannot be patched directly). * test(frontend/auth): clear localStorage between cross-tab logout tests (CR nit) initAuth() seeds in-memory auth from localStorage, so without clearing it between tests a stale token from one case could leak into the next. Extend the existing afterEach to call localStorage.clear() for proper isolation. * test(frontend/auth): address CR nits on cross-tab logout test - replace the out! non-null assertion in loadAuth() with an explicit guard that throws a clear error if isolateModules never ran - add a regression case for the !e.key guard in installStorageListener: a null-key storage event (localStorage.clear()) must not clear in-memory auth or reload
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
1 participant
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.
Summary
Closes #462.
Two QA findings around tab/session behaviour, fixed together because both
touch the sidebar nav surface and the auth bootstrap path.
(a) "Open Link in New Tab" on sidebar items. The left-side nav was
six
<button class="tab-btn">elements so right-click did not offer thebrowser's "Open Link in New Tab" affordance. Converted each to
<a class="tab-btn" href="https://github.com/{tab}">while preservingrole="tab",data-tab,aria-*, and the SVG icon + label. The SPA click handler inapp.tsnowpreventDefault()s only for un-modified left clicks;middle-clicks (button=1), Ctrl/Cmd/Shift-clicks fall through so the
browser opens a new tab/window natively. Admin links to
/admin/general(its default sub-tab) so a cold-open in a fresh tablands on a real route.
(b) Cross-tab session. Tokens lived in
sessionStorage, which istab-scoped, so a second tab on the same origin forced re-login. Moved
authToken,apiKey,csrfTokentolocalStorageand registered astorageevent listener that signs the other tabs out when one tabclears the keys (i.e. logout propagates across tabs within ~1s).
Reverse migration copies any leftover
sessionStoragevalues intolocalStorageon firstinitAuth()after upgrade so users stay signedin across the release.
Security note
localStoragewidens the XSS exposure window from "tab close" to"explicit logout" relative to the previous
sessionStoragesetup.The tradeoff is documented in a
SECURITY:comment block at the topof
frontend/src/api/client.ts. The issue body called out the sametradeoff and explicitly authorized it (admins compare Settings vs.
Opportunities side by side, so multi-tab needs to work).
storageevent so a sign-out anywhereclears the others.
the issue discussion); that is a larger refactor (
Authorizationheader to credentials-mode cookies) tracked separately.
storage tier, no cookie change, no
SameSitechange).hrefvalues are hard-coded HTML,not interpolated from user input.
Tests
frontend/src/__tests__/sidebar-anchors.test.ts:<a href="https://github.com/{tab}">so middle/right-click works (the issue's headline ask).
preventDefaults for un-modified leftclicks, but lets Ctrl/Cmd-click, Shift-click, and middle-click fall
through to the browser.
frontend/src/__tests__/api.test.ts:(
localStoragealready has a token =>isAuthenticated()returnstrue without a re-login).
sessionStoragetolocalStoragereverse migration onupgrade.
sessionStorageMocktolocalStorageMock.Full suite: 1754 tests pass;
tsc --noEmitclean.Test plan
confirm no login prompt.
appears.
right section.
the login modal within ~1s.
Summary by CodeRabbit
New Features
Bug Fixes
Tests