Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between 2a5ac4cbc3b4172e2a0e98e76d98a63c5fee38de and c9199c7. 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (5)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe change adds dashboard agent stopping. It defines and validates stop payloads, exposes stop methods through preload and IPC, and forwards requests to the trusted renderer. The dashboard adds inline and dialog confirmations. The renderer removes agent state and closes or kills the related terminal pane. Tests cover validation, IPC forwarding, UI interactions, live-card re-resolution, and teardown behavior. Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to A stale stop confirmation can target a later card that reuses the same pane key, so this should be corrected before merge. Pinned-tab stops complete without requiring an inaccessible confirmation. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description explains the change, motivation, behavior, teardown rules, and validation results. However, it omits required template sections, including a qualifying Linked Issue entry, Visual Proof or an explicit N/A explanation, Testing checkboxes and platform coverage, Notes, Review, the upstream-boundary checklist, and the final checklist. Resolution Complete the required template sections. Add the issue link in the required format, attach before-and-after visual proof or write N/A with a reason, complete the Testing section and platform coverage, address Review, Agent skill upstream boundary, Notes, and Checklist items, and add AI disclosure details when applicable. Full details: Docstring CoverageExplanation Docstring coverage is 42.11% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 18 files. (5 skipped: 5 unsupported.)
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: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b2ccdf78-4bca-474d-92ad-a0186098be2e
📥 Commits
Reviewing files that changed from the base of the PR and between 026389a and 2a5ac4cbc3b4172e2a0e98e76d98a63c5fee38de.
📒 Files selected for processing (22)
src/main/ipc/dashboard-payload-validation.tssrc/main/ipc/dashboard-popout.test.tssrc/main/ipc/dashboard-popout.tssrc/main/ipc/dashboard-stop-agent-validation.tssrc/preload/api/dashboard-api.tssrc/preload/index.tssrc/renderer/src/components/dashboard-popout/AgentKanbanBoard.test.tsxsrc/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-popout/AgentKanbanCardStopControl.tsxsrc/renderer/src/components/dashboard-popout/AgentKanbanColumn.tsxsrc/renderer/src/components/dashboard-popout/AgentStopConfirmDialog.tsxsrc/renderer/src/components/dashboard/useDashboardPopoutBridge.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/renderer/src/lib/stop-dashboard-agent.test.tssrc/renderer/src/lib/stop-dashboard-agent.tssrc/shared/dashboard-snapshot.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
| const [pendingStopPaneKey, setPendingStopPaneKey] = useState<string | null>(null) | ||
| const resolvedPendingStopCard = useMemo(() => { | ||
| if (!pendingStopPaneKey) { | ||
| return null | ||
| } | ||
| return snapshot.cards.find((card) => card.paneKey === pendingStopPaneKey) ?? null | ||
| }, [pendingStopPaneKey, snapshot.cards]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clear the pending stop key when its card disappears.
When snapshot.cards omits the pending paneKey, resolvedPendingStopCard becomes null, so the dialog closes but pendingStopPaneKey remains set. If a later snapshot reintroduces that pane, the dialog opens without a new stop action and can submit a stale confirmation. Clear the pending state when the live card is absent, and add a remove-then-readd regression test.
Proposed fix
const resolvedPendingStopCard = useMemo(() => {
if (!pendingStopPaneKey) {
return null
}
return snapshot.cards.find((card) => card.paneKey === pendingStopPaneKey) ?? null
}, [pendingStopPaneKey, snapshot.cards])
+ useEffect(() => {
+ if (pendingStopPaneKey !== null && resolvedPendingStopCard === null) {
+ setPendingStopPaneKey(null)
+ }
+ }, [pendingStopPaneKey, resolvedPendingStopCard])📝 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.
| const [pendingStopPaneKey, setPendingStopPaneKey] = useState<string | null>(null) | |
| const resolvedPendingStopCard = useMemo(() => { | |
| if (!pendingStopPaneKey) { | |
| return null | |
| } | |
| return snapshot.cards.find((card) => card.paneKey === pendingStopPaneKey) ?? null | |
| }, [pendingStopPaneKey, snapshot.cards]) | |
| const [pendingStopPaneKey, setPendingStopPaneKey] = useState<string | null>(null) | |
| const resolvedPendingStopCard = useMemo(() => { | |
| if (!pendingStopPaneKey) { | |
| return null | |
| } | |
| return snapshot.cards.find((card) => card.paneKey === pendingStopPaneKey) ?? null | |
| }, [pendingStopPaneKey, snapshot.cards]) | |
| useEffect(() => { | |
| if (pendingStopPaneKey !== null && resolvedPendingStopCard === null) { | |
| setPendingStopPaneKey(null) | |
| } | |
| }, [pendingStopPaneKey, resolvedPendingStopCard]) |
| closeTerminalTab(args.tabId, { | ||
| reason: 'user', | ||
| // Why: the pinned-close confirmation would open a modal in the main window, | ||
| // which is behind the board and may be unattended. Stop the agent anyway | ||
| // and leave the pinned tab itself alone — silently doing nothing would read | ||
| // as a broken button. | ||
| rejectPinned: true, | ||
| onCancel: () => killAgentPane(ptyId) | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Skip the duplicate running-process confirmation.
For a running unpinned agent, closeTerminalTab opens its own dialog because this call omits skipRunningProcessConfirm. The board already confirms this action, so the agent remains running until the user handles a second dialog in the main window. Pass skipRunningProcessConfirm: true.
Proposed fix
closeTerminalTab(args.tabId, {
reason: 'user',
+ skipRunningProcessConfirm: true,
rejectPinned: true,📝 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.
| closeTerminalTab(args.tabId, { | |
| reason: 'user', | |
| // Why: the pinned-close confirmation would open a modal in the main window, | |
| // which is behind the board and may be unattended. Stop the agent anyway | |
| // and leave the pinned tab itself alone — silently doing nothing would read | |
| // as a broken button. | |
| rejectPinned: true, | |
| onCancel: () => killAgentPane(ptyId) | |
| }) | |
| closeTerminalTab(args.tabId, { | |
| reason: 'user', | |
| skipRunningProcessConfirm: true, | |
| // Why: the pinned-close confirmation would open a modal in the main window, | |
| // which is behind the board and may be unattended. Stop the agent anyway | |
| // and leave the pinned tab itself alone — silently doing nothing would read | |
| // as a broken button. | |
| rejectPinned: true, | |
| onCancel: () => killAgentPane(ptyId) | |
| }) |
…ktree Clearing a finished agent off the kanban board required opening its worktree, finding the tab, and closing it there — the card only disappeared once the pane died. For idle agents, which is most of what accumulates on the board, that is three navigations to dismiss one card. Add a per-card stop control in the top-right corner, where the state dot sits (the dot fades on hover so they never collide). Idle and done cards confirm inline: the X becomes a red "Stop agent?" pill that commits on a second click and reverts on mouse-leave, blur, Escape, or a 3s timeout. Anything still running routes through a modal reusing CloseTerminalDialog's strings verbatim, so the board and Cmd+W say the same thing. No bulk "clear idle" action: idle means the agent is not working right now, not that its work is finished, so each dismissal stays deliberate. The row must be dropped BEFORE the pane is torn down. dropAgentStatus only plants its one-shot retention suppressor when a live entry still exists; reversed, the retention sync re-adds the agent as a retained "done" row and the card never leaves the board. Two teardown cases the naive "kill the pty" would get wrong: - A split tab hosts panes the user did not ask to close, so kill only this agent's pty and leave the tab standing. Single-pane tabs go through closeTerminalTab for full Cmd+W semantics. - A pinned tab's close confirmation would open a modal in the main window, which is behind the board and may be unattended. Stop the agent anyway via the onCancel path and leave the pinned tab alone — a button that silently does nothing reads as broken. The IPC deliberately does not raise the main window: the point is clearing an agent without leaving the board. The kanban column moves out of AgentKanbanBoard into its own AgentKanbanColumn, and the stop-args validator into dashboard-stop-agent-validation — dashboard-payload-validation is at the 300-line ceiling, so it only re-exports, matching how dashboard-agent-launch-validation is already wired. Claude-Session: https://claude.ai/code/session_01MZJuvDFhjHtuky5z9BF43a
2a5ac4c to
c9199c7
Compare
|
Rebased onto current |
|
Closing this PR. I am not going to keep iterating contribution PRs here; if a report is useful I will file an issue instead. |
1 participant
Split out of #12095, which bundled this dashboard feature with an unrelated Antigravity quota rewrite. This PR is the dashboard half only — no
src/main/rate-limits/changes. Rebased onto currentmain.Why
Clearing a finished agent off the kanban board required opening its worktree, finding the tab, and closing it there — the card only disappeared once the pane died. For idle agents, which is most of what accumulates on the board, that is three navigations to dismiss one card.
What
A per-card stop control in the top-right corner, where the state dot sits (the dot fades on hover so they never collide).
CloseTerminalDialog's strings verbatim, so the board and Cmd+W say the same thing.Ordering and teardown
The row must be dropped before the pane is torn down.
dropAgentStatusonly plants its one-shot retention suppressor when a live entry still exists; reversed, the retention sync re-adds the agent as a retained "done" row and the card never leaves the board.Two teardown cases the naive "kill the pty" would get wrong:
closeTerminalTabfor full Cmd+W semantics.onCancelpath and leave the pinned tab alone — a button that silently does nothing reads as broken.The IPC deliberately does not raise the main window: the point is clearing an agent without leaving the board.
Structural notes
Two extractions the 300-line ceiling forced, both following existing patterns:
AgentKanbanBoardinto its ownAgentKanbanColumn.dashboard-stop-agent-validation.ts;dashboard-payload-validation.tsis at the ceiling, so it only re-exports — the same wiringdashboard-agent-launch-validationalready uses.ja.jsonusesAgentrather thanエージェント, matching every other Agent string in that catalog and satisfyingverify:localization-catalog.Validation
pnpm typecheck,oxlint, the threeverify:localization-*gates,check:reliability-gates,check:max-lines-ratchet,check:runtime-electron-ratchet, andaudit:code-quality:nativeall pass. Scoped vitest run: 45 files, 409 tests passing.https://claude.ai/code/session_01MZJuvDFhjHtuky5z9BF43a