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 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/ArtifactViewer.tsx b/src/features/chat/ui/ArtifactViewer.tsx index 60cf322a2..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,7 +602,14 @@ export function ArtifactViewer({ /> + {/* 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( () => {}, @@ -609,6 +620,7 @@ export function ArtifactViewer({ {t("artifactViewer.openExternally")} { void revealInFileManager(artifact.resolvedPath).catch( () => {}, @@ -694,6 +706,10 @@ export function ArtifactViewer({ { void openResolvedPath(artifact.resolvedPath).catch(() => {}); }} @@ -776,10 +792,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 +816,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/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/ChatView.tsx b/src/features/chat/ui/ChatView.tsx index 8df4f6359..c62da35b3 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, @@ -124,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); @@ -201,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 @@ -728,7 +736,6 @@ export function ChatView({ ); - const timelineSessionId = effectiveSession?.id ?? sessionId; const messageTimeline = ( + // 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 + // controller.messages during session replacement or reconciliation. + @@ -943,7 +967,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} - + ); } 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..f88e65804 --- /dev/null +++ b/src/features/chat/ui/__tests__/ArtifactViewer.openActions.test.tsx @@ -0,0 +1,196 @@ +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("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(); + 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); + }); + }); +}); diff --git a/src/features/chat/ui/__tests__/ArtifactViewer.test.tsx b/src/features/chat/ui/__tests__/ArtifactViewer.test.tsx index fd1fb67bc..a68a8f880 100644 --- a/src/features/chat/ui/__tests__/ArtifactViewer.test.tsx +++ b/src/features/chat/ui/__tests__/ArtifactViewer.test.tsx @@ -668,6 +668,108 @@ 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 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({ + 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, + }); + const revealItem = screen.getByRole("menuitem", { name: /reveal in/i }); + expect(openItem).toHaveAttribute("aria-disabled", "true"); + expect(revealItem).toHaveAttribute("aria-disabled", "true"); + + // 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 () => { await renderLoadedViewer(); diff --git a/src/features/chat/ui/__tests__/ChatView.mcpApp.test.tsx b/src/features/chat/ui/__tests__/ChatView.mcpApp.test.tsx index 6d8701366..2e3f60078 100644 --- a/src/features/chat/ui/__tests__/ChatView.mcpApp.test.tsx +++ b/src/features/chat/ui/__tests__/ChatView.mcpApp.test.tsx @@ -15,6 +15,7 @@ import { } from "@/app/contexts/TopBarActionsContext"; import { TERMINAL_FALLBACK_CWD_STORAGE_KEY } from "@/features/terminal/lib/terminalCwdPreference"; import type { ChatSession } from "../../stores/chatSessionStore"; +import { useArtifactViewerStore } from "../../stores/artifactViewerStore"; import { useSecurityConfirmationStore } from "@/features/security/stores/securityConfirmationStore"; import { DEFAULT_RUNTIME_CONFIG } from "@/shared/runtime-config/schema"; import { useRuntimeConfigStore } from "@/shared/runtime-config/runtimeConfigStore"; @@ -272,11 +273,50 @@ 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={[]}. 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 }: { 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,135 @@ 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); + // 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", () => { + // 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("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"); 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 a5dd04e21..5071ec63e 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"; @@ -136,135 +137,148 @@ 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} - > - -
- + {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} + > + +
+ + ); }