upstream 取り込み PR #5: #3295 + 19 procedure architecture rework#402
Merged
upstream 取り込み PR #5: #3295 + 19 procedure architecture rework#402
Conversation
PR #5 Phase A: 空の github-extended router を新規作成し workspaces router にネストして登録する。後続 Phase で 19 tRPC procedure を git-status.ts から段階的に移設し、upstream superset-sh#3295 の軽量化版 git-status/github/cache を採用する準備。
PR #5 Phase B-C: fork の 19 tRPC procedure (`cleanupMissingWorktrees`, `createGitHubIssue`, `dispatchGitHubWorkflow`, `getCheckJobSteps`, `getGitHubRepositoryOverview`, `getGitHubWorkflowRuns`, `getJobLogs`, `getJobStatuses`, `getMissingWorktrees`, `getPullRequestIdentityCandidates`, `getWorkflowRunJobs`, `replyToPullRequestComment`, `rerunPullRequestChecks`, `setActiveSyncWorkspace`, `setPullRequestDraftState`, `setPullRequestThreadResolution`, `updatePullRequestAssignees`, `updatePullRequestReviewers`, `uploadGitHubIssueAsset`) と専用 helper を procedures/git-status.ts から github-extended.ts へ移動。 - git-status.ts: 2073 → 383 行 (upstream superset-sh#3295 に近い形へ) - github-extended.ts: 新規 1734 行 (fork 19 procedure + 専用 helper + schema) - main/server 側 typecheck: グリーン - renderer 側 typecheck: 57 件エラー (Phase D: renderer 書き換えで解消) `githubSyncService.initialize` のモジュールトップ呼び出しは git-status.ts に残置、Phase E で整理する。
…hubExtended.* renderer 側の tRPC 呼び出しパスを `workspaces.<procedure>` から `workspaces.githubExtended.<procedure>` に書き換え、Phase B-C で github-extended nested router に移動した 19 procedure との型整合を取る。
…ons for git-status/github/cache
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 50 minutes and 29 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 Walkthroughウォークスルー新しいGitHub拡張ルーター（ 変更内容
推定コードレビュー工数🎯 4 (複雑) | ⏱️ ~60分 関連の可能性のあるPR
ウサギの詩
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/desktop/src/lib/trpc/routers/workspaces/utils/github/cache.ts (1)
351-358:⚠️ Potential issue | 🟡 Minor
invalidatePrefix(worktreePath)は別ワークツリーのキャッシュを巻き込む可能性があります。ステータスのキャッシュキーは
worktreePath単独または${worktreePath}::${branchName}の 2 形態で、invalidatePrefixはstartsWith一致です。パスが別のワークツリーの接頭辞になるケース（例:/x/y/projと/x/y/proj-v2）では誤爆が起きえます。確実に分離するなら exact invalidate と${worktreePath}::プレフィックスの 2 段で行うのが安全です。🛡️ 提案差分
- githubStatusResource.invalidatePrefix(worktreePath); + githubStatusResource.invalidate(worktreePath); + githubStatusResource.invalidatePrefix(`${worktreePath}::`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/github/cache.ts` around lines 351 - 358, The current clearGitHubCachesForWorktree uses githubStatusResource.invalidatePrefix(worktreePath) which can accidentally match other worktrees whose paths start with the same prefix; change this to perform an exact invalidate for the bare worktree key and a prefix invalidate only for branch-scoped keys: call githubStatusResource.invalidate(worktreePath) and then githubStatusResource.invalidatePrefix(`${worktreePath}::`) (keep repoContextResource.invalidate(worktreePath) and recordGitHubCacheMetric as-is) so keys like "/x/y/proj" won’t invalidate "/x/y/proj-v2".apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts (1)
276-308:⚠️ Potential issue | 🟠 Major
branchNameパラメータがキャッシュキーのみに使用され、refresh 時に HEAD に依存したままです。
fetchGitHubPRStatusの JSDoc（行 270-275）では「branchNameが提供された場合、HEAD / チェックアウト済みブランチではなく そのブランチの SHA とアップストリームを解決する」と明記されていますが、実装では次のようになっています：
- キャッシュキー（行 280）:
branchNameがあれば${worktreePath}::${branchName}で正しく分離- refresh 関数（行 210 の
refreshGitHubPRStatus）:branchNameパラメータを受け取らない- resolveGitHubStatusContext（行 167）:
getCurrentBranch(worktreePath)で worktreePath の HEAD を使用結果として、branch 型ワークスペース（git-status.ts:220 の
fetchGitHubPRStatus(repoPath, branchOverride)呼び出し）では、キャッシュミス / TTL 失効時に：
workspace.branch = "feature-x"用のキャッシュキーが要求される- HEAD が
mainにチェックアウトされている場合、resolveGitHubStatusContextはmainのブランチ情報で PR を解決mainのブランチ情報（reviewer/checks/draft state 等）がfeature-xのキャッシュキーに書き込まれるJSDoc の約束と実装の乖離です。
refreshGitHubPRStatus(worktreePath, branchName)の形で branchName を受け取り、resolveGitHubStatusContext内でbranchName ?? getCurrentBranch(...)にフォールバックさせるか、または branch 型ワークスペースではキャッシュミス時に null を返すべきです。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts` around lines 276 - 308, The implementation only uses branchName for the cache key but not for resolving status, causing HEAD-based refreshes to overwrite branch-specific caches; update fetchGitHubPRStatus to call refreshGitHubPRStatus(worktreePath, branchName) and modify resolveGitHubStatusContext to accept an optional branchName and use branchName ?? getCurrentBranch(worktreePath) when resolving SHA/upstream; ensure readCachedGitHubStatus/read/write paths continue to use the existing cacheKey logic so branch-specific keys map to refreshes that use the provided branchName.
🧹 Nitpick comments (1)
apps/desktop/src/lib/trpc/routers/workspaces/github-extended.ts (1)
942-981:Map<string, string>を Set として使っています。Set<string>で十分です。キーと値が常に同一なので実質
Setであり、加えてtarget.split(":")で NWO とランIDを復元するのは冗長です。組を直接保持するSet<string>+ 正規化済みキーで簡素化できます（機能面での修正提案ではなく可読性向上）。♻️ 提案差分
- const runTargets = new Map<string, string>(); + const runTargets = new Set<string>(); for (const check of checksToRerun) { const runId = parseRunIdFromActionsUrl(check.url); const repositoryNameWithOwner = check.url ? extractNwoFromUrl(check.url) : null; if (!runId || !repositoryNameWithOwner) { continue; } - - runTargets.set( - `${repositoryNameWithOwner}:${runId}`, - `${repositoryNameWithOwner}:${runId}`, - ); + runTargets.add(`${repositoryNameWithOwner}:${runId}`); } if (runTargets.size === 0) { /* ... */ } - for (const target of runTargets.values()) { + for (const target of runTargets) { const [repositoryNameWithOwner, runId] = target.split(":");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/workspaces/github-extended.ts` around lines 942 - 981, The code uses runTargets as a Map but only stores identical key/value pairs; replace it with a Set to simplify intent: change runTargets declaration from new Map<string,string>() to new Set<string>(), change runTargets.set(`${repositoryNameWithOwner}:${runId}`, ...) to runTargets.add(`${repositoryNameWithOwner}:${runId}`) in the loop that calls parseRunIdFromActionsUrl and extractNwoFromUrl, and update the iteration to for (const target of runTargets) (remove .values()) before calling execWithShellEnv; keep the same split logic (target.split(":")) to obtain repositoryNameWithOwner and runId, and retain usage of mode and repoPath when constructing the gh api call.
🤖 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/lib/trpc/routers/workspaces/github-extended.ts`:
- Around line 1569-1606: The loop that processes each missing worktree can leave
a workspace stuck in deleting state if an exception occurs after
markWorkspaceAsDeleting; wrap per-workspace teardown (calls to
getWorkspaceRuntimeRegistry().terminal.killByWorkspaceId,
githubSyncService.unregisterWorkspace, workspaceInitManager.clearJob,
deleteWorkspace, etc.) in a try/finally where
clearWorkspaceDeletingStatus(ws.id) is called from the finally block to
guarantee cleanup, and catch/log errors around non-critical calls
(unregisterWorkspace, clearJob, deleteWorkspace) so one failing workspace
doesn't abort the whole loop; also move the
hideProjectIfNoWorkspaces(input.projectId) call out of the for (const wt of
missing) loop and invoke it once after the loop completes.
In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.ts`:
- Around line 146-165: The refreshGitStatus call uses getAheadBehindCount({
repoPath, defaultBranch }) which passes project.defaultBranch, causing
ahead/behind to be computed against the repo default while gitStatus.branch is
set to workspace.branch; change the call in refreshGitStatus to pass
defaultBranch: workspace.branch (i.e., getAheadBehindCount({ repoPath,
defaultBranch: workspace.branch })) so the ahead/behind counts match the branch
label returned and keep the localDb.update(worktrees).set({ gitStatus })
behavior unchanged.
---
Outside diff comments:
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/github/cache.ts`:
- Around line 351-358: The current clearGitHubCachesForWorktree uses
githubStatusResource.invalidatePrefix(worktreePath) which can accidentally match
other worktrees whose paths start with the same prefix; change this to perform
an exact invalidate for the bare worktree key and a prefix invalidate only for
branch-scoped keys: call githubStatusResource.invalidate(worktreePath) and then
githubStatusResource.invalidatePrefix(`${worktreePath}::`) (keep
repoContextResource.invalidate(worktreePath) and recordGitHubCacheMetric as-is)
so keys like "/x/y/proj" won’t invalidate "/x/y/proj-v2".
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts`:
- Around line 276-308: The implementation only uses branchName for the cache key
but not for resolving status, causing HEAD-based refreshes to overwrite
branch-specific caches; update fetchGitHubPRStatus to call
refreshGitHubPRStatus(worktreePath, branchName) and modify
resolveGitHubStatusContext to accept an optional branchName and use branchName
?? getCurrentBranch(worktreePath) when resolving SHA/upstream; ensure
readCachedGitHubStatus/read/write paths continue to use the existing cacheKey
logic so branch-specific keys map to refreshes that use the provided branchName.
---
Nitpick comments:
In `@apps/desktop/src/lib/trpc/routers/workspaces/github-extended.ts`:
- Around line 942-981: The code uses runTargets as a Map but only stores
identical key/value pairs; replace it with a Set to simplify intent: change
runTargets declaration from new Map<string,string>() to new Set<string>(),
change runTargets.set(`${repositoryNameWithOwner}:${runId}`, ...) to
runTargets.add(`${repositoryNameWithOwner}:${runId}`) in the loop that calls
parseRunIdFromActionsUrl and extractNwoFromUrl, and update the iteration to for
(const target of runTargets) (remove .values()) before calling execWithShellEnv;
keep the same split logic (target.split(":")) to obtain repositoryNameWithOwner
and runId, and retain usage of mode and repoPath when constructing the gh api
call.
🪄 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: 95c8466f-e935-4ac7-b597-70a1577333d8
📒 Files selected for processing (12)
apps/desktop/src/lib/trpc/routers/workspaces/github-extended.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/github/cache.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.tsapps/desktop/src/renderer/react-query/workspaces/useCleanupMissingWorktrees.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/KeepAliveWorkspaces.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/project/$projectId/components/ProjectWorktreeAutoSync/ProjectWorktreeAutoSync.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ActionLogsPane/ActionLogsPane.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/RepositoryPanel/RepositoryPanel.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/ReviewPanel/ReviewPanel.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/ReviewPanel/components/CheckSteps/CheckSteps.tsx
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24d9bb9e07
ℹ️ 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".
Codex final review で指摘された JSDoc と実装の不整合を解消。 branchName 引数は **cache key スコープのみ** に使われ、内部の refreshGitHubPRStatus 呼び出しには伝播しないことを明記。 完全な branch override は fork 独自の resolveGitHubStatusContext / resolveAttachedPullRequest の rework が必要なため別 PR で対応する。
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
12 tasks
8 tasks
4 tasks
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
upstream 取り込み PR #5: Architecture rework (最大規模 PR)。upstream superset-sh#3295 の軽量化改善を取り込みつつ、fork の 19 GitHub Repository Tools tRPC procedure を専用 nested router (
workspaces.githubExtended) に分離。当初 PR #388 で superset-sh#3295 を revert した状態を、「案 A (分離型)」で根本解決。
取り込み内容 (4 commits)
f66bfcfb6feat(trpc): add empty github-extended router and mount under workspaces (Phase A)github-extended.tsrouter を新規作成workspaces.tsにrouter({ githubExtended: createGithubExtendedRouter() })として mount59d07a0e6feat(trpc): move 19 GitHub extended procedures to github-extended.ts (Phase B-C)git-status.tsからgithub-extended.tsへ移動git-status.ts: 2073 → 383 行 (83% 縮小、upstream 軽量化版に近い形)github-extended.ts: 新規 1734 行 (fork 19 procedure + 専用 helper + schema)c635bc61ffix(renderer): rewire 19 GitHub extended procedures to workspaces.githubExtended.* (Phase D)RepositoryPanel,ReviewPanel,ActionLogsPane,ProjectWorktreeAutoSync,useCleanupMissingWorktrees,CheckSteps,KeepAliveWorkspaces24d9bb9e0feat(trpc): adopt upstream fix(desktop): resolve GitHub status for branch workspaces superset-sh/superset#3295 branch-workspace corrections for git-status/github/cache (Phase E)getWorkspacePath(workspace)による branch workspace 対応: worktree なしの branch 型 workspace でも正しく repo path 解決fetchGitHubPRStatus(worktreePath, branchName?)に optionalbranchName引数追加、cache key を${path}::${branchName}形式に変更し branch 別 cache 保持fetchGitHubPRComments({ branchName? })対応clearGitHubCachesForWorktreeをinvalidatePrefix化して branch-scoped cache を一括無効化19 tRPC procedure 分離対象 (全て
workspaces.githubExtended.*へ移行)cleanupMissingWorktrees,createGitHubIssue,dispatchGitHubWorkflow,getCheckJobSteps,getGitHubRepositoryOverview,getGitHubWorkflowRuns,getJobLogs,getJobStatuses,getMissingWorktrees,getPullRequestIdentityCandidates,getWorkflowRunJobs,replyToPullRequestComment,rerunPullRequestChecks,setActiveSyncWorkspace,setPullRequestDraftState,setPullRequestThreadResolution,updatePullRequestAssignees,updatePullRequestReviewers,uploadGitHubIssueAsset保持した fork 独自機能
previewUrlResource,commitAuthorResource,noPullRequestMatchResource(cache.ts)fetchGitHubPreviewUrl,fetchCheckJobSteps,fetchStructuredJobLogs,fetchJobStatuses(github.ts)getGitHubStatusinput のincludePreview?: boolean(git-status.ts)getWorktreeInfo,getWorktreesByProject,getExternalWorktreesprocedure取り込みを見送った点
refreshGitHubPRStatus内部の branch override ロジック (refs/heads/${branchName}rev-parse / upstream ref 解決): fork はresolveGitHubStatusContext/resolveAttachedPullRequestの独自 PR attachment チェック実装を使っており upstream と内部実装が大きく異なるため内部 refresh ロジックへの branch override 伝播は見送り。cache key レベルの分離のみ適用。Fork 固有機能ヘルスチェック
作業前 baseline (
/tmp/pr5-baseline/fork-features.txt) と作業後を grep で照合。全 14 項目変化なし (silent regression ゼロ):TERMINAL_OPTIONS,SUPERSET_WORKSPACE_NAME,moonshot-ai.kimi-codelistBranchessortOrder/pinDefaultPR #388 からの継続性
git-status.tsを更に軽量化しても fork extended は独立 namespace で影響を受けないTest plan
bun install正常完了bun run typecheckグリーン (27/27 task)bun run lintグリーン次の PR
Summary by CodeRabbit
リリースノート
新機能
バグ修正