fix(ui): compact and animate shortcut pills - #398
Conversation
Issue: Shortcut pills for directory and pull-request actions remained visible while the focused terminal ran a foreground process, even though their commands could not reach the shell. Fixed pill positions also left large gaps when optional pills disappeared. Solution: Make the Cmd+P and Cmd+O pills follow the foreground-process visibility rule already used by Cmd+T, and close their overlays when the terminal becomes busy. Let PillGroupComponent pack available pills right-aligned with fixed spacing and synchronize overlay geometry with eased 200 ms membership and position transitions. Add behavior and layout tests plus documentation.
There was a problem hiding this comment.
🟡 Changes recommended
Three moderate issues remain in event gating and final animation rendering.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR compacts and animates shortcut pills while hiding shell-dependent actions during foreground processes.
Changes:
- Adds right-aligned pill reflow with eased 200 ms transitions.
- Hides unavailable shortcuts and closes affected overlays.
- Adds tests and updates documentation.
File summaries
| File | Review |
|---|---|
src/ui/components/worktree_overlay.zig |
Applies busy-shell visibility rules. |
src/ui/components/recent_folders_overlay.zig |
Moderate (1 vote): Gate all events while unavailable so queued keyboard/text input reaches the foreground process. |
src/ui/components/pr_dropdown.zig |
Moderate (1 vote): Gate the whole handler while unavailable so the open overlay cannot consume foreground-process input. |
src/ui/components/pill_group.zig |
Moderate (1 vote): Re-arm frame rendering when animation completes so final coordinates are presented. |
src/ui/components/expanding_overlay.zig |
Supports externally positioned overlay geometry. |
src/main.zig |
Registers pill-layout tests. |
README.md |
Documents contextual pill behavior. |
docs/ARCHITECTURE.md |
Documents pill-group layout ownership. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Issue: Review found that open shortcut pickers could consume input during a foreground process, and the pill layout could stop rendering before its final animation coordinates were presented. Solution: Re-arm the pill group's first-frame guard when layout easing completes. Reject all events from unavailable pull-request and recent-folder pickers after Escape cleanup, so foreground-process input is not consumed. Add regression coverage and document event-boundary gating. Addresses: #398 (comment) Addresses: #398 (comment) Addresses: #398 (comment)
There was a problem hiding this comment.
🔵 Needs a closer look
The stuck removal-confirmation modal must be cleared when the terminal becomes busy.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/ui/components/worktree_overlay.zig:144
- If the remove-confirmation modal is open when the focused terminal becomes busy, this early return stops all modal input, but the busy transition only clears
creating; it never clearsconfirming_removal. Becauserenderexplicitly continues drawing that modal, it remains visibly stuck while clicks and keys pass through to the terminal. Clear the pending removal (viaclearPendingRemoval()) in the busy transition before hiding/collapsing the worktree overlay.
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The worktree removal modal becomes non-interactive when the focused terminal becomes busy.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Balanced
Issue: Copilot review found that the worktree removal confirmation modal remained visible after the focused terminal became busy, even though its input was disabled. Solution: Clear the pending removal state, including the confirmation flag and owned path, during the busy transition before collapsing the worktree overlay. Add a regression test covering modal cleanup and document the behavior. Addresses: #398 (review)
There was a problem hiding this comment.
🟡 Changes recommended
Moderate overlay-state and pill-collision issues can disrupt hit-testing and interaction.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/ui/components/pill_group.zig:79
- Seeding every absent pill at its full-row slot can place it directly on top of an existing compacted pill when it becomes available. For example, with only PR + Help visible, PR is at x=680 and the hidden Recent pill is also seeded at x=680; adding a recent folder renders both there until PR moves left, and the higher-z Recent component intercepts clicks over PR. Stage the new pill until the occupied slot clears, or use a collision-free entrance path so layout animation does not make hit-testing ambiguous.
src/ui/components/pr_dropdown.zig:335 - The busy transition hides rendering immediately but leaves the overlay in a 200 ms
.Collapsingstate. If focus moves to an idle terminal before that finishes, the PR pill becomes visible again at the partially collapsed panel size, and its mouse handler cannot open it until the animation ends. Complete the hidden close immediately and reset the flow state so the pill always returns in.Closed.
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Balanced
Issue: Shortcut pickers could reappear in a partially collapsed state after the focused terminal became busy, and newly available pills could overlap existing hit targets during layout changes. Solution: Force unavailable pickers to the closed state while clearing their transient input and flow state. Stage newly available pills outside the row until the existing pills finish reflowing, and cover both behaviors with regression tests and documentation. Addresses: #398 (comment)
There was a problem hiding this comment.
🟡 Changes recommended
Newly added pills snap instead of easing, and the worktree picker can reappear mid-collapse after a brief busy period.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
src/ui/components/worktree_overlay.zig:297
- The busy transition still leaves an open worktree picker in
.Collapsing. If the foreground process exits within 200 ms,availableis refreshed and the picker becomes visible again partway through its collapse, rather than returning as a compact pill. Close every non-closed state immediately here, as the recent-folder and PR pickers do, and reset the flow animation state.
self.clearPendingRemoval();
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Balanced
Issue: Newly available shortcut pills snapped into place after reflow, and a worktree picker could reappear partway through its collapse when a busy terminal became idle quickly. Solution: Split pill membership transitions into reflow and eased entrance phases so staged pills animate while the layout keeps requesting frames. Close unavailable worktree overlays immediately and clear their modal, hover, input, and flow state, with regression coverage for the busy-to-idle transition. Addresses: #398 (comment) Addresses: #398 (comment) (suppressed worktree finding)
There was a problem hiding this comment.
🟡 Changes recommended
Entering pills can overlap existing pills while remaining interactive, causing unsafe click routing.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
Four moderate input-handling and hit-testing issues remain unresolved.
Review details
Suppressed comments (4)
Previously missed (3) — in code that hasn't changed since the last review.
src/ui/components/pr_dropdown.zig:147
- Preserve and handle an outstanding Escape release before rejecting unavailable input. If the shell becomes busy after this picker consumes Escape key-down, this gate—and the new immediate-close/interactivity cleanup that clears
escape_pressed—allows runtime.zig:2613-2623 to send the later key-up as an unexpected ESC byte to the foreground process. Keep the pending-release flag through the busy close and consume that key-up before this gate.
src/ui/components/recent_folders_overlay.zig:156 - Preserve and handle an outstanding Escape release before rejecting unavailable input. If the shell becomes busy after this picker consumes Escape key-down, this gate—and the new immediate-close/interactivity cleanup that clears
escape_pressed—allows runtime.zig:2613-2623 to send the later key-up as an unexpected ESC byte to the foreground process. Keep the pending-release flag through the busy close and consume that key-up before this gate.
src/ui/components/worktree_overlay.zig:137 - Preserve and handle an outstanding Escape release before rejecting unavailable input. If the shell becomes busy after this picker consumes Escape key-down, this gate—and the new immediate-close/interactivity cleanup that clears
escape_pressed—allows runtime.zig:2613-2623 to send the later key-up as an unexpected ESC byte to the foreground process. Keep the pending-release flag through the busy close and consume that key-up before this gate.
src/ui/components/pill_group.zig:176
- An entering pill can still activate an existing pill underneath it. For example, halfway through the tested PR entrance the PR occupies x=760–800 while Help occupies x=740–780; the PR is non-interactive here, so reverse z-order dispatch lets Help consume a click at x=770 and open unexpectedly. Either consume pointer events over entering-pill bounds at the pill-group layer or use an entrance path that never overlaps an interactive pill.
fn pillInteractive(self: *const PillLayout, pill: PillKind) bool {
const idx = @intFromEnum(pill);
return self.visible[idx] and !self.entering[idx];
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
Addressed the four issues from the latest Copilot review in |
Issue
The Cmd+P and Cmd+O shortcut pills remained visible while the focused terminal ran a foreground process, even though their commands could not reach the shell. Optional pills also kept fixed slots, leaving large gaps when a pill was unavailable.
Solution
Cmd+P and Cmd+O now follow Cmd+T's foreground-process visibility rule, and their overlays close when the focused terminal becomes busy.
PillGroupComponentnow owns a right-aligned, fixed-spacing layout for all available pills and synchronizes overlay geometry with 200 ms eased transitions when pills are added, removed, or moved.The change includes visibility, layout, easing, and geometry tests, and updates the user and architecture documentation.
Context
No issue link was provided for this change.
Test plan