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 10 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.
**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.
**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.
|
🤖 Both blocking findings addressed, and screenshot evidence added below. P1 · Provider uses stale session identity → fixed in P1 · Deleted reveal can fail silently → fixed in Screenshots (deleted-file viewer, captured via the Playwright harness with the tauri-mock driven to a Before — PR head
After — current head: both OS hand-offs disabled:
After — error body for a deleted file: "File deleted from disk." strip with no dead "Open in editor" button:
All chat + home suites pass (3,210 tests); |
morgmart
left a comment
morgmart
left a comment
There was a problem hiding this comment.
🤖 Automated code review
REQUEST_CHANGES: the follow-up fixes both prior blocking issues, but the artifact viewer's layout still reads open state under the requested session while the panel now renders under the effective session, and multiple policy providers give one visible session separate artifact/open lifecycles. Supplied visual evidence is present. All captured GitHub check runs passed; combined status remained pending, so required checks still govern merge readiness.
Deterministic publication result: 1 blocking and 1 non-blocking inline finding(s) publishable; 0 duplicate(s) suppressed; 0 blocking screenshot-evidence requirement(s) in this review body.
| <ArtifactViewerPanel sessionId={sessionId} /> | ||
| // Keyed by the same effective identity the providers use, so the | ||
| // panel reads the viewer-store entry that openInApp writes. | ||
| <ArtifactViewerPanel sessionId={timelineSessionId} /> |
There was a problem hiding this comment.
🤖 P1 · Viewer layout reads stale session (blocking)
ArtifactViewerPanel now correctly reads timelineSessionId, but isArtifactViewerOpen is still derived near the top of ChatView with useOpenArtifact(sessionId). During the reconciliation state this follow-up explicitly supports, the panel and the layout calculation therefore read different artifact-store entries. Width allocation and compact-mode decisions can disagree with the viewer actually rendered.
User effect: While a chat is being restored or reconciled, an open artifact viewer can appear without reserved space and overlap or compress the chat, while a stale viewer can leave an empty reserved column.
Recommended fix: Derive the effective timeline session identity before the viewer-store lookup and use it consistently for useOpenArtifact, ArtifactViewerPanel, and every layout calculation driven by viewer visibility.
Test: Extend the reconciliation test so requested and effective session IDs differ, seed viewer state only for the effective session, and assert that the rendered chat layout enters its viewer-open configuration; also verify stale requested-session viewer state does not reserve space.
| // timelineSessionId (the controller's effective session) rather than the | ||
| // raw requested sessionId, which can briefly disagree with | ||
| // controller.messages during session replacement or reconciliation. | ||
| <ArtifactPolicyProvider |
There was a problem hiding this comment.
🤖 P2 · Artifact policy has duplicate owners (non-blocking)
The new ChatView provider wraps a ChatTranscriptSurface that still creates its own provider, and ChatCanvasCard similarly gives its transcript and composer separate providers. Each provider independently derives artifact inventory and owns its own per-path open debounce. The same visible session can therefore process different message projections and bypass duplicate-open suppression when actions originate from different surfaces.
User effect: Opening the same artifact quickly from two parts of one chat can hand it to the operating system twice, and artifact availability can drift between the transcript and adjacent viewer, rail, or composer surfaces.
Recommended fix: Mount one ArtifactPolicyProvider per rendered session boundary, placing transcript, composer, viewer, auto-open mount, and artifact widgets beneath it. Keep standalone transcript support through an explicit wrapper rather than implicit nested ownership.
Test: Add a provider-boundary test proving one policy owner covers all full-chat consumers and one covers both canvas transcript and composer; verify opening one path across two consumer surfaces shares the same debounce lifecycle.
**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.
**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.
|
🤖 Both round-2 findings addressed. P1 · Viewer layout reads stale session → fixed in P2 · Artifact policy has duplicate owners → taken in this PR rather than deferred, in
All chat + home suites pass (3,213 tests); |
morgmart
left a comment
morgmart
left a comment
There was a problem hiding this comment.
🤖 Automated code review
COMMENT: all four previously reported issues are fixed in the current comparison. One new non-blocking documentation finding remains: ChatView's changed ownership comment says ChatTranscriptSurface creates a provider even though this range removes it. Supplied visual evidence is present. GitHub evidence is structurally valid; several checks were still in progress at capture time, so required checks govern merge readiness.
Deterministic publication result: 0 blocking and 1 non-blocking inline finding(s) publishable; 0 duplicate(s) suppressed; 0 blocking screenshot-evidence requirement(s) in this review body.
| // 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 |
There was a problem hiding this comment.
🤖 P3 · Comment reverses provider ownership (non-blocking)
The changed ChatView comment says ChatTranscriptSurface renders its own ArtifactPolicyProvider, but this same comparison removes that provider and establishes the composing session surface as the single owner. The comment now documents the opposite of the corrected architecture beside the load-bearing provider boundary.
User effect: A future maintainer can follow the comment and reintroduce nested providers, bringing back inconsistent artifact inventory and duplicate open-action lifecycles across one chat.
Recommended fix: Update the comment to state that ChatTranscriptSurface intentionally does not own artifact policy and that ChatView provides the single shared boundary for the transcript and its sibling consumers.
Test: No runtime test is needed for prose alone; keep the existing provider-boundary tests that enforce one owner across the composed chat surface.
**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.
|
🤖 P3 addressed in |
morgmart
left a comment
morgmart
left a comment
There was a problem hiding this comment.
🤖 Automated code review
COMMENT: all five prior review findings are fixed in the current comparison. One new non-blocking lifecycle issue remains: the artifact-policy provider changes session identity without remounting, so provider-local debounce state can leak briefly across reconciled sessions. Supplied visual evidence is present. GitHub evidence is structurally valid; macOS Tauri checks and frontend checks were still in progress at capture time, so required checks govern merge readiness.
Deterministic publication result: 0 blocking and 1 non-blocking inline finding(s) publishable; 0 duplicate(s) suppressed; 0 blocking screenshot-evidence requirement(s) in this review body.
| // timelineSessionId (the controller's effective session) rather than the | ||
| // raw requested sessionId, which can briefly disagree with | ||
| // controller.messages during session replacement or reconciliation. | ||
| <ArtifactPolicyProvider |
There was a problem hiding this comment.
🤖 P2 · Provider state crosses session replacement (non-blocking)
The moved ArtifactPolicyProvider receives timelineSessionId, but React reuses the same provider instance when that prop changes. Provider-local refs, including the per-path open debounce map, therefore survive when reconciliation replaces the effective session without unmounting ChatView. A path opened in the previous session remains temporarily marked as recently opened in the replacement session.
User effect: If someone opens the same file path immediately after a chat session is replaced or reconciled, the first open in the new session can appear to do nothing because it is suppressed by the previous session's debounce state.
Recommended fix: Key the ArtifactPolicyProvider by timelineSessionId, or explicitly reset all session-scoped provider state when the session identity changes, so each effective session starts with an independent artifact-action lifecycle.
Test: Add a reconciliation test that opens a path, changes the effective session identity without unmounting ChatView, and verifies that opening the same path in the replacement session reaches the OS boundary immediately.
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.Seven 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).fix(chat)(review follow-up): the outer provider is keyed by the effective session. The outer provider received the raw requestedsessionIdwhile its messages/cwd came from the controller's effective session; during session replacement those can briefly disagree, letting artifact policy or viewer-store writes act under a stale identity. The provider,ArtifactAutoOpenMount, andArtifactViewerPanelnow all usetimelineSessionId, with a discriminating test where the requested and effective ids differ.fix(chat)(review follow-up): Reveal is also disabled for deleted files. Commit 5 kept Reveal enabled claiming file managers could show the containing folder — but the implementation passes the deleted file's path (not the parent) torevealItemInDirand swallows the rejection: another silent dead end. Both hand-offs now disable together and re-enable when polling sees the file reappear (new reappearance test; the deleted-state test verifies clicks reach neither OS boundary).Screenshots (deleted-file viewer states)
Captured via the Playwright harness (tauri-mock driven to the
missingstat state), viewer panel shown at 2x:90cfe94, as reviewed)(images attached in PR comments below)
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.