Open
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
|
|
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:
📝 WalkthroughWalkthroughReplaces per-dialog Radix primitives with a shared Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
arul28
force-pushed
the
ade/fix-lanes-tab-374ebbe8
branch
from
f799faf to
acd72f3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
apps/desktop/src/renderer/components/lanes/AttachLaneDialog.tsx (1)
5-8: Consider extracting shared CSS constants to avoid duplication.The same
SECTION_CLASS_NAME,LABEL_CLASS_NAME, andINPUT_CLASS_NAMEconstants are duplicated in bothAttachLaneDialog.tsxandCreateLaneDialog.tsx. Consider extracting these to a shared file (e.g.,laneDialogStyles.ts) to maintain consistency and reduce maintenance burden.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/lanes/AttachLaneDialog.tsx` around lines 5 - 8, Extract the duplicated CSS constants SECTION_CLASS_NAME, LABEL_CLASS_NAME, and INPUT_CLASS_NAME into a new shared module (e.g., export them from a new laneDialogStyles file), replace the inline definitions in AttachLaneDialog.tsx and CreateLaneDialog.tsx with imports from that shared module, and ensure the exported constant names remain identical so existing references (SECTION_CLASS_NAME, LABEL_CLASS_NAME, INPUT_CLASS_NAME) continue to work without further changes; update imports in both components and remove the local duplicated definitions.apps/desktop/src/renderer/components/lanes/LaneDialogShell.tsx (1)
27-27: Consider preventing dialog close when busy.The close button is disabled when
busy, but users can still close the dialog by clicking the overlay or pressing Escape. If operations should not be interrupted, consider conditionally controllingonOpenChange.💡 Suggested improvement to block close during busy state
- <Dialog.Root open={open} onOpenChange={onOpenChange}> + <Dialog.Root open={open} onOpenChange={(nextOpen) => { + if (busy && !nextOpen) return; // Prevent closing while busy + onOpenChange(nextOpen); + }}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/lanes/LaneDialogShell.tsx` at line 27, The dialog currently lets users dismiss it via overlay/Escape even when busy because onOpenChange is passed through directly; wrap or replace the onOpenChange passed to Dialog.Root in LaneDialogShell with a handler that ignores close attempts while busy (allow opening when nextOpen is true, but if nextOpen is false and busy === true, do not call the original onOpenChange), and ensure any close button still respects the busy state; reference Dialog.Root, the onOpenChange prop and the LaneDialogShell component to locate where to add this conditional handler.apps/desktop/src/renderer/components/lanes/CreateLaneDialog.tsx (1)
211-218: Consider handling emptylocalBranchesgracefully.If
localBranchesis empty (no local branches available), the select will render with no options, which could confuse users. Consider adding a fallback message or disabling the "From primary" option when no local branches exist.💡 Suggested improvement
+ {localBranches.length === 0 ? ( + <option value="" disabled>No local branches available</option> + ) : null} {localBranches.map((branch) => ( <option key={branch.name} value={branch.name}> {branch.name} {branch.isCurrent ? " (current)" : ""} </option> ))}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/lanes/CreateLaneDialog.tsx` around lines 211 - 218, The select rendering of localBranches in CreateLaneDialog currently shows no options when localBranches is empty; update the rendering logic around the select (the block using localBranches.map and the "From primary" option) to handle empty arrays by: when localBranches.length === 0, render a single disabled fallback <option> with a message like "No local branches available" and also disable the "From primary" choice (or disable the entire select) to prevent selection; keep the existing mapping for non-empty arrays (map over localBranches, keying by branch.name and showing branch.isCurrent) and ensure the select's disabled prop or the "From primary" option is toggled based on localBranches.length === 0.apps/desktop/src/renderer/components/lanes/LaneWorkPane.tsx (1)
26-27: Consider memoizing thelaneListarray to avoid unnecessary re-renders.Creating
[work.lane]inline produces a new array reference on every render, which may causeWorkViewAreato re-render even when the lane hasn't changed.Proposed fix
export function LaneWorkPane({ laneId }: { laneId: string | null; }) { const work = useLaneWorkSessions(laneId); - const laneList = work.lane ? [work.lane] : []; + const laneList = useMemo( + () => (work.lane ? [work.lane] : []), + [work.lane], + );You'll also need to add
useMemoto the import on line 1.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/lanes/LaneWorkPane.tsx` around lines 26 - 27, The inline creation of laneList ([work.lane]) creates a new array each render causing unnecessary re-renders of WorkViewArea; import useMemo and replace the inline array with a memoized value, e.g. const laneList = useMemo(() => (work.lane ? [work.lane] : []), [work.lane]), so laneList only changes when work.lane changes; update the imports to include useMemo and ensure WorkViewArea receives the memoized laneList.apps/desktop/src/renderer/components/lanes/useLaneWorkSessions.ts (1)
57-61: Scope key computation duplicatesnormalizeLaneWorkScopeKeylogic from appStore.This duplicates the composite key logic defined in
appStore.ts(lines 59-64). If the key format changes, both locations would need updates.Proposed approach
Consider exporting
normalizeLaneWorkScopeKeyfromappStore.tsand reusing it here, or alternatively, rely solely on the store'sgetLaneWorkViewStatewhich already handles normalization internally.- const scopeKey = useMemo(() => { - const normalizedProjectRoot = projectRoot?.trim() ?? ""; - if (!normalizedProjectRoot || !laneId) return ""; - return `${normalizedProjectRoot}::${laneId}`; - }, [projectRoot, laneId]); + const getLaneWorkViewState = useAppStore((state) => state.getLaneWorkViewState); + // ... and use getLaneWorkViewState(projectRoot, laneId) where neededAlternatively, export the helper from appStore:
// In appStore.ts export { normalizeLaneWorkScopeKey };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/lanes/useLaneWorkSessions.ts` around lines 57 - 61, The scope key computed in useLaneWorkSessions duplicates the composite key logic from appStore; update useLaneWorkSessions to reuse the canonical normalizer instead of reimplementing it by importing and calling normalizeLaneWorkScopeKey (or by using appStore.getLaneWorkViewState which already normalizes) when computing scopeKey, replacing the inline useMemo logic that references projectRoot and laneId with a call to normalizeLaneWorkScopeKey(projectRoot, laneId) (or the equivalent store call) so key format is centralized and changes only need to be made in appStore.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/renderer/components/lanes/useLaneWorkSessions.ts`:
- Around line 119-123: When laneId changes the existing background refresh timer
scheduled by scheduleBackgroundRefresh can still fire with the old closure;
update the useEffect that calls setSessions and refresh so it first clears any
pending timer (e.g. clearTimeout on the timer id stored in the module/ref used
by scheduleBackgroundRefresh or call the library's cancel function) before
resetting sessions and invoking refresh, and also return a cleanup that clears
that same timer so it runs both on unmount and when laneId changes; reference
the useEffect that calls setSessions and refresh and the
scheduleBackgroundRefresh/timer ref used to schedule background refreshes.
---
Nitpick comments:
In `@apps/desktop/src/renderer/components/lanes/AttachLaneDialog.tsx`:
- Around line 5-8: Extract the duplicated CSS constants SECTION_CLASS_NAME,
LABEL_CLASS_NAME, and INPUT_CLASS_NAME into a new shared module (e.g., export
them from a new laneDialogStyles file), replace the inline definitions in
AttachLaneDialog.tsx and CreateLaneDialog.tsx with imports from that shared
module, and ensure the exported constant names remain identical so existing
references (SECTION_CLASS_NAME, LABEL_CLASS_NAME, INPUT_CLASS_NAME) continue to
work without further changes; update imports in both components and remove the
local duplicated definitions.
In `@apps/desktop/src/renderer/components/lanes/CreateLaneDialog.tsx`:
- Around line 211-218: The select rendering of localBranches in CreateLaneDialog
currently shows no options when localBranches is empty; update the rendering
logic around the select (the block using localBranches.map and the "From
primary" option) to handle empty arrays by: when localBranches.length === 0,
render a single disabled fallback <option> with a message like "No local
branches available" and also disable the "From primary" choice (or disable the
entire select) to prevent selection; keep the existing mapping for non-empty
arrays (map over localBranches, keying by branch.name and showing
branch.isCurrent) and ensure the select's disabled prop or the "From primary"
option is toggled based on localBranches.length === 0.
In `@apps/desktop/src/renderer/components/lanes/LaneDialogShell.tsx`:
- Line 27: The dialog currently lets users dismiss it via overlay/Escape even
when busy because onOpenChange is passed through directly; wrap or replace the
onOpenChange passed to Dialog.Root in LaneDialogShell with a handler that
ignores close attempts while busy (allow opening when nextOpen is true, but if
nextOpen is false and busy === true, do not call the original onOpenChange), and
ensure any close button still respects the busy state; reference Dialog.Root,
the onOpenChange prop and the LaneDialogShell component to locate where to add
this conditional handler.
In `@apps/desktop/src/renderer/components/lanes/LaneWorkPane.tsx`:
- Around line 26-27: The inline creation of laneList ([work.lane]) creates a new
array each render causing unnecessary re-renders of WorkViewArea; import useMemo
and replace the inline array with a memoized value, e.g. const laneList =
useMemo(() => (work.lane ? [work.lane] : []), [work.lane]), so laneList only
changes when work.lane changes; update the imports to include useMemo and ensure
WorkViewArea receives the memoized laneList.
In `@apps/desktop/src/renderer/components/lanes/useLaneWorkSessions.ts`:
- Around line 57-61: The scope key computed in useLaneWorkSessions duplicates
the composite key logic from appStore; update useLaneWorkSessions to reuse the
canonical normalizer instead of reimplementing it by importing and calling
normalizeLaneWorkScopeKey (or by using appStore.getLaneWorkViewState which
already normalizes) when computing scopeKey, replacing the inline useMemo logic
that references projectRoot and laneId with a call to
normalizeLaneWorkScopeKey(projectRoot, laneId) (or the equivalent store call) so
key format is centralized and changes only need to be made in appStore.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 01bbee69-72c2-447a-82cf-9b621576fb0e
📒 Files selected for processing (7)
apps/desktop/src/renderer/components/lanes/AttachLaneDialog.tsxapps/desktop/src/renderer/components/lanes/CreateLaneDialog.tsxapps/desktop/src/renderer/components/lanes/LaneDialogShell.tsxapps/desktop/src/renderer/components/lanes/LaneWorkPane.tsxapps/desktop/src/renderer/components/lanes/LanesPage.tsxapps/desktop/src/renderer/components/lanes/useLaneWorkSessions.tsapps/desktop/src/renderer/state/appStore.ts
arul28
force-pushed
the
ade/fix-lanes-tab-374ebbe8
branch
from
acd72f3 to
0ef30a0
Compare
arul28
force-pushed
the
ade/fix-lanes-tab-374ebbe8
branch
from
0ef30a0 to
63f7edb
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/desktop/src/renderer/components/lanes/useLaneWorkSessions.ts (1)
119-131:⚠️ Potential issue | 🟠 MajorBackground refresh timer not cleared when
laneIdchanges, causing potential stale data fetch.When
laneIdchanges, the pending timer fromscheduleBackgroundRefreshmay fire with the oldrefreshclosure referencing the previouslaneId. The cleanup effect at lines 125-131 only runs on unmount, not onlaneIdchange.Clear the timer when
laneIdchanges to prevent stale refreshes:Proposed fix
useEffect(() => { setSessions([]); + if (backgroundRefreshTimerRef.current != null) { + window.clearTimeout(backgroundRefreshTimerRef.current); + backgroundRefreshTimerRef.current = null; + } if (!laneId) return; void refresh({ showLoading: true, force: true }); }, [laneId, refresh]); - -useEffect(() => { - return () => { - if (backgroundRefreshTimerRef.current != null) { - window.clearTimeout(backgroundRefreshTimerRef.current); - } - }; -}, []); + +useEffect(() => { + return () => { + if (backgroundRefreshTimerRef.current != null) { + window.clearTimeout(backgroundRefreshTimerRef.current); + backgroundRefreshTimerRef.current = null; + } + }; +}, [laneId]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/lanes/useLaneWorkSessions.ts` around lines 119 - 131, When laneId changes we must clear any pending background refresh to avoid firing the old refresh closure; update the effect that runs on [laneId, refresh] (the one that calls setSessions([]) and void refresh(...)) to first clear backgroundRefreshTimerRef.current (using window.clearTimeout) and set it to null before resetting sessions/refreshing, or alternatively move the timer cleanup into that effect so scheduleBackgroundRefresh's timer is cancelled on laneId change; reference backgroundRefreshTimerRef, scheduleBackgroundRefresh, laneId, refresh, and setSessions when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/desktop/src/renderer/components/lanes/useLaneWorkSessions.ts`:
- Around line 119-131: When laneId changes we must clear any pending background
refresh to avoid firing the old refresh closure; update the effect that runs on
[laneId, refresh] (the one that calls setSessions([]) and void refresh(...)) to
first clear backgroundRefreshTimerRef.current (using window.clearTimeout) and
set it to null before resetting sessions/refreshing, or alternatively move the
timer cleanup into that effect so scheduleBackgroundRefresh's timer is cancelled
on laneId change; reference backgroundRefreshTimerRef,
scheduleBackgroundRefresh, laneId, refresh, and setSessions when making the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 080d1dbf-c12f-4405-8867-c51581747df2
📒 Files selected for processing (7)
apps/desktop/src/renderer/components/lanes/AttachLaneDialog.tsxapps/desktop/src/renderer/components/lanes/CreateLaneDialog.tsxapps/desktop/src/renderer/components/lanes/LaneDialogShell.tsxapps/desktop/src/renderer/components/lanes/LaneWorkPane.tsxapps/desktop/src/renderer/components/lanes/LanesPage.tsxapps/desktop/src/renderer/components/lanes/useLaneWorkSessions.tsapps/desktop/src/renderer/state/appStore.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/renderer/components/lanes/AttachLaneDialog.tsx
- apps/desktop/src/renderer/state/appStore.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/desktop/src/renderer/components/lanes/useLaneWorkSessions.ts (1)
134-155: Inconsistent error handling in cleanup functions.The PTY exit listener cleanup (lines 140-144) wraps
unsubscribe()in a try-catch, while the agent chat listener cleanup (line 154) does not. Consider applying consistent error handling:Proposed fix for consistency
useEffect(() => { const unsubscribe = window.ade.agentChat.onEvent((payload) => { if (!laneId) return; if (!shouldRefreshSessionListForChatEvent(payload)) return; scheduleBackgroundRefresh(180); }); - return unsubscribe; + return () => { + try { + unsubscribe(); + } catch { + // ignore + } + }; }, [laneId, scheduleBackgroundRefresh]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/lanes/useLaneWorkSessions.ts` around lines 134 - 155, The cleanup for the agent chat listener should mirror the PTY exit listener's defensive pattern: instead of returning unsubscribe directly from the useEffect that registers window.ade.agentChat.onEvent, wrap the unsubscribe call in a try-catch and return a function that calls unsubscribe() inside the try-catch (similar to the cleanup for window.ade.pty.onExit) so any errors thrown during unsubscribe are ignored and handling is consistent; update the useEffect that registers window.ade.agentChat.onEvent and its returned cleanup accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/renderer/components/lanes/useLaneWorkSessions.ts`:
- Around line 80-109: The refresh function currently lets errors from
listSessionsCached bubble up and become unhandled; wrap the await call in a
try/catch so failures are caught, log or surface the error (e.g., console.error
or a toast) and avoid throwing from refresh, while keeping the existing finally
block that clears setLoading, refreshInFlightRef.current and processes
refreshQueuedRef.current; update the code in useLaneWorkSessions.ts inside the
refresh callback (the block that calls listSessionsCached and calls
setSessions/filter with isRunOwnedSession) to catch errors, handle/log them, and
not rethrow so queued or effect-invoked void refresh(...) calls cannot produce
unhandled promise rejections.
---
Nitpick comments:
In `@apps/desktop/src/renderer/components/lanes/useLaneWorkSessions.ts`:
- Around line 134-155: The cleanup for the agent chat listener should mirror the
PTY exit listener's defensive pattern: instead of returning unsubscribe directly
from the useEffect that registers window.ade.agentChat.onEvent, wrap the
unsubscribe call in a try-catch and return a function that calls unsubscribe()
inside the try-catch (similar to the cleanup for window.ade.pty.onExit) so any
errors thrown during unsubscribe are ignored and handling is consistent; update
the useEffect that registers window.ade.agentChat.onEvent and its returned
cleanup accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2d0970ef-bcf5-4fd5-b9ff-2802315255e0
📒 Files selected for processing (2)
apps/desktop/src/renderer/components/lanes/LaneDialogShell.tsxapps/desktop/src/renderer/components/lanes/useLaneWorkSessions.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/components/lanes/LaneDialogShell.tsx
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
apps/desktop/src/main/services/lanes/laneService.test.ts (1)
162-162: MockrunGitOrThrow()with its real return type.
fetchRemoteTrackingBranch()ignores the return value today, butrunGitOrThrow()resolvesstdoutas a string. Returning a git-result object viaas anyweakens this test and can hide future regressions if that helper starts using the real payload.Suggested fix
- vi.mocked(runGitOrThrow).mockResolvedValue({ exitCode: 0, stdout: "", stderr: "" } as any); + vi.mocked(runGitOrThrow).mockResolvedValue("");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/lanes/laneService.test.ts` at line 162, The test currently stubs runGitOrThrow using a loose any cast which hides type issues; update the mock to return the actual git result type used by runGitOrThrow (e.g., GitResult/GitRunResult or the exported return type) so stdout is a string and types are enforced: change vi.mocked(runGitOrThrow).mockResolvedValue({ exitCode: 0, stdout: "", stderr: "" } as any) to mockResolvedValue(...) with the concrete return type (import the type or construct an object matching the real shape) so fetchRemoteTrackingBranch and any future consumers see the correct typed payload.apps/desktop/src/renderer/components/prs/tabs/NormalTab.tsx (1)
271-274: Pass the badge color into the spinner.This stays aligned today only because
PrCiRunningIndicatordefaults to the same warning color asgetPrChecksBadge("pending"). Passingcc.colorhere keeps the two visuals from drifting later.♻️ Suggested cleanup
<span style={{ display: "inline-flex", alignItems: "center", gap: 4 }}> <InlinePrBadge {...cc} /> - {pr.checksStatus === "pending" ? <PrCiRunningIndicator /> : null} + {pr.checksStatus === "pending" ? <PrCiRunningIndicator color={cc.color} /> : null} </span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/prs/tabs/NormalTab.tsx` around lines 271 - 274, The badge color should be forwarded to the spinner so visuals stay consistent: update the InlinePrBadge/PrCiRunningIndicator usage so PrCiRunningIndicator receives the same color used by the badge (e.g., pass cc.color or the result of getPrChecksBadge("pending").color) when pr.checksStatus === "pending"; modify the PrCiRunningIndicator prop signature if needed to accept a color prop and use it for the spinner styling so both InlinePrBadge and PrCiRunningIndicator use the same color source.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/services/conflicts/conflictService.ts`:
- Around line 273-301: resolveLaneRebaseTarget currently always prefers
parent.branchRef for stacked lanes which diverges from the primary-parent logic
in laneService (which prefers upstream/origin refs via an async resolver);
update resolveLaneRebaseTarget to reuse the same async resolver used by
laneService (or extract a shared resolver function) so primary parents resolve
to @{upstream}/origin/<branch> when appropriate, ensuring
resolveLaneRebaseTarget (and its return values comparisonRef/displayBaseBranch)
call the shared async routine for parent lookup instead of unconditionally using
parent.branchRef.
In `@apps/desktop/src/main/services/prs/prPollingService.ts`:
- Around line 39-42: The notification title "Ready to merge" is too definitive
for the "merge_ready" path (derived from open + passing checks + approved
review); update the returned title string in the notification object to a
cautious/accurate phrase (e.g., "Checks passing and approved" or "Merge
candidate — checks passing and approved") so it no longer implies the PR is
actually mergeable, and ensure any other places that build or display the
"merge_ready" notification use the new, weaker title (references: the returned
object with title/message and the "merge_ready" path logic).
In `@apps/desktop/src/renderer/components/app/AppShell.tsx`:
- Around line 735-754: The CTA handlers for the ADE and GitHub buttons (the
onClick that calls selectLane/setLaneInspectorTab/setPrToasts and the onClick
that calls window.ade.prs.openInGitHub/setPrToasts) only remove the toast from
state but leave the timeout and toastTimersRef entry active; create and use a
shared dismissPrToast(id: string) helper that calls setPrToasts(prev =>
prev.filter(...)), looks up the timer from toastTimersRef.current.get(id),
clears it with window.clearTimeout if present, and deletes the key from
toastTimersRef.current, then replace both CTA handlers to call
dismissPrToast(toast.id) after their existing actions so timers are cleaned up
consistently.
In `@apps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsx`:
- Around line 1107-1111: Reset the allowBlockedMerge flag whenever the user
selects a different PR by adding an effect that calls
setAllowBlockedMerge(false) when the active PR identifier changes (e.g., watch
the prop/state that represents the currently selected PR such as selectedPr, pr,
or prNumber); this ensures allowBlockedMerge is not carried over between PRs in
addition to the existing logic that clears it when canAttemptBlockedMerge
becomes false. Reference the allowBlockedMerge state and setAllowBlockedMerge
setter in your change and ensure the same reset is applied where similar state
is managed (the other occurrence around the code marked 1152-1156).
- Around line 1434-1454: The UI shows a spinner for the "No checks" case because
the existing ternaries for icon/titleAccessory and description don't treat
checks.length === 0 as a distinct non-running state; update the conditional
logic in PrDetailPane so the "no checks" branch short-circuits spinner/warning
states: change uses of checksRunning (and the icon/titleAccessory selection) to
require checks.length > 0 (e.g., use checksRunning && checks.length > 0) and add
an explicit branch for checks.length === 0 in the icon/titleAccessory logic so
no running spinner is rendered when checks is empty; adjust description/title
branches accordingly so the "No checks" UI is consistent.
In `@apps/desktop/src/renderer/components/prs/state/PrsContext.tsx`:
- Around line 199-202: The refs rebaseNeedsRef and autoRebaseStatusesRef are
being assigned on every render (rebaseNeedsRef.current = rebaseNeeds and
autoRebaseStatusesRef.current = autoRebaseStatuses) which can expose uncommitted
render state to async handlers; instead, remove those inline assignments and
update the refs inside a React.useEffect that runs after commit with
dependencies [rebaseNeeds] and [autoRebaseStatuses] (or a single effect with
both deps), so rebaseNeedsRef.current and autoRebaseStatusesRef.current are only
changed post-commit and async fallback handlers read stable values.
---
Nitpick comments:
In `@apps/desktop/src/main/services/lanes/laneService.test.ts`:
- Line 162: The test currently stubs runGitOrThrow using a loose any cast which
hides type issues; update the mock to return the actual git result type used by
runGitOrThrow (e.g., GitResult/GitRunResult or the exported return type) so
stdout is a string and types are enforced: change
vi.mocked(runGitOrThrow).mockResolvedValue({ exitCode: 0, stdout: "", stderr: ""
} as any) to mockResolvedValue(...) with the concrete return type (import the
type or construct an object matching the real shape) so
fetchRemoteTrackingBranch and any future consumers see the correct typed
payload.
In `@apps/desktop/src/renderer/components/prs/tabs/NormalTab.tsx`:
- Around line 271-274: The badge color should be forwarded to the spinner so
visuals stay consistent: update the InlinePrBadge/PrCiRunningIndicator usage so
PrCiRunningIndicator receives the same color used by the badge (e.g., pass
cc.color or the result of getPrChecksBadge("pending").color) when
pr.checksStatus === "pending"; modify the PrCiRunningIndicator prop signature if
needed to accept a color prop and use it for the spinner styling so both
InlinePrBadge and PrCiRunningIndicator use the same color source.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bcc8f00e-ccf7-44e4-abe5-8c47b0561cac
📒 Files selected for processing (17)
apps/desktop/src/main/services/conflicts/conflictService.test.tsapps/desktop/src/main/services/conflicts/conflictService.tsapps/desktop/src/main/services/lanes/laneService.test.tsapps/desktop/src/main/services/lanes/laneService.tsapps/desktop/src/main/services/prs/prPollingService.test.tsapps/desktop/src/main/services/prs/prPollingService.tsapps/desktop/src/renderer/components/app/AppShell.tsxapps/desktop/src/renderer/components/prs/detail/PrDetailPane.issueResolver.test.tsxapps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsxapps/desktop/src/renderer/components/prs/shared/prVisuals.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/prs/tabs/NormalTab.tsxapps/desktop/src/renderer/components/prs/tabs/queueWorkflowModel.test.tsapps/desktop/src/renderer/components/prs/tabs/queueWorkflowModel.tsapps/desktop/src/shared/types/prs.ts
✅ Files skipped from review due to trivial changes (2)
- apps/desktop/src/renderer/components/prs/tabs/queueWorkflowModel.test.ts
- apps/desktop/src/renderer/components/prs/tabs/queueWorkflowModel.ts
apps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsx
Outdated
Show resolved
Hide resolved
- Widen LaneDialogShell icon prop to accept size as string | number, matching Phosphor icon types - Clear pending background refresh timer when laneId changes to prevent stale data fetches from old closures Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
arul28
force-pushed
the
ade/fix-lanes-tab-374ebbe8
branch
from
8d25736 to
902c990
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
apps/desktop/src/main/services/conflicts/conflictService.ts (1)
273-305:⚠️ Potential issue | 🟠 MajorUse the same async primary-parent rebase-target resolver as
laneService(with local fallback).Line 294 still hardcodes
origin/${parentBranchRef}for primary parents. That keepsconflictServicebehavior divergent from the async resolver logic inapps/desktop/src/main/services/lanes/laneService.tsand can silently suppress valid rebase needs when that remote-tracking ref is missing/stale (downstream impact:scanRebaseNeeds,getRebaseNeed,rebaseLane).🔧 Suggested fix
-function resolveLaneRebaseTarget(args: { +async function resolveLaneRebaseTarget(args: { lane: LaneSummary; lanesById: Map<string, LaneSummary>; queueOverride: QueueRebaseOverride | null; -}): { + projectRoot: string; +}): Promise<{ comparisonRef: string; displayBaseBranch: string; -} { +}> { if (args.queueOverride) { return { comparisonRef: args.queueOverride.comparisonRef, displayBaseBranch: args.queueOverride.displayBaseBranch, }; } const parent = args.lane.parentLaneId ? args.lanesById.get(args.lane.parentLaneId) ?? null : null; const parentBranchRef = parent?.branchRef?.trim() ?? ""; if (parentBranchRef) { - const comparisonRef = parent?.laneType === "primary" ? `origin/${parentBranchRef}` : parentBranchRef; + let comparisonRef = parentBranchRef; + if (parent?.laneType === "primary") { + const upstreamRes = await runGit( + ["rev-parse", "--abbrev-ref", `${parentBranchRef}@{upstream}`], + { cwd: args.projectRoot, timeoutMs: 10_000 } + ); + const upstream = upstreamRes.exitCode === 0 ? upstreamRes.stdout.trim() : ""; + comparisonRef = upstream || parentBranchRef; + } return { comparisonRef, displayBaseBranch: parentBranchRef, }; } return { comparisonRef: args.lane.baseRef, displayBaseBranch: args.lane.baseRef, }; }-const { comparisonRef, displayBaseBranch } = resolveLaneRebaseTarget({ +const { comparisonRef, displayBaseBranch } = await resolveLaneRebaseTarget({ lane, lanesById, queueOverride, + projectRoot, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/conflicts/conflictService.ts` around lines 273 - 305, The resolveLaneRebaseTarget function currently hardcodes origin/<branch> for primary parents; replace that logic with the same async primary-parent resolver used by laneService (call or reuse laneService.resolveParentRebaseTarget) so it attempts to resolve the remote-tracking ref and falls back to the local branch when the remote-tracking ref is missing/stale; update resolveLaneRebaseTarget (and its handling of QueueRebaseOverride, LaneSummary and QueueRebaseOverride inputs) to await/use laneService.resolveParentRebaseTarget for primary parents and use its fallback value as comparisonRef while still returning displayBaseBranch as the local parentBranchRef.
🧹 Nitpick comments (4)
apps/desktop/src/renderer/components/lanes/useLaneWorkSessions.ts (1)
150-157: Consider adding try/catch around unsubscribe for consistency.The PTY exit subscription at lines 141-146 wraps
unsubscribe()in a try/catch, but this chat event subscription returnsunsubscribedirectly. For defensive consistency:useEffect(() => { const unsubscribe = window.ade.agentChat.onEvent((payload) => { if (!laneId) return; if (!shouldRefreshSessionListForChatEvent(payload)) return; scheduleBackgroundRefresh(180); }); - return unsubscribe; + return () => { + try { + unsubscribe(); + } catch { + // ignore + } + }; }, [laneId, scheduleBackgroundRefresh]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/lanes/useLaneWorkSessions.ts` around lines 150 - 157, The cleanup returned from the useEffect that subscribes via window.ade.agentChat.onEvent should mirror the PTY exit handler by invoking unsubscribe inside a try/catch: replace returning the unsubscribe function directly with a cleanup function that checks for unsubscribe and calls it in a try/catch to swallow/log errors; update the effect that references laneId, scheduleBackgroundRefresh, and shouldRefreshSessionListForChatEvent to use this safe unsubscribe pattern so any exception during teardown is handled consistently.apps/desktop/src/renderer/components/lanes/CreateLaneDialog.tsx (2)
228-232: Error display styling differs fromAttachLaneDialog.
AttachLaneDialogincludes aWarningCircleicon in the error container (lines 79-82), but this dialog renders plain text. Consider aligning the error presentation for consistency.♻️ Suggested alignment
{error ? ( - <div className="rounded-xl border border-red-500/25 bg-red-500/10 px-3 py-2 text-sm text-red-200"> - {error} + <div className="flex items-start gap-2 rounded-xl border border-red-500/25 bg-red-500/10 px-3 py-2 text-sm text-red-200"> + <WarningCircle size={16} className="mt-0.5 shrink-0" /> + <span>{error}</span> </div> ) : null}Don't forget to add
WarningCircleto the import from@phosphor-icons/react.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/lanes/CreateLaneDialog.tsx` around lines 228 - 232, The error UI in CreateLaneDialog currently renders plain text while AttachLaneDialog uses a WarningCircle icon; update CreateLaneDialog to mirror AttachLaneDialog by importing WarningCircle from `@phosphor-icons/react` and rendering it alongside the {error} inside the same styled container (the rounded-xl border bg-red... div) so the visual presentation matches; locate the error block in CreateLaneDialog and add the icon element before the error text, matching the spacing and semantics used in AttachLaneDialog.
234-247: Consider whether to resetselectedTemplateIdon cancel.The cancel button resets lane name, parent lane, child mode, and base branch, but preserves the selected template. If this is intentional (preserving user preference), it's fine. Otherwise, consider clearing it for a full reset.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/lanes/CreateLaneDialog.tsx` around lines 234 - 247, The Cancel handler in CreateLaneDialog currently clears create lane fields but leaves the selected template intact; update the onClick in the Cancel Button to also reset the selected template state (call the setter for selectedTemplateId, e.g., setSelectedTemplateId("") or null) alongside setCreateLaneName, setCreateParentLaneId, setCreateAsChild, and setCreateBaseBranch so the dialog fully resets when cancelled; if preserving the template was intentional, leave as-is but document that behavior in CreateLaneDialog.apps/desktop/src/renderer/components/lanes/AttachLaneDialog.tsx (1)
5-8: Consider extracting shared styling constants.
SECTION_CLASS_NAME,LABEL_CLASS_NAME, andINPUT_CLASS_NAMEare duplicated inCreateLaneDialog.tsx(lines 8-13). Extract these to a shared module (e.g.,laneDialogStyles.ts) to maintain consistency and reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/lanes/AttachLaneDialog.tsx` around lines 5 - 8, SECTION_CLASS_NAME, LABEL_CLASS_NAME and INPUT_CLASS_NAME are duplicated across AttachLaneDialog and CreateLaneDialog; extract these constants into a new shared module (e.g., laneDialogStyles) exporting the three constants, then replace the local constants in AttachLaneDialog (and CreateLaneDialog) with imports from that module; update import statements and ensure exported names match the original symbols (SECTION_CLASS_NAME, LABEL_CLASS_NAME, INPUT_CLASS_NAME) so existing references in render methods remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/services/conflicts/conflictService.test.ts`:
- Around line 956-1025: The test can false-pass because primary parent lane
resolution may try origin/<branch> which the fixture never creates; update the
fixture in this test (around parentLane and the repo setup before
createConflictService) to create a remote-tracking ref for
feature/parent-current (e.g. add an "origin" remote and push or create
refs/remotes/origin/feature/parent-current pointing at the branch commit) so
ref-resolution exercises the intended code paths used by scanRebaseNeeds() and
getRebaseNeed("lane-child") instead of hitting missing-ref fallback behavior.
In `@apps/desktop/src/renderer/components/app/AppShell.tsx`:
- Around line 717-726: The meta chips are being assigned icons by array index
which breaks when getPrToastMeta() filters/deduplicates items; update
prToastPresentation/getPrToastMeta to return structured items (e.g., { kind:
'lane'|'branch'|'repo', label: string }) or otherwise preserve the meta kind
alongside the label, then update the mapping in AppShell.tsx (the meta variable
and the map callback) to select the icon based on item.kind (or the returned
discriminant) instead of index; ensure keys still use toast.id plus index or
label to remain unique.
- Around line 703-715: The dismiss button for PR toasts lacks an explicit
accessible name and relies on title, so screen readers may announce only “×”;
update the button element (the button with onClick that calls setPrToasts and
uses toastTimersRef and toast.id) to include a descriptive aria-label such as
aria-label="Dismiss PR notification" (or include toast-specific text) to provide
a proper accessible name for assistive technologies.
- Around line 748-760: The button handler dismisses the toast immediately and
swallows errors from window.ade.prs.openInGitHub; change it to await
window.ade.prs.openInGitHub(toast.event.prId) inside a try/catch, only call
setPrToasts and clear toastTimersRef.current.delete(toast.id) when the await
succeeds, and in the catch log or report the error so the toast remains for
retry; reference the existing symbols window.ade.prs.openInGitHub,
toast.event.prId, setPrToasts, toastTimersRef, and toast.id when making the
change.
In `@apps/desktop/src/renderer/components/lanes/CreateLaneDialog.tsx`:
- Around line 248-255: The submit button currently only disables when creating
as a child and no parent is selected, allowing submission with an empty
createBaseBranch when creating from primary; update the Button disabled logic in
CreateLaneDialog (the disabled prop where busy, createAsChild,
createParentLaneId are checked) to also require a non-empty createBaseBranch
when createAsChild is false (e.g., add a check like !createAsChild &&
!createBaseBranch.trim().length), and also add the same validation inside
onSubmit (or the submit handler used by buttonLabel/onSubmit) to guard against
programmatic submission by returning early or showing an error if
createBaseBranch is empty.
- Around line 206-218: The select for choosing createBaseBranch in
CreateLaneDialog.tsx can be empty when localBranches is [], so update the
CreateLaneDialog component to handle that case by either (a) rendering a
disabled fallback <option> like "No local branches available" and keeping the
select disabled when busy or localBranches.length === 0, or (b) disable the
entire "From primary" mode (where createBaseBranch is used) and show a notice
when localBranches is empty; ensure you reference the createBaseBranch state,
the localBranches array and the select rendering block to implement the
conditional rendering/disabled logic and preserve existing busy handling.
In `@apps/desktop/src/renderer/components/lanes/LaneDialogShell.tsx`:
- Around line 30-54: Dialog can still be closed via the Escape key even when
busy is true; add an onEscapeKeyDown handler on the Dialog.Content (or the
Dialog root if you prefer) that checks the busy flag and, when busy, calls
event.preventDefault() (and event.stopPropagation() if needed) to prevent the
dialog from closing; update the Dialog.Content declaration in
LaneDialogShell.tsx where busy is in scope so the Esc button and Escape-key
behavior are consistent.
In `@apps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsx`:
- Around line 309-321: The readiness logic in summarizeChecks lets completed
non-success conclusions show as "all checks passed"; update summarizeChecks so
allChecksPassed requires every check to be completed with conclusion ===
"success" (e.g., use checks.every to verify c.status === "completed" &&
c.conclusion === "success"), compute pending as checks with status !==
"completed", and set someChecksFailing to true if any completed check has a
conclusion !== "success" (not just explicit "failure"); keep checksRunning based
on pending > 0 and total as checks.length so other consumers like the
merge-readiness row reflect true non-success completed checks.
---
Duplicate comments:
In `@apps/desktop/src/main/services/conflicts/conflictService.ts`:
- Around line 273-305: The resolveLaneRebaseTarget function currently hardcodes
origin/<branch> for primary parents; replace that logic with the same async
primary-parent resolver used by laneService (call or reuse
laneService.resolveParentRebaseTarget) so it attempts to resolve the
remote-tracking ref and falls back to the local branch when the remote-tracking
ref is missing/stale; update resolveLaneRebaseTarget (and its handling of
QueueRebaseOverride, LaneSummary and QueueRebaseOverride inputs) to await/use
laneService.resolveParentRebaseTarget for primary parents and use its fallback
value as comparisonRef while still returning displayBaseBranch as the local
parentBranchRef.
---
Nitpick comments:
In `@apps/desktop/src/renderer/components/lanes/AttachLaneDialog.tsx`:
- Around line 5-8: SECTION_CLASS_NAME, LABEL_CLASS_NAME and INPUT_CLASS_NAME are
duplicated across AttachLaneDialog and CreateLaneDialog; extract these constants
into a new shared module (e.g., laneDialogStyles) exporting the three constants,
then replace the local constants in AttachLaneDialog (and CreateLaneDialog) with
imports from that module; update import statements and ensure exported names
match the original symbols (SECTION_CLASS_NAME, LABEL_CLASS_NAME,
INPUT_CLASS_NAME) so existing references in render methods remain unchanged.
In `@apps/desktop/src/renderer/components/lanes/CreateLaneDialog.tsx`:
- Around line 228-232: The error UI in CreateLaneDialog currently renders plain
text while AttachLaneDialog uses a WarningCircle icon; update CreateLaneDialog
to mirror AttachLaneDialog by importing WarningCircle from `@phosphor-icons/react`
and rendering it alongside the {error} inside the same styled container (the
rounded-xl border bg-red... div) so the visual presentation matches; locate the
error block in CreateLaneDialog and add the icon element before the error text,
matching the spacing and semantics used in AttachLaneDialog.
- Around line 234-247: The Cancel handler in CreateLaneDialog currently clears
create lane fields but leaves the selected template intact; update the onClick
in the Cancel Button to also reset the selected template state (call the setter
for selectedTemplateId, e.g., setSelectedTemplateId("") or null) alongside
setCreateLaneName, setCreateParentLaneId, setCreateAsChild, and
setCreateBaseBranch so the dialog fully resets when cancelled; if preserving the
template was intentional, leave as-is but document that behavior in
CreateLaneDialog.
In `@apps/desktop/src/renderer/components/lanes/useLaneWorkSessions.ts`:
- Around line 150-157: The cleanup returned from the useEffect that subscribes
via window.ade.agentChat.onEvent should mirror the PTY exit handler by invoking
unsubscribe inside a try/catch: replace returning the unsubscribe function
directly with a cleanup function that checks for unsubscribe and calls it in a
try/catch to swallow/log errors; update the effect that references laneId,
scheduleBackgroundRefresh, and shouldRefreshSessionListForChatEvent to use this
safe unsubscribe pattern so any exception during teardown is handled
consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f891ae67-600a-480a-9218-1de7b5e8022e
📒 Files selected for processing (24)
apps/desktop/src/main/services/conflicts/conflictService.test.tsapps/desktop/src/main/services/conflicts/conflictService.tsapps/desktop/src/main/services/lanes/laneService.test.tsapps/desktop/src/main/services/lanes/laneService.tsapps/desktop/src/main/services/prs/prPollingService.test.tsapps/desktop/src/main/services/prs/prPollingService.tsapps/desktop/src/renderer/components/app/AppShell.tsxapps/desktop/src/renderer/components/lanes/AttachLaneDialog.tsxapps/desktop/src/renderer/components/lanes/CreateLaneDialog.tsxapps/desktop/src/renderer/components/lanes/LaneDialogShell.tsxapps/desktop/src/renderer/components/lanes/LaneWorkPane.tsxapps/desktop/src/renderer/components/lanes/LanesPage.tsxapps/desktop/src/renderer/components/lanes/useLaneWorkSessions.tsapps/desktop/src/renderer/components/prs/detail/PrDetailPane.issueResolver.test.tsxapps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsxapps/desktop/src/renderer/components/prs/shared/prVisuals.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/prs/tabs/NormalTab.tsxapps/desktop/src/renderer/components/prs/tabs/queueWorkflowModel.test.tsapps/desktop/src/renderer/components/prs/tabs/queueWorkflowModel.tsapps/desktop/src/renderer/state/appStore.tsapps/desktop/src/shared/types/prs.ts
✅ Files skipped from review due to trivial changes (6)
- apps/desktop/src/renderer/components/prs/tabs/queueWorkflowModel.ts
- apps/desktop/src/renderer/components/prs/tabs/queueWorkflowModel.test.ts
- apps/desktop/src/main/services/prs/prPollingService.test.ts
- apps/desktop/src/renderer/components/prs/tabs/NormalTab.tsx
- apps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsx
- apps/desktop/src/renderer/components/lanes/LanesPage.tsx
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/desktop/src/renderer/components/prs/tabs/GitHubTab.test.tsx
- apps/desktop/src/shared/types/prs.ts
- apps/desktop/src/main/services/prs/prPollingService.ts
- apps/desktop/src/renderer/components/prs/shared/prVisuals.tsx
- apps/desktop/src/renderer/components/prs/state/PrsContext.tsx
- apps/desktop/src/main/services/lanes/laneService.ts
- apps/desktop/src/main/services/lanes/laneService.test.ts
Comment on lines
+956
to
+1025
| it("prefers the current parent lane branch when baseRef is stale", async () => { | ||
| const repoRoot = fs.mkdtempSync(path.join(os.tmpdir(), "ade-conflicts-parent-branch-")); | ||
| const db = await openKvDb(path.join(repoRoot, "kv.sqlite"), createLogger()); | ||
| const projectId = randomUUID(); | ||
| const now = "2026-03-24T12:00:00.000Z"; | ||
|
|
||
| try { | ||
| db.run( | ||
| "insert into projects(id, root_path, display_name, default_base_ref, created_at, last_opened_at) values (?, ?, ?, ?, ?, ?)", | ||
| [projectId, repoRoot, "demo", "main", now, now] | ||
| ); | ||
|
|
||
| fs.writeFileSync(path.join(repoRoot, "file.txt"), "base\n", "utf8"); | ||
| git(repoRoot, ["init", "-b", "main"]); | ||
| git(repoRoot, ["config", "user.email", "ade@test.local"]); | ||
| git(repoRoot, ["config", "user.name", "ADE Test"]); | ||
| git(repoRoot, ["add", "."]); | ||
| git(repoRoot, ["commit", "-m", "base"]); | ||
|
|
||
| git(repoRoot, ["checkout", "-b", "feature/parent-current"]); | ||
| git(repoRoot, ["checkout", "-b", "feature/child"]); | ||
| fs.writeFileSync(path.join(repoRoot, "file.txt"), "child\n", "utf8"); | ||
| git(repoRoot, ["add", "file.txt"]); | ||
| git(repoRoot, ["commit", "-m", "child work"]); | ||
|
|
||
| git(repoRoot, ["checkout", "main"]); | ||
| fs.writeFileSync(path.join(repoRoot, "main.txt"), "main advance\n", "utf8"); | ||
| git(repoRoot, ["add", "main.txt"]); | ||
| git(repoRoot, ["commit", "-m", "main advance"]); | ||
| git(repoRoot, ["checkout", "feature/child"]); | ||
|
|
||
| const parentLane = { | ||
| ...createLaneSummary(repoRoot, { | ||
| id: "lane-parent", | ||
| name: "Primary", | ||
| branchRef: "feature/parent-current", | ||
| baseRef: "main", | ||
| parentLaneId: null | ||
| }), | ||
| laneType: "primary" as const, | ||
| }; | ||
| const childLane = createLaneSummary(repoRoot, { | ||
| id: "lane-child", | ||
| name: "Child", | ||
| branchRef: "feature/child", | ||
| baseRef: "main", | ||
| parentLaneId: "lane-parent" | ||
| }); | ||
|
|
||
| const service = createConflictService({ | ||
| db, | ||
| logger: createLogger(), | ||
| projectId, | ||
| projectRoot: repoRoot, | ||
| laneService: { | ||
| list: async () => [parentLane, childLane], | ||
| getLaneBaseAndBranch: () => ({ worktreePath: repoRoot, baseRef: "main", branchRef: "feature/child" }) | ||
| } as any, | ||
| projectConfigService: { | ||
| get: () => ({ effective: { providerMode: "guest" }, local: {} }) | ||
| } as any, | ||
| }); | ||
|
|
||
| expect(await service.scanRebaseNeeds()).toEqual([]); | ||
| expect(await service.getRebaseNeed("lane-child")).toBeNull(); | ||
| } finally { | ||
| db.close(); | ||
| fs.rmSync(repoRoot, { recursive: true, force: true }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Harden this test to avoid false positives from missing primary remote-tracking refs.
Because Line 995 sets the parent as a primary lane, the production path may resolve origin/<branch>. This fixture never creates tracking refs, so the test can pass due to ref-resolution failure paths instead of validating the intended parent-branch behavior.
🧪 Suggested fixture hardening
git(repoRoot, ["init", "-b", "main"]);
git(repoRoot, ["config", "user.email", "ade@test.local"]);
git(repoRoot, ["config", "user.name", "ADE Test"]);
git(repoRoot, ["add", "."]);
git(repoRoot, ["commit", "-m", "base"]);
+ const originRoot = path.join(repoRoot, "..", "origin.git");
+ fs.mkdirSync(originRoot, { recursive: true });
+ git(originRoot, ["init", "--bare"]);
+ git(repoRoot, ["remote", "add", "origin", originRoot]);
+ git(repoRoot, ["push", "-u", "origin", "main"]);
git(repoRoot, ["checkout", "-b", "feature/parent-current"]);
+ git(repoRoot, ["push", "-u", "origin", "feature/parent-current"]);
git(repoRoot, ["checkout", "-b", "feature/child"]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/services/conflicts/conflictService.test.ts` around
lines 956 - 1025, The test can false-pass because primary parent lane resolution
may try origin/<branch> which the fixture never creates; update the fixture in
this test (around parentLane and the repo setup before createConflictService) to
create a remote-tracking ref for feature/parent-current (e.g. add an "origin"
remote and push or create refs/remotes/origin/feature/parent-current pointing at
the branch commit) so ref-resolution exercises the intended code paths used by
scanRebaseNeeds() and getRebaseNeed("lane-child") instead of hitting missing-ref
fallback behavior.
Comment on lines
+703
to
+715
| <button | ||
| type="button" | ||
| className="shrink-0 rounded p-1 text-muted-fg transition-colors hover:bg-fg/[0.05] hover:text-fg" | ||
| onClick={() => { | ||
| setPrToasts((prev) => prev.filter((t) => t.id !== toast.id)); | ||
| const timer = toastTimersRef.current.get(toast.id); | ||
| if (timer != null) window.clearTimeout(timer); | ||
| toastTimersRef.current.delete(toast.id); | ||
| }} | ||
| title="Dismiss" | ||
| > | ||
| × | ||
| </button> |
There was a problem hiding this comment.
Add an explicit accessible name to the dismiss button.
title is not a reliable screen-reader name for an icon-only control, so this will often be announced as just “×”. Please add an aria-label, e.g. aria-label="Dismiss PR notification".
♿ Suggested fix
<button
type="button"
className="shrink-0 rounded p-1 text-muted-fg transition-colors hover:bg-fg/[0.05] hover:text-fg"
+ aria-label="Dismiss PR notification"
onClick={() => {
setPrToasts((prev) => prev.filter((t) => t.id !== toast.id));
const timer = toastTimersRef.current.get(toast.id);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <button | |
| type="button" | |
| className="shrink-0 rounded p-1 text-muted-fg transition-colors hover:bg-fg/[0.05] hover:text-fg" | |
| onClick={() => { | |
| setPrToasts((prev) => prev.filter((t) => t.id !== toast.id)); | |
| const timer = toastTimersRef.current.get(toast.id); | |
| if (timer != null) window.clearTimeout(timer); | |
| toastTimersRef.current.delete(toast.id); | |
| }} | |
| title="Dismiss" | |
| > | |
| × | |
| </button> | |
| <button | |
| type="button" | |
| className="shrink-0 rounded p-1 text-muted-fg transition-colors hover:bg-fg/[0.05] hover:text-fg" | |
| aria-label="Dismiss PR notification" | |
| onClick={() => { | |
| setPrToasts((prev) => prev.filter((t) => t.id !== toast.id)); | |
| const timer = toastTimersRef.current.get(toast.id); | |
| if (timer != null) window.clearTimeout(timer); | |
| toastTimersRef.current.delete(toast.id); | |
| }} | |
| title="Dismiss" | |
| > | |
| × | |
| </button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/app/AppShell.tsx` around lines 703 -
715, The dismiss button for PR toasts lacks an explicit accessible name and
relies on title, so screen readers may announce only “×”; update the button
element (the button with onClick that calls setPrToasts and uses toastTimersRef
and toast.id) to include a descriptive aria-label such as aria-label="Dismiss PR
notification" (or include toast-specific text) to provide a proper accessible
name for assistive technologies.
Comment on lines
+717
to
+726
| {meta.length > 0 ? ( | ||
| <div className="mt-2 flex flex-wrap items-center gap-1.5"> | ||
| {meta.map((item, index) => ( | ||
| <span | ||
| key={`${toast.id}-meta-${index}`} | ||
| className="inline-flex max-w-full items-center gap-1 rounded-full border border-border/50 bg-black/10 px-2 py-1 text-[10px] text-muted-fg" | ||
| > | ||
| {index === 0 ? <GitPullRequest size={10} /> : index === 1 ? <GitBranch size={10} /> : <GithubLogo size={10} />} | ||
| <span className="truncate">{item}</span> | ||
| </span> |
There was a problem hiding this comment.
Don't choose meta icons by array position.
getPrToastMeta() returns a filtered/de-duplicated string[], so index 0 is not guaranteed to be the lane and index 1 is not guaranteed to be the branch. When a label is missing, the remaining chips shift and render with the wrong icon. Preserve the icon/kind alongside each label before filtering, or return structured meta items from prToastPresentation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/app/AppShell.tsx` around lines 717 -
726, The meta chips are being assigned icons by array index which breaks when
getPrToastMeta() filters/deduplicates items; update
prToastPresentation/getPrToastMeta to return structured items (e.g., { kind:
'lane'|'branch'|'repo', label: string }) or otherwise preserve the meta kind
alongside the label, then update the mapping in AppShell.tsx (the meta variable
and the map callback) to select the icon based on item.kind (or the returned
discriminant) instead of index; ensure keys still use toast.id plus index or
label to remain unique.
Comment on lines
+748
to
+760
| <button | ||
| type="button" | ||
| className={cn( | ||
| "inline-flex h-8 items-center gap-1.5 rounded-md px-3 text-[11px] font-medium text-[#0F0D14] transition-colors hover:brightness-110", | ||
| tone === "danger" ? "bg-red-300" : tone === "warning" ? "bg-amber-300" : tone === "success" ? "bg-emerald-300" : "bg-[#A78BFA]", | ||
| )} | ||
| onClick={() => { | ||
| void window.ade.prs.openInGitHub(toast.event.prId).catch(() => { }); | ||
| setPrToasts((prev) => prev.filter((t) => t.id !== toast.id)); | ||
| const timer = toastTimersRef.current.get(toast.id); | ||
| if (timer != null) window.clearTimeout(timer); | ||
| toastTimersRef.current.delete(toast.id); | ||
| }} |
There was a problem hiding this comment.
Keep the toast visible when openInGitHub fails.
This dismisses the toast before the IPC call completes and then swallows any rejection. The main handler can throw when the PR row is missing, so users lose both feedback and the retry affordance. Await the call, dismiss on success, and surface/log the failure path.
♻️ Suggested fix
- <button
+ <button
type="button"
className={cn(
"inline-flex h-8 items-center gap-1.5 rounded-md px-3 text-[11px] font-medium text-[`#0F0D14`] transition-colors hover:brightness-110",
tone === "danger" ? "bg-red-300" : tone === "warning" ? "bg-amber-300" : tone === "success" ? "bg-emerald-300" : "bg-[`#A78BFA`]",
)}
- onClick={() => {
- void window.ade.prs.openInGitHub(toast.event.prId).catch(() => { });
- setPrToasts((prev) => prev.filter((t) => t.id !== toast.id));
- const timer = toastTimersRef.current.get(toast.id);
- if (timer != null) window.clearTimeout(timer);
- toastTimersRef.current.delete(toast.id);
+ onClick={async () => {
+ try {
+ await window.ade.prs.openInGitHub(toast.event.prId);
+ setPrToasts((prev) => prev.filter((t) => t.id !== toast.id));
+ const timer = toastTimersRef.current.get(toast.id);
+ if (timer != null) window.clearTimeout(timer);
+ toastTimersRef.current.delete(toast.id);
+ } catch (error) {
+ console.warn("renderer.pr_toast_open_in_github_failed", {
+ prId: toast.event.prId,
+ error,
+ });
+ }
}}
>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/app/AppShell.tsx` around lines 748 -
760, The button handler dismisses the toast immediately and swallows errors from
window.ade.prs.openInGitHub; change it to await
window.ade.prs.openInGitHub(toast.event.prId) inside a try/catch, only call
setPrToasts and clear toastTimersRef.current.delete(toast.id) when the await
succeeds, and in the catch log or report the error so the toast remains for
retry; reference the existing symbols window.ade.prs.openInGitHub,
toast.event.prId, setPrToasts, toastTimersRef, and toast.id when making the
change.
Comment on lines
+206
to
+218
| <select | ||
| value={createBaseBranch} | ||
| onChange={(event) => setCreateBaseBranch(event.target.value)} | ||
| className={SELECT_CLASS_NAME} | ||
| disabled={busy} | ||
| > | ||
| {localBranches.map((branch) => ( | ||
| <option key={branch.name} value={branch.name}> | ||
| {branch.name} | ||
| {branch.isCurrent ? " (current)" : ""} | ||
| </option> | ||
| ))} | ||
| </select> |
There was a problem hiding this comment.
Handle empty localBranches case.
If all branches in createBranches are remote, localBranches will be empty, leaving the select with no options. Consider adding a fallback option or disabling the "From primary" mode when no local branches exist.
🛠️ Proposed fix
<select
value={createBaseBranch}
onChange={(event) => setCreateBaseBranch(event.target.value)}
className={SELECT_CLASS_NAME}
- disabled={busy}
+ disabled={busy || localBranches.length === 0}
>
+ {localBranches.length === 0 && (
+ <option value="">No local branches available</option>
+ )}
{localBranches.map((branch) => (📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <select | |
| value={createBaseBranch} | |
| onChange={(event) => setCreateBaseBranch(event.target.value)} | |
| className={SELECT_CLASS_NAME} | |
| disabled={busy} | |
| > | |
| {localBranches.map((branch) => ( | |
| <option key={branch.name} value={branch.name}> | |
| {branch.name} | |
| {branch.isCurrent ? " (current)" : ""} | |
| </option> | |
| ))} | |
| </select> | |
| <select | |
| value={createBaseBranch} | |
| onChange={(event) => setCreateBaseBranch(event.target.value)} | |
| className={SELECT_CLASS_NAME} | |
| disabled={busy || localBranches.length === 0} | |
| > | |
| {localBranches.length === 0 && ( | |
| <option value="">No local branches available</option> | |
| )} | |
| {localBranches.map((branch) => ( | |
| <option key={branch.name} value={branch.name}> | |
| {branch.name} | |
| {branch.isCurrent ? " (current)" : ""} | |
| </option> | |
| ))} | |
| </select> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/lanes/CreateLaneDialog.tsx` around lines
206 - 218, The select for choosing createBaseBranch in CreateLaneDialog.tsx can
be empty when localBranches is [], so update the CreateLaneDialog component to
handle that case by either (a) rendering a disabled fallback <option> like "No
local branches available" and keeping the select disabled when busy or
localBranches.length === 0, or (b) disable the entire "From primary" mode (where
createBaseBranch is used) and show a notice when localBranches is empty; ensure
you reference the createBaseBranch state, the localBranches array and the select
rendering block to implement the conditional rendering/disabled logic and
preserve existing busy handling.
Comment on lines
+248
to
+255
| <Button | ||
| variant="primary" | ||
| disabled={busy || !createLaneName.trim().length || (createAsChild && !createParentLaneId)} | ||
| onClick={onSubmit} | ||
| > | ||
| {buttonLabel(busy, createAsChild, createParentLaneId, createBaseBranch)} | ||
| </Button> | ||
| </div> |
There was a problem hiding this comment.
Validation may allow empty createBaseBranch when creating from primary.
The submit button is disabled when createAsChild && !createParentLaneId, but when !createAsChild, there's no validation that createBaseBranch is set. If createBaseBranch is initially empty (as shown in LanesPage state initialization), the form could submit with an empty base branch.
🛠️ Proposed fix
<Button
variant="primary"
- disabled={busy || !createLaneName.trim().length || (createAsChild && !createParentLaneId)}
+ disabled={busy || !createLaneName.trim().length || (createAsChild ? !createParentLaneId : !createBaseBranch)}
onClick={onSubmit}
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <Button | |
| variant="primary" | |
| disabled={busy || !createLaneName.trim().length || (createAsChild && !createParentLaneId)} | |
| onClick={onSubmit} | |
| > | |
| {buttonLabel(busy, createAsChild, createParentLaneId, createBaseBranch)} | |
| </Button> | |
| </div> | |
| <Button | |
| variant="primary" | |
| disabled={busy || !createLaneName.trim().length || (createAsChild ? !createParentLaneId : !createBaseBranch)} | |
| onClick={onSubmit} | |
| > | |
| {buttonLabel(busy, createAsChild, createParentLaneId, createBaseBranch)} | |
| </Button> | |
| </div> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/lanes/CreateLaneDialog.tsx` around lines
248 - 255, The submit button currently only disables when creating as a child
and no parent is selected, allowing submission with an empty createBaseBranch
when creating from primary; update the Button disabled logic in CreateLaneDialog
(the disabled prop where busy, createAsChild, createParentLaneId are checked) to
also require a non-empty createBaseBranch when createAsChild is false (e.g., add
a check like !createAsChild && !createBaseBranch.trim().length), and also add
the same validation inside onSubmit (or the submit handler used by
buttonLabel/onSubmit) to guard against programmatic submission by returning
early or showing an error if createBaseBranch is empty.
Comment on lines
+30
to
+54
| <Dialog.Content | ||
| className={`fixed left-1/2 top-[14%] z-50 -translate-x-1/2 rounded-xl border border-white/[0.06] bg-bg/80 p-4 shadow-float backdrop-blur-xl focus:outline-none ${widthClass}`} | ||
| > | ||
| <div className="pointer-events-none absolute inset-x-6 top-0 h-px bg-gradient-to-r from-transparent via-accent/40 to-transparent" /> | ||
| <div className="mb-4 flex items-start justify-between gap-3"> | ||
| <div className="min-w-0"> | ||
| <Dialog.Title className="flex items-center gap-2 text-lg font-semibold text-fg"> | ||
| {Icon ? ( | ||
| <span className="inline-flex h-8 w-8 items-center justify-center rounded-lg border border-white/[0.06] bg-white/[0.03] text-accent"> | ||
| <Icon size={16} /> | ||
| </span> | ||
| ) : null} | ||
| <span className="truncate">{title}</span> | ||
| </Dialog.Title> | ||
| {description ? ( | ||
| <Dialog.Description className="mt-2 max-w-2xl text-sm leading-6 text-muted-fg"> | ||
| {description} | ||
| </Dialog.Description> | ||
| ) : null} | ||
| </div> | ||
| <Dialog.Close asChild> | ||
| <Button variant="ghost" size="sm" disabled={busy}> | ||
| Esc | ||
| </Button> | ||
| </Dialog.Close> |
There was a problem hiding this comment.
Escape key can still close the dialog when busy is true.
The "Esc" button is disabled when busy, but the Radix Dialog will still close on Escape key press by default. This creates inconsistent behavior where users can bypass the busy state via keyboard.
Add onEscapeKeyDown handler to prevent closing when busy:
🛠️ Proposed fix
<Dialog.Content
className={`fixed left-1/2 top-[14%] z-50 -translate-x-1/2 rounded-xl border border-white/[0.06] bg-bg/80 p-4 shadow-float backdrop-blur-xl focus:outline-none ${widthClass}`}
+ onEscapeKeyDown={(e) => {
+ if (busy) e.preventDefault();
+ }}
+ onPointerDownOutside={(e) => {
+ if (busy) e.preventDefault();
+ }}
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <Dialog.Content | |
| className={`fixed left-1/2 top-[14%] z-50 -translate-x-1/2 rounded-xl border border-white/[0.06] bg-bg/80 p-4 shadow-float backdrop-blur-xl focus:outline-none ${widthClass}`} | |
| > | |
| <div className="pointer-events-none absolute inset-x-6 top-0 h-px bg-gradient-to-r from-transparent via-accent/40 to-transparent" /> | |
| <div className="mb-4 flex items-start justify-between gap-3"> | |
| <div className="min-w-0"> | |
| <Dialog.Title className="flex items-center gap-2 text-lg font-semibold text-fg"> | |
| {Icon ? ( | |
| <span className="inline-flex h-8 w-8 items-center justify-center rounded-lg border border-white/[0.06] bg-white/[0.03] text-accent"> | |
| <Icon size={16} /> | |
| </span> | |
| ) : null} | |
| <span className="truncate">{title}</span> | |
| </Dialog.Title> | |
| {description ? ( | |
| <Dialog.Description className="mt-2 max-w-2xl text-sm leading-6 text-muted-fg"> | |
| {description} | |
| </Dialog.Description> | |
| ) : null} | |
| </div> | |
| <Dialog.Close asChild> | |
| <Button variant="ghost" size="sm" disabled={busy}> | |
| Esc | |
| </Button> | |
| </Dialog.Close> | |
| <Dialog.Content | |
| className={`fixed left-1/2 top-[14%] z-50 -translate-x-1/2 rounded-xl border border-white/[0.06] bg-bg/80 p-4 shadow-float backdrop-blur-xl focus:outline-none ${widthClass}`} | |
| onEscapeKeyDown={(e) => { | |
| if (busy) e.preventDefault(); | |
| }} | |
| onPointerDownOutside={(e) => { | |
| if (busy) e.preventDefault(); | |
| }} | |
| > | |
| <div className="pointer-events-none absolute inset-x-6 top-0 h-px bg-gradient-to-r from-transparent via-accent/40 to-transparent" /> | |
| <div className="mb-4 flex items-start justify-between gap-3"> | |
| <div className="min-w-0"> | |
| <Dialog.Title className="flex items-center gap-2 text-lg font-semibold text-fg"> | |
| {Icon ? ( | |
| <span className="inline-flex h-8 w-8 items-center justify-center rounded-lg border border-white/[0.06] bg-white/[0.03] text-accent"> | |
| <Icon size={16} /> | |
| </span> | |
| ) : null} | |
| <span className="truncate">{title}</span> | |
| </Dialog.Title> | |
| {description ? ( | |
| <Dialog.Description className="mt-2 max-w-2xl text-sm leading-6 text-muted-fg"> | |
| {description} | |
| </Dialog.Description> | |
| ) : null} | |
| </div> | |
| <Dialog.Close asChild> | |
| <Button variant="ghost" size="sm" disabled={busy}> | |
| Esc | |
| </Button> | |
| </Dialog.Close> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/lanes/LaneDialogShell.tsx` around lines
30 - 54, Dialog can still be closed via the Escape key even when busy is true;
add an onEscapeKeyDown handler on the Dialog.Content (or the Dialog root if you
prefer) that checks the busy flag and, when busy, calls event.preventDefault()
(and event.stopPropagation() if needed) to prevent the dialog from closing;
update the Dialog.Content declaration in LaneDialogShell.tsx where busy is in
scope so the Esc button and Escape-key behavior are consistent.
Comment on lines
+309
to
+321
| function summarizeChecks(checks: PrCheck[]) { | ||
| const passing = checks.filter((check) => check.conclusion === "success").length; | ||
| const failing = checks.filter((check) => check.conclusion === "failure").length; | ||
| const pending = checks.filter((check) => check.status !== "completed").length; | ||
| return { | ||
| passing, | ||
| failing, | ||
| pending, | ||
| total: checks.length, | ||
| allChecksPassed: checks.length > 0 && failing === 0 && pending === 0, | ||
| someChecksFailing: failing > 0, | ||
| checksRunning: pending > 0, | ||
| }; |
There was a problem hiding this comment.
Don't let completed non-success checks turn the readiness row green.
allChecksPassed currently only excludes explicit "failure" and pending checks. A completed check with any other non-success conclusion will still make the merge-readiness row show “All checks have passed”, which is a false green state.
🩹 Proposed fix
function summarizeChecks(checks: PrCheck[]) {
const passing = checks.filter((check) => check.conclusion === "success").length;
const failing = checks.filter((check) => check.conclusion === "failure").length;
+ const nonPassingCompleted = checks.filter(
+ (check) => check.status === "completed" && check.conclusion !== "success",
+ ).length;
const pending = checks.filter((check) => check.status !== "completed").length;
return {
passing,
failing,
+ nonPassingCompleted,
pending,
total: checks.length,
- allChecksPassed: checks.length > 0 && failing === 0 && pending === 0,
+ allChecksPassed: checks.length > 0 && nonPassingCompleted === 0 && pending === 0,
someChecksFailing: failing > 0,
checksRunning: pending > 0,
};
}- const { passing, pending, total: totalChecks, allChecksPassed, someChecksFailing, checksRunning } = summarizeChecks(checks);
+ const {
+ passing,
+ pending,
+ total: totalChecks,
+ nonPassingCompleted,
+ allChecksPassed,
+ someChecksFailing,
+ checksRunning,
+ } = summarizeChecks(checks);- color={allChecksPassed ? COLORS.success : someChecksFailing ? COLORS.danger : checksRunning ? COLORS.warning : checks.length === 0 ? COLORS.textMuted : COLORS.warning}
+ color={
+ allChecksPassed
+ ? COLORS.success
+ : someChecksFailing
+ ? COLORS.danger
+ : nonPassingCompleted > 0
+ ? COLORS.warning
+ : checksRunning
+ ? COLORS.warning
+ : checks.length === 0
+ ? COLORS.textMuted
+ : COLORS.warning
+ }
icon={
allChecksPassed
? <CheckCircle size={18} weight="fill" style={{ color: COLORS.success, filter: "drop-shadow(0 0 4px rgba(34,197,94,0.4))" }} />
: someChecksFailing
? <XCircle size={18} weight="fill" style={{ color: COLORS.danger, filter: "drop-shadow(0 0 4px rgba(239,68,68,0.4))" }} />
+ : nonPassingCompleted > 0
+ ? <Warning size={18} weight="fill" style={{ color: COLORS.warning, filter: "drop-shadow(0 0 4px rgba(245,158,11,0.4))" }} />
: checksRunning
? <CircleNotch size={18} className="animate-spin" style={{ color: COLORS.warning, filter: "drop-shadow(0 0 4px rgba(245,158,11,0.4))" }} />
: checks.length === 0
? <CheckCircle size={18} weight="fill" style={{ color: COLORS.textMuted }} />
: <CircleNotch size={18} className="animate-spin" style={{ color: COLORS.warning, filter: "drop-shadow(0 0 4px rgba(245,158,11,0.4))" }} />
}
title={
allChecksPassed ? "All checks have passed"
: someChecksFailing ? "Some checks failing"
+ : nonPassingCompleted > 0 ? "Some checks did not pass"
: checksRunning ? "Checks in progress"
: checks.length === 0 ? "No checks" : "Checks in progress"
}
description={
allChecksPassed ? `${passing} successful check${passing !== 1 ? "s" : ""}`
: someChecksFailing && checksRunning ? `${passing}/${totalChecks} checks passing, ${pending} still running`
: someChecksFailing ? `${passing}/${totalChecks} checks passing`
+ : nonPassingCompleted > 0 ? `${passing}/${totalChecks} checks passing`
: checks.length === 0 ? "No status checks are required" : `${pending} check${pending !== 1 ? "s" : ""} pending`
}Also applies to: 1129-1129, 1439-1463
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsx` around
lines 309 - 321, The readiness logic in summarizeChecks lets completed
non-success conclusions show as "all checks passed"; update summarizeChecks so
allChecksPassed requires every check to be completed with conclusion ===
"success" (e.g., use checks.every to verify c.status === "completed" &&
c.conclusion === "success"), compute pending as checks with status !==
"completed", and set someChecksFailing to true if any completed check has a
conclusion !== "success" (not just explicit "failure"); keep checksRunning based
on pending > 0 and total as checks.length so other consumers like the
merge-readiness row reflect true non-success completed checks.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Behavior
Tests
Style