Repository navigation
fix(editor): keep an unresolvable mirrored file tab open with a truthful terminal state - #21375
Conversation
…ful terminal state A host-mirrored file whose read keeps answering `selector_not_found` used to sit on the raw code forever (and, in the reverted #21363, was closed outright, discarding drafts). `selector_not_found` is the host's "could not resolve right now", not proof the workspace is gone, and the file-read path has no definitive absence code. Bound the retries as before, then swap in a truthful terminal message with Retry and Close tab. The tab is never closed automatically; Close routes through the unsaved-changes queue so a dirty draft is confirmed. Fixes #21041
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — two rough edges inline, plus one open question about whether Close sticks.
Reviewed changes
- Bounded
selector_not_foundretry ends in a truthful terminal state.useEditorPanelFileLoadRetrynow swaps the raw resolver code forWORKTREE_HOST_UNRESOLVED_ERRORafter the existing250/1000/2500backoff, andshouldRetryFileLoadErrormarks that message terminal. No close path is added, so the tab and its draft survive. EditorFileLoadErrorViewgains an optionalClose tabaction. A ghost button besideRetry, rendered only when the caller suppliesonClose, with a new catalog entry.- All three error-view call sites wire
onClosethroughrequestEditorFileClose. That routes into the existing unsaved-changes close queue, so a dirty tab is confirmed before it closes. - New coverage. Component-level tests for the two button behaviors, hook-level tests for the classification and terminal transition, and an integration test through
useEditorPanelContentStateasserting nocloseFileand a terminal message on repeatedselector_not_found.
This is the right direction: selector_not_found is UNKNOWN rather than absence (remote-browser-stream-errors.ts, web-runtime-session-tab-lifecycle.ts), the file-read path has no definitive "gone" code, and closeFile deletes editorDrafts with no dirty check — so auto-eviction was the data-loss path the #21368 revert called out. Keeping the tab and offering a dirty-checked Close is the safe shape.
ℹ️ Manual Close may not stick for the same transient state
Close tab routes through requestEditorFileClose → closeFile → notifyHostOfMirroredEditorClose → closeWebRuntimeSessionTab({ reason: 'user' }). When the host answers selector_not_found, web-runtime-session-tab-lifecycle.ts treats it as a non-definitive failure and clears the close intent — only tab_not_found / terminal_tab_not_found are made durable (#21277), and web-runtime-session-tab-activate-close.test.ts pins that selector_not_found suppression expires. If the host republishes its copy, the mirror can reappear after the close-intent TTL. Since this PR makes Close the user's only way out of the unresolved state, it's worth confirming that closing is expected to stick here, or noting it as a known follow-up.
Technical details
# Manual close durability for selector_not_found
## Affected sites
- `src/renderer/src/runtime/web-runtime-session-tab-lifecycle.ts:171-180` — only
`tab_not_found` / `terminal_tab_not_found` become durable; every other failure
calls `clearWebSessionCloseIntent`.
- `src/renderer/src/runtime/web-runtime-session-tab-activate-close.test.ts:318-345` —
pins that a `selector_not_found` close intent does not survive the TTL.
- `src/renderer/src/runtime/close-mirrored-editor-tab.ts:43-58` — records the intent
synchronously, then fires `closeWebRuntimeSessionTab`.
## Open questions for the human
- Is a manual close of a mirrored file tab expected to be durable across a
`selector_not_found` host answer, or is that deliberately left to the
host-resolvable follow-up in the PR notes?DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe editor now preserves runtime error codes during file loading and uses them to bound Priority: ⬆️ High Severity of issue fixed: High Merge Risk: ⚪ Minimal · up to The unresolved-workspace state now displays localized messaging and provides the intended retry and close actions. No actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 30.77% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 10 files. (1 skipped: 1 unsupported.) Full details: Description checkExplanation The description includes the required sections, linked issue, testing details, and rationale. However, it states that no close path or Close action exists, while the changes add an onClose handler, a Close tab action, related localization, and tests. It also marks visual proof as not applicable despite adding a user interaction. Resolution Update the description to match the implemented Close tab behavior and the PR objectives. Explain its unsaved-changes handling and why inline conflict-review rows omit the action. Add the required before/after visual proof, or provide a precise justification that covers the new interaction.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 2e781d63-6ef2-4637-a524-bba6666003d1
📒 Files selected for processing (10)
src/renderer/src/components/editor/EditorConflictReviewSurface.tsxsrc/renderer/src/components/editor/EditorContent.tsxsrc/renderer/src/components/editor/EditorEditFileSurface.tsxsrc/renderer/src/components/editor/EditorFileLoadErrorView.test.tsxsrc/renderer/src/components/editor/EditorFileLoadErrorView.tsxsrc/renderer/src/components/editor/editor-panel-content-types.tssrc/renderer/src/components/editor/useEditorPanelFileLoadRetry.test.tsxsrc/renderer/src/components/editor/useEditorPanelFileLoadRetry.tssrc/renderer/src/components/editor/useEditorPanelHostUnresolvedContentState.test.tsxsrc/renderer/src/i18n/locales/en.json
Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review.
| export const WORKTREE_HOST_UNRESOLVED_ERROR = | ||
| "The host couldn't find this file's workspace. It may have been removed, or the host may still be scanning. Retry, or close the tab." |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Localize the terminal workspace message.
This new user-facing message always renders in English. Add an intent-named localization ID. Translate the message in the error view when loadError equals WORKTREE_HOST_UNRESOLVED_ERROR.
Based on learnings, newly added user-facing TypeScript strings must use intent-named localization IDs.
Source: Learnings
Preserve `RuntimeRpcCallError.code` on `FileContent.loadErrorCode` and gate
the host-unresolved terminal transition with `hasRuntimeRpcErrorCode`, so a
host that sends `{ code: 'selector_not_found', message: 'Selector not found' }`
reaches the same truthful state as one that puts the bare token on the
message. Also drop the Close action on inline conflict-review rows, which
are not open tabs and would have been a dead control.
…he token matcher Separate the terminal state's comparison key from its display text: the retry hook stores `WORKTREE_HOST_UNRESOLVED_CODE` on `loadErrorCode`, and the error view localizes by that code (`editor.fileLoad.hostUnresolved`), so translating the message can never break the terminal check. Export the selector_not_found matcher and cover near misses (case, suffix, prose, wrong code) so only the defined token classifies.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Reviewed the delta since the prior pullfrog review (48e6028), which lands 4943c88.
- Classified
selector_not_foundby machine code, not message text.FileContentgained an optionalloadErrorCode; the file-content loader preservesRuntimeRpcCallError.code, andisHostSelectorNotFoundErrornow matches through the sharedhasRuntimeRpcErrorCodematcher over{code, message}. This closes the exact-message-compare gap where a host sending{code:'selector_not_found', message:'Selector not found'}would have stayed on raw text. - Gated the conflict-review
Close tabon the file being a real open tab.EditorConflictReviewSurfacenow passesonCloseonly whenopenFilescontainscontentFile.id, so synthesized inline overview rows no longer render a dead control. Verified that real conflict-review files are added toopenFileswithid === absolutePath, so genuine tabs still get Close. - Extended coverage to both host shapes. The retry test now parameterizes the bare-code-as-message and code-with-prose-message shapes, and the integration test rejects with a real
RuntimeRpcCallErrorand asserts bothloadErrorandloadErrorCode.
Both prior inline threads are addressed and resolved. The manual-close durability open question remains a documented follow-up, not a blocker. Focused editor tests (10), neighbouring suites (27), and pnpm tc:web all pass locally.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
ℹ️ No critical issues — one minor doc nit inline.
Reviewed changes
Reviewed the delta since the prior pullfrog review (4943c88), which lands 4208583.
- Keyed the terminal state and its display copy on a client sentinel. Added
WORKTREE_HOST_UNRESOLVED_CODE('worktree_host_unresolved'), stored onloadErrorCodeat the terminal transition;shouldRetryFileLoadErrornow tests that code via a new optionalcodeparameter rather than the English message, so a localized or reworded message stays terminal. - Localized the host-unresolved copy in the error view.
EditorFileLoadErrorViewtakes an optionalcodeand renders the translatededitor.fileLoad.hostUnresolvedstring when it matches the sentinel; all three call sites passfileContent.loadErrorCode. - Pinned the token matcher. Exported
isHostSelectorNotFoundErrorand added tests for the exact token plus near-misses (case variants, suffixes, an embedded token), and asserted the sentinel lands onloadErrorCodein both hook and integration tests.
Verified locally: focused and repro suites (17 tests) pass, pnpm tc:web is clean, and pnpm verify:localization-catalog passes.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| mimeType?: string | ||
| fileIdentity?: string | ||
| loadError?: string | ||
| /** Machine code from a runtime RPC failure; `loadError` alone may be prose (#21041). */ |
There was a problem hiding this comment.
This field now holds both host RPC codes and the client WORKTREE_HOST_UNRESOLVED_CODE sentinel written by the retry hook, so the "from a runtime RPC failure" clause no longer covers everything a reader will find here. Worth widening the comment so a future classifier does not assume every value is a host code.
| /** Machine code from a runtime RPC failure; `loadError` alone may be prose (#21041). */ | |
| /** Host RPC code, or the client `WORKTREE_HOST_UNRESOLVED_CODE` sentinel; `loadError` alone may be prose (#21041). */ |
… tab strip The Close button routed through `requestEditorFileClose`, which skips the pinned-tab and shared-reference checks the tab strip applies, has no listener outside the Terminal workbench (floating editor panels), and on the conflict-review overview could target an unrelated open tab whose id is the same absolute path as a synthesized inline row. Rather than reimplement the tab strip's close semantics in a second place, the error view keeps Retry and its copy points the user at closing the tab.
The copy no longer points at a Close control that is gone ("close this
tab from the tab strip") and no longer claims a scan is in progress, since
`selector_not_found` is also thrown synchronously for unregistered folder
workspaces and removed repos. The sentinel becomes
`editor_host_workspace_unresolved` so it cannot be confused with the CLI's
`worktree_host_unresolved` client error. The doc comment narrows the
"no definitive absence code" claim to git worktrees and names the two host
codes that are definitive but not yet classified.
Refs #21041
There was a problem hiding this comment.
ℹ️ No new issues in the new commits — the delta removes the in-view Close cleanly. One prior doc nit remains open inline, so this review stays non-approving.
Reviewed changes
Reviewed the delta since the prior pullfrog review (4208583) and its test-only follow-up (9458eb6), which lands 9229b8b.
- Removed the
Close tabaction fromEditorFileLoadErrorView. The view is now Retry-only; closing stays with the tab strip / Cmd+W, whose path carries the pin, shared-reference, and unsaved-changes semantics the inline control would have had to reimplement. - Dropped the
onClosewiring and imports from all three call sites (EditorContent,EditorEditFileSurface,EditorConflictReviewSurface), which also removes the conflict-surfaceopenFilesgating that existed only to suppress a dead control. - Retired the now-unused
Close tabcatalog entry (296b59cd29) and rewrote the component test to pin Retry as the sole action. - Verified locally: focused editor suites (11 tests) pass,
pnpm tc:webandpnpm verify:localization-catalogpass, andoxlintreports 0 findings on the five touched files. No danglingonClose/requestEditorFileClosereferences remain in the editor error path.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| mimeType?: string | ||
| fileIdentity?: string | ||
| loadError?: string | ||
| /** Machine code from a runtime RPC failure; `loadError` alone may be prose (#21041). */ |
There was a problem hiding this comment.
This is the still-open doc nit from the prior review: the field now holds host RPC codes and the client WORKTREE_HOST_UNRESOLVED_CODE sentinel written by the retry hook, so "from a runtime RPC failure" no longer covers every value a reader will find here. Widening it keeps a future classifier from assuming every value is a host code.
| /** Machine code from a runtime RPC failure; `loadError` alone may be prose (#21041). */ | |
| /** Host RPC code, or the client `WORKTREE_HOST_UNRESOLVED_CODE` sentinel; `loadError` alone may be prose (#21041). */ |
…ful terminal state (#21375) * fix(editor): keep an unresolvable mirrored file tab open with a truthful terminal state A host-mirrored file whose read keeps answering `selector_not_found` used to sit on the raw code forever (and, in the reverted #21363, was closed outright, discarding drafts). `selector_not_found` is the host's "could not resolve right now", not proof the workspace is gone, and the file-read path has no definitive absence code. Bound the retries as before, then swap in a truthful terminal message with Retry and Close tab. The tab is never closed automatically; Close routes through the unsaved-changes queue so a dirty draft is confirmed. Fixes #21041 * fix(editor): classify selector_not_found by RPC code, not message text Preserve `RuntimeRpcCallError.code` on `FileContent.loadErrorCode` and gate the host-unresolved terminal transition with `hasRuntimeRpcErrorCode`, so a host that sends `{ code: 'selector_not_found', message: 'Selector not found' }` reaches the same truthful state as one that puts the bare token on the message. Also drop the Close action on inline conflict-review rows, which are not open tabs and would have been a dead control. * fix(editor): localize the host-unresolved copy by sentinel, and pin the token matcher Separate the terminal state's comparison key from its display text: the retry hook stores `WORKTREE_HOST_UNRESOLVED_CODE` on `loadErrorCode`, and the error view localizes by that code (`editor.fileLoad.hostUnresolved`), so translating the message can never break the terminal check. Export the selector_not_found matcher and cover near misses (case, suffix, prose, wrong code) so only the defined token classifies. * test(editor): name the it.each parameter for the host answer it labels * fix(editor): drop the load-error Close action; closing stays with the tab strip The Close button routed through `requestEditorFileClose`, which skips the pinned-tab and shared-reference checks the tab strip applies, has no listener outside the Terminal workbench (floating editor panels), and on the conflict-review overview could target an unrelated open tab whose id is the same absolute path as a synthesized inline row. Rather than reimplement the tab strip's close semantics in a second place, the error view keeps Retry and its copy points the user at closing the tab. * fix(editor): reword the host-unresolved copy and namespace its sentinel The copy no longer points at a Close control that is gone ("close this tab from the tab strip") and no longer claims a scan is in progress, since `selector_not_found` is also thrown synchronously for unregistered folder workspaces and removed repos. The sentinel becomes `editor_host_workspace_unresolved` so it cannot be confused with the CLI's `worktree_host_unresolved` client error. The doc comment narrows the "no definitive absence code" claim to git worktrees and names the two host codes that are definitive but not yet classified. Refs #21041 (cherry picked from commit 2cc34de)
ELI5
When a remote-session markdown tab could not be read because the host said
selector_not_found, the tab sat on that raw code forever, and the reverted #21363 went the other way and closed it — throwing away unsaved edits. Now the tab retries a few times, then says plainly that the host could not find the file's workspace, with Retry; closing stays with the tab strip's own close control. It never closes on its own.What this PR delivers is deliberately small: a truthful, localized terminal message instead of a raw error string, and an end to pointless retries. It adds no close path and does not change how the host nominates or mirrors tabs, so the issue's three reported symptoms (tab click switching to the broken
.md, a new terminal switching to it, tabs flashing open/closed) are not addressed here — see Notes.What Changed
useEditorPanelFileLoadRetry: after the existing bounded backoff (250/1000/2500 ms), aselector_not_foundread is replaced with a truthful terminal message (WORKTREE_HOST_UNRESOLVED_ERROR) instead of staying on the raw code. Same shape as the owner-not-ready → unreachable transition from [Bug]: Remote host attempting to view a file results in error #6648. No close path is added.FileContent.loadErrorCodepreservesRuntimeRpcCallError.code; the terminal transition classifies via the sharedhasRuntimeRpcErrorCodematcher, so{ code: 'selector_not_found', message: 'Selector not found' }reaches the same state as the bare token.WORKTREE_HOST_UNRESOLVED_CODEonloadErrorCode);EditorFileLoadErrorViewlocalizes the copy by that code (editor.fileLoad.hostUnresolved), so translating the message can never break the terminal check.Why
selector_not_foundis the host resolver's "could not place this workspace right now". It covers a deleted worktree and a cold or failing scan, and the codebase already documents it as UNKNOWN in two places (remote-browser-stream-errors.ts,web-runtime-session-tab-lifecycle.ts). For git worktrees, the file-read path (files.read→resolveRuntimeFileTarget→resolveWorktreeSelector) has no definitive absence code:tab_not_found/terminal_tab_not_foundare only emitted by the session-tab methods, which short-circuit on the explicitid:selector and never consult the resolver — which is exactly why the host keeps publishing (and re-nominating as active) a markdown tab whose file read cannot resolve. Without positive absence evidence the safe direction is keep, never close (docs/reference/ssh-execution-boundary.md), andcloseFiledeleteseditorDraftswith no dirty check, so an automatic close is a data-loss path.Linked Issue
Refs #21041 — narrows the symptom (raw
selector_not_foundtext with endless retries) but does not resolve the issue's reported tab-switching or flashing; do not auto-close.Visual Proof
N/A — error-state copy and a secondary button inside the existing load-error card; no new layout.
Testing
pnpm test src/renderer/src/components/editor/useEditorPanelFileLoadRetry.test.tsx src/renderer/src/components/editor/useEditorPanelHostUnresolvedContentState.test.tsx src/renderer/src/components/editor/EditorFileLoadErrorView.test.tsx— 9 passeduseEditorPanelContentState,useEditorPanelVisibilityContentState,EditorContent,EditorContent.markdown-classification) — 50 passedpnpm tc:web— cleanORCA_CODE_QUALITY_BASE=origin/main node config/scripts/check-changed-code-quality.mjs— passed (0 findings)pnpm verify:localization-catalog— passedcloseFileeviction re-injected on top of the fix): the integration test fails onexpect(closeFile).not.toHaveBeenCalled()(called 1×) and the hook test fails on the terminal message.AI Disclosure
Review
Self-reviewed for correctness, data safety (no automatic close and no new close path of any kind — the tab strip remains the only one), SSH/remote behavior (keep-on-unknown), and cross-platform impact (renderer only).
Agent skill upstream boundary
Notes
selector_not_foundis classified through the sharedhasRuntimeRpcErrorCodematcher, which is case-exact but deliberately tolerates surrounding whitespace (so transport-wrapped"…: selector_not_found\n"classifies). Do not tighten or fork it here: since nothing destructive hangs off the match any more, an over-match costs a slightly-wrong message, never a lost draft. That cheap failure direction is intentional.On the copy "Retry, or close the tab": for a host-mirrored tab, closing it via the tab strip also closes the host's copy (
closeFile→notifyHostOfMirroredEditorClose). That is the pre-existing contract of every mirrored-tab close path and this PR adds no close path, so the message does not restate it; the same message also shows for non-mirrored files read through the runtime, where it would be wrong.Not everything that escapes
files.readis UNKNOWN. Two host codes are definitive and are uncovered here rather than misclassified:folder_workspace_path_missing:<abs path>(folder workspace whose directory is gone) andworktree_execution_host_unresolved(ambiguous host routing). They do not match theselector_not_foundmatcher, so those tabs still sit on their raw string after the retry budget. Classifying them into their own truthful terminal states is a follow-up, not this PR. Also noteselector_not_foundis thrown synchronously for an unregistered folder-workspace id or a removed repo, so the copy says "may not know about it yet" rather than claiming a scan is in progress.Pre-existing gap, out of scope, flagged for a follow-up:
buildMirroredEditorTabs(web-session-tabs-sync/tab-builders.ts) spreads the existing localOpenFileand then overwritesisDirtywith the host snapshot's value, and the client does not forward local dirtiness to the host. A rebuild can therefore flip a locally-dirty mirrored file toisDirty: falsewhileeditorDraftsstill holds the draft, and the tab strip's dirty gate (file.isDirty) would then close it without confirmation. This PR neither introduces nor closes that path (its own guarantee is that nothing here closes a tab, which mutation B proves), but it does mean the tab strip's dirty confirmation is not a reliable draft guard for exactly the mirrored tabs this message targets; the fix belongs in the rebuild (isDirty: existingFile?.isDirty === true || tab.isDirty, or gating oneditorDrafts) and should get its own test.Follow-up, not in this PR: the host could make this state genuinely resolvable —
files.readcould consult the session-tab store's known workspace path (a filesystemENOENTis then a real absence signal the client already treats as terminal), or emit a definitive worktree-absence code only when the catalog positively lacks the worktree. Until one of those exists, auto-eviction on the file-read path cannot be made safe.Checklist