Repository navigation
Conversation
|
Run failed. View the logs →
|
|
Closing this PR. I am not going to keep iterating contribution PRs here; if a report is useful I will file an issue instead. |
📝 WalkthroughWalkthroughAdds dashboard agent-session closure across sidebar and popout views. The change defines and validates a shared close request, transports it through preload and IPC, and handles it in the main renderer. Live sessions show confirmation before closing the full terminal tab. Dead sessions are dismissed directly. Sidebar and popout controls, translations, unit tests, and an end-to-end screenshot spec are included. Priority: ⚪ Not assessed Merge Risk: 🟡 Moderate · up to Closing a retained agent card can unexpectedly close an entire terminal tab and stop work in its split panes. This destructive behavior should be corrected before merge; the remaining issues affect accessibility, dialog polish, and localized close-session UI. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description explains the main change and lists a test plan, but it omits most required template sections, including ELI5, What Changed, Why, Linked Issue with an issue number, Visual Proof, Testing checkboxes, AI Disclosure, Review, Agent skill upstream boundary, Notes, and Checklist. Full details: Docstring CoverageExplanation Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 18 files. (5 skipped: 5 unsupported.)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
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: 7
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 554c98c1-6f6d-48a3-835d-2a793728baa1
📒 Files selected for processing (23)
src/main/ipc/dashboard-payload-validation.tssrc/main/ipc/dashboard-popout.test.tssrc/main/ipc/dashboard-popout.tssrc/preload/api-types.tssrc/preload/index.tssrc/renderer/src/components/dashboard-popout/AgentKanbanBoard.tsxsrc/renderer/src/components/dashboard-popout/AgentKanbanCard.test.tsxsrc/renderer/src/components/dashboard-popout/AgentKanbanCard.tsxsrc/renderer/src/components/dashboard/DashboardAgentRow.tsxsrc/renderer/src/components/dashboard/DashboardAgentRowTrailingControls.tsxsrc/renderer/src/components/dashboard/close-dashboard-agent-session.tssrc/renderer/src/components/dashboard/useDashboardPopoutBridge.test.tsxsrc/renderer/src/components/dashboard/useDashboardPopoutBridge.tssrc/renderer/src/components/sidebar/CloseAgentSessionDialog.tsxsrc/renderer/src/components/sidebar/WorktreeCardAgents.tsxsrc/renderer/src/components/sidebar/use-agent-session-close.tssrc/renderer/src/i18n/locales/en.jsonsrc/renderer/src/i18n/locales/es.jsonsrc/renderer/src/i18n/locales/ja.jsonsrc/renderer/src/i18n/locales/ko.jsonsrc/renderer/src/i18n/locales/zh.jsonsrc/shared/dashboard-snapshot.tstests/e2e/dashboard-close-agent-screenshot.spec.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
| const [closeTarget, setCloseTarget] = useState<DashboardCard | null>(null) | ||
| const handleCloseCard = useCallback((card: DashboardCard) => { | ||
| if (card.ptyId === null) { | ||
| void window.api.dashboard.closeAgent?.({ paneKey: card.paneKey, tabId: card.tabId }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 6 'ptyId: null|ptyId.*null|tabId:.*tabId|retainedAgents' \
src/renderer/src/components/dashboardRepository: stablyai/orca
Length of output: 44631
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- AgentKanbanBoard dismissal branch ---'
sed -n '205,250p' src/renderer/src/components/dashboard-popout/AgentKanbanBoard.tsx
printf '%s\n' '--- snapshot row construction ---'
sed -n '230,315p' src/renderer/src/components/dashboard/build-dashboard-snapshot.ts
printf '%s\n' '--- closeAgent bindings and implementation locations ---'
rg -n -C 5 'closeDashboardAgentSession|closeAgent\??:|closeAgent:' src | head -240Repository: stablyai/orca
Length of output: 13112
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- close session implementation ---'
cat -n src/renderer/src/components/dashboard/close-dashboard-agent-session.ts
printf '%s\n' '--- retained row construction ---'
sed -n '1,220p' src/renderer/src/components/dashboard/build-dashboard-snapshot.ts
sed -n '214,310p' src/renderer/src/components/dashboard/useRetainedAgents.ts
printf '%s\n' '--- close API type ---'
rg -n -C 5 'DashboardCloseAgentArgs' src shared
printf '%s\n' '--- focused snapshot tests around retained rows and PTY liveness ---'
rg -n -C 12 'retained|ptyIdsByTabId|ptyIdsByLeafId|split|dead|gone' \
src/renderer/src/components/dashboard/build-dashboard-snapshot*.test.ts \
src/renderer/src/components/dashboard/close-dashboard-agent-session*.test.ts \
src/renderer/src/components/dashboard/useRetainedAgents*.test.tsRepository: stablyai/orca
Length of output: 23922
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- row builder and retained selector locations ---'
rg -n -C 4 'function buildWorktreeAgentRows|export function buildWorktreeAgentRows|selectRetainedAgentEntriesForWorktree|type RetainedAgentEntry' \
src/renderer/src/components/sidebar src/shared
printf '%s\n' '--- row builder source ---'
builder=$(rg -l 'function buildWorktreeAgentRows|export function buildWorktreeAgentRows' src/renderer/src/components/sidebar | head -1)
if [ -n "$builder" ]; then
cat -n "$builder"
fi
printf '%s\n' '--- retained selector source ---'
selector=$(rg -l 'selectRetainedAgentEntriesForWorktree' src/renderer/src/components/sidebar | head -1)
if [ -n "$selector" ]; then
cat -n "$selector"
fi
printf '%s\n' '--- focused tests ---'
rg -n -C 12 'retained done|retained.*tab|ptyIdsByTabId|ptyIdsByLeafId|split|gone|dead state' \
src/renderer/src/components/dashboard/build-dashboard-snapshot*.test.ts \
src/renderer/src/components/sidebar/*agent-row*.test.ts \
src/renderer/src/components/dashboard/close-dashboard-agent-session*.test.ts 2>/dev/null || trueRepository: stablyai/orca
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- close target resolver ---'
cat -n src/renderer/src/components/terminal/terminal-close-target.ts
printf '%s\n' '--- retained snapshot test ---'
sed -n '238,260p' src/renderer/src/components/dashboard/build-dashboard-snapshot.test.ts
sed -n '318,350p' src/renderer/src/components/dashboard/build-dashboard-snapshot.test.ts
printf '%s\n' '--- relevant terminal tab types and live PTY selectors ---'
rg -n -C 5 'type TerminalTab|interface TerminalTab|selectLivePtyIdsForWorktree|ptyIdsByTabId' \
src/shared/types.ts src/renderer/src/components/sidebar/worktree-card-status-inputs.tsRepository: stablyai/orca
Length of output: 8325
Pass null when dismissing cards without a live PTY.
buildDashboardSnapshot can emit a retained card with ptyId: null while tabId remains resolvable. The current call then reaches closeDashboardAgentSession, which closes the whole terminal tab, including split panes and sibling agents, instead of dismissing only the retained row.
- void window.api.dashboard.closeAgent?.({ paneKey: card.paneKey, tabId: card.tabId })
+ void window.api.dashboard.closeAgent?.({ paneKey: card.paneKey, tabId: null })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void window.api.dashboard.closeAgent?.({ paneKey: card.paneKey, tabId: card.tabId }) | |
| void window.api.dashboard.closeAgent?.({ paneKey: card.paneKey, tabId: null }) |
| <span | ||
| role="button" | ||
| tabIndex={0} | ||
| aria-label={translate('dashboardPopout.card.closeSession', 'Close session')} | ||
| title={translate('dashboardPopout.card.closeSession', 'Close session')} | ||
| onClick={handleCloseClick} | ||
| onKeyDown={handleCloseKeyDown} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Move the close control outside the card button.
Line 276 creates an interactive control inside the native button at line 246. This is invalid interactive-content nesting. Assistive technology and keyboard activation can produce inconsistent results.
Make the close button a sibling of the card button. Keep its click and key handling isolated.
| <Dialog open={open} onOpenChange={onOpenChange}> | ||
| {open ? ( | ||
| <DialogContent className="max-w-sm sm:max-w-sm" showCloseButton={false}> | ||
| <DialogHeader> | ||
| <DialogTitle className="text-sm"> | ||
| {translate( | ||
| 'auto.components.sidebar.WorktreeCardAgents.2ec931a922', | ||
| 'Close agent session?' | ||
| )} | ||
| </DialogTitle> | ||
| <DialogDescription className="text-xs"> | ||
| {translate( | ||
| 'auto.components.sidebar.WorktreeCardAgents.e1250e23e9', | ||
| 'This closes the entire terminal tab running this agent, including all of its split panes. Any other agents running in those panes will be stopped too.' | ||
| )} | ||
| </DialogDescription> | ||
| </DialogHeader> | ||
| <DialogFooter> | ||
| <Button variant="outline" onClick={() => onOpenChange(false)}> | ||
| {translate('auto.components.sidebar.WorktreeCardAgents.7d17689ff0', 'Cancel')} | ||
| </Button> | ||
| <Button variant="destructive" onClick={onConfirm}> | ||
| {translate('auto.components.sidebar.WorktreeCardAgents.6ac88e0ff9', 'Close session')} | ||
| </Button> | ||
| </DialogFooter> | ||
| </DialogContent> | ||
| ) : null} | ||
| </Dialog> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
# Description: Find other Dialog usages and check whether DialogContent is
# gated behind an `open ? ... : null` ternary or rendered unconditionally.
set -euo pipefail
echo "== Files gating DialogContent behind an open-ternary =="
rg -nP '\{\s*(open|.*[Oo]pen)\s*\?\s*\(' -g '*.tsx' src/renderer/src | rg -B2 'DialogContent' || true
echo
echo "== Sample of Dialog usages that render DialogContent unconditionally =="
rg -n -B1 -A1 '<Dialog ' -g '*.tsx' src/renderer/src | rg -A3 'DialogContent'Repository: stablyai/orca
Length of output: 19989
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Reviewed component imports and render =="
cat -n src/renderer/src/components/sidebar/CloseAgentSessionDialog.tsx
echo
echo "== Local dialog primitive definitions =="
fd -i 'dialog.tsx' src/renderer/src
DIALOG_FILE="$(fd -i 'dialog.tsx' src/renderer/src | head -n 1)"
cat -n "$DIALOG_FILE" | sed -n '1,240p'
echo
echo "== Dialog dependency declarations =="
rg -n '"`@radix-ui/react-dialog`"|"radix-ui"' package.json src package.json pnpm-lock.yaml yarn.lock package-lock.json 2>/dev/null | head -80 || trueRepository: stablyai/orca
Length of output: 17237
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Exact local dialog primitive =="
wc -l src/renderer/src/components/ui/dialog.tsx
cat -n src/renderer/src/components/ui/dialog.tsx
echo
echo "== Relevant dependency metadata =="
sed -n '200,225p' package.json
rg -n -A8 -B3 '"radix-ui"' pnpm-lock.yaml yarn.lock package-lock.json 2>/dev/null | head -80 || true
echo
echo "== Direct-child dialog examples =="
cat -n src/renderer/src/components/sidebar/RemoveFolderDialog.tsx | sed -n '65,95p'Repository: stablyai/orca
Length of output: 8986
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Lockfiles =="
git ls-files | rg '(^|/)(pnpm-lock\.yaml|package-lock\.json|yarn\.lock|bun\.lockb?|npm-shrinkwrap\.json)$' || true
echo
echo "== Resolved radix-ui entries =="
for f in pnpm-lock.yaml package-lock.json yarn.lock bun.lock bun.lockb npm-shrinkwrap.json; do
if [ -f "$f" ]; then
echo "-- $f"
rg -n -A4 -B2 'radix-ui' "$f" | head -80 || true
fi
doneRepository: stablyai/orca
Length of output: 3460
🌐 Web query:
radix-ui 1.6.2 Dialog Content Presence source
💡 Result:
<search_synthesis>
In Radix UI, including version 1.6.2, component presence is managed internally by the @radix-ui/react-presence utility [1][2]. This utility enables support for exit animations by allowing components to remain mounted in the DOM after their state changes to "closed," until any associated CSS animations or transitions have completed [3][4]. Key points regarding Dialog Content presence: 1. Internal Presence Management: The Dialog component uses the Presence utility to track its mount/unmount lifecycle [3]. When the open state changes to false, the component monitors for CSS animation endings (via animationend, animationcancel, and animationstart events) to determine when to safely remove the content from the DOM [3]. 2. CSS Animations: You can trigger exit animations by targeting the [data-state="closed"] attribute on the DialogContent component [4]. The presence utility detects the animation and suspends unmounting until the animation completes [3][4]. 3. forceMount Prop: For scenarios where you need to manage presence manually—such as when using third-party animation libraries like Framer Motion or React Spring—Radix UI exposes the forceMount prop on Dialog.Content and Dialog.Overlay [5][6][4]. When forceMount is used, the component will remain mounted regardless of the open state, shifting the responsibility of mounting and unmounting the element to your code [6][4]. 4. Implementation Note: If wrapping Dialog.Content in custom elements, ensure the wrapper does not interfere with the detection of animations, as the presence utility relies on reading styles directly from the component being unmounted [3][7]. Using the asChild prop is often recommended to maintain the correct DOM structure and animation behavior [5][7].
</search_synthesis>
<source_evidence>
Citations:
- 1: https://github.com/radix-ui/primitives/blob/main/packages/react/dialog/CHANGELOG.md
- 2: https://github.com/radix-ui/primitives/tree/main/packages/react/presence
- 3: https://radix-ui-primitives.mintlify.app/api/presence
- 4: https://www.radix-ui.com/primitives/docs/guides/animation
- 5: https://www.radix-ui.com/primitives/docs/components/dialog
- 6: https://mintlify.wiki/radix-ui/primitives/guides/animation
- 7: GitHub issue 3324 in radix-ui/primitives (link omitted to avoid creating a cross-reference)
Keep DialogContent mounted during close.
DialogContent wraps radix-ui@1.6.2's Dialog.Content, which uses Presence to wait for data-state="closed" animations. The {open ? ... : null} branch removes the content before Presence can run, so the fade/zoom-out animation is skipped. Render DialogContent unconditionally and let Dialog control its presence.
| "1b0a156717": "Agents", | ||
| "2ec931a922": "Close agent session?", | ||
| "e1250e23e9": "This closes the entire terminal tab running this agent, including all of its split panes. Any other agents running in those panes will be stopped too.", | ||
| "7d17689ff0": "Cancel", | ||
| "6ac88e0ff9": "Close session" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Translate the new values in es.json.
These entries remain in English in the Spanish locale. Spanish users will see English labels and close-session confirmation text in the sidebar, dashboard, and shared dialog. Replace them with Spanish translations in all three blocks.
Proposed translations
- "1b0a156717": "Agents",
+ "1b0a156717": "Agentes",
- "2ec931a922": "Close agent session?",
+ "2ec931a922": "¿Cerrar la sesión del agente?",
- "e1250e23e9": "This closes the entire terminal tab running this agent, including all of its split panes. Any other agents running in those panes will be stopped too.",
+ "e1250e23e9": "Esto cierra la pestaña de terminal completa donde se ejecuta este agente, incluidos todos sus paneles divididos. También se detendrán los demás agentes que se ejecuten en esos paneles.",
- "7d17689ff0": "Cancel",
+ "7d17689ff0": "Cancelar",
- "6ac88e0ff9": "Close session",
+ "6ac88e0ff9": "Cerrar sesión",
- "271b256627": "Close agent session",
+ "271b256627": "Cerrar la sesión del agente",
- "1c27be5226": "Close session"
+ "1c27be5226": "Cerrar sesión"
- "closeTitle": "Close agent session?",
+ "closeTitle": "¿Cerrar la sesión del agente?",
- "closeDescription": "This closes the entire terminal tab running this agent, including all of its split panes. Any other agents running in those panes will be stopped too.",
+ "closeDescription": "Esto cierra la pestaña de terminal completa donde se ejecuta este agente, incluidos todos sus paneles divididos. También se detendrán los demás agentes que se ejecuten en esos paneles.",
- "closeCancel": "Cancel",
+ "closeCancel": "Cancelar",
- "closeConfirm": "Close session",
+ "closeConfirm": "Cerrar sesión",
- "closeSession": "Close session"
+ "closeSession": "Cerrar sesión"Also applies to: 13206-13207, 14533-14537
| "2ec931a922": "Close agent session?", | ||
| "e1250e23e9": "This closes the entire terminal tab running this agent, including all of its split panes. Any other agents running in those panes will be stopped too.", | ||
| "7d17689ff0": "Cancel", | ||
| "6ac88e0ff9": "Close session" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Translate the new close-session strings.
The Japanese locale contains English values for the close-session dialogs and controls. Japanese users will see English text in the sidebar, dashboard, and popout flows.
Add Japanese translations to all three namespaces. Keep repeated labels such as closeConfirm and closeSession consistent.
This is required by the PR objective to localize the close flow in Japanese.
Also applies to: 13206-13207, 14533-14537
| "2ec931a922": "Close agent session?", | ||
| "e1250e23e9": "This closes the entire terminal tab running this agent, including all of its split panes. Any other agents running in those panes will be stopped too.", | ||
| "7d17689ff0": "Cancel", | ||
| "6ac88e0ff9": "Close session" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add Korean translations for the new close-session strings.
The values at Lines 4417-4420, Lines 13206-13207, and Lines 14533-14537 are still English. Korean users will see English labels and confirmation text in the sidebar, dashboard, and popout flows. Replace these values with approved Korean translations while preserving the existing message meaning.
Also applies to: 13206-13207, 14533-14537
| "2ec931a922": "Close agent session?", | ||
| "e1250e23e9": "This closes the entire terminal tab running this agent, including all of its split panes. Any other agents running in those panes will be stopped too.", | ||
| "7d17689ff0": "Cancel", | ||
| "6ac88e0ff9": "Close session" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not add English placeholders to zh.json.
The new close-session strings override the fallback and display English in the Simplified Chinese UI. Translate each key, or omit each key until the translation is available.
src/renderer/src/i18n/locales/zh.json#L4417-L4420: translate theWorktreeCardAgentsconfirmation strings.src/renderer/src/i18n/locales/zh.json#L13206-L13207: translate theDashboardAgentRowclose-session labels.src/renderer/src/i18n/locales/zh.json#L14533-L14537: translate thedashboardPopoutconfirmation strings.
Based on learnings: new English-source keys should remain absent from non-English locale files until real translations exist.
📍 Affects 1 file
src/renderer/src/i18n/locales/zh.json#L4417-L4420(this comment)src/renderer/src/i18n/locales/zh.json#L13206-L13207src/renderer/src/i18n/locales/zh.json#L14533-L14537
Source: Learnings
1 participant
Summary
Lets you close a CLI agent session from the dashboard / sidebar instead of only from the session itself.
use-agent-session-closehookThis is on top of the earlier stop-from-board work; it is the close-session path (
CloseAgentSessionDialog,close-dashboard-agent-session.ts), not the Antigravity quota rebuild in #12095.Test plan
tests/e2e/dashboard-close-agent-screenshot.spec.ts