feat(desktop): v2 PR checkout via widened checkout procedure#3525
Merged
feat(desktop): v2 PR checkout via widened checkout procedure#3525
Conversation
Design doc for wiring up linkedPR → worktree materialization in the v2 new workspace modal. Extends workspaceCreation.checkout with an optional `pr` field (shells to `gh pr checkout`) rather than adding a new tRPC procedure; client keeps a distinct `pr-checkout` pending intent for progress labels and payload construction.
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 PR-based checkout to V2 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Renderer
participant Server as "API / Server"
participant HostSvc as "Host Service"
participant GH as "gh CLI"
participant FS as "Host FS / worktree"
Renderer->>Server: workspaceCreation.checkout { pr: {...}, composer.baseBranch }
Server->>HostSvc: derive branch (from pr) & check existing workspace
alt workspace exists
HostSvc-->>Server: { alreadyExists: true }
Server-->>Renderer: result (alreadyExists)
else create workspace
Server->>HostSvc: create detached worktree
HostSvc->>GH: run `gh pr checkout <number> --branch <derived> --force`
GH-->>FS: materialize branch refs
HostSvc->>FS: apply worktree config (push.autoSetupRemote)
HostSvc-->>Server: success + warnings
Server->>Server: finishCheckout (write branch.<name>.base, ensure host, create V2 workspace, register local workspace, start terminal, clear progress)
Server-->>Renderer: checkout result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 adds a design document ( Key design decisions documented:
Overall the plan is well-structured and the tradeoffs are clearly argued. Three design points worth resolving before implementation begins. Confidence Score: 4/5Safe to merge as a plan document — three P2 design points worth addressing before implementation begins. This is a plan-only PR with no code changes. The design is well-reasoned: XOR schema enforcement is clean, the V1 pain-point analysis is thorough, and the decision to widen apps/desktop/plans/20260416-v2-pr-checkout-endpoint.md — review the three P2 comments (draft-PR warning, --force residual risk, agent-launch wiring) before starting implementation
|
| Filename | Overview |
|---|---|
| apps/desktop/plans/20260416-v2-pr-checkout-endpoint.md | New design document for v2 PR checkout via widened workspaceCreation.checkout. Well-structured plan with clear V1 pain-point analysis and solid XOR schema design; three design-level P2 concerns around draft-PR warning semantics, residual --force overwrite risk, and underspecified agent-launch wiring for the pr-checkout intent. |
Sequence Diagram
sequenceDiagram
participant U as User
participant Modal as NewWorkspaceModal
participant Picker as PRLinkCommand
participant GH as gh pr view
participant Pending as PendingPage
participant tRPC as workspaceCreation.checkout
participant Git as git / gh pr checkout
participant Cloud as v2Workspace.create
U->>Modal: Attach PR
Modal->>Picker: onSelect(pr)
Picker->>GH: getGitHubPullRequestContent(prNumber)
GH-->>Picker: { headRefName, baseRefName, headRepositoryOwner, isCrossRepository, state }
Picker-->>Modal: Update draft.linkedPR (full shape)
U->>Modal: Submit
Modal->>Pending: insert pendingWorkspace{ intent: pr-checkout, linkedPR }
Note over Pending: isPrCheckout = linkedPR.headRefName !== undefined
Pending->>tRPC: checkoutWorkspace(buildPrCheckoutPayload)
Note over tRPC: Zod refine: branch XOR pr
tRPC->>tRPC: ensureLocalProject
tRPC->>tRPC: DB check: existing workspace for branch?
alt alreadyExists
tRPC-->>Pending: { alreadyExists: true }
Pending->>U: Navigate to existing workspace
else new checkout
tRPC->>Git: git worktree add --detach worktreePath
tRPC->>Git: gh pr checkout N --branch derivedName --force
alt gh fails
Git-->>tRPC: error
tRPC->>Git: git worktree remove --force worktreePath
tRPC-->>Pending: TRPCError
else success
tRPC->>tRPC: git config push.autoSetupRemote true
tRPC->>tRPC: git config branch.<name>.base = pr.baseRefName
tRPC->>Cloud: ensureV2Host + v2Workspace.create (with rollback)
Cloud-->>tRPC: cloudWorkspace
tRPC->>tRPC: registerWorkspace (local DB insert)
tRPC->>tRPC: maybeRunSetupTerminal
tRPC-->>Pending: { workspace, warnings[] }
Pending->>U: Navigate to new workspace
end
end
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/desktop/plans/20260416-v2-pr-checkout-endpoint.md
Line: 163-165
Comment:
**Draft PRs treated identically to closed/merged**
The condition `input.pr.state !== "open"` includes `"draft"` PRs in the same warning bucket as `"closed"` and `"merged"` states, producing: *"PR is draft — commits are included, but the PR may not merge."*
A draft PR is under active development and is very much expected to merge — it's functionally `open` from a checkout perspective. This message risks unnecessarily alarming users who are doing the right thing (checking out a WIP branch before it's marked ready).
Consider either:
1. Splitting the guard: `["closed", "merged"].includes(input.pr.state)` for the "may not merge" warning, leaving `"draft"` warn-free (or with a softer "This is a draft PR" informational note).
2. Treating `"draft"` as equivalent to `"open"` for the purpose of this check, since both are active, uncommitted states.
The `state` enum in the Zod schema (`"open" | "closed" | "merged" | "draft"`) anticipates this distinction — it's worth reflecting it in the warning logic too.
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/plans/20260416-v2-pr-checkout-endpoint.md
Line: 136-148
Comment:
**`--force` still present despite V1 pain-point #3 listing it as a problem**
V1 pain point #3 specifically calls out that `gh pr checkout --force` "silently overwrites any local branch with the same name," and the improvement claimed is that "V1's 'existing worktree' check fires after the git op, not before."
The plan moves the check before the git op (DB-level idempotency at lines 119–127), which is a real improvement. However, `--force` is still passed to `gh pr checkout`, so the silent-overwrite risk persists for a specific edge case: a git branch named `owner/headRefName` (or `headRefName` for same-repo) that exists in the repo but has *no corresponding workspace row* — for example:
- A previous checkout attempt that created the branch but crashed before `registerWorkspace`.
- A manually created local branch that happens to collide with the derived name.
For the first case `--force` is actually desirable (retry is safe). The second case is the original concern and remains unmitigated. Options to consider:
1. Drop `--force` and let `gh` fail if the branch already exists (surface the error with a message prompting the user to rename or delete it).
2. Keep `--force` but add a pre-flight `git branch --list <name>` check in the worktree, aborting with a clear error if a non-workspace branch would be overwritten.
3. Keep `--force` as-is and document the residual risk explicitly in a code comment near the call.
Option 3 is cheapest; option 2 closes the gap fully. The plan should make the choice explicit so the implementer doesn't inherit the V1 footgun silently.
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/plans/20260416-v2-pr-checkout-endpoint.md
Line: 293-301
Comment:
**Agent-launch wiring for `pr-checkout` is underspecified**
The section says "no change — `buildForkAgentLaunch.ts:354` already consumes `pending.linkedPR`," but `pr-checkout` routes to `checkoutWorkspace`, not the fork creation path. The pending-page dispatch (§2) shows only the workspace-creation call; it does not show what agent-launch builder fires afterward.
Questions that should be answered in the plan:
1. Does the pending page call `buildForkAgentLaunch` after `checkoutWorkspace` completes for a `pr-checkout` intent? If so, the name is misleading and it should be renamed or the reuse should be justified (the function name implies fork-specific logic).
2. Does the `checkout` intent today have its own agent-launch path (separate from `buildForkAgentLaunch`), and if so, does `pr-checkout` share it or fork it?
3. If no agent is launched for `checkout`-family intents (user-initiated checkout, user drives the agent themselves), clarify that explicitly — it changes the user experience meaningfully compared to `fork`.
Leaving this implicit risks the implementer either accidentally double-launching an agent or not launching one at all for `pr-checkout` workspaces.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "docs(desktop): plan for v2 PR checkout v..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: 5
🧹 Nitpick comments (2)
apps/desktop/plans/20260416-v2-pr-checkout-endpoint.md (2)
105-111: Optional: Add language specifier to fenced code block.The static analysis tool flagged the missing language specifier. While this is a flow diagram (not code), adding a specifier improves rendering consistency:
-``` +```text ensuring_repo → ensureLocalProject (shared with create)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/plans/20260416-v2-pr-checkout-endpoint.md` around lines 105 - 111, The fenced flow-diagram block lacks a language specifier; update the triple-backtick fence to include a specifier (e.g., "text") so the block renders consistently. Locate the block containing the lines with ensureLocalProject, creating_worktree, registerWorkspace, and maybeRunSetupTerminal and change the opening fence from ``` to ```text (no other changes to content).
315-323: Consider runtime mitigation for the picker edge case.The identified edge case (PR branch appearing in picker → incorrect base config) relies solely on a code comment for mitigation. While this is acknowledged as rare, consider strengthening it with a runtime check:
🛡️ Optional enhancement: detect potential PR branches at checkout time
In the branch-path section of
checkout, before settingbranch.<name>.base:// In branch-path checkout: const looksLikePrBranch = branch.includes('/') && branch.split('/').length === 2; if (looksLikePrBranch) { warnings.push( `Branch name "${branch}" resembles a PR branch (owner/name pattern). ` + `If this is from a pull request, consider using the PR checkout flow for accurate base tracking.` ); }This provides user-facing guidance without blocking the operation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/plans/20260416-v2-pr-checkout-endpoint.md` around lines 315 - 323, The checkout branch-path currently updates branch.<name>.base without runtime detection of PR-style branch names; add a lightweight runtime check inside the branch-path of the checkout flow (the function/method handling "checkout" before writing branch.<name>.base) that inspects the branch string (the variable named branch) to detect owner/name PR-like patterns (e.g., contains a single '/' and two segments) and, if matched, push a user-visible advisory into the existing warnings array (or similar warnings collector) recommending the PR checkout flow rather than blocking; ensure this check runs only in the branch-path and does not change behavior beyond adding the warning.
🤖 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/plans/20260416-v2-pr-checkout-endpoint.md`:
- Around line 206-209: Update the pending row Zod schema for linkedPR (the
symbol linkedPR in the pending row definition) to ensure headRepositoryOwner is
modeled exactly as decided—either z.object({ login: z.string() }) or
z.string()—and keep linkedPR as z.object({...}).nullable() so nullable vs object
discriminates legacy rows; update the linkedPR fields (prNumber, title, url,
state, headRefName, baseRefName, headRepositoryOwner, isCrossRepository) to
match that shape and ensure legacy/malformed rows still fail at the collection
boundary and are routed to the existing fork logic.
- Around line 163-165: The extraWarnings check uses a case-sensitive comparison
on input.pr.state; update the logic in the extraWarnings assignment so it
normalizes the state before comparing (e.g., use input.pr.state.toLowerCase()
=== "open") or compare against an explicit set of allowed values (e.g.,
["OPEN","open"].includes(input.pr.state)) and handle missing/null states safely
(input.pr?.state) to avoid runtime errors; apply this change where extraWarnings
is defined to ensure it works regardless of schema capitalization.
- Around line 84-93: The pr schema's state and draft handling is incorrect:
update the pr object so that the state property uses the uppercase GitHub API
values (e.g., "OPEN","CLOSED","MERGED") via z.enum and add a separate isDraft:
z.boolean() field (instead of putting "draft" inside state); adjust the existing
pr schema (the pr z.object where state is defined and other fields like
headRefName/headRepositoryOwner) to replace the lowercase state enum with the
uppercase enum and add isDraft to match gh pr view --json output while keeping
the pr object optional if required.
- Around line 223-227: The PrSchema currently defines headRepositoryOwner as
z.string() but gh pr view returns an object; update the PrSchema to make
headRepositoryOwner a nested object schema (e.g., z.object({ login: z.string()
})) and adjust the return-mapping in getGitHubPullRequestContent to extract
headRepositoryOwner.login and isCrossRepository (ensure isCrossRepository is
included in the --json output and mapped through the same return object), so the
mapping uses headRepositoryOwner.login rather than treating headRepositoryOwner
as a string.
---
Nitpick comments:
In `@apps/desktop/plans/20260416-v2-pr-checkout-endpoint.md`:
- Around line 105-111: The fenced flow-diagram block lacks a language specifier;
update the triple-backtick fence to include a specifier (e.g., "text") so the
block renders consistently. Locate the block containing the lines with
ensureLocalProject, creating_worktree, registerWorkspace, and
maybeRunSetupTerminal and change the opening fence from ``` to ```text (no other
changes to content).
- Around line 315-323: The checkout branch-path currently updates
branch.<name>.base without runtime detection of PR-style branch names; add a
lightweight runtime check inside the branch-path of the checkout flow (the
function/method handling "checkout" before writing branch.<name>.base) that
inspects the branch string (the variable named branch) to detect owner/name
PR-like patterns (e.g., contains a single '/' and two segments) and, if matched,
push a user-visible advisory into the existing warnings array (or similar
warnings collector) recommending the PR checkout flow rather than blocking;
ensure this check runs only in the branch-path and does not change behavior
beyond adding the warning.
🪄 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: ea531eee-90c2-4ddc-9039-4e1e3b4c32f0
📒 Files selected for processing (1)
apps/desktop/plans/20260416-v2-pr-checkout-endpoint.md
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 1 file
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/plans/20260416-v2-pr-checkout-endpoint.md">
<violation number="1" location="apps/desktop/plans/20260416-v2-pr-checkout-endpoint.md:90">
P1: `headRepositoryOwner` is defined as `z.string()` here, but `gh pr view --json headRepositoryOwner` returns an object with `{ id, login, name }`. This will fail Zod validation at runtime. Either change the schema to `z.object({ login: z.string() })` and extract `.login` where needed, or document that the caller must pre-extract `.login` before passing the value.</violation>
<violation number="2" location="apps/desktop/plans/20260416-v2-pr-checkout-endpoint.md:92">
P1: `"draft"` is not a valid value for the `state` field returned by `gh pr view`. The GitHub CLI returns `state` (OPEN/CLOSED/MERGED) and `isDraft` (boolean) as separate fields. Including `"draft"` in the state enum will never match real data, and the missing `isDraft` field means draft PRs can't be detected.
Consider:
```ts
state: z.enum(["OPEN", "CLOSED", "MERGED"]),
isDraft: z.boolean(),
```</violation>
<violation number="3" location="apps/desktop/plans/20260416-v2-pr-checkout-endpoint.md:138">
P1: The proposed `gh pr checkout` command still uses `--force`, which can overwrite an existing local branch and reintroduces the exact data-loss risk called out in the v1 pain points.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Drop attach-time PR fetch; reuse existing getGitHubPullRequestContent call at pending-page time (zero net new fetches) - LinkedPR + pending-row schema stay narrow; PR content fetched on demand - Share branch.<name>.base config write across branch + PR paths via composer.baseBranch; fixes current gap where only create records base - Confirm forkOwner/headRefName naming over gh default (collision safety for our workspace-manager use case)
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
apps/desktop/plans/20260416-v2-pr-checkout-endpoint.md (4)
288-299: 🛠️ Refactor suggestion | 🟠 MajorEnsure payload construction reflects the corrected
headRepositoryOwnerschema.When the
prschema is updated to defineheadRepositoryOwneras an object, the payload construction (line 291) should extract theloginfield if not already done at the fetch layer. Verify consistency betweenprContent(returned fromgetGitHubPullRequestContent) and the mutation input schema.If
prContent.headRepositoryOwneris an object, extract the login:pr: { number: prContent.number, url: prContent.url, title: prContent.title, headRefName: prContent.headRefName, baseRefName: prContent.baseRefName, headRepositoryOwner: prContent.headRepositoryOwner.login, // or pass the object if schema updated isCrossRepository: prContent.isCrossRepository, state: prContent.state, isDraft: prContent.isDraft, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/plans/20260416-v2-pr-checkout-endpoint.md` around lines 288 - 299, The payload currently uses prContent.headRepositoryOwner directly but the schema changed to make headRepositoryOwner an object; update the payload construction (the pr object created where pendingId/projectId/workspaceName are assembled) to extract headRepositoryOwner.login from prContent (or, if the mutation schema was also updated to accept the object, explicitly pass prContent.headRepositoryOwner), and ensure this change is consistent with the data returned by getGitHubPullRequestContent and the mutation input type.
176-178:⚠️ Potential issue | 🟡 MinorState comparison will break if schema is fixed to use uppercase.
The condition
input.pr.state !== "open"uses lowercase, but if the schema is corrected to match GitHub's API (uppercase "OPEN", "CLOSED", "MERGED"), this comparison will always be true, incorrectly showing warnings for all PRs.Proposed fix
- extraWarnings: input.pr.state !== "open" + extraWarnings: input.pr.state !== "OPEN"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/plans/20260416-v2-pr-checkout-endpoint.md` around lines 176 - 178, The extraWarnings condition compares input.pr.state to lowercase "open" which will break if the schema uses uppercase (e.g., "OPEN"); update the check in the extraWarnings assignment to normalize the state (e.g., convert input.pr.state to lowercase) or compare against the canonical uppercase values (e.g., "OPEN","CLOSED","MERGED") so the condition correctly detects non-open PRs; locate the extraWarnings expression referencing input.pr.state and apply the normalization/comparison change there.
104-113:⚠️ Potential issue | 🟠 MajorFix state schema to match GitHub CLI's API response structure.
The proposed
state: z.enum(["open", "closed", "merged", "draft"])doesn't align with GitHub'sgh pr view --jsonresponse. GitHub returnsstate(uppercase: OPEN, CLOSED, MERGED) andisDraftas separate fields, not a combined enum.Proposed schema fix
pr: z.object({ number: z.number().int().positive(), url: z.string().url(), title: z.string(), headRefName: z.string(), baseRefName: z.string(), headRepositoryOwner: z.string(), isCrossRepository: z.boolean(), - state: z.enum(["open", "closed", "merged", "draft"]), + state: z.enum(["OPEN", "CLOSED", "MERGED"]), + isDraft: z.boolean(), }).optional(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/plans/20260416-v2-pr-checkout-endpoint.md` around lines 104 - 113, The pr schema's state field is wrong — replace state: z.enum(["open","closed","merged","draft"]) with state: z.enum(["OPEN","CLOSED","MERGED"]) to match GitHub CLI output and add a separate isDraft: z.boolean().optional() field; update any downstream logic that relied on the old "draft" enum to instead check isDraft on the pr object (refer to the pr object, state, and isDraft symbols when making the change).
110-110:⚠️ Potential issue | 🟠 MajorFix
headRepositoryOwnerschema — it's an object, not a string.Line 210 correctly uses
pr.headRepositoryOwner.toLowerCase(), but GitHub CLI'sgh pr view --json headRepositoryOwnerreturns an object with aloginproperty, not a string. The schema and usage are inconsistent with the actual API.Proposed schema and usage fix
Update the schema:
- headRepositoryOwner: z.string(), + headRepositoryOwner: z.object({ login: z.string() }),Then update line 210 in
derivePrLocalBranchName:- const owner = pr.headRepositoryOwner.toLowerCase(); + const owner = pr.headRepositoryOwner.login.toLowerCase();And update the function signature (line 206):
headRefName: string; - headRepositoryOwner: string; + headRepositoryOwner: { login: string }; isCrossRepository: boolean;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/plans/20260416-v2-pr-checkout-endpoint.md` at line 110, The headRepositoryOwner schema is declared as z.string() but GitHub returns an object with a login property; update the schema for headRepositoryOwner to z.object({ login: z.string() }) (or z.object({ login: z.string() }).nullable() if nullable) and change all uses to access the login field (e.g., inside derivePrLocalBranchName replace pr.headRepositoryOwner.toLowerCase() with pr.headRepositoryOwner.login.toLowerCase()); also update the derivePrLocalBranchName function signature/type to expect headRepositoryOwner as that object shape so typings remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/desktop/plans/20260416-v2-pr-checkout-endpoint.md`:
- Around line 288-299: The payload currently uses prContent.headRepositoryOwner
directly but the schema changed to make headRepositoryOwner an object; update
the payload construction (the pr object created where
pendingId/projectId/workspaceName are assembled) to extract
headRepositoryOwner.login from prContent (or, if the mutation schema was also
updated to accept the object, explicitly pass prContent.headRepositoryOwner),
and ensure this change is consistent with the data returned by
getGitHubPullRequestContent and the mutation input type.
- Around line 176-178: The extraWarnings condition compares input.pr.state to
lowercase "open" which will break if the schema uses uppercase (e.g., "OPEN");
update the check in the extraWarnings assignment to normalize the state (e.g.,
convert input.pr.state to lowercase) or compare against the canonical uppercase
values (e.g., "OPEN","CLOSED","MERGED") so the condition correctly detects
non-open PRs; locate the extraWarnings expression referencing input.pr.state and
apply the normalization/comparison change there.
- Around line 104-113: The pr schema's state field is wrong — replace state:
z.enum(["open","closed","merged","draft"]) with state:
z.enum(["OPEN","CLOSED","MERGED"]) to match GitHub CLI output and add a separate
isDraft: z.boolean().optional() field; update any downstream logic that relied
on the old "draft" enum to instead check isDraft on the pr object (refer to the
pr object, state, and isDraft symbols when making the change).
- Line 110: The headRepositoryOwner schema is declared as z.string() but GitHub
returns an object with a login property; update the schema for
headRepositoryOwner to z.object({ login: z.string() }) (or z.object({ login:
z.string() }).nullable() if nullable) and change all uses to access the login
field (e.g., inside derivePrLocalBranchName replace
pr.headRepositoryOwner.toLowerCase() with
pr.headRepositoryOwner.login.toLowerCase()); also update the
derivePrLocalBranchName function signature/type to expect headRepositoryOwner as
that object shape so typings remain consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6d52f165-20df-4083-bc3e-231eb563cb80
📒 Files selected for processing (1)
apps/desktop/plans/20260416-v2-pr-checkout-endpoint.md
Widen the checkout procedure with an optional `pr` field (structured PR metadata) so the modal's linkedPR can materialize the PR branch into a worktree via `gh pr checkout`. Exactly one of `branch` or `pr` enforced at the zod layer. - `getGitHubPullRequestContent` surfaces `headRepositoryOwner` and `isCrossRepository` (already returned by `gh pr view`, now mapped) - `derivePrLocalBranchName` pure helper: `<forkOwner>/<headRefName>` for cross-repo PRs, head ref as-is for same-repo - New `finishCheckout` local helper: `branch.<name>.base` config write + cloud register + rollback + local insert + setup terminal. Called from both the new PR path and the existing branch path — regular checkouts now also record their base (previously a gap where only `create` did) - PR path: detached worktree → `gh pr checkout --branch <derived> --force` → push.autoSetupRemote; returns `alreadyExists: true` on idempotent re-checkout of an existing workspace's branch - `composer.baseBranch` added; client fills from `pr.baseRefName` (PR mode) or picker selection (branch mode) - Warning surfaced when PR state is not "open" - `execGh` accepts cwd/timeout options; falls back to raw stdout when output isn't JSON (for `gh pr checkout`, which doesn't return JSON)
…aunch Modal routes to `intent: "pr-checkout"` whenever a linkedPR is attached (replaces the old fork-with-PR behavior that never checked out the PR's branch). The pending page fetches `getGitHubPullRequestContent` once, feeds the result into both the `checkout` mutation's pr payload and the agent-launch resolver — zero net new fetches vs the previous flow, which fetched the same data later for the prompt body. - pendingWorkspaceSchema.intent: "pr-checkout" added - useSubmitWorkspace: selects intent + placeholder name/branch from linkedPR when present - useCheckoutDashboardWorkspace: CheckoutWorkspaceInput widened with optional pr, composer.baseBranch - buildIntentPayload: buildPrCheckoutPayload pure builder + unit tests; buildCheckoutPayload plumbs composer.baseBranch - page.tsx useFireIntent: new pr-checkout case — imperative getGitHubPullRequestContent → buildPrCheckoutPayload → checkout.mutate → resolvedPr passed to dispatchForkLaunch so the agent-launch resolver skips a re-fetch - buildForkAgentLaunch: accepts resolvedPr; fetchPullRequest resolver returns it directly when URL matches - dispatchForkLaunch: threads resolvedPr through
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/page.tsx (3)
266-285:⚠️ Potential issue | 🟠 Major
pr-checkoutwon't show progress steps — the poll is gated off.
intentHasProgressonly matches"fork"and"checkout", so thegetProgressquery isenabled: falsefor the"pr-checkout"intent. Per PR objectives §"pending-row + retry + progress steps", the PR flow instruments multi-step progress on the host side (detached worktree add →gh pr checkout --force→ cloud register → setup terminal), but the user will only see the generic spinner branch at L425-432. Include"pr-checkout"here:🔧 Proposed fix
- const intentHasProgress = - pending?.intent === "fork" || pending?.intent === "checkout"; + const intentHasProgress = + pending?.intent === "fork" || + pending?.intent === "checkout" || + pending?.intent === "pr-checkout";🤖 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/pending/`$pendingId/page.tsx around lines 266 - 285, The enabled check for polling progress excludes the "pr-checkout" intent causing progress steps to be hidden; update the intentHasProgress logic in this component so it also returns true for "pr-checkout" (modify the intentHasProgress declaration used by the useQuery enabled predicate) so that useQuery({ queryKey: ["workspaceCreation", "getProgress", pendingId, hostUrl], ... enabled: pending?.status === "creating" && !!hostUrl && intentHasProgress }) will run for fork, checkout, and pr-checkout intents.
363-368:⚠️ Potential issue | 🟡 MinorMissing
creatingLabelbranch for"pr-checkout".The label falls through to
"Creating workspace...", which is misleading for the PR flow. Minor UX polish:🔧 Proposed fix
const creatingLabel = pending.intent === "adopt" ? "Adopting worktree..." : pending.intent === "checkout" ? "Checking out branch..." - : "Creating workspace..."; + : pending.intent === "pr-checkout" + ? "Checking out pull request..." + : "Creating workspace...";🤖 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/pending/`$pendingId/page.tsx around lines 363 - 368, The creatingLabel fallback misses the "pr-checkout" intent, causing PR checkouts to show "Creating workspace..."; update the conditional that assigns creatingLabel (the expression referencing pending.intent and creatingLabel) to include a branch for pending.intent === "pr-checkout" and return an appropriate label like "Checking out PR..." so PR flows display the correct message.
156-179:⚠️ Potential issue | 🟠 MajorAdd
alreadyExiststo result type and short-circuit dispatch for existing pr-checkout workspaces.The host-service
checkoutprocedure returnsalreadyExists: truewhen a workspace already exists for a PR's branch (see workspace-creation.ts:1052-1053), but the renderer'sresulttype doesn't expose this field. This causesneedsLaunchDispatchto becometrueunconditionally, which then:
- Rebuilds and fires a fork-agent launch into an existing workspace.
- Rewrites
.superset/attachments/*into the worktree, potentially overwriting files.- Treats re-opening an idempotent workspace as a fresh creation, losing existing state.
Add
alreadyExists?: booleanto the result type and check!result.alreadyExistsin the dispatch gate to match the documented intent and preserve workspace state on re-open.🔧 Suggested fix
let result: { workspace?: { id?: string } | null; terminals?: Array<{ id: string; role: string; label: string }>; warnings?: string[]; + alreadyExists?: boolean; }; ... -const needsLaunchDispatch = - (pending.intent === "fork" || pending.intent === "pr-checkout") && - !!result.workspace?.id; +const needsLaunchDispatch = + (pending.intent === "fork" || pending.intent === "pr-checkout") && + !!result.workspace?.id && + !result.alreadyExists;🤖 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/pending/`$pendingId/page.tsx around lines 156 - 179, The result type returned from the host-service checkout needs to include alreadyExists?: boolean and the launch gate should short-circuit when a workspace already exists; update the result type (so callers like the code using result in page.tsx can see result.alreadyExists), then change the needsLaunchDispatch logic in the block that computes needsLaunchDispatch (which currently checks pending.intent and result.workspace?.id) to also require !result.alreadyExists before calling dispatchForkLaunch; keep the rest of the dispatch payload (agentConfigs from trpcUtils.settings.getAgentPresets.fetch, dispatchForkLaunch call, and the collections.pendingWorkspaces.update callback that uses pendingId) unchanged.
🧹 Nitpick comments (2)
packages/host-service/src/trpc/router/workspace-creation/utils/exec-gh.ts (1)
17-36: The polymorphic return type is maintainable as-is, but the design could be clearer.The current call sites use Zod schema validation rather than type assertions, so they're safe from the runtime type mismatch risk described. However, the function does return three different types (
object | string | {}), which is confusing for future callers:
- Line 1101:
pr checkoutdoesn't use the return value.- Lines 1554, 1592:
issue view --jsonandpr view --jsonpass the return toIssueSchema.parse()andPrSchema.parse(), which reject both strings and empty objects if required fields are missing.The
{}fallback for empty stdout is semantically odd—ifgh --jsonreturns nothing, an empty object doesn't accurately represent that. Consider returningnullor throwing instead. If you keep the polymorphic form, a mode selector (e.g.,{ mode: "json" | "text" }) would clarify intent for maintainers and prevent future misuse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/trpc/router/workspace-creation/utils/exec-gh.ts` around lines 17 - 36, The execGh function currently returns an ambiguous polymorphic value (object|string|{}) which is confusing; change execGh to return a clear discriminated form or null: add an options.mode?: "json"|"text" parameter to execGh (or default to "json"), have execGh call execFileAsync as before, but when stdout.trim() is empty return null (not {}), when mode==="json" attempt JSON.parse and throw a descriptive error on parse failure, and when mode==="text" return the trimmed string; update callers that expect JSON (e.g. where IssueSchema.parse and PrSchema.parse are used) to call execGh([...], { mode: "json" }) and handle nulls, and callers that ignore the result (e.g. pr checkout) can call execGh([...], { mode: "text" }) or ignore a null result; reference execGh, getStrictShellEnvironment, and execFileAsync when locating the implementation.packages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts (1)
1112-1123: Cleanup ongh pr checkoutfailure doesn't remove a partially-created branch.If
gh pr checkoutfails mid-operation after creating the local branch<derived>but before completing remote setup, theworktree remove --forcecall leaves the branch behind. A retry then hits the--forcepath against a stale half-configured branch, or the idempotency check next time around sees nothing inworkspacesbut the branch ref still exists (making diagnostics confusing).Best-effort
git branch -D <derived>alongside the worktree removal would tighten this up.♻️ Suggested cleanup
} catch (err) { await git .raw(["worktree", "remove", "--force", worktreePath]) .catch(() => {}); + await git.raw(["branch", "-D", branch]).catch(() => {}); clearProgress(input.pendingId); throw new TRPCError({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts` around lines 1112 - 1123, The catch block after the `gh pr checkout` attempt (where `git.raw(["worktree", "remove", "--force", worktreePath])` is called) should also attempt best-effort removal of any partially-created local branch named by `derived` to avoid stale refs; update the error handler to call `git.raw(["branch", "-D", derived])` (or equivalent) alongside the worktree removal and swallow any errors from that call (like the existing `.catch(() => {})`), then proceed to `clearProgress(input.pendingId)` and throw the TRPCError as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-name.ts`:
- Around line 15-17: derivePrLocalBranchName currently only checks falsy
headRefName so a whitespace-only string like " " is allowed and produces
invalid branch names; update the guard in derivePrLocalBranchName to trim
pr.headRefName, validate that the trimmed value is non-empty (throw the existing
Error with the same message if empty), and then use the trimmed value for all
subsequent logic (both same-repo and cross-repo branch name construction) so you
never emit owner/" " or branch names with trailing/leading whitespace.
In
`@packages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts`:
- Around line 1083-1111: The current call to execGh("pr checkout", "--force")
can clobber an unrelated local branch named by branch; before invoking execGh
(after adding the detached worktree but before the execGh call that uses
worktreePath, input.pr.number and branch), check for the existence of a local
branch with the same name in the repository (use git.raw or git.branch/lookup
APIs) that is not inside the new worktree; if one exists either (A) abort with
clearProgress(input.pendingId) and throw a TRPCError({ code: "CONFLICT",
message: "Local branch exists: <branch>" }) so the caller can resolve, or (B)
programmatically rename the existing local branch to a collision-free name and
add a warning to the response warnings[] before proceeding; implement one of
these flows and remove the unconditional "--force" arg from execGh when choosing
option (A) or keep it only after a safe rename when choosing (B).
- Around line 1645-1646: PrSchema's headRepositoryOwner is currently
non-nullable which causes PrSchema.parse(raw) to throw when GitHub returns null
for deleted fork heads; update the schema for headRepositoryOwner to allow null
(e.g., optional or z.nullable) and adjust downstream handling where
isCrossRepository and headRepositoryOwner are used (e.g., workspace-creation
logic that derives cross-repo info and the renderer / PR input types) to either
treat null as same-repo fallback or explicitly reject with a clear error when
isCrossRepository === true but owner is null, and update any type definitions
(renderer/PR-input types) to accept the widened nullable owner.
---
Outside diff comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/`$pendingId/page.tsx:
- Around line 266-285: The enabled check for polling progress excludes the
"pr-checkout" intent causing progress steps to be hidden; update the
intentHasProgress logic in this component so it also returns true for
"pr-checkout" (modify the intentHasProgress declaration used by the useQuery
enabled predicate) so that useQuery({ queryKey: ["workspaceCreation",
"getProgress", pendingId, hostUrl], ... enabled: pending?.status === "creating"
&& !!hostUrl && intentHasProgress }) will run for fork, checkout, and
pr-checkout intents.
- Around line 363-368: The creatingLabel fallback misses the "pr-checkout"
intent, causing PR checkouts to show "Creating workspace..."; update the
conditional that assigns creatingLabel (the expression referencing
pending.intent and creatingLabel) to include a branch for pending.intent ===
"pr-checkout" and return an appropriate label like "Checking out PR..." so PR
flows display the correct message.
- Around line 156-179: The result type returned from the host-service checkout
needs to include alreadyExists?: boolean and the launch gate should
short-circuit when a workspace already exists; update the result type (so
callers like the code using result in page.tsx can see result.alreadyExists),
then change the needsLaunchDispatch logic in the block that computes
needsLaunchDispatch (which currently checks pending.intent and
result.workspace?.id) to also require !result.alreadyExists before calling
dispatchForkLaunch; keep the rest of the dispatch payload (agentConfigs from
trpcUtils.settings.getAgentPresets.fetch, dispatchForkLaunch call, and the
collections.pendingWorkspaces.update callback that uses pendingId) unchanged.
---
Nitpick comments:
In `@packages/host-service/src/trpc/router/workspace-creation/utils/exec-gh.ts`:
- Around line 17-36: The execGh function currently returns an ambiguous
polymorphic value (object|string|{}) which is confusing; change execGh to return
a clear discriminated form or null: add an options.mode?: "json"|"text"
parameter to execGh (or default to "json"), have execGh call execFileAsync as
before, but when stdout.trim() is empty return null (not {}), when mode==="json"
attempt JSON.parse and throw a descriptive error on parse failure, and when
mode==="text" return the trimmed string; update callers that expect JSON (e.g.
where IssueSchema.parse and PrSchema.parse are used) to call execGh([...], {
mode: "json" }) and handle nulls, and callers that ignore the result (e.g. pr
checkout) can call execGh([...], { mode: "text" }) or ignore a null result;
reference execGh, getStrictShellEnvironment, and execFileAsync when locating the
implementation.
In
`@packages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts`:
- Around line 1112-1123: The catch block after the `gh pr checkout` attempt
(where `git.raw(["worktree", "remove", "--force", worktreePath])` is called)
should also attempt best-effort removal of any partially-created local branch
named by `derived` to avoid stale refs; update the error handler to call
`git.raw(["branch", "-D", derived])` (or equivalent) alongside the worktree
removal and swallow any errors from that call (like the existing `.catch(() =>
{})`), then proceed to `clearProgress(input.pendingId)` and throw the TRPCError
as before.
🪄 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: abe158db-4f87-45ab-99bd-8e5c53e0267a
📒 Files selected for processing (12)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildForkAgentLaunch.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/dispatchForkLaunch.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/page.tsxapps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/hooks/useSubmitWorkspace/useSubmitWorkspace.tsapps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/hooks/useCheckoutDashboardWorkspace/useCheckoutDashboardWorkspace.tsapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.tspackages/host-service/src/trpc/router/workspace-creation/utils/exec-gh.tspackages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-name.test.tspackages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-name.tspackages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts
Kitenite
changed the title
… branch Before running \`gh pr checkout --force\`, probe for a pre-existing local branch with the derived name via \`git show-ref --verify\`. If found, surface a warning pointing at \`git reflog\` for recovery — the user's point-and-click flow still completes, but they're informed and can recover any unpushed commits that got reset. Addresses PR review feedback (coderabbit/greptile/cubic-dev-ai): the idempotency check rules out Superset-managed workspaces but not stray branches created by a user's prior manual \`gh pr checkout\` in the primary working tree. Blocking would force CLI-level recovery, which is poor UX for a modal-driven flow; warning + reflog recovery is the right balance.
…ed forks
Two remaining PR review items:
1. `derivePrLocalBranchName` now trims `headRefName` before the empty
check — previously a whitespace-only input (e.g. " ") slipped past
the guard and produced a garbage branch name like "owner/ ".
2. `PrSchema.headRepositoryOwner` is now nullable — `gh pr view` returns
null when the PR's head fork repository has been deleted, which
previously crashed the zod parse. The getGitHubPullRequestContent
return mapping uses `?.login ?? null`; buildPrCheckoutPayload fails
fast on cross-repo PRs with null owner with a clear user-facing
message ("head fork repository has been deleted") rather than letting
the opaque server error bubble up. Same-repo PRs pass through an
empty owner string (unused by the derivation).
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts (1)
1668-1669:⚠️ Potential issue | 🟠 Major
headRepositoryOwneris still non-nullable — deleted-fork PRs will breakPrSchema.parse.This matches the prior review flag that does not appear to have been addressed. GitHub returns
nullforheadRepositoryOwnerwhen the head fork has been deleted (common on old closed/merged PRs), soz.object({ login: z.string() }).parse(...)on line 1624 will throw and surface as an opaqueFailed to fetch PR#N`` error from the catch at 1640–1644 — blocking both the info fetch and the PR checkout flow.Suggest making it nullable and having consumers treat a null owner as same-repo (or reject cleanly when
isCrossRepository === truebut owner is null):🛡️ Proposed schema/output relaxation
- headRepositoryOwner: z.object({ login: z.string() }), + headRepositoryOwner: z.object({ login: z.string() }).nullable(), isCrossRepository: z.boolean(),And at line 1633:
- headRepositoryOwner: data.headRepositoryOwner.login, + headRepositoryOwner: data.headRepositoryOwner?.login ?? null,Then widen
headRepositoryOwnerin theprinput schema (line 978) andderivePrLocalBranchNameto acceptstring | null, falling back to same-repo semantics when absent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts` around lines 1668 - 1669, The Pr schema currently requires headRepositoryOwner (z.object({ login: z.string() })) which will throw on deleted-fork PRs; change headRepositoryOwner to be nullable (e.g., z.object({ login: z.string() }).nullable()) in the Pr schema and in the `pr` input schema, update any typing to allow string | null for owner login, and modify `derivePrLocalBranchName` to accept string | null and treat null as same-repo semantics (or explicitly throw a clear, handled error when `isCrossRepository === true` and owner is null); also audit callers of `PrSchema.parse`/`derivePrLocalBranchName` to handle the nullable owner rather than letting parse fail with an opaque error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@packages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts`:
- Around line 1668-1669: The Pr schema currently requires headRepositoryOwner
(z.object({ login: z.string() })) which will throw on deleted-fork PRs; change
headRepositoryOwner to be nullable (e.g., z.object({ login: z.string()
}).nullable()) in the Pr schema and in the `pr` input schema, update any typing
to allow string | null for owner login, and modify `derivePrLocalBranchName` to
accept string | null and treat null as same-repo semantics (or explicitly throw
a clear, handled error when `isCrossRepository === true` and owner is null);
also audit callers of `PrSchema.parse`/`derivePrLocalBranchName` to handle the
nullable owner rather than letting parse fail with an opaque error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e858f73d-32dc-4127-ac57-c36d66407f2c
📒 Files selected for processing (1)
packages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/pending/`$pendingId/buildIntentPayload.ts:
- Around line 110-121: The current normalization silently coerces any
prContent.state to "open"; update the boundary so unrecognized states fail fast:
tighten the incoming type for prContent.state (e.g. make
getGitHubPullRequestContent return a discriminated union of "open" | "closed" |
"merged"), and replace the ternary fallback that sets normalizedState with an
explicit check on prContent.state that returns one of the three valid values or
throws a descriptive Error (referencing prContent.state and the normalizedState
variable inside buildIntentPayload) so any unknown upstream values are caught
immediately.
In
`@packages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts`:
- Around line 1047-1185: Idempotency early-return can return a stale DB row if
the on-disk worktree was manually deleted; after finding existing via
ctx.db.query.workspaces.findFirst (variable existing), check the filesystem with
fs.existsSync(existing.worktreePath) and if the path is missing,
clearProgress(input.pendingId) and treat it as non-existent (i.e., either delete
the stale row via ctx.db.mutation.workspaces.delete or simply continue the
create flow) instead of returning alreadyExists: true; if the path exists, keep
the current early-return behavior that returns { workspace: { id: existing.id },
alreadyExists: true }.
- Around line 322-341: The current checkout code unconditionally writes
branch.<name>.base via args.git.raw which can silently overwrite an existing
base saved by another workspace; change the logic in the checkout/registration
block (where setProgress is called and args.baseBranch is checked) to first read
the existing value with args.git.raw(["-C", args.worktreePath, "config",
"--get", `branch.${args.branch}.base`]) and compare it to args.baseBranch, and
only call args.git.raw([... "config", `branch.${args.branch}.base`,
args.baseBranch]) when the existing value is different or absent; preserve the
existing .catch handling for the write and handle the read error by treating it
as "absent" so you don't overwrite needlessly when values match.
🪄 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: 57326864-b6a4-442b-bfb9-0cebb2331fdb
📒 Files selected for processing (6)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildForkAgentLaunch.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.tspackages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-name.test.tspackages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-name.tspackages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts
✅ Files skipped from review due to trivial changes (1)
- packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-name.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-name.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.test.ts
MocA-Love
pushed a commit
to MocA-Love/superset
that referenced
this pull request
Apr 17, 2026
…t-sh#3525) * docs(desktop): plan for v2 PR checkout via widened checkout procedure Design doc for wiring up linkedPR → worktree materialization in the v2 new workspace modal. Extends workspaceCreation.checkout with an optional `pr` field (shells to `gh pr checkout`) rather than adding a new tRPC procedure; client keeps a distinct `pr-checkout` pending intent for progress labels and payload construction. * docs(desktop): refine v2 PR checkout plan after walkthrough - Drop attach-time PR fetch; reuse existing getGitHubPullRequestContent call at pending-page time (zero net new fetches) - LinkedPR + pending-row schema stay narrow; PR content fetched on demand - Share branch.<name>.base config write across branch + PR paths via composer.baseBranch; fixes current gap where only create records base - Confirm forkOwner/headRefName naming over gh default (collision safety for our workspace-manager use case) * feat(host-service): PR-checkout mode in workspaceCreation.checkout Widen the checkout procedure with an optional `pr` field (structured PR metadata) so the modal's linkedPR can materialize the PR branch into a worktree via `gh pr checkout`. Exactly one of `branch` or `pr` enforced at the zod layer. - `getGitHubPullRequestContent` surfaces `headRepositoryOwner` and `isCrossRepository` (already returned by `gh pr view`, now mapped) - `derivePrLocalBranchName` pure helper: `<forkOwner>/<headRefName>` for cross-repo PRs, head ref as-is for same-repo - New `finishCheckout` local helper: `branch.<name>.base` config write + cloud register + rollback + local insert + setup terminal. Called from both the new PR path and the existing branch path — regular checkouts now also record their base (previously a gap where only `create` did) - PR path: detached worktree → `gh pr checkout --branch <derived> --force` → push.autoSetupRemote; returns `alreadyExists: true` on idempotent re-checkout of an existing workspace's branch - `composer.baseBranch` added; client fills from `pr.baseRefName` (PR mode) or picker selection (branch mode) - Warning surfaced when PR state is not "open" - `execGh` accepts cwd/timeout options; falls back to raw stdout when output isn't JSON (for `gh pr checkout`, which doesn't return JSON) * feat(desktop): wire pr-checkout intent through pending page + agent launch Modal routes to `intent: "pr-checkout"` whenever a linkedPR is attached (replaces the old fork-with-PR behavior that never checked out the PR's branch). The pending page fetches `getGitHubPullRequestContent` once, feeds the result into both the `checkout` mutation's pr payload and the agent-launch resolver — zero net new fetches vs the previous flow, which fetched the same data later for the prompt body. - pendingWorkspaceSchema.intent: "pr-checkout" added - useSubmitWorkspace: selects intent + placeholder name/branch from linkedPR when present - useCheckoutDashboardWorkspace: CheckoutWorkspaceInput widened with optional pr, composer.baseBranch - buildIntentPayload: buildPrCheckoutPayload pure builder + unit tests; buildCheckoutPayload plumbs composer.baseBranch - page.tsx useFireIntent: new pr-checkout case — imperative getGitHubPullRequestContent → buildPrCheckoutPayload → checkout.mutate → resolvedPr passed to dispatchForkLaunch so the agent-launch resolver skips a re-fetch - buildForkAgentLaunch: accepts resolvedPr; fetchPullRequest resolver returns it directly when URL matches - dispatchForkLaunch: threads resolvedPr through * fix(host-service): warn user when pr-checkout clobbers existing local branch Before running \`gh pr checkout --force\`, probe for a pre-existing local branch with the derived name via \`git show-ref --verify\`. If found, surface a warning pointing at \`git reflog\` for recovery — the user's point-and-click flow still completes, but they're informed and can recover any unpushed commits that got reset. Addresses PR review feedback (coderabbit/greptile/cubic-dev-ai): the idempotency check rules out Superset-managed workspaces but not stray branches created by a user's prior manual \`gh pr checkout\` in the primary working tree. Blocking would force CLI-level recovery, which is poor UX for a modal-driven flow; warning + reflog recovery is the right balance. * fix(host-service): harden pr-checkout against whitespace refs + deleted forks Two remaining PR review items: 1. `derivePrLocalBranchName` now trims `headRefName` before the empty check — previously a whitespace-only input (e.g. " ") slipped past the guard and produced a garbage branch name like "owner/ ". 2. `PrSchema.headRepositoryOwner` is now nullable — `gh pr view` returns null when the PR's head fork repository has been deleted, which previously crashed the zod parse. The getGitHubPullRequestContent return mapping uses `?.login ?? null`; buildPrCheckoutPayload fails fast on cross-repo PRs with null owner with a clear user-facing message ("head fork repository has been deleted") rather than letting the opaque server error bubble up. Same-repo PRs pass through an empty owner string (unused by the derivation).
2 tasks
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
Apr 17, 2026
upstream取り込み: v2 PR checkout (superset-sh#3525)
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
Apr 17, 2026
Upstream commits processed (cherry-picked, then adapted where needed): - 07c1ee0 fix(desktop): Cmd+O firing open-in twice on v1 workspace route (superset-sh#3511) → PR #302 (with fork tearoff-window adaptation for Cmd+O) - 4a1f41a chore(deps): upgrade tanstack/db + electric, drop durable-streams patch (superset-sh#3509) → PR #303 (fork keeps fstream patch) - a3df489 feat(desktop): v2 PR checkout via widened checkout procedure (superset-sh#3525) → PR #304 (clean) - c504a50 feat(desktop): v2 file editor — foundation, views, and stability pass (superset-sh#3526) → PR #310 (foundation: 58 files path-checkout) → PR #311 (adaptation: 20 files manual port with SpreadsheetViewer/memo/fork-hotkeys preserved) - 78b7dc8 feat(desktop): promote "Create Section Below" to top-level on workspace menu (superset-sh#3537) → PR #308 Record merge so upstream/main..main shows 0 behind.
This was referenced Apr 24, 2026
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
Implements PR checkout in the v2 new-workspace modal. When a user attaches a PR via the "Link PR" field, submit now materializes the PR's branch into a worktree (via
gh pr checkout) instead of forking a new branch offbaseBranch. PreviouslylinkedPRwas only fed to the agent as prompt context; the workspace itself had no PR commits.Widens
workspaceCreation.checkoutwith an optionalprfield rather than adding a new procedure —checkoutalready means "materialize an externally-defined branch into a worktree", and a PR branch is just a variant. Zod.refineenforces{branch} XOR {pr}. The client still has a distinctpr-checkoutpending intent for progress labels, payload construction, and idempotency navigation — but both modes route to the same tRPC mutation.What changed
Server (
packages/host-service):getGitHubPullRequestContentsurfacesheadRepositoryOwner+isCrossRepository(already fetched bygh pr view, now mapped)workspaceCreation.checkoutwidened: optionalpr(number/url/title/headRefName/baseRefName/headRepositoryOwner/isCrossRepository/state), optionalcomposer.baseBranchfinishCheckoutlocal helper — shared postlude for both paths. Writesbranch.<name>.basefromcomposer.baseBranch(fixes a gap — regular picker-driven checkouts previously didn't record their base; onlycreatedid)gh pr checkout --branch <derived> --force→push.autoSetupRemotealreadyExists: true, no git opswarnings[], not blockedderivePrLocalBranchNamepure helper:<forkOwner>/<headRefName>for cross-repo, head ref as-is for same-repoexecGhacceptscwd/timeout; tolerates non-JSON stdout forgh pr checkoutRenderer (
apps/desktop):pendingWorkspaceSchema.intentgains"pr-checkout"useSubmitWorkspace: selects intent + placeholder name/branch when linkedPR is attacheduseCheckoutDashboardWorkspace: input widened to mirror server (optionalpr,composer.baseBranch)buildPrCheckoutPayloadnew pure builder;buildCheckoutPayloadplumbscomposer.baseBranchuseFireIntent: newpr-checkoutcase fetchesgetGitHubPullRequestContentimperatively, builds payload, firescheckout.mutate, threadsresolvedPrtodispatchForkLaunchbuildForkAgentLaunchacceptsresolvedPr—fetchPullRequestresolver returns it when URL matches, skipping the redundant tRPC callZero net new fetches. Today fork-with-PR fetches
getGitHubPullRequestContentat agent-launch time (for body). New flow fetches the same call at pending-page time, shared between mutation payload and agent-launch resolver.Decisions (see plan §4)
createFromPrproceduregh pr checkoutas fetch mechanism (requiresgh auth login)pr.baseRefName; server readscomposer.baseBranchuniformly, no intent branching for the config write<forkOwner>/<headRefName>(v1 scheme) over gh default — empirically verified gh uses no prefix, which risks silent overwrite of user's real branches in our workspace-manager use caseFull design doc:
apps/desktop/plans/20260416-v2-pr-checkout-endpoint.mdVerification
derivePrLocalBranchName: 9 cases (same-repo, cross-repo, case-folding, slash preservation, empty-field rejection)buildPrCheckoutPayload: 7 cases (shape mapping, state normalization, baseBranch plumbing)Manual smoke test plan
<forkOwner>/<headRefName>, fork remote added by ghalreadyExists: truenavigation (no git ops)ghnot authenticated / not installed: clear error at pending page (no silent fallback to fork)Summary by CodeRabbit