From 4aed3cdc3f07406fd4fa4f9fae9ae83345511b11 Mon Sep 17 00:00:00 2001 From: Brandon Sherman Date: Tue, 1 Sep 2026 16:02:09 +1000 Subject: [PATCH 01/10] fix(chat): put the chat row back inside ArtifactPolicyProvider MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Category:** bug-fix **User Impact:** "Open in editor" in the artifact viewer's ⋯ menu and the right rail's artifact rows work again in full chat, and artifact auto-open is restored. **Problem:** PR #178 moved ArtifactPolicyProvider from ChatView into the extracted ChatTranscriptSurface. The transcript kept its provider, but sibling consumers in the chat row — ArtifactViewerPanel (the file viewer that mounts beside the conversation), the right rail's ArtifactsWidget, and ArtifactAutoOpenMount — were left outside it. They received the inert default context, whose openResolvedPath is an async no-op and whose artifact list is empty, so viewer/right-rail opens did nothing (silently — call sites swallow errors) and auto-open never fired. The main composer's artifact @-mentions survived only because the composer renders inside the transcript via the footer prop. **Solution:** Add an outer ArtifactPolicyProvider around ChatView's chat row. ChatTranscriptSurface keeps its own inner provider (Home canvas cards rely on it), so the transcript nests one provider inside the other. Unlike the pre-#178 provider, the outer one receives the ungated controller.messages rather than the paint-gated timeline list; that is safe for auto-open (the hook baselines whatever is present while history loads and on its first settled pass) and means the baseline is computed from the real message list instead of a transiently empty one. The ChatView test previously mocked the provider as a pass-through, which is why this was invisible to the suite. The mock now stamps a structural marker with the provider's props, and a regression test asserts the viewer panel mounts inside a provider that received real session data; it fails against the unfixed ChatView. --- src/features/chat/ui/ChatView.tsx | 16 ++++- .../ui/__tests__/ChatView.mcpApp.test.tsx | 66 ++++++++++++++++++- 2 files changed, 79 insertions(+), 3 deletions(-) diff --git a/src/features/chat/ui/ChatView.tsx b/src/features/chat/ui/ChatView.tsx index 8df4f6359..0b0c89a93 100644 --- a/src/features/chat/ui/ChatView.tsx +++ b/src/features/chat/ui/ChatView.tsx @@ -20,6 +20,7 @@ import { CONVERSATION_MIN_WIDTH_WITH_VIEWER, } from "./ArtifactViewerPanel"; import { useOpenArtifact } from "../stores/artifactViewerStore"; +import { ArtifactPolicyProvider } from "../hooks/ArtifactPolicyContext"; import { ArtifactAutoOpenMount } from "./ArtifactAutoOpenMount"; import { CP_TOTAL_W, @@ -793,7 +794,18 @@ export function ChatView({ }); return ( - <> + // The provider must wrap the whole chat row — not just the transcript — + // because siblings of the transcript consume the artifact context too: + // ArtifactViewerPanel ("Open in editor"), the right rail's + // ArtifactsWidget (row opens), and ArtifactAutoOpenMount (the artifact + // list). ChatTranscriptSurface renders its own provider for the + // transcript, but without this outer one its siblings get the inert + // default context and their actions silently no-op. + - + ); } diff --git a/src/features/chat/ui/__tests__/ChatView.mcpApp.test.tsx b/src/features/chat/ui/__tests__/ChatView.mcpApp.test.tsx index 6d8701366..b520930a3 100644 --- a/src/features/chat/ui/__tests__/ChatView.mcpApp.test.tsx +++ b/src/features/chat/ui/__tests__/ChatView.mcpApp.test.tsx @@ -272,11 +272,51 @@ vi.mock("@/features/terminal/ui/TerminalPanel", () => ({ ), })); +// The provider mock leaves a structural marker so tests can assert which +// subtrees actually live inside it. Consumers outside the provider silently +// receive the inert default context (every action is a no-op), so provider +// placement is load-bearing: see "keeps the artifact viewer panel inside the +// artifact policy provider" below. The marker stamps the provider's props so +// tests can also assert the enclosing provider received real data — nesting +// alone would pass even with e.g. messages={[]}. Note the marker renders for +// every provider (ChatView's outer one AND ChatTranscriptSurface's inner +// one), so assertions must use closest()/within(), never a singular +// getByTestId. vi.mock("../../hooks/ArtifactPolicyContext", () => ({ - ArtifactPolicyProvider: ({ children }: { children: ReactNode }) => children, + ArtifactPolicyProvider: ({ + children, + sessionId, + sessionCwd, + messages, + }: { + children: ReactNode; + sessionId?: string | null; + sessionCwd?: string | null; + messages: unknown[]; + }) => ( +
+ {children} +
+ ), useSessionArtifacts: () => [], })); +vi.mock("../ArtifactViewerPanel", async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + ArtifactViewerPanel: ({ sessionId }: { sessionId: string }) => ( +
+ ), + }; +}); + vi.mock("../../hooks/useChatSessionController", () => ({ useChatSessionController: mocks.useChatSessionController, })); @@ -523,6 +563,30 @@ describe("ChatView MCP app messaging", () => { }); }); + it("keeps the artifact viewer panel inside the artifact policy provider", () => { + // Regression guard: the viewer's "Open in editor" action comes from the + // artifact actions context. If the panel mounts outside the provider, it + // silently receives the inert default context and the action no-ops with + // no error (this shipped once — the provider moved into + // ChatTranscriptSurface and left the panel orphaned). + render( + , + ); + + const panel = screen.getByTestId("artifact-viewer-panel"); + const provider = panel.closest( + "[data-testid='artifact-policy-provider']", + ) as HTMLElement | null; + expect(provider).not.toBeNull(); + // Nesting alone is not enough — the enclosing provider must be the one + // fed real session data, not an accidental wrapper with empty props. + expect(provider?.dataset.sessionId).toBe("session-1"); + expect(Number(provider?.dataset.messageCount)).toBeGreaterThan(0); + }); + it("gates session surveys through the dedicated build capability", () => { vi.stubEnv("VITE_FEEDBACK", "0"); vi.stubEnv("VITE_FEEDBACK_SURVEYS", "1"); From 175107c6b53324b887e649f84250bed728849e75 Mon Sep 17 00:00:00 2001 From: Brandon Sherman Date: Tue, 1 Sep 2026 16:02:22 +1000 Subject: [PATCH 02/10] fix(tauri): allow popped-out session windows to open artifact paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Category:** bug-fix **User Impact:** "Open in editor" works in popped-out chat windows, not just the main window. **Problem:** The session-window capability granted opener:allow-open-path only for $DOWNLOAD, while the main window allows the artifact roots users actually work in ($HOME, $TEMP, mounted volumes, workspace paths). A popped-out session window renders the same ChatView and artifact viewer, so even with the provider fix its "Open in editor" hand-off reached Tauri and was rejected by capability scope — the same silent click-does-nothing symptom, since call sites swallow the rejection. **Solution:** Mirror the main window's opener:allow-open-path scope in the session-window capability. Generated capability schema regenerated via cargo check. --- src-tauri/capabilities/session-window.json | 29 +++++++++++++++++++++- src-tauri/gen/schemas/capabilities.json | 2 +- 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/src-tauri/capabilities/session-window.json b/src-tauri/capabilities/session-window.json index 5c2bee1ed..673803bd1 100644 --- a/src-tauri/capabilities/session-window.json +++ b/src-tauri/capabilities/session-window.json @@ -28,7 +28,34 @@ "identifier": "opener:allow-open-path", "allow": [ { - "path": "$DOWNLOAD" + "path": "$HOME/**" + }, + { + "path": "$HOME/.goose/**" + }, + { + "path": "$TEMP/**" + }, + { + "path": "/Volumes/**" + }, + { + "path": "/mnt/**" + }, + { + "path": "/workspace/**" + }, + { + "path": "/workspaces/**" + }, + { + "path": "/opt/**" + }, + { + "path": "/srv/**" + }, + { + "path": "*:/**" } ] }, diff --git a/src-tauri/gen/schemas/capabilities.json b/src-tauri/gen/schemas/capabilities.json index 343a796e1..07fa89bf0 100644 --- a/src-tauri/gen/schemas/capabilities.json +++ b/src-tauri/gen/schemas/capabilities.json @@ -1 +1 @@ -{"default":{"identifier":"default","description":"Capability for the main window","local":true,"windows":["main"],"permissions":["core:default","core:window:allow-start-dragging","core:window:allow-toggle-maximize","core:window:allow-show","core:window:allow-close","core:window:allow-set-size","core:window:allow-set-min-size","deep-link:default","opener:default",{"identifier":"opener:allow-open-url","allow":[{"url":"linear://*"},{"url":"https://linear.app/*"}]},{"identifier":"opener:allow-open-path","allow":[{"path":"$HOME/**"},{"path":"$HOME/.goose/**"},{"path":"$TEMP/**"},{"path":"/Volumes/**"},{"path":"/mnt/**"},{"path":"/workspace/**"},{"path":"/workspaces/**"},{"path":"/opt/**"},{"path":"/srv/**"},{"path":"*:/**"}]},"window-state:allow-restore-state","window-state:allow-save-window-state","dialog:allow-open","dialog:allow-save","clipboard-manager:allow-write-text","app-test-driver:default","berdctl:default","updater:default","process:allow-restart","notification:default"]},"session-window":{"identifier":"session-window","description":"Capability for popped-out chat session windows","local":true,"windows":["session:*"],"permissions":["core:default","core:window:allow-start-dragging","core:window:allow-show","core:window:allow-close","core:window:allow-set-focus","core:window:allow-set-size","core:window:allow-set-min-size","opener:default",{"identifier":"opener:allow-open-url","allow":[{"url":"linear://*"},{"url":"https://linear.app/*"}]},{"identifier":"opener:allow-open-path","allow":[{"path":"$DOWNLOAD"}]},"dialog:allow-open","dialog:allow-save","berdctl:allow-status"],"platforms":["macOS"]},"voice-buddy":{"identifier":"voice-buddy","description":"Capability for the always-on-top voice conversation buddy","local":true,"windows":["voice-buddy"],"permissions":["core:default","core:window:allow-start-dragging"]}} \ No newline at end of file +{"default":{"identifier":"default","description":"Capability for the main window","local":true,"windows":["main"],"permissions":["core:default","core:window:allow-start-dragging","core:window:allow-toggle-maximize","core:window:allow-show","core:window:allow-close","core:window:allow-set-size","core:window:allow-set-min-size","deep-link:default","opener:default",{"identifier":"opener:allow-open-url","allow":[{"url":"linear://*"},{"url":"https://linear.app/*"}]},{"identifier":"opener:allow-open-path","allow":[{"path":"$HOME/**"},{"path":"$HOME/.goose/**"},{"path":"$TEMP/**"},{"path":"/Volumes/**"},{"path":"/mnt/**"},{"path":"/workspace/**"},{"path":"/workspaces/**"},{"path":"/opt/**"},{"path":"/srv/**"},{"path":"*:/**"}]},"window-state:allow-restore-state","window-state:allow-save-window-state","dialog:allow-open","dialog:allow-save","clipboard-manager:allow-write-text","app-test-driver:default","berdctl:default","updater:default","process:allow-restart","notification:default"]},"session-window":{"identifier":"session-window","description":"Capability for popped-out chat session windows","local":true,"windows":["session:*"],"permissions":["core:default","core:window:allow-start-dragging","core:window:allow-show","core:window:allow-close","core:window:allow-set-focus","core:window:allow-set-size","core:window:allow-set-min-size","opener:default",{"identifier":"opener:allow-open-url","allow":[{"url":"linear://*"},{"url":"https://linear.app/*"}]},{"identifier":"opener:allow-open-path","allow":[{"path":"$HOME/**"},{"path":"$HOME/.goose/**"},{"path":"$TEMP/**"},{"path":"/Volumes/**"},{"path":"/mnt/**"},{"path":"/workspace/**"},{"path":"/workspaces/**"},{"path":"/opt/**"},{"path":"/srv/**"},{"path":"*:/**"}]},"dialog:allow-open","dialog:allow-save","berdctl:allow-status"],"platforms":["macOS"]},"voice-buddy":{"identifier":"voice-buddy","description":"Capability for the always-on-top voice conversation buddy","local":true,"windows":["voice-buddy"],"permissions":["core:default","core:window:allow-start-dragging"]}} \ No newline at end of file From ab6b861104c82432c960634886d7aaa858097ee3 Mon Sep 17 00:00:00 2001 From: Brandon Sherman Date: Tue, 1 Sep 2026 16:02:39 +1000 Subject: [PATCH 03/10] fix(home): give the canvas card composer an artifact provider MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Category:** bug-fix **User Impact:** @-file mentions in a Home canvas chat card's composer can suggest the session's artifacts again. **Problem:** Same regression family as the chat-row fix: in ChatCanvasCard the composer is a sibling of ChatTranscriptSurface, so it sits outside the transcript's ArtifactPolicyProvider. Its mention handlers read the session artifact list from context and silently saw an empty list. (In full chat the composer is passed as the transcript's footer, so it renders inside the provider — the canvas card was the only orphaned composer.) **Solution:** Wrap CanvasCardComposer in an ArtifactPolicyProvider fed the full (unbounded) session messages, so mentions cover the whole session rather than the card's bounded projection. --- src/features/home/widgets/ChatCanvasCard.tsx | 24 +++++++++++++++----- 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/src/features/home/widgets/ChatCanvasCard.tsx b/src/features/home/widgets/ChatCanvasCard.tsx index a5dd04e21..710272ffd 100644 --- a/src/features/home/widgets/ChatCanvasCard.tsx +++ b/src/features/home/widgets/ChatCanvasCard.tsx @@ -11,6 +11,7 @@ import { DEFAULT_CHAT_TITLE } from "@/features/chat/lib/sessionTitle"; import { projectRecentConversationExchanges } from "@/features/chat/lib/boundedConversationProjection"; import type { ChatSession } from "@/features/chat/stores/chatSessionStore"; import { useChatStore } from "@/features/chat/stores/chatStore"; +import { ArtifactPolicyProvider } from "@/features/chat/hooks/ArtifactPolicyContext"; import { ChatTranscriptSurface } from "@/features/chat/ui/ChatTranscriptSurface"; import { LoadingBerd } from "@/features/chat/ui/LoadingBerd"; import { selectProjects } from "@/features/projects/stores/projectSelectors"; @@ -258,12 +259,23 @@ export function ChatCanvasCard({ onClick={(event) => event.stopPropagation()} onFocusCapture={activateComposerFromFocus} > - + {/* The composer is a sibling of ChatTranscriptSurface, so it sits + outside the transcript's own ArtifactPolicyProvider. Its @-file + mentions read the session artifact list from context; without a + provider here they silently see an empty list. Full messages (not + the bounded projection) so mentions cover the whole session. */} + + +
); From 1244c8b8dd5f4b48a9725db713c28ae97c1423ff Mon Sep 17 00:00:00 2001 From: Brandon Sherman Date: Tue, 1 Sep 2026 16:02:39 +1000 Subject: [PATCH 04/10] fix(chat): don't let a failed artifact open consume the retry debounce MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Category:** bug-fix **User Impact:** If handing a file to the OS fails, clicking again immediately retries instead of being silently absorbed. **Problem:** openResolvedPath recorded its per-path debounce timestamp before awaiting the Tauri opener. When the open was rejected (capability scope, missing handler), the failure still occupied the 1200ms dedupe window, so an immediate retry returned early and did nothing. **Solution:** Clear the debounce entry when openPath rejects, then rethrow. Also adds integration-style tests that render the REAL ArtifactPolicyProvider (only the Tauri boundary mocked) and assert that "Open in editor" in the viewer and a right-rail artifact row open both reach the opener — the provider-scope regression class is invisible to tests that mock the context, so these keep the real wiring covered. The debounce test fails against the pre-fix openResolvedPath. --- .../chat/hooks/ArtifactPolicyContext.tsx | 10 +- .../ArtifactViewer.openActions.test.tsx | 159 ++++++++++++++++++ 2 files changed, 168 insertions(+), 1 deletion(-) create mode 100644 src/features/chat/ui/__tests__/ArtifactViewer.openActions.test.tsx diff --git a/src/features/chat/hooks/ArtifactPolicyContext.tsx b/src/features/chat/hooks/ArtifactPolicyContext.tsx index 65120c93f..9e9b0300e 100644 --- a/src/features/chat/hooks/ArtifactPolicyContext.tsx +++ b/src/features/chat/hooks/ArtifactPolicyContext.tsx @@ -420,7 +420,15 @@ export function ArtifactPolicyProvider({ return; } lastOpenAtByPathRef.current.set(key, now); - await openPath(resolvedTarget); + try { + await openPath(resolvedTarget); + } catch (error) { + // A failed hand-off must not consume the debounce window: the user + // should be able to retry immediately (e.g. after fixing whatever the + // OS rejected) instead of having the retry silently absorbed. + lastOpenAtByPathRef.current.delete(key); + throw error; + } }, [filesAreRemote, remoteHost, resolveOpenTarget, normalizedSessionCwd, t], ); diff --git a/src/features/chat/ui/__tests__/ArtifactViewer.openActions.test.tsx b/src/features/chat/ui/__tests__/ArtifactViewer.openActions.test.tsx new file mode 100644 index 000000000..ef2775285 --- /dev/null +++ b/src/features/chat/ui/__tests__/ArtifactViewer.openActions.test.tsx @@ -0,0 +1,159 @@ +import { render, screen } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import type { Message } from "@/shared/types/messages"; +import { ArtifactPolicyProvider } from "../../hooks/ArtifactPolicyContext"; +import { ArtifactViewer } from "../ArtifactViewer"; +import { ArtifactsWidget } from "../widgets/ArtifactsWidget"; + +// End-to-end context wiring tests: unlike ArtifactViewer.test.tsx (which +// mocks useArtifactActionsContext), these render the REAL +// ArtifactPolicyProvider so a click on "Open in editor" must flow through the +// real context -> openResolvedPath -> pathExists -> the Tauri opener. The +// provider-scope regression that shipped in #178 (consumers silently getting +// the inert default context) is invisible to tests that mock the context; +// this file exists so the real wiring stays covered. + +const mockOpenPath = vi.fn<(path: string) => Promise>(); +const mockPathExists = vi.fn<(path: string) => Promise>(); +const mockReadTextFile = vi.fn(); +const mockStatFile = vi.fn(); + +vi.mock("@tauri-apps/plugin-opener", () => ({ + openPath: (path: string) => mockOpenPath(path), + revealItemInDir: vi.fn().mockResolvedValue(undefined), +})); + +vi.mock("@/shared/api/system", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + pathExists: (path: string) => mockPathExists(path), + readTextFile: (path: string) => mockReadTextFile(path), + statFile: (path: string) => mockStatFile(path), + }; +}); + +// jsdom has no Tauri internals, so the real asset-URL converter throws. +vi.mock("@tauri-apps/api/core", () => ({ + convertFileSrc: (path: string) => `asset://localhost/${path}`, + invoke: vi.fn(), +})); + +function messageWithArtifact(path: string): Message { + return { + id: "assistant-1", + role: "assistant", + created: 1, + content: [ + { + type: "toolRequest", + id: "tool-1", + name: "developer__text_editor", + arguments: {}, + status: "success", + locations: [{ path }], + }, + ], + } as unknown as Message; +} + +function renderWithRealProvider(ui: React.ReactNode, path: string) { + return render( + + {ui} + , + ); +} + +beforeEach(() => { + vi.spyOn(document, "hasFocus").mockReturnValue(true); + mockOpenPath.mockReset(); + mockOpenPath.mockResolvedValue(undefined); + mockPathExists.mockReset(); + mockPathExists.mockResolvedValue(true); + mockReadTextFile.mockReset(); + mockReadTextFile.mockResolvedValue({ contents: "# Title\n\nBody." }); + mockStatFile.mockReset(); + mockStatFile.mockResolvedValue({ byteSize: "20", modifiedAtNs: "1" }); +}); + +afterEach(() => { + vi.restoreAllMocks(); +}); + +describe("ArtifactViewer open actions through the real provider", () => { + it("hands the file to the OS editor via the real context", async () => { + const user = userEvent.setup(); + renderWithRealProvider( + , + "/p/report.md", + ); + + await user.click(screen.getByRole("button", { name: /file actions/i })); + await user.click(screen.getByRole("menuitem", { name: /open in editor/i })); + + await vi.waitFor(() => { + expect(mockOpenPath).toHaveBeenCalledWith("/p/report.md"); + }); + }); +}); + +describe("ArtifactsWidget open actions through the real provider", () => { + it("opens a non-viewable artifact externally via the real context", async () => { + const user = userEvent.setup(); + renderWithRealProvider( + , + "/p/data.csv", + ); + + await user.click(screen.getByRole("button", { name: /data\.csv/i })); + + await vi.waitFor(() => { + expect(mockOpenPath).toHaveBeenCalledWith("/p/data.csv"); + }); + }); +}); + +describe("openResolvedPath failure handling", () => { + it("does not let a failed hand-off consume the retry debounce", async () => { + const user = userEvent.setup(); + mockOpenPath.mockRejectedValueOnce(new Error("forbidden path")); + renderWithRealProvider( + , + "/p/report.md", + ); + + await user.click(screen.getByRole("button", { name: /file actions/i })); + await user.click(screen.getByRole("menuitem", { name: /open in editor/i })); + await vi.waitFor(() => { + expect(mockOpenPath).toHaveBeenCalledTimes(1); + }); + + // Retry immediately: the failed attempt must not occupy the 1200ms + // dedupe window, or the user's second click is silently absorbed. + await user.click(screen.getByRole("button", { name: /file actions/i })); + await user.click(screen.getByRole("menuitem", { name: /open in editor/i })); + await vi.waitFor(() => { + expect(mockOpenPath).toHaveBeenCalledTimes(2); + }); + }); +}); From 90cfe94fcae6540bbfdc34a59417167b933f8f76 Mon Sep 17 00:00:00 2001 From: Brandon Sherman Date: Wed, 2 Sep 2026 11:08:39 +1000 Subject: [PATCH 05/10] fix(chat): don't offer 'Open in editor' for a deleted artifact MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Category:** bug-fix **User Impact:** The artifact viewer no longer offers "Open in editor" for a file it knows is deleted from disk. **Problem:** When the viewed file is gone, the viewer shows the "File deleted from disk." strip — but the error body still rendered an "Open in editor" button, and the header's ⋯ menu still offered the same action. Nothing can open a file that no longer exists, so both were guaranteed dead clicks (made worse by call sites swallowing the failure). **Solution:** When the divergence state is missing (the same signal that already hides the strip's pointless Reload button), hide the error body's "Open in editor" button and disable the ⋯ menu item. "Reveal in Finder" stays enabled — the containing folder still exists. Polling keeps watching the path, so if the file reappears the view heals and the actions return. Tests: error body offers no editor button for a missing file but keeps it for unreadable-but-present files; the menu item is disabled and its click never reaches the OS hand-off. Both fail against the unfixed viewer. --- src/features/chat/ui/ArtifactViewer.tsx | 29 +++++++- .../chat/ui/__tests__/ArtifactViewer.test.tsx | 67 +++++++++++++++++++ 2 files changed, 93 insertions(+), 3 deletions(-) diff --git a/src/features/chat/ui/ArtifactViewer.tsx b/src/features/chat/ui/ArtifactViewer.tsx index 60cf322a2..f5920fd9a 100644 --- a/src/features/chat/ui/ArtifactViewer.tsx +++ b/src/features/chat/ui/ArtifactViewer.tsx @@ -598,7 +598,14 @@ export function ArtifactViewer({ /> + {/* A deleted file cannot be handed to an editor, so the + action is disabled rather than left as a dead click. + Reveal stays enabled: file managers can still show the + containing folder. */} { void openResolvedPath(artifact.resolvedPath).catch( () => {}, @@ -694,6 +701,12 @@ export function ArtifactViewer({ { void openResolvedPath(artifact.resolvedPath).catch(() => {}); }} @@ -776,10 +789,13 @@ function ImageBody({ function MarkdownBody({ markdownView, textState, + fileIsMissing = false, onOpenExternally, }: { markdownView: MarkdownView; textState: TextState; + /** The file is gone from disk, so opening it externally cannot succeed. */ + fileIsMissing?: boolean; onOpenExternally: () => void; }) { const { t } = useTranslation("chat"); @@ -797,9 +813,16 @@ function MarkdownBody({

{t("artifactViewer.loadError")}

- + {/* "Open in editor" is an escape hatch for files Berd can't render + itself (encoding, size, permissions) — but a DELETED file can't be + opened by anything, so offering it would be a guaranteed dead + click. Polling keeps watching; if the file reappears the view + heals and the action returns. */} + {!fileIsMissing ? ( + + ) : null} ); } diff --git a/src/features/chat/ui/__tests__/ArtifactViewer.test.tsx b/src/features/chat/ui/__tests__/ArtifactViewer.test.tsx index fd1fb67bc..8cd3117bb 100644 --- a/src/features/chat/ui/__tests__/ArtifactViewer.test.tsx +++ b/src/features/chat/ui/__tests__/ArtifactViewer.test.tsx @@ -668,6 +668,73 @@ describe("ArtifactViewer divergence grace period", () => { expect(contentBody().className).toMatch(/\bopacity-60\b/); }); + it("does not offer 'Open in editor' in the error body for a deleted file", async () => { + // Initial load of an already-deleted file: the error body shows alongside + // the "File deleted from disk." strip. Offering "Open in editor" there + // would be a guaranteed dead click — nothing can open a file that is gone. + mockStatFile.mockRejectedValue({ + kind: "missing", + message: "no such file", + }); + render(); + await act(flushAsyncWork); + + expect(screen.getByText(/couldn't load/i)).toBeInTheDocument(); + expect(screen.getByRole("status")).toHaveTextContent( + "File deleted from disk.", + ); + expect( + screen.queryByRole("button", { name: /open in editor/i }), + ).not.toBeInTheDocument(); + }); + + it("keeps 'Open in editor' in the error body for unreadable (still present) files", async () => { + // The escape-hatch reading of the button: Berd can't render the file, but + // an external editor might. Only deletion removes it. + mockStatFile.mockRejectedValue({ kind: "other", message: "EACCES" }); + render(); + await act(flushAsyncWork); + + expect(screen.getByText(/couldn't load/i)).toBeInTheDocument(); + expect( + screen.getByRole("button", { name: /open in editor/i }), + ).toBeInTheDocument(); + }); + + it("disables the header menu's 'Open in editor' once the file is deleted", async () => { + mockOpenResolvedPath.mockClear(); + await renderLoadedViewer(); + + mockStatFile.mockRejectedValue({ + kind: "missing", + message: "no such file", + }); + await advancePollCycle(); + await advancePollCycle(); + expect(screen.getByRole("status")).toHaveTextContent( + "File deleted from disk.", + ); + + // userEvent needs real timers to advance its internal delays; the polled + // divergence state is already settled at this point. + vi.useRealTimers(); + const user = userEvent.setup(); + await user.click(screen.getByRole("button", { name: /file actions/i })); + + const openItem = screen.getByRole("menuitem", { + name: /open in editor/i, + }); + expect(openItem).toHaveAttribute("aria-disabled", "true"); + // Reveal stays available: the file manager can still show the folder. + expect( + screen.getByRole("menuitem", { name: /reveal in/i }), + ).not.toHaveAttribute("aria-disabled", "true"); + + // A click on the disabled item must not reach the OS hand-off. + await user.click(openItem); + expect(mockOpenResolvedPath).not.toHaveBeenCalled(); + }); + it("reports an unreadable file with a reload action", async () => { await renderLoadedViewer(); From dcf8b24b6cb3d05799c44cc1c4ca43f6e2c8d386 Mon Sep 17 00:00:00 2001 From: Brandon Sherman Date: Thu, 3 Sep 2026 10:27:21 +1000 Subject: [PATCH 06/10] fix(chat): key the outer artifact provider by the effective session **Category:** bug-fix (review follow-up) **User Impact:** During session replacement or reconciliation, artifact actions and the viewer panel can no longer act under a stale session identity. **Problem:** The outer ArtifactPolicyProvider received the raw requested sessionId while its messages and cwd came from the controller's effective session (timelineSessionId = effectiveSession?.id ?? sessionId). When those briefly disagree, the provider's remote/local policy and viewer-store writes describe a different session than the snapshot it governs, and ArtifactViewerPanel (keyed by the raw id) reads a different store entry than openInApp writes. **Solution:** Pass timelineSessionId to the outer provider, ArtifactAutoOpenMount, and ArtifactViewerPanel so identity, messages, and cwd describe one session snapshot. Adds the discriminating test the reviewer asked for: requested id differs from controller.session.id and the provider (and panel) must carry the effective id. --- src/features/chat/ui/ChatView.tsx | 12 ++++-- .../ui/__tests__/ChatView.mcpApp.test.tsx | 39 +++++++++++++++++++ 2 files changed, 48 insertions(+), 3 deletions(-) diff --git a/src/features/chat/ui/ChatView.tsx b/src/features/chat/ui/ChatView.tsx index 0b0c89a93..d15991c80 100644 --- a/src/features/chat/ui/ChatView.tsx +++ b/src/features/chat/ui/ChatView.tsx @@ -801,15 +801,19 @@ export function ChatView({ // list). ChatTranscriptSurface renders its own provider for the // transcript, but without this outer one its siblings get the inert // default context and their actions silently no-op. + // The identity, messages, and cwd must describe one session snapshot: + // timelineSessionId (the controller's effective session) rather than the + // raw requested sessionId, which can briefly disagree with + // controller.messages during session replacement or reconciliation. @@ -955,7 +959,9 @@ export function ChatView({ {sessionId && !isAgentBuilderSession ? ( - + // Keyed by the same effective identity the providers use, so the + // panel reads the viewer-store entry that openInApp writes. + ) : null} { expect(Number(provider?.dataset.messageCount)).toBeGreaterThan(0); }); + it("keys the artifact policy provider by the controller's effective session, not the requested id", () => { + // During session replacement/reconciliation the requested sessionId can + // briefly disagree with the session snapshot the controller serves. The + // provider governs filesystem policy and viewer-store identity for its + // whole subtree, so its id must describe the same snapshot as the + // messages and cwd it receives — the effective session's id. + const controller = mocks.useChatSessionController( + "ignored", + ) as unknown as Record; + mocks.useChatSessionController.mockReturnValue({ + ...controller, + session: { + id: "session-effective", + title: "Reconciled", + workingDir: "/tmp/project", + createdAt: "2026-05-27T00:00:00.000Z", + updatedAt: "2026-05-27T00:00:00.000Z", + messageCount: 1, + intent: null, + }, + }); + + render( + , + ); + + const panel = screen.getByTestId("artifact-viewer-panel"); + const provider = panel.closest( + "[data-testid='artifact-policy-provider']", + ) as HTMLElement | null; + expect(provider?.dataset.sessionId).toBe("session-effective"); + // The viewer panel reads the viewer store under the same effective + // identity that openInApp writes to. + expect(panel.dataset.sessionId).toBe("session-effective"); + }); + it("gates session surveys through the dedicated build capability", () => { vi.stubEnv("VITE_FEEDBACK", "0"); vi.stubEnv("VITE_FEEDBACK_SURVEYS", "1"); From 3dd16849def4866fb9d5969a5302bb661a0e0990 Mon Sep 17 00:00:00 2001 From: Brandon Sherman Date: Thu, 3 Sep 2026 10:27:21 +1000 Subject: [PATCH 07/10] fix(chat): disable Reveal too once the viewed file is deleted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Category:** bug-fix (review follow-up) **User Impact:** No dead 'Reveal in Finder' click on a deleted file; both OS hand-offs come back automatically if the file reappears. **Problem:** The previous commit disabled 'Open in editor' for deleted files but deliberately kept Reveal enabled, claiming file managers could show the containing folder. That claim didn't match the implementation: the action passes the deleted file's path (not the parent directory) to revealItemInDir and swallows the rejection — the same silent dead end the commit claimed to remove. **Solution:** Disable both menu items on the shared fileIsMissing state (same signal that hides the strip's Reload button). Polling keeps watching the path, so recovery re-enables them — covered by a new reappearance test. The deleted-state test now verifies clicks on both disabled items never reach either OS boundary, not just aria-disabled. --- src/features/chat/ui/ArtifactViewer.tsx | 23 +++++---- .../chat/ui/__tests__/ArtifactViewer.test.tsx | 47 ++++++++++++++++--- 2 files changed, 54 insertions(+), 16 deletions(-) diff --git a/src/features/chat/ui/ArtifactViewer.tsx b/src/features/chat/ui/ArtifactViewer.tsx index f5920fd9a..c51794edb 100644 --- a/src/features/chat/ui/ArtifactViewer.tsx +++ b/src/features/chat/ui/ArtifactViewer.tsx @@ -158,6 +158,10 @@ export function ArtifactViewer({ const [diskStatus, setDiskStatus] = useState("checking"); const diskStatusRef = useRef(diskStatus); const [divergedKind, setDivergedKind] = useState("other"); + // Known-gone from disk (initial load or polling verdict). Gates every + // affordance that targets the file itself: the error body's "Open in + // editor" and both OS hand-offs in the ⋯ menu. + const fileIsMissing = diskStatus === "diverged" && divergedKind === "missing"; const divergenceStrikesRef = useRef(0); const [imageDiskRevision, setImageDiskRevision] = useState(0); const imageDiskRevisionRef = useRef(0); @@ -598,14 +602,14 @@ export function ArtifactViewer({ /> - {/* A deleted file cannot be handed to an editor, so the - action is disabled rather than left as a dead click. - Reveal stays enabled: file managers can still show the - containing folder. */} + {/* Both OS hand-offs target the file itself, so a deleted + file makes them guaranteed dead clicks (the editor can't + open it; revealItemInDir receives the missing path and + its rejection is swallowed). Disable rather than hide so + the menu shape stays stable; polling keeps watching the + path and re-enables both if the file reappears. */} { void openResolvedPath(artifact.resolvedPath).catch( () => {}, @@ -616,6 +620,7 @@ export function ArtifactViewer({ {t("artifactViewer.openExternally")} { void revealInFileManager(artifact.resolvedPath).catch( () => {}, @@ -704,9 +709,7 @@ export function ArtifactViewer({ // A deleted file cannot be handed to an editor; the "file // deleted" strip above is the whole answer (mirrors the strip // hiding its Reload button for the same reason). - fileIsMissing={ - diskStatus === "diverged" && divergedKind === "missing" - } + fileIsMissing={fileIsMissing} onOpenExternally={() => { void openResolvedPath(artifact.resolvedPath).catch(() => {}); }} diff --git a/src/features/chat/ui/__tests__/ArtifactViewer.test.tsx b/src/features/chat/ui/__tests__/ArtifactViewer.test.tsx index 8cd3117bb..a68a8f880 100644 --- a/src/features/chat/ui/__tests__/ArtifactViewer.test.tsx +++ b/src/features/chat/ui/__tests__/ArtifactViewer.test.tsx @@ -701,8 +701,13 @@ describe("ArtifactViewer divergence grace period", () => { ).toBeInTheDocument(); }); - it("disables the header menu's 'Open in editor' once the file is deleted", async () => { + it("disables both OS hand-offs in the header menu once the file is deleted", async () => { + // Both menu actions target the file itself, so a deleted file makes each + // a guaranteed dead click: the editor cannot open a missing path, and + // revealItemInDir receives the missing path (not the parent directory) + // with its rejection swallowed. mockOpenResolvedPath.mockClear(); + mockRevealInFileManager.mockClear(); await renderLoadedViewer(); mockStatFile.mockRejectedValue({ @@ -724,15 +729,45 @@ describe("ArtifactViewer divergence grace period", () => { const openItem = screen.getByRole("menuitem", { name: /open in editor/i, }); + const revealItem = screen.getByRole("menuitem", { name: /reveal in/i }); expect(openItem).toHaveAttribute("aria-disabled", "true"); - // Reveal stays available: the file manager can still show the folder. - expect( - screen.getByRole("menuitem", { name: /reveal in/i }), - ).not.toHaveAttribute("aria-disabled", "true"); + expect(revealItem).toHaveAttribute("aria-disabled", "true"); - // A click on the disabled item must not reach the OS hand-off. + // Clicks on the disabled items must not reach either OS boundary — + // aria-disabled alone doesn't prove the handler is inert. await user.click(openItem); + await user.click(revealItem); expect(mockOpenResolvedPath).not.toHaveBeenCalled(); + expect(mockRevealInFileManager).not.toHaveBeenCalled(); + }); + + it("re-enables both OS hand-offs when the deleted file reappears", async () => { + // Polling keeps watching the path: recovery must restore the actions, + // otherwise disabling on deletion would be a one-way trap. + mockRevealInFileManager.mockClear(); + await renderLoadedViewer(); + + mockStatFile.mockRejectedValue({ + kind: "missing", + message: "no such file", + }); + await advancePollCycle(); + await advancePollCycle(); + expect(screen.getByRole("status")).toHaveTextContent( + "File deleted from disk.", + ); + + mockStatFile.mockResolvedValue({ byteSize: "20", modifiedAtNs: "1" }); + await advancePollCycle(); + expect(screen.queryByRole("status")).not.toBeInTheDocument(); + + vi.useRealTimers(); + const user = userEvent.setup(); + await user.click(screen.getByRole("button", { name: /file actions/i })); + const revealItem = screen.getByRole("menuitem", { name: /reveal in/i }); + expect(revealItem).not.toHaveAttribute("aria-disabled", "true"); + await user.click(revealItem); + expect(mockRevealInFileManager).toHaveBeenCalledWith("/p/report.md"); }); it("reports an unreadable file with a reload action", async () => { From 3887555b91846665d3abe160f4274dd400be315c Mon Sep 17 00:00:00 2001 From: Brandon Sherman Date: Thu, 3 Sep 2026 11:05:12 +1000 Subject: [PATCH 08/10] fix(chat): derive viewer-open layout from the effective session MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Category:** bug-fix (review follow-up) **User Impact:** During session replacement/reconciliation, the artifact viewer's reserved width always agrees with the panel actually rendered — no overlapped chat under an unreserved viewer, no empty reserved column from a stale entry. **Problem:** The previous follow-up moved ArtifactViewerPanel (and the providers) to timelineSessionId but left isArtifactViewerOpen reading useOpenArtifact(sessionId) with the raw requested id. The layout math (chat-row occlusion, gap, conversation min-width) could therefore disagree with the panel about which viewer-store entry is authoritative. **Solution:** Hoist the timelineSessionId derivation above the store lookup and key useOpenArtifact by it, so the panel, providers, and every viewer-driven layout decision read one store entry. All useOpenArtifact call sites in ChatView audited (rg) — this was the only remaining raw-sessionId reader. Extends the reconciliation test per the review spec: viewer state seeded only for the effective session must reserve the conversation's viewer-open floor; stale requested-session state must not. --- src/features/chat/ui/ChatView.tsx | 10 ++- .../ui/__tests__/ChatView.mcpApp.test.tsx | 67 +++++++++++++++++++ 2 files changed, 75 insertions(+), 2 deletions(-) diff --git a/src/features/chat/ui/ChatView.tsx b/src/features/chat/ui/ChatView.tsx index d15991c80..97fd3c6c6 100644 --- a/src/features/chat/ui/ChatView.tsx +++ b/src/features/chat/ui/ChatView.tsx @@ -125,7 +125,6 @@ export function ChatView({ }: ChatViewProps) { const { t } = useTranslation("chat"); useRegisterSecurityConfirmationSurface(sessionId); - const isArtifactViewerOpen = useOpenArtifact(sessionId) !== null; const mountStart = useRef(performance.now()); const terminalRootRef = useRef(null); const chatColumnRef = useRef(null); @@ -202,6 +201,14 @@ export function ChatView({ ]); const workspaceRepository = useWorkspaceRepository(); const effectiveSession = controller.session ?? activeSession ?? null; + // The effective session identity: during session replacement or + // reconciliation the requested sessionId can briefly disagree with the + // snapshot the controller serves. Every artifact-store read/write and + // every layout decision derived from viewer state must use THIS id, so + // the panel, the policy provider, and the width math all describe the + // same store entry. (Audited: all useOpenArtifact call sites in ChatView.) + const timelineSessionId = effectiveSession?.id ?? sessionId; + const isArtifactViewerOpen = useOpenArtifact(timelineSessionId) !== null; const isReadOnly = Boolean(readOnlyStatus); // A remote session's cwd and artifact paths live on its SSH host: the // in-chat terminal (a local PTY), local folder pickers, and local file @@ -729,7 +736,6 @@ export function ChatView({ ); - const timelineSessionId = effectiveSession?.id ?? sessionId; const messageTimeline = ( { // fed real session data, not an accidental wrapper with empty props. expect(provider?.dataset.sessionId).toBe("session-1"); expect(Number(provider?.dataset.messageCount)).toBeGreaterThan(0); + // Exactly ONE policy owner per rendered session boundary: a second + // (nested) provider would derive artifact inventory twice and split the + // per-path open debounce across surfaces. + expect(screen.getAllByTestId("artifact-policy-provider")).toHaveLength(1); }); it("keys the artifact policy provider by the controller's effective session, not the requested id", () => { @@ -626,6 +631,68 @@ describe("ChatView MCP app messaging", () => { expect(panel.dataset.sessionId).toBe("session-effective"); }); + it("derives viewer-open layout from the effective session's store entry", () => { + // The panel renders under the effective session id, so the layout math + // (isArtifactViewerOpen) must read the same store entry. If it read the + // requested id instead, an open viewer would get no reserved width — or + // a stale requested-session entry would reserve an empty column. + const controller = mocks.useChatSessionController( + "ignored", + ) as unknown as Record; + mocks.useChatSessionController.mockReturnValue({ + ...controller, + session: { + id: "session-effective", + title: "Reconciled", + workingDir: "/tmp/project", + createdAt: "2026-05-27T00:00:00.000Z", + updatedAt: "2026-05-27T00:00:00.000Z", + messageCount: 1, + intent: null, + }, + }); + // Viewer state exists ONLY for the effective session. + useArtifactViewerStore.getState().open("session-effective", { + resolvedPath: "/tmp/project/report.md", + filename: "report.md", + }); + + const { unmount } = render( + , + ); + + // The conversation column reserves its viewer-open floor: layout agrees + // with the rendered panel about which store entry is authoritative. + const chatColumn = document.querySelector( + "[data-chat-column]", + ) as HTMLElement; + expect(chatColumn.style.minWidth).not.toBe(""); + + unmount(); + useArtifactViewerStore.getState().close("session-effective"); + + // Inverse: stale viewer state under the REQUESTED id must not reserve + // space — the effective session has nothing open. + useArtifactViewerStore.getState().open("session-requested", { + resolvedPath: "/tmp/project/stale.md", + filename: "stale.md", + }); + render( + , + ); + const chatColumnAfter = document.querySelector( + "[data-chat-column]", + ) as HTMLElement; + expect(chatColumnAfter.style.minWidth).toBe(""); + useArtifactViewerStore.getState().close("session-requested"); + }); + it("gates session surveys through the dedicated build capability", () => { vi.stubEnv("VITE_FEEDBACK", "0"); vi.stubEnv("VITE_FEEDBACK_SURVEYS", "1"); From 5f1a9409acfaf9b751d523d19d5013cd1103e37d Mon Sep 17 00:00:00 2001 From: Brandon Sherman Date: Thu, 3 Sep 2026 11:05:12 +1000 Subject: [PATCH 09/10] refactor(chat): one artifact policy provider per session boundary **Category:** bug-fix (review follow-up, P2) **User Impact:** Opening the same artifact quickly from two surfaces of one chat hands it to the OS once, and artifact availability can no longer drift between the transcript and its sibling surfaces. **Problem:** ChatTranscriptSurface mounted its own ArtifactPolicyProvider inside ChatView's outer one, and ChatCanvasCard gave its transcript and composer separate providers. Each provider derives artifact inventory independently and owns a private per-path open debounce, so one visible session had duplicate policy owners: double collection work per message change, divergent projections (gated vs ungated), and duplicate-open suppression that surfaces could bypass. **Solution:** Remove the transcript's implicit provider. The surface that composes a session boundary now mounts exactly one provider: ChatView's chat row (transcript + viewer panel + right rail + auto-open) and ChatCanvasCard (transcript + composer). Provider-boundary tests assert one owner covers all consumers on both surfaces (fail against the nested structure), and a debounce test proves an open from the viewer and the artifacts widget shares one per-path lifecycle. All ChatTranscriptSurface consumers audited (rg): ChatView and ChatCanvasCard are the only mounts; both now provide the context above it. --- .../chat/ui/ChatTranscriptSurface.tsx | 46 ++-- .../ArtifactViewer.openActions.test.tsx | 37 +++ .../home/widgets/ChatCanvasCard.test.tsx | 51 ++++ src/features/home/widgets/ChatCanvasCard.tsx | 258 +++++++++--------- 4 files changed, 240 insertions(+), 152 deletions(-) diff --git a/src/features/chat/ui/ChatTranscriptSurface.tsx b/src/features/chat/ui/ChatTranscriptSurface.tsx index 23959a017..e5c87de74 100644 --- a/src/features/chat/ui/ChatTranscriptSurface.tsx +++ b/src/features/chat/ui/ChatTranscriptSurface.tsx @@ -11,7 +11,6 @@ import type { Persona } from "@/shared/types/agents"; import type { Message } from "@/shared/types/messages"; import { scheduleAfterNextPaint } from "@/app/lib/scheduleAfterNextPaint"; import { useChatStore } from "@/features/chat/stores/chatStore"; -import { ArtifactPolicyProvider } from "@/features/chat/hooks/ArtifactPolicyContext"; import type { TranscriptSearchBackend } from "@/features/chat/lib/transcriptSearchBackend"; import { useSessionFeedbackSurvey } from "../response-feedback/useSessionFeedbackSurvey"; import { ChatLoadingSkeleton } from "./ChatLoadingSkeleton"; @@ -162,30 +161,29 @@ export function ChatTranscriptSurface({ ); + // No ArtifactPolicyProvider here: exactly one provider is mounted per + // rendered session boundary, owned by the surface that composes the + // transcript with its siblings (ChatView's chat row, ChatCanvasCard). + // Two providers for one visible session would derive artifact inventory + // twice and split the per-path open debounce across surfaces. return ( - - - + messages={timelineMessages} + streamingMessageId={streamingMessageId} + sessionFeedbackSurvey={sessionFeedbackSurvey} + scrollTargetMessageId={scrollTargetMessageId} + scrollTargetQuery={scrollTargetQuery} + onScrollTargetHandled={onScrollTargetHandled} + searchContentRef={searchContentRef} + searchBackendRef={searchBackendRef} + showPlaceholder={showLoading} + placeholder={placeholder} + startContent={startContent} + footer={footer} + footerStatus={footerStatus} + rendererPolicy={rendererPolicy} + {...callbacks} + /> ); } diff --git a/src/features/chat/ui/__tests__/ArtifactViewer.openActions.test.tsx b/src/features/chat/ui/__tests__/ArtifactViewer.openActions.test.tsx index ef2775285..f88e65804 100644 --- a/src/features/chat/ui/__tests__/ArtifactViewer.openActions.test.tsx +++ b/src/features/chat/ui/__tests__/ArtifactViewer.openActions.test.tsx @@ -126,6 +126,43 @@ describe("ArtifactsWidget open actions through the real provider", () => { }); }); +describe("one policy owner per session boundary", () => { + it("shares the per-path open debounce across consumer surfaces", async () => { + // The viewer and the right-rail widget render under ONE provider (as in + // ChatView's chat row). Opening the same path quickly from both surfaces + // must hand it to the OS once: a second provider would own a separate + // debounce map and let the duplicate through. + const user = userEvent.setup(); + renderWithRealProvider( + <> + + + , + "/p/data.csv", + ); + + // Surface 1: the viewer's ⋯ menu. + await user.click(screen.getByRole("button", { name: /file actions/i })); + await user.click(screen.getByRole("menuitem", { name: /open in editor/i })); + await vi.waitFor(() => { + expect(mockOpenPath).toHaveBeenCalledTimes(1); + }); + + // Surface 2: the artifacts widget row, immediately after. + await user.click(screen.getByRole("button", { name: /data\.csv/i })); + // Give any (incorrect) second hand-off a chance to fire. + await new Promise((resolve) => setTimeout(resolve, 50)); + expect(mockOpenPath).toHaveBeenCalledTimes(1); + }); +}); + describe("openResolvedPath failure handling", () => { it("does not let a failed hand-off consume the retry debounce", async () => { const user = userEvent.setup(); diff --git a/src/features/home/widgets/ChatCanvasCard.test.tsx b/src/features/home/widgets/ChatCanvasCard.test.tsx index 186732601..ff6701efe 100644 --- a/src/features/home/widgets/ChatCanvasCard.test.tsx +++ b/src/features/home/widgets/ChatCanvasCard.test.tsx @@ -24,6 +24,25 @@ vi.mock("@/features/chat/capabilities/ConversationComposerCapability", () => ({ ConversationComposerCapability: mocks.composer, })); +// Structural marker so tests can assert provider boundaries: exactly one +// policy owner must cover both the transcript and the composer. +vi.mock("@/features/chat/hooks/ArtifactPolicyContext", () => ({ + ArtifactPolicyProvider: ({ + children, + sessionId, + }: { + children: ReactNode; + sessionId?: string | null; + }) => ( +
+ {children} +
+ ), +})); + vi.mock("@/features/chat/hooks/useChatTranscriptReadModel", () => ({ useChatTranscriptReadModel: () => ({ messages: mocks.messages, @@ -473,3 +492,35 @@ describe("ChatCanvasCard focus", () => { ).toBeInTheDocument(); }); }); + +describe("ChatCanvasCard artifact policy boundary", () => { + beforeEach(() => { + vi.clearAllMocks(); + mocks.messages = []; + mocks.isLoadingHistory = false; + mocks.chatState = "idle"; + }); + + it("mounts one provider covering both the transcript and the composer", () => { + renderCard(false); + + // Exactly one policy owner for the card's session boundary… + const providers = screen.getAllByTestId("artifact-policy-provider"); + expect(providers).toHaveLength(1); + expect(providers[0].dataset.sessionId).toBe("canvas-session"); + + // …and both consumer surfaces live inside it, sharing one artifact + // inventory and one per-path open debounce. + const provider = providers[0]; + expect( + screen + .getByTestId("transcript") + .closest("[data-testid='artifact-policy-provider']"), + ).toBe(provider); + expect( + screen + .getByTestId("canvas-composer") + .closest("[data-testid='artifact-policy-provider']"), + ).toBe(provider); + }); +}); diff --git a/src/features/home/widgets/ChatCanvasCard.tsx b/src/features/home/widgets/ChatCanvasCard.tsx index 710272ffd..5071ec63e 100644 --- a/src/features/home/widgets/ChatCanvasCard.tsx +++ b/src/features/home/widgets/ChatCanvasCard.tsx @@ -137,137 +137,139 @@ export function ChatCanvasCard({ }; return ( -
-
- {showActivity ? : null} - {project ? ( - - ) : null} -

{title}

+
+ {showActivity ? : null} + {project ? ( + + ) : null} +

+ {title} +

+
event.stopPropagation()} + > + + +
+
+ {/* biome-ignore lint/a11y/noStaticElementInteractions: transcript body click grants ephemeral canvas composer focus after the canvas gesture classifier. */} + {/* biome-ignore lint/a11y/useKeyWithClickEvents: keyboard users focus the composer directly; this handler classifies pointer click ownership only. */}
event.stopPropagation()} + onClick={focusAndMarkRead} > - - -
-
- {/* biome-ignore lint/a11y/noStaticElementInteractions: transcript body click grants ephemeral canvas composer focus after the canvas gesture classifier. */} - {/* biome-ignore lint/a11y/useKeyWithClickEvents: keyboard users focus the composer directly; this handler classifies pointer click ownership only. */} -
event.stopPropagation()} - onClick={focusAndMarkRead} - > - - {t("home:widgets.chatPin.earlierMessages")} - -
- ) : null - } - footerStatus={ - showActivity && !transcript.isLoadingHistory ? ( -
- - -
- ) : null - } - /> - - {/* biome-ignore lint/a11y/noStaticElementInteractions: this normal-flow surface classifies pointer ownership while its nested composer controls retain keyboard semantics. */} - {/* biome-ignore lint/a11y/useKeyWithClickEvents: keyboard activation is handled by the nested composer controls and focus capture. */} -
{ - event.stopPropagation(); - activateComposerFromPointer(); - }} - onClick={(event) => event.stopPropagation()} - onFocusCapture={activateComposerFromFocus} - > - {/* The composer is a sibling of ChatTranscriptSurface, so it sits - outside the transcript's own ArtifactPolicyProvider. Its @-file - mentions read the session artifact list from context; without a - provider here they silently see an empty list. Full messages (not - the bounded projection) so mentions cover the whole session. */} - {t("home:widgets.chatPin.earlierMessages")} + +
+ ) : null + } + footerStatus={ + showActivity && !transcript.isLoadingHistory ? ( +
+ + +
+ ) : null + } + /> + + {/* biome-ignore lint/a11y/noStaticElementInteractions: this normal-flow surface classifies pointer ownership while its nested composer controls retain keyboard semantics. */} + {/* biome-ignore lint/a11y/useKeyWithClickEvents: keyboard activation is handled by the nested composer controls and focus capture. */} +
{ + event.stopPropagation(); + activateComposerFromPointer(); + }} + onClick={(event) => event.stopPropagation()} + onFocusCapture={activateComposerFromFocus} > - -
-
+ + + ); } From 2eba344d5df16835193bc9598df510ee2b7cd705 Mon Sep 17 00:00:00 2001 From: Brandon Sherman Date: Thu, 3 Sep 2026 12:38:27 +1000 Subject: [PATCH 10/10] docs(chat): correct the provider-ownership comment in ChatView MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Category:** documentation (review follow-up, P3) **User Impact:** None at runtime; prevents a future maintainer from following stale prose and reintroducing nested providers. **Problem:** The comment beside ChatView's ArtifactPolicyProvider still said ChatTranscriptSurface renders its own provider — the opposite of the single-owner architecture the previous commit established. **Solution:** The comment now states that ChatView owns the single policy boundary and that ChatTranscriptSurface intentionally does not mount a provider (enforced by the provider-boundary tests). Prose audited for other stale ownership claims (rg): the ChatView test-file mock comment carried the same stale claim and is corrected in the same commit; no other hits. --- src/features/chat/ui/ChatView.tsx | 16 +++++++++------- .../chat/ui/__tests__/ChatView.mcpApp.test.tsx | 7 +++---- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/src/features/chat/ui/ChatView.tsx b/src/features/chat/ui/ChatView.tsx index 97fd3c6c6..c62da35b3 100644 --- a/src/features/chat/ui/ChatView.tsx +++ b/src/features/chat/ui/ChatView.tsx @@ -800,13 +800,15 @@ export function ChatView({ }); return ( - // The provider must wrap the whole chat row — not just the transcript — - // because siblings of the transcript consume the artifact context too: - // ArtifactViewerPanel ("Open in editor"), the right rail's - // ArtifactsWidget (row opens), and ArtifactAutoOpenMount (the artifact - // list). ChatTranscriptSurface renders its own provider for the - // transcript, but without this outer one its siblings get the inert - // default context and their actions silently no-op. + // The single artifact policy owner for this session boundary. It must + // wrap the whole chat row because the transcript AND its siblings + // consume the context: ArtifactViewerPanel ("Open in editor"), the + // right rail's ArtifactsWidget (row opens), and ArtifactAutoOpenMount + // (the artifact list). ChatTranscriptSurface intentionally does NOT own + // artifact policy — a nested provider would derive a second artifact + // inventory and split the per-path open debounce (enforced by the + // provider-boundary tests). Consumers outside the provider get the + // inert default context and silently no-op. // The identity, messages, and cwd must describe one session snapshot: // timelineSessionId (the controller's effective session) rather than the // raw requested sessionId, which can briefly disagree with diff --git a/src/features/chat/ui/__tests__/ChatView.mcpApp.test.tsx b/src/features/chat/ui/__tests__/ChatView.mcpApp.test.tsx index 7bb72c4f0..2e3f60078 100644 --- a/src/features/chat/ui/__tests__/ChatView.mcpApp.test.tsx +++ b/src/features/chat/ui/__tests__/ChatView.mcpApp.test.tsx @@ -279,10 +279,9 @@ vi.mock("@/features/terminal/ui/TerminalPanel", () => ({ // placement is load-bearing: see "keeps the artifact viewer panel inside the // artifact policy provider" below. The marker stamps the provider's props so // tests can also assert the enclosing provider received real data — nesting -// alone would pass even with e.g. messages={[]}. Note the marker renders for -// every provider (ChatView's outer one AND ChatTranscriptSurface's inner -// one), so assertions must use closest()/within(), never a singular -// getByTestId. +// alone would pass even with e.g. messages={[]}. ChatView owns the single +// provider for the chat row (ChatTranscriptSurface intentionally does not +// mount one), and the boundary test asserts exactly one marker renders. vi.mock("../../hooks/ArtifactPolicyContext", () => ({ ArtifactPolicyProvider: ({ children,