fix(desktop): resolve GitHub status for branch workspaces#3295
Conversation
Use getWorkspacePath() instead of requiring a worktree record so branch-based workspaces can access GitHub PR status, comments, and review thread resolution. Pass the workspace branch name explicitly to fetchGitHubPRStatus so the correct branch is looked up even when the repo is checked out to a different branch. Bail out early when a branch override's SHA cannot be resolved to prevent getPRForBranch from falling back to HEAD and matching the wrong PR. Closes superset-sh#2592
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplace worktree-only path resolution with unified Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate 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 fixes GitHub PR status, comments, and review thread resolution for branch workspaces (workspaces that share the main repo directory rather than using a git worktree). The root cause was that the prior implementation required a worktree record to resolve the repository path, leaving branch workspaces with no path and therefore no GitHub data. Key changes:
Confidence Score: 4/5Safe to merge — both prior review concerns are resolved and the fix is well-tested. Both previous reviewer concerns (prefix-based cache invalidation and error re-throw on non-override path) have been fully addressed in this revision. The core logic for branch workspace path resolution, branch-scoped cache keys, and early bail-out on unresolvable SHA is correct. Tests cover the main scenarios well. Score held at 4 rather than 5 due to the manual test plan items still being unchecked. No files require special attention; git-status.ts and github.ts are the most critical but have been carefully reviewed.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.ts | Core fix: consistently uses getWorkspacePath() for all procedures, scopes branchOverride for GitHub calls, gates DB writes on worktreeId, and adds branch-workspace stub in getWorktreeInfo. Logic is correct. |
| apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts | Introduces composite cache key for branch workspaces and SHA bail-out to prevent wrong-PR fallback. Error handling correctly re-throws unexpected errors on the non-override path. Previous thread concerns addressed. |
| apps/desktop/src/lib/trpc/routers/workspaces/utils/github/cache.ts | clearGitHubCachesForWorktree updated to use invalidatePrefix for the status resource, correctly evicting composite-key entries for branch workspaces. |
| apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.test.ts | Good coverage: tests for branch workspaces, worktree workspaces, null workspace, unresolvable path, and no-DB-write constraint. Test assertions are accurate and match the implementation logic. |
Sequence Diagram
sequenceDiagram
participant Client
participant git-status.ts as git-status router
participant worktree.ts as getWorkspacePath
participant github.ts as fetchGitHubPRStatus
participant cache.ts as githubStatusResource
Client->>git-status.ts: getGitHubStatus(workspaceId)
git-status.ts->>worktree.ts: getWorkspacePath(workspace)
alt branch workspace
worktree.ts-->>git-status.ts: mainRepoPath
git-status.ts->>github.ts: fetchGitHubPRStatus(mainRepoPath, workspace.branch)
github.ts->>cache.ts: read(mainRepoPath::branch, loader)
else worktree workspace
worktree.ts-->>git-status.ts: worktreePath
git-status.ts->>github.ts: fetchGitHubPRStatus(worktreePath, null)
github.ts->>cache.ts: read(worktreePath, loader)
end
cache.ts-->>github.ts: GitHubStatus or null
github.ts-->>git-status.ts: GitHubStatus or null
alt worktree workspace and status changed
git-status.ts->>git-status.ts: localDb.update(worktrees)
end
git-status.ts-->>Client: GitHubStatus or null
Client->>git-status.ts: resolveReviewThread(workspaceId, threadId)
git-status.ts->>git-status.ts: resolveReviewThread(worktreePath)
git-status.ts->>cache.ts: clearGitHubCachesForWorktree(repoPath)
cache.ts->>cache.ts: invalidatePrefix(repoPath)
Reviews (2): Last reviewed commit: "fix(desktop): address PR review feedback" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts">
<violation number="1" location="apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts:120">
P2: `rev-parse` error handling now swallows all failures, masking real git errors and continuing with undefined SHA instead of failing the status refresh path.</violation>
<violation number="2" location="apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts:222">
P2: Branch-scoped GitHub status cache keys were introduced, but invalidation still clears only the old worktree-only key, leaving branch entries stale.</violation>
</file>
<file name="apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.ts">
<violation number="1" location="apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.ts:235">
P2: Branch workspace PR comments can be resolved from repo HEAD instead of the workspace branch when PR target input is omitted.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.test.ts (1)
110-135: Consider adding assertion forgetWorktreenot being called.For completeness, the branch workspace test could also verify that
getWorktreeis not called whenworktreeIdis null, since this was part of the original bug (unnecessary dependency on worktree lookup).💡 Optional enhancement
test("returns GitHub status for branch workspaces (no worktreeId)", async () => { const branchWorkspace = { id: "ws-branch-1", projectId: "proj-1", worktreeId: null, type: "branch" as const, branch: "feature/branch-workspace", name: "Feature Branch", }; mockFetchGitHubPRStatus.mockClear(); + mockGetWorktree.mockClear(); mockGetWorkspace.mockReturnValue(branchWorkspace); mockGetWorkspacePath.mockReturnValue("/repos/my-project"); mockFetchGitHubPRStatus.mockResolvedValue(fakeGitHubStatus); const result = await procedures.getGitHubStatus({ input: { workspaceId: "ws-branch-1" }, }); expect(result).not.toBeNull(); expect(mockGetWorkspacePath).toHaveBeenCalledWith(branchWorkspace); expect(mockFetchGitHubPRStatus).toHaveBeenCalledWith( "/repos/my-project", "feature/branch-workspace", ); + expect(mockGetWorktree).not.toHaveBeenCalled(); });🤖 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/procedures/git-status.test.ts` around lines 110 - 135, The branch-workspace test for procedures.getGitHubStatus should assert that getWorktree is not invoked when worktreeId is null; after calling procedures.getGitHubStatus in this test (the one that sets branchWorkspace.worktreeId = null), add an expectation like expect(mockGetWorktree).not.toHaveBeenCalled() to ensure mockGetWorktree is not called, keeping the existing assertions for mockGetWorkspacePath and mockFetchGitHubPRStatus.apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts (1)
218-226: UseinvalidatePrefixfor GitHub status cache to clear branch-specific entries.The cache invalidation has an inconsistency:
pullRequestCommentsResourcecorrectly usesinvalidatePrefix()to clear all entries matching a worktree prefix, butgithubStatusResourceuses exact-keyinvalidate(). SincefetchGitHubPRStatuscreates composite cache keys like${worktreePath}::${branchName}when called with a branch override (as ingit-status.ts:194), branch-specific entries won't be cleared byclearGitHubCachesForWorktree.Update
githubStatusResource.invalidate(worktreePath)togithubStatusResource.invalidatePrefix(worktreePath)in cache.ts to match the pattern already used for PR comments and ensure stale branch-specific data doesn't persist after cache invalidation.🤖 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 218 - 226, The github status cache is being invalidated by exact key which misses composite keys created by fetchGitHubPRStatus (which uses cache keys like `${worktreePath}::${branchName}`); update the cache invalidation in the cache.ts code that calls githubStatusResource.invalidate(worktreePath) to instead call githubStatusResource.invalidatePrefix(worktreePath) so branch-specific entries are cleared; locate usages around clearGitHubCachesForWorktree or where githubStatusResource is referenced and replace invalidate(...) with invalidatePrefix(...) consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.test.ts`:
- Around line 110-135: The branch-workspace test for procedures.getGitHubStatus
should assert that getWorktree is not invoked when worktreeId is null; after
calling procedures.getGitHubStatus in this test (the one that sets
branchWorkspace.worktreeId = null), add an expectation like
expect(mockGetWorktree).not.toHaveBeenCalled() to ensure mockGetWorktree is not
called, keeping the existing assertions for mockGetWorkspacePath and
mockFetchGitHubPRStatus.
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts`:
- Around line 218-226: The github status cache is being invalidated by exact key
which misses composite keys created by fetchGitHubPRStatus (which uses cache
keys like `${worktreePath}::${branchName}`); update the cache invalidation in
the cache.ts code that calls githubStatusResource.invalidate(worktreePath) to
instead call githubStatusResource.invalidatePrefix(worktreePath) so
branch-specific entries are cleared; locate usages around
clearGitHubCachesForWorktree or where githubStatusResource is referenced and
replace invalidate(...) with invalidatePrefix(...) consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 13f6f86f-98eb-414e-98a6-c163d3df479d
📒 Files selected for processing (3)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.test.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts
- Use invalidatePrefix for GitHub status cache so branch-scoped keys (path::branch) are properly cleared on invalidation - Restore re-throw for unexpected git rev-parse errors on the non-override (worktree) path instead of swallowing all errors - Pass branch override to resolvePullRequestCommentsTarget so PR comments resolve from the workspace branch, not HEAD
|
Tip: Greploop — Automatically fix all review issues by running Use the Greptile plugin for Claude Code to query reviews, search comments, and manage custom context directly from your terminal. |
|
@saddlepaddle @AviPeltz - would appreciate a review! |
…perset-sh#3295)" This reverts commit 8291613.
upstream 取り込み PR #5: superset-sh#3295 + 19 procedure architecture rework
2 participants
Summary
getWorkspacePath()instead of requiring a worktree record so branch-based workspaces can access GitHub PR status, comments, and review thread resolutionfetchGitHubPRStatusso the correct branch is looked up even when the repo is checked out to a different branchgetPRForBranchfrom falling back to HEAD and matching the wrong PR (addresses review feedback from fix(desktop): solve #2592 — GitHub status unavailable for branch workspaces #2593)Closes #2592 — supersedes #2593
Test plan
getGitHubStatuscovering branch workspaces, worktree workspaces, null workspace, unresolvable path, and cache-write behaviorgetWorktreeInforeturning info for branch workspacesSummary by cubic
Fixes GitHub PR status, comments, and review thread resolution for branch workspaces by resolving the repo path without a worktree and explicitly targeting the workspace branch. Also improves cache invalidation to prevent stale or wrong PRs.
getWorkspacePathfor both workspace types; handle missing paths safely.rev-parseerrors on the non-override path.getWorktreeInfo.Written for commit b2cf68c. Summary will update on new commits.
Summary by CodeRabbit
Tests
Bug Fixes
Improvements