From 587ea35d34ffa16fc586f36009b97ec15b2cc105 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Fri, 9 Oct 2026 10:29:36 +0200 Subject: [PATCH] fix(coach): bind the delete journal to the account and record it was sent for A tab outlives a logout, another login and a switch into a managed profile. A delete re-sent in another scope met a 404 that read as "already gone" and dropped the entry, or tripped the record fence and toasted a failure for a row the person could not see. Entries now carry their owner, only the current owner's are re-sent, and a session end clears the journal with the rest of the session state. --- .../__tests__/use-coach-delete.test.ts | 35 +++++- .../insights/coach-panel/use-coach.ts | 72 ++++-------- .../__tests__/use-auth-session-clear.test.ts | 12 ++ src/hooks/use-auth.ts | 4 + .../ai/coach/__tests__/delete-journal.test.ts | 96 +++++++++++++++ src/lib/ai/coach/delete-journal.ts | 111 ++++++++++++++++++ 6 files changed, 282 insertions(+), 48 deletions(-) create mode 100644 src/lib/ai/coach/__tests__/delete-journal.test.ts create mode 100644 src/lib/ai/coach/delete-journal.ts diff --git a/src/components/insights/coach-panel/__tests__/use-coach-delete.test.ts b/src/components/insights/coach-panel/__tests__/use-coach-delete.test.ts index 09d83dbe8..7def301bb 100644 --- a/src/components/insights/coach-panel/__tests__/use-coach-delete.test.ts +++ b/src/components/insights/coach-panel/__tests__/use-coach-delete.test.ts @@ -6,6 +6,10 @@ import type { CoachConversationsPage, } from "@/lib/ai/coach/types"; import { queryKeys } from "@/lib/query-keys"; +import { + readCoachDeleteJournal, + writeCoachDeleteJournal, +} from "@/lib/ai/coach/delete-journal"; import { CONVERSATION_DELETE_UNDO_MS, commitCoachConversationDelete, @@ -180,18 +184,45 @@ describe("Coach conversation delete caches and request", () => { respond(404); const client = seeded(); const onFailed = vi.fn(); - commitCoachConversationDelete(client, "c1", onFailed); + commitCoachConversationDelete(client, "c1", null, onFailed); await vi.waitFor(() => expect(global.fetch).toHaveBeenCalledOnce()); await new Promise((resolve) => setTimeout(resolve, 0)); expect(onFailed).not.toHaveBeenCalled(); expect(headIds(client)).toEqual(["c2"]); }); + it("settles a 404 against the journal entry of its own scope only", async () => { + const map = new Map(); + vi.stubGlobal("window", { + sessionStorage: { + getItem: (key: string) => map.get(key) ?? null, + setItem: (key: string, value: string) => void map.set(key, value), + removeItem: (key: string) => void map.delete(key), + }, + }); + try { + // The same id journaled in another record must survive this settle. + writeCoachDeleteJournal("user-a:managed-1", "c1", true); + respond(404); + const client = seeded(); + const onFailed = vi.fn(); + commitCoachConversationDelete(client, "c1", "user-a:own", onFailed); + expect(readCoachDeleteJournal("user-a:own")).toEqual(["c1"]); + await vi.waitFor(() => + expect(readCoachDeleteJournal("user-a:own")).toEqual([]), + ); + expect(onFailed).not.toHaveBeenCalled(); + expect(readCoachDeleteJournal("user-a:managed-1")).toEqual(["c1"]); + } finally { + vi.unstubAllGlobals(); + } + }); + it("restores the lists and reports a failed delete", async () => { respond(500); const client = seeded(); const onFailed = vi.fn(); - commitCoachConversationDelete(client, "c1", onFailed); + commitCoachConversationDelete(client, "c1", null, onFailed); expect(headIds(client)).toEqual(["c2"]); await vi.waitFor(() => expect(onFailed).toHaveBeenCalledOnce()); expect(headIds(client)).toEqual(["c1", "c2"]); diff --git a/src/components/insights/coach-panel/use-coach.ts b/src/components/insights/coach-panel/use-coach.ts index 1572f2880..3aa90e76a 100644 --- a/src/components/insights/coach-panel/use-coach.ts +++ b/src/components/insights/coach-panel/use-coach.ts @@ -46,6 +46,13 @@ import { apiPost, } from "@/lib/api/api-fetch"; import { queryKeys } from "@/lib/query-keys"; +import { getRecordScope } from "@/lib/query-keys/record-scope"; +import { + coachDeleteJournalOwner, + resendCoachDeleteJournal, + writeCoachDeleteJournal, +} from "@/lib/ai/coach/delete-journal"; +import { useAuth } from "@/hooks/use-auth"; /** * v1.4.20 phase B2b — TanStack Query + SSE client for the AI Coach @@ -430,46 +437,6 @@ function subscribeHiddenConversations(listener: () => void): () => void { return () => hiddenConversationListeners.delete(listener); } -/** - * Deletes sent but not yet confirmed, kept in `sessionStorage` so they - * outlive a reload. A reload inside the undo window sends the DELETE with - * `keepalive` from the old page, and the new page's list read can reach the - * server before it: without the journal that read brings the row back. The - * new page hides every journaled id and sends its DELETE again; a 404 then - * means it is already gone. - */ -const DELETE_JOURNAL_KEY = "healthlog:coach-conversation-deletes"; - -function readDeleteJournal(): string[] { - try { - const raw = window.sessionStorage.getItem(DELETE_JOURNAL_KEY); - const parsed: unknown = raw ? JSON.parse(raw) : []; - return Array.isArray(parsed) - ? parsed.filter((id): id is string => typeof id === "string") - : []; - } catch { - return []; - } -} - -function writeDeleteJournal(id: string, present: boolean): void { - try { - const ids = new Set(readDeleteJournal()); - if (present) ids.add(id); - else ids.delete(id); - if (ids.size === 0) { - window.sessionStorage.removeItem(DELETE_JOURNAL_KEY); - } else { - window.sessionStorage.setItem( - DELETE_JOURNAL_KEY, - JSON.stringify([...ids]), - ); - } - } catch { - // Storage unavailable: the keepalive request still carries the delete. - } -} - const deletesInFlight = new Set(); /** @@ -477,16 +444,20 @@ const deletesInFlight = new Set(); * `pagehide` is on the wire before the page is torn down. Every list cache * drops the row first; a failure restores them, shows the row again and * calls `onFailed`. A 404 is success: the row is gone, which is the point. + * That reading holds because the delete is only ever sent in the scope it + * was journaled under (`owner`, see `@/lib/ai/coach/delete-journal`); with no + * known owner nothing is journaled and the keepalive request is all there is. */ export function commitCoachConversationDelete( queryClient: QueryClient, id: string, + owner: string | null, onFailed: () => void, ): void { if (deletesInFlight.has(id)) return; deletesInFlight.add(id); setConversationHidden(id, true); - writeDeleteJournal(id, true); + if (owner !== null) writeCoachDeleteJournal(owner, id, true); const request = deleteCoachConversationRequest(id); const snapshot = removeCoachConversationFromCaches(queryClient, id); request @@ -506,7 +477,7 @@ export function commitCoachConversationDelete( ) .finally(() => { deletesInFlight.delete(id); - writeDeleteJournal(id, false); + if (owner !== null) writeCoachDeleteJournal(owner, id, false); void queryClient.invalidateQueries({ queryKey: QUERY_KEYS.list() }); }); } @@ -522,7 +493,7 @@ export function commitCoachConversationDelete( * sent to the background, a tab switched away), on `pagehide` (reload, * close, navigation away) and when the owning component unmounts; the * request goes out with `keepalive`, and a delete the page could not confirm - * is sent again by the next one (see the journal above). Before that, a + * is sent again by the next one (see `@/lib/ai/coach/delete-journal`). Before that, a * delete followed by a reload inside the window was never sent, and the * conversation came back. * @@ -532,6 +503,11 @@ export function commitCoachConversationDelete( export function useDeleteCoachConversationWithUndo() { const queryClient = useQueryClient(); const { t } = useTranslations(); + const { user } = useAuth(); + // The account and the record this page acts on; the journal is bound to it. + const owner = user + ? coachDeleteJournalOwner(user.id, getRecordScope()) + : null; const pendingDeleteIds = useSyncExternalStore( subscribeHiddenConversations, () => hiddenConversationIds, @@ -544,7 +520,7 @@ export function useDeleteCoachConversationWithUndo() { useEffect(() => { const failedMessage = t("insights.coach.historyDeleteFailed"); commitRef.current = (id) => - commitCoachConversationDelete(queryClient, id, () => + commitCoachConversationDelete(queryClient, id, owner, () => toast.error(failedMessage), ); }); @@ -560,9 +536,13 @@ export function useDeleteCoachConversationWithUndo() { ); useEffect(() => { - // Finish what a previous page of this tab could not confirm. - for (const id of readDeleteJournal()) commitRef.current(id); + // Finish what a previous page of this tab could not confirm, in this + // scope only: another account's or another record's entries stay put. + if (owner === null) return; + resendCoachDeleteJournal(owner, (id) => commitRef.current(id)); + }, [owner]); + useEffect(() => { const onVisibilityChange = () => { if (document.visibilityState === "hidden") getQueue().flush(); }; diff --git a/src/hooks/__tests__/use-auth-session-clear.test.ts b/src/hooks/__tests__/use-auth-session-clear.test.ts index e98968f80..6c64d4a92 100644 --- a/src/hooks/__tests__/use-auth-session-clear.test.ts +++ b/src/hooks/__tests__/use-auth-session-clear.test.ts @@ -29,6 +29,12 @@ vi.mock("@/lib/day/last-day", () => ({ clearLastDay: () => clearLastDay(), })); +// Coach deletes the ending session could not confirm belong to its account. +const clearCoachDeleteJournal = vi.fn(); +vi.mock("@/lib/ai/coach/delete-journal", () => ({ + clearCoachDeleteJournal: () => clearCoachDeleteJournal(), +})); + // `use-auth` pulls in the typed fetch wrapper, the router, and the i18n // context at module load. None of them matter for the pure session-end // helper, so stub them so the import is side-effect free in the test env. @@ -48,6 +54,7 @@ import { clearCachesForSessionEnd } from "../use-auth"; afterEach(() => { clearOfflineCachesForSessionEnd.mockClear(); clearLastDay.mockClear(); + clearCoachDeleteJournal.mockClear(); }); describe("clearCachesForSessionEnd — cross-user in-memory wipe", () => { @@ -80,4 +87,9 @@ describe("clearCachesForSessionEnd — cross-user in-memory wipe", () => { clearCachesForSessionEnd(new QueryClient()); expect(clearLastDay).toHaveBeenCalledTimes(1); }); + + it("clears the Coach delete journal", () => { + clearCachesForSessionEnd(new QueryClient()); + expect(clearCoachDeleteJournal).toHaveBeenCalledTimes(1); + }); }); diff --git a/src/hooks/use-auth.ts b/src/hooks/use-auth.ts index a8830df66..038625e60 100644 --- a/src/hooks/use-auth.ts +++ b/src/hooks/use-auth.ts @@ -28,6 +28,7 @@ import type { ModuleAccessState } from "@/lib/sharing/module-disclosure"; import type { OnboardingStateDto } from "@/lib/onboarding/needs"; import type { TourProgress } from "@/lib/onboarding/tour-progress"; import { clearLastDay } from "@/lib/day/last-day"; +import { clearCoachDeleteJournal } from "@/lib/ai/coach/delete-journal"; import { clearOfflineCachesForSessionEnd } from "@/lib/pwa/query-persister"; import { setRecordScope, @@ -624,6 +625,9 @@ export function clearCachesForSessionEnd(queryClient: QueryClient): void { clearWasAuthenticated(); // The day the layer last showed is a date of this account's record. clearLastDay(); + // Coach deletes this session could not confirm belong to its account; the + // next one must not re-send them against its own record. + clearCoachDeleteJournal(); void clearOfflineCachesForSessionEnd(); } diff --git a/src/lib/ai/coach/__tests__/delete-journal.test.ts b/src/lib/ai/coach/__tests__/delete-journal.test.ts new file mode 100644 index 000000000..a3be2f9cd --- /dev/null +++ b/src/lib/ai/coach/__tests__/delete-journal.test.ts @@ -0,0 +1,96 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import { + clearCoachDeleteJournal, + coachDeleteJournalOwner, + readCoachDeleteJournal, + resendCoachDeleteJournal, + writeCoachDeleteJournal, +} from "../delete-journal"; + +function memoryStorage(): Storage { + const map = new Map(); + return { + get length() { + return map.size; + }, + clear: () => map.clear(), + getItem: (key) => map.get(key) ?? null, + key: (index) => [...map.keys()][index] ?? null, + removeItem: (key) => void map.delete(key), + setItem: (key, value) => void map.set(key, String(value)), + }; +} + +const KEY = "healthlog:coach-conversation-deletes"; +let storage: Storage; + +beforeEach(() => { + storage = memoryStorage(); + vi.stubGlobal("window", { sessionStorage: storage }); +}); +afterEach(() => { + vi.unstubAllGlobals(); +}); + +const alice = coachDeleteJournalOwner("user-a", null); +const aliceInManaged = coachDeleteJournalOwner("user-a", "managed-1"); +const bob = coachDeleteJournalOwner("user-b", null); + +describe("Coach delete journal", () => { + it("binds each entry to the account and the record it was sent for", () => { + writeCoachDeleteJournal(alice, "c1", true); + writeCoachDeleteJournal(aliceInManaged, "c2", true); + expect(readCoachDeleteJournal(alice)).toEqual(["c1"]); + expect(readCoachDeleteJournal(aliceInManaged)).toEqual(["c2"]); + expect(readCoachDeleteJournal(bob)).toEqual([]); + + writeCoachDeleteJournal(alice, "c1", false); + expect(readCoachDeleteJournal(alice)).toEqual([]); + expect(readCoachDeleteJournal(aliceInManaged)).toEqual(["c2"]); + }); + + it("re-sends nothing after a logout and another login in the same tab", () => { + writeCoachDeleteJournal(alice, "c1", true); + const commit = vi.fn(); + resendCoachDeleteJournal(bob, commit); + expect(commit).not.toHaveBeenCalled(); + // The entry waits for its own scope instead of being spent on the wrong + // one, where a 404 would have read as "already gone". + expect(readCoachDeleteJournal(alice)).toEqual(["c1"]); + }); + + it("re-sends nothing outside the record after a switch into a managed profile", () => { + writeCoachDeleteJournal(alice, "own-1", true); + writeCoachDeleteJournal(aliceInManaged, "managed-c", true); + + const inManaged = vi.fn(); + resendCoachDeleteJournal(aliceInManaged, inManaged); + expect(inManaged.mock.calls).toEqual([["managed-c"]]); + + const backHome = vi.fn(); + resendCoachDeleteJournal(alice, backHome); + expect(backHome.mock.calls).toEqual([["own-1"]]); + }); + + it("drops entries without an owner instead of re-sending them anywhere", () => { + storage.setItem(KEY, JSON.stringify(["legacy-id"])); + const commit = vi.fn(); + resendCoachDeleteJournal(alice, commit); + expect(commit).not.toHaveBeenCalled(); + }); + + it("is emptied by a session end, whoever the entries belonged to", () => { + writeCoachDeleteJournal(alice, "c1", true); + writeCoachDeleteJournal(bob, "c2", true); + clearCoachDeleteJournal(); + expect(storage.getItem(KEY)).toBeNull(); + }); + + it("stays quiet without storage", () => { + vi.stubGlobal("window", undefined); + expect(() => writeCoachDeleteJournal(alice, "c1", true)).not.toThrow(); + expect(readCoachDeleteJournal(alice)).toEqual([]); + expect(() => clearCoachDeleteJournal()).not.toThrow(); + }); +}); diff --git a/src/lib/ai/coach/delete-journal.ts b/src/lib/ai/coach/delete-journal.ts new file mode 100644 index 000000000..372092e4f --- /dev/null +++ b/src/lib/ai/coach/delete-journal.ts @@ -0,0 +1,111 @@ +/** + * Coach conversation deletes sent but not yet confirmed, kept in + * `sessionStorage` so they outlive a reload. A reload inside the undo window + * sends the DELETE with `keepalive` from the old page, and the new page's list + * read can reach the server before it: without the journal that read brings + * the row back. The new page hides every journaled id and sends its DELETE + * again; a 404 then means it is already gone. + * + * Every entry carries the owner it was sent for: the signed-in account and + * the record it was reading (`:`). A tab outlives + * a logout, another login and a switch into a managed profile, and a delete + * re-sent in any of those other scopes asks the wrong record: the server + * answers 404, which would read as "already gone" and drop the entry while the + * conversation still exists, or the record fence refuses it and a failure + * toast names a row the person cannot see. So only entries of the current + * owner are re-sent, which is also what keeps a 404 meaningful, and a session + * end clears the journal with the rest of the session's state. + */ +const DELETE_JOURNAL_KEY = "healthlog:coach-conversation-deletes"; + +export interface CoachDeleteJournalEntry { + id: string; + owner: string; +} + +/** The journal owner for an account reading `recordScope` (null = own). */ +export function coachDeleteJournalOwner( + userId: string, + recordScope: string | null, +): string { + return `${userId}:${recordScope ?? "own"}`; +} + +function safeSessionStorage(): Storage | null { + try { + return typeof window === "undefined" ? null : window.sessionStorage; + } catch { + return null; + } +} + +function readAll(storage: Storage): CoachDeleteJournalEntry[] { + try { + const raw = storage.getItem(DELETE_JOURNAL_KEY); + const parsed: unknown = raw ? JSON.parse(raw) : []; + if (!Array.isArray(parsed)) return []; + // An entry without an owner (the earlier bare-id shape) cannot be bound + // to a scope and is dropped rather than re-sent anywhere. + return parsed.filter( + (entry): entry is CoachDeleteJournalEntry => + typeof entry === "object" && + entry !== null && + typeof (entry as CoachDeleteJournalEntry).id === "string" && + typeof (entry as CoachDeleteJournalEntry).owner === "string", + ); + } catch { + return []; + } +} + +/** The ids journaled for `owner`, and only those. */ +export function readCoachDeleteJournal(owner: string): string[] { + const storage = safeSessionStorage(); + if (!storage) return []; + return readAll(storage) + .filter((entry) => entry.owner === owner) + .map((entry) => entry.id); +} + +/** + * Re-send what a previous page of this tab could not confirm, for `owner` + * only: another account's or another record's entries stay where they are + * until their own scope comes back or the session ends. + */ +export function resendCoachDeleteJournal( + owner: string, + commit: (id: string) => void, +): void { + for (const id of readCoachDeleteJournal(owner)) commit(id); +} + +/** Add (`present`) or remove one delete of `owner`. Best effort. */ +export function writeCoachDeleteJournal( + owner: string, + id: string, + present: boolean, +): void { + const storage = safeSessionStorage(); + if (!storage) return; + try { + const entries = readAll(storage).filter( + (entry) => !(entry.owner === owner && entry.id === id), + ); + if (present) entries.push({ id, owner }); + if (entries.length === 0) storage.removeItem(DELETE_JOURNAL_KEY); + else storage.setItem(DELETE_JOURNAL_KEY, JSON.stringify(entries)); + } catch { + // Storage unavailable: the keepalive request still carries the delete. + } +} + +/** Forget every journaled delete, whoever it belonged to (a session end). */ +export function clearCoachDeleteJournal(): void { + const storage = safeSessionStorage(); + if (!storage) return; + try { + storage.removeItem(DELETE_JOURNAL_KEY); + } catch { + return; + } +}