From dcc3575d2d94ec1c2ff19ea6298fbbe27eb92dcd Mon Sep 17 00:00:00 2001 From: Danielle Sheehan Date: Sun, 30 Aug 2026 18:31:02 -0400 Subject: [PATCH] Fix SidebarSearch debounce reset and stale-response race MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found via /code-review while working on an unrelated docs PR (it scanned the whole repo since that branch had no code diff of its own) — both issues checked out as real against the actual merged code, independent of the review's wrong line numbers. - getPage's identity changes on every commit anywhere in the workspace (WorkspaceContext replaces the `pages` array on each updatePageBlocks/updatePageTitle), not just when search-relevant state changes. It was in the debounce effect's deps, so typing in the main editor while a search was pending kept resetting or double-firing the debounce — burning calls against the budgeted search endpoint for no new input. Fixed by reading it through a ref (the standard "latest ref" pattern) instead, so the effect only depends on `query`. - searchNotes results had no staleness guard, unlike the sibling RelatedPagesSection's `cancelled` flag. A slower in-flight request for an old query could resolve after a newer, faster one and silently overwrite its correct results. Added the same guard. Both regression tests fail against the pre-fix code (verified via git stash) and pass against the fix. Co-Authored-By: Claude Sonnet 5 --- src/components/SidebarSearch.test.tsx | 143 ++++++++++++++++++++++++++ src/components/SidebarSearch.tsx | 18 +++- 2 files changed, 159 insertions(+), 2 deletions(-) create mode 100644 src/components/SidebarSearch.test.tsx diff --git a/src/components/SidebarSearch.test.tsx b/src/components/SidebarSearch.test.tsx new file mode 100644 index 0000000..55f0696 --- /dev/null +++ b/src/components/SidebarSearch.test.tsx @@ -0,0 +1,143 @@ +import { render, screen, fireEvent } from "@testing-library/react"; +import { act } from "react"; +import { MemoryRouter } from "react-router"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { WorkspaceContext, type WorkspaceContextValue } from "../context/workspace-context"; + +const mocks = vi.hoisted(() => ({ + isAiServiceConfigured: vi.fn(() => true), + searchNotes: vi.fn(), +})); + +vi.mock("../lib/aiClient", () => ({ + isAiServiceConfigured: mocks.isAiServiceConfigured, + searchNotes: mocks.searchNotes, +})); + +function createMockWorkspaceValue( + overrides: Partial = {}, +): WorkspaceContextValue { + const base: WorkspaceContextValue = { + pages: [], + databases: [], + homePageId: "home", + lastOpenedPageId: null, + externalWorkspaceRevision: 0, + remoteSyncStatus: "disabled", + remoteSyncError: null, + getPage: vi.fn(() => undefined), + getDatabase: vi.fn(), + setLastOpenedPageId: vi.fn(), + resolveOpenPageId: vi.fn(() => "home"), + updatePageTitle: vi.fn(), + updatePageBlocks: vi.fn(), + updateDatabase: vi.fn(), + createPage: vi.fn(), + createDatabasePage: vi.fn(), + deletePageSubtree: vi.fn(), + movePageWithinSiblings: vi.fn(), + ancestryFor: vi.fn(), + childrenOf: vi.fn(), + }; + return { ...base, ...overrides }; +} + +async function loadComponent() { + return (await import("./SidebarSearch")).default; +} + +describe("SidebarSearch", () => { + beforeEach(() => { + vi.useFakeTimers({ shouldAdvanceTime: true }); + mocks.isAiServiceConfigured.mockReset().mockReturnValue(true); + mocks.searchNotes.mockReset(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + it("does not reset or double-fire the debounce when getPage's identity changes mid-debounce", async () => { + mocks.searchNotes.mockResolvedValue({ matches: [] }); + const SidebarSearch = await loadComponent(); + const value = createMockWorkspaceValue(); + + const { rerender } = render( + + + + + , + ); + + fireEvent.change(screen.getByLabelText("Search notes"), { target: { value: "hello" } }); + + // Simulate an edit elsewhere in the workspace changing getPage's identity + // mid-debounce (WorkspaceContext gives getPage a new reference on every commit). + await act(async () => { + await vi.advanceTimersByTimeAsync(150); + }); + const nextValue = createMockWorkspaceValue({ getPage: vi.fn(() => undefined) }); + rerender( + + + + + , + ); + + await act(async () => { + await vi.advanceTimersByTimeAsync(200); + }); + + expect(mocks.searchNotes).toHaveBeenCalledTimes(1); + expect(mocks.searchNotes).toHaveBeenCalledWith("hello"); + }); + + it("does not let a stale, slower response overwrite a newer query's results", async () => { + let resolveFirst: (value: { matches: [] }) => void = () => {}; + const firstCall = new Promise<{ matches: [] }>((resolve) => { + resolveFirst = resolve; + }); + mocks.searchNotes.mockImplementationOnce(() => firstCall); + mocks.searchNotes.mockResolvedValueOnce({ + matches: [{ pageId: "p2", blockId: "b2", similarity: 0.9 }], + }); + + const SidebarSearch = await loadComponent(); + const value = createMockWorkspaceValue({ + getPage: vi.fn((id: string) => + id === "p2" ? ({ id: "p2", title: "Second Page", blocks: [] } as never) : undefined, + ), + }); + + render( + + + + + , + ); + + const input = screen.getByLabelText("Search notes"); + fireEvent.change(input, { target: { value: "first" } }); + await act(async () => { + await vi.advanceTimersByTimeAsync(300); + }); + + fireEvent.change(input, { target: { value: "second" } }); + await act(async () => { + await vi.advanceTimersByTimeAsync(300); + }); + + // Second (faster) request resolves first. + expect(await screen.findByText("Second Page")).toBeInTheDocument(); + + // Now the stale first request resolves — it must not clobber the current results. + await act(async () => { + resolveFirst({ matches: [] }); + }); + + expect(screen.getByText("Second Page")).toBeInTheDocument(); + }); +}); diff --git a/src/components/SidebarSearch.tsx b/src/components/SidebarSearch.tsx index 6b002f6..4852056 100644 --- a/src/components/SidebarSearch.tsx +++ b/src/components/SidebarSearch.tsx @@ -30,6 +30,15 @@ export default function SidebarSearch() { const containerRef = useRef(null); const debounceRef = useRef | null>(null); + // getPage's identity changes on every edit anywhere in the workspace (WorkspaceContext + // replaces the `pages` array on each commit), not just when search-relevant state + // changes. Reading it via a ref keeps the debounce effect below from resetting or + // double-firing on unrelated typing elsewhere in the app. + const getPageRef = useRef(getPage); + useEffect(() => { + getPageRef.current = getPage; + }, [getPage]); + useEffect(() => { const onOutsideClick = (e: MouseEvent) => { if (containerRef.current && !containerRef.current.contains(e.target as Node)) { @@ -49,13 +58,16 @@ export default function SidebarSearch() { return; } + let cancelled = false; + debounceRef.current = setTimeout(() => { searchNotes(trimmed) .then((result) => { + if (cancelled) return; const rows = dedupeByPage(result.matches) .slice(0, MAX_RESULTS) .map((match): SearchResultRow => { - const page = getPage(match.pageId); + const page = getPageRef.current(match.pageId); const block = page?.blocks.find((b) => b.id === match.blockId); const snippet = block ? blockHtmlToPlainText(block.content) : ""; return { @@ -72,6 +84,7 @@ export default function SidebarSearch() { setOpen(true); }) .catch((err: unknown) => { + if (cancelled) return; console.error("Search failed", err); setResults([]); setOpen(true); @@ -79,9 +92,10 @@ export default function SidebarSearch() { }, DEBOUNCE_MS); return () => { + cancelled = true; if (debounceRef.current) clearTimeout(debounceRef.current); }; - }, [query, getPage]); + }, [query]); if (!isAiServiceConfigured()) return null;