Repository navigation
fix(sidebar): stop a removed folder workspace from leaving a blank row - #26376
Conversation
|
Important Review skippedWe couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 Walkthrough
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: 🔵 Low · up to The change prevents deleted folder workspaces from reappearing as blank rows. A rejected runtime update may still leave stale lineage in a narrow race, and one test may not catch a host-specific write. Both are small follow-ups. 🚥 Pre-merge checks | ✅ 3 | ❌ 2
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:
8cf5fa85-be9a-49a1-a4b3-701d09099b0b
📥 Commits
Reviewing files that changed from the base of the PR and between 369b202 and d40d7831717bbc9b51d8428b7d2cac216f357cf4.
📒 Files selected for processing (7)
src/main/folder-workspace-meta-presence.tssrc/main/ipc/worktrees-create-metadata-persistence.test.tssrc/main/ipc/worktrees/metadata/register-worktree-metadata-handlers.tssrc/main/runtime/runtime-managed-worktree-metadata.test.tssrc/main/runtime/runtime-managed-worktree-metadata.tssrc/main/runtime/runtime-store-contract.tssrc/preload/api/worktree-api.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
| updates: { lastActivityAt: 2 } | ||
| }) | ||
|
|
||
| expect(table[workspaceId]).toBeUndefined() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that host-qualified writes are refused.
The shared table tracks setWorktreeMeta, but both host-qualified updates use setWorktreeMetaForHost. This assertion therefore passes even if the SSH update reaches its setter and recreates the metadata row in a real store. Assert that setWorktreeMetaForHost was not called in both removed-workspace cases, or back that setter with host-specific table state. (github.com)
| if (isRemovedFolderWorkspaceInstance(args.store, worktree.id, executionHostId)) { | ||
| throw new Error('selector_not_found') |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Check workspace presence before changing lineage.
If removal finishes after resolveWorktree, a worktree.set request with lineage.noParent or lineage.parentWorktree changes lineage before this check throws selector_not_found. The rejected request can leave stale lineage. Resolve the parent first when necessary, then check presence before any lineage mutation. Keep the check after the asynchronous resolution. (github.com)
A folder-project workspace exists only as its worktreeMeta row. When it is deleted while a terminal is still bound, the renderer keeps the row until its teardown runs, and a PTY reattach in that window stamps lastActivityAt through worktrees:updateMeta. setWorktreeMeta then minted a default row with an empty name, which the sidebar listed under the project name and which came back after every delete. The metadata IPC now refuses to create a row for a folder workspace instance that no longer exists. Fixes stablyai#22712 Claude-Session: https://claude.ai/code/session_01XZXEdbC4RKp288k2gBzLUW
Move the existence check into a shared predicate that matches any same-id folder owner, so an owner at a different path on another host cannot bypass it, and apply it to the runtime's worktree.set path, where a resolution snapshot taken before the removal still returns the workspace. Claude-Session: https://claude.ai/code/session_01XZXEdbC4RKp288k2gBzLUW
d40d783 to
b2c176a
Compare
|
Thank you @bbingz! This was a sharp diagnosis and a nicely contained fix, and the regression tests made it easy to confirm the bug on main. I rebased onto main and added one small test-only commit (a typed fixture parameter for a lint rule that landed after your last push). Merged. Much appreciated. |
ELI5
In a plain-folder project, deleting a workspace that still had a terminal open could leave a blank card in the sidebar, named after the project. Deleting that card did nothing. Now the workspace goes away and stays away.
What Changed
Before: in a folder (non-git) project, right-click a workspace with an open terminal and choose Delete Workspace. A blank card named after the project took its place. Deleting the card again left it there.
After: the workspace is removed and no card is left behind. Workspaces that still exist save their activity, unread state, and names exactly as before.
Mechanism: a workspace in a folder project has no checkout of its own. It exists only as its metadata record, and the sidebar lists every record whose id carries the
::workspace:<uuid>suffix. Deleting it removes that record in the main process. The renderer keeps the workspace in its own list until its teardown runs after the delete returns. If the terminal's PTY binds again in that window, the renderer stamps "last activity" on the workspace through theworktrees:updateMetaIPC. The store's write treated the unknown id as new and created a default record with an empty name. That empty record is the blank card, and the sidebar shows the project name in place of the empty name. A later delete removed it, but the next late write created it again.A new shared check in
folder-workspace-meta-presence.tsanswers one question: is this id a folder-workspace instance of any folder project with that id, and does it have no record on the host being written? Two write paths now use it:worktrees:updateMeta, the renderer's path, skips the write and returnsnull.worktree.set, the runtime path used by the CLI and paired clients, fails withselector_not_found, like any other missing workspace. This covers a write whose workspace lookup started before the delete and finished after it.Every other write is unchanged:
Both renderer callers ignore the IPC's return value, and the preload type now says it can be
null.Why
The record is the only thing that makes a folder workspace exist, so the safe rule is: a background update must never create one. I considered three places for the fix:
worktree.set. Together they cover every caller that writes on behalf of an open workspace.I chose option 3.
The issue also lists other state the deleted workspace leaves behind, such as stranded terminals and session tabs. Related issues track those, for example #15909 and #14474. This PR only fixes the blank card that cannot be removed.
Linked Issue
Fixes #22712
Visual Proof
N/A. The fix stops a sidebar row from appearing. It adds nothing to the screen. The new test reproduces the blank row through the real removal and listing handlers. I did not launch the app.
Testing
Verified on macOS. I did not launch the app, and I did not test on Windows, Linux, or SSH.
worktrees-create-metadata-persistence.test.ts, repro: a folder workspace is deleted throughworktrees:remove. Then a late activity write and a late host-qualified unread write arrive throughworktrees:updateMeta, andworktrees:listis checked. Without the fix, the list contains the deleted workspace again with an empty name, an empty comment, a new instance id, and no creation time. That matches the record in the issue. With the fix, the workspace stays gone.runtime-managed-worktree-metadata.test.ts: aworktree.setwhose lookup returned a deleted folder workspace fails withselector_not_foundand writes nothing. It fails without the fix. A write to a folder workspace that exists still lands.vitest runoversrc/main/ipc/worktrees/,src/main/ipc/worktrees-*.test.ts, the runtime metadata tests,first-work-folder-rename.test.ts,folder-workspace-pty-identity.test.ts, andorca-runtime-linked-issue-live-meta.integration.test.ts: 54 files and 490 tests pass.tsc --noEmitforconfig/tsconfig.node.jsonandconfig/tsconfig.tc.web.json,oxlint,oxfmt --check, andpnpm run check:code-quality:changed: all clean.AI Disclosure
Written with Claude Code (Claude Opus 5.5) and cross-reviewed with OpenAI Codex CLI.
Review
Cross-reviewed with OpenAI Codex CLI in two read-only passes. The first pass found two gaps, and both are fixed with tests:
worktree.setcould still write after a workspace lookup that began before the delete. It now runs the same check before writing.The second pass reported no actionable findings.
selector_not_founderror they already get for any missing workspace.Agent skill upstream boundary
Notes
Ensure no issues in: Security, Cross-platoform support (Linux, Windows, Mac), Remote SSH, Mobile, general backwards compatibility, performance
Mobile is not affected because the mobile app writes metadata through the runtime, not this IPC. Several open PRs edit the same handler, the preload type, or the runtime metadata update for unrelated features, for example #18676, #24956, #17251, #22723, and #26101. None of them changes how a missing folder-workspace record is handled. Their hunks sit next to this one, so rebasing them may need a trivial merge.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)https://claude.ai/code/session_01XZXEdbC4RKp288k2gBzLUW