feat(desktop): scaffold PR action header in v2 sidebar (gated)#3777
Merged
AviPeltz merged 9 commits intoApr 27, 2026
Conversation
Wires the cherry-picked wip into v2 alongside the recent main refactor: restores `v2UserPreferencesSchema` + `pendingWorkspaceSchema` fields the wip dropped, points the sidebar-open read at user prefs (where the toggle hotkey writes), adds `focusRequestId` route param + `getDefaultBranchName` import the wip referenced without declaring, and drops the removed `changesSubtab` write. PR action header rendering is gated behind a `PR_ACTION_HEADER_ENABLED = false` constant — empty bar still renders so sidebar layout stays consistent. Flip to true once chat lands in v2 and the slash-command flow is verified end-to-end.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an agent-driven PR creation flow: new agent command docs, PR flow state machine and selectors, UI components (PRActionHeader, PRStatusGroup, PRDetailCard), hooks to build/send Changes
Sequence DiagramsequenceDiagram
participant User
participant Sidebar as WorkspaceSidebar
participant StateHook as usePRFlowState
participant PRHeader as PRActionHeader
participant Dispatch as usePRFlowDispatch
participant Chat as Chat Pane
participant Agent as PR Agent
participant Git as Git CLI
participant Backend as TRPC
User->>Sidebar: view workspace
Sidebar->>StateHook: request flowState(workspaceId)
StateHook->>Backend: getPullRequest(), getBranchSyncStatus()
Backend-->>StateHook: PR + sync data
StateHook-->>Sidebar: PRFlowState
Sidebar->>PRHeader: render(state)
User->>PRHeader: click "Create PR"
PRHeader->>Dispatch: dispatch(state, draft?)
Dispatch->>Chat: open chat with initialPrompt and attachment (pr-context.md)
Chat->>Agent: user runs /pr/create-pr
Agent->>Chat: read pr-context.md
Agent->>Git: run commit/push commands per preconditions
Git-->>Agent: command results
Agent->>Backend: create PR (gh or API via TRPC)
Backend-->>Agent: PR created (URL)
Agent->>Chat: report PR URL
Chat->>User: display PR link
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Greptile SummaryThis PR cherry-picks and integrates the WIP PR-action-header scaffold (state machine, dispatcher,
Confidence Score: 4/5Safe to merge for the feature-flag-gated PR scaffold, but the One P1 regression: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx —
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx | Major refactor: inlines several hooks, migrates sidebar to a portal, adds openChatPane; contains a P1 bug where revealPath writes rightSidebarOpen to workspace-local state but sidebar visibility reads from user prefs |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/WorkspaceSidebar.tsx | Refactored: splits Changes/Review/Files into separate top-level tabs, adds hysteresis to compact breakpoint, wires usePRFlowState/usePRFlowDispatch, gates PRActionHeader behind feature flag |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/utils/getPRFlowState/getPRFlowState.ts | New pure state reducer + selectors for the PR action header; well-structured, exhaustive switch cases, correctly scoped to MVP UnavailableReason variants |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/usePRFlowDispatch/usePRFlowDispatch.ts | New hook that builds and dispatches the chat launch config; logic is correct, but encodeAsDataUrl uses deprecated unescape() |
| packages/host-service/src/trpc/router/git/git.ts | Adds getBranchSyncStatus tRPC procedure; correctly parses git rev-list --left-right --count @{upstream}...HEAD (left=pull, right=push) |
| apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.ts | Migrates changesSubtab to activeTab (3-way enum), adds rightSidebarOpen and rightSidebarWidth fields; rightSidebarOpen is written by revealPath in page.tsx but never read for sidebar visibility |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/layout.tsx | Adds #workspace-right-sidebar-slot div for the portal target, removes migration hooks, restructures layout nesting |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/PRActionHeader.tsx | New gated UI component; renders correctly from PRFlowState selectors; split-button and disabled-tooltip cases are implemented cleanly |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/utils/buildPRContext/buildPRContext.ts | New pure function that generates the pr-context.md markdown attachment; well-tested with snapshot assertions for all relevant branch states |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["User clicks 'Create PR' or 'Create draft PR'"] --> B["PRActionHeader\n(gated: PR_ACTION_HEADER_ENABLED)"]
B --> C["usePRFlowDispatch\nplanDispatch(state, {draft})"]
C --> D["buildPRContext(state)\n→ pr-context.md markdown"]
D --> E["encodeAsDataUrl()\ndata:text/markdown;base64,..."]
E --> F["onOpenChat(launchConfig)\ninitialPrompt: /pr/create-pr [--draft]"]
F --> G["store.addTab\n{kind: chat, data: {launchConfig}}"]
G --> H["Chat pane opens\nAgent reads pr-context.md"]
H --> I["Agent: commit/push/gh pr create"]
J["usePRFlowState(workspaceId)"] --> K["workspaceTrpc.git.getPullRequest"]
J --> L["workspaceTrpc.git.getBranchSyncStatus"]
K & L --> M["getPRFlowState()\npure reducer"]
M --> N["PRFlowState\nloading / unavailable / no-pr / pr-exists / busy / error"]
N --> B
subgraph Selectors
N --> O["selectActionButton"]
N --> P["selectPRLink"]
N --> Q["selectStatusBadge"]
end
Comments Outside Diff (1)
-
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx, line 393-396 (link)Portal target queried once — no retry if DOM not ready
document.getElementById("workspace-right-sidebar-slot")runs in a one-timeuseEffectwith no dependency array. If the layout component defers rendering the slot div (e.g., due to a concurrent render or a future conditional),sidebarSlotElstaysnulland the sidebar portal never mounts with no error or fallback. Consider a smallMutationObserverguard or a shared ref to the slot element passed through context so the lookup is guaranteed to succeed.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: 393-396 Comment: **Portal target queried once — no retry if DOM not ready** `document.getElementById("workspace-right-sidebar-slot")` runs in a one-time `useEffect` with no dependency array. If the layout component defers rendering the slot div (e.g., due to a concurrent render or a future conditional), `sidebarSlotEl` stays `null` and the sidebar portal never mounts with no error or fallback. Consider a small `MutationObserver` guard or a shared ref to the slot element passed through context so the lookup is guaranteed to succeed. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All 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: 208-213
Comment:
**`revealPath` writes to the wrong state tree — sidebar won't open**
`revealPath` sets `draft.rightSidebarOpen = true` on the *workspace local state* (via `collections.v2WorkspaceLocalState.update`), but sidebar visibility is controlled by `v2UserPreferences.rightSidebarOpen` (user prefs). Because these are two independent state trees, clicking "reveal in sidebar" will update a value that nobody reads for sidebar visibility, so the sidebar stays closed.
The old code called `setRightSidebarOpen(true)` from `useV2UserPreferences`, which wrote to the correct store. Either switch back to that call here, or update the sidebar open reader to consume the workspace-local field that is already being written.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/usePRFlowDispatch/usePRFlowDispatch.ts
Line: 70-76
Comment:
**`unescape` is deprecated — use `TextEncoder` for UTF-8 base64**
`unescape(encodeURIComponent(content))` encodes non-ASCII characters correctly but relies on the deprecated `unescape` global (removed from the WHATWG specification; may trigger linter errors or warnings). Branch names and commit messages can easily contain non-ASCII characters. The `TextEncoder` API is available in both browsers and Node 18+:
```ts
function encodeAsDataUrl(content: string, mediaType: string): string {
const bytes = new TextEncoder().encode(content);
const binary = Array.from(bytes, (b) => String.fromCharCode(b)).join("");
const base64 = btoa(binary);
return `data:${mediaType};base64,${base64}`;
}
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx
Line: 393-396
Comment:
**Portal target queried once — no retry if DOM not ready**
`document.getElementById("workspace-right-sidebar-slot")` runs in a one-time `useEffect` with no dependency array. If the layout component defers rendering the slot div (e.g., due to a concurrent render or a future conditional), `sidebarSlotEl` stays `null` and the sidebar portal never mounts with no error or fallback. Consider a small `MutationObserver` guard or a shared ref to the slot element passed through context so the lookup is guaranteed to succeed.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "chore(desktop): integrate PR action head..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+208
to
+213
| } | ||
| state.openPane({ | ||
| pane: { | ||
| kind: "file", | ||
| data: { | ||
| filePath, |
There was a problem hiding this comment.
revealPath writes to the wrong state tree — sidebar won't open
revealPath sets draft.rightSidebarOpen = true on the workspace local state (via collections.v2WorkspaceLocalState.update), but sidebar visibility is controlled by v2UserPreferences.rightSidebarOpen (user prefs). Because these are two independent state trees, clicking "reveal in sidebar" will update a value that nobody reads for sidebar visibility, so the sidebar stays closed.
The old code called setRightSidebarOpen(true) from useV2UserPreferences, which wrote to the correct store. Either switch back to that call here, or update the sidebar open reader to consume the workspace-local field that is already being written.
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: 208-213
Comment:
**`revealPath` writes to the wrong state tree — sidebar won't open**
`revealPath` sets `draft.rightSidebarOpen = true` on the *workspace local state* (via `collections.v2WorkspaceLocalState.update`), but sidebar visibility is controlled by `v2UserPreferences.rightSidebarOpen` (user prefs). Because these are two independent state trees, clicking "reveal in sidebar" will update a value that nobody reads for sidebar visibility, so the sidebar stays closed.
The old code called `setRightSidebarOpen(true)` from `useV2UserPreferences`, which wrote to the correct store. Either switch back to that call here, or update the sidebar open reader to consume the workspace-local field that is already being written.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+70
to
+76
| default: | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| function encodeAsDataUrl(content: string, mediaType: string): string { | ||
| const base64 = |
There was a problem hiding this comment.
unescape is deprecated — use TextEncoder for UTF-8 base64
unescape(encodeURIComponent(content)) encodes non-ASCII characters correctly but relies on the deprecated unescape global (removed from the WHATWG specification; may trigger linter errors or warnings). Branch names and commit messages can easily contain non-ASCII characters. The TextEncoder API is available in both browsers and Node 18+:
function encodeAsDataUrl(content: string, mediaType: string): string {
const bytes = new TextEncoder().encode(content);
const binary = Array.from(bytes, (b) => String.fromCharCode(b)).join("");
const base64 = btoa(binary);
return `data:${mediaType};base64,${base64}`;
}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/hooks/usePRFlowDispatch/usePRFlowDispatch.ts
Line: 70-76
Comment:
**`unescape` is deprecated — use `TextEncoder` for UTF-8 base64**
`unescape(encodeURIComponent(content))` encodes non-ASCII characters correctly but relies on the deprecated `unescape` global (removed from the WHATWG specification; may trigger linter errors or warnings). Branch names and commit messages can easily contain non-ASCII characters. The `TextEncoder` API is available in both browsers and Node 18+:
```ts
function encodeAsDataUrl(content: string, mediaType: string): string {
const bytes = new TextEncoder().encode(content);
const binary = Array.from(bytes, (b) => String.fromCharCode(b)).join("");
const base64 = btoa(binary);
return `data:${mediaType};base64,${base64}`;
}
```
How can I resolve this? If you propose a fix, please make it concise.
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/WorkspaceSidebar.tsx (1)
122-155:⚠️ Potential issue | 🟠 MajorPR-flow hooks poll even when the feature flag is off.
usePRFlowStateissues two tRPC queries (git.getPullRequest,git.getBranchSyncStatus) withrefetchInterval: 10_000andrefetchOnWindowFocus: true, and runs unconditionally here. WithPR_ACTION_HEADER_ENABLED = false(the shipping state of this PR), the data anddispatchare never used, but every open v2 workspace will still hit both procedures every 10 seconds. Given the PR description notes the gate exists specifically because the chat pane it dispatches to doesn't yet exist in v2, the polling is pure overhead until the flag flips.Two cheap options:
- Plumb the flag into
usePRFlowStateand pass it through to each query'senabled(e.g.,enabled: PR_ACTION_HEADER_ENABLED && !!workspaceId), and short-circuitusePRFlowDispatch's callers behind the same flag.- Or, only call these hooks from inside
PRActionHeaderand lift the flag check above the hook calls in this component.Hooks rules are still satisfied as long as the flag is a stable module constant.
Sketch of option 1
- const { flowState, onRetry } = usePRFlowState(workspaceId); - const dispatch = usePRFlowDispatch({ - onOpenChat: onOpenChat ?? (() => {}), - }); + const { flowState, onRetry } = usePRFlowState(workspaceId, { + enabled: PR_ACTION_HEADER_ENABLED, + }); + const dispatch = usePRFlowDispatch({ + onOpenChat: onOpenChat ?? (() => {}), + });…with
usePRFlowStateforwardingenabledinto bothuseQuerycalls.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/WorkspaceSidebar.tsx around lines 122 - 155, usePRFlowState and usePRFlowDispatch are called unconditionally causing git.getPullRequest and git.getBranchSyncStatus to poll even when PR_ACTION_HEADER_ENABLED is false; either pass the flag into usePRFlowState (e.g., add an enabled parameter and forward it to both useQuery calls for git.getPullRequest and git.getBranchSyncStatus as enabled: PR_ACTION_HEADER_ENABLED && !!workspaceId) and short-circuit usePRFlowDispatch callers when the flag is false, or lift the PR_ACTION_HEADER_ENABLED check above the hooks and only call usePRFlowState/usePRFlowDispatch inside PRActionHeader (move hook calls into PRActionHeader) so polling only runs when the feature flag is true; reference usePRFlowState, usePRFlowDispatch, PRActionHeader, PR_ACTION_HEADER_ENABLED, workspaceId, git.getPullRequest, and git.getBranchSyncStatus when making the change.
🧹 Nitpick comments (7)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/SidebarHeader/SidebarHeader.tsx (1)
25-60: Duplicate non-compact button render —btnis unused whencompactis false.
btnis constructed for every tab (lines 25–37) but is only consumed inside thecompactbranch'sTooltipTrigger. The non-compact branch (lines 50–60) re-renders an essentially identical button instead of just returningbtn. This is what forced thesize-3icon update to be applied in two places in this PR (lines 34 and 57) and will keep doubling future class/handler edits.♻️ Suggested simplification
- {tabs.map((tab) => { - const isActive = activeTab === tab.id; - const btn = ( - <button - type="button" - onClick={() => onTabChange(tab.id)} - className={getSidebarHeaderTabButtonClassName({ - isActive, - compact, - })} - > - {tab.icon && <tab.icon className="size-3" />} - {!compact && tab.label} - </button> - ); - - if (compact) { - return ( - <Tooltip key={tab.id}> - <TooltipTrigger asChild>{btn}</TooltipTrigger> - <TooltipContent side="bottom" showArrow={false}> - {tab.label} - </TooltipContent> - </Tooltip> - ); - } - - return ( - <button - key={tab.id} - type="button" - onClick={() => onTabChange(tab.id)} - className={getSidebarHeaderTabButtonClassName({ isActive })} - > - {tab.icon && <tab.icon className="size-3" />} - {tab.label} - </button> - ); - })} + {tabs.map((tab) => { + const isActive = activeTab === tab.id; + const btn = ( + <button + key={tab.id} + type="button" + onClick={() => onTabChange(tab.id)} + className={getSidebarHeaderTabButtonClassName({ + isActive, + compact, + })} + > + {tab.icon && <tab.icon className="size-3" />} + {!compact && tab.label} + </button> + ); + + if (compact) { + return ( + <Tooltip key={tab.id}> + <TooltipTrigger asChild>{btn}</TooltipTrigger> + <TooltipContent side="bottom" showArrow={false}> + {tab.label} + </TooltipContent> + </Tooltip> + ); + } + + return btn; + })}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/components/SidebarHeader/SidebarHeader.tsx around lines 25 - 60, The code builds a button into the local variable btn but then duplicates the same button in the non-compact return, causing drift and duplicated edits; instead reuse btn for both branches. Change the btn declaration in SidebarHeader.tsx to include the key prop (e.g., add key={tab.id} to the button created in btn) and return btn directly in the non-compact branch (remove the duplicated button JSX); keep using TooltipTrigger asChild and TooltipContent for the compact branch, and ensure onTabChange, getSidebarHeaderTabButtonClassName, tab.icon and tab.label remain unchanged..agents/commands/pr/create-pr.md (1)
65-70: Add a language to thegh pr createfenced block (MD040).markdownlint is flagging the unlabeled fence. Use
bash(orsh) to satisfy the rule and get proper highlighting.📝 Proposed fix
-``` +```bash gh pr create \ --base <defaultBranch> \ --title "<title>" \ --body "<body>"</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In @.agents/commands/pr/create-pr.md around lines 65 - 70, The fenced code block
showing the gh pr create command is unlabeled and triggers markdownlint MD040;
update that code fence in .agents/commands/pr/create-pr.md by adding a language
tag (e.g., bash) to the opening triple-backtick so the block becomes ```bash and
provides proper syntax highlighting for the gh pr create snippet.</details> </blockquote></details> <details> <summary>apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/WorkspaceSidebar.tsx (1)</summary><blockquote> `80-82`: **Unnecessary `as SidebarTabId` cast.** `workspaceLocalStateSchema.sidebarState.activeTab` is a `z.enum(["changes","files","review"])` with a default, so `localState?.sidebarState?.activeTab` should already be typed as `"changes" | "files" | "review" | undefined` from the inferred row type. The `as SidebarTabId | undefined` assertion can be dropped, which restores actual type checking if the schema enum ever changes. ```diff - const activeTab: SidebarTabId = - (localState?.sidebarState?.activeTab as SidebarTabId | undefined) ?? - "changes"; + const activeTab: SidebarTabId = + localState?.sidebarState?.activeTab ?? "changes"; ``` As per coding guidelines, "Avoid using `any` type; prefer explicit type safety in TypeScript." <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/WorkspaceSidebar.tsx around lines 80 - 82, Remove the unnecessary type assertion on the activeTab expression: drop the "as SidebarTabId | undefined" cast and let the inferred type from localState?.sidebarState?.activeTab (the z.enum union) flow. Keep the nullish coalescing to default to "changes" so the final declaration const activeTab: SidebarTabId = localState?.sidebarState?.activeTab ?? "changes"; remains valid and benefits from actual schema-driven types; update the WorkspaceSidebar's activeTab initialization accordingly. ``` </details> </blockquote></details> <details> <summary>apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/usePRFlowState/usePRFlowState.ts (1)</summary><blockquote> `35-55`: **Drop the type assertions on tRPC query outputs.** `prQuery.data`, `prQuery.error`, and `syncQuery.error` are already typed by tRPC's inferred types. Casting them to `FlowPullRequest | null` / `Error | null` masks any future schema drift between `git.getPullRequest`'s server type and `FlowPullRequest`, and `prQuery.error` is a `TRPCClientErrorLike`, not a plain `Error` — the `as Error` cast claims a type the value may not satisfy. Prefer either (a) defining `FlowPullRequest` as the inferred output of `git.getPullRequest` so no cast is needed, or (b) writing a small narrowing function that maps the tRPC payload into `FlowPullRequest`. For errors, accept `unknown`/`TRPCClientErrorLike` in `getPRFlowState`'s `loadError` field rather than asserting `Error`. As per coding guidelines, "Avoid using `any` type; prefer explicit type safety in TypeScript." <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/usePRFlowState/usePRFlowState.ts around lines 35 - 55, The current usePRFlowState hook uses unsafe casts (prQuery.data as FlowPullRequest | null and prQuery.error as Error | null and syncQuery.error as Error | null); remove these assertions and pass the raw tRPC outputs (prQuery.data, prQuery.error, syncQuery.error) into getPRFlowState, and either (a) change getPRFlowState's parameter types (e.g., allow pr?: FlowPullRequest | undefined or the tRPC-inferred output type and change loadError to unknown | TRPCClientErrorLike) or (b) add a small mapper function (e.g., normalizePR and normalizeError) that explicitly converts/narrows tRPC results into FlowPullRequest and an Error-like shape before calling getPRFlowState; update references to getPRFlowState and usePRFlowState accordingly so no `as` assertions remain. ``` </details> </blockquote></details> <details> <summary>apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/utils/getPRFlowState/getPRFlowState.ts (1)</summary><blockquote> `71-91`: **Consider an `assertNever` default for forward-compat.** `selectActionButton`, `selectStatusBadge`, and `getPRFromState` are all exhaustive over the current `PRFlowState` kinds, but adding new kinds (the plan enumerates 12+ post-PR variants for later phases) won't get a compile error here without an `assertNever(state)` default. Optional polish to catch future omissions at type-check time. <details> <summary>♻️ Sketch</summary> ```ts function assertNever(x: never): never { throw new Error(`Unhandled PRFlowState: ${JSON.stringify(x)}`); } // then in each switch: default: return assertNever(state); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/utils/getPRFlowState/getPRFlowState.ts around lines 71 - 91, Add an assertNever guard to the exhaustive switches to catch future PRFlowState variants: implement a helper assertNever(x: never): never that throws, then add a default: return assertNever(state); to the switch statements in selectActionButton as well as selectStatusBadge and getPRFromState so the compiler will error if a new PRFlowState kind is added without handling it. ``` </details> </blockquote></details> <details> <summary>apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/PRActionHeader.tsx (1)</summary><blockquote> `82-195`: **Split `ActionButton` and `CreatePRSplitButton` into their own files.** Per the repo guideline, each component lives in its own file (and ideally its own folder). `PRActionHeader.tsx` currently declares three components. Recommend extracting `ActionButton` and `CreatePRSplitButton` into co-located sibling folders (e.g., `PRActionHeader/components/ActionButton/ActionButton.tsx` + `index.ts`, same for `CreatePRSplitButton`). As per coding guidelines: "Do not create multi-component files; use one component per file." and "Use one folder per component with structure: `ComponentName/ComponentName.tsx` + `index.ts` for barrel export." <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/PRActionHeader.tsx around lines 82 - 195, PRActionHeader currently defines multiple components in one file; extract the ActionButton and CreatePRSplitButton components into their own component folders/files per guideline. Create folders/components ActionButton/ActionButton.tsx and CreatePRSplitButton/CreatePRSplitButton.tsx (with index.ts barrel exports) that export the respective React components (preserving props types: ActionButton expects variant, state, dispatch, onRetry, onCancelBusy; CreatePRSplitButton expects state, dispatch), update PRActionHeader.tsx to import these two components and remove their inline definitions, and keep all JSX and event handlers identical so behavior is unchanged. ``` </details> </blockquote></details> <details> <summary>apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/usePRFlowDispatch/usePRFlowDispatch.ts (1)</summary><blockquote> `75-81`: **Replace deprecated `unescape` with a modern UTF-8 → base64 approach.** `unescape` is an Annex B legacy function and flagged as deprecated. The `unescape(encodeURIComponent(...))` pattern works but is not recommended. In the Electron renderer, use `TextEncoder` + `btoa` (natively available) for the browser path instead: <details> <summary>♻️ Suggested replacement</summary> ```diff function encodeAsDataUrl(content: string, mediaType: string): string { - const base64 = - typeof btoa === "function" - ? btoa(unescape(encodeURIComponent(content))) - : Buffer.from(content, "utf-8").toString("base64"); + let base64: string; + if (typeof btoa === "function") { + const bytes = new TextEncoder().encode(content); + let binary = ""; + for (let i = 0; i < bytes.length; i++) { + binary += String.fromCharCode(bytes[i]); + } + base64 = btoa(binary); + } else { + base64 = Buffer.from(content, "utf-8").toString("base64"); + } return `data:${mediaType};base64,${base64}`; } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/usePRFlowDispatch/usePRFlowDispatch.ts around lines 75 - 81, The encodeAsDataUrl function uses the deprecated unescape(encodeURIComponent(...)) pattern for UTF-8 → base64; replace that with a modern approach: for the browser branch (when typeof btoa === "function") use TextEncoder to encode content to a Uint8Array and convert that byte array to a binary string (e.g., via String.fromCharCode.apply or a small loop) before calling btoa, and keep the existing Buffer.from(content, "utf-8").toString("base64") path for the non-browser branch; preserve the return value format `data:${mediaType};base64,${base64}` and refer to encodeAsDataUrl to implement the change. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@apps/desktop/plans/20260420-1045-agent-driven-pr-flow.md:
- Around line 84-110: The fenced ASCII diagram block needs a language tag to
satisfy markdownlint MD040; update the triple-backtick fence for the diagram
(the block containing PRActionHeader, getPRFlowState, usePRFlowDispatch,
buildPRContext, ensureChatPane, ChatLaunchConfig) fromtotext (or
another appropriate tag) so the diagram is treated as plain text.In
@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/utils/buildPRContext/buildPRContext.ts:
- Around line 60-67: The generated PR command uses sync.defaultBranch and emits
a literal "" when null; in buildPRContext (the function that pushes
lines via lines.push) change the logic so the gh pr create suggestion is only
added when sync.defaultBranch is truthy (use sync.defaultBranch directly in the
template), and when sync.defaultBranch is null/undefined replace that
single-line suggestion with a clear guidance line (e.g., instruct to determine
the base branch with gh repo view --json defaultBranchRef or to set the base
manually) so the output never shows the literal "" and the "Base"
unknown state remains consistent with the command guidance.In
@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/WorkspaceSidebar.tsx:
- Around line 151-155: The placeholder rendered when PR_ACTION_HEADER_ENABLED is
false should be an invisible layout spacer rather than a visible muted/bordered
bar: in the conditional that renders PRActionHeader (the PRActionHeader
component controlled by PR_ACTION_HEADER_ENABLED), replace the div with one that
only reserves space (e.g., className="h-12 shrink-0" or use "h-12 invisible" /
"aria-hidden") and remove the border-b, border-border, bg-muted/45,
dark:bg-muted/35, gap-2 and px-2 classes so it becomes a no-op whitespace spacer
instead of a visible strip.In
@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx:
- Around line 222-231: The revealPath callback currently writes
draft.rightSidebarOpen on collections.v2WorkspaceLocalState (via
collections.v2WorkspaceLocalState.update) but the page now reads sidebarOpen
from v2UserPreferences.rightSidebarOpen, so clicking reveal won't open the
sidebar; change revealPath to call the user-preferences setter returned by
useV2UserPreferences (instead of writing to v2WorkspaceLocalState) to set
rightSidebarOpen = true and keep the existing setSelectedFilePath(path)
behavior; locate the revealPath function and replace the
v2WorkspaceLocalState.update write with the appropriate useV2UserPreferences
setter call to toggle rightSidebarOpen.- Line 226: The update in revealPath calls
v2WorkspaceLocalState.update(workspaceId, ...) and writes
draft.sidebarState.activeTab without ensuring draft.sidebarState exists, which
can throw if the row predates the schema; fix by either (A) adding a guard
before the update—use collections.v2WorkspaceLocalState.get(workspaceId) and
return early if falsy—or inside the updater ensure draft.sidebarState is
initialized (e.g., draft.sidebarState = draft.sidebarState || { projectId: ...,
tabOrder: 0, sectionId: null, changesFilter: { kind: "all" }, activeTab:
"changes" }) before setting activeTab; alternatively (B) add .default({...}) to
the sidebarState field in the v2WorkspaceLocalState schema so the parent object
is always present. Target the revealPath handler, the call to
v2WorkspaceLocalState.update, and the sidebarState definition in the
v2WorkspaceLocalState schema when applying the change.In
@apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.ts:
- Around line 42-43: Remove the dead rightSidebarOpen field from
workspaceLocalStateSchema and stop writing to it in page.tsx: delete the
rightSidebarOpen entry in workspaceLocalStateSchema (the
z.boolean().default(false) line) and remove or update any assignments in
page.tsx that set workspaceLocalState.rightSidebarOpen so that code uses
v2UserPreferences.rightSidebarOpen exclusively; keep workspaceLocalStateSchema
and page.tsx consistent to avoid orphaned writes.
Outside diff comments:
In
@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/WorkspaceSidebar.tsx:
- Around line 122-155: usePRFlowState and usePRFlowDispatch are called
unconditionally causing git.getPullRequest and git.getBranchSyncStatus to poll
even when PR_ACTION_HEADER_ENABLED is false; either pass the flag into
usePRFlowState (e.g., add an enabled parameter and forward it to both useQuery
calls for git.getPullRequest and git.getBranchSyncStatus as enabled:
PR_ACTION_HEADER_ENABLED && !!workspaceId) and short-circuit usePRFlowDispatch
callers when the flag is false, or lift the PR_ACTION_HEADER_ENABLED check above
the hooks and only call usePRFlowState/usePRFlowDispatch inside PRActionHeader
(move hook calls into PRActionHeader) so polling only runs when the feature flag
is true; reference usePRFlowState, usePRFlowDispatch, PRActionHeader,
PR_ACTION_HEADER_ENABLED, workspaceId, git.getPullRequest, and
git.getBranchSyncStatus when making the change.
Nitpick comments:
In @.agents/commands/pr/create-pr.md:
- Around line 65-70: The fenced code block showing the gh pr create command is
unlabeled and triggers markdownlint MD040; update that code fence in
.agents/commands/pr/create-pr.md by adding a language tag (e.g., bash) to the
opening triple-backtick so the block becomes ```bash and provides proper syntax
highlighting for the gh pr create snippet.In
@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/PRActionHeader.tsx:
- Around line 82-195: PRActionHeader currently defines multiple components in
one file; extract the ActionButton and CreatePRSplitButton components into their
own component folders/files per guideline. Create folders/components
ActionButton/ActionButton.tsx and CreatePRSplitButton/CreatePRSplitButton.tsx
(with index.ts barrel exports) that export the respective React components
(preserving props types: ActionButton expects variant, state, dispatch, onRetry,
onCancelBusy; CreatePRSplitButton expects state, dispatch), update
PRActionHeader.tsx to import these two components and remove their inline
definitions, and keep all JSX and event handlers identical so behavior is
unchanged.In
@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/utils/getPRFlowState/getPRFlowState.ts:
- Around line 71-91: Add an assertNever guard to the exhaustive switches to
catch future PRFlowState variants: implement a helper assertNever(x: never):
never that throws, then add a default: return assertNever(state); to the switch
statements in selectActionButton as well as selectStatusBadge and getPRFromState
so the compiler will error if a new PRFlowState kind is added without handling
it.In
@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/SidebarHeader/SidebarHeader.tsx:
- Around line 25-60: The code builds a button into the local variable btn but
then duplicates the same button in the non-compact return, causing drift and
duplicated edits; instead reuse btn for both branches. Change the btn
declaration in SidebarHeader.tsx to include the key prop (e.g., add key={tab.id}
to the button created in btn) and return btn directly in the non-compact branch
(remove the duplicated button JSX); keep using TooltipTrigger asChild and
TooltipContent for the compact branch, and ensure onTabChange,
getSidebarHeaderTabButtonClassName, tab.icon and tab.label remain unchanged.In
@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/usePRFlowDispatch/usePRFlowDispatch.ts:
- Around line 75-81: The encodeAsDataUrl function uses the deprecated
unescape(encodeURIComponent(...)) pattern for UTF-8 → base64; replace that with
a modern approach: for the browser branch (when typeof btoa === "function") use
TextEncoder to encode content to a Uint8Array and convert that byte array to a
binary string (e.g., via String.fromCharCode.apply or a small loop) before
calling btoa, and keep the existing Buffer.from(content,
"utf-8").toString("base64") path for the non-browser branch; preserve the return
value formatdata:${mediaType};base64,${base64}and refer to encodeAsDataUrl
to implement the change.In
@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/usePRFlowState/usePRFlowState.ts:
- Around line 35-55: The current usePRFlowState hook uses unsafe casts
(prQuery.data as FlowPullRequest | null and prQuery.error as Error | null and
syncQuery.error as Error | null); remove these assertions and pass the raw tRPC
outputs (prQuery.data, prQuery.error, syncQuery.error) into getPRFlowState, and
either (a) change getPRFlowState's parameter types (e.g., allow pr?:
FlowPullRequest | undefined or the tRPC-inferred output type and change
loadError to unknown | TRPCClientErrorLike) or (b) add a small mapper function
(e.g., normalizePR and normalizeError) that explicitly converts/narrows tRPC
results into FlowPullRequest and an Error-like shape before calling
getPRFlowState; update references to getPRFlowState and usePRFlowState
accordingly so noasassertions remain.In
@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/WorkspaceSidebar.tsx:
- Around line 80-82: Remove the unnecessary type assertion on the activeTab
expression: drop the "as SidebarTabId | undefined" cast and let the inferred
type from localState?.sidebarState?.activeTab (the z.enum union) flow. Keep the
nullish coalescing to default to "changes" so the final declaration const
activeTab: SidebarTabId = localState?.sidebarState?.activeTab ?? "changes";
remains valid and benefits from actual schema-driven types; update the
WorkspaceSidebar's activeTab initialization accordingly.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `d94e361b-1bc7-42f5-8620-9b734cc0eb61` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between b6db24710d3932e80ed13cc0f281142b5fa79905 and dce8eab08f77e5859ac7fb78c2afda3eb23f6a58. </details> <details> <summary>📒 Files selected for processing (25)</summary> * `.agents/commands/pr/create-pr.md` * `apps/desktop/plans/20260420-1045-agent-driven-pr-flow.md` * `apps/desktop/src/renderer/routes/_authenticated/_dashboard/layout.tsx` * `apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/WorkspaceSidebar.tsx` * `apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/PRActionHeader.tsx` * `apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/index.ts` * `apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/utils/buildPRContext/buildPRContext.test.ts` * `apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/utils/buildPRContext/buildPRContext.ts` * `apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/utils/buildPRContext/index.ts` * `apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/utils/getPRFlowState/getPRFlowState.test.ts` * `apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/utils/getPRFlowState/getPRFlowState.ts` * `apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/utils/getPRFlowState/index.ts` * `apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/SidebarHeader/SidebarHeader.tsx` * `apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesHeader/ChangesHeader.tsx` * `apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/usePRFlowDispatch/index.ts` * `apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/usePRFlowDispatch/usePRFlowDispatch.test.ts` * `apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/usePRFlowDispatch/usePRFlowDispatch.ts` * `apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/usePRFlowState/index.ts` * `apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/usePRFlowState/usePRFlowState.ts` * `apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/useReviewTab.tsx` * `apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx` * `apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/writeSidebarState.ts` * `apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.ts` * `apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/headerTabStyles.ts` * `packages/host-service/src/trpc/router/git/git.ts` </details> <details> <summary>💤 Files with no reviewable changes (1)</summary> * apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/writeSidebarState.ts </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+84
to
+110
| ``` | ||
| ┌─────────────────────────────────────────┐ | ||
| │ PRActionHeader (new) │ top of right sidebar | ||
| │ [#NNN ↗pr-link] [status] [▶action] │ | ||
| └──────────────────┬──────────────────────┘ | ||
| │ click | ||
| ▼ | ||
| ┌───────────────────────────────┐ | ||
| │ getPRFlowState (pure) │ reducer over PR + branch + checks | ||
| └──────────────┬────────────────┘ | ||
| ▼ | ||
| ┌───────────────────────────────┐ | ||
| │ usePRFlowDispatch │ | ||
| │ 1. build pr-context.md │ buildPRContext (pure) | ||
| │ 2. data-URL encode │ | ||
| │ 3. ensureChatPane │ new tab OR reuse | ||
| │ 4. addTab / openPane with │ | ||
| │ ChatLaunchConfig │ | ||
| └──────────────┬────────────────┘ | ||
| ▼ | ||
| chat pane opens with | ||
| initialPrompt = "/<skill>" | ||
| initialFiles = [pr-context.md] | ||
| ▼ | ||
| agent runs skill, calling existing | ||
| tRPC mutations + gh CLI as tools | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add a language tag to the fenced code block.
markdownlint flagged MD040 on Line 84. Use text (or similar) for the ASCII architecture diagram.
📝 Diff
-```
+```text
┌─────────────────────────────────────────┐
│ PRActionHeader (new) │ top of right sidebar🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 84-84: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/plans/20260420-1045-agent-driven-pr-flow.md` around lines 84 -
110, The fenced ASCII diagram block needs a language tag to satisfy markdownlint
MD040; update the triple-backtick fence for the diagram (the block containing
PRActionHeader, getPRFlowState, usePRFlowDispatch, buildPRContext,
ensureChatPane, ChatLaunchConfig) from ``` to ```text (or another appropriate
tag) so the diagram is treated as plain text.
Comment on lines
+151
to
+155
| {PR_ACTION_HEADER_ENABLED ? ( | ||
| <PRActionHeader state={flowState} dispatch={dispatch} onRetry={onRetry} /> | ||
| ) : ( | ||
| <div className="flex h-12 shrink-0 items-center gap-2 border-b border-border bg-muted/45 px-2 dark:bg-muted/35" /> | ||
| )} |
Contributor
There was a problem hiding this comment.
Empty 48px gated bar will visibly render as a styled strip.
When the flag is off, the placeholder still has border-b border-border bg-muted/45 dark:bg-muted/35 and gap-2 px-2. That's not a no-op preserve-layout spacer — it's a visible muted/bordered band at the top of every v2 workspace sidebar. If the only goal is to reserve 48px so toggling the flag doesn't shift content, drop the background/border on the placeholder so it reads as empty whitespace; if the band is intentional product UI, ignore.
- ) : (
- <div className="flex h-12 shrink-0 items-center gap-2 border-b border-border bg-muted/45 px-2 dark:bg-muted/35" />
- )}
+ ) : (
+ <div aria-hidden className="h-12 shrink-0" />
+ )}📝 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
| {PR_ACTION_HEADER_ENABLED ? ( | |
| <PRActionHeader state={flowState} dispatch={dispatch} onRetry={onRetry} /> | |
| ) : ( | |
| <div className="flex h-12 shrink-0 items-center gap-2 border-b border-border bg-muted/45 px-2 dark:bg-muted/35" /> | |
| )} | |
| {PR_ACTION_HEADER_ENABLED ? ( | |
| <PRActionHeader state={flowState} dispatch={dispatch} onRetry={onRetry} /> | |
| ) : ( | |
| <div aria-hidden className="h-12 shrink-0" /> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/WorkspaceSidebar.tsx
around lines 151 - 155, The placeholder rendered when PR_ACTION_HEADER_ENABLED
is false should be an invisible layout spacer rather than a visible
muted/bordered bar: in the conditional that renders PRActionHeader (the
PRActionHeader component controlled by PR_ACTION_HEADER_ENABLED), replace the
div with one that only reserves space (e.g., className="h-12 shrink-0" or use
"h-12 invisible" / "aria-hidden") and remove the border-b, border-border,
bg-muted/45, dark:bg-muted/35, gap-2 and px-2 classes so it becomes a no-op
whitespace spacer instead of a visible strip.
Comment on lines
+42
to
+43
| rightSidebarOpen: z.boolean().default(false), | ||
| rightSidebarWidth: z.number().default(340), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find readers/writers of `rightSidebarOpen` across the repo to confirm whether
# the per-workspace field is still consumed after pointing the reader at user prefs.
rg -nP --type=ts --type=tsx -C2 '\brightSidebarOpen\b'Repository: superset-sh/superset
Length of output: 92
🏁 Script executed:
#!/bin/bash
# Search for rightSidebarOpen in .ts and .tsx files across the repo
rg --type ts 'rightSidebarOpen' -C 2Repository: superset-sh/superset
Length of output: 6102
🏁 Script executed:
#!/bin/bash
# Find and examine the schema files mentioned in the review
fd -e ts -e tsx | grep -E '(schema|workspaceLocalState|v2UserPreferences)' | head -20Repository: superset-sh/superset
Length of output: 941
🏁 Script executed:
#!/bin/bash
# Look directly at the file in question and surrounding context
cat -n apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.ts | head -60Repository: superset-sh/superset
Length of output: 2356
Remove unused rightSidebarOpen from workspaceLocalStateSchema.
rightSidebarOpen on workspaceLocalStateSchema is dead state: it's written to in page.tsx but never read. All actual reads use v2UserPreferences.rightSidebarOpen instead. The orphaned write should be removed, and the schema field deleted to avoid confusing future contributors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.ts`
around lines 42 - 43, Remove the dead rightSidebarOpen field from
workspaceLocalStateSchema and stop writing to it in page.tsx: delete the
rightSidebarOpen entry in workspaceLocalStateSchema (the
z.boolean().default(false) line) and remove or update any assignments in
page.tsx that set workspaceLocalState.rightSidebarOpen so that code uses
v2UserPreferences.rightSidebarOpen exclusively; keep workspaceLocalStateSchema
and page.tsx consistent to avoid orphaned writes.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
7 issues found across 25 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=".agents/commands/pr/create-pr.md">
<violation number="1" location=".agents/commands/pr/create-pr.md:45">
P1: Unquoted dynamic Git refs in shell command templates allow shell injection via crafted branch names. Quote refs (and use `--` where applicable) before passing them to commands.</violation>
</file>
<file name="packages/host-service/src/trpc/router/git/git.ts">
<violation number="1" location="packages/host-service/src/trpc/router/git/git.ts:476">
P2: Do not swallow `git.status()` errors here; returning `hasUncommitted: false` on failure can misreport a dirty repo as clean.</violation>
</file>
<file name="apps/desktop/plans/20260420-1045-agent-driven-pr-flow.md">
<violation number="1" location="apps/desktop/plans/20260420-1045-agent-driven-pr-flow.md:222">
P3: The plan states there are 15 main states, but the defined state union/table include 16. This inconsistency can cause implementation and test coverage gaps.</violation>
<violation number="2" location="apps/desktop/plans/20260420-1045-agent-driven-pr-flow.md:366">
P3: The documented skill count is incorrect: the list contains 13 skills, not 12.</violation>
<violation number="3" location="apps/desktop/plans/20260420-1045-agent-driven-pr-flow.md:471">
P3: The manual test references an outdated button/skill name (`Publish & Create PR` and `/pr/publish-and-create`) that conflicts with the documented MVP flow (`Create PR` and `/pr/create-pr`).</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx:224">
P1: `revealPath` sets `rightSidebarOpen` on `v2WorkspaceLocalState`, but the sidebar renders based on `v2UserPreferences.rightSidebarOpen`. These are different stores, so "reveal in sidebar" will not actually open the sidebar. Use the `setRightSidebarOpen` setter from `useV2UserPreferences` instead.</violation>
<violation number="2" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx:454">
P2: `.split("/").pop()` won't extract the filename correctly on Windows paths that use backslashes. Use a cross-platform path utility or split on both separators.
(Based on your team's feedback about using cross-platform path utilities instead of split.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| }): PRFlowState; | ||
| ``` | ||
|
|
||
| 15 main states, down from 33. Precedence (short-circuit in this order): |
Contributor
There was a problem hiding this comment.
P3: The plan states there are 15 main states, but the defined state union/table include 16. This inconsistency can cause implementation and test coverage gaps.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/plans/20260420-1045-agent-driven-pr-flow.md, line 222:
<comment>The plan states there are 15 main states, but the defined state union/table include 16. This inconsistency can cause implementation and test coverage gaps.</comment>
<file context>
@@ -0,0 +1,521 @@
+}): PRFlowState;
+```
+
+15 main states, down from 33. Precedence (short-circuit in this order):
+
+1. `error` if `loadError` and no last-known data
</file context>
Suggested change
| 15 main states, down from 33. Precedence (short-circuit in this order): | |
| 16 main states, down from 33. Precedence (short-circuit in this order): |
| a short body describing the goal and the allowed tool calls. The agent | ||
| consumes `pr-context.md` as an attachment. | ||
|
|
||
| Skills to write (12): |
Contributor
There was a problem hiding this comment.
P3: The documented skill count is incorrect: the list contains 13 skills, not 12.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/plans/20260420-1045-agent-driven-pr-flow.md, line 366:
<comment>The documented skill count is incorrect: the list contains 13 skills, not 12.</comment>
<file context>
@@ -0,0 +1,521 @@
+a short body describing the goal and the allowed tool calls. The agent
+consumes `pr-context.md` as an attachment.
+
+Skills to write (12):
+
+- `pr/create-pr.md` — one skill for the whole pre-PR path (commit,
</file context>
Suggested change
| Skills to write (12): | |
| Skills to write (13): |
| assert `addTab` called with the right `initialPrompt` and a | ||
| base64-decodable `pr-context.md`. | ||
| - **Manual:** unpublished branch → click **Publish & Create PR** → | ||
| verify chat pane opens with `/pr/publish-and-create` + attachment and |
Contributor
There was a problem hiding this comment.
P3: The manual test references an outdated button/skill name (Publish & Create PR and /pr/publish-and-create) that conflicts with the documented MVP flow (Create PR and /pr/create-pr).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/plans/20260420-1045-agent-driven-pr-flow.md, line 471:
<comment>The manual test references an outdated button/skill name (`Publish & Create PR` and `/pr/publish-and-create`) that conflicts with the documented MVP flow (`Create PR` and `/pr/create-pr`).</comment>
<file context>
@@ -0,0 +1,521 @@
+ assert `addTab` called with the right `initialPrompt` and a
+ base64-decodable `pr-context.md`.
+- **Manual:** unpublished branch → click **Publish & Create PR** →
+ verify chat pane opens with `/pr/publish-and-create` + attachment and
+ the agent completes the flow.
+
</file context>
Restores main's page.tsx (PR #3747 refactor) — the wip's inline openFilePane stored unconverted paths and tripped workspace-fs's ensureWithinRoot for callers that didn't pre-normalize. Re-adds the pendingReveal prop and widens onSelectDiffFile on WorkspaceSidebar so main's page.tsx props line up. PRActionHeader now mirrors v1's PRButton: when a PR exists and is open, the right side renders a link + chevron dropdown with squash / merge / rebase, wired to workspaceTrpc.github.mergePR. After a successful merge, triggers pullRequests.refreshByWorkspaces and refetches the local queries so the UI reflects merged state immediately instead of waiting for the 30s background sync. Closed/merged/draft PRs render the link without the dropdown. Gating: renamed PR_ACTION_HEADER_ENABLED → CREATE_PR_BUTTON_ENABLED to reflect that only the create-PR dispatch is gated (chat doesn't exist in v2 yet). The status group + merge dropdown always render. Also: refresh button on the Changes tab that invalidates getStatus, getDiff, listCommits, listBranches, and getBaseBranch in parallel and spins while in flight. getPullRequest now returns repoOwner/repoName (needed for the merge mutation).
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (6)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/utils/getPRFlowState/getPRFlowState.test.ts (2)
202-208: Test name mentions error but only asserts busy.The label says "during busy/error when PR known" but the assertion only covers
kind: "busy". Consider adding anerrorcase so the test matches its description, or tighten the name.♻️ Optional addition
test("PR link still visible during busy/error when PR known", () => { const p = pr(); expect(selectPRLink({ kind: "busy", pr: p })).toMatchObject({ kind: "pr-link", }); + expect( + selectPRLink({ kind: "error", pr: p, message: "x" }), + ).toMatchObject({ kind: "pr-link" }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/utils/getPRFlowState/getPRFlowState.test.ts around lines 202 - 208, The test named "PR link still visible during busy/error when PR known" is only asserting the busy state; update the test for consistency by either renaming it to reflect only the busy case or adding an assertion for the error case: call selectPRLink with { kind: "error", pr: p } (reuse the existing pr() fixture and the selectPRLink helper) and expect the result toMatchObject({ kind: "pr-link" }) as well, so both busy and error states are covered while keeping the original busy assertion.
210-246: Consider adding pull-only andpr-existsstatus-badge coverage.The
no-prbadge cases skip the pull-only path (e.g.pullCount: 2, pushCount: 0) which is a common branch alongside push-only and diverged. Additionally,selectStatusBadgeforkind: "pr-exists"(and other non-no-prkinds, if supported) isn't exercised here — worth a quick test or two if those branches exist in the implementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/utils/getPRFlowState/getPRFlowState.test.ts around lines 210 - 246, Add missing unit tests for the "pull-only" branch and for non-"no-pr" kinds: extend the test suite for selectStatusBadge by adding a case where kind: "no-pr" with sync({ pullCount: 2, pushCount: 0 }) asserts the expected "pulls to fetch" badge (or whatever the implementation returns for pull-only), and add at least one test for kind: "pr-exists" (using the same sync(...) helper) to assert the badge string returned for PR-present state; locate tests around selectStatusBadge and the sync(...) factory to add these assertions so the pull-only and pr-exists code-paths are covered.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/WorkspaceSidebar.tsx (2)
87-89: Use the existingisSidebarTabIdguard when reading persistedactiveTab.The read path
as SidebarTabId | undefinedis an unchecked cast — if a stale persisted value (older"diffs"/"review"subtab name, or any string from a future schema migration) is on disk,activeTabwill hold a value outside theSidebarTabIdunion,tabs.findwill returnundefined, and the sidebar body silently renders nothing. ThesetActiveTabwriter already validates viaisSidebarTabId; mirror that on the read so the union is actually enforced.♻️ Suggested change
- const activeTab: SidebarTabId = - (localState?.sidebarState?.activeTab as SidebarTabId | undefined) ?? - "changes"; + const persistedTab = localState?.sidebarState?.activeTab; + const activeTab: SidebarTabId = + persistedTab && isSidebarTabId(persistedTab) ? persistedTab : "changes";As per coding guidelines: "Avoid using
anytype; prefer explicit type safety in TypeScript." The same intent applies toascasts that bypass the union check when a runtime guard is already available.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/WorkspaceSidebar.tsx around lines 87 - 89, The current read casts localState?.sidebarState?.activeTab to SidebarTabId unchecked; instead validate the persisted value with the existing isSidebarTabId guard before using it so you never assign an out-of-union string to activeTab. In the activeTab initialization (where localState is read), call isSidebarTabId(localState?.sidebarState?.activeTab) and if true use that value, otherwise fall back to "changes"; keep setActiveTab unchanged (it already validates). This ensures tabs.find won't get an invalid key and the sidebar body won't render empty.
129-132:onOpenChat ?? (() => {})allocates a new no-op each render.The inline fallback creates a fresh function identity on every render of
WorkspaceSidebar. IfusePRFlowDispatchmemoizes its returneddispatchagainstonOpenChat(typical for auseCallback-style hook), that memoization is broken whenever the parent doesn't passonOpenChat, defeating downstreamReact.memo/useEffectdeps inPRActionHeader. Hoist a stable no-op:♻️ Suggested change
+const NOOP_OPEN_CHAT: OpenChatFn = () => {}; + export function WorkspaceSidebar({ ... }: WorkspaceSidebarProps) { ... - const dispatch = usePRFlowDispatch({ - onOpenChat: onOpenChat ?? (() => {}), - }); + const dispatch = usePRFlowDispatch({ + onOpenChat: onOpenChat ?? NOOP_OPEN_CHAT, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/WorkspaceSidebar.tsx around lines 129 - 132, The inline fallback onOpenChat ?? (() => {}) creates a new function identity each render and can break memoization in usePRFlowDispatch and downstream components like PRActionHeader; fix by providing a stable no-op instead: hoist a single stable no-op function (e.g., a module-level const NOOP or a useCallback(() => {}, []) inside WorkspaceSidebar) and pass that stable reference into usePRFlowDispatch when onOpenChat is undefined so dispatch memoization remains valid.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/useChangesTab.tsx (1)
126-141: Consider functionalsetIsRefreshingand surfacing invalidation errors.Two minor nits on
handleRefresh:
isRefreshingis in the dependency array purely for the early-return guard, which meanshandleRefresh's identity flips on every refresh cycle and re-creates the boundonClick. Reading it via auseRef(or just relying ondisabled={isRefreshing}on theButton, which already prevents double-clicks here) lets you drop it from the deps.- If any of the
invalidatecalls reject, the rejection escapes throughvoid handleRefresh()as an unhandled rejection — the user sees the spinner stop but gets no feedback. Wrapping in atry/catchand surfacing via the existingtoast(already imported) keeps refresh failures observable.♻️ Suggested tweak
- const [isRefreshing, setIsRefreshing] = useState(false); - const handleRefresh = useCallback(async () => { - if (isRefreshing) return; - setIsRefreshing(true); - try { - await Promise.all([ - utils.git.getStatus.invalidate({ workspaceId }), - utils.git.getDiff.invalidate({ workspaceId }), - utils.git.listCommits.invalidate({ workspaceId }), - utils.git.listBranches.invalidate({ workspaceId }), - utils.git.getBaseBranch.invalidate({ workspaceId }), - ]); - } finally { - setIsRefreshing(false); - } - }, [utils, workspaceId, isRefreshing]); + const [isRefreshing, setIsRefreshing] = useState(false); + const handleRefresh = useCallback(async () => { + setIsRefreshing(true); + try { + await Promise.all([ + utils.git.getStatus.invalidate({ workspaceId }), + utils.git.getDiff.invalidate({ workspaceId }), + utils.git.listCommits.invalidate({ workspaceId }), + utils.git.listBranches.invalidate({ workspaceId }), + utils.git.getBaseBranch.invalidate({ workspaceId }), + ]); + } catch (err) { + toast.error( + err instanceof Error ? err.message : "Failed to refresh changes", + ); + } finally { + setIsRefreshing(false); + } + }, [utils, workspaceId]);The
disabled={isRefreshing}on the triggerButtonalready prevents concurrent invocations from the UI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/useChangesTab.tsx around lines 126 - 141, The handleRefresh function currently depends on isRefreshing which flips identity on every refresh and can cause needless re-renders; remove isRefreshing from the dependency array by reading it from a ref or by relying on the Button's disabled={isRefreshing} and use the functional form setIsRefreshing(prev => true/false) to update state. Also wrap the Promise.all(...) invalidate calls in a try/catch inside handleRefresh and call the existing toast to surface any error (e.g., toast.error(...)) before clearing state so failures aren't swallowed; keep references to utils.git.getStatus.invalidate, utils.git.getDiff.invalidate, utils.git.listCommits.invalidate, utils.git.listBranches.invalidate, and utils.git.getBaseBranch.invalidate when implementing this change.packages/host-service/src/trpc/router/git/git.ts (1)
442-477: Optional: parallelize independent git operations.
currentBranch,defaultBranch,remotes, the upstream tracking probe, andstatus()are all independent. Running them sequentially can add noticeable latency on slower disks/large repos, especially since this query is likely invoked frequently to drive the PR header's badges. ConsiderPromise.allto issue them concurrently.♻️ Suggested parallelization
- const currentBranch = ( - await git.revparse(["--abbrev-ref", "HEAD"]).catch(() => "") - ).trim(); - const isDetached = !currentBranch || currentBranch === "HEAD"; - - const defaultBranch = await getDefaultBranchName(git); - const isDefaultBranch = - !isDetached && !!defaultBranch && currentBranch === defaultBranch; - - const remotes = await git.getRemotes(false).catch(() => []); - const hasRepo = remotes.length > 0; - - let hasUpstream = false; - let pushCount = 0; - let pullCount = 0; - try { - await git.raw(["rev-parse", "--abbrev-ref", "@{upstream}"]); - hasUpstream = true; - const tracking = await git.raw([ - "rev-list", - "--left-right", - "--count", - "@{upstream}...HEAD", - ]); - const [pullStr, pushStr] = tracking.trim().split(/\s+/); - pullCount = Number.parseInt(pullStr || "0", 10); - pushCount = Number.parseInt(pushStr || "0", 10); - } catch { - // no upstream — counts stay zero - } - - const status = await git.status().catch(() => null); - const hasUncommitted = status ? status.files.length > 0 : false; + const [currentBranchRaw, defaultBranch, remotes, tracking, status] = + await Promise.all([ + git.revparse(["--abbrev-ref", "HEAD"]).catch(() => ""), + getDefaultBranchName(git), + git.getRemotes(false).catch(() => []), + git + .raw([ + "rev-list", + "--left-right", + "--count", + "@{upstream}...HEAD", + ]) + .catch(() => null), + git.status().catch(() => null), + ]); + + const currentBranch = currentBranchRaw.trim(); + const isDetached = !currentBranch || currentBranch === "HEAD"; + const isDefaultBranch = + !isDetached && !!defaultBranch && currentBranch === defaultBranch; + const hasRepo = remotes.length > 0; + + let hasUpstream = false; + let pullCount = 0; + let pushCount = 0; + if (tracking !== null) { + hasUpstream = true; + const [pullStr, pushStr] = tracking.trim().split(/\s+/); + pullCount = Number.parseInt(pullStr || "0", 10); + pushCount = Number.parseInt(pushStr || "0", 10); + } + + const hasUncommitted = status ? status.files.length > 0 : false;Note: this drops the explicit
git rev-parse --abbrev-ref @{upstream}probe sincerev-listalready fails when no upstream is configured, which is sufficient to derivehasUpstream.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/trpc/router/git/git.ts` around lines 442 - 477, Multiple independent git calls (git.revparse for currentBranch, getDefaultBranchName(git), git.getRemotes, the upstream tracking rev-list, and git.status) are run sequentially causing unnecessary latency; change the code to run these in parallel with Promise.all: concurrently call git.revparse(["--abbrev-ref","HEAD"]) to get currentBranch, getDefaultBranchName(git) for defaultBranch, git.getRemotes(false) for remotes, git.raw(["rev-list","--left-right","--count","@{upstream}...HEAD"]) for tracking (drop the separate rev-parse upstream probe and derive hasUpstream from whether rev-list succeeds), and git.status() for status; then compute isDetached, isDefaultBranch, hasRepo, hasUpstream (based on whether rev-list threw), pullCount/pushCount (parse rev-list output or default to 0), and hasUncommitted from status, wrapping each promise with catch/defaults so failures of one do not break others.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/utils/getPRFlowState/getPRFlowState.test.ts:
- Around line 202-208: The test named "PR link still visible during busy/error
when PR known" is only asserting the busy state; update the test for consistency
by either renaming it to reflect only the busy case or adding an assertion for
the error case: call selectPRLink with { kind: "error", pr: p } (reuse the
existing pr() fixture and the selectPRLink helper) and expect the result
toMatchObject({ kind: "pr-link" }) as well, so both busy and error states are
covered while keeping the original busy assertion.
- Around line 210-246: Add missing unit tests for the "pull-only" branch and for
non-"no-pr" kinds: extend the test suite for selectStatusBadge by adding a case
where kind: "no-pr" with sync({ pullCount: 2, pushCount: 0 }) asserts the
expected "pulls to fetch" badge (or whatever the implementation returns for
pull-only), and add at least one test for kind: "pr-exists" (using the same
sync(...) helper) to assert the badge string returned for PR-present state;
locate tests around selectStatusBadge and the sync(...) factory to add these
assertions so the pull-only and pr-exists code-paths are covered.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/useChangesTab.tsx:
- Around line 126-141: The handleRefresh function currently depends on
isRefreshing which flips identity on every refresh and can cause needless
re-renders; remove isRefreshing from the dependency array by reading it from a
ref or by relying on the Button's disabled={isRefreshing} and use the functional
form setIsRefreshing(prev => true/false) to update state. Also wrap the
Promise.all(...) invalidate calls in a try/catch inside handleRefresh and call
the existing toast to surface any error (e.g., toast.error(...)) before clearing
state so failures aren't swallowed; keep references to
utils.git.getStatus.invalidate, utils.git.getDiff.invalidate,
utils.git.listCommits.invalidate, utils.git.listBranches.invalidate, and
utils.git.getBaseBranch.invalidate when implementing this change.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/WorkspaceSidebar.tsx:
- Around line 87-89: The current read casts localState?.sidebarState?.activeTab
to SidebarTabId unchecked; instead validate the persisted value with the
existing isSidebarTabId guard before using it so you never assign an
out-of-union string to activeTab. In the activeTab initialization (where
localState is read), call isSidebarTabId(localState?.sidebarState?.activeTab)
and if true use that value, otherwise fall back to "changes"; keep setActiveTab
unchanged (it already validates). This ensures tabs.find won't get an invalid
key and the sidebar body won't render empty.
- Around line 129-132: The inline fallback onOpenChat ?? (() => {}) creates a
new function identity each render and can break memoization in usePRFlowDispatch
and downstream components like PRActionHeader; fix by providing a stable no-op
instead: hoist a single stable no-op function (e.g., a module-level const NOOP
or a useCallback(() => {}, []) inside WorkspaceSidebar) and pass that stable
reference into usePRFlowDispatch when onOpenChat is undefined so dispatch
memoization remains valid.
In `@packages/host-service/src/trpc/router/git/git.ts`:
- Around line 442-477: Multiple independent git calls (git.revparse for
currentBranch, getDefaultBranchName(git), git.getRemotes, the upstream tracking
rev-list, and git.status) are run sequentially causing unnecessary latency;
change the code to run these in parallel with Promise.all: concurrently call
git.revparse(["--abbrev-ref","HEAD"]) to get currentBranch,
getDefaultBranchName(git) for defaultBranch, git.getRemotes(false) for remotes,
git.raw(["rev-list","--left-right","--count","@{upstream}...HEAD"]) for tracking
(drop the separate rev-parse upstream probe and derive hasUpstream from whether
rev-list succeeds), and git.status() for status; then compute isDetached,
isDefaultBranch, hasRepo, hasUpstream (based on whether rev-list threw),
pullCount/pushCount (parse rev-list output or default to 0), and hasUncommitted
from status, wrapping each promise with catch/defaults so failures of one do not
break others.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4a3cae52-d5c1-483f-a800-c5008481dd97
📒 Files selected for processing (5)
apps/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/utils/getPRFlowState/getPRFlowState.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/useChangesTab.tsxpackages/host-service/src/trpc/router/git/git.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/PRActionHeader.tsx
Contributor
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
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/hooks/useChangesTab/useChangesTab.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/useChangesTab.tsx:150">
P2: Handle refresh failures explicitly instead of discarding a potentially rejecting async call from the click handler.
(Based on your team's feedback about handling async rejections explicitly.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Restores the full-height v2 sidebar layout that was on the wip: the sidebar now portals into the workspace-right-sidebar-slot div in layout.tsx (next to TopBar) instead of rendering inside the page below TopBar via ResizablePanelGroup. Width is persisted on v2UserPreferences.rightSidebarWidth (new field, default 340) with a setter on useV2UserPreferences. The renderer's explicit-width ResizablePanel handles drag-resize and double-click reset. Resize dragging is forwarded to useBrowserShellInteractionPassthrough so the browser pane keeps ignoring clicks during resize. The workspace area no longer needs the resizable panel group since it's the only thing in the page now — it expands to fill flex-1, and the sidebar floats next to it via portal. Also drops the unused commitCount prop on ChangesHeader (caught by lint after the wip's refactor stopped using it).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx (1)
199-213: Extract the default sidebar width to a shared constant.
340appears as both the fallback (Line 203) and the double-click reset target (Line 287). If the default ever shifts, the two sites can drift. Promote it to a named constant (and consider re-using it fromuseV2UserPreferencesif the same default lives there) so the fallback and reset always agree.♻️ Suggested refactor
+const DEFAULT_RIGHT_SIDEBAR_WIDTH = 340; + @@ - const sidebarWidth = v2UserPreferences.rightSidebarWidth ?? 340; + const sidebarWidth = + v2UserPreferences.rightSidebarWidth ?? DEFAULT_RIGHT_SIDEBAR_WIDTH; @@ - onDoubleClickHandle={() => setRightSidebarWidth(340)} + onDoubleClickHandle={() => + setRightSidebarWidth(DEFAULT_RIGHT_SIDEBAR_WIDTH) + }Also applies to: 276-301
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/page.tsx around lines 199 - 213, The hardcoded default sidebar width (340) is duplicated; extract it into a shared constant (e.g., DEFAULT_RIGHT_SIDEBAR_WIDTH) and replace the literal in the fallback for v2UserPreferences.rightSidebarWidth (used to compute sidebarWidth) and the double-click reset logic so both sites use the same named value; if useV2UserPreferences already exports a default width, reuse that symbol instead of introducing a new one to avoid divergence.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/PRActionHeader.tsx (1)
209-249: Reuse the existing selectors instead of recomputing PR + link state.
getPRFlowState.tsalready exportsselectPRLink(state)which returns{ state, number, url }derived from the samepr.isDraft/pr.stateladder you re-implement here at lines 243–249. The PR-extraction at lines 209–214 also re-implements the privategetPRFromStatehelper from that file. Drift between the two copies (e.g., a future PR lifecycle value) will silently desync the link rendering fromselectStatusBadge. Consider consumingselectPRLinkhere (and exportinggetPRFromStateif you still need raw PR fields likerepoOwner/repoName/urlfor the merge call).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/PRActionHeader.tsx around lines 209 - 249, Replace the local PR extraction and linkState ladder with the shared selector: stop re-implementing getPRFromState and the pr.isDraft/pr.state logic and instead call selectPRLink(state) to derive the link info (and use its .state/.url/.number as needed); if you still need raw PR fields for mergePRMutation (repoOwner/repoName/url), export and import getPRFromState from getPRFlowState.ts and use that to obtain the raw PR, then pass those fields into mergePRMutation—this ensures PR link rendering (previously computed into linkState) and status badge logic remain consistent with selectPRLink/selectStatusBadge.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/PRActionHeader.tsx:
- Around line 171-193: CreatePRSplitButton currently only dispatches a non-draft
create and misleads by naming; either implement the split behavior or rename:
Option A — implement a true split button by adding a secondary
trigger/chevron/menu in CreatePRSplitButton that dispatches dispatch({ state,
draft: true }) for the draft option while keeping the primary button dispatch({
state, draft: false }), and ensure the menu/trigger UI matches existing
tooltip/ARIA patterns; Option B — rename CreatePRSplitButton to CreatePRButton
and update all call sites to use the new name, leaving the single-button
dispatch({ state, draft: false }) as-is; in both cases keep PRFlowDispatch and
PRFlowState usage consistent and adjust tests/refs to the component name or new
draft dispatch as appropriate.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/utils/getPRFlowState/getPRFlowState.ts:
- Around line 149-158: The helper _unavailableTooltip in getPRFlowState.ts is a
duplicate of unavailableTooltip in PRActionHeader.tsx; export the shared tooltip
function from getPRFlowState (keep the UnavailableReason signature and strings
in sync with unavailableBadge/enum), remove the duplicate local copy in
PRActionHeader.tsx, import the exported tooltip function there, and handle the
extra "create-disabled" case that existed only in the PRActionHeader caller
(e.g., map "create-disabled" to an appropriate UnavailableReason or handle it
before calling unavailableTooltip).
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/page.tsx:
- Around line 218-221: The flash happens because sidebarSlotEl is initialized
null and populated only in useEffect, so the conditional that renders the portal
(sidebarOpen && sidebarSlotEl && createPortal(...)) is false on first paint; fix
by initializing sidebarSlotEl synchronously (use useState(() =>
document.getElementById("workspace-right-sidebar-slot") as HTMLElement | null)
instead of null and removing the setSidebarSlotEl useEffect) or, if you prefer a
post-mount approach, switch the effect to useLayoutEffect to populate
sidebarSlotEl before paint; update references to sidebarSlotEl,
setSidebarSlotEl, useState/useEffect (or useLayoutEffect) and ensure
createPortal conditional relies on the synchronous lookup.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/PRActionHeader.tsx:
- Around line 209-249: Replace the local PR extraction and linkState ladder with
the shared selector: stop re-implementing getPRFromState and the
pr.isDraft/pr.state logic and instead call selectPRLink(state) to derive the
link info (and use its .state/.url/.number as needed); if you still need raw PR
fields for mergePRMutation (repoOwner/repoName/url), export and import
getPRFromState from getPRFlowState.ts and use that to obtain the raw PR, then
pass those fields into mergePRMutation—this ensures PR link rendering
(previously computed into linkState) and status badge logic remain consistent
with selectPRLink/selectStatusBadge.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/page.tsx:
- Around line 199-213: The hardcoded default sidebar width (340) is duplicated;
extract it into a shared constant (e.g., DEFAULT_RIGHT_SIDEBAR_WIDTH) and
replace the literal in the fallback for v2UserPreferences.rightSidebarWidth
(used to compute sidebarWidth) and the double-click reset logic so both sites
use the same named value; if useV2UserPreferences already exports a default
width, reuse that symbol instead of introducing a new one to avoid divergence.
🪄 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: b3d44ed2-2c55-407f-a9b0-b22191ef2eb7
📒 Files selected for processing (5)
apps/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/utils/getPRFlowState/getPRFlowState.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/ReviewTabContent/ReviewTabContent.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/components/ReviewTabContent/ReviewTabContent.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/WorkspaceSidebar.tsx
Comment on lines
+149
to
+158
| function _unavailableTooltip(reason: UnavailableReason): string { | ||
| switch (reason) { | ||
| case "no-repo": | ||
| return "No GitHub repository connected"; | ||
| case "default-branch": | ||
| return "Switch to a feature branch to create a pull request"; | ||
| case "detached-head": | ||
| return "Checkout a branch to create a pull request"; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
_unavailableTooltip is dead code and is a byte-for-byte duplicate of unavailableTooltip in PRActionHeader.tsx (lines 156–169).
The leading underscore signals "intentionally unused", but the identical function actually lives in the consumer file. This looks like a copy that got orphaned during the cherry-pick — pick one of:
♻️ Option A — export from here, drop the copy in PRActionHeader.tsx
-function _unavailableTooltip(reason: UnavailableReason): string {
+export function unavailableTooltip(reason: UnavailableReason): string {
switch (reason) {
case "no-repo":
return "No GitHub repository connected";
case "default-branch":
return "Switch to a feature branch to create a pull request";
case "detached-head":
return "Checkout a branch to create a pull request";
}
}Then in PRActionHeader.tsx, import it and delete the local copy; the "create-disabled" branch can be handled in the caller (it's the only extra case the local version had).
♻️ Option B — just delete the unused helper
-function _unavailableTooltip(reason: UnavailableReason): string {
- switch (reason) {
- case "no-repo":
- return "No GitHub repository connected";
- case "default-branch":
- return "Switch to a feature branch to create a pull request";
- case "detached-head":
- return "Checkout a branch to create a pull request";
- }
-}Option A is preferable since the strings are intended to stay in lockstep with unavailableBadge/the unavailable-reason enum.
📝 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
| function _unavailableTooltip(reason: UnavailableReason): string { | |
| switch (reason) { | |
| case "no-repo": | |
| return "No GitHub repository connected"; | |
| case "default-branch": | |
| return "Switch to a feature branch to create a pull request"; | |
| case "detached-head": | |
| return "Checkout a branch to create a pull request"; | |
| } | |
| } | |
| // _unavailableTooltip function removed |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/utils/getPRFlowState/getPRFlowState.ts
around lines 149 - 158, The helper _unavailableTooltip in getPRFlowState.ts is a
duplicate of unavailableTooltip in PRActionHeader.tsx; export the shared tooltip
function from getPRFlowState (keep the UnavailableReason signature and strings
in sync with unavailableBadge/enum), remove the duplicate local copy in
PRActionHeader.tsx, import the exported tooltip function there, and handle the
extra "create-disabled" case that existed only in the PRActionHeader caller
(e.g., map "create-disabled" to an appropriate UnavailableReason or handle it
before calling unavailableTooltip).
PR badge now carries two ambient indicators next to the number — a CI
rollup dot (success/failure/pending, circle icons) and a review decision
dot (approved/changes-requested/pending, user icons). Distinct icon
families so they read as "build" vs "people" at 12px. Suppressed for
closed/merged PRs (post-state CI is noise) and for PRs with no checks
or no review requested (quiet for trivial PRs).
Hovering the link now opens a 320px card with the title (line-clamped),
state pill, branch, full check rollup ("8 of 10 passing"), review
status, "Updated 2h ago", and a "View on GitHub" link. Pulled in via
@superset/ui/hover-card so animations + portal placement come for free.
Extracted computeChecksRollup + coerceCheckStatus to a shared util so
useReviewTab and the new dots draw from the same source — keeps the
sidebar tab and the badge from drifting on edge cases.
Co-located the new pieces under PRActionHeader/components/PRStatusGroup
following the project's per-component-folder convention.
…ch-changes-into-current-branc # Conflicts: # apps/desktop/src/renderer/routes/_authenticated/_dashboard/layout.tsx # apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/PRActionHeader.tsx (2)
69-117: Add an exhaustiveness check on thevariant.kindswitch.
ActionSlothas no explicit return type and nodefaultarm. If a newActionButtonVariant.kindis added later (e.g. a futuremerge-conflictstate), this switch will silently fall through and the slot will renderundefined— easy to miss in review. Anever-typed default makes it a build error:- case "retry": - return ( - <button …>…</button> - ); - } + case "retry": + return ( + <button …>…</button> + ); + default: { + const _exhaustive: never = variant; + return _exhaustive; + } + }The same pattern would benefit
unavailableTooltip(Lines 140-153).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/PRActionHeader.tsx around lines 69 - 117, The switch over variant.kind in PRActionHeader must be made exhaustive: add a final default case that narrows to never (e.g., const _exhaustiveCheck: never = variant; throw new Error(...)) so the compiler errors if a new ActionButtonVariant.kind is added; apply the same pattern to the unavailableTooltip switch to force a compile-time exhaustiveness check. Locate the switch block that returns PRStatusGroup/CreatePRIconButton/UnavailableIcon/etc. and the unavailableTooltip switch and add the never-typed default assertion with a clear error message to fail the build when an unhandled variant.kind appears.
120-177: ExtractUnavailableIconandCreatePRIconButtoninto their own files.Both are real React components defined alongside
PRActionHeaderin the same file. They should live under their own component folders.As per coding guidelines: "Do not create multi-component files; use one component per file."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/PRActionHeader.tsx around lines 120 - 177, Move the UnavailableIcon component (and its helper unavailableTooltip) and the CreatePRIconButton component into separate files/components and update imports where used: create e.g. UnavailableIcon.tsx exporting the UnavailableIcon component (export default) and exporting or keeping unavailableTooltip private inside that file, and CreatePRIconButton.tsx exporting CreatePRIconButton; ensure both new files import the required types (UnavailableReason, PRFlowState, PRFlowDispatch) and UI primitives (Tooltip, TooltipTrigger, TooltipContent, VscGitPullRequest) used in the original file, then replace the inline definitions in PRActionHeader.tsx with imports of UnavailableIcon and CreatePRIconButton. Ensure props/signatures remain identical and update any relative import paths accordingly.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRStatusGroup/components/PRDetailCard/PRDetailCard.tsx (2)
102-222: Split sibling components (ChecksLine,ReviewLine,DetailLine) into their own files.These are exported-style React components living in
PRDetailCard.tsx. Promote each into its own folder underPRDetailCard/components/<Name>/<Name>.tsx(with anindex.ts);stateLabelToPillClasscan stay co-located as a util but the components shouldn't share a file.As per coding guidelines: "Do not create multi-component files; use one component per file." and "Use one folder per component with structure:
ComponentName/ComponentName.tsx+index.tsfor barrel export."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRStatusGroup/components/PRDetailCard/PRDetailCard.tsx around lines 102 - 222, The file currently contains three sibling components (ChecksLine, ReviewLine, DetailLine) inside PRDetailCard.tsx; split each into its own folder under PRDetailCard/components: create folders ChecksLine/ChecksLine.tsx + index.ts, ReviewLine/ReviewLine.tsx + index.ts, and DetailLine/DetailLine.tsx + index.ts, move the corresponding function implementation into each file, export the component as default (or named) from its index, and update PRDetailCard.tsx to import the three components from their new barrel index files; keep any shared util like stateLabelToPillClass co-located or in a util file but ensure no multi-component file remains.
27-34:stateLabelre-derives whatlinkStatealready encodes.
linkStateis passed in and is already the result of the sameisDraft/pr.stateladder computed byPRStatusGroup. Map directly offlinkStateso the two callsites can't drift:- const stateLabel = pr.isDraft - ? "Draft" - : pr.state === "merged" - ? "Merged" - : pr.state === "closed" - ? "Closed" - : "Open"; + const stateLabel = LINK_STATE_LABELS[linkState];with a small
Record<PRState, string>constant. (See related comment onPRStatusGroupLines 84-92.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRStatusGroup/components/PRDetailCard/PRDetailCard.tsx around lines 27 - 34, PRDetailCard currently recomputes stateLabel from pr.isDraft/pr.state which duplicates logic encoded by the incoming linkState; stop re-deriving and map directly from linkState using a small Record mapping. Replace the stateLabel calculation in PRDetailCard with a const Record<PRState, string> (e.g., STATUS_LABELS) and derive stateLabel = STATUS_LABELS[linkState]; keep statePillClass = stateLabelToPillClass(linkState) as-is so both label and pill use the same source of truth (reference symbols: PRDetailCard, stateLabel, linkState, statePillClass, stateLabelToPillClass, PRState).apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRStatusGroup/components/PRStatusIndicators/PRStatusIndicators.tsx (1)
41-93: SplitChecksDotandReviewDotinto their own files.
ChecksDotandReviewDotare defined alongsidePRStatusIndicatorsin the same file. Per the repo's component guideline these should each live in their own folder with a barrel export.As per coding guidelines: "Do not create multi-component files; use one component per file." and "Use one folder per component with structure:
ComponentName/ComponentName.tsx+index.tsfor barrel export."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRStatusGroup/components/PRStatusIndicators/PRStatusIndicators.tsx around lines 41 - 93, Extract the two subcomponents ChecksDot and ReviewDot from PRStatusIndicators.tsx into their own component folders (ChecksDot/ChecksDot.tsx + ChecksDot/index.ts and ReviewDot/ReviewDot.tsx + ReviewDot/index.ts) with default exports, preserving their prop types (ChecksDot({ status }: { status: ChecksRollup["overall"] }) and ReviewDot({ decision }: { decision: NonNullable<PullRequest["reviewDecision"]> }). Replace the inline definitions in PRStatusIndicators.tsx with imports from the new barrels (import ChecksDot from './ChecksDot' and import ReviewDot from './ReviewDot') and ensure any classnames, icons (LuCircleCheck, LuCircleX, LuCircleDashed, LuUserCheck, LuUserX, LuUserMinus), and aria attributes are kept identical so behavior and types remain unchanged.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRStatusGroup/PRStatusGroup.tsx (1)
84-92: Duplicate state-derivation betweenPRStatusGroupandPRDetailCard.The
pr.isDraft → "draft" / pr.state → "merged"|"closed"|"open"ladder is computed here and again asstateLabelinsidePRDetailCard. SincelinkStateis already passed intoPRDetailCard, derive the human label there fromlinkState(e.g. a smalllinkStateToLabelmap) so both places stay in sync if a new state is ever added.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRStatusGroup/PRStatusGroup.tsx around lines 84 - 92, PRStatusGroup currently computes linkState (draft/merged/closed/open) and PRDetailCard recomputes the same ladder into stateLabel; instead, keep the existing linkState from PRStatusGroup and remove the duplicate derivation in PRDetailCard by mapping linkState → human label there (e.g. add a linkStateToLabel map inside PRDetailCard and derive stateLabel = linkStateToLabel[linkState] with a sensible default). Update PRDetailCard to accept and use the linkState prop (instead of recomputing from pr.isDraft/pr.state) so both components stay in sync; reference PRStatusGroup, PRDetailCard, linkState, and stateLabel when making changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRStatusGroup/PRStatusGroup.tsx:
- Around line 64-75: The onSuccess handler shows the "PR merged" toast then
calls refreshPRMutation.mutateAsync without catching rejections, which can leave
the UI stale; update the onSuccess for the merge mutation so the call to
refreshPRMutation.mutateAsync({ workspaceIds: [workspaceId] }) is wrapped in
try/catch (keeping the current finally that calls onRefresh?.()), and in the
catch log the error and show a softer user-facing message (e.g., toast.error or
toast.info like "Couldn't refresh PR state") so refresh failures are surfaced to
users while still preserving the original success flow; reference the onSuccess
function, refreshPRMutation.mutateAsync, workspaceId and onRefresh in your
change.
- Around line 91-101: Add a persistent mergeable state and use it in the UI: add
a new column to the pullRequests table to store MergeableState ("mergeable" |
"conflicting" | "unknown"), update your DB migration and ORM model, populate
that column when fetching PR details from GitHub in getPullRequest (use the
API's mergeable field), and persist it. Then update the frontend canMerge logic
in PRStatusGroup (replace the current canMerge check) to require pr.state ===
"open" && !pr.isDraft && pr.mergeable === "mergeable" so the merge button (and
handleMerge/mergePRMutation path) is disabled/unavailable for "conflicting" or
non-mergeable states. Ensure the MergeableState type is used consistently across
getPullRequest, the DB model, and the PRStatusGroup component.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRStatusGroup/components/PRDetailCard/PRDetailCard.tsx:
- Around line 102-222: The file currently contains three sibling components
(ChecksLine, ReviewLine, DetailLine) inside PRDetailCard.tsx; split each into
its own folder under PRDetailCard/components: create folders
ChecksLine/ChecksLine.tsx + index.ts, ReviewLine/ReviewLine.tsx + index.ts, and
DetailLine/DetailLine.tsx + index.ts, move the corresponding function
implementation into each file, export the component as default (or named) from
its index, and update PRDetailCard.tsx to import the three components from their
new barrel index files; keep any shared util like stateLabelToPillClass
co-located or in a util file but ensure no multi-component file remains.
- Around line 27-34: PRDetailCard currently recomputes stateLabel from
pr.isDraft/pr.state which duplicates logic encoded by the incoming linkState;
stop re-deriving and map directly from linkState using a small Record mapping.
Replace the stateLabel calculation in PRDetailCard with a const Record<PRState,
string> (e.g., STATUS_LABELS) and derive stateLabel = STATUS_LABELS[linkState];
keep statePillClass = stateLabelToPillClass(linkState) as-is so both label and
pill use the same source of truth (reference symbols: PRDetailCard, stateLabel,
linkState, statePillClass, stateLabelToPillClass, PRState).
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRStatusGroup/components/PRStatusIndicators/PRStatusIndicators.tsx:
- Around line 41-93: Extract the two subcomponents ChecksDot and ReviewDot from
PRStatusIndicators.tsx into their own component folders (ChecksDot/ChecksDot.tsx
+ ChecksDot/index.ts and ReviewDot/ReviewDot.tsx + ReviewDot/index.ts) with
default exports, preserving their prop types (ChecksDot({ status }: { status:
ChecksRollup["overall"] }) and ReviewDot({ decision }: { decision:
NonNullable<PullRequest["reviewDecision"]> }). Replace the inline definitions in
PRStatusIndicators.tsx with imports from the new barrels (import ChecksDot from
'./ChecksDot' and import ReviewDot from './ReviewDot') and ensure any
classnames, icons (LuCircleCheck, LuCircleX, LuCircleDashed, LuUserCheck,
LuUserX, LuUserMinus), and aria attributes are kept identical so behavior and
types remain unchanged.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRStatusGroup/PRStatusGroup.tsx:
- Around line 84-92: PRStatusGroup currently computes linkState
(draft/merged/closed/open) and PRDetailCard recomputes the same ladder into
stateLabel; instead, keep the existing linkState from PRStatusGroup and remove
the duplicate derivation in PRDetailCard by mapping linkState → human label
there (e.g. add a linkStateToLabel map inside PRDetailCard and derive stateLabel
= linkStateToLabel[linkState] with a sensible default). Update PRDetailCard to
accept and use the linkState prop (instead of recomputing from
pr.isDraft/pr.state) so both components stay in sync; reference PRStatusGroup,
PRDetailCard, linkState, and stateLabel when making changes.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/PRActionHeader.tsx:
- Around line 69-117: The switch over variant.kind in PRActionHeader must be
made exhaustive: add a final default case that narrows to never (e.g., const
_exhaustiveCheck: never = variant; throw new Error(...)) so the compiler errors
if a new ActionButtonVariant.kind is added; apply the same pattern to the
unavailableTooltip switch to force a compile-time exhaustiveness check. Locate
the switch block that returns
PRStatusGroup/CreatePRIconButton/UnavailableIcon/etc. and the unavailableTooltip
switch and add the never-typed default assertion with a clear error message to
fail the build when an unhandled variant.kind appears.
- Around line 120-177: Move the UnavailableIcon component (and its helper
unavailableTooltip) and the CreatePRIconButton component into separate
files/components and update imports where used: create e.g. UnavailableIcon.tsx
exporting the UnavailableIcon component (export default) and exporting or
keeping unavailableTooltip private inside that file, and CreatePRIconButton.tsx
exporting CreatePRIconButton; ensure both new files import the required types
(UnavailableReason, PRFlowState, PRFlowDispatch) and UI primitives (Tooltip,
TooltipTrigger, TooltipContent, VscGitPullRequest) used in the original file,
then replace the inline definitions in PRActionHeader.tsx with imports of
UnavailableIcon and CreatePRIconButton. Ensure props/signatures remain identical
and update any relative import paths accordingly.
🪄 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: d5cc6bbe-46d3-4be6-900d-93d8a7c2bde7
📒 Files selected for processing (10)
apps/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/PRStatusGroup/PRStatusGroup.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRStatusGroup/components/PRDetailCard/PRDetailCard.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRStatusGroup/components/PRDetailCard/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRStatusGroup/components/PRStatusIndicators/PRStatusIndicators.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRStatusGroup/components/PRStatusIndicators/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRStatusGroup/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/utils/computeChecksStatus/computeChecksStatus.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/utils/computeChecksStatus/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/useReviewTab.tsx
✅ Files skipped from review due to trivial changes (4)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRStatusGroup/index.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRStatusGroup/components/PRDetailCard/index.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/utils/computeChecksStatus/index.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRStatusGroup/components/PRStatusIndicators/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/useReviewTab.tsx
Comment on lines
+91
to
+101
| const canMerge = pr.state === "open" && !pr.isDraft; | ||
| const showIndicators = pr.state === "open"; // includes draft | ||
|
|
||
| const handleMerge = (mergeMethod: "merge" | "squash" | "rebase") => { | ||
| mergePRMutation.mutate({ | ||
| owner: pr.repoOwner, | ||
| repo: pr.repoName, | ||
| pullNumber: pr.number, | ||
| mergeMethod, | ||
| }); | ||
| }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the PullRequest type definition that PRStatusGroup consumes.
fd -t f 'getPRFlowState' apps/desktop/src
ast-grep --pattern 'export type PullRequest = $_'
ast-grep --pattern 'type PullRequest = $_'
# Look for mergeable-related fields on PR types/router schemas.
rg -nP --type=ts -C2 '\bmergeable(State|Status)?\b'Repository: superset-sh/superset
Length of output: 3385
🏁 Script executed:
cat -n packages/host-service/src/trpc/router/git/git.ts | sed -n '520,560p'Repository: superset-sh/superset
Length of output: 1673
🏁 Script executed:
cat -n packages/host-service/src/trpc/router/git/types.ts | sed -n '40,75p'Repository: superset-sh/superset
Length of output: 1131
🏁 Script executed:
cat -n apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/\$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRStatusGroup/PRStatusGroup.tsx | sed -n '1,30p'Repository: superset-sh/superset
Length of output: 1236
🏁 Script executed:
# Check if mergeable field is being populated from GitHub API or hardcoded
rg -n "mergeable" packages/host-service/src/trpc/router/git/git.ts -B5 -A5
# Check the full context of the getPullRequest procedure
cat -n packages/host-service/src/trpc/router/git/git.ts | sed -n '475,550p'Repository: superset-sh/superset
Length of output: 3185
🏁 Script executed:
# Check if mergeable is ever populated from GitHub API elsewhere in the codebase
rg -n "mergeable" packages/host-service/src --type ts -B3 -A3
# Check the pullRequests schema to see if mergeable is stored
fd -t f "schema" packages/host-service/src --type ts | xargs grep -l "pullRequests\|migration"
# Look for where PR data is fetched from GitHub
rg -n "github|graphql" packages/host-service/src/trpc/router/git/git.ts -i | head -20Repository: superset-sh/superset
Length of output: 1789
🏁 Script executed:
# Check the database schema for pullRequests table
find packages/host-service/src -name "*.ts" -type f | xargs grep -l "pullRequests\s*:" | head -5
# Look for where pullRequests table is defined
rg -n "pullRequests.*=" packages/host-service/src --type=ts -B2 -A5 | head -60
# Check if mergeable field exists in the database
rg -n "mergeable" packages/host-service/src -C3Repository: superset-sh/superset
Length of output: 2815
🏁 Script executed:
# Get the full pullRequests table schema
cat -n packages/host-service/src/db/schema.ts | sed -n '49,100p'Repository: superset-sh/superset
Length of output: 1922
🏁 Script executed:
# Check how mergePRMutation handles failures and displays errors
rg -n "mergePRMutation\|mergePR" apps/desktop/src -B3 -A3 | head -40
# Look for the merge mutation definition and error handling
rg -n "mergePR" packages/host-service/src/trpc/router -B5 -A10Repository: superset-sh/superset
Length of output: 1415
🏁 Script executed:
# Get the full mergePR mutation implementation
cat -n packages/host-service/src/trpc/router/github/github.ts | sed -n '134,180p'Repository: superset-sh/superset
Length of output: 692
Merge button doesn't account for merge conflicts or non-mergeable state — but the current architecture doesn't track this information.
The review comment's suggested fix won't work as intended: pr.mergeable is hardcoded to "unknown" in the router (at getPullRequest) and is never populated from GitHub or stored in the database. Checking pr.mergeable !== false would always evaluate to true and have no effect.
However, the underlying concern is valid. When a user clicks merge on a PR with conflicts, failing checks, or blocked status, the mutation will fail with a generic error from GitHub rather than preventing the action upfront.
To address this properly:
- Add a
mergeablecolumn to thepullRequeststable schema - Fetch and store the
mergeablestate from GitHub's API when fetching PR details - Update
canMergelogic to check the actual mergeable state
The current code structure has this field type defined (MergeableState: "mergeable" | "conflicting" | "unknown") but never uses it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRStatusGroup/PRStatusGroup.tsx
around lines 91 - 101, Add a persistent mergeable state and use it in the UI:
add a new column to the pullRequests table to store MergeableState ("mergeable"
| "conflicting" | "unknown"), update your DB migration and ORM model, populate
that column when fetching PR details from GitHub in getPullRequest (use the
API's mergeable field), and persist it. Then update the frontend canMerge logic
in PRStatusGroup (replace the current canMerge check) to require pr.state ===
"open" && !pr.isDraft && pr.mergeable === "mergeable" so the merge button (and
handleMerge/mergePRMutation path) is disabled/unavailable for "conflicting" or
non-mergeable states. Ensure the MergeableState type is used consistently across
getPullRequest, the DB model, and the PRStatusGroup component.
… first PR badge now wears the PR state color: open is emerald-tinted (border + bg + divider), merged violet, closed rose, draft muted. Tinting the whole bordered group makes state legible at a glance without the icon having to carry it alone. Dropped the review-decision dot and the "Review" line in the hover detail card — review noise was redundant with the Review tab's own indicator and made the badge feel busy. Only the CI checks dot remains next to the PR number. Sidebar tab order is now Files → Changes → Review (Files first).
Contributor
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
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/components/PRStatusGroup/components/PRStatusIndicators/PRStatusIndicators.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRStatusGroup/components/PRStatusIndicators/PRStatusIndicators.tsx:14">
P2: Review status is no longer rendered when a PR has no checks, so approved/changes-requested state can be hidden entirely.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| * reported so the row stays quiet for trivial PRs. | ||
| */ | ||
| export function PRStatusIndicators({ checks }: PRStatusIndicatorsProps) { | ||
| if (checks.overall === "none") return null; |
Contributor
There was a problem hiding this comment.
P2: Review status is no longer rendered when a PR has no checks, so approved/changes-requested state can be hidden entirely.
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/PRStatusGroup/components/PRStatusIndicators/PRStatusIndicators.tsx, line 14:
<comment>Review status is no longer rendered when a PR has no checks, so approved/changes-requested state can be hidden entirely.</comment>
<file context>
@@ -1,40 +1,22 @@
- const hasReview = reviewDecision !== null;
- if (!hasChecks && !hasReview) return null;
+export function PRStatusIndicators({ checks }: PRStatusIndicatorsProps) {
+ if (checks.overall === "none") return null;
return (
</file context>
Bot reviews flagged a mix of stale concerns (already addressed in earlier commits) and live issues. Fixing the live ones: - page.tsx: restore min-w-[320px] on the workspace area so dragging the sidebar wider can't collapse the editor pane to unusable width (lost when we portaled the sidebar out of the resizable panel group) - page.tsx: initialize sidebarSlotEl synchronously via useState init so users with rightSidebarOpen=true persisted don't see a 1-frame flash on mount; the slot is mounted by the parent layout before this child renders, so a synchronous lookup is safe - PRStatusGroup: catch refresh failures after a successful merge — previously the rejection escaped the async onSuccess silently and left the UI stale. Now logs and shows a soft toast - useChangesTab: catch refresh failures from the Changes-tab refresh button — previously void-discarded - git.ts getBranchSyncStatus: log when git.status() fails instead of silently misreporting a dirty repo as clean. Defaults to false on failure with a comment explaining why over-reporting on every transient blip would be more annoying than briefly under-reporting Cleanup: - Drop dead _unavailableTooltip helper in getPRFlowState (byte-for-byte copy of the one in PRActionHeader, orphaned during cherry-pick) - Drop dead rightSidebarOpen + rightSidebarWidth from workspaceLocalStateSchema — both moved to v2UserPreferences and the per-workspace fields are written by nobody and read by nobody Dead-code-but-harmless-to-fix-now (will matter once create-PR ships): - Replace deprecated unescape() with TextEncoder for UTF-8 base64 in usePRFlowDispatch.encodeAsDataUrl - buildPRContext: stop emitting literal `<default>` when defaultBranch is null; instruct the agent to resolve via `gh repo view` instead - create-pr.md: quote refs in `git push -u origin -- "<branch>"` and the `<base>..HEAD` ranges to prevent shell injection on crafted names
Merged
6 tasks
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
Apr 28, 2026
upstream merge 2026-04-27 PR-D5: PR action header (superset-sh#3777, scope-limited)
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
May 8, 2026
…umn doesn't eat main content upstream superset-sh#3777 added a portal slot div with className 'flex h-full shrink-0' as a sibling of the main content row inside fork's flex-col layout. With no consumer in fork yet, the empty div still claimed 100% height (h-full) of the column and shrink-0 prevented it from yielding, collapsing the main content area to 0px and producing a black screen with only the topbar visible. Switch to className 'contents' so the empty slot is invisible to layout; future portals can override or wrap their own positioning.
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
diff-editorbranch into v2 (PRActionHeader, usePRFlowState, usePRFlowDispatch, buildPRContext, getPRFlowState, agent slash command, plan doc).v2UserPreferencesSchema+pendingWorkspaceSchemafields the wip dropped, points the sidebar-open reader at user prefs (matches what the toggle hotkey writes), addsfocusRequestIdroute param +getDefaultBranchNameimport the wip referenced without declaring, drops the removedchangesSubtabwrite.PR_ACTION_HEADER_ENABLED = falsesince the flow currently dispatches to a chat session and v2 doesn't have chat yet — an empty 48px bar still renders so the sidebar layout doesn't shift when the flag flips.Why gated
The PRActionHeader's "Create PR" / "Create draft PR" buttons hand off to a v2 chat pane via
onOpenChat→/pr/create-prslash command + a synthesizedpr-context.mdattachment. v2 doesn't have the chat pane yet, so clicking would no-op. Flip totrueonce chat lands and the agent flow is verified end-to-end.Test plan
useV2UserPreferences.rightSidebarOpenrewire).PR_ACTION_HEADER_ENABLED = truelocally and confirm the button + dropdown render and dispatch (chat pane will be the no-op blocker).bun run typecheckclean.Summary by cubic
Added an icon-only PR action header to the v2 sidebar with a state‑tinted PR badge, a CI status dot, a hover detail card, and a merge dropdown; Create PR still dispatches to chat and is gated by
CREATE_PR_BUTTON_ENABLED = false. The sidebar now renders full height via a portal, persists its width, adds overflow guards and a Changes tab refresh, and the tab order is now Files → Changes → Review.New Features
PRActionHeader: icon-only state machine; PR link shows a CI status dot and a hover detail card; merge dropdown (squash/merge/rebase) wired toworkspaceTrpc.github.mergePR, then refreshes viapullRequests.refreshByWorkspaces./pr/create-prwith a synthesizedpr-context.mdfromgetPRFlowStateandbuildPRContext(with tests); gated byCREATE_PR_BUTTON_ENABLED = false.getStatus,getDiff,listCommits,listBranches, andgetBaseBranch.Bug Fixes
git.getBranchSyncStatusnow logsgit.status()failures and falls back safely; removed dead per-workspace sidebar fields.TextEncoderfor base64 in dispatch; quote refs in thecreate-prcommand doc;buildPRContextdefers missing default-branch resolution togh repo view.Written for commit fc0c4f5. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Improvements
Documentation & Tests