feat(desktop): agent picker for v2 PR action button#4979
Open
Kitenite wants to merge 3 commits into
Open
Conversation
Replace the top-right icon-only Create PR button with a labeled, bordered split-button that mirrors the v1 PRButton / v2 PRStatusGroup pill style. Primary click runs the existing slash-command flow through the default agent (new chat tab today); chevron exposes an agent picker, stubbed with "Coming soon" rows pending the phase-2 data wiring. Surfaces all PR states cleanly: - no-pr → "Create PR" pill - pr-exists → "Update PR" pill alongside the existing #N status pill - busy → same pill, primary disabled, spinner + "Creating…/Updating…" - loading / unavailable / error paths unchanged planDispatch + buildPRContext learn the pr-exists case so Update PR opens a chat with /pr/update-pr and a pr-context.md attachment (the matching slash command is a follow-up). Flip the CREATE_PR_BUTTON_ENABLED gate on now that the flow ships. Phase 1 of plans/20260528-1500-pr-button-agent-select.md.
Phase 2 of the PR action button. The chevron dropdown now renders a real agent picker (active terminal sessions + "Start new" presets) instead of placeholder items, and the primary button routes to the last-picked agent. - Extract the comment composer's selection/persistence into a shared `useAgentTarget` hook (renderer/hooks/agents); `useDiffCommentTarget` and the new `usePRActionAgentTarget` are now thin wrappers with surface-scoped storage keys so picks don't trample each other. - Extract `createNewAgentSession` out of usePaneRegistry into a shared `useCreateNewAgentSession` hook, consumed by both the DiffPane composer and the PR action header. - Add `usePRActionDispatch` to route a submit by target kind: chat-tab fallback (no pick) via the legacy flow, existing terminal via `sendToTerminalAgent` with pr-context inlined, or a new preset launch. - Drop the `createPREnabled` kill-switch; the split button is the default now.
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
Contributor
📝 WalkthroughWalkthroughThis PR implements a phased redesign of the PR action button in the desktop app, replacing legacy icon-driven UI with an agent-session-aware split-button. It introduces shared agent-target selection abstractions for persistence and validation, routes PR submissions to legacy chat tabs, existing terminal agents, or new agent sessions, supports updating existing PRs, and refactors the diff-comment composer to reuse the shared logic. ChangesPR Action Split-Button with Agent Selection
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
|
Ready to review this PR? Stage has broken it down into 8 individual chapters for you: 
Chapters generated by Stage for commit a7c69ef on May 28, 2026 8:39pm UTC. |
Contributor
Greptile SummaryUpgrades the v2 top-right PR action control from an icon-only button to a split button with a working agent picker, mirroring the affordance from the DiffPane comment composer. Shared infrastructure (
Confidence Score: 4/5Safe to merge; all three transports route correctly and the shared hook extraction preserves existing behaviour for the DiffPane composer. The dispatch hook computes
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/hooks/agents/useAgentTarget/useAgentTarget.ts | New shared hook extracting selection state, localStorage persistence, and fallback logic from useDiffCommentTarget — clean abstraction with parameterised storage keys; logic is correct. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useCreateNewAgentSession/useCreateNewAgentSession.ts | Extracted useCreateNewAgentSession hook from usePaneRegistry; logic is identical to the removed inline callback, instantiated in two places (page.tsx + usePaneRegistry) creating two separate mutation objects. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRActionSplitButton/hooks/usePRActionDispatch/usePRActionDispatch.ts | Routes PR submit to terminal, new-session, or legacy chat-tab transport; calls planDispatch (which builds the attachment) then buildPRContext again via formatInlinedPrompt, producing the context string twice for non-chat-tab paths. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/PRActionHeader.tsx | Wires agent picker data, selection state, and dispatch into the ActionSlot; uses inline import() type annotations for AgentTarget in two places instead of a top-level import. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx | Instantiates useCreateNewAgentSession directly and passes it to WorkspaceSidebar; usePaneRegistry (called on the same line) also instantiates the hook internally, creating two mutation objects for the same endpoint. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsx | createNewAgentSession correctly replaced by the extracted hook; callback is functionally equivalent to the removed inline implementation. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/components/AgentCommentComposer/hooks/useDiffCommentTarget/useDiffCommentTarget.ts | Thinned to a wrapper re-exporting from the shared useAgentTarget; backward-compatible re-exports preserve existing consumers. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRActionSplitButton/PRActionSplitButton.tsx | New split-button component; handlePick correctly passes the direct target to onSubmit (not stale resolvedTarget), so the one-click-pick-and-submit flow is correct. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRActionSplitButton/components/PRAgentPickerMenu/PRAgentPickerMenu.tsx | DropdownMenu picker grouping Active sessions + Start new; hardcodes placement split-pane on new items, consistent with plan — no toggle is surfaced here. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/utils/buildPRContext/buildPRContext.ts | Adds renderPrExists for the pr-exists state, generating a well-structured markdown context block with PR metadata, branch/sync status, and preconditions. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
SB["PRActionSplitButton\n(primary click or dropdown pick)"]
PAD["usePRActionDispatch\n(routes by target.kind)"]
T_NULL["target = null\n(no agent picked)"]
T_EXIST["target.kind = 'existing'"]
T_NEW["target.kind = 'new'"]
FD["flowDispatch\n(legacy chat tab +\nattachment)"]
TA["sendToTerminalAgent\n(inlined slash cmd +\npr-context)"]
NA["onCreateNewAgentSession\n(useCreateNewAgentSession)\n(inlined seed prompt)"]
SB -->|"onSubmit(target)"| PAD
PAD --> T_NULL --> FD
PAD --> T_EXIST --> TA
PAD --> T_NEW -->|"onCreateNewAgentSession?"| NA
PAD --> T_NEW -->|"no handler"| ERR["toast.error"]
subgraph "Storage (per-surface keys)"
LS1["lastSelectedPRActionTerminalId"]
LS2["lastSelectedPRActionNewAgentConfigId"]
LS3["lastSelectedPRActionPlacement"]
end
SB -.->|"usePRActionAgentTarget"| LS1
SB -.->|"usePRActionAgentTarget"| LS2
SB -.->|"usePRActionAgentTarget"| LS3
Comments Outside Diff (1)
-
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx, line 1865 (link)Duplicate
useCreateNewAgentSessioninstanceV2WorkspaceContentcallsuseCreateNewAgentSession({ store })directly here and again implicitly viausePaneRegistry(which now calls the same hook with the samestore). Both produce independentworkspaceTrpc.agents.run.useMutation()instances with separate loading state. Functionally correct but wasteful — consider passingcreateNewAgentSessionfromusePaneRegistryout to the page (or vice-versa) so the same instance serves both consumers.Prompt To Fix With AI
This is a comment left during a code review. Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx Line: 1865 Comment: **Duplicate `useCreateNewAgentSession` instance** `V2WorkspaceContent` calls `useCreateNewAgentSession({ store })` directly here and again implicitly via `usePaneRegistry` (which now calls the same hook with the same `store`). Both produce independent `workspaceTrpc.agents.run.useMutation()` instances with separate loading state. Functionally correct but wasteful — consider passing `createNewAgentSession` from `usePaneRegistry` out to the page (or vice-versa) so the same instance serves both consumers. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRActionSplitButton/hooks/usePRActionDispatch/usePRActionDispatch.ts:89-92
`buildPRContext` is computed twice for every terminal/new-agent dispatch: first inside `planDispatch` (which base64-encodes it into an attachment that is then discarded), and again inside `formatInlinedPrompt`. For those two transport paths only `plan.prompt` (the slash command string) is needed from `planDispatch`. Having `formatInlinedPrompt` accept the already-built context string would eliminate the redundant computation.
```suggestion
function formatInlinedPrompt(prompt: string, context: string): string {
return `${prompt}\n\n--- pr-context.md ---\n${context}`;
}
```
### Issue 2 of 3
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx:1865
**Duplicate `useCreateNewAgentSession` instance**
`V2WorkspaceContent` calls `useCreateNewAgentSession({ store })` directly here and again implicitly via `usePaneRegistry` (which now calls the same hook with the same `store`). Both produce independent `workspaceTrpc.agents.run.useMutation()` instances with separate loading state. Functionally correct but wasteful — consider passing `createNewAgentSession` from `usePaneRegistry` out to the page (or vice-versa) so the same instance serves both consumers.
### Issue 3 of 3
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/PRActionHeader.tsx:66-85
Two inline `import()` type annotations are used for `AgentTarget` in this file. Since `AgentTarget` is already re-exported from the `PRActionSplitButton` barrel (which is already imported just above), a single top-level import is cleaner and avoids the repeated inline syntax.
```suggestion
const onPickTarget = (
target: AgentTarget,
) => {
onValueChange(
target.kind === "existing"
? `existing:${target.terminalId}`
: `new:${target.configId}`,
);
};
const splitButtonProps = {
sessions,
configs,
selectedValue,
resolvedTarget,
onPickTarget,
onSubmit: (target: AgentTarget | null) => submit({ state, target }),
};
```
Reviews (1): Last reviewed commit: "feat(desktop): wire agent picker into v2..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+89
to
+92
| function formatInlinedPrompt(prompt: string, state: PRFlowState): string { | ||
| const context = buildPRContext(state); | ||
| return `${prompt}\n\n--- pr-context.md ---\n${context}`; | ||
| } |
Contributor
There was a problem hiding this comment.
buildPRContext is computed twice for every terminal/new-agent dispatch: first inside planDispatch (which base64-encodes it into an attachment that is then discarded), and again inside formatInlinedPrompt. For those two transport paths only plan.prompt (the slash command string) is needed from planDispatch. Having formatInlinedPrompt accept the already-built context string would eliminate the redundant computation.
Suggested change
| function formatInlinedPrompt(prompt: string, state: PRFlowState): string { | |
| const context = buildPRContext(state); | |
| return `${prompt}\n\n--- pr-context.md ---\n${context}`; | |
| } | |
| function formatInlinedPrompt(prompt: string, context: string): string { | |
| return `${prompt}\n\n--- pr-context.md ---\n${context}`; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRActionSplitButton/hooks/usePRActionDispatch/usePRActionDispatch.ts
Line: 89-92
Comment:
`buildPRContext` is computed twice for every terminal/new-agent dispatch: first inside `planDispatch` (which base64-encodes it into an attachment that is then discarded), and again inside `formatInlinedPrompt`. For those two transport paths only `plan.prompt` (the slash command string) is needed from `planDispatch`. Having `formatInlinedPrompt` accept the already-built context string would eliminate the redundant computation.
```suggestion
function formatInlinedPrompt(prompt: string, context: string): string {
return `${prompt}\n\n--- pr-context.md ---\n${context}`;
}
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+66
to
+85
| const onPickTarget = ( | ||
| target: import("renderer/hooks/agents/useAgentTarget").AgentTarget, | ||
| ) => { | ||
| onValueChange( | ||
| target.kind === "existing" | ||
| ? `existing:${target.terminalId}` | ||
| : `new:${target.configId}`, | ||
| ); | ||
| }; | ||
|
|
||
| const splitButtonProps = { | ||
| sessions, | ||
| configs, | ||
| selectedValue, | ||
| resolvedTarget, | ||
| onPickTarget, | ||
| onSubmit: ( | ||
| target: import("renderer/hooks/agents/useAgentTarget").AgentTarget | null, | ||
| ) => submit({ state, target }), | ||
| }; |
Contributor
There was a problem hiding this comment.
import() type annotations are used for AgentTarget in this file. Since AgentTarget is already re-exported from the PRActionSplitButton barrel (which is already imported just above), a single top-level import is cleaner and avoids the repeated inline syntax.
Suggested change
| const onPickTarget = ( | |
| target: import("renderer/hooks/agents/useAgentTarget").AgentTarget, | |
| ) => { | |
| onValueChange( | |
| target.kind === "existing" | |
| ? `existing:${target.terminalId}` | |
| : `new:${target.configId}`, | |
| ); | |
| }; | |
| const splitButtonProps = { | |
| sessions, | |
| configs, | |
| selectedValue, | |
| resolvedTarget, | |
| onPickTarget, | |
| onSubmit: ( | |
| target: import("renderer/hooks/agents/useAgentTarget").AgentTarget | null, | |
| ) => submit({ state, target }), | |
| }; | |
| const onPickTarget = ( | |
| target: AgentTarget, | |
| ) => { | |
| onValueChange( | |
| target.kind === "existing" | |
| ? `existing:${target.terminalId}` | |
| : `new:${target.configId}`, | |
| ); | |
| }; | |
| const splitButtonProps = { | |
| sessions, | |
| configs, | |
| selectedValue, | |
| resolvedTarget, | |
| onPickTarget, | |
| onSubmit: (target: AgentTarget | null) => submit({ state, target }), | |
| }; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/PRActionHeader.tsx
Line: 66-85
Comment:
Two inline `import()` type annotations are used for `AgentTarget` in this file. Since `AgentTarget` is already re-exported from the `PRActionSplitButton` barrel (which is already imported just above), a single top-level import is cleaner and avoids the repeated inline syntax.
```suggestion
const onPickTarget = (
target: AgentTarget,
) => {
onValueChange(
target.kind === "existing"
? `existing:${target.terminalId}`
: `new:${target.configId}`,
);
};
const splitButtonProps = {
sessions,
configs,
selectedValue,
resolvedTarget,
onPickTarget,
onSubmit: (target: AgentTarget | null) => submit({ state, target }),
};
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/plans/20260528-1500-pr-button-agent-select.md`:
- Line 28: Line 28 begins with the token "`#4966`" which is being parsed as a
Markdown heading; update that token to avoid a heading by prefixing it with text
(for example change "`#4966`) gives us the pattern we want to reuse:" to "PR
`#4966`) gives us the pattern we want to reuse:" or equivalent) so the line is
treated as normal text rather than a heading.
- Around line 42-46: Update the Markdown fenced code blocks that render ASCII UI
(e.g., the block containing "┌──────────────────┬───┐ │ 📥 Create PR │ ▾ │
└──────────────────┴───┘") to include an explicit language identifier (use
"text") after the opening backticks so they become ```text; do the same for the
other fenced blocks mentioned in the review (the blocks around the other
ASCII/UI sections) to satisfy MD040.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRActionSplitButton/components/PRAgentPickerMenu/PRAgentPickerMenu.tsx:
- Around line 91-101: The menu is hardcoding placement:"split-pane" when calling
onPickTarget in the DropdownMenuItem callback (for config.id), which overrides
the persisted default; change the call in PRAgentPickerMenu so it forwards the
current placement from the parent (or omits placement to let
usePRActionAgentTarget resolve it) instead of the literal "split-pane" — update
the onSelect handler that calls onPickTarget({ kind: "new", configId: config.id,
placement: ... }) to use the placement prop or resolvedTarget passed into
PRAgentPickerMenu rather than the hardcoded string.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/PRActionHeader.tsx:
- Around line 198-200: In PRActionHeader, the unavailable/error tooltip text
rendered via TooltipContent (and produced by unavailableTooltip(reason)) must be
selectable; update the TooltipContent usage in PRActionHeader.tsx to include
explicit select-text and cursor-text classes (e.g., via the component's
className prop) so the tooltip text becomes copyable when renderer-level
selection is disabled—if TooltipContent does not accept className, wrap
unavailableTooltip(reason) in a span/div with className="select-text
cursor-text" inside TooltipContent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 60946a07-b2a8-4b25-9dc1-9139e04afc9e
📒 Files selected for processing (23)
apps/desktop/plans/20260528-1500-pr-button-agent-select.mdapps/desktop/src/renderer/hooks/agents/useAgentTarget/index.tsapps/desktop/src/renderer/hooks/agents/useAgentTarget/useAgentTarget.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/WorkspaceSidebar.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/PRActionHeader.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRActionSplitButton/PRActionSplitButton.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRActionSplitButton/components/PRAgentPickerMenu/PRAgentPickerMenu.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRActionSplitButton/components/PRAgentPickerMenu/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRActionSplitButton/hooks/usePRActionAgentTarget/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRActionSplitButton/hooks/usePRActionAgentTarget/usePRActionAgentTarget.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRActionSplitButton/hooks/usePRActionDispatch/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRActionSplitButton/hooks/usePRActionDispatch/usePRActionDispatch.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRActionSplitButton/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/utils/buildPRContext/buildPRContext.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/utils/getPRFlowState/getPRFlowState.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/utils/getPRFlowState/getPRFlowState.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/usePRFlowDispatch/usePRFlowDispatch.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/usePRFlowDispatch/usePRFlowDispatch.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useCreateNewAgentSession/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useCreateNewAgentSession/useCreateNewAgentSession.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/components/AgentCommentComposer/hooks/useDiffCommentTarget/useDiffCommentTarget.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx
| no agent handoff. | ||
|
|
||
| The DiffPane comment composer (`AgentCommentComposer`, shipped in | ||
| #4966) gives us the pattern we want to reuse: |
Contributor
There was a problem hiding this comment.
Fix unintended heading token at Line 28.
#4966 at line start is parsed as a heading marker. Prefix it with text (for example, PR #4966``) to avoid malformed markdown structure.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 28-28: No space after hash on atx style heading
(MD018, no-missing-space-atx)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/plans/20260528-1500-pr-button-agent-select.md` at line 28, Line
28 begins with the token "`#4966`" which is being parsed as a Markdown heading;
update that token to avoid a heading by prefixing it with text (for example
change "`#4966`) gives us the pattern we want to reuse:" to "PR `#4966`) gives us
the pattern we want to reuse:" or equivalent) so the line is treated as normal
text rather than a heading.
Comment on lines
+42
to
+46
| ``` | ||
| ┌──────────────────┬───┐ | ||
| │ 📥 Create PR │ ▾ │ | ||
| └──────────────────┴───┘ | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add languages to fenced code blocks.
The fenced blocks are missing language identifiers, which triggers markdown lint (MD040). Add explicit languages (for example, text) to each fence.
Also applies to: 80-91, 198-220
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 42-42: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/plans/20260528-1500-pr-button-agent-select.md` around lines 42 -
46, Update the Markdown fenced code blocks that render ASCII UI (e.g., the block
containing "┌──────────────────┬───┐ │ 📥 Create PR │ ▾ │
└──────────────────┴───┘") to include an explicit language identifier (use
"text") after the opening backticks so they become ```text; do the same for the
other fenced blocks mentioned in the review (the blocks around the other
ASCII/UI sections) to satisfy MD040.
Comment on lines
+91
to
+101
| <DropdownMenuItem | ||
| key={config.id} | ||
| onSelect={() => | ||
| onPickTarget({ | ||
| kind: "new", | ||
| configId: config.id, | ||
| // Placement is the persisted default — picked up by | ||
| // the parent's usePRActionAgentTarget. The menu | ||
| // itself doesn't surface a toggle (one-click flow). | ||
| placement: "split-pane", | ||
| }) |
Contributor
There was a problem hiding this comment.
Don’t hardcode new-session placement in menu picks.
This always submits "split-pane" and can ignore a persisted placement default (new-tab). It also conflicts with the comment at Lines 97–99. Pass the current placement from parent (or submit using resolved target after selection) instead of hardcoding here.
Suggested direction
interface PRAgentPickerMenuProps {
sessions: TerminalAgentBinding[];
configs: HostAgentConfig[];
value: string | null;
+ placement: AgentSessionPlacement;
onPickTarget: (target: AgentTarget) => void;
}
@@
- onPickTarget({
+ onPickTarget({
kind: "new",
configId: config.id,
- placement: "split-pane",
+ placement,
})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRActionSplitButton/components/PRAgentPickerMenu/PRAgentPickerMenu.tsx
around lines 91 - 101, The menu is hardcoding placement:"split-pane" when
calling onPickTarget in the DropdownMenuItem callback (for config.id), which
overrides the persisted default; change the call in PRAgentPickerMenu so it
forwards the current placement from the parent (or omits placement to let
usePRActionAgentTarget resolve it) instead of the literal "split-pane" — update
the onSelect handler that calls onPickTarget({ kind: "new", configId: config.id,
placement: ... }) to use the placement prop or resolvedTarget passed into
PRAgentPickerMenu rather than the hardcoded string.
Comment on lines
+198
to
+200
| <TooltipContent side="bottom"> | ||
| {unavailableTooltip(reason)} | ||
| </TooltipContent> |
Contributor
There was a problem hiding this comment.
Make unavailable tooltip text selectable.
Line 198 renders unavailable/error text without select-text cursor-text, so users can’t select/copy it when renderer-level selection is disabled.
Proposed fix
- <TooltipContent side="bottom">
+ <TooltipContent side="bottom" className="select-text cursor-text">
{unavailableTooltip(reason)}
</TooltipContent>As per coding guidelines: apps/desktop/**/*.{tsx,jsx}: Error text must be selectable by users with explicit select-text cursor-text classes; renderer sets user-select: none on body.
📝 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.
Suggested change
| <TooltipContent side="bottom"> | |
| {unavailableTooltip(reason)} | |
| </TooltipContent> | |
| <TooltipContent side="bottom" className="select-text cursor-text"> | |
| {unavailableTooltip(reason)} | |
| </TooltipContent> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/PRActionHeader.tsx
around lines 198 - 200, In PRActionHeader, the unavailable/error tooltip text
rendered via TooltipContent (and produced by unavailableTooltip(reason)) must be
selectable; update the TooltipContent usage in PRActionHeader.tsx to include
explicit select-text and cursor-text classes (e.g., via the component's
className prop) so the tooltip text becomes copyable when renderer-level
selection is disabled—if TooltipContent does not accept className, wrap
unavailableTooltip(reason) in a span/div with className="select-text
cursor-text" inside TooltipContent.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 23 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/utils/buildPRContext/buildPRContext.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/utils/buildPRContext/buildPRContext.ts:128">
P2: `pr-exists` preconditions miss the no-upstream publish case, so the agent may omit publishing the branch before PR update steps.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRActionSplitButton/components/PRAgentPickerMenu/PRAgentPickerMenu.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRActionSplitButton/components/PRAgentPickerMenu/PRAgentPickerMenu.tsx:100">
P2: New-session picks force `placement: "split-pane"`, which can bypass persisted placement and make dropdown submits behave differently from the resolved default target.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRActionSplitButton/PRActionSplitButton.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRActionSplitButton/PRActionSplitButton.tsx:59">
P1: Handle rejected `onSubmit` promises in click handlers to avoid unhandled promise rejections.
(Based on your team's feedback about handling async calls and errors explicitly.) [FEEDBACK_USED]</violation>
</file>
Architecture diagram
sequenceDiagram
participant User as User
participant UI as PRActionSplitButton
participant Dropdown as PRAgentPickerMenu
participant PickHook as usePRActionAgentTarget
participant Dispatch as usePRActionDispatch
participant CreateSession as useCreateNewAgentSession
participant TerminalBindings as Terminal Bindings
participant AgentConfigs as Agent Configs
participant FlowDispatch as PRFlowDispatch (Legacy)
participant TerminalAgent as Existing Terminal Agent
participant HostService as Host Service
Note over User,HostService: PR Action Button Agent Picker Flow
User->>UI: Click primary button (last picked agent)
UI->>PickHook: Get resolvedAgentTarget
alt resolvedTarget is null
UI->>Dispatch: submit(null)
Dispatch->>FlowDispatch: Legacy chat tab flow
FlowDispatch->>FlowDispatch: /pr/create-pr or /pr/update-pr
else resolvedTarget.kind is "existing"
UI->>Dispatch: submit(existingTarget)
Dispatch->>TerminalAgent: sendToTerminalAgent(inlined prompt)
TerminalAgent-->>Dispatch: Success
else resolvedTarget.kind is "new"
UI->>Dispatch: submit(newTarget)
Dispatch->>CreateSession: createNewAgentSession(configId, placement, prompt)
CreateSession->>HostService: workspaceTrpc.agents.run(configId, prompt)
HostService-->>CreateSession: terminal session result
alt placement is "split-pane" and active tab exists
CreateSession->>CreateSession: addPane to active tab
else
CreateSession->>CreateSession: addTab with new pane
end
CreateSession-->>Dispatch: { terminalId }
end
Note over User,HostService: Agent Picker Dropdown Flow
User->>UI: Click chevron dropdown
UI->>Dropdown: Open PRAgentPickerMenu
Dropdown->>TerminalBindings: Get active sessions (sorted by lastEventAt)
TerminalBindings-->>Dropdown: Active sessions list
Dropdown->>AgentConfigs: Get available HostAgentConfigs
AgentConfigs-->>Dropdown: Configs list
alt has sessions
Dropdown-->>User: Show "Active sessions" group
User->>Dropdown: Pick existing session
else has configs
Dropdown-->>User: Show "Start new" group
User->>Dropdown: Pick new config
else no sessions and no configs
Dropdown-->>User: Show "No agents configured" disabled item
end
Dropdown->>PickHook: onPickTarget(target)
PickHook->>PickHook: Persist to localStorage
Dropdown->>UI: Submit with picked target
Note over PickHook,FlowDispatch: Target Resolution Logic
Note over PickHook: Priority: last picked terminal > most recent active > last picked config > first config
alt last picked session is alive
PickHook->>PickHook: Use existing session
else no sessions, last picked config exists
PickHook->>PickHook: Use config (new session)
else no data
PickHook->>PickHook: Return null (fallback to legacy chat)
end
Note over Dispatch,FlowDispatch: Dispatch Routing Logic
alt target is null
Dispatch->>FlowDispatch: Legacy chat tab with slah command + pr-context.md
else target.kind is "existing"
Dispatch->>TerminalAgent: Send inlined slah command + context via xterm
else target.kind is "new"
alt onClickNewAgentSession exists
Dispatch->>CreateSession: Launch new preset with seed prompt
else
Dispatch->>Dispatch: toast.error("Couldn't start agent session")
end
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| busy = false, | ||
| }: PRActionSplitButtonProps) { | ||
| const copy = labels(kind, busy); | ||
| const primaryHandler = () => void onSubmit(resolvedTarget); |
Contributor
There was a problem hiding this comment.
P1: Handle rejected onSubmit promises in click handlers to avoid unhandled promise rejections.
(Based on your team's feedback about handling async calls and errors explicitly.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRActionSplitButton/PRActionSplitButton.tsx, line 59:
<comment>Handle rejected `onSubmit` promises in click handlers to avoid unhandled promise rejections.
(Based on your team's feedback about handling async calls and errors explicitly.) </comment>
<file context>
@@ -0,0 +1,133 @@
+ busy = false,
+}: PRActionSplitButtonProps) {
+ const copy = labels(kind, busy);
+ const primaryHandler = () => void onSubmit(resolvedTarget);
+ const handlePick = (target: AgentTarget) => {
+ onPickTarget(target);
</file context>
Comment on lines
+128
to
+130
| if (sync.hasUpstream && sync.pushCount > 0) { | ||
| preconditions.push("- Push unpushed commits."); | ||
| } |
Contributor
There was a problem hiding this comment.
P2: pr-exists preconditions miss the no-upstream publish case, so the agent may omit publishing the branch before PR update steps.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/utils/buildPRContext/buildPRContext.ts, line 128:
<comment>`pr-exists` preconditions miss the no-upstream publish case, so the agent may omit publishing the branch before PR update steps.</comment>
<file context>
@@ -77,6 +83,75 @@ function renderNoPR(sync: BranchSyncStatus): string {
+ if (sync.hasUncommitted) {
+ preconditions.push("- Commit or stash uncommitted changes.");
+ }
+ if (sync.hasUpstream && sync.pushCount > 0) {
+ preconditions.push("- Push unpushed commits.");
+ }
</file context>
Suggested change
| if (sync.hasUpstream && sync.pushCount > 0) { | |
| preconditions.push("- Push unpushed commits."); | |
| } | |
| if (!sync.hasUpstream) { | |
| preconditions.push("- Publish the branch (`git push -u origin <branch>`)."); | |
| } else if (sync.pushCount > 0) { | |
| preconditions.push("- Push unpushed commits."); | |
| } |
| // Placement is the persisted default — picked up by | ||
| // the parent's usePRActionAgentTarget. The menu | ||
| // itself doesn't surface a toggle (one-click flow). | ||
| placement: "split-pane", |
Contributor
There was a problem hiding this comment.
P2: New-session picks force placement: "split-pane", which can bypass persisted placement and make dropdown submits behave differently from the resolved default target.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRActionSplitButton/components/PRAgentPickerMenu/PRAgentPickerMenu.tsx, line 100:
<comment>New-session picks force `placement: "split-pane"`, which can bypass persisted placement and make dropdown submits behave differently from the resolved default target.</comment>
<file context>
@@ -0,0 +1,162 @@
+ // Placement is the persisted default — picked up by
+ // the parent's usePRActionAgentTarget. The menu
+ // itself doesn't surface a toggle (one-click flow).
+ placement: "split-pane",
+ })
+ }
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Turns the top-right v2 Create / Update PR control into a split button with a working agent picker. The primary region runs the last-picked agent; the chevron dropdown lets you switch where PR authoring is handed off — a running terminal session or a freshly launched preset — without losing the one-click default. This mirrors the agent-pick affordance shipped on the DiffPane comment composer (#4966).
Two phases land here:
29a20e127) — bordered split-button shell with all-states routing (create / update / busy / spinner), replacing the icon-only button.a7c69ef29) — the real agent picker dropdown and dispatch routing.What changed (phase 2)
useAgentTargethook (renderer/hooks/agents/useAgentTarget) — selection state + localStorage persistence + validation/fallbacks, extracted from the comment composer.useDiffCommentTargetand the newusePRActionAgentTargetare now thin wrappers with surface-scoped storage keys, so PR picks and comment picks don't trample each other.useCreateNewAgentSessionhook — pulled out ofusePaneRegistryso both the DiffPane composer and the PR action header launch new preset sessions through one code path.PRAgentPickerMenu—DropdownMenurendition of the picker, grouped "Active sessions" + "Start new" with preset icons, matchingAgentPickerSelect. One-click = pick + submit.usePRActionDispatch— routes a submit by target kind:/pr/*slash command +pr-context.md)sendToTerminalAgentwith the slash command + pr-context inlined (xterm can't carry separate attachments)useCreateNewAgentSessionseeded with the same inlined promptcreatePREnabledkill-switch — the split button is the default now.Test plan
bun run lint— cleanbun run typecheck --filter=@superset/desktop— passesbun testforhooks/agents+PRActionHeader— 32 passPlan:
apps/desktop/plans/20260528-1500-pr-button-agent-select.md(phase 3 — per-project custom prompt — still pending).Summary by cubic
Adds a split “Create/Update PR” button with an agent picker to the v2 workspace header. One click runs the last-picked agent; the dropdown lets you send the PR flow to an existing terminal session or start a new preset, with a chat-tab fallback if no agent is picked.
New Features
pr-context.md; existing session → send inlined prompt to terminal; new preset → launch session seeded with the same prompt./pr/update-pr);pr-context.mdnow includes existing PR details and branch sync info.Refactors
useAgentTargethook;useDiffCommentTargetandusePRActionAgentTargetwrap it with surface-scoped storage keys.useCreateNewAgentSessionhook extracted fromusePaneRegistryfor launching terminal agents.usePRActionDispatchto route submits by target; addsPRAgentPickerMenufor the dropdown.selectActionButtonreturnsupdate-pr-dropdown; tests updated accordingly.Written for commit a7c69ef. Summary will update on new commits.
Review in cubic
Summary by CodeRabbit
Release Notes