Skip to content

upstream取り込み: v2 PR checkout (#3525)#304

Merged
MocA-Love merged 1 commit intomainfrom
upstream-merge/pr3-v2-pr-checkout
Apr 17, 2026
Merged

upstream取り込み: v2 PR checkout (#3525)#304
MocA-Love merged 1 commit intomainfrom
upstream-merge/pr3-v2-pr-checkout

Conversation

@MocA-Love
Copy link
Copy Markdown
Owner

@MocA-Love MocA-Love commented Apr 17, 2026

概要

upstream superset-sh/superset から v2 PR checkout 機能 (superset-sh#3525) を取り込む PR です。behind 2 → 1。

取り込み commit

SHA タイトル 分類
a3df48954 feat(desktop): v2 PR checkout via widened checkout procedure (superset-sh#3525) おそらく安全

fork 適応修正

なし。workspace-creation.ts のみ auto-merge、fork 独自の 34391277c 修正と共存。

Codex pre-review: v2 固有バグ 3件を defer

以下はすべて upstream 由来v2 workspace creation フロー (/_dashboard/pending/\$pendingId) 内完結。fork 独自領域 (auto-updater / SpreadsheetViewer / quit lifecycle / Better Auth / packages/panes) への影響は無し。v2 PR checkout は新機能で現時点で fork 内の運用影響が小さいため、upstream 本流での修正を待つ方針で取り込みます。

  • Major: pr-checkout で添付ファイルが無言で消失
    useSubmitWorkspace は attachments を保存して attachmentCount を立てるが、page.tsxpr-checkout 分岐は loadAttachments() を呼ばず、buildPrCheckoutPayloadlinkedContext.attachments を渡さないまま末尾で clearAttachments() される
  • Major: alreadyExists: true の idempotent path を renderer が未処理
    workspace-creation.ts は既存 workspace で alreadyExists: true を返すが、page.tsx はフラグを見ずに pr-checkout で毎回 dispatchForkLaunch() を再実行 → terminal/chat pane が再オープン
  • Minor: page.tsx::intentHasProgress が `"fork" | "checkout"` のみで新規 `"pr-checkout"` を含めていない → progress UI 欠損(host-service 側は checkout と同じ progress を更新済)

テストチェックリスト

  • v2 PR checkout の基本フロー(same-repo PR / cross-repo PR / deleted fork head)
  • v1 の PR checkout に regression なし

Summary by CodeRabbit

リリースノート

  • 新機能
    • プルリクエストからのワークスペース作成に対応しました
    • PR チェックアウト機能を追加しました
    • ベースブランチの設定管理に対応しました

…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).
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 17, 2026

📝 Walkthrough

Walkthrough

このプルリクエストは、V2ワークスペース作成システムにPRチェックアウト機能を追加します。レンダラー側で新しい「pr-checkout」インテントを導入し、PR固有のペイロードビルダーを実装し、ホストサービス側でPR用の独立したチェックアウトフローを確立し、確定的なローカルブランチ名派生とベースブランチ永続化を追加します。

Changes

Cohort / File(s) Summary
PR Checkout Feature Plan
apps/desktop/plans/20260416-v2-pr-checkout-endpoint.md
新しいPRチェックアウト機能の完全な計画ドキュメント、実装仕様、テスト戦略を含む。
Renderer Pending Page & Intent Dispatch
apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/page.tsx, apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.ts, apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.test.ts
新しいpr-checkoutインテント処理、buildPrCheckoutPayloadビルダー、PR内容の事前フェッチ、PR固有のペイロード構築とテストを実装。
Renderer Fork Agent Launch & Dispatch
apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildForkAgentLaunch.ts, apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/dispatchForkLaunch.ts
事前解決されたPRペイロードの再利用をサポートするResolvedPrContent型と、冗長なGitHub PR内容フェッチをバイパスするロジック。
Renderer Form Submission & Checkout Schema
apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/hooks/useSubmitWorkspace/useSubmitWorkspace.ts, apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/hooks/useCheckoutDashboardWorkspace/useCheckoutDashboardWorkspace.ts, apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.ts
PR モード検出、PR派生のペイロード値、チェックアウト入力スキーマの拡張(PR対応の排他的選択肢)、pr-checkoutインテント値の追加。
Host Service Utilities
packages/host-service/src/trpc/router/workspace-creation/utils/exec-gh.ts, packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-name.ts, packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-name.test.ts
ghコマンド実行の強化(オプション、出力フォールバック)、PR固有のローカルブランチ名派生ユーティリティとその包括的なテスト。
Host Service Workspace Creation
packages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts
チェックアウト入力を分別化(branchまたはprの排他的選択肢)、PR検出時の既存ワークスペース確認、gh pr checkout実行、共有finishCheckoutヘルパーによる統一後処理(ベースブランチ永続化、クラウド登録、ロールバック処理)、拡張GitHubメタデータ(headRepositoryOwnerisCrossRepository)。

Sequence Diagram(s)

sequenceDiagram
    participant Client as Renderer (Client)
    participant HostService as Host Service
    participant Git as Git / worktree
    participant GitHub as GitHub (gh CLI)
    participant Cloud as Cloud / Database

    Client->>HostService: checkout({pr: {number, headRefName, ...}})
    activate HostService
    
    HostService->>HostService: derivePrLocalBranchName(pr)<br/>→ "owner/branch" (if cross-repo)
    
    HostService->>Cloud: Check if workspace<br/>exists for (projectId, branch)
    Cloud-->>HostService: {alreadyExists: true | false}
    
    alt Workspace already exists
        HostService-->>Client: {alreadyExists: true, ...}
    else New checkout required
        HostService->>Git: Create detached worktree
        HostService->>GitHub: gh pr checkout<br/><number><br/>--branch <derived>
        GitHub-->>Git: Check out PR branch
        HostService->>Git: Configure push.autoSetupRemote
        
        alt Push configuration fails
            HostService->>Git: Rollback worktree
            HostService-->>Client: Error
        else Success: continue
            HostService->>Cloud: ensureV2Host(...)
            Cloud-->>HostService: Host ready
            
            alt Cloud/DB operation fails
                HostService->>Git: Rollback worktree
                HostService-->>Client: Error with rollback
            else Success: finalize
                HostService->>Cloud: v2Workspace.create(...)
                Cloud-->>HostService: Workspace created
                HostService->>Cloud: Insert local<br/>workspace row +<br/>write branch.<name>.base
                Cloud-->>HostService: Row inserted
                HostService->>Cloud: Optionally start<br/>setup.sh terminal
                HostService-->>Client: {workspace, terminals, warnings}
            end
        end
    end
    deactivate HostService
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Poem

ウサギの代わりに、新しいPRのチェックアウト🐰
ローカルブランチは「owner/branch」で誇らしげに
既存なら再利用、新規なら確実にロールバック✨
worktreeは安全に、ベースはしっかり記録🌿
フォークもPRも同じ終着点へ、共に歩む道🚀

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed PR title is specific and relates to the main change: merging upstream v2 PR checkout feature, though the Japanese phrasing may reduce clarity for non-Japanese speakers.
Description check ✅ Passed Description is provided in Japanese with context about upstream integration, deferred bugs, and test checklist, but does not match the required template structure (missing Type of Change, Testing sections in template format).

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch upstream-merge/pr3-v2-pr-checkout

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9dcb44d597

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +111 to +114
case "pr-checkout": {
if (!pending.linkedPR) {
throw new Error("pr-checkout intent requires a linkedPR");
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Load pending attachments for pr-checkout flow

The new pr-checkout branch skips the attachment-loading path used by fork, so when attachmentCount > 0 the stored files are never read into loadedAttachments. As a result, PR checkout requests and the subsequent launch dispatch run without user-provided files, and clearAttachments(pendingId) later deletes them, causing silent attachment loss for PR-based workspace creation.

Useful? React with 👍 / 👎.

Comment on lines +156 to +159
const needsLaunchDispatch =
(pending.intent === "fork" || pending.intent === "pr-checkout") &&
!!result.workspace?.id;
if (needsLaunchDispatch && result.workspace?.id) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Skip launch dispatch for already-existing PR workspaces

workspaceCreation.checkout can now return alreadyExists: true for idempotent PR checkout, but this condition dispatches fork launch whenever a workspace id is present. In the existing-workspace path that re-triggers terminal/chat launch intents and reopens panes even though no workspace was created. The dispatch gate should explicitly exclude alreadyExists responses.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (10)
packages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts (1)

976-1028: 入力スキーマの discriminated union 化を将来の改善候補として検討

.refine((v) => Boolean(v.branch) !== Boolean(v.pr)) で XOR を担保していますが、z.discriminatedUnion("mode", [...]) もしくは 2 つのスキーマの z.union([...]) に書き換えると、型レベルでも input.prinput.branch が排他であることを推論できます。現在は下流で input.pr のナローイングが効かない場面が出ます(branch 側の input.branch ?? "" など)。任意改善です。

🤖 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 976 - 1028, The input schema for the checkout protectedProcedure
currently uses a single object with a refine XOR on branch vs pr which prevents
TypeScript from properly narrowing input.pr vs input.branch; replace this with a
discriminated union (e.g., two branches: one schema with a required branch
string and composer/linkedContext, and another schema with a required pr object
and composer/linkedContext) or a z.union of two explicit schemas so the runtime
validation and TypeScript inference align and callers inside checkout can
reliably narrow input.pr and input.branch; update the schema declaration where
checkout.input(...) is defined to use z.discriminatedUnion or z.union instead of
the .refine XOR on branch/pr.
apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/hooks/useSubmitWorkspace/useSubmitWorkspace.ts (1)

42-71: LGTM(軽微な可読性メモのみ)

draft.linkedPR !== null を別変数に入れた後も draft.linkedPR?.prNumber のように optional chaining が残るのは、TS が別変数経由のナローイングを伝搬させないためで、ランタイムは安全です。もし気になるなら以下のようにローカル束縛で narrow すると ?.|| のフォールバックが不要になります(任意)。

💡 任意の可読性リファクタ
-			const isPrCheckout = draft.linkedPR !== null;
-			const prPlaceholderBranch = isPrCheckout
-				? `pr-${draft.linkedPR?.prNumber}`
-				: null;
-			const prPlaceholderName = isPrCheckout
-				? draft.linkedPR?.title || `PR #${draft.linkedPR?.prNumber}`
-				: null;
+			const linkedPR = draft.linkedPR;
+			const prPlaceholderBranch = linkedPR
+				? `pr-${linkedPR.prNumber}`
+				: null;
+			const prPlaceholderName = linkedPR
+				? linkedPR.title || `PR #${linkedPR.prNumber}`
+				: null;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/hooks/useSubmitWorkspace/useSubmitWorkspace.ts`
around lines 42 - 71, The code checks draft.linkedPR !== null into isPrCheckout
but still uses optional chaining (draft.linkedPR?.prNumber / ?.title), which is
unnecessary and hurts readability; create a local narrowed binding (e.g., const
linkedPR = draft.linkedPR) after computing isPrCheckout and then reference
linkedPR.prNumber and linkedPR.title directly when building prPlaceholderBranch
and prPlaceholderName (and remove the redundant optional chaining and fallback
patterns there), keeping the same semantics used by isPrCheckout.
packages/host-service/src/trpc/router/workspace-creation/utils/exec-gh.ts (1)

17-36: 戻り値型が unknown に広がったことによる --json 呼び出し側への影響について確認

JSON パース失敗時に trimmed な文字列を返す挙動にしたため、--json で呼び出している getGitHubIssueContent / getGitHubPullRequestContent のような consumer は Zod.parse(raw) 側で「object を期待したが string が来た」という分かりにくいエラーを返すようになります(従来は JSON.parse の例外がそのまま TRPCError にラップされていたはず)。gh pr checkout 用の新規ユースケースには必要な変更ですが、--json 系の呼び出し側でデバッグしづらくなる点は把握しておく価値があります。

また、if (!trimmed) return {}; は空出力でも {} を返すため、--json 系では Zod パースで別種の誤解を生む余地があります。必要なら将来的に { kind: "json"; value: unknown } | { kind: "text"; value: string } のようなタグ付き戻り値に揃えると consumer 側の前提が明示できます(任意改善)。

🤖 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, execGh currently returns unknown and sometimes a raw
string, which breaks --json consumers like
getGitHubIssueContent/getGitHubPullRequestContent by turning JSON-parse failures
into confusing Zod errors; change execGh to return a discriminated union instead
of unknown (e.g. { kind: "json"; value: unknown } | { kind: "text"; value:
string } | { kind: "empty" }) so callers can explicitly handle JSON vs plain
text vs empty output, adjust the parsing logic in execGh to try JSON and set
kind:"json" on success, kind:"text" on parse failure (with the trimmed string),
kind:"empty" when stdout is empty, and update consumer functions
(getGitHubIssueContent/getGitHubPullRequestContent and any --json callers) to
branch on the kind and only run Zod.parse on value when kind === "json".
apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.ts (2)

142-142: composer.baseBranch に空文字列が入るリスクの確認

prContent.baseBranchgh pr viewbaseRefName 由来のため通常は非空ですが、サーバー側の PrSchema では z.string()(空文字列を許容)です。万一空文字列が来た場合、finishCheckoutbranch.<name>.base に空文字を書き込む可能性があります。prContent.baseBranch || undefined とした方が安全です。

🛡️ Proposed fix
 		composer: {
 			prompt: pending.prompt || undefined,
 			// PR's base is authoritative for the Changes tab — see plan §3.
-			baseBranch: prContent.baseBranch,
+			baseBranch: prContent.baseBranch || undefined,
 			runSetupScript: pending.runSetupScript,
 		},
🤖 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/buildIntentPayload.ts
at line 142, The code currently assigns prContent.baseBranch directly to
composer.baseBranch which can propagate an empty string into finishCheckout and
end up writing an empty value to branch.<name>.base; change the assignment in
buildIntentPayload so composer.baseBranch is set to prContent.baseBranch ||
undefined to convert empty string to undefined, ensuring finishCheckout and any
downstream logic treat missing baseRef as undefined rather than an empty string;
update any related tests or callers that expect a possible undefined value
accordingly.

116-121: state 正規化のフォールバック先について

未知の state 値はすべて "open" にマップされています。gh pr viewstate は現状 OPEN/CLOSED/MERGED のみを返すので実害はありませんが、GitHub が将来的に新しい状態(例: LOCKED)を追加した場合、静かに "open" として扱われワーニング(closed/merged 時の警告)も出なくなります。デフォルトで警告寄りに倒す(例: 未知は "closed" として扱う、もしくは extraWarnings に「未知の PR 状態」行を足す)選択肢もご検討ください。

🤖 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/buildIntentPayload.ts
around lines 116 - 121, The current normalization sets unknown prContent.state
values to "open"; change this to be conservative and surface unknown states:
check prContent.state against known values ("open","closed","merged"), and if
it's not one of them push a warning into extraWarnings (e.g. "unknown PR state:
<state>") and set normalizedState to "closed" (or otherwise conservative).
Update the code around normalizedState and ensure extraWarnings (or the function
that builds it) receives the new warning so unknown states are not silently
treated as open.
apps/desktop/plans/20260416-v2-pr-checkout-endpoint.md (2)

250-266: §2c の疑似コードが React の Rules of Hooks に反している

useFireIntent 内の switch の中で useQuery を呼び出す書き方は、条件付きフック呼び出しとなりルール違反です。実装側 (page.tsx) では正しく hostClient.workspaceCreation.getGitHubPullRequestContent.query(...) を直接呼び出す形に修正されていますが、本プランのコードサンプルはそのまま参照すると誤解を招くため、最終的な実装に合わせて更新することをお勧めします。

🤖 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 250 -
266, The snippet calls useQuery inside the switch in useFireIntent (violating
React Rules of Hooks); change the pr-checkout branch to not call useQuery but to
perform the fetch imperatively like the page.tsx implementation: call
hostServiceClient.workspaceCreation.getGitHubPullRequestContent.query({
projectId: pending.projectId, prNumber: pending.linkedPR.prNumber }) (or await
that promise) inside the "pr-checkout" case, then pass the returned prContent
into buildPrCheckoutPayload and await checkoutWorkspace; remove the conditional
useQuery invocation from useFireIntent so hooks remain top-level and stable.

104-113: zod enum の "draft" とクライアント型の不一致

プランでは state: z.enum(["open", "closed", "merged", "draft"]) とありますが、クライアント側 (useCheckoutDashboardWorkspace.ts) の pr.state"open" | "closed" | "merged" のみで "draft" を持ちません。また buildPrCheckoutPayload でも "draft""open" に正規化されます。gh pr viewstate 実値は OPEN/CLOSED/MERGED のみで、draft は別フィールド isDraft のため実害はありませんが、プラン/zod/クライアント型のいずれかを揃えると混乱が減ります。

🤖 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 zod schema for pr includes "draft" in
z.enum(["open","closed","merged","draft"]) which conflicts with the client types
(useCheckoutDashboardWorkspace.ts) and buildPrCheckoutPayload that expect
pr.state to be "open"|"closed"|"merged" and normalize draft via isDraft; remove
"draft" from the z.enum (or alternatively add draft handling to
useCheckoutDashboardWorkspace.ts and buildPrCheckoutPayload) so that pr.state in
the schema matches the client type and normalization logic (update the pr schema
declaration and any consumers referencing pr.state accordingly).
apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/page.tsx (2)

264-285: pr-checkoutintentHasProgress から漏れている点の確認

PR 説明にも記載されている既知の上流バグですが、intentHasProgressforkcheckout のみを含み、pr-checkout では進捗ステップが表示されずジェネリックなスピナーにフォールバックします(行425-432)。PR 目的どおり追跡・延期する方針でも問題ありませんが、このファイル内に TODO コメントを残して上流修正の取り込み忘れを防ぐことを検討してください。

 const intentHasProgress =
+  // TODO(upstream): include "pr-checkout" once upstream fixes the
+  // progress gating (see PR notes / V2_PR_CHECKOUT plan §2c).
   pending?.intent === "fork" || pending?.intent === "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 264 - 285, Add a short TODO comment above the intentHasProgress
definition noting that "pr-checkout" is intentionally omitted due to an upstream
bug (so progress falls back to a spinner) and reference the upstream issue/PR so
we don't forget to include it later; place the TODO next to the
intentHasProgress variable (which currently checks pending?.intent === "fork" ||
pending?.intent === "checkout") and mention that once the upstream fix is merged
we should include "pr-checkout" in that condition so useQuery will poll progress
for PR checkouts.

156-159: 二重判定の簡素化(任意)

needsLaunchDispatch 計算で !!result.workspace?.id を確認した直後、行159で再度 result.workspace?.id をチェックしています。ブール値は既に真と分かっているため冗長ですが、TypeScript のナローイングを効かせるために必要な書き方でもあります。型ガード目的であればコメントで意図を明確にするのがよいでしょう。

🤖 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 - 159, needsLaunchDispatch の定義で既に !!result.workspace?.id
を判定しているため、直後の if (needsLaunchDispatch && result.workspace?.id) の
result.workspace?.id は冗長です。修正方法はどちらかを選んでください:1) シンプル化するなら if
(needsLaunchDispatch) にして二重チェックを削除する(参照は
needsLaunchDispatch・pending.intent・result.workspace?.id の箇所を確認)、または 2)
TypeScript のナローイング目的で残すなら // keep explicit workspace id check for TS narrowing
のようなコメントを追加して意図を明確にする(どちらの場合も型エラーが出るなら非 null アサーションや適切な型ガードを使って解消する)。
apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildForkAgentLaunch.ts (1)

455-505: pr-checkout フローで resolvedPr キャッシュが silent に失敗する可能性がある

resolvedPr.url === url の厳密一致により、両者が異なるソースから提供される場合、キャッシュが機能しないままフォールバックパスが実行されます。pending.linkedPR.urlsearchPullRequests の Octokit 結果から、resolvedPr.urlgetGitHubPullRequestContent の gh CLI 出力から来ており、GitHub の URL 標準化により実際の不一致は稀ですが、この最適化の本来の目的(重複フェッチ回避)が静かに失敗する可能性があります。

より明確にするため、以下のいずれかを検討してください:

  • URL の代わりに PR 番号で照合する(resolvedPr.number === pending.linkedPR.prNumber
  • キャッシュ再利用時に明示的にログを出力して、動作が期待通りであることを確認可能にする
🤖 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/buildForkAgentLaunch.ts
around lines 455 - 505, The cache check in fetchPullRequest currently compares
resolvedPr.url === url which can silently miss matches from different sources;
change the guard to compare by PR number (e.g., resolvedPr && resolvedPr.number
=== pending.linkedPR.prNumber) to reliably reuse the cached PR, and add a
concise debug/info log inside that branch (referencing fetchPullRequest,
resolvedPr, pending.linkedPR) so cache reuse is visible during pr-checkout
flows.
🤖 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-114: The client detects deleted head forks correctly (check in
buildIntentPayload where prContent.isCrossRepository and
prContent.headRepositoryOwner are used) but the server schema in
workspace-creation.ts declares headRepositoryOwner as z.string() without
enforcing non-empty values; either strengthen the server schema to
headRepositoryOwner: z.string().min(1) for cases that require a non-empty owner,
or change the server schema to allow null for same-repo PRs and keep the client
using null rather than falling back to an empty string (adjust the client logic
in buildIntentPayload where it currently substitutes "" for headRepositoryOwner)
so both sides agree on a consistent non-empty-or-null contract.

In
`@packages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts`:
- Around line 1064-1087: The idempotency branch that returns alreadyExists: true
after finding an existing row via ctx.db.query.workspaces.findFirst can still
hit the "phantom-id" case referenced in adopt (lines around adopt handling)
where a workspace deleted in cloud remains locally; add a TODO comment in this
PR-path block (near derivePrLocalBranchName, the existing check, and
clearProgress) that references the issue number/URL and explains that the
renderer currently may re-launch deleted workspaces and that this path must be
revisited once the renderer change lands; optionally include a brief note to
surface this behavior in release notes or UI wording until the renderer fix is
deployed.
- Around line 1180-1189: The warning for non-open PRs currently uses a single
message when input.pr.state !== "open", which conflates closed vs merged PRs;
update the branching in the workspace-creation logic that pushes to
extraWarnings (the conditional that checks input.pr.state) to emit distinct
messages for each state value (e.g., if input.pr.state === "closed" push a
warning like "PR is closed — commits are included but the PR will not be
merged.", if input.pr.state === "merged" push a message like "PR is already
merged — commits are included from the merged PR.", otherwise keep a generic
fallback) so the text accurately reflects merged vs closed states while still
adding the existing preExistingLocalBranch warning for branch reset.
- Around line 326-341: finishCheckout is unconditionally writing
branch.<name>.base via the git.raw call (the args.baseBranch block) which can
record local-only names and break downstream ref reconstruction; mirror the
create flow by only writing args.baseBranch when the checkout's start point is a
remote-tracking ref. Add a guard around the git.raw([... "config",
`branch.${args.branch}.base`, args.baseBranch]) call that checks the start point
kind (e.g. args.startPoint && args.startPoint.kind === "remote-tracking") and
only performs the git config write when that condition is true, leaving the
current catch logging unchanged.

---

Nitpick comments:
In `@apps/desktop/plans/20260416-v2-pr-checkout-endpoint.md`:
- Around line 250-266: The snippet calls useQuery inside the switch in
useFireIntent (violating React Rules of Hooks); change the pr-checkout branch to
not call useQuery but to perform the fetch imperatively like the page.tsx
implementation: call
hostServiceClient.workspaceCreation.getGitHubPullRequestContent.query({
projectId: pending.projectId, prNumber: pending.linkedPR.prNumber }) (or await
that promise) inside the "pr-checkout" case, then pass the returned prContent
into buildPrCheckoutPayload and await checkoutWorkspace; remove the conditional
useQuery invocation from useFireIntent so hooks remain top-level and stable.
- Around line 104-113: The zod schema for pr includes "draft" in
z.enum(["open","closed","merged","draft"]) which conflicts with the client types
(useCheckoutDashboardWorkspace.ts) and buildPrCheckoutPayload that expect
pr.state to be "open"|"closed"|"merged" and normalize draft via isDraft; remove
"draft" from the z.enum (or alternatively add draft handling to
useCheckoutDashboardWorkspace.ts and buildPrCheckoutPayload) so that pr.state in
the schema matches the client type and normalization logic (update the pr schema
declaration and any consumers referencing pr.state accordingly).

In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/`$pendingId/buildForkAgentLaunch.ts:
- Around line 455-505: The cache check in fetchPullRequest currently compares
resolvedPr.url === url which can silently miss matches from different sources;
change the guard to compare by PR number (e.g., resolvedPr && resolvedPr.number
=== pending.linkedPR.prNumber) to reliably reuse the cached PR, and add a
concise debug/info log inside that branch (referencing fetchPullRequest,
resolvedPr, pending.linkedPR) so cache reuse is visible during pr-checkout
flows.

In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/`$pendingId/buildIntentPayload.ts:
- Line 142: The code currently assigns prContent.baseBranch directly to
composer.baseBranch which can propagate an empty string into finishCheckout and
end up writing an empty value to branch.<name>.base; change the assignment in
buildIntentPayload so composer.baseBranch is set to prContent.baseBranch ||
undefined to convert empty string to undefined, ensuring finishCheckout and any
downstream logic treat missing baseRef as undefined rather than an empty string;
update any related tests or callers that expect a possible undefined value
accordingly.
- Around line 116-121: The current normalization sets unknown prContent.state
values to "open"; change this to be conservative and surface unknown states:
check prContent.state against known values ("open","closed","merged"), and if
it's not one of them push a warning into extraWarnings (e.g. "unknown PR state:
<state>") and set normalizedState to "closed" (or otherwise conservative).
Update the code around normalizedState and ensure extraWarnings (or the function
that builds it) receives the new warning so unknown states are not silently
treated as open.

In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/`$pendingId/page.tsx:
- Around line 264-285: Add a short TODO comment above the intentHasProgress
definition noting that "pr-checkout" is intentionally omitted due to an upstream
bug (so progress falls back to a spinner) and reference the upstream issue/PR so
we don't forget to include it later; place the TODO next to the
intentHasProgress variable (which currently checks pending?.intent === "fork" ||
pending?.intent === "checkout") and mention that once the upstream fix is merged
we should include "pr-checkout" in that condition so useQuery will poll progress
for PR checkouts.
- Around line 156-159: needsLaunchDispatch の定義で既に !!result.workspace?.id
を判定しているため、直後の if (needsLaunchDispatch && result.workspace?.id) の
result.workspace?.id は冗長です。修正方法はどちらかを選んでください:1) シンプル化するなら if
(needsLaunchDispatch) にして二重チェックを削除する(参照は
needsLaunchDispatch・pending.intent・result.workspace?.id の箇所を確認)、または 2)
TypeScript のナローイング目的で残すなら // keep explicit workspace id check for TS narrowing
のようなコメントを追加して意図を明確にする(どちらの場合も型エラーが出るなら非 null アサーションや適切な型ガードを使って解消する)。

In
`@apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/hooks/useSubmitWorkspace/useSubmitWorkspace.ts`:
- Around line 42-71: The code checks draft.linkedPR !== null into isPrCheckout
but still uses optional chaining (draft.linkedPR?.prNumber / ?.title), which is
unnecessary and hurts readability; create a local narrowed binding (e.g., const
linkedPR = draft.linkedPR) after computing isPrCheckout and then reference
linkedPR.prNumber and linkedPR.title directly when building prPlaceholderBranch
and prPlaceholderName (and remove the redundant optional chaining and fallback
patterns there), keeping the same semantics used by isPrCheckout.

In `@packages/host-service/src/trpc/router/workspace-creation/utils/exec-gh.ts`:
- Around line 17-36: execGh currently returns unknown and sometimes a raw
string, which breaks --json consumers like
getGitHubIssueContent/getGitHubPullRequestContent by turning JSON-parse failures
into confusing Zod errors; change execGh to return a discriminated union instead
of unknown (e.g. { kind: "json"; value: unknown } | { kind: "text"; value:
string } | { kind: "empty" }) so callers can explicitly handle JSON vs plain
text vs empty output, adjust the parsing logic in execGh to try JSON and set
kind:"json" on success, kind:"text" on parse failure (with the trimmed string),
kind:"empty" when stdout is empty, and update consumer functions
(getGitHubIssueContent/getGitHubPullRequestContent and any --json callers) to
branch on the kind and only run Zod.parse on value when kind === "json".

In
`@packages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts`:
- Around line 976-1028: The input schema for the checkout protectedProcedure
currently uses a single object with a refine XOR on branch vs pr which prevents
TypeScript from properly narrowing input.pr vs input.branch; replace this with a
discriminated union (e.g., two branches: one schema with a required branch
string and composer/linkedContext, and another schema with a required pr object
and composer/linkedContext) or a z.union of two explicit schemas so the runtime
validation and TypeScript inference align and callers inside checkout can
reliably narrow input.pr and input.branch; update the schema declaration where
checkout.input(...) is defined to use z.discriminatedUnion or z.union instead of
the .refine XOR on branch/pr.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 656d91d5-4a14-4aa6-9f0a-4df0e22b90cc

📥 Commits

Reviewing files that changed from the base of the PR and between 38407ca and 9dcb44d.

📒 Files selected for processing (13)
  • apps/desktop/plans/20260416-v2-pr-checkout-endpoint.md
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildForkAgentLaunch.ts
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.test.ts
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.ts
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/dispatchForkLaunch.ts
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/page.tsx
  • apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/hooks/useSubmitWorkspace/useSubmitWorkspace.ts
  • apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/hooks/useCheckoutDashboardWorkspace/useCheckoutDashboardWorkspace.ts
  • apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.ts
  • packages/host-service/src/trpc/router/workspace-creation/utils/exec-gh.ts
  • packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-name.test.ts
  • packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-name.ts
  • packages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts

Comment on lines +110 to +114
if (prContent.isCrossRepository && !prContent.headRepositoryOwner) {
throw new Error(
`Cannot check out PR #${prContent.number}: the head fork repository has been deleted.`,
);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Confirm server-side handling of headRepositoryOwner in checkout input
rg -nP -C5 'headRepositoryOwner' --type=ts

Repository: MocA-Love/superset

Length of output: 50374


🏁 Script executed:

cat -n "apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.ts" | sed -n '100,145p'

Repository: MocA-Love/superset

Length of output: 183


🏁 Script executed:

find . -name "buildIntentPayload.ts" -type f

Repository: MocA-Love/superset

Length of output: 163


🏁 Script executed:

cat -n './apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.ts' | sed -n '100,145p'

Repository: MocA-Love/superset

Length of output: 2005


🏁 Script executed:

cat -n 'packages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts' | sed -n '985,1010p'

Repository: MocA-Love/superset

Length of output: 1155


🏁 Script executed:

cat -n 'packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-name.ts' | head -30

Repository: MocA-Love/superset

Length of output: 1158


🏁 Script executed:

rg -n 'headRepositoryOwner' 'packages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts' -A 3 -B 3

Repository: MocA-Love/superset

Length of output: 1363


cross-repo + null owner のエラーハンドリングは適切、ただしサーバー側の検証に脆弱性あり

isCrossRepository && !headRepositoryOwner の場合にヘッド fork の削除を早期に検知し、ユーザーに分かりやすいメッセージで失敗させる設計は適切です。同一 repo PR で headRepositoryOwner が null になったケースは行 135 で空文字列にフォールバックされます。

ただし、サーバー側の検証に問題があります。workspace-creation.ts 行 995 の headRepositoryOwner: z.string() は非空を保証しておらず、.min(1) などの制約がありません。現在はこの値を使わない同一 repo パスでは実害がありませんが、将来サーバー側で非空バリデーションが追加されると、クライアントの空文字列フォールバック(行 135)が破損します。

サーバー側で headRepositoryOwner: z.string().min(1) へ変更するか、スキーマレベルで同一 repo PR には null を許可する設計が必要です。

🤖 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/buildIntentPayload.ts
around lines 110 - 114, The client detects deleted head forks correctly (check
in buildIntentPayload where prContent.isCrossRepository and
prContent.headRepositoryOwner are used) but the server schema in
workspace-creation.ts declares headRepositoryOwner as z.string() without
enforcing non-empty values; either strengthen the server schema to
headRepositoryOwner: z.string().min(1) for cases that require a non-empty owner,
or change the server schema to allow null for same-repo PRs and keep the client
using null rather than falling back to an empty string (adjust the client logic
in buildIntentPayload where it currently substitutes "" for headRepositoryOwner)
so both sides agree on a consistent non-empty-or-null contract.

Comment on lines +326 to +341
if (args.baseBranch) {
await args.git
.raw([
"-C",
args.worktreePath,
"config",
`branch.${args.branch}.base`,
args.baseBranch,
])
.catch((err) => {
console.warn(
`[workspaceCreation.checkout] failed to record base branch ${args.baseBranch}:`,
err,
);
});
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify the fork-specific guard still exists only in `create` and not in the new finishCheckout helper.
rg -nP -C3 'branch\.\$\{.*\}\.base|`branch\.\$\{' --type=ts

Repository: MocA-Love/superset

Length of output: 10756


finishCheckoutbranch.<name>.base 書き込みに remote-tracking チェックが欠けている

create 手続き(行 853)では startPoint.kind === "remote-tracking" でガードしているのに対し、finishCheckout(行 326-341、特に行 332)は無条件に args.baseBranch を書き込んでいます。

ダウンストリーム(git.tsgetStatus/listCommits/getDiff)は保存された base 名に自動的に origin/ を前置して ref を再構築するため、local-only ブランチ名が書き込まれると origin/<local-name> という存在しない ref になり、Changes タブが壊れます。

PR 経路は常に GitHub の remote ref(pr.baseRefName)を送信するため実害はありませんが、branch 経路で local-only base が送られた場合、この interim mitigation が抜けて同じ不具合が再燃します。

create と同じ方針に揃えるため、finishCheckout でも startPoint.kind === "remote-tracking" チェック(またはこれに相当する判定)を追加し、remote-tracking のときのみ書き込むようにしてください。

🤖 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 326 - 341, finishCheckout is unconditionally writing
branch.<name>.base via the git.raw call (the args.baseBranch block) which can
record local-only names and break downstream ref reconstruction; mirror the
create flow by only writing args.baseBranch when the checkout's start point is a
remote-tracking ref. Add a guard around the git.raw([... "config",
`branch.${args.branch}.base`, args.baseBranch]) call that checks the start point
kind (e.g. args.startPoint && args.startPoint.kind === "remote-tracking") and
only performs the git config write when that condition is true, leaving the
current catch logging unchanged.

Comment on lines +1064 to +1087
// ── PR path ────────────────────────────────────────────────────────
if (input.pr) {
const branch = derivePrLocalBranchName(input.pr);

// Idempotency: existing workspace for this PR's branch →
// return it. Renderer navigates to it via `alreadyExists: true`
// instead of treating as a new create.
const existing = ctx.db.query.workspaces
.findFirst({
where: and(
eq(workspaces.projectId, input.projectId),
eq(workspaces.branch, branch),
),
})
.sync();
if (existing) {
clearProgress(input.pendingId);
return {
workspace: { id: existing.id },
terminals: [],
warnings: [],
alreadyExists: true as const,
};
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

冪等性チェックの既知の落とし穴について確認

既存の workspaces 行を検出したら即 alreadyExists: true で返す構造ですが、adopt 手続きの行 1367-1373 に書かれている phantom-id の問題(クラウド側で削除された行が local に残ってしまうケース)はここでも同様に起き得ます。PR 概要にも「(2) idempotent path (alreadyExists: true) not handled by renderer causing re-launches」として defer することが明記されていますが、レンダラ側の対応が入るまでの挙動(削除された workspace に遷移しようとする・再 launch される)をリリースノートなり UI 文言なりで閉じておくのが安全です。追従 PR のトラッキングのためにも、ここに TODO コメントで参照 issue を残しておくことをおすすめします。

🤖 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 1064 - 1087, The idempotency branch that returns alreadyExists:
true after finding an existing row via ctx.db.query.workspaces.findFirst can
still hit the "phantom-id" case referenced in adopt (lines around adopt
handling) where a workspace deleted in cloud remains locally; add a TODO comment
in this PR-path block (near derivePrLocalBranchName, the existing check, and
clearProgress) that references the issue number/URL and explains that the
renderer currently may re-launch deleted workspaces and that this path must be
revisited once the renderer change lands; optionally include a brief note to
surface this behavior in release notes or UI wording until the renderer fix is
deployed.

Comment on lines +1180 to +1189
if (input.pr.state !== "open") {
extraWarnings.push(
`PR is ${input.pr.state} — commits are included, but the PR may not merge.`,
);
}
if (preExistingLocalBranch) {
extraWarnings.push(
`Reset existing local branch "${branch}" to PR HEAD. If you had unpushed commits there, recover them via \`git reflog show ${branch}\`.`,
);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

マージ済み PR に対して警告文言がやや誤解を生む

input.pr.state !== "open" の分岐は closedmerged の両方を含みますが、文言は「commits are included, but the PR may not merge.」で統一されています。既に merge 済みの PR に対しては「merge しないかもしれない」は不正確です。最低限 state ごとに表現を分けるのが親切です。

💡 文言分岐例
-			if (input.pr.state !== "open") {
-				extraWarnings.push(
-					`PR is ${input.pr.state} — commits are included, but the PR may not merge.`,
-				);
-			}
+			if (input.pr.state === "merged") {
+				extraWarnings.push(
+					`PR #${input.pr.number} is already merged — commits are included for reference.`,
+				);
+			} else if (input.pr.state === "closed") {
+				extraWarnings.push(
+					`PR #${input.pr.number} is closed — commits are included, but the PR won't merge unless reopened.`,
+				);
+			}
📝 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
if (input.pr.state !== "open") {
extraWarnings.push(
`PR is ${input.pr.state} — commits are included, but the PR may not merge.`,
);
}
if (preExistingLocalBranch) {
extraWarnings.push(
`Reset existing local branch "${branch}" to PR HEAD. If you had unpushed commits there, recover them via \`git reflog show ${branch}\`.`,
);
}
if (input.pr.state === "merged") {
extraWarnings.push(
`PR #${input.pr.number} is already merged — commits are included for reference.`,
);
} else if (input.pr.state === "closed") {
extraWarnings.push(
`PR #${input.pr.number} is closed — commits are included, but the PR won't merge unless reopened.`,
);
}
if (preExistingLocalBranch) {
extraWarnings.push(
`Reset existing local branch "${branch}" to PR HEAD. If you had unpushed commits there, recover them via \`git reflog show ${branch}\`.`,
);
}
🤖 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 1180 - 1189, The warning for non-open PRs currently uses a single
message when input.pr.state !== "open", which conflates closed vs merged PRs;
update the branching in the workspace-creation logic that pushes to
extraWarnings (the conditional that checks input.pr.state) to emit distinct
messages for each state value (e.g., if input.pr.state === "closed" push a
warning like "PR is closed — commits are included but the PR will not be
merged.", if input.pr.state === "merged" push a message like "PR is already
merged — commits are included from the merged PR.", otherwise keep a generic
fallback) so the text accurately reflects merged vs closed states while still
adding the existing preExistingLocalBranch warning for branch reset.

@MocA-Love MocA-Love merged commit 82abdf3 into main Apr 17, 2026
14 checks passed
MocA-Love added a commit 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants