From 6eb60e5ec9f694e7fedb1c697cbb2ed837a6c16e Mon Sep 17 00:00:00 2001 From: Will Gordon Date: Mon, 17 Aug 2026 16:42:03 -0400 Subject: [PATCH 1/3] fix(notifications): stop toast replay across nav and page reload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ToastContainer's dedup state was component-local, so it reset on every mount. Since it only renders inside Header, which the router mounts for /dashboard but not /settings, navigating away and back unmounted and remounted it, replaying a toast for every notification still active in the store — most visibly a GitHub-status outage, which can sit unchanged in the store for hours. A hard page refresh reproduced the same symptom via a different path: it wipes the notification store itself, so the next poll's unchanged outage looks brand new again. Persist the last-toasted message per source to sessionStorage, checked before showing a toast and pruned once a source's notification clears from the store. sessionStorage survives both a same-tab remount and a refresh, so a single mechanism covers both triggers without touching notifyTransitions()'s existing unconditional-push behavior. --- src/app/components/shared/ToastContainer.tsx | 46 +++++++++++- .../components/shared/ToastContainer.test.tsx | 75 ++++++++++++++++++- 2 files changed, 119 insertions(+), 2 deletions(-) diff --git a/src/app/components/shared/ToastContainer.tsx b/src/app/components/shared/ToastContainer.tsx index 238d368b..51e2e850 100644 --- a/src/app/components/shared/ToastContainer.tsx +++ b/src/app/components/shared/ToastContainer.tsx @@ -47,9 +47,42 @@ interface ToastItem { dismissing: boolean; } +// Persisted (sessionStorage) record of the last message actually toasted per +// source. This component only renders inside Header, which the router mounts +// for /dashboard but not /settings — navigating away and back fully unmounts +// and remounts it, wiping any in-memory-only dedup state. A hard page refresh +// wipes the entire notification store too. Either way, a still-active, +// unchanged notification (most visibly a GitHub-status outage, which can sit +// unchanged in the store for hours) would otherwise look brand new again and +// re-toast. sessionStorage survives both a remount and a refresh, so this is +// the one place dedup needs to persist beyond the component's own lifetime. +const TOASTED_MESSAGES_KEY = "github-tracker:toasted-messages"; + +function loadToastedMessages(): Map { + const raw = sessionStorage.getItem(TOASTED_MESSAGES_KEY); + const parsed: unknown = raw ? JSON.parse(raw) : []; + if (!Array.isArray(parsed)) return new Map(); + return new Map( + parsed.filter( + (e): e is [string, string] => + Array.isArray(e) && e.length === 2 && typeof e[0] === "string" && typeof e[1] === "string" + ) + ); +} + +function persistToastedMessages(map: Map): void { + sessionStorage.setItem(TOASTED_MESSAGES_KEY, JSON.stringify([...map.entries()])); +} + +// Test-only reset — mirrors resetGitHubStatusState()/resetPollState() etc. +export function resetToastState(): void { + sessionStorage.removeItem(TOASTED_MESSAGES_KEY); +} + export default function ToastContainer() { const seenTimestamps = new Map(); const lastToastedAt = new Map(); + const toastedMessages = loadToastedMessages(); const [visibleToasts, setVisibleToasts] = createSignal>(new Map()); const timeouts = new Map>(); const dismissingTimeouts = new Map>(); @@ -115,10 +148,13 @@ export default function ToastContainer() { const lastToasted = lastToastedAt.get(notif.source); const inCooldown = lastToasted !== undefined && Date.now() - lastToasted < COOLDOWN_MS; const muted = isMuted(notif.source); + const alreadyToasted = toastedMessages.get(notif.source) === notif.message; - if (inCooldown || muted) continue; + if (inCooldown || muted || alreadyToasted) continue; lastToastedAt.set(notif.source, Date.now()); + toastedMessages.set(notif.source, notif.message); + persistToastedMessages(toastedMessages); setVisibleToasts((prev) => { const next = new Map(prev); next.set(notif.id, { notification: notif, dismissing: false }); @@ -135,6 +171,14 @@ export default function ToastContainer() { for (const source of lastToastedAt.keys()) { if (!currentSources.has(source)) lastToastedAt.delete(source); } + let toastedMessagesChanged = false; + for (const source of toastedMessages.keys()) { + if (!currentSources.has(source)) { + toastedMessages.delete(source); + toastedMessagesChanged = true; + } + } + if (toastedMessagesChanged) persistToastedMessages(toastedMessages); for (const id of visibleToasts().keys()) { if (!currentIds.has(id)) { const t = timeouts.get(id); diff --git a/tests/components/shared/ToastContainer.test.tsx b/tests/components/shared/ToastContainer.test.tsx index 98393156..0d7ef21f 100644 --- a/tests/components/shared/ToastContainer.test.tsx +++ b/tests/components/shared/ToastContainer.test.tsx @@ -8,11 +8,12 @@ import { dismissError, getNotifications, } from "../../../src/app/lib/errors"; -import ToastContainer from "../../../src/app/components/shared/ToastContainer"; +import ToastContainer, { resetToastState } from "../../../src/app/components/shared/ToastContainer"; beforeEach(() => { clearNotifications(); clearMutedSources(); + resetToastState(); vi.useFakeTimers(); // Ensure matchMedia returns non-reduced-motion vi.spyOn(window, "matchMedia").mockReturnValue({ matches: false } as MediaQueryList); @@ -153,4 +154,76 @@ describe("ToastContainer", () => { // Toast should be removed (store pruning path) expect(screen.queryAllByRole("alert")).toHaveLength(0); }); + + it("does not re-toast a still-active notification across a remount (e.g. dashboard -> settings -> dashboard nav)", () => { + const first = render(() => ); + pushNotification("github-status", "Actions Outage", "error"); + expect(screen.queryAllByRole("alert")).toHaveLength(1); + + // Simulate navigating away: ToastContainer/Header only render inside + // DashboardPage, so leaving /dashboard unmounts this component while the + // notification (an ongoing outage) stays in the store, unchanged. + first.unmount(); + expect(screen.queryAllByRole("alert")).toHaveLength(0); + + // Simulate navigating back: a fresh mount, same unchanged notification + // still present in the store — must not replay the toast. + render(() => ); + expect(screen.queryAllByRole("alert")).toHaveLength(0); + }); + + it("still toasts a genuinely new notification for the same source after a remount", () => { + const first = render(() => ); + pushNotification("github-status", "Actions Outage", "error"); + expect(screen.queryAllByRole("alert")).toHaveLength(1); + + first.unmount(); + + // A distinct incident (new message) resolves the prior one and starts — + // must still surface as a toast even though this source was already + // toasted once before the remount. + pushNotification("github-status", "Issues Outage", "error"); + render(() => ); + const alerts = screen.queryAllByRole("alert"); + expect(alerts).toHaveLength(1); + expect(alerts[0].textContent).toContain("Issues Outage"); + }); + + it("does not re-toast after a simulated hard refresh (sessionStorage survives, in-memory state does not)", () => { + const first = render(() => ); + pushNotification("github-status", "Actions Outage", "error"); + expect(screen.queryAllByRole("alert")).toHaveLength(1); + first.unmount(); + + // A hard refresh wipes the notification store itself (module-level state + // resets), but NOT sessionStorage. Simulate that: clear the store the way + // a fresh page load would leave it, then re-announce the same unchanged + // outage on the next poll cycle after reload — exactly what + // notifyTransitions() does today (it unconditionally re-pushes for any + // still-active incident). + clearNotifications(); + pushNotification("github-status", "Actions Outage", "error"); + + render(() => ); + expect(screen.queryAllByRole("alert")).toHaveLength(0); + }); + + it("re-toasts once a source's notification clears and later recurs with the same text", () => { + const first = render(() => ); + pushNotification("github-status", "Actions Outage", "error"); + expect(screen.queryAllByRole("alert")).toHaveLength(1); + + // Incident fully resolves — source drops out of the store entirely. + const notifId = getNotifications()[0].id; + dismissError(notifId); + expect(screen.queryAllByRole("alert")).toHaveLength(0); + + first.unmount(); + + // A later, unrelated incident happens to have identical text — should + // not be suppressed forever just because the same string was toasted once. + pushNotification("github-status", "Actions Outage", "error"); + render(() => ); + expect(screen.queryAllByRole("alert")).toHaveLength(1); + }); }); From 6efcfefcdb87e90c92946882b7f09622af8b34ae Mon Sep 17 00:00:00 2001 From: Will Gordon Date: Tue, 18 Aug 2026 11:15:44 -0400 Subject: [PATCH 2/3] fix(notifications): coalesce toast bursts, harden dedup persistence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Restarts a toast's auto-dismiss timer on message updates instead of leaving a stale one from the prior message, which could dismiss the toast early - Reintroduces a short (3s) per-source coalescing throttle for rapid bursts of textually-different messages (e.g. a ticking rate-limit retry countdown), distinct from the persisted exact-message dedup — with a trailing-edge re-check so a suppressed value that's never superseded still surfaces once the window elapses, even when errors.ts's own same-message no-op guard would otherwise leave it permanently stuck - Wraps sessionStorage read/write in try/catch matching the codebase's established pattern, and clears toast dedup state on logout via onAuthCleared --- src/app/components/shared/ToastContainer.tsx | 132 ++++++++--- tests/components/layout/Header.test.tsx | 1 + .../components/shared/ToastContainer.test.tsx | 216 +++++++++++++++++- 3 files changed, 306 insertions(+), 43 deletions(-) diff --git a/src/app/components/shared/ToastContainer.tsx b/src/app/components/shared/ToastContainer.tsx index 51e2e850..2249ba81 100644 --- a/src/app/components/shared/ToastContainer.tsx +++ b/src/app/components/shared/ToastContainer.tsx @@ -5,6 +5,7 @@ import { type AppNotification, type NotificationSeverity, } from "../../lib/errors"; +import { onAuthCleared } from "../../stores/auth"; export interface SeverityConfig { path: string; @@ -59,35 +60,71 @@ interface ToastItem { const TOASTED_MESSAGES_KEY = "github-tracker:toasted-messages"; function loadToastedMessages(): Map { - const raw = sessionStorage.getItem(TOASTED_MESSAGES_KEY); - const parsed: unknown = raw ? JSON.parse(raw) : []; - if (!Array.isArray(parsed)) return new Map(); - return new Map( - parsed.filter( - (e): e is [string, string] => - Array.isArray(e) && e.length === 2 && typeof e[0] === "string" && typeof e[1] === "string" - ) - ); + try { + const raw = sessionStorage.getItem(TOASTED_MESSAGES_KEY); + const parsed: unknown = raw ? JSON.parse(raw) : []; + if (!Array.isArray(parsed)) return new Map(); + return new Map( + parsed.filter( + (e): e is [string, string] => + Array.isArray(e) && e.length === 2 && typeof e[0] === "string" && typeof e[1] === "string" + ) + ); + } catch { + return new Map(); + } } function persistToastedMessages(map: Map): void { - sessionStorage.setItem(TOASTED_MESSAGES_KEY, JSON.stringify([...map.entries()])); + try { + sessionStorage.setItem(TOASTED_MESSAGES_KEY, JSON.stringify([...map.entries()])); + } catch { + /* best-effort — dedup persistence is low-stakes, no user-facing notification needed */ + } } -// Test-only reset — mirrors resetGitHubStatusState()/resetPollState() etc. +// Resets toast dedup state. Called on logout via the onAuthCleared registration +// below, and directly by tests to isolate sessionStorage between cases (mirrors +// resetGitHubStatusState()/resetPollState() etc.). export function resetToastState(): void { sessionStorage.removeItem(TOASTED_MESSAGES_KEY); } +// toastedMessages stores per-source API/search/graphql error text, which is +// user-scoped data (unlike github-status.ts's global GitHub-status feed, which +// intentionally does NOT hook into onAuthCleared — see the note in that file). +// Clear it on logout so a previous user's toast history can't leak into the +// next session on a shared browser tab. +onAuthCleared(resetToastState); + export default function ToastContainer() { const seenTimestamps = new Map(); - const lastToastedAt = new Map(); const toastedMessages = loadToastedMessages(); const [visibleToasts, setVisibleToasts] = createSignal>(new Map()); const timeouts = new Map>(); const dismissingTimeouts = new Map>(); - const COOLDOWN_MS = 60_000; + // lastToastedAt + COALESCE_MS: short, in-memory-only per-source throttle. + // Distinct from toastedMessages above (which persists which exact message + // was last shown, surviving a remount/refresh): lastToastedAt only + // coalesces a rapid burst of textually-DIFFERENT updates from the same + // source (e.g. a fast-ticking rate-limit retry countdown) into a single + // visible toast, so it doesn't need to survive a remount — a genuinely new + // incident more than a few seconds later should always show promptly. + const lastToastedAt = new Map(); + const COALESCE_MS = 3_000; + + // A coalesced (suppressed) update can be the LAST thing that ever happens + // for a source — e.g. a flapping status message settles back to a value + // that's already in toastedMessages, at which point errors.ts's own + // same-message no-op guard means the store never fires another change + // event for it, so this component would never get another chance to + // re-evaluate it. coalesceTimers schedules a one-shot re-check for exactly + // when the coalescing window ends, reading whatever the store holds AT + // THAT TIME (not the coalesced value itself) so a value that was + // suppressed and never superseded still surfaces once the window elapses. + const coalesceTimers = new Map>(); + const reducedMotion = window.matchMedia("(prefers-reduced-motion: reduce)").matches; const animDelay = reducedMotion ? 0 : 300; @@ -126,6 +163,12 @@ export default function ToastContainer() { } function scheduleAutoDismiss(notification: AppNotification) { + // Clear any prior timer so an update to an existing toast gets a fresh + // full dismiss window, rather than inheriting a timer sized for the + // original (now possibly stale) message. + const existing = timeouts.get(notification.id); + if (existing !== undefined) clearTimeout(existing); + const delay = notification.severity === "error" ? 10_000 : 5_000; const t = setTimeout(() => { timeouts.delete(notification.id); @@ -134,6 +177,33 @@ export default function ToastContainer() { timeouts.set(notification.id, t); } + function showToast(notif: AppNotification) { + lastToastedAt.set(notif.source, Date.now()); + toastedMessages.set(notif.source, notif.message); + persistToastedMessages(toastedMessages); + setVisibleToasts((prev) => { + const next = new Map(prev); + next.set(notif.id, { notification: notif, dismissing: false }); + return next; + }); + scheduleAutoDismiss(notif); + } + + function scheduleCoalesceRecheck(source: string) { + if (coalesceTimers.has(source)) return; + const lastToasted = lastToastedAt.get(source) ?? Date.now(); + const remaining = Math.max(0, COALESCE_MS - (Date.now() - lastToasted)); + const t = setTimeout(() => { + coalesceTimers.delete(source); + const current = getNotifications().find(n => n.source === source); + if (!current || isMuted(source)) return; + if (toastedMessages.get(source) === current.message) return; + seenTimestamps.set(current.id, current.timestamp); + showToast(current); + }, remaining); + coalesceTimers.set(source, t); + } + createEffect(() => { const notifs = getNotifications(); for (const notif of notifs) { @@ -146,21 +216,16 @@ export default function ToastContainer() { seenTimestamps.set(notif.id, notif.timestamp); const lastToasted = lastToastedAt.get(notif.source); - const inCooldown = lastToasted !== undefined && Date.now() - lastToasted < COOLDOWN_MS; + const coalescing = lastToasted !== undefined && Date.now() - lastToasted < COALESCE_MS; const muted = isMuted(notif.source); const alreadyToasted = toastedMessages.get(notif.source) === notif.message; - if (inCooldown || muted || alreadyToasted) continue; - - lastToastedAt.set(notif.source, Date.now()); - toastedMessages.set(notif.source, notif.message); - persistToastedMessages(toastedMessages); - setVisibleToasts((prev) => { - const next = new Map(prev); - next.set(notif.id, { notification: notif, dismissing: false }); - return next; - }); - scheduleAutoDismiss(notif); + if (coalescing || muted || alreadyToasted) { + if (coalescing) scheduleCoalesceRecheck(notif.source); + continue; + } + + showToast(notif); } const currentIds = new Set(notifs.map(n => n.id)); @@ -168,15 +233,15 @@ export default function ToastContainer() { if (!currentIds.has(id)) seenTimestamps.delete(id); } const currentSources = new Set(notifs.map(n => n.source)); - for (const source of lastToastedAt.keys()) { - if (!currentSources.has(source)) lastToastedAt.delete(source); - } + const staleSources = new Set( + [...lastToastedAt.keys(), ...toastedMessages.keys()].filter(source => !currentSources.has(source)) + ); let toastedMessagesChanged = false; - for (const source of toastedMessages.keys()) { - if (!currentSources.has(source)) { - toastedMessages.delete(source); - toastedMessagesChanged = true; - } + for (const source of staleSources) { + lastToastedAt.delete(source); + if (toastedMessages.delete(source)) toastedMessagesChanged = true; + const ct = coalesceTimers.get(source); + if (ct !== undefined) { clearTimeout(ct); coalesceTimers.delete(source); } } if (toastedMessagesChanged) persistToastedMessages(toastedMessages); for (const id of visibleToasts().keys()) { @@ -193,6 +258,7 @@ export default function ToastContainer() { onCleanup(() => { for (const t of timeouts.values()) clearTimeout(t); for (const t of dismissingTimeouts.values()) clearTimeout(t); + for (const t of coalesceTimers.values()) clearTimeout(t); }); return ( diff --git a/tests/components/layout/Header.test.tsx b/tests/components/layout/Header.test.tsx index 8fd47c2d..3983bf84 100644 --- a/tests/components/layout/Header.test.tsx +++ b/tests/components/layout/Header.test.tsx @@ -24,6 +24,7 @@ vi.mock("../../../src/app/stores/auth", () => ({ name: "The Octocat", }), clearAuth: vi.fn(), + onAuthCleared: vi.fn(), })); // Mock errors module so Header's notification imports work diff --git a/tests/components/shared/ToastContainer.test.tsx b/tests/components/shared/ToastContainer.test.tsx index 0d7ef21f..ae2a1c5e 100644 --- a/tests/components/shared/ToastContainer.test.tsx +++ b/tests/components/shared/ToastContainer.test.tsx @@ -10,6 +10,25 @@ import { } from "../../../src/app/lib/errors"; import ToastContainer, { resetToastState } from "../../../src/app/components/shared/ToastContainer"; +// Mirrors the private TOASTED_MESSAGES_KEY constant in ToastContainer.tsx — +// hardcoded here the same way tests/stores/config.test.ts hardcodes STORAGE_KEY. +const TOASTED_MESSAGES_KEY = "github-tracker:toasted-messages"; + +// Mock the auth store's onAuthCleared so the "clears on logout" test can +// directly invoke the callback ToastContainer registers, without pulling in +// clearAuth()'s broader logout side effects (localStorage, IndexedDB, +// Sentry) — mirrors the capture pattern in tests/components/DashboardPage.test.tsx. +// vi.hoisted() is required here (unlike that file) because this file imports +// ToastContainer statically at the top, so the mock factory runs — and calls +// onAuthCleared(resetToastState) via ToastContainer's module-scope +// registration — before a plain top-level `const` would finish initializing. +const { authClearCallbacks } = vi.hoisted(() => ({ authClearCallbacks: [] as (() => void)[] })); +vi.mock("../../../src/app/stores/auth", () => ({ + onAuthCleared: vi.fn((cb: () => void) => { + authClearCallbacks.push(cb); + }), +})); + beforeEach(() => { clearNotifications(); clearMutedSources(); @@ -112,26 +131,126 @@ describe("ToastContainer", () => { expect(screen.queryAllByRole("alert")).toHaveLength(0); }); - it("cooldown: no new toast within 60s for same source with different message", () => { + it("coalesces a rapid burst of distinct messages from the same source, promoting the latest one once the window elapses", () => { render(() => ); pushNotification("api", "First error", "error"); expect(screen.queryAllByRole("alert")).toHaveLength(1); - // Manually dismiss so toast is gone from screen - const dismissBtn = screen.getByLabelText("Dismiss notification"); - fireEvent.click(dismissBtn); + // Two more distinct messages arrive within the coalescing window (e.g. a + // fast-ticking rate-limit retry countdown) — neither spawns/replaces the + // visible toast immediately. + vi.advanceTimersByTime(500); + pushNotification("api", "Second error", "error"); + expect(screen.getByRole("alert").textContent).toContain("First error"); + + vi.advanceTimersByTime(500); + pushNotification("api", "Third error", "error"); + expect(screen.getByRole("alert").textContent).toContain("First error"); + + // Once the coalescing window (3s from the first toast) elapses with no + // further push, the LATEST suppressed value is automatically promoted — + // a burst that never gets superseded doesn't get silently lost forever. + vi.advanceTimersByTime(2001); + expect(screen.queryAllByRole("alert")).toHaveLength(1); + expect(screen.getByRole("alert").textContent).toContain("Third error"); + }); + + it("does not spuriously re-toast when a coalesced burst settles back to the already-displayed message", () => { + render(() => ); + pushNotification("api", "State Alpha", "error"); + expect(screen.queryAllByRole("alert")).toHaveLength(1); + + // A different value arrives and is coalesced, then flaps back to the + // original value (also coalesced) before the window elapses. Once the + // window's trailing-edge check runs, the store's current value already + // matches what's displayed — this must NOT trigger a spurious re-toast + // or reset the auto-dismiss timer (errors.ts treats a push identical to + // the store's current value as a no-op, so this path only reaches + // ToastContainer via the same coalescing re-check machinery, not a new + // store event). + vi.advanceTimersByTime(500); + pushNotification("api", "State Beta", "error"); vi.advanceTimersByTime(300); + pushNotification("api", "State Alpha", "error"); + expect(screen.getByRole("alert").textContent).toContain("State Alpha"); + + vi.advanceTimersByTime(2201); + expect(screen.queryAllByRole("alert")).toHaveLength(1); + expect(screen.getByRole("alert").textContent).toContain("State Alpha"); + + // Confirm no restart occurred: the toast still dismisses on its ORIGINAL + // 10s schedule from the very first push (500+300+2201+7299 = 10300), + // not a schedule reset by a spurious re-check promotion. + vi.advanceTimersByTime(7299); expect(screen.queryAllByRole("alert")).toHaveLength(0); + }); - // Push different message within 60s — should NOT show new toast (cooldown) - pushNotification("api", "Second error", "error"); + it("restarts the auto-dismiss timer when an update passes through once the coalescing window has elapsed", () => { + render(() => ); + pushNotification("rate-limit", "Retrying in 5s", "warning"); + expect(screen.queryAllByRole("alert")).toHaveLength(1); + + // Exactly at the 3s coalescing window boundary, a fresh retry countdown + // arrives for the same source and is allowed through — the toast must + // get a full fresh dismiss window rather than inheriting the stale one. + vi.advanceTimersByTime(3000); + pushNotification("rate-limit", "Retrying in 3s", "warning"); + expect(screen.getByRole("alert").textContent).toContain("Retrying in 3s"); + + // The ORIGINAL timer (scheduled at t=0 for 5000ms, i.e. due 2000ms from + // here) would — if not cleared on update — fire its dismiss animation and + // complete removal 300ms later (at 2300ms from here). Advance past that + // point: the toast must still be fully present, not dismissed early. + vi.advanceTimersByTime(2500); + expect(screen.queryAllByRole("alert")).toHaveLength(1); + expect(screen.getByRole("alert").textContent).toContain("Retrying in 3s"); + + // The fresh window (started at the update, t=3000) completes 5000ms + + // 300ms animation delay after the update — 3000ms from the point above. + vi.advanceTimersByTime(3000); expect(screen.queryAllByRole("alert")).toHaveLength(0); + }); - // Advance past cooldown (60s) - vi.advanceTimersByTime(60_001); + it("treats a non-array but validly-parsed sessionStorage value as empty dedup state", () => { + sessionStorage.setItem(TOASTED_MESSAGES_KEY, "42"); + render(() => ); + pushNotification("api", "First error", "error"); + expect(screen.queryAllByRole("alert")).toHaveLength(1); + }); - // Push again — should show toast now - pushNotification("api", "Third error", "error"); + it("drops malformed tuples from a parseable sessionStorage array without crashing, keeping well-formed entries", () => { + sessionStorage.setItem( + TOASTED_MESSAGES_KEY, + JSON.stringify([ + ["api", 123], + ["api"], + ["not-a-tuple"], + ["search", "Results incomplete"], + ]) + ); + + // Push the "search" notification before mount so it's already a currently- + // active source on the component's first effect run — otherwise the + // pruning pass (which clears dedup entries for sources with no active + // notification) would wipe this preloaded entry before we can exercise it. + pushNotification("search", "Results incomplete", "warning"); + + expect(() => render(() => )).not.toThrow(); + + // The well-formed "search" entry survived parsing and suppressed the + // already-active, unchanged notification on mount. + expect(screen.queryAllByRole("alert")).toHaveLength(0); + + // Malformed "api"/"not-a-tuple" entries were dropped — a fresh "api" + // notification still toasts normally. + pushNotification("api", "First error", "error"); + expect(screen.queryAllByRole("alert")).toHaveLength(1); + }); + + it("does not throw when sessionStorage contains invalid JSON, and toasts normally afterward", () => { + sessionStorage.setItem(TOASTED_MESSAGES_KEY, "{not valid json"); + expect(() => render(() => )).not.toThrow(); + pushNotification("api", "First error", "error"); expect(screen.queryAllByRole("alert")).toHaveLength(1); }); @@ -226,4 +345,81 @@ describe("ToastContainer", () => { render(() => ); expect(screen.queryAllByRole("alert")).toHaveLength(1); }); + + it("coalesces correctly when toastedMessages starts pre-populated from a prior mount, not built fresh", () => { + sessionStorage.setItem(TOASTED_MESSAGES_KEY, JSON.stringify([["api", "Old error"]])); + // Keep the source's notification active so its persisted entry survives + // the mount's pruning pass. + pushNotification("api", "Old error", "error"); + + render(() => ); + expect(screen.queryAllByRole("alert")).toHaveLength(0); + + // A rapid burst of distinct messages arrives in THIS mount — lastToastedAt + // starts empty (component-local) even though toastedMessages (the OTHER + // map) started non-empty from sessionStorage; coalescing must still + // engage correctly against the fresh map. Advance 1ms first so this + // push's timestamp differs from the pre-mount "Old error" push — fake + // timers don't advance on their own, and the effect only treats a push + // as an update when its timestamp is strictly greater than the last seen. + vi.advanceTimersByTime(1); + pushNotification("api", "New error A", "error"); + expect(screen.getByRole("alert").textContent).toContain("New error A"); + + vi.advanceTimersByTime(500); + pushNotification("api", "New error B", "error"); + expect(screen.getByRole("alert").textContent).toContain("New error A"); + + vi.advanceTimersByTime(2501); + expect(screen.getByRole("alert").textContent).toContain("New error B"); + }); + + it("prunes a persisted toastedMessages entry with no matching lastToastedAt entry when its source has no active notification", () => { + sessionStorage.setItem(TOASTED_MESSAGES_KEY, JSON.stringify([["ghost", "Old message"]])); + // No active notification for "ghost" — lastToastedAt never had an entry + // for it either (it always starts empty on mount). The merged pruning + // loop must still correctly prune a source present in only ONE of the + // two maps. + render(() => ); + expect(sessionStorage.getItem(TOASTED_MESSAGES_KEY)).toBe(JSON.stringify([])); + + // Confirms the entry was actually removed, not just hidden from view. + pushNotification("ghost", "Old message", "warning"); + expect(screen.queryAllByRole("alert")).toHaveLength(1); + }); + + it("respects a mute applied during a pending coalescing window, skipping the trailing-edge promotion", () => { + render(() => ); + pushNotification("api", "First error", "error"); + expect(screen.queryAllByRole("alert")).toHaveLength(1); + + // A distinct message arrives and is coalesced, scheduling a trailing + // re-check for when the window elapses. + vi.advanceTimersByTime(500); + pushNotification("api", "Second error", "error"); + + // The source is muted before the re-check fires. + addMutedSource("api"); + + // Once the window elapses, the re-check must honor the mute and skip + // promoting the coalesced value — the original toast stays as-is. + vi.advanceTimersByTime(2501); + expect(screen.getByRole("alert").textContent).toContain("First error"); + }); + + it("registers resetToastState with onAuthCleared so toast dedup state clears on logout", () => { + render(() => ); + pushNotification("github-status", "Actions Outage", "error"); + expect(screen.queryAllByRole("alert")).toHaveLength(1); + expect(sessionStorage.getItem(TOASTED_MESSAGES_KEY)).toContain("Actions Outage"); + + // ToastContainer registers resetToastState with onAuthCleared at module + // scope — confirm the registration actually happened (not just that the + // module loaded without error), then simulate what clearAuth() does on + // logout by invoking every captured callback. + expect(authClearCallbacks.length).toBeGreaterThan(0); + for (const cb of authClearCallbacks) cb(); + + expect(sessionStorage.getItem(TOASTED_MESSAGES_KEY)).toBeNull(); + }); }); From 710b595f485d97f487ae820bf927cd9c4a72836e Mon Sep 17 00:00:00 2001 From: Will Gordon Date: Tue, 18 Aug 2026 11:18:57 -0400 Subject: [PATCH 3/3] fix(auth): reset workspace state on a genuine identity switch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Settings > Replace Token previously left poll/notification/toast/ dashboard-cache state and repo/org/tracked-user/view preferences from the prior identity in place when swapping to a different GitHub account, since setAuthFromPat never routed through the onAuthCleared cleanup path a real logout uses. Detects a login change (case-insensitive) and performs a full reset — config, view state, IndexedDB cache, and every registered onAuthCleared callback — matching clearAuth()'s exact ordering, while leaving same-identity token rotation (e.g. refreshing an expired PAT) completely unaffected so preferences aren't lost on routine rotation. --- src/app/stores/auth.ts | 34 ++++++++++++++++++ tests/stores/auth.test.ts | 73 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+) diff --git a/src/app/stores/auth.ts b/src/app/stores/auth.ts index 29a5126a..fd1610b9 100644 --- a/src/app/stores/auth.ts +++ b/src/app/stores/auth.ts @@ -193,6 +193,40 @@ export function setAuth(response: TokenExchangeResponse): void { } export function setAuthFromPat(token: string, userData: GitHubUser): void { + const previousLogin = user()?.login; + const isIdentitySwitch = + previousLogin !== undefined && previousLogin.toLowerCase() !== userData.login.toLowerCase(); + + if (isIdentitySwitch) { + // A different GitHub identity is taking over this browser tab/session + // (Settings > Replace token, used for a user switch rather than rotating + // one's own token) — do a full reset matching clearAuth(): config + // (selectedRepos/selectedOrgs/trackedUsers/etc.) and view state are + // genuinely per-identity data, not just UI preferences, so the incoming + // identity must not inherit the outgoing one's. Reset in-memory stores + // BEFORE clearing localStorage, so the persistence effects re-write + // defaults (not stale user data) — same ordering as clearAuth(). We do + // NOT touch AUTH_STORAGE_KEY or DASHBOARD_STORAGE_KEY here: the former is + // overwritten below with the new token, and the latter is already + // cleared by resetDashboardData() in the callback loop below. + resetConfig(); + resetViewState(); + localStorage.removeItem(CONFIG_STORAGE_KEY); + localStorage.removeItem(VIEW_STORAGE_KEY); + // Clear IndexedDB cache to prevent data leakage between identities. + clearCache().catch((err) => { + console.warn("[auth] Cache clear failed during identity switch:", err); + Sentry.captureException(err, { tags: { source: "auth-identity-switch-cache-clear" } }); + }); + // Clear per-user in-memory + cached state (poll data, notifications, + // toast dedup, dashboard cache) the same way a real logout does, BEFORE + // adopting the new identity below, so the incoming identity doesn't + // inherit the outgoing one's data. + for (const cb of _onClearCallbacks) { + try { cb(); } catch (e) { console.warn("[auth] onAuthCleared callback threw during identity switch:", e); } + } + } + setAuth({ access_token: token }); setUser({ login: userData.login, avatar_url: userData.avatar_url, name: userData.name }); updateConfig({ authMethod: "pat" }); diff --git a/tests/stores/auth.test.ts b/tests/stores/auth.test.ts index a4ab1084..c664fd44 100644 --- a/tests/stores/auth.test.ts +++ b/tests/stores/auth.test.ts @@ -1224,3 +1224,76 @@ describe("setAuthFromPat", () => { expect(configMod.config.authMethod).toBe("oauth"); }); }); + +describe("setAuthFromPat — identity switch", () => { + let mod: typeof import("../../src/app/stores/auth"); + + const userA = { login: "usera", avatar_url: "https://avatars.githubusercontent.com/u/1", name: "User A" }; + const userB = { login: "userb", avatar_url: "https://avatars.githubusercontent.com/u/2", name: "User B" }; + + beforeEach(async () => { + localStorageMock.clear(); + vi.resetModules(); + mod = await import("../../src/app/stores/auth"); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("does not run onAuthCleared callbacks on the first setAuthFromPat call (no previous identity)", () => { + const cb = vi.fn(); + mod.onAuthCleared(cb); + mod.setAuthFromPat("ghp_token1", userA); + expect(cb).not.toHaveBeenCalled(); + }); + + it("does not run onAuthCleared callbacks when replacing a token for the same identity (case-insensitive)", () => { + mod.setAuthFromPat("ghp_token1", userA); + const cb = vi.fn(); + mod.onAuthCleared(cb); + mod.setAuthFromPat("ghp_token2", { ...userA, login: userA.login.toUpperCase() }); + expect(cb).not.toHaveBeenCalled(); + }); + + it("runs onAuthCleared callbacks when setAuthFromPat switches to a different GitHub identity", () => { + mod.setAuthFromPat("ghp_token1", userA); + const cb = vi.fn(); + mod.onAuthCleared(cb); + mod.setAuthFromPat("ghp_token2", userB); + expect(cb).toHaveBeenCalledTimes(1); + expect(mod.user()).toEqual(userB); + }); + + it("still adopts the new identity even if a registered callback throws during an identity switch", () => { + mod.setAuthFromPat("ghp_token1", userA); + mod.onAuthCleared(() => { + throw new Error("boom"); + }); + expect(() => mod.setAuthFromPat("ghp_token2", userB)).not.toThrow(); + expect(mod.user()).toEqual(userB); + expect(mod.token()).toBe("ghp_token2"); + }); + + it("clears config and view localStorage keys on a genuine identity switch", () => { + mod.setAuthFromPat("ghp_token1", userA); + localStorageMock.setItem("github-tracker:config", '{"theme":"dark"}'); + localStorageMock.setItem("github-tracker:view", '{"lastActiveTab":"actions"}'); + + mod.setAuthFromPat("ghp_token2", userB); + + expect(localStorageMock.getItem("github-tracker:config")).toBeNull(); + expect(localStorageMock.getItem("github-tracker:view")).toBeNull(); + }); + + it("preserves config and view localStorage keys when replacing a token for the same identity", () => { + mod.setAuthFromPat("ghp_token1", userA); + localStorageMock.setItem("github-tracker:config", '{"theme":"dark"}'); + localStorageMock.setItem("github-tracker:view", '{"lastActiveTab":"actions"}'); + + mod.setAuthFromPat("ghp_token2", userA); + + expect(localStorageMock.getItem("github-tracker:config")).toBe('{"theme":"dark"}'); + expect(localStorageMock.getItem("github-tracker:view")).toBe('{"lastActiveTab":"actions"}'); + }); +});