fix: keep worktree removal responsive - #2334
Conversation
kangal-bot
added
the
ai-review
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughWorktree removal now runs in the background with spinner progress on workspace rows. Completion and failure handling preserve or restore the correct UI state. Tests cover spinner timing, removal lifecycle, workspace preservation, and rendering. Configuration documentation describes the updated behavior. ChangesBackground worktree removal
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant worktrees.rs
participant deferred.rs
participant runtime.rs
participant sidebar.rs
User->>worktrees.rs: request worktree removal
worktrees.rs->>runtime.rs: start removal spinner
worktrees.rs->>deferred.rs: submit deferred removal
runtime.rs->>sidebar.rs: advance spinner frame
deferred.rs->>worktrees.rs: report completion or failure
worktrees.rs->>runtime.rs: stop removal spinner
worktrees.rs->>sidebar.rs: render updated workspace state
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThe PR moves worktree removal into a responsive background UI flow while retaining failure confirmation.
Confidence Score: 5/5The PR appears safe to merge within the scope of this follow-up review. No blocking failure remains in the eligible review scope.
|
| Filename | Overview |
|---|---|
| src/app/worktrees.rs | Moves TUI removal into the background, manages completion state, and preserves workspace focus during successful removal. |
| src/app/runtime.rs | Adds the worktree-removal spinner deadline and advances its animation through scheduled tasks. |
| src/app/api/worktrees/deferred.rs | Integrates spinner cleanup and confirmation restoration with deferred API removal completion. |
| src/app/state.rs | Adds the spinner frame to transient worktree-removal state. |
| src/ui/sidebar.rs | Renders animated removal indicators and status text on affected workspace and group rows. |
| src/app/mod.rs | Initializes the runtime-only spinner deadline. |
| src/app/api/worktrees.rs | Extends deferred-removal tests to cover spinner cleanup and mode preservation. |
| src/app/input/mouse.rs | Updates mouse interaction fixtures for the expanded worktree-removal state. |
| docs/next/website/src/content/docs/configuration.mdx | Documents responsive background worktree removal in English. |
| docs/next/website/src/content/docs/ja/configuration.mdx | Documents responsive background worktree removal in Japanese. |
| docs/next/website/src/content/docs/zh-cn/configuration.mdx | Documents responsive background worktree removal in Simplified Chinese. |
Sequence Diagram
sequenceDiagram
participant User
participant App
participant Git as Background git
participant Sidebar
User->>App: Confirm worktree removal
App->>Git: Start deferred removal
App->>App: Dismiss confirmation
App->>Sidebar: Animate affected workspace
alt Removal succeeds
Git-->>App: Success
App->>Sidebar: Remove workspace and stop spinner
else Removal fails
Git-->>App: Error
App->>App: Stop spinner and reopen confirmation
end
Reviews (2): Last reviewed commit: "address coderabbit wet code nit" | Re-trigger Greptile
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/app/api/worktrees/deferred.rs (1)
505-531: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the duplicated match-condition check.
Lines 512-522 and 523-528 both re-derive
remove.workspace_id == result.workspace_id && remove.path == result.pathseparately: once to mutateremoving/error/force_confirmation, and again immediately after to decide whether to stop the spinner and force the mode switch. Combine both checks into a singleif let Some(remove) = &mut self.state.worktree_remove { if <condition> { ...mutate...; stop spinner; set mode; } }block so the two decisions can never drift apart if one condition is edited later without the other.♻️ Proposed consolidation
- if let Some(remove) = &mut self.state.worktree_remove { - if remove.workspace_id == result.workspace_id && remove.path == result.path { - remove.removing = false; - if code == "dirty_worktree_requires_force" && !remove.force_confirmation { - remove.force_confirmation = true; - remove.error = None; - } else { - remove.error = Some(message.clone()); - } - } - } - if self.state.worktree_remove.as_ref().is_some_and(|remove| { - remove.workspace_id == result.workspace_id && remove.path == result.path - }) { - self.stop_worktree_remove_spinner(); - self.state.mode = crate::app::Mode::ConfirmRemoveWorktree; - } + let matches_current_remove = self.state.worktree_remove.as_ref().is_some_and(|remove| { + remove.workspace_id == result.workspace_id && remove.path == result.path + }); + if matches_current_remove { + if let Some(remove) = &mut self.state.worktree_remove { + remove.removing = false; + if code == "dirty_worktree_requires_force" && !remove.force_confirmation { + remove.force_confirmation = true; + remove.error = None; + } else { + remove.error = Some(message.clone()); + } + } + self.stop_worktree_remove_spinner(); + self.state.mode = crate::app::Mode::ConfirmRemoveWorktree; + }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bd89c319-e92a-4269-8574-b1030e4f7377
📒 Files selected for processing (11)
docs/next/website/src/content/docs/configuration.mdxdocs/next/website/src/content/docs/ja/configuration.mdxdocs/next/website/src/content/docs/zh-cn/configuration.mdxsrc/app/api/worktrees.rssrc/app/api/worktrees/deferred.rssrc/app/input/mouse.rssrc/app/mod.rssrc/app/runtime.rssrc/app/state.rssrc/app/worktrees.rssrc/ui/sidebar.rs
|
i don't have have a problem here. please open a discussion if it feels slow, or an issue if it's preventing you to use herdr. |
3 participants
changes