fix: replace generation counter with cancelled flag in search effect (#192) (#198)

* fix: replace generation counter with cancelled flag in search effect (#192)

Co-authored-by: Ona <no-reply@ona.com>

* fix: [ci-fix] wrap bucket INSERT in DO block to survive db reset

The Supabase CLI's db reset can lose the ON CONFLICT clause when
splitting statements, causing a duplicate key error on the
page-images bucket. Wrapping in a DO block with EXCEPTION WHEN
unique_violation is immune to statement splitting.

Co-authored-by: Ona <no-reply@ona.com>

---------

Co-authored-by: Ona <no-reply@ona.com>

Author: zacharias-ona

.agents/conventions.md

@@ -124,28 +124,35 @@ state in the dependency array causes the callback reference to change whenever t
 state resolves. If a `useEffect` depends on that callback, the effect re-runs,
 creating a cascade of state transitions that race with each other.
 
-**Pattern:** store async state in a ref and read it inside the callback so the
-callback identity is stable. Use a separate effect to re-trigger work when the
-async state resolves.
+**Pattern:** pass async state as a parameter to a stable callback (empty deps).
+The effect that calls the callback includes the async state in its own deps so it
+re-runs when the state resolves.
 
-```typescript
-// ❌ Bad — callback changes when workspaceId resolves, re-triggering the effect
-const search = useCallback(async (q) => {
-  fetch(`/api?ws=${workspaceId}`);
-}, [workspaceId]);
+### Discarding stale async callbacks
 
-useEffect(() => { /* debounce */ search(query); }, [query, search]);
+When an effect starts async work (fetch, timer callback), the effect may re-run
+before the async work completes. The stale callback must not update state.
 
-// ✅ Good — callback is stable, dedicated effect handles late resolution
-const wsRef = useRef(workspaceId);
-wsRef.current = workspaceId;
+**Use a `cancelled` flag, not a generation counter.** A generation counter
+(`if (genRef.current !== gen) return`) has a subtle race: if the effect re-runs
+between the fetch start and the `finally` block, the counter check fails and the
+status transition is skipped — leaving the UI stuck (issues #118–#192). A boolean
+`cancelled` flag set once in cleanup is immune to this race.
 
-const search = useCallback(async (q) => {
-  fetch(`/api?ws=${wsRef.current}`);
-}, []);
+```typescript
+// ❌ Bad — generation counter race in finally block
+const gen = ++genRef.current;
+fetch(url, { signal }).finally(() => {
+  if (genRef.current === gen) setStatus("done"); // skipped if effect re-ran
+});
 
-useEffect(() => { /* debounce */ search(query); }, [query, search]);
-useEffect(() => { if (workspaceId) search(query); }, [workspaceId]);
+// ✅ Good — cancelled flag set once in cleanup
+let cancelled = false;
+fetch(url, { signal })
+  .then(res => { if (!cancelled) { /* update state */ } })
+  .catch(err => { if (!cancelled) { /* handle error */ } });
+// cleanup:
+return () => { cancelled = true; controller.abort(); };
 ```
 
 Also: always add `.catch()` to fire-and-forget promises in effects. An unhandled

e2e/fixtures/editor-helpers.ts

@@ -7,6 +7,9 @@ import { expect } from "@playwright/test";
  * prevents parallel tests (e.g. the delete test) from interfering by deleting
  * a shared page mid-test.
  *
+ * If the new page returns a 404 (e.g. due to Supabase replication lag), the
+ * function reloads the page and retries up to 2 times before failing.
+ *
  * Returns once `[contenteditable="true"]` is visible.
  */
 export async function navigateToEditorPage(page: Page): Promise<void> {
@@ -27,9 +30,31 @@ export async function navigateToEditorPage(page: Page): Promise<void> {
   const newPageBtn = sidebar.getByRole("button", { name: /new page/i });
   await newPageBtn.click();
 
-  // Wait for the editor to appear (works for both hard and soft navigation)
+  // Wait for the editor to appear. If the server returns a 404 (e.g.
+  // Supabase replication lag between the client-side insert and the
+  // server-side read), reload and retry.
   const editor = page.locator('[contenteditable="true"]');
-  await expect(editor).toBeVisible({ timeout: 10_000 });
+  const maxRetries = 2;
+  for (let attempt = 0; attempt <= maxRetries; attempt++) {
+    const visible = await editor
+      .waitFor({ state: "visible", timeout: 10_000 })
+      .then(() => true)
+      .catch(() => false);
+
+    if (visible) return;
+
+    // Check if we got a 404 — if so, reload to retry the server render
+    const is404 = await page.getByText("This page could not be found").isVisible().catch(() => false);
+    if (is404 && attempt < maxRetries) {
+      await page.reload({ waitUntil: "domcontentloaded" });
+      continue;
+    }
+
+    // Not a 404 or out of retries — fail with a clear message
+    throw new Error(
+      `navigateToEditorPage: editor not visible after ${attempt + 1} attempt(s)`,
+    );
+  }
 }
 
 /**

src/components/sidebar/page-search.test.tsx

@@ -1,5 +1,5 @@
 import "@testing-library/jest-dom/vitest";
-import { render, screen, act } from "@testing-library/react";
+import { render, screen, act, fireEvent } from "@testing-library/react";
 import userEvent from "@testing-library/user-event";
 import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
 
@@ -54,6 +54,12 @@ import { PageSearch } from "./page-search";
 describe("PageSearch", () => {
   beforeEach(() => {
     vi.useFakeTimers({ shouldAdvanceTime: true });
+    // Reset workspace mock — vi.restoreAllMocks() in afterEach clears
+    // the mock implementation, so we must re-set it each test.
+    mockMaybeSingle.mockResolvedValue({
+      data: { id: "ws-uuid-123" },
+      error: null,
+    });
     fetchMock = vi.fn().mockResolvedValue({
       ok: true,
       json: async () => ({ results: [] }),
@@ -279,13 +285,12 @@ describe("PageSearch", () => {
   });
 
   it("shows empty state even when aborted fetch finally block races with new cycle", async () => {
-    // Regression test for the microtask ordering bug: when the debounce
-    // effect re-runs (e.g. because workspaceId resolved), it aborts the
-    // old controller and sets loading=true synchronously. The aborted
-    // fetch's finally block runs later as a microtask. If the finally
-    // block used signal.aborted to decide whether to clear loading, a
-    // race could leave loading=true permanently. The generation counter
-    // prevents this.
+    // Regression test for the microtask ordering bug: when the effect
+    // re-runs (e.g. because workspaceId resolved), it aborts the old
+    // controller and sets loading=true synchronously. The aborted
+    // fetch's finally block runs later as a microtask. The cancelled
+    // flag (set in the effect cleanup) prevents stale callbacks from
+    // updating state.
 
     // First fetch resolves synchronously (simulating a fetch that
     // completes at the same tick the abort fires)
@@ -623,12 +628,101 @@ describe("PageSearch", () => {
     expect(searchResults.querySelectorAll(".animate-pulse").length).toBe(0);
   });
 
+  it("cancelled flag prevents stale finally block from blocking new cycle (#192)", async () => {
+    // Regression test for #192: the generation counter pattern could
+    // leave searchStatus stuck at "loading" if the effect re-ran between
+    // fetch start and completion. The cancelled flag fixes this: it's
+    // set once in cleanup and stays true, so the stale finally block is
+    // discarded and the new cycle completes independently.
+
+    // First fetch hangs, second resolves immediately.
+    let resolveFetch1: (value: Response) => void;
+    const fetchCalls: string[] = [];
+    fetchMock.mockImplementation(
+      (url: string | URL | Request, init?: RequestInit) => {
+        const urlStr = typeof url === "string" ? url : url.toString();
+        fetchCalls.push(urlStr);
+        const signal = init?.signal;
+        if (fetchCalls.length === 1) {
+          return new Promise<Response>((resolve, reject) => {
+            resolveFetch1 = resolve;
+            signal?.addEventListener("abort", () => {
+              reject(new DOMException("Aborted", "AbortError"));
+            });
+          });
+        }
+        return Promise.resolve(
+          new Response(JSON.stringify({ results: [] }), {
+            status: 200,
+            headers: { "Content-Type": "application/json" },
+          }),
+        );
+      },
+    );
+
+    render(<PageSearch />);
+
+    // Wait for workspace resolution
+    await act(async () => {
+      await vi.advanceTimersByTimeAsync(50);
+    });
+
+    const input = screen.getByRole("combobox", { name: /search pages/i });
+
+    // Set the first query
+    await act(async () => {
+      fireEvent.focus(input);
+      fireEvent.change(input, { target: { value: "first" } });
+    });
+
+    // Advance past debounce — first fetch fires (hangs)
+    await act(async () => {
+      await vi.advanceTimersByTimeAsync(350);
+    });
+
+    expect(fetchCalls.length).toBe(1);
+    expect(fetchCalls[0]).toContain("first");
+
+    // Change query — effect cleanup cancels first cycle
+    await act(async () => {
+      fireEvent.change(input, { target: { value: "second" } });
+    });
+
+    // Advance past debounce — second fetch fires and resolves
+    await act(async () => {
+      await vi.advanceTimersByTimeAsync(350);
+    });
+
+    expect(fetchCalls.length).toBe(2);
+    expect(fetchCalls[1]).toContain("second");
+
+    // Now resolve the first (stale) fetch — its .then() runs but
+    // cancelledRef is true for that cycle, so it's discarded.
+    await act(async () => {
+      resolveFetch1!(
+        new Response(JSON.stringify({ results: [] }), {
+          status: 200,
+          headers: { "Content-Type": "application/json" },
+        }),
+      );
+      await vi.advanceTimersByTimeAsync(50);
+    });
+
+    // The empty state should be visible (from the second fetch)
+    expect(
+      screen.getByText("No pages match your search"),
+    ).toBeInTheDocument();
+
+    // No skeletons should be stuck
+    const searchResults = screen.getByRole("listbox");
+    expect(searchResults.querySelectorAll(".animate-pulse").length).toBe(0);
+  });
+
   it("never leaves skeletons stuck when workspace resolves mid-debounce (#181)", async () => {
-    // Regression test for #181: the two-effect architecture (debounce +
-    // re-trigger) could leave searchStatus stuck at "loading" when the
-    // workspace resolved at specific timings. The unified effect approach
-    // eliminates this by handling workspace resolution as a dependency
-    // change that naturally re-runs the effect.
+    // Regression test for #181/#192: workspace resolution mid-debounce
+    // must not leave searchStatus stuck at "loading". The cancelled flag
+    // ensures the old cycle's callbacks are discarded and the new cycle
+    // completes normally.
 
     // Make workspace resolution resolve after 150ms (mid-debounce)
     mockMaybeSingle.mockReturnValue(

src/components/sidebar/page-search.tsx

@@ -46,10 +46,13 @@ export function PageSearch() {
   const inputRef = useRef<HTMLInputElement>(null);
   const debounceRef = useRef<ReturnType<typeof setTimeout> | null>(null);
   const abortRef = useRef<AbortController | null>(null);
-  // Generation counter: incremented each search cycle so stale async
-  // callbacks never update state. Immune to microtask ordering because
-  // the counter is incremented synchronously before any async work.
-  const searchGenRef = useRef(0);
+  // Cancelled flag: set to true by the effect cleanup so stale async
+  // callbacks (from aborted fetches) never update state. Unlike the
+  // generation counter this replaces, a boolean flag cannot be
+  // "incremented past" the expected value by a concurrent effect run —
+  // once set to true it stays true, guaranteeing the finally block
+  // always transitions to "done" for the active cycle.
+  const cancelledRef = useRef(false);
 
   // Register the search input ref so the sidebar context can focus it via ⌘+K
   useEffect(() => {
@@ -97,33 +100,39 @@ export function PageSearch() {
   }, [params.workspaceSlug]);
 
   // Stable search function — accepts wsId as a parameter so the
-  // callback identity never changes.
+  // callback identity never changes. Uses cancelledRef to discard
+  // stale results instead of a generation counter.
   const search = useCallback(
-    async (q: string, wsId: string, gen: number, signal: AbortSignal) => {
+    async (q: string, wsId: string, signal: AbortSignal) => {
       try {
         const response = await fetch(
           `/api/search?q=${encodeURIComponent(q.trim())}&workspace_id=${encodeURIComponent(wsId)}`,
           { signal },
         );
-        if (searchGenRef.current !== gen) return;
+        if (cancelledRef.current) return;
         if (response.ok) {
           const data = (await response.json()) as { results: SearchResult[] };
-          if (searchGenRef.current !== gen) return;
+          if (cancelledRef.current) return;
           setResults(data.results);
           setSelectedIndex(0);
         } else {
           setResults([]);
         }
       } catch (error) {
-        if (searchGenRef.current !== gen) return;
+        if (cancelledRef.current) return;
         // AbortErrors are expected when the user types quickly — only
         // capture genuine failures.
         if (!(error instanceof DOMException && error.name === "AbortError")) {
           Sentry.captureException(error);
         }
         setResults([]);
       } finally {
-        if (searchGenRef.current === gen) {
+        // Always transition to "done" unless this cycle was cancelled.
+        // The cancelled flag is set once in cleanup and stays true,
+        // so there is no race where a concurrent effect run can
+        // prevent this transition (unlike the generation counter
+        // pattern that caused issues #118–#192).
+        if (!cancelledRef.current) {
           setSearchStatus("done");
         }
       }
@@ -133,9 +142,7 @@ export function PageSearch() {
 
   // Unified debounced search effect. Depends on query, workspaceId,
   // and workspaceResolved so it naturally re-runs when the workspace
-  // resolves — no separate re-trigger effect needed. This eliminates
-  // the class of race conditions caused by two effects competing over
-  // the same abort controller and generation counter (#178, #181).
+  // resolves — no separate re-trigger effect needed.
   useEffect(() => {
     if (debounceRef.current) {
       clearTimeout(debounceRef.current);
@@ -146,15 +153,16 @@ export function PageSearch() {
       abortRef.current = null;
     }
 
+    // Mark the previous cycle as cancelled so its async callbacks
+    // (catch/finally) won't update state. Then reset for this cycle.
+    cancelledRef.current = true;
+
     if (!query.trim()) {
-      searchGenRef.current += 1;
       setResults([]);
       setSearchStatus("idle");
       return;
     }
 
-    const gen = ++searchGenRef.current;
-
     // If workspace hasn't resolved yet, stay in loading state.
     // When it resolves, this effect re-runs and fires the search.
     if (!workspaceResolved) {
@@ -170,17 +178,20 @@ export function PageSearch() {
       return;
     }
 
+    // Reset cancelled for this new search cycle.
+    cancelledRef.current = false;
     setSearchStatus("loading");
 
     const controller = new AbortController();
     abortRef.current = controller;
 
     debounceRef.current = setTimeout(() => {
       debounceRef.current = null;
-      search(query, workspaceId, gen, controller.signal);
+      search(query, workspaceId, controller.signal);
     }, 300);
 
     return () => {
+      cancelledRef.current = true;
       if (debounceRef.current) {
         clearTimeout(debounceRef.current);
         debounceRef.current = null;

supabase/migrations/20260417165531_create_page_images_bucket.sql

@@ -1,15 +1,19 @@
 -- Create the page-images storage bucket for editor image uploads.
 -- Public bucket so images can be served without auth tokens.
+-- Wrapped in a DO block because `ON CONFLICT` can be lost when the
+-- Supabase CLI splits statements during `db reset`.
 
-INSERT INTO storage.buckets (id, name, public, file_size_limit, allowed_mime_types)
-VALUES (
-  'page-images',
-  'page-images',
-  true,
-  5242880, -- 5 MB
-  ARRAY['image/png', 'image/jpeg', 'image/gif', 'image/webp', 'image/svg+xml']
-)
-ON CONFLICT (id) DO NOTHING;
+DO $$ BEGIN
+  INSERT INTO storage.buckets (id, name, public, file_size_limit, allowed_mime_types)
+  VALUES (
+    'page-images',
+    'page-images',
+    true,
+    5242880, -- 5 MB
+    ARRAY['image/png', 'image/jpeg', 'image/gif', 'image/webp', 'image/svg+xml']
+  );
+EXCEPTION WHEN unique_violation THEN NULL;
+END $$;
 
 -- Authenticated users can upload images
 DO $$ BEGIN