Skip to content

Commit 5eb21cd

Browse files
thephezclaude
andcommitted
fix(dashnote): apply CodeRabbit review feedback
Address actionable findings from CodeRabbit review on PR #74. Net effect: nine bug fixes / a11y improvements in dashnote, plus tighter test coverage for the cache, session, and reload-stale-response invariants. - a11y: drawer toggle aria-label flips with state; OperationResultNotice uses assertive aria-live for errors so role="alert" semantics hold - LoginModal: clear stale error and mnemonic when modal reopens - NotesWorkspace: monotonic reload token + session-snapshot guard so a late listMyNotes response from a previous identity/contract can't clobber state or write the wrong workspace into cache - notesCache: key by identityId + contractId + network (per CLAUDE.md); clearCachedNotes sweeps every contract+network slot for an identity - useTheme: optional-chain .matches so missing matchMedia falls back to "dark" instead of throwing - SessionContext: isolate DPNS lookup failures so a name-service hiccup doesn't fail an otherwise-valid session; clear any prior remembered identity when login uses rememberMe: false (prevents logout falling back to the wrong identity) - App.test.tsx: import ReactNode instead of using bare React.ReactNode (jsx: react-jsx doesn't put React in scope as a namespace) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 7fa48e7 commit 5eb21cd

11 files changed

Lines changed: 277 additions & 33 deletions

File tree

