Repository navigation
Show merged PR status in thread sidebar when no open PR exists - #53
juliusmarminge wants to merge 7 commits into
Conversation
- extend `GitStatusResult` with nullable `mergedPr` metadata - in server git status, query merged PR only when no open PR is found - in sidebar, display a `Merged` status pill for threads on branches with merged PRs - update server/web/contracts tests for the new status shape
|
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:
📝 WalkthroughWalkthroughReplaces Changes
Sequence DiagramsequenceDiagram
participant Web as Sidebar (Web)
participant API as GitManager (Server)
participant GH as GitHub CLI
Web->>API: status(cwd, branch)
API->>GH: gh pr list --state all --json number,title,url,baseRefName,headRefName,state,updatedAt,mergedAt
GH-->>API: list (may be empty)
alt list non-empty
API->>API: parsePullRequestList -> normalize state/updatedAt
API->>API: sort by updatedAt -> pick latest PR
API-->>Web: GitStatusResult { pr: latestPR, ... }
else empty
API-->>Web: GitStatusResult { pr: null, ... }
end
alt pr.state == "merged"
Web->>Web: render "PR merged" pill
else pr.state == "open"
Web->>Web: render "PR open" pill and link
else
Web->>Web: no PR pill
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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 unit tests (beta)
Comment |
Show per-thread PR state in the Sidebar and return
|
Greptile SummaryThis PR adds merged PR detection to the git status flow. When no open PR exists for a branch, the server now falls back to querying
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Sidebar as Sidebar (useQueries)
participant WS as WebSocket Server
participant GM as GitManager.status()
participant GH as GitHub CLI (gh)
Sidebar->>WS: git.status({ cwd })
WS->>GM: status({ cwd })
GM->>GM: gitCore.statusDetails(cwd)
alt branch exists
GM->>GH: gh pr list --head branch --state open
alt open PR found
GH-->>GM: [{ number, title, url, ... }]
GM-->>WS: { openPr: {...}, mergedPr: null }
else no open PR
GH-->>GM: []
GM->>GH: gh pr list --head branch --state merged
alt merged PR found
GH-->>GM: [{ number, title, url, ... }]
GM-->>WS: { openPr: null, mergedPr: {...} }
else no merged PR
GH-->>GM: []
GM-->>WS: { openPr: null, mergedPr: null }
end
end
else no branch (detached HEAD)
GM-->>WS: { openPr: null, mergedPr: null }
end
WS-->>Sidebar: GitStatusResult
Sidebar->>Sidebar: Derive mergedPrByThreadId
Sidebar->>Sidebar: Show "Merged" pill if applicable
Last reviewed commit: 46347e2 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let openPr: ReturnType<typeof toStatusOpenPr> | null = null; | ||
| if (details.branch && details.hasUpstream) { | ||
| let mergedPr: ReturnType<typeof toStatusOpenPr> | null = null; | ||
| if (details.branch) { |
There was a problem hiding this comment.
hasUpstream guard removed for findOpenPr too
The old condition was if (details.branch && details.hasUpstream), which avoided calling gh pr list for branches that have never been pushed to a remote. Removing hasUpstream is necessary for the merged PR path (the remote branch may have been deleted after merge), but it also means findOpenPr now runs for local-only branches that can't possibly have an open PR. This adds an unnecessary gh CLI call (with 30s timeout) on every status check for unpushed branches.
Consider restoring the hasUpstream check for findOpenPr and only skipping it for the merged lookup:
| if (details.branch) { | |
| if (details.branch) { | |
| try { | |
| if (details.hasUpstream) { | |
| const existing = await this.findOpenPr(input.cwd, details.branch); | |
| if (existing) { | |
| openPr = toStatusOpenPr(existing); | |
| } | |
| } | |
| if (!openPr) { | |
| const merged = await this.findMergedPr(input.cwd, details.branch); | |
| if (merged) { | |
| mergedPr = toStatusOpenPr(merged); | |
| } | |
| } |
| const threadGitStatusQueries = useQueries({ | ||
| queries: threadGitTargets.map((target) => { | ||
| const base = gitStatusQueryOptions(api, target.cwd); | ||
| return { | ||
| ...base, | ||
| enabled: Boolean(base.enabled && target.branch !== null), | ||
| staleTime: 30_000, | ||
| refetchInterval: 60_000, | ||
| }; | ||
| }), | ||
| }); |
There was a problem hiding this comment.
Per-thread git status polling may cause performance issues
This fires a separate useQueries entry for every thread. Although queries with the same cwd will share a cache key, threads using worktrees each have a unique path, meaning N worktree threads = N independent status queries. Each query triggers git status --porcelain + up to 2 gh pr list calls (open + merged), polling every 60s. For users with many worktree threads, this could saturate gh API rate limits or slow down the sidebar.
Consider deduplicating by cwd before passing to useQueries, or only querying for visible/expanded threads.
| for (let index = 0; index < threadGitTargets.length; index += 1) { | ||
| const target = threadGitTargets[index]; | ||
| if (!target) continue; | ||
| const status = threadGitStatusQueries[index]?.data as GitStatusResult | undefined; |
There was a problem hiding this comment.
Unsafe as type assertion on query data
threadGitStatusQueries[index]?.data is typed as the inferred return type from the query function, but using as GitStatusResult | undefined bypasses type safety. If the query's return type ever diverges from GitStatusResult, this cast will silently hide the mismatch. Consider using a type-safe approach, e.g., assigning the result of gitStatusQueryOptions with an explicit generic or using a type guard.
- server: only query open PRs when the current branch has an upstream; still fall back to merged PR lookup - tests: cover status behavior for branches without upstream tracking - web: fetch git status once per cwd, validate query data with schema parsing, and map merged PR badges by thread branch match
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/server/src/gitManager.ts (1)
421-447: Consider extracting a shared helper to reduce duplication.
findMergedPrandfindOpenPrare nearly identical, differing only in the--stateargument. This could be consolidated into a single method with a state parameter.♻️ Optional refactor to reduce duplication
- private async findOpenPr(cwd: string, branch: string): Promise<OpenPrInfo | null> { + private async findPrByState( + cwd: string, + branch: string, + state: "open" | "merged", + ): Promise<OpenPrInfo | null> { const stdout = await this.runGhStdout(cwd, [ "pr", "list", "--head", branch, "--state", - "open", + state, "--limit", "1", "--json", "number,title,url,baseRefName,headRefName", ]); const raw = trimStdout(stdout); if (raw.length === 0) return null; let parsedJson: unknown; try { parsedJson = JSON.parse(raw); } catch { throw new Error("GitHub CLI returned invalid PR list JSON."); } const parsed = parseOpenPrList(parsedJson); return parsed[0] ?? null; } - private async findMergedPr(cwd: string, branch: string): Promise<OpenPrInfo | null> { - // ... duplicate implementation - } + + private async findOpenPr(cwd: string, branch: string): Promise<OpenPrInfo | null> { + return this.findPrByState(cwd, branch, "open"); + } + + private async findMergedPr(cwd: string, branch: string): Promise<OpenPrInfo | null> { + return this.findPrByState(cwd, branch, "merged"); + }
- Replace `openPr`/`mergedPr` with `pr` in server and contracts - Add PR `state` (`open`/`closed`/`merged`) to git status schema - Resolve latest branch PR via `gh pr list --state all` sorted by `updatedAt`
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/server/src/gitManager.ts (2)
216-240:⚠️ Potential issue | 🟠 MajorValidate
GitStatusResultwith the contract schema.
This method returns a shared contract object but skipsgitStatusResultSchema.parse, which can allow malformed payloads to escape. Please validate output for consistency with other endpoints.✅ Proposed fix
-import { - gitRunStackedActionInputSchema, - gitRunStackedActionResultSchema, - gitStatusInputSchema, - type GitStatusPrState, - type GitRunStackedActionInput, - type GitRunStackedActionResult, - type GitStatusInput, - type GitStatusResult, -} from "@t3tools/contracts"; +import { + gitRunStackedActionInputSchema, + gitRunStackedActionResultSchema, + gitStatusInputSchema, + gitStatusResultSchema, + type GitStatusPrState, + type GitRunStackedActionInput, + type GitRunStackedActionResult, + type GitStatusInput, + type GitStatusResult, +} from "@t3tools/contracts"; @@ - return { + return gitStatusResultSchema.parse({ branch: details.branch, hasWorkingTreeChanges: details.hasWorkingTreeChanges, workingTree: details.workingTree, hasUpstream: details.hasUpstream, aheadCount: details.aheadCount, behindCount: details.behindCount, pr, - }; + });As per coding guidelines, "apps/**/*.{ts,tsx}: Use Zod schemas from
packages/contractsfor shared type contracts covering provider events, WebSocket protocol, and model/session types".
437-467:⚠️ Potential issue | 🟠 MajorPrefer open PRs over merged/closed when both exist.
Sorting all states byupdatedAtcan select a merged/closed PR even when an open PR exists for the branch, which would show the wrong status pill. Please prioritize open PRs first, then fall back to the most recent non-open PR. Also guard against invalidupdatedAtvalues for deterministic ordering.🛠️ Proposed fix
- const parsed = parsePullRequestList(parsedJson).toSorted((a, b) => { - const left = a.updatedAt ? Date.parse(a.updatedAt) : 0; - const right = b.updatedAt ? Date.parse(b.updatedAt) : 0; - return right - left; - }); - return parsed[0] ?? null; + const parsed = parsePullRequestList(parsedJson); + const toTimestamp = (value: string | null): number => { + const parsedValue = value ? Date.parse(value) : 0; + return Number.isFinite(parsedValue) ? parsedValue : 0; + }; + const byUpdatedDesc = (a: PullRequestInfo, b: PullRequestInfo) => + toTimestamp(b.updatedAt) - toTimestamp(a.updatedAt); + + const open = parsed.filter((pr) => pr.state === "open").toSorted(byUpdatedDesc); + if (open[0]) return open[0]; + + const nonOpen = parsed.filter((pr) => pr.state !== "open").toSorted(byUpdatedDesc); + return nonOpen[0] ?? null;
Co-authored-by: codex <codex@users.noreply.github.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/server/src/gitManager.ts (1)
219-238:⚠️ Potential issue | 🟠 MajorOpen PR can be masked by a newer closed/merged PR.
findLatestPrselects byupdatedAtacross all states. If a closed/merged PR has a newer timestamp than an existing open PR on the same branch, status will surface the non‑open PR and the UI will disable open‑PR actions incorrectly. Prefer open PRs explicitly before falling back to the latest non‑open entry.🐛 Suggested fix
- let pr: ReturnType<typeof toStatusPr> | null = null; - if (details.branch) { - try { - const existing = await this.findLatestPr(input.cwd, details.branch); - if (existing) { - pr = toStatusPr(existing); - } - } catch { - // PR lookup is best-effort for status rendering. - } - } + let pr: ReturnType<typeof toStatusPr> | null = null; + if (details.branch) { + try { + const open = await this.findOpenPr(input.cwd, details.branch); + const existing = open ?? (await this.findLatestPr(input.cwd, details.branch)); + if (existing) { + pr = toStatusPr(existing); + } + } catch { + // PR lookup is best-effort for status rendering. + } + }Also applies to: 436-466
🧹 Nitpick comments (1)
apps/web/src/components/GitActionsControl.logic.test.ts (1)
28-86: Add coverage for non-open PR states.Now that
pr.statedrives open-PR detection, consider adding a case that asserts"merged"/"closed"behaves like “no open PR” (e.g., create‑PR actions stay enabled). This guards against future regressions in state handling.
Co-authored-by: codex <codex@users.noreply.github.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
Bugbot Autofix prepared fixes for 2 of the 2 bugs found in the latest run.
Or push these changes by commenting: Preview (8578146fb9)diff --git a/apps/server/src/gitManager.ts b/apps/server/src/gitManager.ts
--- a/apps/server/src/gitManager.ts
+++ b/apps/server/src/gitManager.ts
@@ -458,6 +458,9 @@
}
const parsed = parsePullRequestList(parsedJson).toSorted((a, b) => {
+ const aOpen = a.state === "open" ? 1 : 0;
+ const bOpen = b.state === "open" ? 1 : 0;
+ if (aOpen !== bOpen) return bOpen - aOpen;
const left = a.updatedAt ? Date.parse(a.updatedAt) : 0;
const right = b.updatedAt ? Date.parse(b.updatedAt) : 0;
return right - left;
diff --git a/apps/web/src/components/Sidebar.tsx b/apps/web/src/components/Sidebar.tsx
--- a/apps/web/src/components/Sidebar.tsx
+++ b/apps/web/src/components/Sidebar.tsx
@@ -1,10 +1,4 @@
-import {
- GitPullRequestIcon,
- MonitorIcon,
- MoonIcon,
- SunIcon,
- TerminalIcon,
-} from "lucide-react";
+import { GitPullRequestIcon, TerminalIcon } from "lucide-react";
import { useCallback, useEffect, useMemo, useState } from "react";
import type { ResolvedKeybindingsConfig } from "@t3tools/contracts";
import { useMutation, useQueries, useQuery, useQueryClient } from "@tanstack/react-query"; |
Co-authored-by: codex <codex@users.noreply.github.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is ON, but it could not run because the spend limit has been reached. To enable Bugbot Autofix, raise your spend limit in the Cursor dashboard.
| const right = b.updatedAt ? Date.parse(b.updatedAt) : 0; | ||
| return right - left; | ||
| }); | ||
| return parsed[0] ?? null; |
There was a problem hiding this comment.
findLatestPr may hide open PR behind stale merged PR
Medium Severity
findLatestPr sorts all PRs (open, closed, merged) solely by updatedAt and returns the first result. If a branch has both an open PR and a merged/closed PR that was updated more recently (e.g., via a comment on the old PR), the merged PR is returned instead of the open one. This causes the UI to show "PR merged" and offer "Create PR" even though an open PR exists, contradicting the stated intent of showing merged state only "when no open PR exists."
Additional Locations (1)
Bots answer people on other machines
1 participant
Summary
GitStatusResultwith a nullablemergedPrfield in shared contracts.gh pr list --state merged) when no open PR is found.openPr/mergedPrasnull.Mergedstatus pill when branch-matched and no open PR exists.mergedPrbehavior.Testing
apps/server/src/gitManager.test.ts: verified merged PR fallback lookup, no merged lookup when open PR exists, and failure resilience (openPr/mergedPrnull).apps/server/src/wsServer.test.ts: updated status expectations to includemergedPr: null.apps/web/src/components/GitActionsControl.logic.test.ts: updated status fixture to includemergedPr: null.packages/contracts/src/git.test.ts: updated schema parsing test to includemergedPr: null.Summary by CodeRabbit
New Features
Bug Fixes
Note
Medium Risk
Changes the shared
GitStatusResultcontract and server status flow to callgh pr list --state alland return the latest PR with a derived state; incorrect parsing/sorting orghbehavior could cause misleading PR status or extra load.Overview
Git status now returns a unified
probject (ornull) withstate(open/closed/merged), replacing the previousopenPrfield in the shared contracts and all callers.On the server,
GitManager.statusnow queries GitHub viagh pr list --state alland picks the most recently updated PR, normalizingMERGED/mergedAtintostate: "merged"while keeping best-effort behavior (failures yieldpr: null).On the web, git actions/CTAs now treat “open PR” specifically as
gitStatus.pr?.state === "open", and the thread sidebar periodically queries git status per thread/worktree to display a PR-state icon (open/closed/merged) when the thread’s branch matches the repo status.Written by Cursor Bugbot for commit 4b3825e. This will update automatically on new commits. Configure here.
Note
Display merged PR status in the thread sidebar by returning
prwithstatefromGitManager.statusand removingopenPrRename
openPrtoprwith astatefield in the Git status contract, add latest-PR lookup across all states inGitManager.status, and render a per-thread PR state icon in the sidebar. Update server and web logic/tests to usegitStatus.pr?.state === "open"for actions and UI.📍Where to Start
Start with the status flow in
makeGitManager.statusin apps/server/src/git/Layers/GitManager.ts, then reviewfindLatestPr,parsePullRequestList, andtoStatusPr, followed by the sidebar integration in apps/web/src/components/Sidebar.tsx.Macroscope summarized e8dfc42.