Optimize PRs tab loading#289
Merged
Merged
Conversation
Instruments ADE for an iterative perf-optimization loop driven by an agentskills.io SKILL. Adds main-process metrics sampler (app metrics + memory), renderer-side perfMark/perfMeasure + PerformanceObserver web vitals, a tap into the existing IPC trace wrapper, and a JSONL + summary aggregator that produces a single fitness score per scenario run. Includes per-tab scenario harness (lanes + boot), launch shortcuts (perf-launch.mjs / run-perf-scenario.mjs with --no-project and --tab), and the ade-autoresearch / ade-perf-lanes / ade-perf-boot skills under .agents/skills/. Smoke-verified end-to-end on the lanes tab — summary.json populated with real signal across all six fitness components. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(app): honor hash routes in browser router * perf(lanes): skip disabled local runtime bridge real lanes baseline failed at lanes.idle-at-rest with V8 OOM; optimized scenarios all passed, fitness 7028.82 * perf(lanes): suppress duplicate git actions fullscreen pane UI audit: Git Actions expand went from 2 toolbars to 1. Post-marker IPC changed from 30 calls / 223ms to 29 calls / 192ms, removing one duplicate ade.git.getSyncStatus. * perf(lanes): fast-path disabled sync runtime UI audit: sync IPC in local-runtime-disabled perf mode went from 4 failed calls / 1145ms to 3 successful calls / 2ms. * perf(lanes): scope lane refreshes during UI actions * ship: prepare lane for review * ship: iteration 1 - address snapshot refresh review * ship: iteration 2 - address refresh review * ship: iteration 3 - refresh git action overlays
Squash-merge Work tab optimization lane into ade/autoresearch-d23e8745 after automate/finalize and review fixes.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds PR performance guidance and implements batched/parallel PR operations and caching: shared PrSnapshotHydration type, bulk merge-contexts, includeExternalClosed snapshot mode, bounded concurrent PR refresh, batched conflict analysis, queue-target fetch TTL, stash apply/drop refactor, non-cancellable batched lane deletion, IPC/preload/browser wiring, and tests. ChangesPR Service & Lane Deletion Enhancements
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Owner
Author
|
@copilot review but do not make fixes |
Owner
Author
|
@cursor review |
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
Contributor
Completed a review-only pass at bea68db (no fixes made). One follow-up I’d flag: |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
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/main/services/prs/prService.ts (2)
5068-5115:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't reuse an open-only in-flight snapshot for
includeExternalClosedcallers.The cache validity check handles
includeExternalClosed, but the in-flight fast path does not. If an open-only fetch is already running,getGithubSnapshot({ includeExternalClosed: true })will await that narrower request and return a snapshot missing closed external PRs.Suggested fix
let cachedGithubSnapshot: GitHubPrSnapshot | null = null; let cachedGithubSnapshotAt = 0; let cachedGithubSnapshotIncludesExternalClosed = false; let githubSnapshotInFlight: Promise<GitHubPrSnapshot> | null = null; +let githubSnapshotInFlightIncludesExternalClosed = false; @@ const startSnapshotRequest = (allowStaleOnError: boolean): Promise<GitHubPrSnapshot> => { const staleFallback = cachedGithubSnapshot; const request = getGithubSnapshotUncached({ includeExternalClosed }) .then((snapshot) => { cachedGithubSnapshot = snapshot; cachedGithubSnapshotAt = Date.now(); cachedGithubSnapshotIncludesExternalClosed = includeExternalClosed; return snapshot; }) @@ .finally(() => { if (githubSnapshotInFlight === request) { githubSnapshotInFlight = null; + githubSnapshotInFlightIncludesExternalClosed = false; } }); + githubSnapshotInFlightIncludesExternalClosed = includeExternalClosed; githubSnapshotInFlight = request; return request; }; @@ - if (!force && githubSnapshotInFlight) { + if ( + !force + && githubSnapshotInFlight + && (!includeExternalClosed || githubSnapshotInFlightIncludesExternalClosed) + ) { return githubSnapshotInFlight; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/prs/prService.ts` around lines 5068 - 5115, getGithubSnapshot currently reuses githubSnapshotInFlight for any caller, so a request with includeExternalClosed: true can await an in-flight open-only fetch and miss closed externals; change the in-flight tracking to include the request's includeExternalClosed flag and only reuse it when the in-flight request's includesExternalClosed matches the current includeExternalClosed (e.g. replace githubSnapshotInFlight: Promise<...> with an object like { promise, includesExternalClosed } or add a githubSnapshotInFlightIncludesExternalClosed boolean, set it inside startSnapshotRequest, and update the early-return that checks "if (!force && githubSnapshotInFlight)" to also require matching includeExternalClosed before returning the in-flight promise).
3979-3983:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve target-lane lookups against branch names, not raw refs.
row.base_branchis a plain branch name, but these lookups compare it tolane.branchRefdirectly. For normal refs likerefs/heads/main,targetLaneIdfalls back tonull, so bothgetMergeContextandgetMergeContextscan lose the actual target lane for lane-backed PRs.Suggested fix
- const byBranch = lanes.find((lane) => normalizeBranchName(lane.branchRef) === normalized); + const byBranch = lanes.find((lane) => normalizeBranchName(branchNameFromRef(lane.branchRef)) === normalized);- lanes - .map((lane) => [normalizeBranchName(lane.branchRef), lane.id] as const) + lanes + .map((lane) => [normalizeBranchName(branchNameFromRef(lane.branchRef)), lane.id] as const) .filter(([branch]) => Boolean(branch)),Also applies to: 4084-4087
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/prs/prService.ts` around lines 3979 - 3983, The lookup in findLaneIdByBranch incorrectly compares raw branch names to lane.branchRef; update the function to compare normalized branch names on both sides by applying normalizeBranchName to lane.branchRef before matching (i.e., compute normalized = normalizeBranchName(rawBranch) and compare to normalizeBranchName(lane.branchRef)), so findLaneIdByBranch returns the correct lane id for plain branch names used by row.base_branch; also apply the same normalization fix to the analogous lookup used around lines handling targetLaneId in getMergeContext/getMergeContexts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.agents/skills/ade-perf-prs/SKILL.md:
- Line 83: The heading contains a duplicated word ("Keep GitHub first open
open-only and local-first"); update that heading (the string "Keep GitHub first
open open-only and local-first") to remove the extra "open" — e.g., "Keep GitHub
first: open-only and local-first" or "Keep GitHub first open-only and
local-first" — so the guidance reads correctly.
In `@apps/desktop/src/renderer/components/lanes/LanesPage.tsx`:
- Around line 1392-1415: hasBlockedSelectedDescendant currently stops walking
ancestry when a parent is not in deleteLaneById (which is built from
actionable), so a selected grandparent won't see a blocked grandchild if the
intermediate parent wasn't selected; change the ancestry walk to follow parents
through the full lane graph rather than only actionable: replace lookups against
deleteLaneById inside hasBlockedSelectedDescendant with lookups against the
complete lanes map/source used in this component (or fall back to that map when
deleteLaneById.get(...) returns undefined) so the while loop continues up the
real parent chain and correctly detects blocked selected descendants; keep
existing uses of blockedLaneIds, actionable, and planLaneDeleteBatches
unchanged.
In `@apps/desktop/src/renderer/components/prs/state/PrsContext.tsx`:
- Around line 916-940: The snapshot hydration logic only updates detail state
when arrays are non-empty, leaving prior data visible for a PR with empty
snapshot fields; update the block that handles snapshot to always call the
detail setters so empty primary fields are cleared: call
setDetailStatus(snapshot.status ?? undefined) (or clear it when falsy), call
setDetailChecks(snapshot.checks.length ? snapshot.checks : []), call
setDetailReviews(snapshot.reviews.length ? snapshot.reviews : []), and call
setDetailComments(snapshot.comments.length ? snapshot.comments : []); treat
these clearing assignments as hydration (set hydrated = true) so
setDetailBusy(false) runs immediately when the snapshot is applied.
In `@apps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsx`:
- Around line 466-475: The component currently only sets externalHistoryLoaded
true when includeExternalClosed is requested, but never resets it to false when
a snapshot without external items replaces snapshot; update the promise handler
that calls setSnapshot(next) (the getGitHubSnapshot(...) .then handler) to set
externalHistoryLoaded to the actual includeExternalClosed value for that
fetch—i.e., call setExternalHistoryLoaded(includeExternalClosed === true) (or
derive from next if the snapshot object exposes that metadata) immediately after
setSnapshot(next) so externalHistoryLoaded stays consistent with the current
snapshot.
---
Outside diff comments:
In `@apps/desktop/src/main/services/prs/prService.ts`:
- Around line 5068-5115: getGithubSnapshot currently reuses
githubSnapshotInFlight for any caller, so a request with includeExternalClosed:
true can await an in-flight open-only fetch and miss closed externals; change
the in-flight tracking to include the request's includeExternalClosed flag and
only reuse it when the in-flight request's includesExternalClosed matches the
current includeExternalClosed (e.g. replace githubSnapshotInFlight: Promise<...>
with an object like { promise, includesExternalClosed } or add a
githubSnapshotInFlightIncludesExternalClosed boolean, set it inside
startSnapshotRequest, and update the early-return that checks "if (!force &&
githubSnapshotInFlight)" to also require matching includeExternalClosed before
returning the in-flight promise).
- Around line 3979-3983: The lookup in findLaneIdByBranch incorrectly compares
raw branch names to lane.branchRef; update the function to compare normalized
branch names on both sides by applying normalizeBranchName to lane.branchRef
before matching (i.e., compute normalized = normalizeBranchName(rawBranch) and
compare to normalizeBranchName(lane.branchRef)), so findLaneIdByBranch returns
the correct lane id for plain branch names used by row.base_branch; also apply
the same normalization fix to the analogous lookup used around lines handling
targetLaneId in getMergeContext/getMergeContexts.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d1b2c597-ceec-4fac-a574-b677272742ba
⛔ Files ignored due to path filters (4)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/lanes/README.mdis excluded by!docs/**docs/features/pull-requests/README.mdis excluded by!docs/**docs/perf/prs-tab-action-inventory.mdis excluded by!docs/**
📒 Files selected for processing (28)
.agents/skills/ade-perf-prs/SKILL.mdapps/desktop/src/main/services/adeActions/registry.tsapps/desktop/src/main/services/git/gitOperationsService.test.tsapps/desktop/src/main/services/git/gitOperationsService.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/lanes/autoRebaseService.tsapps/desktop/src/main/services/lanes/laneService.test.tsapps/desktop/src/main/services/lanes/laneService.tsapps/desktop/src/main/services/prs/prService.test.tsapps/desktop/src/main/services/prs/prService.tsapps/desktop/src/main/services/shared/queueRebase.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.test.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/automations/adeActionSchemas.tsapps/desktop/src/renderer/components/lanes/LanesPage.test.tsapps/desktop/src/renderer/components/lanes/LanesPage.tsxapps/desktop/src/renderer/components/lanes/ManageLaneDialog.tsxapps/desktop/src/renderer/components/lanes/lanePageModel.tsapps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsxapps/desktop/src/renderer/components/prs/state/PrsContext.test.tsxapps/desktop/src/renderer/components/prs/state/PrsContext.tsxapps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsxapps/desktop/src/renderer/components/terminals/SessionContextMenu.tsxapps/desktop/src/renderer/components/terminals/WorkStartSurface.test.tsxapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/types/prs.ts
💤 Files with no reviewable changes (1)
- apps/desktop/src/renderer/components/lanes/ManageLaneDialog.tsx
Owner
Author
|
@copilot review but do not make fixes |
Owner
Author
|
@cursor review |
Copilot stopped work on behalf of
arul28 due to an error
May 12, 2026 17:01
Owner
Author
|
@copilot review but do not make fixes |
Owner
Author
|
@cursor review |
Copilot stopped work on behalf of
arul28 due to an error
May 12, 2026 17:39
Copilot stopped work on behalf of
arul28 due to an error
May 12, 2026 21:40
Owner
Author
|
@copilot review but do not make fixes |
Owner
Author
|
@cursor review |
Copilot stopped work on behalf of
arul28 due to an error
May 12, 2026 21:54
Owner
Author
|
@copilot review but do not make fixes |
Owner
Author
|
@cursor review |
Copilot stopped work on behalf of
arul28 due to an error
May 12, 2026 22:07
Owner
Author
|
@cursor review |
Owner
Author
|
@copilot review but do not make fixes |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
apps/desktop/src/main/services/shared/queueRebase.test.ts (1)
70-91: ⚡ Quick winConsider adding a test for cache hits within the TTL window.
The existing test verifies that branches are refetched after the TTL expires, but there's no complementary test confirming that fetches are correctly skipped within the TTL. Adding a test case where the same branch is requested twice with time < TTL (e.g.,
initialNow + 1000) and asserting thatrunGitOrThrowis called only once would strengthen confidence that the cache gating works in both directions.✅ Example test case for cache hit within TTL
+ it("skips refetch for queue target branches within the TTL", async () => { + const initialNow = 3_000_000; + vi.mocked(Date.now).mockReturnValue(initialNow); + + await fetchQueueTargetTrackingBranches({ + db: makeDb(["queue-cached"]), + projectId: "project-cached", + projectRoot: "/tmp/ade-cached", + }); + expect(mockGit.runGitOrThrow).toHaveBeenCalledTimes(1); + + mockGit.runGitOrThrow.mockClear(); + vi.mocked(Date.now).mockReturnValue(initialNow + 1000); // Still within TTL + + await fetchQueueTargetTrackingBranches({ + db: makeDb(["queue-cached"]), + projectId: "project-cached", + projectRoot: "/tmp/ade-cached", + }); + + expect(mockGit.runGitOrThrow).not.toHaveBeenCalled(); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/shared/queueRebase.test.ts` around lines 70 - 91, Add a complementary test that verifies cache hits inside the TTL by calling fetchQueueTargetTrackingBranches twice with Date.now mocked to initialNow and then initialNow + 1000 (or any value < TEST_QUEUE_TARGET_FETCH_TTL_MS), using the same db/projectId/projectRoot setup, and asserting mockGit.runGitOrThrow was called only once; use the same helpers (fetchQueueTargetTrackingBranches, makeDb) and TEST_QUEUE_TARGET_FETCH_TTL_MS constant, clear mocks appropriately between calls and avoid advancing time past the TTL so the second call hits the cache.apps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsx (1)
507-510: 💤 Low valueConsider extracting snapshot state clearing to a separate effect for clarity.
The snapshot state clearing (lines 507-510) is placed within an effect primarily concerned with detail tab initialization. While functionally correct, this mixing of concerns makes the code harder to follow.
The clearing logic should run whenever
pr.idchanges, but the effect dependencies include bothinitialDetailTabandpr.id. A dedicated effect would be clearer:React.useEffect(() => { setSnapshotStatus(null); setSnapshotChecks([]); setSnapshotReviews([]); setSnapshotComments([]); }, [pr.id]);However, since the current implementation works correctly (snapshot clearing happens before hydration at line 849), this is optional.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsx` around lines 507 - 510, Extract the snapshot-clearing logic out of the effect currently handling detail-tab initialization and put it into its own React.useEffect that runs when pr.id changes; specifically remove the setSnapshotStatus, setSnapshotChecks, setSnapshotReviews, and setSnapshotComments calls from the existing initialization effect and create a new effect that invokes those four setters with null/empty-array values and lists [pr.id] as its dependency so snapshot state is reset whenever the PR id changes.apps/desktop/src/renderer/components/prs/detail/PrDetailPane.issueResolver.test.tsx (1)
464-464: ⚡ Quick winProvide a default mock for
getCommitsto prevent undefined errors.Unlike
getDetailandgetFileswhich have default mocks (lines 452-463),getCommitsis passed through as-is. If the component callswindow.ade.prs.getCommits()and the test doesn't provide a mock, it will fail with "undefined is not a function".🛡️ Add default mock
- getCommits: args.getCommits, + getCommits: args.getCommits ?? vi.fn().mockResolvedValue([]),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/components/prs/detail/PrDetailPane.issueResolver.test.tsx` at line 464, Tests are failing when window.ade.prs.getCommits is not provided because getCommits is passed through as-is; add a default mock (e.g., jest.fn()) for getCommits the same way getDetail and getFiles have defaults so calling window.ade.prs.getCommits() won't be undefined. Update the object that sets getCommits (currently set to args.getCommits) to use a fallback mock (args.getCommits || jest.fn()) or initialize a local const getCommits = args.getCommits ?? jest.fn() and pass that into the component/test harness.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/main/services/lanes/autoRebaseService.ts`:
- Around line 296-303: The pruning branch that clears statuses (the else-if
checking hasFreshLaneStatus, lane.status.behind <= 0, status.source !==
"manual", and preserveLaneIds) is removing descendant "blocked by ancestor"
rebasePending statuses; update processRoot() so descendant statuses written when
an ancestor blocks are tagged (e.g., set status.source = "ancestor-blocked" or
status.blockedByAncestor = true on rebasePending), and change the prune
condition in autoRebaseService (the branch that calls clearStatus(lane.id)) to
skip clearing when the status indicates an ancestor-block (e.g., add &&
status.source !== "ancestor-blocked" && !status.blockedByAncestor) so only truly
stale self-statuses are removed.
In `@apps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsx`:
- Around line 493-496: The current fallback logic for status vs arrays is
inconsistent: change the checks for checks, reviews, and comments to use
null-coalescing like status so we don't mix live and snapshot timestamps; when
computing the derived values (the constants status, checks, reviews, comments
that use liveDetailReady and liveStatus), replace expressions like
liveChecks.length > 0 ? liveChecks : snapshotChecks (and analogous
liveReviews/liveComments) with liveChecks ?? snapshotChecks (and liveReviews ??
snapshotReviews, liveComments ?? snapshotComments) so all fields uniformly
prefer live data when present and otherwise fall back to snapshot data.
In `@apps/desktop/src/renderer/components/prs/state/PrsContext.tsx`:
- Around line 969-979: The snapshot prefill is being hidden because
setDetailBusy(true) is left true and the cache timestamp isn’t updated, so when
the snapshot resolves it gets masked and immediately invalidated; after applying
the snapshot inside the window.ade.prs.listSnapshots promise handler, call
setDetailBusy(false) and set detailStatePrIdRef.current = prId so the prefilled
state is recognized by refreshDetailSilently/hasFreshDetailCache, but do NOT set
detailLoadedAtByPrIdRef.current[prId] (leave it unset so the primary live fetch
is still forced); ensure the promise handler still returns early on
cancelled/selectedPrIdRef mismatch or liveDetailApplied as before.
---
Nitpick comments:
In `@apps/desktop/src/main/services/shared/queueRebase.test.ts`:
- Around line 70-91: Add a complementary test that verifies cache hits inside
the TTL by calling fetchQueueTargetTrackingBranches twice with Date.now mocked
to initialNow and then initialNow + 1000 (or any value <
TEST_QUEUE_TARGET_FETCH_TTL_MS), using the same db/projectId/projectRoot setup,
and asserting mockGit.runGitOrThrow was called only once; use the same helpers
(fetchQueueTargetTrackingBranches, makeDb) and TEST_QUEUE_TARGET_FETCH_TTL_MS
constant, clear mocks appropriately between calls and avoid advancing time past
the TTL so the second call hits the cache.
In
`@apps/desktop/src/renderer/components/prs/detail/PrDetailPane.issueResolver.test.tsx`:
- Line 464: Tests are failing when window.ade.prs.getCommits is not provided
because getCommits is passed through as-is; add a default mock (e.g., jest.fn())
for getCommits the same way getDetail and getFiles have defaults so calling
window.ade.prs.getCommits() won't be undefined. Update the object that sets
getCommits (currently set to args.getCommits) to use a fallback mock
(args.getCommits || jest.fn()) or initialize a local const getCommits =
args.getCommits ?? jest.fn() and pass that into the component/test harness.
In `@apps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsx`:
- Around line 507-510: Extract the snapshot-clearing logic out of the effect
currently handling detail-tab initialization and put it into its own
React.useEffect that runs when pr.id changes; specifically remove the
setSnapshotStatus, setSnapshotChecks, setSnapshotReviews, and
setSnapshotComments calls from the existing initialization effect and create a
new effect that invokes those four setters with null/empty-array values and
lists [pr.id] as its dependency so snapshot state is reset whenever the PR id
changes.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dd7480ea-6623-4a77-b6af-92ea51036082
📒 Files selected for processing (16)
apps/desktop/src/main/services/adeActions/registry.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/lanes/autoRebaseService.test.tsapps/desktop/src/main/services/lanes/autoRebaseService.tsapps/desktop/src/main/services/prs/prService.test.tsapps/desktop/src/main/services/prs/prService.tsapps/desktop/src/main/services/shared/queueRebase.test.tsapps/desktop/src/main/services/shared/queueRebase.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/components/prs/detail/PrDetailPane.issueResolver.test.tsxapps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsxapps/desktop/src/renderer/components/prs/state/PrsContext.test.tsxapps/desktop/src/renderer/components/prs/state/PrsContext.tsxapps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsxapps/desktop/src/renderer/components/prs/tabs/IntegrationTab.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/desktop/src/main/services/lanes/autoRebaseService.test.ts
- apps/desktop/src/main/services/shared/queueRebase.ts
- apps/desktop/src/renderer/components/prs/state/PrsContext.test.tsx
- apps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsx
- apps/desktop/src/main/services/prs/prService.ts
Copilot stopped work on behalf of
arul28 due to an error
May 12, 2026 22:25
Owner
Author
|
@copilot review but do not make fixes |
Owner
Author
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit c4433b5. Configure here.
Copilot stopped work on behalf of
arul28 due to an error
May 12, 2026 22:38
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
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.
Note
Medium Risk
Touches PR listing/detail hydration, IPC contracts, and lane deletion behavior; regressions could affect PR/workflow rendering and destructive lane delete UX despite added test coverage.
Overview
Improves PRs tab first-paint performance by making expensive work opt-in and more batched: PR lists now default to
listWithConflicts({ includeConflictAnalysis: false }), workflow routes request conflict analysis explicitly, and lane reads on PR paths use metadata-only status.Adds new PR IPC/actions for
getMergeContexts(prIds)(bulk, chunked below SQLite bind limits) andlistSnapshots({ prId })to prefill PR detail panes from cached snapshot rows while live GitHub data loads; also addsincludeExternalClosedmode togetGitHubSnapshotwith hardened in-flight/caching behavior.Bounds and parallelizes explicit/background PR refresh (
REFRESH_CONCURRENCY), batches conflict analysis via a newconflictService.getBatchAssessmentcall, TTL-throttles queue-targetgit fetchattempts, and tweaks git stash restore toapplythendrop(logging drop failures).Changes lane deletion to be non-cancellable end-to-end (UI cancel removed, service cancel stubbed), and deletes multiple selected lanes in leaf-first parallel batches to avoid parent/child ordering issues.
Adds a new
ade-perf-prsskill doc capturing these performance guardrails and updates tests/types/preload mocks to match the new IPC surface.Reviewed by Cursor Bugbot for commit c4433b5. Configure here.
Summary by CodeRabbit
Performance & Efficiency
New Features
Improvements
Lane Management
VCS UX
Greptile Summary
This PR speeds up the PRs tab by making expensive operations opt-in, batched, and cache-aware: conflict analysis is now only computed when the workflow tab is active, merge contexts are bulk-fetched via a new chunked
getMergeContextsAPI, and PR detail data is prefilled from SQLite snapshots before live GitHub calls settle.PrsContextnow loads cached snapshot rows from SQLite immediately on PR selection, painting status/checks/reviews/comments before the live GitHub fetch returns; aliveDetailAppliedflag anddetailLiveDataPrIdtracker prevent snapshot data from overwriting fresh live results.getGithubSnapshotnow tracks per-requestincludeExternalClosedmetadata so that a narrower open-only request settling while a wider closed-history request is in-flight no longer resetscachedGithubSnapshotIncludesExternalClosed; deduplication guards are updated accordingly.autoRebaseServicereplaces theincludeAllprune bypass withpreserveLaneIdsfor finer-grained control;git stash popis replaced withapply+ best-effortdrop.Confidence Score: 4/5
Safe to merge with awareness of two minor edge cases in the new snapshot-prefill and queue-fetch paths.
The change is large and touches core PR data-fetching, caching, and conflict-analysis paths, but all previously identified race conditions in the GitHub snapshot logic and autoRebase pruning have been addressed. The two remaining points are an edge-case ordering issue where liveDetailApplied blocks snapshot fallback after a full rate-limit rejection, and a semantic shift in fetchRemoteTrackingBranch that always returns false in the fallback even after a successful broad fetch — neither affects the common path.
The liveDetailApplied flag assignment in PrsContext.tsx and the fetchRemoteTrackingBranch fallback return value in queueRebase.ts warrant a second look.
Important Files Changed
Sequence Diagram
sequenceDiagram participant UI as PrsContext / GitHubTab participant Preload as Preload Bridge participant PrSvc as prService (main) participant DB as SQLite participant GH as GitHub API Note over UI,GH: PR tab open — first load UI->>Preload: "listWithConflicts({ includeConflictAnalysis: false })" Preload->>PrSvc: listWithConflicts PrSvc->>DB: listRows() DB-->>PrSvc: rows (no conflict queries) PrSvc-->>UI: PrWithConflicts[] (conflictAnalysis: null) UI->>Preload: getMergeContexts(prIds[]) Preload->>PrSvc: getMergeContexts (chunked IN clauses) PrSvc->>DB: SELECT pr_rows + groups + members (3 queries) DB-->>PrSvc: batch results PrSvc-->>UI: "Record<id, PrMergeContext>" Note over UI,GH: PR selected — detail load UI->>Preload: "listSnapshots({ prId })" Preload->>PrSvc: listSnapshots PrSvc->>DB: SELECT snapshot row DB-->>PrSvc: PrSnapshotHydration PrSvc-->>UI: snapshot (prefills status/checks/reviews/comments) par Live fetch (concurrent) UI->>GH: getStatus / getChecks / getReviews / getComments GH-->>UI: "live results → liveDetailApplied=true" end Note over UI: liveDetailReady=true → live data shown Note over UI,GH: GitHub tab — filter changes to closed/merged UI->>Preload: "getGitHubSnapshot({ includeExternalClosed: true })" Preload->>PrSvc: getGithubSnapshot alt open-only in-flight PrSvc->>PrSvc: start new includeExternalClosed request Note over PrSvc: pendingOpenOnlySnapshot saved if cache empty end PrSvc->>GH: search/issues (no is:open filter) GH-->>PrSvc: full history snapshot PrSvc->>PrSvc: publishGithubSnapshot(snapshot, true) PrSvc-->>UI: GitHubPrSnapshot (with closed/merged)Comments Outside Diff (2)
apps/desktop/src/main/services/prs/prService.ts, line 5113-5115 (link)includeExternalClosed. If a request for open-only data is already in flight when a caller asks forincludeExternalClosed: true, this branch returns the open-only promise — silently dropping the closed-history requirement. The snapshot settles withcachedGithubSnapshotIncludesExternalClosed = false, so the caller that needed closed PRs receives incomplete results with no error.Prompt To Fix With AI
apps/desktop/src/renderer/components/prs/state/PrsContext.tsx, line 626-658 (link)Object.keys(contexts).length === 0. IfgetMergeContextsreturns even one result, the fallback is skipped entirely — so PRs whose IDs are absent from the batch response (e.g. external GitHub-only PRs not yet in the local DB) will silently have no merge context loaded. The old per-ID loop would have attempted each one individually and skipped failures. A safer condition would be to also fall back for anyuniquePrIdsnot present incontexts.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (18): Last reviewed commit: "Refine PR snapshot and rebase status fre..." | Re-trigger Greptile