example-apps/dashnote/src/components/AppShell.tsx

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,8 @@ export function AppShell({
133133
<button
134134
type="button"
135135
onClick={() => setDrawerOpen(!drawerOpen)}
136-
aria-label="Open menu"
136+
aria-label={drawerOpen ? "Close menu" : "Open menu"}
137+
title={drawerOpen ? "Close menu" : "Open menu"}
137138
aria-expanded={drawerOpen}
138139
className="flex h-8 w-8 items-center justify-center rounded-md text-ink-3 hover:text-ink"
139140
>

example-apps/dashnote/src/components/LoginModal.tsx

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,8 @@ export function LoginModal({ open, onClose }: LoginModalProps) {
3535
if (open) {
3636
setRememberMe(true);
3737
setUseDifferentIdentity(false);
38+
setError(null);
39+
setMnemonic("");
3840
}
3941
}, [open]);
4042

example-apps/dashnote/src/components/NotesWorkspace.tsx

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,9 @@ export function NotesWorkspace({
7575
const [conflictWarning, setConflictWarning] = useState<string | null>(null);
7676
const lastRevalidatedAt = useRef(0);
7777
const inFlightWriteRef = useRef(false);
78+
// Monotonic token so a late listMyNotes() response from a previous
79+
// identity/contract/session can't clobber state for the current one.
80+
const reloadTokenRef = useRef(0);
7881
// Mirror editor state in refs so revalidation routines can compare against
7982
// the live values without participating in their dependency arrays (which
8083
// would re-fire effects on every keystroke).
@@ -158,13 +161,25 @@ export function NotesWorkspace({
158161
if (!hadNotes) setListLoading(true);
159162
setRevalidating(true);
160163
setError(null);
164+
reloadTokenRef.current += 1;
165+
const myToken = reloadTokenRef.current;
166+
const startedIdentityId = identityId;
167+
const startedContractId = contractId;
161168
try {
162169
const nextNotes = await listMyNotes({
163170
sdk,
164171
contractId,
165172
ownerId: identityId,
166173
log,
167174
});
175+
// Bail if a newer reload started, or session keys changed under us.
176+
if (
177+
reloadTokenRef.current !== myToken ||
178+
startedIdentityId !== identityId ||
179+
startedContractId !== contractId
180+
) {
181+
return;
182+
}
168183
lastRevalidatedAt.current = Date.now();
169184
const changed = !notesEqualByRevision(prevNotes, nextNotes);
170185
if (changed) {
@@ -217,11 +232,14 @@ export function NotesWorkspace({
217232
});
218233
setEditsReady(true);
219234
} catch (err) {
235+
if (reloadTokenRef.current !== myToken) return;
220236
setError(errorMessage(err));
221237
if (!hadNotes) setNotes([]);
222238
} finally {
223-
setListLoading(false);
224-
setRevalidating(false);
239+
if (reloadTokenRef.current === myToken) {
240+
setListLoading(false);
241+
setRevalidating(false);
242+
}
225243
}
226244
},
227245
[contractId, identityId, log, sdk, status, isDesktop],

example-apps/dashnote/src/components/OperationResultNotice.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ export function OperationResultNotice({
2525
return (
2626
<div
2727
role={tone === "error" ? "alert" : "status"}
28-
aria-live="polite"
28+
aria-live={tone === "error" ? "assertive" : "polite"}
2929
className={`rounded-lg border px-4 py-3 max-md:rounded-none max-md:border-0 max-md:bg-transparent max-md:px-4 max-md:py-3 ${toneClass[tone]}`}
3030
>
3131
<div className="text-[10px] font-semibold uppercase tracking-[0.12em]">

example-apps/dashnote/src/hooks/useTheme.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ export function getInitialTheme(): Theme {
88
if (typeof window === "undefined") return "dark";
99
const stored = window.localStorage.getItem(STORAGE_KEY);
1010
if (stored === "light" || stored === "dark") return stored;
11-
if (window.matchMedia?.("(prefers-color-scheme: light)").matches) {
11+
if (window.matchMedia?.("(prefers-color-scheme: light)")?.matches) {
1212
return "light";
1313
}
1414
return "dark";

example-apps/dashnote/src/lib/notesCache.ts

Lines changed: 31 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,19 @@ interface CachedWorkspace {
2222
notes: NoteRecord[];
2323
}
2424

25-
function storageKey(identityId: string): string {
26-
return `${STORAGE_PREFIX}${identityId}`;
25+
function storageKey(
26+
identityId: string,
27+
contractId: string,
28+
network: Network,
29+
): string {
30+
return `${STORAGE_PREFIX}${identityId}.${contractId}.${network}`;
31+
}
32+
33+
// Prefix for every cache entry belonging to a single identity, regardless
34+
// of which contract/network it was scoped to. Used by clearCachedNotes
35+
// when the caller doesn't know (or care about) the contract+network.
36+
function identityPrefix(identityId: string): string {
37+
return `${STORAGE_PREFIX}${identityId}.`;
2738
}
2839

2940
export function loadCachedNotes(
@@ -32,8 +43,9 @@ export function loadCachedNotes(
3243
network: Network,
3344
): NoteRecord[] | null {
3445
if (!identityId || !contractId) return null;
46+
const key = storageKey(identityId, contractId, network);
3547
try {
36-
const raw = localStorage.getItem(storageKey(identityId));
48+
const raw = localStorage.getItem(key);
3749
if (!raw) return null;
3850
const parsed = JSON.parse(raw) as Partial<CachedWorkspace>;
3951
if (
@@ -43,13 +55,13 @@ export function loadCachedNotes(
4355
parsed.network !== network ||
4456
!Array.isArray(parsed.notes)
4557
) {
46-
localStorage.removeItem(storageKey(identityId));
58+
localStorage.removeItem(key);
4759
return null;
4860
}
4961
return parsed.notes as NoteRecord[];
5062
} catch {
5163
try {
52-
localStorage.removeItem(storageKey(identityId));
64+
localStorage.removeItem(key);
5365
} catch {
5466
// ignore
5567
}
@@ -73,16 +85,28 @@ export function saveCachedNotes(
7385
notes,
7486
};
7587
try {
76-
localStorage.setItem(storageKey(identityId), JSON.stringify(payload));
88+
localStorage.setItem(
89+
storageKey(identityId, contractId, network),
90+
JSON.stringify(payload),
91+
);
7792
} catch {
7893
// ignore — quota or disabled storage
7994
}
8095
}
8196

97+
// Clears every cached workspace for `identityId` across all contract/network
98+
// combinations. Forget-identity / logout flows don't know which combos the
99+
// user has visited, so we sweep by prefix.
82100
export function clearCachedNotes(identityId: string): void {
83101
if (!identityId) return;
84102
try {
85-
localStorage.removeItem(storageKey(identityId));
103+
const prefix = identityPrefix(identityId);
104+
const toRemove: string[] = [];
105+
for (let i = 0; i < localStorage.length; i += 1) {
106+
const k = localStorage.key(i);
107+
if (k && k.startsWith(prefix)) toRemove.push(k);
108+
}
109+
toRemove.forEach((k) => localStorage.removeItem(k));
86110
} catch {
87111
// ignore
88112
}

example-apps/dashnote/src/session/SessionContext.tsx

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -163,15 +163,23 @@ export function SessionProvider({ children }: { children: ReactNode }) {
163163

164164
// Resolve the DPNS name after auth so we can persist it alongside
165165
// the identity ID — DPNS bindings are permanent, so what we save
166-
// here is correct for every future load.
166+
// here is correct for every future load. A naming-service failure
167+
// shouldn't kill an otherwise-valid session: caption is optional.
167168
let resolvedName: string | null = null;
168169
if (resolvedId) {
169-
resolvedName = await resolveDpnsName(connected, resolvedId);
170+
try {
171+
resolvedName = await resolveDpnsName(connected, resolvedId);
172+
} catch (dpnsErr) {
173+
log(`DPNS lookup failed: ${errorMessage(dpnsErr)}`, "info");
174+
}
170175
setDpnsName(resolvedName);
171176
}
172177
if (rememberMe && resolvedId) {
173178
saveRememberedIdentity({ id: resolvedId, name: resolvedName });
174179
setRememberedIdentityId(resolvedId);
180+
} else {
181+
clearRememberedIdentity();
182+
setRememberedIdentityId(null);
175183
}
176184
} catch (err) {
177185
const message = errorMessage(err);
@@ -229,10 +237,14 @@ export function SessionProvider({ children }: { children: ReactNode }) {
229237
// (e.g. the identity registered a name after we last saved, or this
230238
// record predates the dpnsName field).
231239
if (!remembered.name) {
232-
const fresh = await resolveDpnsName(connected, remembered.id);
233-
if (fresh) {
234-
setDpnsName(fresh);
235-
saveRememberedIdentity({ id: remembered.id, name: fresh });
240+
try {
241+
const fresh = await resolveDpnsName(connected, remembered.id);
242+
if (fresh) {
243+
setDpnsName(fresh);
244+
saveRememberedIdentity({ id: remembered.id, name: fresh });
245+
}
246+
} catch (dpnsErr) {
247+
log(`DPNS lookup failed: ${errorMessage(dpnsErr)}`, "info");
236248
}
237249
}
238250
} catch (err) {

example-apps/dashnote/test/App.test.tsx

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import {
77
screen,
88
waitFor,
99
} from "@testing-library/react";
10+
import type { ReactNode } from "react";
1011
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
1112

1213
import App from "../src/App";
@@ -37,7 +38,7 @@ vi.mock("../src/components/AppShell", () => ({
3738
onLoginOpen,
3839
onTabChange,
3940
}: {
40-
children: React.ReactNode;
41+
children: ReactNode;
4142
onLoginOpen: () => void;
4243
onTabChange: (tab: "notes" | "how-it-works") => void;
4344
}) => (

example-apps/dashnote/test/NotesWorkspace.test.tsx

Lines changed: 83 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -293,7 +293,9 @@ describe("NotesWorkspace", () => {
293293

294294
// The cache should hold the merged (fresh) revision so a cold reload
295295
// would paint the up-to-date content immediately.
296-
const cacheRaw = localStorage.getItem("dashnote.notes.identity-1");
296+
const cacheRaw = localStorage.getItem(
297+
"dashnote.notes.identity-1.contract-1.testnet",
298+
);
297299
expect(cacheRaw).toBeTruthy();
298300
const cached = JSON.parse(cacheRaw as string);
299301
expect(cached.notes).toHaveLength(1);
@@ -687,7 +689,7 @@ describe("NotesWorkspace", () => {
687689
describe("cache hydration & revalidation", () => {
688690
function seedCache(notes: unknown[], identityId = "identity-1") {
689691
localStorage.setItem(
690-
`dashnote.notes.${identityId}`,
692+
`dashnote.notes.${identityId}.contract-1.testnet`,
691693
JSON.stringify({
692694
version: 1,
693695
identityId,
@@ -937,6 +939,85 @@ describe("NotesWorkspace", () => {
937939
expect(screen.queryByDisplayValue("Mobile body")).toBeNull();
938940
});
939941

942+
it("ignores a late listMyNotes response from a previous identity/contract", async () => {
943+
// Scenario: workspace is mounted for identity-1 + contract-1, and
944+
// listMyNotes is in flight. Before it resolves, the user switches to
945+
// identity-2 + contract-2 (re-render with a new session). The first
946+
// request finally resolves with notes for the *previous* session — it
947+
// must be ignored, both for state (no stale notes painted) and for
948+
// cache writes (no notes saved under the previous identity/contract).
949+
mockUseSession.mockReturnValue(makeSession());
950+
951+
// First call: hold open so we can resolve it after the switch.
952+
let resolveFirst: (value: unknown[]) => void = () => {};
953+
// Second call: also hold open so we can assert against the *old*
954+
// response specifically without the new one painting first.
955+
let resolveSecond: (value: unknown[]) => void = () => {};
956+
mockListMyNotes
957+
.mockImplementationOnce(
958+
() =>
959+
new Promise((resolve) => {
960+
resolveFirst = resolve;
961+
}),
962+
)
963+
.mockImplementationOnce(
964+
() =>
965+
new Promise((resolve) => {
966+
resolveSecond = resolve;
967+
}),
968+
);
969+
mockGetNote.mockImplementation(() => new Promise(() => {}));
970+
971+
const { rerender } = render(<NotesWorkspace onOpenSettings={vi.fn()} />);
972+
973+
// Switch to a new identity+contract while the first listMyNotes is
974+
// still pending. Re-rendering with a fresh session value triggers the
975+
// hydrate effect, which kicks off a new reloadNotes (and bumps the
976+
// reload token).
977+
mockUseSession.mockReturnValue(
978+
makeSession({ identityId: "identity-2", contractId: "contract-2" }),
979+
);
980+
rerender(<NotesWorkspace onOpenSettings={vi.fn()} />);
981+
982+
// Wait until the post-rerender hydrate effect has actually issued its
983+
// own listMyNotes call. This proves the new reload token is in place
984+
// before we resolve the stale request — otherwise the test could pass
985+
// by microtask-timing accident rather than by the guard's logic.
986+
await waitFor(() => {
987+
expect(mockListMyNotes).toHaveBeenCalledTimes(2);
988+
});
989+
990+
// Now resolve the *stale* first request with notes that belong to the
991+
// previous session. The guard inside reloadNotes should drop them.
992+
const staleNote = {
993+
id: "stale-1",
994+
ownerId: "identity-1",
995+
title: "Stale title from old session",
996+
message: "should not appear",
997+
createdAt: 1000,
998+
updatedAt: 2000,
999+
revision: 1,
1000+
};
1001+
resolveFirst([staleNote]);
1002+
1003+
// Poll the invariant directly — `waitFor` retries until microtasks
1004+
// queued by resolveFirst() have settled. If the guard is missing,
1005+
// this fails (cache write happens synchronously in the await
1006+
// continuation); if it's working, the cache stays null.
1007+
await waitFor(() => {
1008+
expect(
1009+
localStorage.getItem("dashnote.notes.identity-1.contract-1.testnet"),
1010+
).toBeNull();
1011+
});
1012+
expect(screen.queryByText(/stale title from old session/i)).toBeNull();
1013+
expect(
1014+
localStorage.getItem("dashnote.notes.identity-2.contract-2.testnet"),
1015+
).toBeNull();
1016+
1017+
// Clean up: let the second pending promise resolve so cleanup() doesn't hang.
1018+
resolveSecond([]);
1019+
});
1020+
9401021
it("keeps cached notes visible when the network revalidation fails", async () => {
9411022
mockUseSession.mockReturnValue(makeSession());
9421023
seedCache([

0 commit comments

Comments
 (0)