From 28300d49bda547896f284440b8461ad37597e290 Mon Sep 17 00:00:00 2001 From: Daniil Perkin Date: Sun, 27 Sep 2026 22:19:31 +0200 Subject: [PATCH 1/4] fix(board): sync the cached board after buddy and authored card writes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue #233: keeping an AI reply as a checklist wrote the card server-side, but the board kept serving what it had already read — a stale cache, not a stale server. The write path (POST /me/board/cards) had no relationship to the board query, so an open board behind the floating dock never refetched at all, and a visit within the 30s `staleTime` was served the pre-card board. This closes every write into the board that lives outside the board itself, following the pattern PathStepCard already established: - SaveReplyToBoard ("Keep as checklist") — the reported defect. - SaveToBoard ("Keep on my board") — the shared button behind chat replies, buddy replies and task cards. - SelectionActions ("Add to board") — cards authored from a selection. - useBuddyConversation — the buddy's own board writes: - confirmed board actions (place/amend/tick/reword checklist, place_note) invalidate after a write that actually succeeded. `claim_goal` is in the set too: confirming it also pins the CURRENT_TASK card onto the board ("It's on your board too"). - the `place_card` tool applies mid-stream with no confirmation step, so the `tool_use` event is the only signal a card landed; the turn marks the board stale when it ends — the failing path included, since a turn that placed a card and then broke still wrote one. The three component paths invalidate by project (`board.byProject`); the buddy hook invalidates `board.all()`, because the backend re-resolves the project server-side for buddy writes and the client never learns which board it wrote. Writes that fail invalidate nothing — a broken save must not cost the board its cache. The remaining addCard caller, useGeneratedPathCards, needs nothing here: its only caller (BoardPage's generate flow) already calls the board's own `refresh` after applying the plan. Refs #233 --- src/features/board/save/SaveToBoard.tsx | 11 +++ .../board/selection/SelectionActions.tsx | 11 ++- .../buddy/components/SaveReplyToBoard.tsx | 9 +++ .../buddy/hooks/useBuddyConversation.ts | 70 ++++++++++++++++++- src/features/buddy/types.ts | 17 +++++ 5 files changed, 115 insertions(+), 3 deletions(-) diff --git a/src/features/board/save/SaveToBoard.tsx b/src/features/board/save/SaveToBoard.tsx index cd581838..a3fae6d3 100644 --- a/src/features/board/save/SaveToBoard.tsx +++ b/src/features/board/save/SaveToBoard.tsx @@ -1,8 +1,10 @@ import { useState, type ReactNode } from "react"; +import { useQueryClient } from "@tanstack/react-query"; import { Button, type ButtonSize } from "../../../components/ui/Button"; import { useToast } from "../../../context/useToast"; import { boardService } from "../../../services/boardService"; +import { queryKeys } from "../../../services/queryKeys"; import { useProjectContext } from "../../projects/useProjectContext"; import { rememberOrigin, type CardOrigin } from "../layout/cardOrigins"; import type { AuthoredCardRequest } from "../types"; @@ -76,6 +78,7 @@ export function SaveToBoard({ }: SaveToBoardProps) { const { selectedProjectId } = useProjectContext(); const toast = useToast(); + const queryClient = useQueryClient(); const [saving, setSaving] = useState(false); const [saved, setSaved] = useState(false); @@ -90,6 +93,14 @@ export function SaveToBoard({ try { const created = await boardService.addCard(selectedProjectId, card); + // Kept from wherever this was found — a chat, a buddy reply, a task card — so the board is + // usually not on screen, and even where it is (this button lives on it too) what drew it is + // an earlier read. Marking the cache stale is what makes whichever comes next — the open + // board, or the next visit — show the card instead of the state it was read at. + void queryClient.invalidateQueries({ + queryKey: queryKeys.board.byProject(selectedProjectId), + }); + const where = origin?.(); // Never allowed to fail the save: the card is what was asked for, the trail back is extra. if (where) rememberOrigin(selectedProjectId, created.id, where); diff --git a/src/features/board/selection/SelectionActions.tsx b/src/features/board/selection/SelectionActions.tsx index 70c7197d..3dc4a4a4 100644 --- a/src/features/board/selection/SelectionActions.tsx +++ b/src/features/board/selection/SelectionActions.tsx @@ -1,4 +1,5 @@ import { useCallback, useContext, useState } from "react"; +import { useQueryClient } from "@tanstack/react-query"; import { useNavigate } from "react-router-dom"; import { BookmarkPlus, Eraser, Highlighter, MessageCircle, Reply } from "lucide-react"; import { Button } from "../../../components/ui/Button"; @@ -7,6 +8,7 @@ import { useFocusMode } from "../../../context/useFocusMode"; import { useProjectContext } from "../../projects/useProjectContext"; import { ChatContext } from "../../../context/ChatContext"; import { boardService } from "../../../services/boardService"; +import { queryKeys } from "../../../services/queryKeys"; import { rememberOrigin } from "../layout/cardOrigins"; import { useCardMarks } from "../marks/useCardMarks"; import { DEFAULT_HIGHLIGHT } from "../marks/highlightColors"; @@ -48,6 +50,7 @@ export function SelectionActions() { const quoteSelection = chatContext?.quoteSelection; const [saving, setSaving] = useState(false); const toast = useToast(); + const queryClient = useQueryClient(); const navigate = useNavigate(); const add = useCallback(async () => { @@ -65,6 +68,12 @@ export function SelectionActions() { url: selection.origin, label: selection.source ?? "where you were", }); + // Found anywhere in the app, so the board is almost never on screen — and where it is, its + // cache was read before this card existed. Marking it stale is what makes the next board + // read — a visit, or the open board behind this page — show it. + void queryClient.invalidateQueries({ + queryKey: queryKeys.board.byProject(selectedProjectId), + }); toast.success( request.kind === "LINK" ? "Link saved to your board" : "Note saved to your board", { @@ -78,7 +87,7 @@ export function SelectionActions() { } finally { setSaving(false); } - }, [selection, selectedProjectId, toast, navigate, clear]); + }, [selection, selectedProjectId, toast, navigate, clear, queryClient]); /** * Hands the selection to the buddy as a quote, unsent. diff --git a/src/features/buddy/components/SaveReplyToBoard.tsx b/src/features/buddy/components/SaveReplyToBoard.tsx index 368f469b..fc76e438 100644 --- a/src/features/buddy/components/SaveReplyToBoard.tsx +++ b/src/features/buddy/components/SaveReplyToBoard.tsx @@ -1,8 +1,10 @@ import { useState } from "react"; +import { useQueryClient } from "@tanstack/react-query"; import { ListPlus } from "lucide-react"; import { Button } from "../../../components/ui/Button"; import { useToast } from "../../../context/useToast"; import { boardService } from "../../../services/boardService"; +import { queryKeys } from "../../../services/queryKeys"; import { extractChecklist, toChecklistRequest } from "../../board/generation/checklistFromMarkdown"; import { useProjectContext } from "../../projects/useProjectContext"; @@ -29,6 +31,7 @@ type SaveReplyToBoardProps = { export function SaveReplyToBoard({ content }: SaveReplyToBoardProps) { const { selectedProjectId } = useProjectContext(); const toast = useToast(); + const queryClient = useQueryClient(); const [saving, setSaving] = useState(false); const [saved, setSaved] = useState(false); @@ -42,6 +45,12 @@ export function SaveReplyToBoard({ content }: SaveReplyToBoardProps) { setSaving(true); try { await boardService.addCard(selectedProjectId, toChecklistRequest(checklist)); + // The board this list just joined is very often already cached — this dock can be floating + // over it, and a visit within `staleTime` would otherwise serve the pre-card board. Marking + // it stale is what makes the open board (or the next visit) show the list the hire kept. + void queryClient.invalidateQueries({ + queryKey: queryKeys.board.byProject(selectedProjectId), + }); setSaved(true); toast.success("Kept on your board", { description: `"${checklist.title}" — ${checklist.items.length} things to tick off.`, diff --git a/src/features/buddy/hooks/useBuddyConversation.ts b/src/features/buddy/hooks/useBuddyConversation.ts index 8be71944..c0e5692c 100644 --- a/src/features/buddy/hooks/useBuddyConversation.ts +++ b/src/features/buddy/hooks/useBuddyConversation.ts @@ -1,4 +1,5 @@ import { useCallback, useEffect, useRef, useState } from "react"; +import { useQueryClient } from "@tanstack/react-query"; import { useDinoUnlocked, useSpaceOpensDino } from "../../easter-eggs/hooks/useDinoWaitingGame"; import { matchEggPhrase } from "../../easter-eggs/lib/eggPhrases"; import { playEggEffect } from "../../easter-eggs/eggEffectBus"; @@ -12,6 +13,15 @@ import { type BuddyOpeningAction, } from "../../../services/buddyService"; import { useAuth } from "../../../context/useAuth"; +import { queryKeys } from "../../../services/queryKeys"; +import { + BUDDY_ACTION_AMEND_CHECKLIST, + BUDDY_ACTION_CLAIM_GOAL, + BUDDY_ACTION_PLACE_CHECKLIST, + BUDDY_ACTION_PLACE_NOTE, + BUDDY_ACTION_REWORD_CHECKLIST, + BUDDY_ACTION_TICK_CHECKLIST, +} from "../types"; import type { ActionPatch, BuddyMessageView, ProposedAction } from "../types"; /** @@ -44,6 +54,29 @@ function isNotFound(e: unknown): boolean { return e instanceof Error && "status" in e && (e as { status: number }).status === 404; } +/** + * The hire-confirmed buddy actions that write to the board, by their wire names: the five + * `BuddyBoardWriteActions` handles (placing, amending, ticking and rewording a checklist, and + * the note), plus `claim_goal`, which pins the claimed task as the board's CURRENT_TASK card — + * its own outcome line says so ("It's on your board too"). Every other confirm changes something + * else — a task claim, an attestation request, a flag, a username — and needs no board sync. + */ +const BUDDY_BOARD_ACTIONS = new Set([ + BUDDY_ACTION_PLACE_CHECKLIST, + BUDDY_ACTION_AMEND_CHECKLIST, + BUDDY_ACTION_TICK_CHECKLIST, + BUDDY_ACTION_REWORD_CHECKLIST, + BUDDY_ACTION_PLACE_NOTE, + BUDDY_ACTION_CLAIM_GOAL, +]); + +/** + * The mid-answer tool that puts a card on the board. `place_card` is deliberately not a confirmed + * action — it applies the moment the mentor runs it — so a turn that ran it is the only signal + * the client ever gets that the board moved. Acted on by `sendMessage` when the turn ends. + */ +const BUDDY_BOARD_TOOLS = new Set(["place_card"]); + /** * Where "is the buddy in team mode" lives between reloads — scoped to the signed-in user, the * way the project selection is. A shared browser must not hand one manager's team conversation @@ -87,6 +120,7 @@ export function useBuddyConversation( const [messages, setMessages] = useState([]); const [isThinking, setIsThinking] = useState(false); const [isStreaming, setIsStreaming] = useState(false); + const queryClient = useQueryClient(); // The tool the buddy is running right now, if any -- drives "Checking your progress…" // in place of a generic spinner. Cleared as soon as the answer starts streaming. const [activeTool, setActiveTool] = useState(null); @@ -557,12 +591,29 @@ export function useBuddyConversation( // Once the hire says anything, the opener's one-click suggestion has served its purpose. setOpenerAction(null); + // Whether this turn ran a tool that puts a card on the board. A property rather than a + // `let`, so the reads below see the writes made in the stream callbacks — see `greet`. + const touched = { board: false }; + + /** + * Marks the board stale when this turn placed a card on it, and only then. + * + * `place_card` is not confirmed and reports nothing on the wire, so the `tool_use` event + * during the turn is the whole signal that the board moved. It is acted on once the turn + * ends, the failing paths included: the card lands as the tool runs, so a turn that wrote + * one and then broke still wrote it. + */ + const syncBoardIfTouched = () => { + if (touched.board) void queryClient.invalidateQueries({ queryKey: queryKeys.board.all() }); + }; + try { await streamMessage( text, { onToolUse: (name) => { setActiveTool(name); + if (BUDDY_BOARD_TOOLS.has(name)) touched.board = true; }, onToken: (token) => { @@ -655,15 +706,20 @@ export function useBuddyConversation( // Read at call time: a turn speaks to whichever conversation is current when it starts. teamProjectIdRef.current ?? undefined, ); + + // The turn is over — the first moment a card placed mid-answer is certainly on the board. + syncBoardIfTouched(); } catch (e) { console.error(e); setIsStreaming(false); setIsThinking(false); setActiveTool(null); failReply(assistantId); + // Same reason as above: a turn that placed a card and then broke still wrote it. + syncBoardIfTouched(); } }, - [failReply], + [failReply, queryClient], ); /** Patches one proposed action in place, keyed by its message and action id. */ @@ -752,6 +808,16 @@ export function useBuddyConversation( ok: result.ok, outcome: result.message, }); + + // A confirmed board write changed a surface that is not on screen here: the board + // itself. Marking the cached board stale is what lets an open board — the dock can be + // floating over it — or a visit within the cache window show the change, instead of the + // state it was read at. `board.all()` rather than the selected project's key, because + // the backend re-resolves the project server-side (the caller's single onboarding + // project) and the client never learns which board it wrote. + if (result.ok && "action" in action && BUDDY_BOARD_ACTIONS.has(action.action)) { + void queryClient.invalidateQueries({ queryKey: queryKeys.board.all() }); + } } catch (e) { console.error(e); // A settled proposal does NOT come back 404 — the backend answers 200 with ok: false @@ -775,7 +841,7 @@ export function useBuddyConversation( } })(); }, - [beginDecision, endDecision, patchAction], + [beginDecision, endDecision, patchAction, queryClient], ); /** diff --git a/src/features/buddy/types.ts b/src/features/buddy/types.ts index a5c932b7..1da87ef9 100644 --- a/src/features/buddy/types.ts +++ b/src/features/buddy/types.ts @@ -53,6 +53,23 @@ export const BUDDY_ACTION_TICK_CHECKLIST = "tick_checklist_items"; /** The backend's `reword_checklist_item` action: one line replaced, shown before and after. */ export const BUDDY_ACTION_REWORD_CHECKLIST = "reword_checklist_item"; +/** + * The backend's `place_note` action: an explanation the mentor offered to keep as a note. + * + * Named like the checklist actions above because the same reader needs it: the confirm path has + * to recognise which actions write to the board (see `BUDDY_BOARD_ACTIONS` in `useBuddyConversation`). + */ +export const BUDDY_ACTION_PLACE_NOTE = "place_note"; + +/** + * The backend's `claim_goal` action: the hire starting to work toward a task. + * + * Confirming this writes twice — the goal claim itself, and the CURRENT_TASK card pinned onto + * the board ("It's on your board too") — which is why the board-syncing set includes it despite + * its not being one of the board tools. + */ +export const BUDDY_ACTION_CLAIM_GOAL = "claim_goal"; + /** * An action proposed in hire mode: the buddy offers to do something *for this hire*, and the * confirm echoes the offer's own payload back verbatim. What gets written is what was shown on From 02bf8038219cafbd240e41545ec811a252e443b7 Mon Sep 17 00:00:00 2001 From: Daniil Perkin Date: Sun, 27 Sep 2026 22:19:32 +0200 Subject: [PATCH 2/4] test(board): pin board invalidation across buddy and save surfaces MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Covers the sync behaviour the fix introduces, and the boundaries it must not cross: - SaveReplyToBoard (new): the #233 regression itself — saving the checklist marks `board.byProject(selectedProjectId)` stale; a reply without a list renders nothing; a failed save toasts and leaves the cache alone. - SaveToBoard (new): the shared "keep this" button invalidates once the card is really on the board, still reports to `onSaved` for its callers, and does neither when the write failed. - SelectionActions (existing, one test added): the toolbar's save marks the board stale. - useBuddy (existing, new `board synchronisation` describe): a confirmed board action marks the board stale, `claim_goal` included; a failed confirm and a non-board action (request_attestation) do not; a turn that ran `place_card` marks it, and a turn that ran other tools does not. Two harness notes for whoever extends these next: the seeded board query is observer-less (pure `setQueryData`), so it must not get `gcTime: 0` — it would be garbage-collected before the invalidation can be observed and `isInvalidated` would read as `undefined`. And the save-button test clears storage via an optional call (`window.localStorage?.clear()`) because jsdom ships with or without a Storage backing depending on the Node version in use. Refs #233 --- .../unit/features/board/SaveToBoard.test.tsx | 104 +++++++++++ .../features/board/SelectionActions.test.tsx | 35 +++- .../features/buddy/SaveReplyToBoard.test.tsx | 94 ++++++++++ tests/unit/features/buddy/useBuddy.test.tsx | 176 ++++++++++++++++++ 4 files changed, 406 insertions(+), 3 deletions(-) create mode 100644 tests/unit/features/board/SaveToBoard.test.tsx create mode 100644 tests/unit/features/buddy/SaveReplyToBoard.test.tsx diff --git a/tests/unit/features/board/SaveToBoard.test.tsx b/tests/unit/features/board/SaveToBoard.test.tsx new file mode 100644 index 00000000..39fc1665 --- /dev/null +++ b/tests/unit/features/board/SaveToBoard.test.tsx @@ -0,0 +1,104 @@ +import { render, screen, waitFor } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { BookmarkPlus } from "lucide-react"; +import type { ReactElement } from "react"; +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { SaveToBoard } from "../../../../src/features/board/save/SaveToBoard"; +import { boardService } from "../../../../src/services/boardService"; +import { queryKeys } from "../../../../src/services/queryKeys"; +import type { AuthoredCardRequest } from "../../../../src/features/board/types"; + +let selectedProjectId = "p1"; +vi.mock("../../../../src/features/projects/useProjectContext", () => ({ + useProjectContext: () => ({ selectedProjectId }), +})); + +const toast = { success: vi.fn(), error: vi.fn() }; +vi.mock("../../../../src/context/useToast", () => ({ useToast: () => toast })); + +const request = (): AuthoredCardRequest => ({ kind: "NOTE", text: "A reply, frozen." }); + +/** Renders under a client whose cache already holds this project's board, read a moment ago. */ +function renderSave(ui: ReactElement) { + const client = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + client.setQueryData(queryKeys.board.byProject("p1"), { + boardId: "b1", + projectId: "p1", + cards: [], + }); + render({ui}); + return { client }; +} + +const boardIsStale = (client: QueryClient) => + client.getQueryState(queryKeys.board.byProject("p1"))?.isInvalidated ?? false; + +function props(overrides: Partial[0]> = {}) { + return { + request, + label: "Keep on my board", + icon: , + ...overrides, + }; +} + +describe("SaveToBoard", () => { + beforeEach(() => { + vi.clearAllMocks(); + selectedProjectId = "p1"; + // jsdom on this Node has no Storage backing (CI's Node does); clearing is hygiene when present. + window.localStorage?.clear(); + }); + + /** The one button every "keep this" offer is built on — chat, buddy replies, task cards. */ + it("puts the built card on the selected project's board", async () => { + const addCard = vi.spyOn(boardService, "addCard").mockResolvedValue({ id: "c1" } as never); + renderSave(); + + await userEvent.click(screen.getByRole("button", { name: "Keep on my board" })); + + await waitFor(() => expect(addCard).toHaveBeenCalledOnce()); + expect(addCard.mock.calls[0][0]).toBe("p1"); + expect(addCard.mock.calls[0][1]).toEqual({ kind: "NOTE", text: "A reply, frozen." }); + }); + + it("marks the board stale once the card is really on it", async () => { + vi.spyOn(boardService, "addCard").mockResolvedValue({ id: "c1" } as never); + const { client } = renderSave(); + + await userEvent.click(screen.getByRole("button", { name: "Keep on my board" })); + + await waitFor(() => expect(boardIsStale(client)).toBe(true)); + }); + + /** The board-surface callers still get their callback — for the affordances that are theirs. */ + it("tells the caller the card landed", async () => { + vi.spyOn(boardService, "addCard").mockResolvedValue({ id: "c1" } as never); + const onSaved = vi.fn(); + renderSave(); + + await userEvent.click(screen.getByRole("button", { name: "Break this into a checklist" })); + + await waitFor(() => expect(onSaved).toHaveBeenCalledOnce()); + }); + + it("leaves the board and the caller alone when the write failed", async () => { + vi.spyOn(boardService, "addCard").mockRejectedValue(new Error("nope")); + const onSaved = vi.fn(); + const { client } = renderSave(); + + await userEvent.click(screen.getByRole("button", { name: "Keep on my board" })); + + await waitFor(() => expect(toast.error).toHaveBeenCalled()); + expect(onSaved).not.toHaveBeenCalled(); + expect(boardIsStale(client)).toBe(false); + }); + + it("offers nothing when no project is selected", () => { + selectedProjectId = ""; + renderSave(); + + expect(screen.queryByRole("button")).not.toBeInTheDocument(); + }); +}); diff --git a/tests/unit/features/board/SelectionActions.test.tsx b/tests/unit/features/board/SelectionActions.test.tsx index 3a29adad..dae5ec39 100644 --- a/tests/unit/features/board/SelectionActions.test.tsx +++ b/tests/unit/features/board/SelectionActions.test.tsx @@ -1,9 +1,11 @@ import { render, screen, waitFor } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import { MemoryRouter } from "react-router-dom"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import { SelectionActions } from "../../../../src/features/board/selection/SelectionActions"; import { boardService } from "../../../../src/services/boardService"; +import { queryKeys } from "../../../../src/services/queryKeys"; import { ChatContext, type ChatContextValue } from "../../../../src/context/ChatContext"; import { openAiBuddy } from "../../../../src/features/buddy/aiBuddyBus"; import { FocusModeContext } from "../../../../src/context/FocusModeContext"; @@ -58,8 +60,11 @@ describe("SelectionActions", () => { document.dispatchEvent(new Event("selectionchange")); } - function renderToolbar({ focused = false }: { focused?: boolean } = {}) { - return render( + function renderToolbar({ + focused = false, + client, + }: { focused?: boolean; client?: QueryClient } = {}) { + const tree = ( {}, toggleFocused: () => {} }} @@ -68,8 +73,14 @@ describe("SelectionActions", () => { - , + ); + + // A client of its own only where a test reads the cache back; every other render keeps the + // one the harness hands it. + return client + ? render({tree}) + : render(tree); } it("offers nothing until something is selected", () => { @@ -97,6 +108,24 @@ describe("SelectionActions", () => { expect(addCard.mock.calls[0][1]).toMatchObject({ kind: "NOTE" }); }); + it("marks the board stale once the selection is really on it", async () => { + vi.spyOn(boardService, "addCard").mockResolvedValue({ id: "c1" } as never); + const client = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + client.setQueryData(queryKeys.board.byProject("p1"), { + boardId: "b1", + projectId: "p1", + cards: [], + }); + renderToolbar({ client }); + highlight("Run the migration first."); + + await userEvent.click(await screen.findByRole("button", { name: /add to board/i })); + + await waitFor(() => + expect(client.getQueryState(queryKeys.board.byProject("p1"))?.isInvalidated).toBe(true), + ); + }); + /** * Being pulled to the board to confirm something landed is the interruption this feature exists * to avoid. The toast carries the way there for whoever wants it. diff --git a/tests/unit/features/buddy/SaveReplyToBoard.test.tsx b/tests/unit/features/buddy/SaveReplyToBoard.test.tsx new file mode 100644 index 00000000..db0cb621 --- /dev/null +++ b/tests/unit/features/buddy/SaveReplyToBoard.test.tsx @@ -0,0 +1,94 @@ +import { render, screen, waitFor } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import type { ReactElement } from "react"; +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { SaveReplyToBoard } from "../../../../src/features/buddy/components/SaveReplyToBoard"; +import { boardService } from "../../../../src/services/boardService"; +import { queryKeys } from "../../../../src/services/queryKeys"; + +let selectedProjectId = "p1"; +vi.mock("../../../../src/features/projects/useProjectContext", () => ({ + useProjectContext: () => ({ selectedProjectId }), +})); + +const toast = { success: vi.fn(), error: vi.fn() }; +vi.mock("../../../../src/context/useToast", () => ({ useToast: () => toast })); + +/** A reply holding a list — the shape that makes the offer appear at all. */ +const REPLY = "Here is how to start:\n\n## Getting started\n\n- Run it locally\n- Open a PR\n"; + +/** + * Renders under a client whose cache already holds this project's board, read a moment ago — the + * exact state that used to keep serving a board without the card that was just kept. + */ +function renderReply(ui: ReactElement) { + const client = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + client.setQueryData(queryKeys.board.byProject("p1"), { + boardId: "b1", + projectId: "p1", + cards: [], + }); + render({ui}); + return { client }; +} + +const boardIsStale = (client: QueryClient) => + client.getQueryState(queryKeys.board.byProject("p1"))?.isInvalidated ?? false; + +describe("SaveReplyToBoard", () => { + beforeEach(() => { + vi.clearAllMocks(); + selectedProjectId = "p1"; + }); + + it("offers nothing when the reply holds no list", () => { + renderReply(); + + expect(screen.queryByRole("button")).not.toBeInTheDocument(); + }); + + it("keeps the reply's list as a checklist card on the selected project", async () => { + const addCard = vi.spyOn(boardService, "addCard").mockResolvedValue({ id: "c1" } as never); + renderReply(); + + await userEvent.click(screen.getByRole("button", { name: /keep as checklist/i })); + + await waitFor(() => expect(addCard).toHaveBeenCalledOnce()); + expect(addCard.mock.calls[0][0]).toBe("p1"); + expect(addCard.mock.calls[0][1]).toMatchObject({ + kind: "CHECKLIST", + title: "Getting started", + items: [ + { text: "Run it locally", done: false }, + { text: "Open a PR", done: false }, + ], + }); + }); + + /** + * Issue #233: the write landed, but the board this dock can be floating over kept serving its + * cached copy for up to `staleTime`. The card existing is what the button says; the cache is + * what the hire actually sees. + */ + it("marks the board stale once the list is really on it", async () => { + vi.spyOn(boardService, "addCard").mockResolvedValue({ id: "c1" } as never); + const { client } = renderReply(); + + await userEvent.click(screen.getByRole("button", { name: /keep as checklist/i })); + + await waitFor(() => expect(boardIsStale(client)).toBe(true)); + // And the button acknowledges, as it always has. + expect(screen.getByRole("button", { name: /checklist on your board/i })).toBeInTheDocument(); + }); + + it("leaves the board alone when the save failed", async () => { + vi.spyOn(boardService, "addCard").mockRejectedValue(new Error("nope")); + const { client } = renderReply(); + + await userEvent.click(screen.getByRole("button", { name: /keep as checklist/i })); + + await waitFor(() => expect(toast.error).toHaveBeenCalled()); + expect(boardIsStale(client)).toBe(false); + }); +}); diff --git a/tests/unit/features/buddy/useBuddy.test.tsx b/tests/unit/features/buddy/useBuddy.test.tsx index e3664c65..29779768 100644 --- a/tests/unit/features/buddy/useBuddy.test.tsx +++ b/tests/unit/features/buddy/useBuddy.test.tsx @@ -1,10 +1,13 @@ import { renderHook, act, waitFor } from "@testing-library/react"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import type { ReactNode } from "react"; import { describe, it, expect, vi, beforeEach } from "vitest"; import { useBuddy } from "../../../../src/features/buddy/hooks/useBuddy"; import { BuddyProviderWithStubs } from "./buddyTestHarness"; import { openAiBuddy } from "../../../../src/features/buddy/aiBuddyBus"; import { http, HttpResponse } from "msw"; import { server } from "../../setup/vitest.setup"; +import { queryKeys } from "../../../../src/services/queryKeys"; /** * A greeting that opens the visit and writes nothing. @@ -455,4 +458,177 @@ describe("useBuddy", () => { expect(result.current.messages[1].actions?.[0].status).toBe("resolved"); }); }); + + /** + * The board a confirmed action writes to is a cache entry nobody here is looking at: the dock + * can float over the board page, and a visit within `staleTime` serves the board as it was read. + * These pin which confirms mark it stale — and which correctly do not. + */ + describe("board synchronisation", () => { + /** A client already holding this hire's board, and a wrapper that puts it under the session. */ + function boardContext() { + const client = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + client.setQueryData(queryKeys.board.byProject("p1"), { + boardId: "b1", + projectId: "p1", + cards: [], + }); + + function Wrapper({ children }: { children: ReactNode }) { + return ( + + {children} + + ); + } + + return { client, Wrapper }; + } + + const boardIsStale = (client: QueryClient) => + client.getQueryState(queryKeys.board.byProject("p1"))?.isInvalidated ?? false; + + function stream(events: string[]) { + const encoder = new TextEncoder(); + return new HttpResponse( + new ReadableStream({ + start(controller) { + for (const event of events) controller.enqueue(encoder.encode(`data: ${event}\n\n`)); + controller.close(); + }, + }), + { headers: { "Content-Type": "text/event-stream" } }, + ); + } + + /** Opens the session, sends a question whose reply proposes `proposalEvent`, and confirms it. */ + async function confirmProposal( + proposalEvent: string, + outcome: { ok: boolean; message: string }, + ) { + const { client, Wrapper } = boardContext(); + server.use( + http.get("/api/v1/onboarding/me/buddy/messages", () => HttpResponse.json([])), + http.post("/api/v1/onboarding/me/buddy/open/stream", () => silentGreeting()), + http.post("/api/v1/onboarding/me/buddy/messages", () => stream([proposalEvent])), + http.post("/api/v1/onboarding/me/buddy/actions", () => HttpResponse.json(outcome)), + ); + + const { result } = renderHook(() => useBuddy(), { wrapper: Wrapper }); + act(() => { + result.current.toggleOpen(); + }); + await waitFor(() => expect(result.current.messages).toHaveLength(0)); + act(() => { + result.current.setDraft("how do I start?"); + }); + act(() => { + result.current.handleSubmit({ preventDefault: vi.fn() } as unknown as React.FormEvent); + }); + await waitFor(() => expect(result.current.messages[1]?.actions?.[0]).toBeDefined()); + + act(() => { + result.current.confirmAction( + result.current.messages[1].id, + result.current.messages[1].actions![0], + ); + }); + await waitFor(() => expect(result.current.messages[1].actions?.[0].status).toBe("resolved")); + + return { client }; + } + + const PLACE_CHECKLIST = + '{"type":"action_proposal","action":"place_checklist","label":"Keep this as a checklist",' + + '"checklist_title":"Getting started","checklist_items":["Run it locally","Open a PR"]}'; + + it("marks the board stale when a confirmed action wrote a card", async () => { + const { client } = await confirmProposal(PLACE_CHECKLIST, { ok: true, message: "Kept." }); + + expect(boardIsStale(client)).toBe(true); + }); + + /** `claim_goal` writes twice: the claim, and the CURRENT_TASK card it pins on the board. */ + it("marks the board stale when claim_goal pins a task card", async () => { + const { client } = await confirmProposal( + '{"type":"action_proposal","action":"claim_goal","label":"Work toward this task","task_id":"t-1"}', + { ok: true, message: "You're now working toward it." }, + ); + + expect(boardIsStale(client)).toBe(true); + }); + + it("leaves the board alone when the action changed nothing", async () => { + const { client } = await confirmProposal(PLACE_CHECKLIST, { + ok: false, + message: "I couldn't keep that just now.", + }); + + expect(boardIsStale(client)).toBe(false); + }); + + it("leaves the board alone for an action that never touches it", async () => { + const { client } = await confirmProposal( + '{"type":"action_proposal","action":"request_attestation","label":"Ask them to confirm this","title":"the auth fix","attester_id":"u-9"}', + { ok: true, message: "Asked them to confirm it." }, + ); + + expect(boardIsStale(client)).toBe(false); + }); + + /** Runs a full turn — open, ask, answer — with the given stream events. */ + async function completeTurn(events: string[]) { + const { client, Wrapper } = boardContext(); + server.use( + http.get("/api/v1/onboarding/me/buddy/messages", () => HttpResponse.json([])), + http.post("/api/v1/onboarding/me/buddy/open/stream", () => silentGreeting()), + http.post("/api/v1/onboarding/me/buddy/messages", () => stream(events)), + ); + + const { result } = renderHook(() => useBuddy(), { wrapper: Wrapper }); + act(() => { + result.current.toggleOpen(); + }); + await waitFor(() => expect(result.current.messages).toHaveLength(0)); + act(() => { + result.current.setDraft("put the PR review task on my board"); + }); + act(() => { + result.current.handleSubmit({ preventDefault: vi.fn() } as unknown as React.FormEvent); + }); + await waitFor(() => expect(result.current.isThinking).toBe(false)); + // The board check runs when the stream call resolves, a beat after the last event — settle + // before asserting an absence. + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 50)); + }); + + return { client }; + } + + /** + * `place_card` is the one board write that is not confirmed — it applies the moment the + * mentor runs it — so the tool_use event during the turn is the whole signal the client gets + * that the board moved. + */ + it("marks the board stale when the turn placed a card on it", async () => { + const { client } = await completeTurn([ + '{"type":"tool_use","name":"place_card"}', + '{"type":"token","content":"It is on your board."}', + '{"type":"done"}', + ]); + + expect(boardIsStale(client)).toBe(true); + }); + + it("leaves the board alone when the turn ran other tools", async () => { + const { client } = await completeTurn([ + '{"type":"tool_use","name":"get_my_metrics"}', + '{"type":"token","content":"You are on track."}', + '{"type":"done"}', + ]); + + expect(boardIsStale(client)).toBe(false); + }); + }); }); From 584486a9fe9410bd668197db9be3665b3fdc31af Mon Sep 17 00:00:00 2001 From: Daniil Perkin Date: Mon, 28 Sep 2026 10:38:04 +0200 Subject: [PATCH 3/4] =?UTF-8?q?upgrade(board):=20address=20the=20review=20?= =?UTF-8?q?=E2=80=94=20precise=20claims,=20quiet=20MSW,=20deterministic=20?= =?UTF-8?q?and=20wider=20board-sync=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A deep review of #271 found no functional defect in the invalidation itself, but one inaccurate claim, one test-harness leak, one timing-fragile pattern and real coverage gaps. Each finding was fact-checked against the code before acting; two further findings were checked and answered rather than changed (see below). Fact-check results and what changed: 1. "Writes that fail invalidate nothing" is false for `place_card` (review §2.2). Confirmed. The wire carries only `tool_use` — buddyService's parser has no tool-result event — so the client can never know whether the tool wrote a card or was refused. The code is right to invalidate on invocation: an unnecessary refetch of an unchanged board is cheaper than a missing card. But the comment and the PR text overstated the guarantee; both now say what actually happens — the mark is applied when the turn ends, the failing paths included, because a turn that wrote a card and then broke still wrote it. 2. Unhandled `GET /buddy/suggestions` on every dock open (review §3.1). Confirmed empirically: a probe on MSW's `request:unhandled` event recorded exactly this request when the dock opens — and it fires for the pre-existing dock tests in the same file too, not only the new ones. Fixed at the source of truth for default handlers: `tests/unit/setup/msw-handlers.ts` now answers with an empty chip row, so every dock-opening test stays on the "no unhandled requests" side of the setup. Re-ran the probe afterwards: the unhandled log is empty. `useBuddySuggestions`' own suite mocks the service directly and is unaffected. 3. Flaky `setTimeout(…, 50)` settle in the turn tests (review §3.2). Confirmed — that was a fixed sleep standing in for "the sync has run by now". Replaced with deterministic waits: the positive case waits for the invalidation itself (`waitFor(stale)`), and the negative case no longer asserts an absence at all — it runs a board-silent turn followed by a card-placing turn, waits for the second sync, and asserts the invalidation spy fired exactly once. Whichever way the first turn's timing falls, only the second can have produced that single call. 4. Coverage gaps (review §4). Added: the four missing confirmed actions (`amend_checklist`, `tick_checklist_items`, `reword_checklist_item`, `place_note`) as a table over all six board-writing actions; a "turn broke after placing a card" test (the stream delivers `tool_use` and then drops — the sync must not wait for a clean finish); a `SelectionActions` failure test (a rejected write leaves the board cache fresh); and the one the review called out as absent entirely — an active-observer test: `SaveReplyToBoard.test.tsx` now mounts a real `useQuery` on the same board key and proves the keep triggers an actual refetch, not just a stale flag on an unobserved cache entry. Deliberately not changed (checked, then answered rather than edited): - §3.3 redundant refetch: real, and harmless — when `SaveToBoard` runs inside `AddTaskToBoard` on the board page, the invalidation and the caller's `refresh()` both converge on the same query, and React Query deduplicates concurrent refetches of one query into a single request. Restructuring for it would couple the save button to which page it is rendered on. - §3.4 project-context divergence: by design. Buddy actions resolve the project server-side (the backend picks the hire's onboarding project), which is why the hook invalidates the whole `board` prefix while the component paths invalidate exactly the project they wrote to. A hire normally has one onboarding project; when the header selection matches it, both paths converge on the same key anyway. - §5 dead code: none — and the nested test `QueryClientProvider` it flags is intentional: those tests need their own client to read the stale flag back. Gates (Node 22.21.0, matching CI): format:check, lint, build green; unit 330 files / 3218 tests; a11y 55 files / 69 tests. --- .../buddy/hooks/useBuddyConversation.ts | 12 +- .../features/board/SelectionActions.test.tsx | 18 +++ .../features/buddy/SaveReplyToBoard.test.tsx | 35 +++- tests/unit/features/buddy/useBuddy.test.tsx | 151 +++++++++++++----- tests/unit/setup/msw-handlers.ts | 4 + 5 files changed, 175 insertions(+), 45 deletions(-) diff --git a/src/features/buddy/hooks/useBuddyConversation.ts b/src/features/buddy/hooks/useBuddyConversation.ts index c0e5692c..e9534587 100644 --- a/src/features/buddy/hooks/useBuddyConversation.ts +++ b/src/features/buddy/hooks/useBuddyConversation.ts @@ -596,12 +596,14 @@ export function useBuddyConversation( const touched = { board: false }; /** - * Marks the board stale when this turn placed a card on it, and only then. + * Marks the board stale when this turn ran the card-placing tool, and only then. * - * `place_card` is not confirmed and reports nothing on the wire, so the `tool_use` event - * during the turn is the whole signal that the board moved. It is acted on once the turn - * ends, the failing paths included: the card lands as the tool runs, so a turn that wrote - * one and then broke still wrote it. + * `place_card` is not confirmed and the wire reports nothing about its outcome — the + * `tool_use` event is the whole signal, and it cannot say whether the tool wrote a card or + * was refused. So the mark is applied once the turn ends, the failing paths included: a + * turn that wrote a card and then broke still wrote it, and a turn whose tool refused + * costs one refetch of an unchanged board. Refreshing a board that did not change beats + * missing one that did. */ const syncBoardIfTouched = () => { if (touched.board) void queryClient.invalidateQueries({ queryKey: queryKeys.board.all() }); diff --git a/tests/unit/features/board/SelectionActions.test.tsx b/tests/unit/features/board/SelectionActions.test.tsx index dae5ec39..6b103d2a 100644 --- a/tests/unit/features/board/SelectionActions.test.tsx +++ b/tests/unit/features/board/SelectionActions.test.tsx @@ -126,6 +126,24 @@ describe("SelectionActions", () => { ); }); + /** The other half of the stale-marking rule: a write that never landed must not cost the board. */ + it("leaves the board alone when the write failed", async () => { + vi.spyOn(boardService, "addCard").mockRejectedValue(new Error("nope")); + const client = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + client.setQueryData(queryKeys.board.byProject("p1"), { + boardId: "b1", + projectId: "p1", + cards: [], + }); + renderToolbar({ client }); + highlight("Run the migration first."); + + await userEvent.click(await screen.findByRole("button", { name: /add to board/i })); + + await waitFor(() => expect(toast.error).toHaveBeenCalled()); + expect(client.getQueryState(queryKeys.board.byProject("p1"))?.isInvalidated).toBe(false); + }); + /** * Being pulled to the board to confirm something landed is the interruption this feature exists * to avoid. The toast carries the way there for whoever wants it. diff --git a/tests/unit/features/buddy/SaveReplyToBoard.test.tsx b/tests/unit/features/buddy/SaveReplyToBoard.test.tsx index db0cb621..97eaa38d 100644 --- a/tests/unit/features/buddy/SaveReplyToBoard.test.tsx +++ b/tests/unit/features/buddy/SaveReplyToBoard.test.tsx @@ -1,6 +1,6 @@ import { render, screen, waitFor } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; -import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { QueryClient, QueryClientProvider, useQuery } from "@tanstack/react-query"; import type { ReactElement } from "react"; import { describe, it, expect, vi, beforeEach } from "vitest"; import { SaveReplyToBoard } from "../../../../src/features/buddy/components/SaveReplyToBoard"; @@ -82,6 +82,39 @@ describe("SaveReplyToBoard", () => { expect(screen.getByRole("button", { name: /checklist on your board/i })).toBeInTheDocument(); }); + /** + * The mechanical promise of #233 in one test: a board mounted behind the dock is an *active + * reader* of the key the keep invalidates, and invalidation alone would be a comment if it + * never reached one. The flag assertions above are about the cache; this one is about the + * refetch the mounted board actually gets. + */ + it("refetches a mounted board after the keep", async () => { + vi.spyOn(boardService, "addCard").mockResolvedValue({ id: "c1" } as never); + const fetchBoard = vi + .fn<() => Promise>() + .mockResolvedValue({ boardId: "b1", projectId: "p1", cards: [] }); + + function MountedBoard() { + useQuery({ queryKey: queryKeys.board.byProject("p1"), queryFn: () => fetchBoard() }); + return null; + } + + render( + + + + , + ); + await waitFor(() => expect(fetchBoard).toHaveBeenCalledOnce()); + + await userEvent.click(screen.getByRole("button", { name: /keep as checklist/i })); + + // The initial read plus the one the invalidation triggered. + await waitFor(() => expect(fetchBoard.mock.calls.length).toBeGreaterThanOrEqual(2)); + }); + it("leaves the board alone when the save failed", async () => { vi.spyOn(boardService, "addCard").mockRejectedValue(new Error("nope")); const { client } = renderReply(); diff --git a/tests/unit/features/buddy/useBuddy.test.tsx b/tests/unit/features/buddy/useBuddy.test.tsx index 29779768..55c482ab 100644 --- a/tests/unit/features/buddy/useBuddy.test.tsx +++ b/tests/unit/features/buddy/useBuddy.test.tsx @@ -5,7 +5,7 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; import { useBuddy } from "../../../../src/features/buddy/hooks/useBuddy"; import { BuddyProviderWithStubs } from "./buddyTestHarness"; import { openAiBuddy } from "../../../../src/features/buddy/aiBuddyBus"; -import { http, HttpResponse } from "msw"; +import { delay, http, HttpResponse } from "msw"; import { server } from "../../setup/vitest.setup"; import { queryKeys } from "../../../../src/services/queryKeys"; @@ -542,21 +542,49 @@ describe("useBuddy", () => { '{"type":"action_proposal","action":"place_checklist","label":"Keep this as a checklist",' + '"checklist_title":"Getting started","checklist_items":["Run it locally","Open a PR"]}'; - it("marks the board stale when a confirmed action wrote a card", async () => { - const { client } = await confirmProposal(PLACE_CHECKLIST, { ok: true, message: "Kept." }); - - expect(boardIsStale(client)).toBe(true); - }); - - /** `claim_goal` writes twice: the claim, and the CURRENT_TASK card it pins on the board. */ - it("marks the board stale when claim_goal pins a task card", async () => { - const { client } = await confirmProposal( + /** + * Every confirmed action that writes to the board, by its wire name: the five board-write + * handles plus `claim_goal`, which writes twice — the claim, and the CURRENT_TASK card it + * pins ("it's on your board too"). A table beats six near-identical tests drifting apart one + * rename at a time. + */ + const BOARD_WRITING_PROPOSALS: [string, string][] = [ + ["place_checklist", PLACE_CHECKLIST], + [ + "amend_checklist", + '{"type":"action_proposal","action":"amend_checklist","label":"Add that step",' + + '"card_id":"c-1","checklist_title":"Getting started",' + + '"checklist_items":["Run it locally","Open a PR","Ping the PM"]}', + ], + [ + "tick_checklist_items", + '{"type":"action_proposal","action":"tick_checklist_items","label":"Tick those off",' + + '"card_id":"c-1","checklist_items":["Run it locally"]}', + ], + [ + "reword_checklist_item", + '{"type":"action_proposal","action":"reword_checklist_item","label":"Reword that step",' + + '"card_id":"c-1","line_before":"Run it locally","line_after":"Run the app locally"}', + ], + [ + "place_note", + '{"type":"action_proposal","action":"place_note","label":"Keep this as a note",' + + '"note_text":"Deploys need the VPN."}', + ], + [ + "claim_goal", '{"type":"action_proposal","action":"claim_goal","label":"Work toward this task","task_id":"t-1"}', - { ok: true, message: "You're now working toward it." }, - ); + ], + ]; - expect(boardIsStale(client)).toBe(true); - }); + it.each(BOARD_WRITING_PROPOSALS)( + "marks the board stale when a confirmed %s wrote a card", + async (_action, proposal) => { + const { client } = await confirmProposal(proposal, { ok: true, message: "Done." }); + + expect(boardIsStale(client)).toBe(true); + }, + ); it("leaves the board alone when the action changed nothing", async () => { const { client } = await confirmProposal(PLACE_CHECKLIST, { @@ -576,13 +604,14 @@ describe("useBuddy", () => { expect(boardIsStale(client)).toBe(false); }); - /** Runs a full turn — open, ask, answer — with the given stream events. */ - async function completeTurn(events: string[]) { + /** Opens the session; each question asked answers with the next entry of `answers`. */ + async function openSession(answers: string[][]) { const { client, Wrapper } = boardContext(); + let answered = 0; server.use( http.get("/api/v1/onboarding/me/buddy/messages", () => HttpResponse.json([])), http.post("/api/v1/onboarding/me/buddy/open/stream", () => silentGreeting()), - http.post("/api/v1/onboarding/me/buddy/messages", () => stream(events)), + http.post("/api/v1/onboarding/me/buddy/messages", () => stream(answers[answered++])), ); const { result } = renderHook(() => useBuddy(), { wrapper: Wrapper }); @@ -590,45 +619,89 @@ describe("useBuddy", () => { result.current.toggleOpen(); }); await waitFor(() => expect(result.current.messages).toHaveLength(0)); + + return { client, result }; + } + + /** Sends one question and waits for its turn to end. */ + async function ask(result: { current: ReturnType }, text: string) { act(() => { - result.current.setDraft("put the PR review task on my board"); + result.current.setDraft(text); }); act(() => { result.current.handleSubmit({ preventDefault: vi.fn() } as unknown as React.FormEvent); }); await waitFor(() => expect(result.current.isThinking).toBe(false)); - // The board check runs when the stream call resolves, a beat after the last event — settle - // before asserting an absence. - await act(async () => { - await new Promise((resolve) => setTimeout(resolve, 50)); - }); - - return { client }; } + const PLACE_CARD_TURN = [ + '{"type":"tool_use","name":"place_card"}', + '{"type":"token","content":"It is on your board."}', + '{"type":"done"}', + ]; + const METRICS_TURN = [ + '{"type":"tool_use","name":"get_my_metrics"}', + '{"type":"token","content":"You are on track."}', + '{"type":"done"}', + ]; + /** * `place_card` is the one board write that is not confirmed — it applies the moment the - * mentor runs it — so the tool_use event during the turn is the whole signal the client gets - * that the board moved. + * mentor runs it — so its `tool_use` event during the turn is the whole signal the client + * gets that the board may have moved. */ it("marks the board stale when the turn placed a card on it", async () => { - const { client } = await completeTurn([ - '{"type":"tool_use","name":"place_card"}', - '{"type":"token","content":"It is on your board."}', - '{"type":"done"}', - ]); + const { client, result } = await openSession([PLACE_CARD_TURN]); + + await ask(result, "put the PR review task on my board"); - expect(boardIsStale(client)).toBe(true); + await waitFor(() => expect(boardIsStale(client)).toBe(true)); }); it("leaves the board alone when the turn ran other tools", async () => { - const { client } = await completeTurn([ - '{"type":"tool_use","name":"get_my_metrics"}', - '{"type":"token","content":"You are on track."}', - '{"type":"done"}', - ]); + const { client, result } = await openSession([METRICS_TURN, PLACE_CARD_TURN]); + const invalidations = vi.spyOn(client, "invalidateQueries"); - expect(boardIsStale(client)).toBe(false); + await ask(result, "how am I doing?"); + await ask(result, "put the PR review task on my board"); + + // Waiting for the *second* turn's sync closes the window: whatever the board-silent first + // turn was going to do has happened by now, so a single invalidation proves the metrics + // read marked nothing on its own. + await waitFor(() => expect(boardIsStale(client)).toBe(true)); + expect(invalidations).toHaveBeenCalledTimes(1); + }); + + /** A reply that delivers its events and then drops — the failure lands after `place_card` ran. */ + function streamThenBreak(events: string[]) { + const encoder = new TextEncoder(); + return new HttpResponse( + new ReadableStream({ + async start(controller) { + for (const event of events) controller.enqueue(encoder.encode(`data: ${event}\n\n`)); + await delay(20); + controller.error(new Error("connection lost")); + }, + }), + { headers: { "Content-Type": "text/event-stream" } }, + ); + } + + /** + * The turn can fail after the tool already ran — the card is on the board either way, so the + * sync must not wait for a clean finish. This is the "failing paths included" claim, pinned. + */ + it("still marks the board stale when the turn broke after placing a card", async () => { + const { client, result } = await openSession([]); + server.use( + http.post("/api/v1/onboarding/me/buddy/messages", () => + streamThenBreak(['{"type":"tool_use","name":"place_card"}']), + ), + ); + + await ask(result, "put the PR review task on my board"); + + await waitFor(() => expect(boardIsStale(client)).toBe(true)); }); }); }); diff --git a/tests/unit/setup/msw-handlers.ts b/tests/unit/setup/msw-handlers.ts index ff95a7f4..34ec5d8e 100644 --- a/tests/unit/setup/msw-handlers.ts +++ b/tests/unit/setup/msw-handlers.ts @@ -656,4 +656,8 @@ export const handlers = [ http.get("/api/v1/connectors/confluence/sources", () => HttpResponse.json({ connectorId: "confluence", sources: [] }), ), + // Every surface that opens the buddy dock asks for its suggestion chips. A default empty list + // keeps that request handled for the many tests that open the dock without being about the + // chips; `useBuddySuggestions`' own suite mocks the service directly and never sees this. + http.get("/api/v1/onboarding/me/buddy/suggestions", () => HttpResponse.json([])), ]; From 0d5323cdbbd7c65f5200bdb98418ca9ffa488154 Mon Sep 17 00:00:00 2001 From: Daniil Perkin Date: Mon, 28 Sep 2026 12:54:16 +0200 Subject: [PATCH 4/4] upgrade(board): funnel every exterior board write through one invalidation helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 2 on #271 asked for the four copy-pasted invalidation blocks to be shared rather than repeated, with the near-duplicate rationale comments collapsed to one place. The blocks were not identical — three knew the project they wrote to and marked `board.byProject(id)`, while the two in the buddy hook did not and marked `board.all()` — so the helper keeps both scopes as one contract with the "why" written once. What changed: - New `useInvalidateBoard(projectId?)` in `features/board/hooks/`. With a projectId it marks exactly that project's board; omitted it marks every cached board, for writes whose project the backend resolved server-side (buddy actions) and never told the client. The doc comment carries the full rationale, so every call site shrinks to one line pointing at it. - `SaveReplyToBoard`, `SaveToBoard` and `SelectionActions` now call `invalidateBoard()` and no longer import `useQueryClient` or `queryKeys` — the board's cache keys stop being knowledge those components carry. Buddy → board is a new feature-to-feature import edge, but not a new kind of edge (settings already imports from profile); the alternative home, src/hooks/, is for generic utilities that know nothing about the board. - `useBuddyConversation` drops its own `useQueryClient`; both of its sites — `confirmAction`'s confirmed board actions and the turn-end `place_card` sync — call the same helper with no project, and both `useCallback` dep arrays now carry `invalidateBoard` instead. - Caller-specific notes survive the trim: the confirm site keeps the "backend re-resolves the project server-side" note (it is why the scope is `all`), and the `place_card` site keeps the "tool_use is the whole signal" note (it is why the sync runs at turn end, failing paths included). - New `tests/unit/features/board/useInvalidateBoard.test.tsx` pins both scopes: a write with a known project marks only that board, a write without one marks every cached board. Also noted (review nit): the `buddy/suggestions` MSW default handler in `tests/unit/setup/msw-handlers.ts` is indeed unrelated to board sync — it is there because every dock-opening test fires that request; the PR description now spells this out so it does not read as a stray change. Gates (Node 22.21.0, matching CI): format:check, lint, build green; unit 331 files / 3220 tests; a11y 55 files / 69 tests. --- .../board/hooks/useInvalidateBoard.ts | 29 ++++++++++ src/features/board/save/SaveToBoard.tsx | 14 ++--- .../board/selection/SelectionActions.tsx | 15 ++---- .../buddy/components/SaveReplyToBoard.tsx | 13 ++--- .../buddy/hooks/useBuddyConversation.ts | 34 +++++------- .../board/useInvalidateBoard.test.tsx | 53 +++++++++++++++++++ 6 files changed, 108 insertions(+), 50 deletions(-) create mode 100644 src/features/board/hooks/useInvalidateBoard.ts create mode 100644 tests/unit/features/board/useInvalidateBoard.test.tsx diff --git a/src/features/board/hooks/useInvalidateBoard.ts b/src/features/board/hooks/useInvalidateBoard.ts new file mode 100644 index 00000000..a204a2e4 --- /dev/null +++ b/src/features/board/hooks/useInvalidateBoard.ts @@ -0,0 +1,29 @@ +import { useCallback } from "react"; +import { useQueryClient } from "@tanstack/react-query"; +import { queryKeys } from "../../../services/queryKeys"; + +/** + * Marks the cached board stale after a write made outside the board itself — a checklist kept + * from a reply, a card authored from a selection, a confirmed buddy action. + * + * These writes land on a surface the hire is usually not looking at, so nothing else would ever + * re-read the board: an open board behind the dock keeps serving its previous read, and a visit + * inside the `staleTime` window is served the pre-write copy. Marking is what makes whichever + * comes next — the open board, or the next visit — show the card. Call it only once the write has + * really happened: a failed save must not cost the board its cache. + * + * @param projectId The project the card went to. Omit it only for writes whose project the + * backend resolved server-side (buddy actions) and never told the client — that marks every + * cached board rather than guessing one. + * @returns A stable callback; call it after a successful write to invalidate the board cache. + */ +export function useInvalidateBoard(projectId?: string) { + const queryClient = useQueryClient(); + + return useCallback(() => { + void queryClient.invalidateQueries({ + queryKey: + projectId === undefined ? queryKeys.board.all() : queryKeys.board.byProject(projectId), + }); + }, [queryClient, projectId]); +} diff --git a/src/features/board/save/SaveToBoard.tsx b/src/features/board/save/SaveToBoard.tsx index a3fae6d3..96b34f62 100644 --- a/src/features/board/save/SaveToBoard.tsx +++ b/src/features/board/save/SaveToBoard.tsx @@ -1,11 +1,10 @@ import { useState, type ReactNode } from "react"; -import { useQueryClient } from "@tanstack/react-query"; import { Button, type ButtonSize } from "../../../components/ui/Button"; import { useToast } from "../../../context/useToast"; import { boardService } from "../../../services/boardService"; -import { queryKeys } from "../../../services/queryKeys"; import { useProjectContext } from "../../projects/useProjectContext"; +import { useInvalidateBoard } from "../hooks/useInvalidateBoard"; import { rememberOrigin, type CardOrigin } from "../layout/cardOrigins"; import type { AuthoredCardRequest } from "../types"; @@ -78,7 +77,7 @@ export function SaveToBoard({ }: SaveToBoardProps) { const { selectedProjectId } = useProjectContext(); const toast = useToast(); - const queryClient = useQueryClient(); + const invalidateBoard = useInvalidateBoard(selectedProjectId); const [saving, setSaving] = useState(false); const [saved, setSaved] = useState(false); @@ -93,13 +92,8 @@ export function SaveToBoard({ try { const created = await boardService.addCard(selectedProjectId, card); - // Kept from wherever this was found — a chat, a buddy reply, a task card — so the board is - // usually not on screen, and even where it is (this button lives on it too) what drew it is - // an earlier read. Marking the cache stale is what makes whichever comes next — the open - // board, or the next visit — show the card instead of the state it was read at. - void queryClient.invalidateQueries({ - queryKey: queryKeys.board.byProject(selectedProjectId), - }); + // This button is often pressed far away from the board — see `useInvalidateBoard`. + invalidateBoard(); const where = origin?.(); // Never allowed to fail the save: the card is what was asked for, the trail back is extra. diff --git a/src/features/board/selection/SelectionActions.tsx b/src/features/board/selection/SelectionActions.tsx index 3dc4a4a4..60788d82 100644 --- a/src/features/board/selection/SelectionActions.tsx +++ b/src/features/board/selection/SelectionActions.tsx @@ -1,5 +1,4 @@ import { useCallback, useContext, useState } from "react"; -import { useQueryClient } from "@tanstack/react-query"; import { useNavigate } from "react-router-dom"; import { BookmarkPlus, Eraser, Highlighter, MessageCircle, Reply } from "lucide-react"; import { Button } from "../../../components/ui/Button"; @@ -8,7 +7,7 @@ import { useFocusMode } from "../../../context/useFocusMode"; import { useProjectContext } from "../../projects/useProjectContext"; import { ChatContext } from "../../../context/ChatContext"; import { boardService } from "../../../services/boardService"; -import { queryKeys } from "../../../services/queryKeys"; +import { useInvalidateBoard } from "../hooks/useInvalidateBoard"; import { rememberOrigin } from "../layout/cardOrigins"; import { useCardMarks } from "../marks/useCardMarks"; import { DEFAULT_HIGHLIGHT } from "../marks/highlightColors"; @@ -50,7 +49,7 @@ export function SelectionActions() { const quoteSelection = chatContext?.quoteSelection; const [saving, setSaving] = useState(false); const toast = useToast(); - const queryClient = useQueryClient(); + const invalidateBoard = useInvalidateBoard(selectedProjectId); const navigate = useNavigate(); const add = useCallback(async () => { @@ -68,12 +67,8 @@ export function SelectionActions() { url: selection.origin, label: selection.source ?? "where you were", }); - // Found anywhere in the app, so the board is almost never on screen — and where it is, its - // cache was read before this card existed. Marking it stale is what makes the next board - // read — a visit, or the open board behind this page — show it. - void queryClient.invalidateQueries({ - queryKey: queryKeys.board.byProject(selectedProjectId), - }); + // Found anywhere in the app, so the board is almost never on screen — see `useInvalidateBoard`. + invalidateBoard(); toast.success( request.kind === "LINK" ? "Link saved to your board" : "Note saved to your board", { @@ -87,7 +82,7 @@ export function SelectionActions() { } finally { setSaving(false); } - }, [selection, selectedProjectId, toast, navigate, clear, queryClient]); + }, [selection, selectedProjectId, toast, navigate, clear, invalidateBoard]); /** * Hands the selection to the buddy as a quote, unsent. diff --git a/src/features/buddy/components/SaveReplyToBoard.tsx b/src/features/buddy/components/SaveReplyToBoard.tsx index fc76e438..14d054b9 100644 --- a/src/features/buddy/components/SaveReplyToBoard.tsx +++ b/src/features/buddy/components/SaveReplyToBoard.tsx @@ -1,11 +1,10 @@ import { useState } from "react"; -import { useQueryClient } from "@tanstack/react-query"; import { ListPlus } from "lucide-react"; import { Button } from "../../../components/ui/Button"; import { useToast } from "../../../context/useToast"; import { boardService } from "../../../services/boardService"; -import { queryKeys } from "../../../services/queryKeys"; import { extractChecklist, toChecklistRequest } from "../../board/generation/checklistFromMarkdown"; +import { useInvalidateBoard } from "../../board/hooks/useInvalidateBoard"; import { useProjectContext } from "../../projects/useProjectContext"; type SaveReplyToBoardProps = { @@ -31,7 +30,7 @@ type SaveReplyToBoardProps = { export function SaveReplyToBoard({ content }: SaveReplyToBoardProps) { const { selectedProjectId } = useProjectContext(); const toast = useToast(); - const queryClient = useQueryClient(); + const invalidateBoard = useInvalidateBoard(selectedProjectId); const [saving, setSaving] = useState(false); const [saved, setSaved] = useState(false); @@ -45,12 +44,8 @@ export function SaveReplyToBoard({ content }: SaveReplyToBoardProps) { setSaving(true); try { await boardService.addCard(selectedProjectId, toChecklistRequest(checklist)); - // The board this list just joined is very often already cached — this dock can be floating - // over it, and a visit within `staleTime` would otherwise serve the pre-card board. Marking - // it stale is what makes the open board (or the next visit) show the list the hire kept. - void queryClient.invalidateQueries({ - queryKey: queryKeys.board.byProject(selectedProjectId), - }); + // The board this list joined may be cached right behind this dock — see `useInvalidateBoard`. + invalidateBoard(); setSaved(true); toast.success("Kept on your board", { description: `"${checklist.title}" — ${checklist.items.length} things to tick off.`, diff --git a/src/features/buddy/hooks/useBuddyConversation.ts b/src/features/buddy/hooks/useBuddyConversation.ts index e9534587..2b9c8953 100644 --- a/src/features/buddy/hooks/useBuddyConversation.ts +++ b/src/features/buddy/hooks/useBuddyConversation.ts @@ -1,5 +1,4 @@ import { useCallback, useEffect, useRef, useState } from "react"; -import { useQueryClient } from "@tanstack/react-query"; import { useDinoUnlocked, useSpaceOpensDino } from "../../easter-eggs/hooks/useDinoWaitingGame"; import { matchEggPhrase } from "../../easter-eggs/lib/eggPhrases"; import { playEggEffect } from "../../easter-eggs/eggEffectBus"; @@ -13,7 +12,7 @@ import { type BuddyOpeningAction, } from "../../../services/buddyService"; import { useAuth } from "../../../context/useAuth"; -import { queryKeys } from "../../../services/queryKeys"; +import { useInvalidateBoard } from "../../board/hooks/useInvalidateBoard"; import { BUDDY_ACTION_AMEND_CHECKLIST, BUDDY_ACTION_CLAIM_GOAL, @@ -120,7 +119,7 @@ export function useBuddyConversation( const [messages, setMessages] = useState([]); const [isThinking, setIsThinking] = useState(false); const [isStreaming, setIsStreaming] = useState(false); - const queryClient = useQueryClient(); + const invalidateBoard = useInvalidateBoard(); // The tool the buddy is running right now, if any -- drives "Checking your progress…" // in place of a generic spinner. Cleared as soon as the answer starts streaming. const [activeTool, setActiveTool] = useState(null); @@ -596,17 +595,13 @@ export function useBuddyConversation( const touched = { board: false }; /** - * Marks the board stale when this turn ran the card-placing tool, and only then. - * - * `place_card` is not confirmed and the wire reports nothing about its outcome — the - * `tool_use` event is the whole signal, and it cannot say whether the tool wrote a card or - * was refused. So the mark is applied once the turn ends, the failing paths included: a - * turn that wrote a card and then broke still wrote it, and a turn whose tool refused - * costs one refetch of an unchanged board. Refreshing a board that did not change beats - * missing one that did. + * `place_card` is the one board write that is not confirmed: its `tool_use` event is the + * whole signal the client gets, and it cannot say whether the tool wrote a card or was + * refused. So the board is marked stale when the turn ends — the failing paths included. + * See `useInvalidateBoard` for why the mark is what makes this visible. */ const syncBoardIfTouched = () => { - if (touched.board) void queryClient.invalidateQueries({ queryKey: queryKeys.board.all() }); + if (touched.board) invalidateBoard(); }; try { @@ -721,7 +716,7 @@ export function useBuddyConversation( syncBoardIfTouched(); } }, - [failReply, queryClient], + [failReply, invalidateBoard], ); /** Patches one proposed action in place, keyed by its message and action id. */ @@ -811,14 +806,11 @@ export function useBuddyConversation( outcome: result.message, }); - // A confirmed board write changed a surface that is not on screen here: the board - // itself. Marking the cached board stale is what lets an open board — the dock can be - // floating over it — or a visit within the cache window show the change, instead of the - // state it was read at. `board.all()` rather than the selected project's key, because - // the backend re-resolves the project server-side (the caller's single onboarding - // project) and the client never learns which board it wrote. + // `board.all()`, not a project key: the backend re-resolves the project server-side + // (the caller's single onboarding project) and never tells the client which board — + // see `useInvalidateBoard`. if (result.ok && "action" in action && BUDDY_BOARD_ACTIONS.has(action.action)) { - void queryClient.invalidateQueries({ queryKey: queryKeys.board.all() }); + invalidateBoard(); } } catch (e) { console.error(e); @@ -843,7 +835,7 @@ export function useBuddyConversation( } })(); }, - [beginDecision, endDecision, patchAction, queryClient], + [beginDecision, endDecision, patchAction, invalidateBoard], ); /** diff --git a/tests/unit/features/board/useInvalidateBoard.test.tsx b/tests/unit/features/board/useInvalidateBoard.test.tsx new file mode 100644 index 00000000..c97906f9 --- /dev/null +++ b/tests/unit/features/board/useInvalidateBoard.test.tsx @@ -0,0 +1,53 @@ +import { renderHook, waitFor } from "@testing-library/react"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import type { ReactNode } from "react"; +import { describe, it, expect } from "vitest"; +import { useInvalidateBoard } from "../../../../src/features/board/hooks/useInvalidateBoard"; +import { queryKeys } from "../../../../src/services/queryKeys"; + +/** A client holding two projects' boards, read a moment ago. */ +function boardContext() { + const client = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + client.setQueryData(queryKeys.board.byProject("p1"), { + boardId: "b1", + projectId: "p1", + cards: [], + }); + client.setQueryData(queryKeys.board.byProject("p2"), { + boardId: "b2", + projectId: "p2", + cards: [], + }); + + function Wrapper({ children }: { children: ReactNode }) { + return {children}; + } + + return { client, Wrapper }; +} + +const boardIsStale = (client: QueryClient, projectId: string) => + client.getQueryState(queryKeys.board.byProject(projectId))?.isInvalidated ?? false; + +describe("useInvalidateBoard", () => { + it("marks only the board that was written to", async () => { + const { client, Wrapper } = boardContext(); + const { result } = renderHook(() => useInvalidateBoard("p1"), { wrapper: Wrapper }); + + result.current(); + + await waitFor(() => expect(boardIsStale(client, "p1")).toBe(true)); + // The other project's board is untouched — the writer knew where the card went. + expect(boardIsStale(client, "p2")).toBe(false); + }); + + it("marks every cached board when the writer never learned the project", async () => { + const { client, Wrapper } = boardContext(); + const { result } = renderHook(() => useInvalidateBoard(), { wrapper: Wrapper }); + + result.current(); + + await waitFor(() => expect(boardIsStale(client, "p1")).toBe(true)); + expect(boardIsStale(client, "p2")).toBe(true); + }); +});