mirror of
https://github.com/MBombeck/HealthLog.git
synced 2026-11-08 04:32:42 +00:00
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.
This commit is contained in:
1 parent
c40b21de3d
commit
587ea35d34
6 files changed
+282
-48
No files matched your search
@@ -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<string, string>();
|
||||
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"]);
|
||||
|
||||
@@ -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<string>();
|
||||
|
||||
/**
|
||||
@@ -477,16 +444,20 @@ const deletesInFlight = new Set<string>();
|
||||
* `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();
|
||||
};
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
@@ -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();
|
||||
}
|
||||
|
||||
|
||||
@@ -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<string, string>();
|
||||
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();
|
||||
});
|
||||
});
|
||||
@@ -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 (`<userId>:<recordScope | "own">`). 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;
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user