fix: require explicit workspace group close - #3206
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe ChangesWorkspace group closure
Application event compatibility
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The PR changes workspace group-close behavior and the protocol contract, but no actionable merge-blocking risk remains at the current head. It is merge-ready after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant CLI
participant Runtime
participant WorkspaceAPI
participant WorkspaceState
participant EventStream
CLI->>Runtime: workspace ID and optional --group
Runtime->>WorkspaceAPI: WorkspaceCloseParams
WorkspaceAPI->>WorkspaceState: validate and select worktree group
WorkspaceState-->>WorkspaceAPI: selected workspace snapshots
WorkspaceAPI->>WorkspaceState: remove group workspaces
WorkspaceAPI->>EventStream: emit WorkspaceClosed for each workspace
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c8acc22e-5a53-4cb7-a917-45f0cb4db9b5
📒 Files selected for processing (23)
docs/next/CHANGELOG.mddocs/next/api/herdr-api.schema.jsondocs/next/website/src/content/docs/cli-reference.mdxdocs/next/website/src/content/docs/ja/cli-reference.mdxdocs/next/website/src/content/docs/ja/socket-api.mdxdocs/next/website/src/content/docs/socket-api.mdxdocs/next/website/src/content/docs/zh-cn/cli-reference.mdxdocs/next/website/src/content/docs/zh-cn/socket-api.mdxsrc/api/schema.rssrc/api/schema/tests.rssrc/api/schema/workspaces.rssrc/app/actions.rssrc/app/api/workspaces.rssrc/app/input/modal.rssrc/app/input/navigate.rssrc/app/runtime_mutations.rssrc/cli/runtime.rssrc/cli/workspace.rssrc/protocol/wire.rstests/api_ping.rstests/cli/sessions.rstests/cli/workspace.rstests/support/mod.rs
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.
812c14a to
abcf10b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ed68b7c9-d22b-4096-8aac-58096bc86312
📒 Files selected for processing (3)
docs/next/website/src/content/docs/ja/socket-api.mdxdocs/next/website/src/content/docs/socket-api.mdxdocs/next/website/src/content/docs/zh-cn/socket-api.mdx
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.
Greptile SummaryThis PR makes closure of a primary workspace with open linked-worktree workspaces require explicit group intent.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/app/api/workspaces.rs | Enforces explicit group-close intent, snapshots group members before mutation, performs cleanup, and publishes final closure events. |
| src/app/actions.rs | Centralizes group membership selection and safely removes potentially noncontiguous members in descending index order. |
| src/app/input/modal.rs | Routes confirmed context-menu and modal group closures through the public API mutation path. |
| src/app/input/navigate.rs | Preserves confirmation behavior for workspace and last-tab actions while explicitly requesting group closure when appropriate. |
| src/api/schema/workspaces.rs | Adds the backward-decodable close_group request field with an omitted-value default of false. |
| src/cli/workspace.rs | Adds the documented workspace close <id> --group command form and forwards explicit group intent. |
| src/protocol/wire.rs | Increments the exact-match client/server protocol constant from 20 to 21. |
| docs/next/api/herdr-api.schema.json | Publishes the protocol increment and updated workspace.close request schema. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[workspace.close request] --> B{Primary with linked workspaces?}
B -->|No| C[Close target workspace]
B -->|Yes| D{Explicit close_group?}
D -->|No| E[Return workspace_group_close_required]
D -->|Yes| F[Snapshot every group member]
F --> G[Remove group members in reverse index order]
G --> H[Shut down detached terminal runtimes]
H --> I[Emit one workspace.closed event per removed workspace]
Reviews (3): Last reviewed commit: "fix: require explicit workspace group cl..." | Re-trigger Greptile
abcf10b to
7ad6f56
Compare
7ad6f56 to
f90311f
Compare
refs herdrdev#2874 Co-authored-by: akbash-bot <300245827+akbash-bot@users.noreply.github.com>
One Mode::ConfirmClose modal is now driven by three independent tokens: the fork's confirm_close_pane and confirm_respawn_pane, and upstream's confirm_close_workspace_id from the explicit workspace-group close (herdrdev#3206). Each opener cleared at most one of the others: the fork's two cleared each other but not the workspace id, and begin_workspace_close_confirmation cleared neither fork token. A workspace confirmation the user dismissed by opening a pane confirmation therefore stayed pending, and the next accept consumed it through take_confirmed_workspace_close_index and closed a workspace nobody had confirmed. Each of the three now clears the other two.
Summary
workspace.closeof a primary checkout while linked-worktree workspaces remain open--groupand raw APIclose_group: truefor explicit group closureworkspace.closedevent per workspace removed by a group closeSupersedes #2876 after rebasing onto current
master; that PR's canonical head branch is no longer writable byakbash-bot.Checks
cargo test api_workspace_close_noncontiguous_group_preserves_adversarial_identity_state -- --nocapturejust ci 'all() - test(live_server_holds_one_pty_master_fd_per_pane)'(3522 passed; excluded test has a checkout-localtarget/debug/herdrPID-discovery assumption incompatible with Kennel's required externalCARGO_TARGET_DIR)just windows-lintrefs #2874