Review tab parallel review organization#322
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughAdds stash OID propagation and branch-scoped stash operations; implements “create lane from PR branch” preflight/create flows and auto-link ignore persistence; refactors PR snapshot hydration and detail progressive loading; extends review service to persist per-reviewer runs and candidate findings; updates IPC, preload, types, DB migrations, UI, and many tests. Changes Primary Cohort
🎯 5 (Critical) | ⏱️ ~120 minutes
Suggested labels: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
|
@coderabbitai review |
|
@copilot review but do not make fixes |
✅ Actions performedReview triggered.
|
|
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. |
Reviewed the branch through commit |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/desktop/src/main/services/prs/prService.ts (2)
3240-3264:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPersist the auto-link ignore for any local PR removal.
This guard is too narrow. When
closeOnGitHubistrueand the lane stays active, the row is removed locally but no ignore is recorded, and the next GitHub snapshot can backfill the same PR again because backfill readsstate: "all". That breaks the new “intentionally unmapped” behavior for both successfully closed PRs and failed-close cases.💡 Suggested fix
- const localUnmapOnly = args.closeOnGitHub !== true && args.archiveLane !== true; ... - if (localUnmapOnly) { - rememberAutoLinkIgnore(row); - } + // Any explicit local removal should stay unmapped until the user + // relinks/recreates it. The ignore is cleared on explicit relink paths. + rememberAutoLinkIgnore(row);🤖 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 3240 - 3264, The current guard uses localUnmapOnly so rememberAutoLinkIgnore(row) runs only when closeOnGitHub is false, which misses cases where we remove the row locally after attempting to close on GitHub; move or remove that guard so rememberAutoLinkIgnore(row) is invoked whenever we perform the local PR unmap/removal (i.e., unconditionally in this removal code path), keeping the existing githubService PATCH/try-catch (githubClosed, githubCloseError) intact and only changing the condition around rememberAutoLinkIgnore to ensure it runs for both closeOnGitHub true and false.
5686-5736:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t reuse an in-flight GitHub snapshot after repo/auth changes.
invalidateGithubSnapshotCache()only clears the published snapshot. If an old request is still sitting ingithubSnapshotInFlight, the laterif (!force && githubSnapshotInFlight)branch will return that stale promise, so callers can receive the previous repo/session snapshot after switching context.💡 Suggested fix
-let githubSnapshotInFlight: { request: Promise<GitHubPrSnapshot> } | null = null; +let githubSnapshotInFlight: { + request: Promise<GitHubPrSnapshot>; + repoKey: string | null; + viewerLogin: string | null; +} | null = null; ... - inFlight = { request }; + inFlight = { + request, + repoKey: repoRefKey(precheckedGithubStatus.repo), + viewerLogin: precheckedGithubStatus.userLogin ?? null, + }; githubSnapshotInFlight = inFlight; return request; }; + + const currentRepoKey = repoRefKey(githubStatus.repo); + const currentViewerLogin = githubStatus.userLogin ?? null; + if ( + githubSnapshotInFlight && + (githubSnapshotInFlight.repoKey !== currentRepoKey + || githubSnapshotInFlight.viewerLogin !== currentViewerLogin) + ) { + githubSnapshotInFlight = null; + }🤖 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 5686 - 5736, The in-flight snapshot promise (githubSnapshotInFlight) can be returned after repo/auth changes because invalidateGithubSnapshotCache() only clears the published snapshot; fix by ensuring invalidateGithubSnapshotCache also clears/invalidates githubSnapshotInFlight (or increments githubSnapshotCacheEpoch) so stale in-flight requests are not reused; update the code that calls invalidateGithubSnapshotCache or the invalidateGithubSnapshotCache implementation to set githubSnapshotInFlight = null (and/or bump githubSnapshotCacheEpoch) so the later `if (!force && githubSnapshotInFlight)` branch cannot return a stale promise, keeping the logic in startSnapshotRequest, githubSnapshotCacheEpoch, and publishGithubSnapshot consistent.apps/desktop/src/renderer/components/prs/state/PrsContext.tsx (1)
1372-1405:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the fallback detail path from marking partial data as live.
This fallback branch still treats partial primary success as fresh detail:
markPrimarySettled()keeps the PR live whenprimaryFulfilledCount > 0, and the per-piece success path updatesdetailStatePrIdRef,detailLoadedAtByPrIdRef, anddetailLiveDataPrIdimmediately. In any environment wherewindow.ade.prs.listSnapshotsis unavailable, one successful status/checks/reviews/comments call can still mask stale sections and short-circuit later refreshes. MirrorstartProgressivePrimaryFetch()here and only mark the detail cache fresh after all four primary calls succeed.🤖 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/state/PrsContext.tsx` around lines 1372 - 1405, The fallback primary-piece path (loadPrimaryPiece) is marking partial results as fresh/live; change it to mirror startProgressivePrimaryFetch by deferring updates that mark the cache live until all primary requests succeed: do not set detailStatePrIdRef.current, detailLoadedAtByPrIdRef.current[prId], or call setDetailLiveDataPrId(prId) inside the per-piece success branch of loadPrimaryPiece; only mark those after markPrimarySettled determines primaryFulfilledCount === primaryRequestCount (and not rateLimited). Adjust markPrimarySettled to only treat the detail as fresh when primaryFulfilledCount === primaryRequestCount and ensure detailFetchInProgress.current is cleared only at the same finalization point; keep liveDetailApplied true for tracking but do not commit the cache refs/live flag until all four primary calls succeed.
🧹 Nitpick comments (3)
apps/desktop/src/main/services/ai/tools/ctoOperatorTools.test.ts (1)
201-216: ⚡ Quick winStrengthen the stash-selection fixture to actually prove “latest” resolution.
This test currently passes with a single stash entry, so it doesn’t verify the selection logic when multiple stashes exist. Add at least one older/mismatched stash and assert the expected one is chosen.
🤖 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/ai/tools/ctoOperatorTools.test.ts` around lines 201 - 216, The test for gitStashPop uses only one stash so it doesn't prove the "latest" selection; update the fixture returned by gitService.listStashes in the test for tools.gitStashPop.execute to include multiple stashes (e.g., an older mismatch and the intended latest stash with createdAt later), keep the expected latest entry having oid "oid-3" and ref "stash@{3}", and assert that gitService.stashPop was called with that latest stash ({ laneId, stashRef: "stash@{3}", stashOid: "oid-3" }) while still verifying listStashes was called with { laneId: "lane-1" } and the result matches the expected operationId.apps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsx (2)
652-656: 💤 Low valuePotential type issue with
ctxReviewThreadsassignment.The condition
(ctxReviewThreads?.length ?? 0) > 0ensures the array has elements, butsetReviewThreads(ctxReviewThreads)passes the potentially-undefined value without a non-null assertion. While logically safe at runtime, TypeScript may flag this.Suggested fix
React.useEffect(() => { if (ctxDetailPrId === pr.id && (ctxReviewThreads?.length ?? 0) > 0) { - setReviewThreads(ctxReviewThreads); + setReviewThreads(ctxReviewThreads!); } }, [ctxDetailPrId, ctxReviewThreads, pr.id]);🤖 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 652 - 656, The useEffect may pass an undefined value to setReviewThreads by calling setReviewThreads(ctxReviewThreads) even though you guarded length; update the assignment to guarantee a non-null array or assert non-null so TypeScript won’t complain — e.g., inside the effect where you check (ctxReviewThreads?.length ?? 0) > 0, call setReviewThreads with a safe value (use ctxReviewThreads ?? [] or ctxReviewThreads! as appropriate) referencing ctxReviewThreads, setReviewThreads, and the surrounding useEffect that checks ctxDetailPrId and pr.id.
4060-4117: 💤 Low valueDuplicate modal code could use the extracted
PrReviewSubmitModalcomponent.The review modal JSX here duplicates the
PrReviewSubmitModalcomponent defined at lines 3259-3347. Consider replacing this inline modal with the extracted component for consistency.Suggested refactor
- {/* ---- Review Modal ---- */} - {props.showReviewModal && ( - <div style={{ position: "fixed", inset: 0, background: "rgba(0,0,0,0.7)", backdropFilter: "blur(4px)", display: "flex", alignItems: "center", justifyContent: "center", zIndex: 100 }}> - <div style={{ background: COLORS.cardBgSolid, border: `1px solid ${COLORS.outlineBorder}`, borderRadius: 16, padding: 24, width: 500, maxHeight: "80vh", overflow: "auto", boxShadow: "0 20px 60px rgba(0,0,0,0.5)" }}> - {/* ... modal content ... */} - </div> - </div> - )} + <PrReviewSubmitModal + open={props.showReviewModal} + actionBusy={actionBusy} + reviewBody={props.reviewBody} + setReviewBody={props.setReviewBody} + reviewEvent={props.reviewEvent} + setReviewEvent={props.setReviewEvent} + onCancel={() => props.setShowReviewModal(false)} + onSubmit={() => void props.onSubmitReview()} + />🤖 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 4060 - 4117, The modal JSX in PrDetailPane duplicates the existing PrReviewSubmitModal component; replace the inline modal block with a single PrReviewSubmitModal usage to avoid duplication. Locate the modal JSX in the PrDetailPane render and remove it, then render <PrReviewSubmitModal ...> passing the same props/state handlers used in the inline code (showReviewModal, setShowReviewModal, reviewEvent, setReviewEvent, reviewBody, setReviewBody, onSubmitReview, and actionBusy) so behavior and styling remain identical; ensure imports/reference to PrReviewSubmitModal are present and remove the duplicated styles/handlers from the inline fragment.
🤖 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/ade-cli/src/cli.ts`:
- Around line 2707-2724: The function resolveStashSelectionForCli currently only
accepts stashRef and always falls back to the first stash; change its signature
to accept both stashRef: string | null and stashOid: string | null, then resolve
the selected stash by: 1) if stashRef is provided, find stash where
asString(stash.ref) === stashRef; 2) else if stashOid is provided, find stash
where asString(stash.oid) === stashOid; 3) else use the first stash. Set
selectedRef and stashOid from the matched stash and return them, preserving the
existing CliUsageError messages when nothing matches (but use the provided
stashOid in the error message when searching by oid). Update any call sites that
invoke resolveStashSelectionForCli to pass the new stashOid argument.
In `@apps/desktop/src/main/services/ai/tools/ctoOperatorTools.ts`:
- Around line 2630-2648: The gitStashPop tool currently lets a user-supplied
stashRef that doesn't match any stash fall through and causes
deps.gitService!.stashPop to throw a low-level "stashOid is required" error;
change the logic in tools.gitStashPop (around resolveLaneId, selectedStash,
stashRef handling) to detect when a trimmed stashRef was provided but
selectedStash is undefined and throw a clear "stash not found" error (or
similar) before calling deps.gitService!.stashPop; keep existing behavior when
no stashRef is provided (use stashes[0]) and continue passing stashOid only when
selectedStash?.oid exists.
In `@apps/desktop/src/main/services/ipc/registerIpc.ts`:
- Around line 8591-8607: The handler for IPC.prsGetHealth currently fabricates a
PrHealth object when ensurePrPolling() returns falsy; instead modify the
ipcMain.handle for IPC.prsGetHealth to return an explicit unavailable value
(e.g., null) rather than a fake PrHealth, by checking ensurePrPolling() and
returning null when ctx is absent, update the handler's Promise return type
(PrHealth | null) and adjust callers to handle null accordingly; keep the
successful path calling ctx.prService.getPrHealth(arg.prId) unchanged.
- Around line 8313-8332: The emptyPrDetail and emptyPrMergeContext helpers
currently return plausible-but-empty PR objects that can be persisted and poison
the cache; change them to return an explicit unavailable sentinel (e.g., an
object with a clear flag like unavailable: true or simply null) instead of a
valid-looking payload, and update the PrMergeContext/PR detail type definitions
and all callers of emptyPrDetail and emptyPrMergeContext to handle this sentinel
(or move the fallback entirely into the renderer so these helpers are never used
for cached state). Ensure callers distinguish sentinel vs real data before
caching or rendering.
- Around line 8970-8989: The handlers for IPC.prsGetFiles / IPC.prsGetCommits /
IPC.prsGetActionRuns / IPC.prsGetActivity use ensurePrPolling() and return []
when ctx is missing, which can overwrite hydrated cached subresources with
empties; change each handler (the ipcMain.handle registrations) to return a
nullable or availability-tagged response instead of an empty array (e.g., null
or { unavailable: true }) when ensurePrPolling() yields no ctx, update the
returned Promise/return types for ctx.prService.getFiles, getCommits,
getActionRuns, getActivity callers to accept the nullable/availability shape so
callers can skip caching fallback responses rather than persisting empty lists.
In `@apps/desktop/src/main/services/review/reviewService.ts`:
- Around line 548-557: The UI regex in classifyChangedFileRisk is wrong:
`/\b(renderer|components|tsx$|css$)\b/i` uses word boundaries and treats `$`
incorrectly, so files like "foo.tsx" or "style.css" won't match. Fix the UI test
by changing the pattern in classifyChangedFileRisk to explicitly match
extensions (e.g., use `\.tsx$` and `\.css$`) and combine with the existing word
checks — for example replace that regex with one that matches either
`\b(renderer|components)\b` or the end-of-string extensions like `\.tsx$|\.css$`
(keep the `i` flag).
In `@apps/desktop/src/main/services/state/kvDb.ts`:
- Around line 537-555: The early continue conditioned on
hasClockTable/hasPksTable/triggerCount (the if (!hasClockTable && !hasPksTable
&& triggerCount === 0) continue;) can skip necessary cleanup of
crsql_master/crsql_changes rows; modify the logic so metadata-only cleanup still
runs: either remove/adjust that continue to also check for metadata rows (use
rawHasTable/rawHasColumn and/or a lightweight existence query against
crsql_master/crsql_changes) or move the runStatement deletions for
crsql_master/crsql_changes to execute before the continue; keep usages of
hasClockTable, hasPksTable, triggerCount, dropCrrTriggers, runStatement,
rawHasTable, rawHasColumn and quoteIdentifier intact while ensuring deletion of
metadata rows always happens when present.
In `@apps/desktop/src/renderer/components/prs/detail/PrDetailTimelineRails.tsx`:
- Around line 606-608: The Cmd/Ctrl+Enter onKeyDown handler in
PrDetailTimelineRails currently calls onAddComment unguarded; update the
onKeyDown handlers (the one shown and the similar handler around lines
referenced) so they check the same conditions as the submit button before
invoking onAddComment: ensure event.key === "Enter" && (event.metaKey ||
event.ctrlKey) and also !actionBusy and that the draft is non-empty (e.g.,
draft.trim().length > 0) so duplicate or empty submissions are prevented.
- Line 620: The inline style that sets outline: "none" on the composer textarea
in PrDetailTimelineRails.tsx removes the keyboard focus indicator; revert this
by removing the outline: "none" or replace it with an accessible alternative
(e.g., a visible outline/box-shadow on :focus or :focus-visible) for the
textarea element so keyboard users retain a clear focus ring; locate the
textarea/composer styling in PrDetailTimelineRails.tsx and update the inline
style or CSS class accordingly (remove outline: "none" or provide an accessible
focus style).
In `@apps/desktop/src/renderer/components/prs/state/PrsContext.tsx`:
- Around line 948-956: The effect that retries refreshCore() on every active &&
error (inside useEffect referencing active, error, refreshCore and retryTimer)
must stop unconditional retries: add local state (e.g., retryCount) and cap
retries (maxRetries) or switch to exponential backoff, increment retryCount each
scheduled retry and only schedule another timeout if retryCount < maxRetries;
when max reached, stop scheduling further retries and surface a "manual retry"
action so the user can call refreshCore() explicitly; ensure the cleanup still
clears retryTimer and reset retryCount when refreshCore succeeds or when active
becomes false.
In `@apps/desktop/src/renderer/components/prs/tabs/GitHubTab.test.tsx`:
- Around line 431-447: The test "requires confirmation before unmapping a GitHub
PR from its lane" creates a spy confirmSpy on window.confirm but never restores
it, potentially leaking into other tests; update the test to restore the spy
after use by calling confirmSpy.mockRestore() (or vi.restoreAllMocks()) at the
end of the test or in an afterEach cleanup, ensuring window.confirm is returned
to its original implementation; locate the confirmSpy declared in
GitHubTab.test.tsx and add the restore call (or global restore) so subsequent
tests are unaffected.
In `@apps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsx`:
- Around line 1080-1094: The effect currently only collects linkedPrId from
filteredItems.slice(0, 8) so it refreshes only the first 8 PRs; update it to
gather linkedPrId from the actually rendered/visible items instead (e.g., use
the component's visible/rendered items array or virtualization window rather
than slice(0,8)). Specifically, replace the slice(0, 8) source with the
prop/state that represents what is currently rendered (or derive it from the
virtualization/scroll window), keep the uniquePrIds/key logic tied to
visibleLinkedHydrationKeyRef and keep the timer + onRefreshAll call unchanged so
visible rendered linked PRs get targeted refreshes.
In `@apps/desktop/src/renderer/components/review/ReviewPage.tsx`:
- Around line 392-397: The function formatPublicationOutcome incorrectly uses a
ternary that returns the same string ("completed") in both branches, losing
singular/plural distinction; update the return to pluralize the noun instead of
the adjective—for example, change the template to use publishedCount and choose
between "publication" and "publications" based on publishedCount (publishedCount
=== 1 ? "publication" : "publications") so the message becomes e.g. "1
publication completed." or "2 publications completed."; adjust this inside
formatPublicationOutcome where publishedCount is used.
---
Outside diff comments:
In `@apps/desktop/src/main/services/prs/prService.ts`:
- Around line 3240-3264: The current guard uses localUnmapOnly so
rememberAutoLinkIgnore(row) runs only when closeOnGitHub is false, which misses
cases where we remove the row locally after attempting to close on GitHub; move
or remove that guard so rememberAutoLinkIgnore(row) is invoked whenever we
perform the local PR unmap/removal (i.e., unconditionally in this removal code
path), keeping the existing githubService PATCH/try-catch (githubClosed,
githubCloseError) intact and only changing the condition around
rememberAutoLinkIgnore to ensure it runs for both closeOnGitHub true and false.
- Around line 5686-5736: The in-flight snapshot promise (githubSnapshotInFlight)
can be returned after repo/auth changes because invalidateGithubSnapshotCache()
only clears the published snapshot; fix by ensuring
invalidateGithubSnapshotCache also clears/invalidates githubSnapshotInFlight (or
increments githubSnapshotCacheEpoch) so stale in-flight requests are not reused;
update the code that calls invalidateGithubSnapshotCache or the
invalidateGithubSnapshotCache implementation to set githubSnapshotInFlight =
null (and/or bump githubSnapshotCacheEpoch) so the later `if (!force &&
githubSnapshotInFlight)` branch cannot return a stale promise, keeping the logic
in startSnapshotRequest, githubSnapshotCacheEpoch, and publishGithubSnapshot
consistent.
In `@apps/desktop/src/renderer/components/prs/state/PrsContext.tsx`:
- Around line 1372-1405: The fallback primary-piece path (loadPrimaryPiece) is
marking partial results as fresh/live; change it to mirror
startProgressivePrimaryFetch by deferring updates that mark the cache live until
all primary requests succeed: do not set detailStatePrIdRef.current,
detailLoadedAtByPrIdRef.current[prId], or call setDetailLiveDataPrId(prId)
inside the per-piece success branch of loadPrimaryPiece; only mark those after
markPrimarySettled determines primaryFulfilledCount === primaryRequestCount (and
not rateLimited). Adjust markPrimarySettled to only treat the detail as fresh
when primaryFulfilledCount === primaryRequestCount and ensure
detailFetchInProgress.current is cleared only at the same finalization point;
keep liveDetailApplied true for tracking but do not commit the cache refs/live
flag until all four primary calls succeed.
---
Nitpick comments:
In `@apps/desktop/src/main/services/ai/tools/ctoOperatorTools.test.ts`:
- Around line 201-216: The test for gitStashPop uses only one stash so it
doesn't prove the "latest" selection; update the fixture returned by
gitService.listStashes in the test for tools.gitStashPop.execute to include
multiple stashes (e.g., an older mismatch and the intended latest stash with
createdAt later), keep the expected latest entry having oid "oid-3" and ref
"stash@{3}", and assert that gitService.stashPop was called with that latest
stash ({ laneId, stashRef: "stash@{3}", stashOid: "oid-3" }) while still
verifying listStashes was called with { laneId: "lane-1" } and the result
matches the expected operationId.
In `@apps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsx`:
- Around line 652-656: The useEffect may pass an undefined value to
setReviewThreads by calling setReviewThreads(ctxReviewThreads) even though you
guarded length; update the assignment to guarantee a non-null array or assert
non-null so TypeScript won’t complain — e.g., inside the effect where you check
(ctxReviewThreads?.length ?? 0) > 0, call setReviewThreads with a safe value
(use ctxReviewThreads ?? [] or ctxReviewThreads! as appropriate) referencing
ctxReviewThreads, setReviewThreads, and the surrounding useEffect that checks
ctxDetailPrId and pr.id.
- Around line 4060-4117: The modal JSX in PrDetailPane duplicates the existing
PrReviewSubmitModal component; replace the inline modal block with a single
PrReviewSubmitModal usage to avoid duplication. Locate the modal JSX in the
PrDetailPane render and remove it, then render <PrReviewSubmitModal ...> passing
the same props/state handlers used in the inline code (showReviewModal,
setShowReviewModal, reviewEvent, setReviewEvent, reviewBody, setReviewBody,
onSubmitReview, and actionBusy) so behavior and styling remain identical; ensure
imports/reference to PrReviewSubmitModal are present and remove the duplicated
styles/handlers from the inline fragment.
🪄 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: 6c0bbdd0-cf90-444d-a0f9-ce96db760a75
⛔ Files ignored due to path filters (1)
.ade/ade.yamlis excluded by!.ade/**
📒 Files selected for processing (59)
.claude/skills/plan/SKILL.mdapps/ade-cli/src/adeRpcServer.test.tsapps/ade-cli/src/adeRpcServer.tsapps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.tsapps/desktop/scripts/ensure-ade-cli-build.cjsapps/desktop/src/main/rendererCsp.test.tsapps/desktop/src/main/services/adeActions/registry.tsapps/desktop/src/main/services/ai/tools/ctoOperatorTools.test.tsapps/desktop/src/main/services/ai/tools/ctoOperatorTools.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/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/review/reviewService.test.tsapps/desktop/src/main/services/review/reviewService.tsapps/desktop/src/main/services/state/kvDb.test.tsapps/desktop/src/main/services/state/kvDb.tsapps/desktop/src/main/services/sync/syncRemoteCommandService.test.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.test.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/components/app/AppShell.tsxapps/desktop/src/renderer/components/automations/adeActionSchemas.tsapps/desktop/src/renderer/components/graph/WorkspaceGraphPage.tsxapps/desktop/src/renderer/components/lanes/LaneGitActionsPane.test.tsxapps/desktop/src/renderer/components/lanes/LaneGitActionsPane.tsxapps/desktop/src/renderer/components/lanes/LanesPage.tsxapps/desktop/src/renderer/components/prs/PRsPage.tsxapps/desktop/src/renderer/components/prs/detail/PrDetailPane.issueResolver.test.tsxapps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsxapps/desktop/src/renderer/components/prs/detail/PrDetailTimelineRails.tsxapps/desktop/src/renderer/components/prs/prsRouteState.tsapps/desktop/src/renderer/components/prs/shared/PrTimeline.test.tsxapps/desktop/src/renderer/components/prs/shared/PrTimeline.tsxapps/desktop/src/renderer/components/prs/state/PrsContext.test.tsxapps/desktop/src/renderer/components/prs/state/PrsContext.tsxapps/desktop/src/renderer/components/prs/state/PrsContextWarmCache.test.tsxapps/desktop/src/renderer/components/prs/tabs/GitHubTab.test.tsxapps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsxapps/desktop/src/renderer/components/prs/tabs/IntegrationTab.tsxapps/desktop/src/renderer/components/review/ReviewFindingCard.tsxapps/desktop/src/renderer/components/review/ReviewPage.test.tsxapps/desktop/src/renderer/components/review/ReviewPage.tsxapps/desktop/src/renderer/components/review/reviewFindingCopy.tsapps/desktop/src/renderer/components/review/reviewFindingLabels.test.tsapps/desktop/src/renderer/components/review/reviewFindingLabels.tsapps/desktop/src/renderer/components/shared/ReviewLaunchModelControls.tsxapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/types/git.tsapps/desktop/src/shared/types/prs.tsapps/desktop/src/shared/types/review.tsapps/ios/ADE/Resources/DatabaseBootstrap.sqlapps/ios/ADE/Services/Database.swiftscripts/dev-shared.mjs
💤 Files with no reviewable changes (1)
- .claude/skills/plan/SKILL.md
✅ Actions performedReview triggered.
|
Reviewed through commit |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/desktop/src/renderer/components/prs/state/PrsContext.tsx (1)
51-52: ⚡ Quick winRename this constant to camelCase.
REFRESH_ERROR_RETRY_DELAYS_MSdoesn't follow the repo's TS/JS variable naming convention. Please rename it to something likerefreshErrorRetryDelaysMsto keep the new retry code aligned with the project standard.As per coding guidelines,
**/*.{ts,tsx,js,jsx}: Use camelCase for variable names in TypeScript/JavaScript files.🤖 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/state/PrsContext.tsx` around lines 51 - 52, Rename the constant REFRESH_ERROR_RETRY_DELAYS_MS to camelCase (e.g., refreshErrorRetryDelaysMs) and update every usage/reference in this file (and any imports if it's exported) to the new name; preserve the value and the "as const" typing so the tuple and ms suffix remain intact and any retry logic (functions that reference REFRESH_ERROR_RETRY_DELAYS_MS) continues to work unchanged.apps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsx (1)
1925-1932: 💤 Low valueEffect triggers on every render due to unstable
virtualItemsreference.
getVirtualItems()returns a new array on each render, so this effect runs every time. The parent'shandleHydrationItemsChangecomparison prevents infinite loops and unnecessary state updates, so this is functionally correct.If you notice performance issues, consider memoizing the mapped items array or comparing indices instead of the array reference.
🤖 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/tabs/GitHubTab.tsx` around lines 1925 - 1932, The effect runs every render because virtualizer.getVirtualItems() returns a new array reference; fix by deriving a stable key (e.g., virtual indices) or memoizing the mapped items before the effect: call virtualizer.getVirtualItems() once, map to indices (virtualItems.map(v => v.index)) and use React.useMemo to compute the GitHubPrListItem[] from items using those indices, then use that stable memoized array (or the indices array) in the dependency list for the effect that calls onHydrationItemsChange; update references to virtualizer.getVirtualItems(), virtualItems, items, onHydrationItemsChange, and GitHubPrListItem accordingly.
🤖 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/state/kvDb.ts`:
- Around line 537-545: The DELETEs against crsql_master/crsql_changes run
unconditionally before the early-continue check and will fail on readonly DBs;
change the logic to probe for matching rows first and only call runStatement(...
"delete ...") when rows exist. Specifically, for crsql_master and crsql_changes
(use rawHasTable/rawHasColumn to guard), run a cheap existence query (e.g.,
SELECT 1 or SELECT count(1) with the same WHERE clause using tableName) and only
add the delete changes to deletedMetadataCount when that query reports rows > 0;
keep the early-continue condition using hasClockTable, hasPksTable,
triggerCount, and deletedMetadataCount unchanged.
---
Nitpick comments:
In `@apps/desktop/src/renderer/components/prs/state/PrsContext.tsx`:
- Around line 51-52: Rename the constant REFRESH_ERROR_RETRY_DELAYS_MS to
camelCase (e.g., refreshErrorRetryDelaysMs) and update every usage/reference in
this file (and any imports if it's exported) to the new name; preserve the value
and the "as const" typing so the tuple and ms suffix remain intact and any retry
logic (functions that reference REFRESH_ERROR_RETRY_DELAYS_MS) continues to work
unchanged.
In `@apps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsx`:
- Around line 1925-1932: The effect runs every render because
virtualizer.getVirtualItems() returns a new array reference; fix by deriving a
stable key (e.g., virtual indices) or memoizing the mapped items before the
effect: call virtualizer.getVirtualItems() once, map to indices
(virtualItems.map(v => v.index)) and use React.useMemo to compute the
GitHubPrListItem[] from items using those indices, then use that stable memoized
array (or the indices array) in the dependency list for the effect that calls
onHydrationItemsChange; update references to virtualizer.getVirtualItems(),
virtualItems, items, onHydrationItemsChange, and GitHubPrListItem accordingly.
🪄 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: 7d608dbb-56e2-45a4-9d26-f2c857a58630
📒 Files selected for processing (17)
apps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/desktop/scripts/after-pack-runtime-fixes.cjsapps/desktop/scripts/validate-win-artifacts.mjsapps/desktop/src/main/services/ai/tools/ctoOperatorTools.test.tsapps/desktop/src/main/services/ai/tools/ctoOperatorTools.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/review/reviewService.test.tsapps/desktop/src/main/services/review/reviewService.tsapps/desktop/src/main/services/state/kvDb.test.tsapps/desktop/src/main/services/state/kvDb.tsapps/desktop/src/renderer/components/prs/detail/PrDetailTimelineRails.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.test.tsxapps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsxapps/desktop/src/renderer/components/review/ReviewPage.tsx
|
@coderabbitai review |
|
@copilot review but do not make fixes |
✅ Actions performedReview triggered.
|
Reviewed through commit |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@copilot review but do not make fixes |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Reviewed through commit |
|
@copilot review but do not make fixes |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Reviewed through commit |
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/desktop/src/main/services/git/gitOperationsService.ts (1)
1084-1097:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t treat post-apply ref-resolution failures as a successful pop.
This
catchnow swallows both thestash dropfailure and any failure from the secondresolveCurrentBranchStashRef(). If that re-list /rev-parsestep fails for an unrelated git error, ADE still reports success even though it never confirmed whether the stash was removed. Only downgrade the actual drop failure; let lookup failures bubble, or explicitly whitelist the “already gone/shifted” cases you want to tolerate.✂️ Narrow the swallowed path to the drop call
await runGitOrThrow(["stash", "apply", currentApplyRef], { cwd: lane.worktreePath, timeoutMs: 30_000 }); - try { - const currentDropRef = await resolveCurrentBranchStashRef(lane, stash); + const currentDropRef = await resolveCurrentBranchStashRef(lane, stash); + try { await runGitOrThrow(["stash", "drop", currentDropRef], { cwd: lane.worktreePath, timeoutMs: 30_000 }); invalidateStashReadCaches(); } catch (error) { const message = error instanceof Error ? error.message : String(error); logger.warn("git.stash_pop_drop_failed", {🤖 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/git/gitOperationsService.ts` around lines 1084 - 1097, The current catch block is swallowing errors from both resolveCurrentBranchStashRef and the stash drop, so move the call to resolveCurrentBranchStashRef(lane, stash) outside (before) the try or separate the operations so only the runGitOrThrow(["stash","drop", currentDropRef]) is wrapped in the catch; let resolveCurrentBranchStashRef failures bubble (or explicitly handle only the known "already gone" rev-parse cases), and keep the logger.warn("git.stash_pop_drop_failed", {...}) only for drop failures; ensure invalidateStashReadCaches() still runs after a successful drop.
🤖 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.
Outside diff comments:
In `@apps/desktop/src/main/services/git/gitOperationsService.ts`:
- Around line 1084-1097: The current catch block is swallowing errors from both
resolveCurrentBranchStashRef and the stash drop, so move the call to
resolveCurrentBranchStashRef(lane, stash) outside (before) the try or separate
the operations so only the runGitOrThrow(["stash","drop", currentDropRef]) is
wrapped in the catch; let resolveCurrentBranchStashRef failures bubble (or
explicitly handle only the known "already gone" rev-parse cases), and keep the
logger.warn("git.stash_pop_drop_failed", {...}) only for drop failures; ensure
invalidateStashReadCaches() still runs after a successful drop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9b15e84d-98c8-4ce0-808a-dcf276a9c228
📒 Files selected for processing (5)
apps/desktop/src/main/services/git/gitOperationsService.test.tsapps/desktop/src/main/services/git/gitOperationsService.tsapps/desktop/src/main/services/review/reviewService.test.tsapps/desktop/src/main/services/review/reviewService.tsapps/desktop/src/main/services/state/kvDb.ts
What changed
Validation
npx vitest run src/main/services/review/reviewService.test.ts src/main/services/prs/prService.test.ts src/main/rendererCsp.test.ts src/renderer/components/prs/detail/PrDetailPane.issueResolver.test.tsx src/renderer/components/prs/shared/PrTimeline.test.tsx src/renderer/components/prs/tabs/GitHubTab.test.tsx src/renderer/components/prs/state/PrsContext.test.tsx src/main/services/state/kvDb.bootstrapSql.test.ts --reporter=dotnpx vitest run src/renderer/components/prs/state/PrsContext.test.tsx src/renderer/components/prs/detail/PrDetailPane.issueResolver.test.tsx src/main/services/prs/prService.test.ts src/main/services/ai/tools/ctoOperatorTools.test.ts --reporter=dotnpm --prefix apps/desktop run typechecknpm --prefix apps/desktop run lintnpm --prefix apps/ade-cli run typechecknpm --prefix apps/ade-cli run testNotes
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Greptile Summary
This PR rewrites the review system from sequential to parallel specialist sessions (diff-risk, cross-file-impact, checks-and-tests, security-data, ui-regression) via
Promise.all, adds newReviewReviewerRun/ReviewCandidateFindingDB tables, a warm cross-PR snapshot cache, and aCreateLaneFromPrBranchDialogwith preflight checks.pr_auto_link_ignorestable prevents the backfill loop from re-linking deliberately unlinked PRs.normalizeGitHubFiltermigrates warm-cache entries away from the now-removed\"all\"filter option.Confidence Score: 3/5
Needs attention before merge — a race condition can silently discard completed review results, and a React anti-pattern can corrupt snapshot timestamps in Strict Mode.
Two P1 issues were found: the cancelledRuns race condition that discards valid completed work, and the side-effect inside a React state updater. The pre-existing getReviewThreads removal (detailReviewThreads always empty) and double-truncation of the manifest prompt from the previous review also remain unresolved.
reviewService.ts (cancelledRuns race condition), PrsContext.tsx (side-effect in state updater)
Important Files Changed
Comments Outside Diff (4)
apps/desktop/src/main/services/review/reviewService.ts, line 1960-1970 (link)manifestPromptis truncated tomaxPromptCharsand then the fully assembled prompt (instructions + manifest + context artifact hints + rule overlays + validation context) is truncated again to the samemaxPromptChars. Because the instructions prefix appears before the manifest in the prompt, the second truncation cuts off everything after the manifest — context artifact IDs, rule overlays, and validation signals — whenever the manifest is close to the budget limit.For the default bounded budget (60 k
maxPromptChars) with 25 files at 1 500-char excerpts, the manifest JSON alone is ~50–70 k chars. The reviewer would silently receive a prompt missing artifact IDs (cannot cite evidence), missing rule overlays (custom review rules ignored), and missing validation context. The fix is to truncate the manifest tomaxPromptCharsminus the surrounding prompt skeleton size, or to place the manifest at the end of the prompt as the previousdiffTextwas.Prompt To Fix With AI
apps/desktop/src/renderer/components/prs/state/PrsContext.tsx, line 1043-1060 (link)detailReviewThreadspermanently empty after first PR selectiongetReviewThreadswas removed from both the primary detail fetch (thePromise.allSettledinrefreshDetailSilently) and the secondary detail fetch (startSecondaryDetailFetchsteps).setDetailReviewThreadsis now only ever called with[]— to clear the state. The state is initialized fromwarmCache?.detailReviewThreads ?? []so warm-cache data shows briefly on first mount, but as soon as the user selects a PR the clear fires and threads are never repopulated. Every consumer ofdetailReviewThreadswill always see an empty array after the initial navigation.Prompt To Fix With AI
apps/desktop/src/main/services/review/reviewService.ts, line 1-1177 (link)If
cancelRunis called just as all passes finish theirPromise.all, the guardif (cancelledRuns.has(runId)) returnexecutes after the work is done, discarding valid results with no user-visible feedback. ThecancelledRunsSet also grows unboundedly — entries are never removed after the run is discarded or completes normally.Prompt To Fix With AI
apps/desktop/src/renderer/components/prs/state/PrsContext.tsx, line 1-352 (link)The
mergeDetailSnapshotscallback mutatesdetailSnapshotLoadedAtByPrIdRef.currentinside thesetDetailSnapshotsByPrIdupdater function. React may invoke state updater functions more than once in Strict Mode (double-invoking in development), so this ref mutation can fire multiple times per state transition, corrupting the loaded-at timestamp.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (6): Last reviewed commit: "ship: bound review manifest prompts" | Re-trigger Greptile