fix(chat): restore artifact open actions ("Open in editor") broken by provider scoping - #267
fix(chat): restore artifact open actions ("Open in editor") broken by provider scoping#267badideasforsale wants to merge 5 commits into
Conversation
**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.
**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.
**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.
**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.
b53957e to
1244c8b
Compare
|
🤖 Rebased onto main ( |
**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.
morgmart
left a comment
morgmart
left a comment
There was a problem hiding this comment.
🤖 Automated code review
REQUEST_CHANGES: the new outer artifact policy can apply filesystem behavior using the wrong session identity during reconciliation, and the deleted-file menu deliberately retains a Reveal action whose deleted target may fail silently. This PR changes graphical artifact-viewer states but the supplied PR evidence contains no screenshots or recording; please add screenshots or a short screen recording. The supplied GitHub evidence is structurally valid: all captured check runs passed, while combined status remained pending, so required checks still govern merge readiness.
Deterministic publication result: 2 blocking and 0 non-blocking inline finding(s) publishable; 0 duplicate(s) suppressed; 1 blocking screenshot-evidence requirement(s) in this review body.
🤖 Blocking · Screenshots needed
This PR changes Berd’s graphical interface. Please add screenshots or a short screen recording so the visual result can be reviewed. Screenshots are review evidence; they do not replace accessibility, responsive, theme, localization, or behavior validation.
| <ArtifactPolicyProvider | ||
| messages={controller.messages} | ||
| sessionCwd={controller.sessionArtifactCwd} | ||
| sessionId={sessionId} |
There was a problem hiding this comment.
🤖 P1 · Provider uses stale session identity (blocking)
ChatView derives timelineSessionId from effectiveSession?.id ?? sessionId and gives that identity to the rendered transcript, but the newly added outer ArtifactPolicyProvider receives the raw requested sessionId while receiving controller messages and cwd. During session replacement or reconciliation, its identity can disagree with the session snapshot it governs. This provider owns artifact behavior for the viewer, rail, and auto-open subtree, including remote/local policy and viewer-store identity.
User effect: While a chat is being restored or reconciled, a remote artifact can incorrectly expose a local file action, a valid local action can be blocked, or an artifact can open under the obsolete chat session.
Recommended fix: Pass timelineSessionId, or the same effective-session identity used by the timeline, to the outer ArtifactPolicyProvider so its ID, messages, cwd, and remote/local policy describe one session snapshot.
Test: Add a discriminating ChatView test where the requested sessionId differs from controller.session.id and assert that the enclosing artifact policy provider receives the effective controller session ID; the current matching-ID test does not catch this case.
| <DropdownMenuContent align="end"> | ||
| {/* 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 |
There was a problem hiding this comment.
🤖 P1 · Deleted reveal can fail silently (blocking)
The changed code explicitly keeps Reveal enabled after the artifact is known to be missing, asserting that file managers can show the containing folder. The action still passes the deleted artifact path to revealItemInDir rather than targeting an existing parent directory, and its rejection is swallowed. The implementation therefore does not establish the recovery behavior promised by the new state decision.
User effect: After Berd reports that a file was deleted, the remaining enabled Reveal action can appear to do nothing, leaving another silent dead end beside the action this PR removes.
Recommended fix: For a missing artifact, either disable or hide Reveal, or implement an explicit containing-folder action that targets an existing parent directory and uses an accurate label.
Test: Add a discriminating missing-file test that selects the remaining action and verifies either that it is unavailable or that the file-manager boundary receives the existing parent-directory path. Do not only assert the absence of aria-disabled.
2 participants
Summary
"Open in editor" in the artifact viewer's ⋯ menu did nothing — a silent no-op. This PR fixes the root cause and four adjacent defects in the same failure family, found during pre-review and dogfooding of the fix.
Root cause: #178 moved
ArtifactPolicyProviderfromChatViewinto the extractedChatTranscriptSurface. The transcript kept its provider, but sibling consumers in the chat row —ArtifactViewerPanel("Open in editor"), the right rail'sArtifactsWidget(row opens), andArtifactAutoOpenMount(artifact auto-open) — fell outside it and received the inert default context, whoseopenResolvedPathis an async no-op and whose artifact list is empty. Call sites swallow errors (.catch(() => {})), so nothing surfaced anywhere.Five commits, each independently revertable:
fix(chat): outerArtifactPolicyProvideraround ChatView's chat row. Restores context for the viewer panel, right-rail artifacts widget, and auto-open. The transcript's inner provider stays (Home canvas cards rely on it); nesting is benign — actions are memoized and artifact collection is signature-cached. The outer provider intentionally receives ungatedcontroller.messages: the auto-open hook baselines whatever is present while history loads and on its first settled pass, so history artifacts cannot pop the viewer — and baselining the real list is safer than the paint-gated (transiently empty) one.fix(tauri): session-window opener capability. Popped-outsession:*windows only allowedopener:allow-open-pathfor$DOWNLOAD, so even with the provider fixed, "Open in editor" was rejected by capability scope in popped-out windows — same silent symptom. The scope now mirrors the main window's.fix(home): canvas card composer provider. Same add interactive chats to the Home canvas #178 family:CanvasCardComposeris a sibling ofChatTranscriptSurface, so its @-file mentions silently read an empty session-artifact list. It now gets its own provider fed the full session messages.fix(chat): failed opens no longer consume the retry debounce.openResolvedPathrecorded its 1200 ms dedupe timestamp before awaiting the opener, so a rejected hand-off blocked an immediate retry.fix(chat): no "Open in editor" for deleted files. The viewer already knows when the file is gone (the "File deleted from disk." strip), yet the error body still offered an "Open in editor" button and the ⋯ menu kept the action enabled — guaranteed dead clicks on a nonexistent file. The error-body button now hides and the menu item disables when the divergence kind ismissing(the same signal that already hides the strip's Reload button); "Reveal in Finder" stays available and the actions return if polling sees the file reappear.Known follow-ups deliberately not in this PR (happy to file issues):
.catch(() => {})— that swallowing is what made both the add interactive chats to the Home canvas #178 regression and the capability gap invisible.AutomationBuilderViewrendersMessageTimeline/ChatInputwith no artifact provider (artifact links/opens inert there); pre-existing, out of scope here.Related issue
None found for this exact bug. Closest: #171 (Files panel opens Markdown externally instead of the viewer) — adjacent surface, different defect; not addressed here.
Testing
ArtifactPolicyProvidertest mock now stamps its props on a structural marker; a ChatView test asserts the viewer panel mounts inside a provider that received real session data. Fails against the unfixed ChatView (verified both ways via stash).ArtifactViewer.openActions.test.tsx) render the real provider with only the Tauri boundary mocked: viewer "Open in editor" and right-rail artifact row both reachopenPath; a failed open doesn't consume the retry debounce (also verified to fail pre-fix). The provider-scope bug class is invisible to tests that mock the context, so these keep the real wiring covered.just checkclean;just tauri-checkclean (capability schema regenerated); full chat + home suites: 3,116 tests pass.Two independent adversarial review passes were run on this branch before opening; findings from both (session-window capability gap, canvas composer orphan, debounce lockout, test-mock drift risks, commit-message inaccuracies) are incorporated.