Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Preview deployment for your docs. Learn more about Mintlify Previews.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
Disabled knowledge base sources:
📝 WalkthroughWalkthroughRemoves queue-rehearsal infrastructure and DB schema; adds PR issue-resolution (prompt + agent chat), review-thread GraphQL handling, queue rebase override logic, queue reordering UI, and multiple renderer/service/type/API updates wiring these features. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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 |
Owner
Author
|
@codex review please |
There was a problem hiding this comment.
Pull request overview
This PR updates ADE’s PR/workflow system by removing queue rehearsal plumbing, improving queue-aware rebasing/suggestion logic, and adding an agent-driven PR issue resolution workflow (failed checks + unresolved review threads) surfaced in the PR detail UI and exposed via IPC + workflow tools.
Changes:
- Add PR issue resolution support (types, availability model, IPC endpoints, main-process prompt builder/launcher, and agent workflow tools for refresh/rerun/reply/resolve).
- Remove queue rehearsal concepts across shared types, IPC, orchestration/finalization, UI loading, and local mocks.
- Introduce queue-aware rebase targeting (queue target tracking fetch + overrides) and extract queue workflow UI state into a dedicated model with tests.
Reviewed changes
Copilot reviewed 60 out of 63 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| docs/features/PULL_REQUESTS.md | Updates PR feature documentation; documents PR issue resolution, queue model extraction, and URL-driven route state. |
| docs/features/CONFLICTS.md | Updates conflicts wording around merge simulation. |
| docs/features/CHAT.md | Documents new PR issue resolution workflow tools. |
| docs/features/AGENTS.md | Updates workflow tool tier description to include PR issue resolution tools. |
| docs/architecture/SYSTEM_OVERVIEW.md | Notes PR issue resolution and updates workflow tool tier summary. |
| docs/architecture/DESKTOP_APP.md | Updates phased-loading description for PR workflows. |
| docs/architecture/AI_INTEGRATION.md | Updates workflow tool list to include PR issue resolution tools. |
| apps/desktop/src/shared/types/prs.ts | Adds review thread + issue resolution types; removes queue rehearsal-related types/events; adds queue reorder args. |
| apps/desktop/src/shared/types/orchestrator.ts | Removes queue rehearsal finalization policy/status/state fields. |
| apps/desktop/src/shared/types/lanes.ts | Extends rebase suggestion shape with base label + group context. |
| apps/desktop/src/shared/types/conflicts.ts | Extends rebase args with AI config fields; imports AiPermissionMode. |
| apps/desktop/src/shared/prIssueResolution.ts | Adds shared availability + default-scope helpers for issue resolution. |
| apps/desktop/src/shared/ipc.ts | Adds IPC channels for review threads + issue resolution + queue reorder; removes queue rehearsal IPC. |
| apps/desktop/src/renderer/components/prs/tabs/WorkflowsTab.tsx | Removes rehearsal UI dependency; uses extracted queue workflow model; refactors integration badge helpers. |
| apps/desktop/src/renderer/components/prs/tabs/queueWorkflowModel.ts | New queue workflow state model (bucketing/selection/guidance/warnings). |
| apps/desktop/src/renderer/components/prs/tabs/queueWorkflowModel.test.ts | Tests for queue workflow model behavior. |
| apps/desktop/src/renderer/components/prs/tabs/NormalTab.tsx | Removes onTabChange wiring from detail pane usage. |
| apps/desktop/src/renderer/components/prs/tabs/IntegrationTab.tsx | Filters integration PR list; updates detail pane tab navigation API usage. |
| apps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsx | Adds “open queue” affordance + queue context plumbing; refactors adeKind badge styling. |
| apps/desktop/src/renderer/components/prs/tabs/GitHubTab.test.tsx | Extends tests for queue affordance + queue context plumbing. |
| apps/desktop/src/renderer/components/prs/state/PrsContext.tsx | Stops loading/handling queue rehearsal state; keeps queue landing state lazy loading. |
| apps/desktop/src/renderer/components/prs/shared/PrIssueResolverModal.test.tsx | Adds tests for scope defaults/disabled states and prompt copy. |
| apps/desktop/src/renderer/components/prs/shared/PrAiResolverPanel.test.tsx | Removes queue rehearsal state from mocked PR context value. |
| apps/desktop/src/renderer/components/prs/prsRouteState.ts | New URL search/hash route state parsing + canonical search builder. |
| apps/desktop/src/renderer/components/prs/prsRouteState.test.ts | Tests route parsing/building behavior. |
| apps/desktop/src/renderer/components/prs/PRsPage.tsx | Makes PR tab navigation URL-driven; wires GitHub “open queue” into workflow routing. |
| apps/desktop/src/renderer/components/prs/detail/PrDetailPane.issueResolver.test.tsx | Adds coverage for issue resolver CTA visibility + launching + prompt copy + markdown rendering. |
| apps/desktop/src/renderer/components/prs/CreatePrModal.test.tsx | Adds coverage for queue builder ordering + drag-drop ordering for createQueue. |
| apps/desktop/src/renderer/components/missions/CreateMissionDialog.tsx | Removes queue rehearsal option; updates explanatory copy. |
| apps/desktop/src/renderer/components/lanes/LanesPage.tsx | Uses new base label when showing “behind” tooltip. |
| apps/desktop/src/renderer/components/lanes/LaneRebaseBanner.tsx | Uses new base label in rebase banner copy. |
| apps/desktop/src/renderer/components/graph/WorkspaceGraphPage.tsx | Removes onTabChange wiring from detail pane usage. |
| apps/desktop/src/renderer/browserMock.ts | Removes queue rehearsal mock; adds review thread + issue resolution + review-thread action mocks. |
| apps/desktop/src/preload/preload.ts | Adds preload APIs for review threads + issue resolution + queue reorder; removes rehearsal APIs. |
| apps/desktop/src/preload/global.d.ts | Updates window.ade typings for new PR APIs and removed rehearsal APIs. |
| apps/desktop/src/main/services/state/onConflictAudit.test.ts | Removes queue rehearsal table from approved conflict audit list. |
| apps/desktop/src/main/services/state/kvDb.ts | Removes queue rehearsal table migration. |
| apps/desktop/src/main/services/shared/queueRebase.ts | New helper for queue-aware rebase overrides + queue target tracking fetch. |
| apps/desktop/src/main/services/prs/queueRehearsalService.test.ts | Deletes queue rehearsal service tests (feature removed). |
| apps/desktop/src/main/services/prs/prService.reviewThreads.test.ts | Adds review thread GraphQL behavior coverage. |
| apps/desktop/src/main/services/prs/prIssueResolver.ts | New main-process prompt builder + launcher for PR issue resolution sessions. |
| apps/desktop/src/main/services/prs/prIssueResolver.test.ts | Tests prompt composition and chat launch/preview behavior. |
| apps/desktop/src/main/services/orchestrator/missionStateDoc.ts | Removes queue rehearsal fields/status handling from mission state doc normalization. |
| apps/desktop/src/main/services/orchestrator/aiOrchestratorService.ts | Removes queue rehearsal finalization path + state updates. |
| apps/desktop/src/main/services/orchestrator/aiOrchestratorService.test.ts | Removes rehearsal-related orchestrator test. |
| apps/desktop/src/main/services/missions/missionPreflightService.ts | Removes rehearsal capability checks; updates queue preflight messaging. |
| apps/desktop/src/main/services/missions/missionPreflightService.test.ts | Removes rehearsal-related preflight test. |
| apps/desktop/src/main/services/lanes/rebaseSuggestionService.ts | Makes rebase suggestions queue-aware (override base ref/label/context); adds refresh API. |
| apps/desktop/src/main/services/lanes/rebaseSuggestionService.test.ts | Adds tests for queue-aware rebase suggestions. |
| apps/desktop/src/main/services/lanes/laneService.ts | Makes lane status behind-count queue-aware; adds cache invalidation method. |
| apps/desktop/src/main/services/ipc/registerIpc.ts | Registers new IPC handlers (review threads, issue resolution start/preview, reorder queue) and removes rehearsal IPC wiring. |
| apps/desktop/src/main/services/conflicts/conflictService.ts | Makes rebase needs + rebases queue-aware; logs AI rebase config. |
| apps/desktop/src/main/services/conflicts/conflictService.test.ts | Adds test coverage for queue-aware rebases. |
| apps/desktop/src/main/services/ai/tools/workflowTools.ts | Adds dedicated PR issue resolution tools + shared error formatting helper. |
| apps/desktop/src/main/services/ai/tools/workflowTools.test.ts | Adds coverage for new PR issue resolution workflow tools. |
| apps/desktop/src/main/main.ts | Removes queue rehearsal service wiring; wires rebaseSuggestionService into prService; passes projectRoot into rebaseSuggestionService. |
| .ade/kv.sqlite | Adds a local SQLite file to the repo (should not be committed). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
566
to
575
| if (!row) return DEFAULT_LANE_STATUS; | ||
| const parent = row.parent_lane_id ? rowsById.get(row.parent_lane_id) : null; | ||
| let baseRef = parent?.branch_ref ?? row.base_ref; | ||
| const queueOverride = await resolveQueueRebaseOverride({ | ||
| db, | ||
| projectId, | ||
| projectRoot, | ||
| laneId: row.id, | ||
| }); | ||
| let baseRef = queueOverride?.comparisonRef ?? parent?.branch_ref ?? row.base_ref; | ||
|
|
There was a problem hiding this comment.
listLanes() now calls resolveQueueRebaseOverride() inside resolveStatus() for every lane. resolveQueueRebaseOverride() performs DB queries (and may run git commands to check tracking refs), so this can add noticeable overhead when many lanes are listed/refreshed. Consider precomputing queue overrides once per listLanes() invocation (or only for lanes that are in queue groups) and reusing the result within resolveStatus() to avoid an N+1 pattern.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 56a7ce62d7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+193
to
+197
| groupId: queueState.groupId, | ||
| name: queueState.groupName, | ||
| targetBranch: queueState.targetBranch, | ||
| landingState: queueState, | ||
| members: toMembers(queueState.entries), |
There was a problem hiding this comment.
buildQueueGroups() now seeds selectedGroup.members from queueState.entries, but prService.land() removes landed PRs from pr_group_members. After a queue lands its first item, handleDropMember() sends the full selectedGroup.members.map(...) list to reorderQueuePrs(), and the backend rejects it because the landed PR IDs are no longer current members (apps/desktop/src/main/services/prs/prService.ts, reorderQueuePrs). The result is that drag-and-drop stops working for partially landed queues even though the UI still says the remaining order can be changed.
Useful? React with 👍 / 👎.
Comment on lines
+858
to
+861
| {member.pr?.state !== "merged" ? ( | ||
| <button | ||
| type="button" | ||
| onClick={() => setDeleteTargetPrId(deleteTargetPrId === member.prId ? null : member.prId)} |
There was a problem hiding this comment.
This remove action stays enabled even when the queue is landing or paused. In that state, deletePr() drops the PR row and group membership, but queueLandingService continues driving the persisted state.entries and later calls prService.getStatus() / prService.land() for the deleted prId, which forces the run into a manual failure path instead of continuing cleanly. If queue automation owns the group, member removal needs to be blocked or the queue state must be rewritten in the same transaction.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/desktop/src/main/services/conflicts/conflictService.ts (1)
4371-4384:⚠️ Potential issue | 🟠 MajorFinish queue-target preflight before emitting
rebase-started.This path now depends on the queue override, but it still emits
rebase-startedbefore that async preflight and never refreshes queue tracking refs like the scan/get paths do. A directrebaseLane()call can therefore use a stale/missing comparison ref, and a preflight failure leaves listeners with a start event but no matching terminal event.Proposed fix
- if (onEvent) { - onEvent({ type: "rebase-started", laneId: args.laneId, timestamp: new Date().toISOString() }); - } - - const queueOverride = await resolveQueueRebaseOverride({ + try { + await fetchQueueTargetTrackingBranches({ + db, + projectId, + projectRoot, + }); + } catch (error) { + logger.warn("rebaseLane: queue target refresh failed", { + laneId: args.laneId, + error: error instanceof Error ? error.message : String(error), + }); + } + const queueOverride = await resolveQueueRebaseOverride({ db, projectId, projectRoot, laneId: lane.id, }); const rebaseTarget = queueOverride?.comparisonRef ?? lane.baseRef; + if (onEvent) { + onEvent({ type: "rebase-started", laneId: args.laneId, timestamp: new Date().toISOString() }); + } const rebaseRes = await runGit(🤖 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 4371 - 4384, The code emits onEvent({type: "rebase-started"}) before performing the async queue preflight (resolveQueueRebaseOverride), which can lead to stale/missing comparison refs and orphaned start events on preflight failure; change the flow so you first await resolveQueueRebaseOverride (and refresh queue tracking refs the same way scan/get paths do) and only emit the "rebase-started" event after that succeeds, and ensure errors from resolveQueueRebaseOverride or runGit(["rebase", ...]) cause a terminal error event (or no start event) so listeners always see matching terminal events; update the logic around resolveQueueRebaseOverride, rebaseTarget, runGit and onEvent accordingly.apps/desktop/src/renderer/browserMock.ts (1)
1411-1506:⚠️ Potential issue | 🟡 MinorAdd
reorderQueuePrsto thewindow.ade.prsmock.The renderer component
QueueTab.tsxcallswindow.ade.prs.reorderQueuePrs(), which is now exposed in the preload layer but missing from the browserMock. Browser-preview mode will fail when attempting to reorder queue items. Add a stub matching the preload signature:reorderQueuePrs: (args: ReorderQueuePrsArgs) => Promise<void>.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/browserMock.ts` around lines 1411 - 1506, The mock for window.ade.prs is missing the reorderQueuePrs method used by QueueTab.tsx; add a stub with the preload signature named reorderQueuePrs that accepts (args: ReorderQueuePrsArgs) and returns a Promise<void> (e.g., an async function that resolves immediately). Insert it alongside other methods in the prs mock (near methods like startQueueAutomation, pauseQueueAutomation, getQueueState) so calls to window.ade.prs.reorderQueuePrs(...) in the renderer succeed in browser-preview mode.
🧹 Nitpick comments (4)
apps/desktop/src/main/services/lanes/laneService.ts (1)
568-578: Integration of queue rebase override is correctly implemented.The logic properly prioritizes
queueOverride.comparisonRefforbaseRefresolution, falling back toparent?.branch_refand thenrow.base_ref. The guard on line 578 (!queueOverride) correctly prevents the primary-lane upstream logic from overwriting a queue-based comparison reference.One consideration:
resolveQueueRebaseOverrideis called withinresolveStatus, which is invoked for each lane duringlistLanes. The function performs multiple database queries and a git operation per lane. For repositories with many lanes, this adds per-lane async overhead. If performance becomes a concern with large lane counts, consider batching or caching these lookups.🤖 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.ts` around lines 568 - 578, Performance issue: resolveQueueRebaseOverride is called per-lane inside resolveStatus during listLanes, causing repeated DB and git ops. Fix by batching or caching: move the resolveQueueRebaseOverride calls out of the per-lane loop in listLanes (or add a memoization layer keyed by repo/project/lane) so resolveStatus uses a precomputed map of queue overrides; alternatively implement a batched resolver that takes multiple lane ids and returns all comparisonRefs in one go, then reference that map from resolveStatus to compute baseRef. Ensure you update references to resolveQueueRebaseOverride, resolveStatus, and listLanes accordingly.apps/desktop/src/renderer/components/prs/tabs/IntegrationTab.tsx (1)
1850-1850: Unify rebase-tab navigation behind the shared route helper.This callback now uses tab state, but
RebaseGuidancePanelin the same file still hardcodes#/prs?tab=rebase&laneId=.... Routing both entry points through the shared PR-route builder will keep them aligned with the new workflow URL shape and avoid another drift.🤖 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/IntegrationTab.tsx` at line 1850, The Rebase entrypoints are inconsistent: the onOpenRebaseTab callback uses setActiveTab("rebase") while RebaseGuidancePanel still hardcodes '#/prs?tab=rebase&laneId=...'. Replace the hardcoded URL in RebaseGuidancePanel with the shared PR-route builder helper (the same function used elsewhere to construct PR URLs) and update the onOpenRebaseTab flow to navigate via that route helper as well so both entry points call the same route-construction function instead of using setActiveTab or string literals.apps/desktop/src/main/services/ipc/registerIpc.ts (1)
4316-4323: Extract the"PR not found"fallback into a shared predicate.This is another
err.message.includes("PR not found")branch in the file. One wording change inprServicewill silently change renderer behavior across several endpoints, so it’s worth centralizing the check now, or exposing a typed not-found error fromprService.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/ipc/registerIpc.ts` around lines 4316 - 4323, Extract the duplicate "PR not found" string check into a single predicate and use it here: replace the inline check inside the ipcMain.handle for IPC.prsGetReviewThreads (the try/catch around ctx.prService.getReviewThreads(arg.prId)) with a call to a shared helper like isPrNotFoundError(err) (or make prService throw a typed NotFound error and check instanceof NotFoundError). Add that helper next to other IPC helpers and update any other handlers in the file that currently use err.message.includes("PR not found") to call isPrNotFoundError so the renderer behavior stays consistent if wording changes.apps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsx (1)
533-546: Lane fallback logic may silently hide actionable issues.When
laneForPris null (no active lane), the availability is overridden to report no actionable issues. This could confuse users who see failing checks/comments in the UI but can't use the resolver. Consider showing a disabled state with an explanatory tooltip instead of hiding the button entirely.🤖 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 533 - 546, The current logic in the issueResolutionAvailability useMemo (which calls getPrIssueResolutionAvailability using checks and reviewThreads) overwrites actionable flags when laneForPr is null, silently hiding issues; instead preserve the original availability and add an explicit flag (e.g. resolverEnabled or laneAvailable) that is false when laneForPr is null so the UI can render the resolver button disabled with a tooltip; update the useMemo to return { ...availability, resolverEnabled: !!laneForPr } (or similar named flag) and ensure consumers read resolverEnabled rather than relying on hasActionable* booleans being suppressed.
🤖 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 4163-4167: fetchQueueTargetTrackingBranches should not be a hard
dependency for status reads; wrap the call to fetchQueueTargetTrackingBranches
in a try/catch around both places where it’s currently awaited and, on error,
log or swallow the error and fall back to using lane.baseRef for the local read
path so local error handling can proceed; apply the same guard to the second
occurrence as well so both read paths behave identically.
In `@apps/desktop/src/main/services/lanes/rebaseSuggestionService.ts`:
- Around line 104-118: When a queueOverride is present, don't reuse
lane.parentLaneId because that collapses queue-target suggestions with
direct-parent suggestions; in the resolveQueueRebaseOverride handling (see
resolveQueueRebaseOverride, queueOverride, lane.parentLaneId,
queueOverride.queueGroupId) always set the returned parentLaneId to the
queue-scoped identity (e.g. `queue:${queueOverride.queueGroupId}`) instead of
using lane.parentLaneId ?? `queue:...`; keep reading parentHeadSha via
readRefHeadSha and return baseLabel and groupContext as before.
In `@apps/desktop/src/main/services/prs/prService.reviewThreads.test.ts`:
- Around line 141-145: The current test's apiRequest mock (the vi.fn that
inspects the local variable query) only counts global occurrences of
"createdAt"/"updatedAt", which misses regressions when those fields shift from
comments.nodes to reviewThreads.nodes; update the assertion to verify the
timestamps are selected specifically under comments (e.g., assert the query
contains "comments" + a selection block that includes createdAt/updatedAt or
uses a regex that matches "comments\\s*\\{[^}]*createdAt" /
"comments\\s*\\{[^}]*updatedAt"), and also add an explicit negative assertion
that "reviewThreads" does not include those fields (or assert
createdAt/updatedAt are not present within the "reviewThreads" selection block)
so the test fails if the fields move to reviewThreads; locate and change the
assertions in the apiRequest mock in prs/prService.reviewThreads.test.ts (the
vi.fn that binds requestPath/body and sets const query = body?.query ?? "").
- Around line 125-126: The test creates a temp dir (root) and opens a real DB
via openKvDb(path.join(root, ".ade.db"), createLogger()) but never closes or
deletes it; wrap the setup in a try/finally so you always await closing the DB
(call the DB's close method returned by openKvDb, e.g., await db.close() or
await db.closeDb() if applicable) only if db was created, and in the finally
remove the temp workspace (fs.rmSync(root, { recursive: true, force: true }) or
fs.rmdirSync with recursive) to ensure file handles are released and the temp
directory is removed.
In `@apps/desktop/src/main/services/prs/prService.ts`:
- Around line 1079-1166: The GraphQL query only requests comments(first: 50) and
the code in the nodes -> threads.push block derives url/createdAt/updatedAt from
that truncated comment slice; instead request thread-level metadata in the query
(e.g. include node-level createdAt, updatedAt and url or the appropriate
ReviewThread fields) and update the mapping in the nodes loop (in the code that
builds comments, computes latestComment, and pushes into threads) to prefer
node.createdAt/node.updatedAt/node.url when present, falling back to
latestComment only as a last resort; modify the query string named query and the
mapping at threads.push to use those thread-level fields rather than deriving
them solely from comments(first: 50).
- Around line 1831-1842: Post-merge cache invalidation is currently unguarded
(laneService.invalidateCache?.()) so if it throws after GitHub merged the PR the
overall operation may incorrectly report failure; wrap the invalidateCache call
in its own try/catch (after fetchRemoteTrackingBranch) that logs any error (use
logger.warn or logger.error) but does not rethrow or change the merge result,
ensuring invalidateCache remains best-effort; locate this change around the
fetchRemoteTrackingBranch call in prs/prService.ts and update the code that
invokes laneService.invalidateCache to be defensive and swallow failures.
- Around line 4286-4355: The replyToReviewThread and resolveReviewThread
handlers call GraphQL mutations using a supplied threadId but only call
requireRow(args.prId) locally; add a guard that the provided threadId actually
belongs to the PR identified by args.prId before performing the mutation.
Implement this by first querying GitHub for the thread node (using the threadId)
to fetch its associated pullRequest identifier (or pullRequest
number/repo+number) and compare it against the local PR mapping validated by
requireRow(args.prId); if they do not match, throw an authorization/error and do
not call addPullRequestReviewThreadReply or resolveReviewThread. Keep checks
adjacent to the existing requireRow call and use the same variable names
threadId and args.prId so the change is easy to locate.
- Around line 234-250: The parser parseConflictMarkers uses markerRegex which
only matches LF newlines, so CRLF files yield no excerpts; update
parseConflictMarkers to handle CRLF by normalizing input newlines (e.g., replace
\r\n with \n) before running the regex and/or update markerRegex to accept both
CRLF and LF (use \r?\n in the pattern for the opening and separator markers).
Ensure the rest of the logic (oursLines, theirsLines, conflictMarkers, diffHunk)
still trims/joins correctly after normalization so previews work for Windows
worktrees.
In `@apps/desktop/src/renderer/components/prs/CreatePrModal.tsx`:
- Around line 1027-1128: The queue list only supports drag-and-drop
(queueLaneIds, queueDragLaneId, handleQueueLaneDrop) so keyboard/AT users can't
reorder; add explicit move-up and move-down buttons (or a two-button control)
next to each row that call a new helper (e.g., moveQueueLane(laneId, direction)
or moveUpLane/moveDownLane) which updates state via setQueueLaneIds by swapping
positions in the array, ensure each control is a focusable button with
aria-labels like "Move lane up" / "Move lane down" and visible focus styles, and
wire keyboard handlers (onKeyDown) on the row or buttons to accept Enter/Space
and ArrowUp/ArrowDown to trigger the same move functions; keep existing drag
handlers but ensure buttons are always available and disabled appropriately when
at top/bottom (or when queueLaneIds.length <= 1).
- Around line 236-244: The reorderQueueLaneIds function misplaces the moved item
when the draggedIndex is before the targetIndex because targetIndex is computed
from the original array and not adjusted after removing the dragged element;
update reorderQueueLaneIds to compute or normalize the insert index after
splicing out the dragged item (e.g., if draggedIndex < targetIndex, decrement
targetIndex by 1 before calling next.splice), ensuring you still handle the
early-return cases for invalid indices and return the new array from
reorderQueueLaneIds.
In
`@apps/desktop/src/renderer/components/prs/detail/PrDetailPane.issueResolver.test.tsx`:
- Around line 57-93: The fixture's visibilityCases rely on global renderPane()
supplying a failing PrStatus which masks bugs; update each case in
visibilityCases to include a scenario-specific PrStatus field (e.g.,
"changes_requested", "success", or "pending") and ensure the test passes that
status into renderPane/PrDetailPane instead of relying on the global default.
Locate visibilityCases and the helper functions makeCheck and makeThread in the
test, add a status property per case that matches the checks/reviewThreads
intent (failed when there are failing checks, success when all checks pass and
threads resolved, in_progress/pending when checks run), and update the calls to
renderPane to use case.status so each case is isolated; apply the same pattern
to the other fixture blocks referenced (around lines 145-155 and 157-223).
In `@apps/desktop/src/renderer/components/prs/PRsPage.tsx`:
- Around line 55-61: In PRsPage, prevent writing the canonical URL until route
hydration completes by adding a didHydrateRouteRef (e.g., React.useRef(false))
next to the local state and set it true after the initial syncFromLocation run;
update the write effect that calls history.replace() (the effect paired with
parsePrsRouteState / syncFromLocation) to early-return unless
didHydrateRouteRef.current is true so the provider's default state doesn't
overwrite deep links, and change the read effect to use location.hash (not
window.location.hash) and include location.hash in its dependency array so
HashRouter/hashes stay consistent with parsePrsRouteState.
In `@apps/desktop/src/renderer/components/prs/prsRouteState.ts`:
- Line 2: PrActiveTab currently excludes "github" causing buildPrsRouteSearch to
mis-classify github tabs as workflows; update the PrActiveTab type to include
"github" (in addition to "normal" and PrWorkflowTab) and modify
buildPrsRouteSearch to explicitly handle the "github" value instead of treating
all non-"normal" values as a PrWorkflowTab; also check parsePrsRouteState and
any serialization logic that maps tab strings to ensure "github" round-trips
correctly and is not coerced into PrWorkflowTab.
In `@apps/desktop/src/renderer/components/prs/tabs/IntegrationTab.tsx`:
- Around line 551-554: The empty-state gate currently checks prs.length but this
component now renders only the filtered list integrationPrs; change the guards
and any places where prs is mapped for the integration panel to use
integrationPrs (the React.useMemo result using mergeContextByPrId) so the "No
integration PRs" message displays when there are zero integration PRs. Also
update any related conditional renders or maps (references: integrationPrs, prs,
mergeContextByPrId) to use integrationPrs and ensure dependent hooks/arrays
reference it where necessary.
---
Outside diff comments:
In `@apps/desktop/src/main/services/conflicts/conflictService.ts`:
- Around line 4371-4384: The code emits onEvent({type: "rebase-started"}) before
performing the async queue preflight (resolveQueueRebaseOverride), which can
lead to stale/missing comparison refs and orphaned start events on preflight
failure; change the flow so you first await resolveQueueRebaseOverride (and
refresh queue tracking refs the same way scan/get paths do) and only emit the
"rebase-started" event after that succeeds, and ensure errors from
resolveQueueRebaseOverride or runGit(["rebase", ...]) cause a terminal error
event (or no start event) so listeners always see matching terminal events;
update the logic around resolveQueueRebaseOverride, rebaseTarget, runGit and
onEvent accordingly.
In `@apps/desktop/src/renderer/browserMock.ts`:
- Around line 1411-1506: The mock for window.ade.prs is missing the
reorderQueuePrs method used by QueueTab.tsx; add a stub with the preload
signature named reorderQueuePrs that accepts (args: ReorderQueuePrsArgs) and
returns a Promise<void> (e.g., an async function that resolves immediately).
Insert it alongside other methods in the prs mock (near methods like
startQueueAutomation, pauseQueueAutomation, getQueueState) so calls to
window.ade.prs.reorderQueuePrs(...) in the renderer succeed in browser-preview
mode.
---
Nitpick comments:
In `@apps/desktop/src/main/services/ipc/registerIpc.ts`:
- Around line 4316-4323: Extract the duplicate "PR not found" string check into
a single predicate and use it here: replace the inline check inside the
ipcMain.handle for IPC.prsGetReviewThreads (the try/catch around
ctx.prService.getReviewThreads(arg.prId)) with a call to a shared helper like
isPrNotFoundError(err) (or make prService throw a typed NotFound error and check
instanceof NotFoundError). Add that helper next to other IPC helpers and update
any other handlers in the file that currently use err.message.includes("PR not
found") to call isPrNotFoundError so the renderer behavior stays consistent if
wording changes.
In `@apps/desktop/src/main/services/lanes/laneService.ts`:
- Around line 568-578: Performance issue: resolveQueueRebaseOverride is called
per-lane inside resolveStatus during listLanes, causing repeated DB and git ops.
Fix by batching or caching: move the resolveQueueRebaseOverride calls out of the
per-lane loop in listLanes (or add a memoization layer keyed by
repo/project/lane) so resolveStatus uses a precomputed map of queue overrides;
alternatively implement a batched resolver that takes multiple lane ids and
returns all comparisonRefs in one go, then reference that map from resolveStatus
to compute baseRef. Ensure you update references to resolveQueueRebaseOverride,
resolveStatus, and listLanes accordingly.
In `@apps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsx`:
- Around line 533-546: The current logic in the issueResolutionAvailability
useMemo (which calls getPrIssueResolutionAvailability using checks and
reviewThreads) overwrites actionable flags when laneForPr is null, silently
hiding issues; instead preserve the original availability and add an explicit
flag (e.g. resolverEnabled or laneAvailable) that is false when laneForPr is
null so the UI can render the resolver button disabled with a tooltip; update
the useMemo to return { ...availability, resolverEnabled: !!laneForPr } (or
similar named flag) and ensure consumers read resolverEnabled rather than
relying on hasActionable* booleans being suppressed.
In `@apps/desktop/src/renderer/components/prs/tabs/IntegrationTab.tsx`:
- Line 1850: The Rebase entrypoints are inconsistent: the onOpenRebaseTab
callback uses setActiveTab("rebase") while RebaseGuidancePanel still hardcodes
'#/prs?tab=rebase&laneId=...'. Replace the hardcoded URL in RebaseGuidancePanel
with the shared PR-route builder helper (the same function used elsewhere to
construct PR URLs) and update the onOpenRebaseTab flow to navigate via that
route helper as well so both entry points call the same route-construction
function instead of using setActiveTab or string literals.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4c4bff38-8990-47e1-a911-93a36b942a39
⛔ Files ignored due to path filters (8)
.ade/kv.sqliteis excluded by!.ade/**docs/architecture/AI_INTEGRATION.mdis excluded by!docs/**docs/architecture/DESKTOP_APP.mdis excluded by!docs/**docs/architecture/SYSTEM_OVERVIEW.mdis excluded by!docs/**docs/features/AGENTS.mdis excluded by!docs/**docs/features/CHAT.mdis excluded by!docs/**docs/features/CONFLICTS.mdis excluded by!docs/**docs/features/PULL_REQUESTS.mdis excluded by!docs/**
📒 Files selected for processing (55)
apps/desktop/src/main/main.tsapps/desktop/src/main/services/ai/tools/workflowTools.test.tsapps/desktop/src/main/services/ai/tools/workflowTools.tsapps/desktop/src/main/services/conflicts/conflictService.test.tsapps/desktop/src/main/services/conflicts/conflictService.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/lanes/laneService.tsapps/desktop/src/main/services/lanes/rebaseSuggestionService.test.tsapps/desktop/src/main/services/lanes/rebaseSuggestionService.tsapps/desktop/src/main/services/missions/missionPreflightService.test.tsapps/desktop/src/main/services/missions/missionPreflightService.tsapps/desktop/src/main/services/orchestrator/aiOrchestratorService.test.tsapps/desktop/src/main/services/orchestrator/aiOrchestratorService.tsapps/desktop/src/main/services/orchestrator/missionStateDoc.tsapps/desktop/src/main/services/prs/prIssueResolver.test.tsapps/desktop/src/main/services/prs/prIssueResolver.tsapps/desktop/src/main/services/prs/prService.reviewThreads.test.tsapps/desktop/src/main/services/prs/prService.tsapps/desktop/src/main/services/prs/queueRehearsalService.test.tsapps/desktop/src/main/services/prs/queueRehearsalService.tsapps/desktop/src/main/services/shared/queueRebase.tsapps/desktop/src/main/services/state/kvDb.tsapps/desktop/src/main/services/state/onConflictAudit.test.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/graph/WorkspaceGraphPage.tsxapps/desktop/src/renderer/components/lanes/LaneRebaseBanner.tsxapps/desktop/src/renderer/components/lanes/LanesPage.tsxapps/desktop/src/renderer/components/missions/CreateMissionDialog.tsxapps/desktop/src/renderer/components/prs/CreatePrModal.test.tsxapps/desktop/src/renderer/components/prs/CreatePrModal.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/prsRouteState.test.tsapps/desktop/src/renderer/components/prs/prsRouteState.tsapps/desktop/src/renderer/components/prs/shared/PrAiResolverPanel.test.tsxapps/desktop/src/renderer/components/prs/shared/PrIssueResolverModal.test.tsxapps/desktop/src/renderer/components/prs/shared/PrIssueResolverModal.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/IntegrationTab.tsxapps/desktop/src/renderer/components/prs/tabs/NormalTab.tsxapps/desktop/src/renderer/components/prs/tabs/QueueTab.tsxapps/desktop/src/renderer/components/prs/tabs/WorkflowsTab.tsxapps/desktop/src/renderer/components/prs/tabs/queueWorkflowModel.test.tsapps/desktop/src/renderer/components/prs/tabs/queueWorkflowModel.tsapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/prIssueResolution.tsapps/desktop/src/shared/types/conflicts.tsapps/desktop/src/shared/types/lanes.tsapps/desktop/src/shared/types/orchestrator.tsapps/desktop/src/shared/types/prs.ts
💤 Files with no reviewable changes (10)
- apps/desktop/src/renderer/components/prs/shared/PrAiResolverPanel.test.tsx
- apps/desktop/src/main/services/state/onConflictAudit.test.ts
- apps/desktop/src/renderer/components/prs/tabs/NormalTab.tsx
- apps/desktop/src/renderer/components/graph/WorkspaceGraphPage.tsx
- apps/desktop/src/main/services/state/kvDb.ts
- apps/desktop/src/main/services/missions/missionPreflightService.test.ts
- apps/desktop/src/main/services/prs/queueRehearsalService.test.ts
- apps/desktop/src/main/services/prs/queueRehearsalService.ts
- apps/desktop/src/shared/types/orchestrator.ts
- apps/desktop/src/main/services/orchestrator/aiOrchestratorService.test.ts
Comment on lines
+4163
to
+4167
| await fetchQueueTargetTrackingBranches({ | ||
| db, | ||
| projectId, | ||
| projectRoot, | ||
| }); |
There was a problem hiding this comment.
Don’t make queue-ref refresh a hard dependency for status reads.
fetchQueueTargetTrackingBranches(...) now runs before any local error handling. If that git/network call fails, both read paths reject before they can fall back to lane.baseRef, so one queue refresh glitch hides rebase status for every lane.
Proposed fix
- await fetchQueueTargetTrackingBranches({
- db,
- projectId,
- projectRoot,
- });
+ try {
+ await fetchQueueTargetTrackingBranches({
+ db,
+ projectId,
+ projectRoot,
+ });
+ } catch (error) {
+ logger.warn("conflicts.queue_target_refresh_failed", {
+ error: error instanceof Error ? error.message : String(error),
+ });
+ }Apply the same guard in both methods.
Also applies to: 4234-4238
🤖 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
4163 - 4167, fetchQueueTargetTrackingBranches should not be a hard dependency
for status reads; wrap the call to fetchQueueTargetTrackingBranches in a
try/catch around both places where it’s currently awaited and, on error, log or
swallow the error and fall back to using lane.baseRef for the local read path so
local error handling can proceed; apply the same guard to the second occurrence
as well so both read paths behave identically.
apps/desktop/src/main/services/prs/prService.reviewThreads.test.ts
Outdated
Show resolved
Hide resolved
apps/desktop/src/renderer/components/prs/detail/PrDetailPane.issueResolver.test.tsx
Show resolved
Hide resolved
Comment on lines
55
to
+61
| React.useEffect(() => { | ||
| const syncFromLocation = () => { | ||
| try { | ||
| const searchParams = new URLSearchParams(window.location.search); | ||
| const fromSearch = searchParams.get("tab"); | ||
| const workflow = searchParams.get("workflow"); | ||
| const searchLaneId = searchParams.get("laneId"); | ||
| const fromHash = (() => { | ||
| const hash = window.location.hash ?? ""; | ||
| const queryIndex = hash.indexOf("?"); | ||
| if (queryIndex < 0) return null; | ||
| const hashParams = new URLSearchParams(hash.slice(queryIndex + 1)); | ||
| return { | ||
| tab: hashParams.get("tab"), | ||
| workflow: hashParams.get("workflow"), | ||
| laneId: hashParams.get("laneId"), | ||
| }; | ||
| })(); | ||
|
|
||
| const tab = fromSearch ?? fromHash?.tab ?? null; | ||
| const workflowTab = workflow ?? fromHash?.workflow ?? null; | ||
| const laneId = searchLaneId ?? fromHash?.laneId ?? null; | ||
| const routeState = parsePrsRouteState({ | ||
| search: location.search, | ||
| hash: window.location.hash, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== PRs context initial state =="
fd 'PrsContext.tsx$' apps/desktop/src --exec sed -n '1,260p' {}
echo
echo "== PR route parse/build helpers =="
fd 'prsRouteState.ts$' apps/desktop/src --exec sed -n '1,260p' {}
echo
echo "== Router setup =="
rg -n "HashRouter|BrowserRouter|MemoryRouter|createHashRouter|createBrowserRouter|RouterProvider" apps/desktop/srcRepository: arul28/ADE
Length of output: 13162
🏁 Script executed:
fd 'PRsPage.tsx$' apps/desktop/src --exec wc -l {}Repository: arul28/ADE
Length of output: 110
🏁 Script executed:
fd 'PRsPage.tsx$' apps/desktop/src --exec sed -n '50,120p' {}Repository: arul28/ADE
Length of output: 2415
Don't write the canonical URL until route hydration has finished.
On the first mount, syncFromLocation() only queues state updates. The write effect below still runs in the same commit with the provider's default state (activeTab="normal", selectedPrId=null, etc.), so it can immediately replace() a deep link or back/forward URL with default search params. The read effect also reads window.location.hash instead of location.hash and omits location.hash from its dependencies, creating inconsistency with HashRouter.
💡 Suggested fix
Add a hydration ref near the other local state:
const didHydrateRouteRef = React.useRef(false); const routeState = parsePrsRouteState({
search: location.search,
- hash: window.location.hash,
+ hash: location.hash,
});
const tab = routeState.tab;
const workflowTab = routeState.workflowTab;
const laneId = routeState.laneId;
@@
if (tab === "rebase" || workflowTab === "rebase") {
setSelectedRebaseItemId(laneId ?? null);
}
+ didHydrateRouteRef.current = true;
} catch {
+ didHydrateRouteRef.current = true;
// Ignore malformed URLs and fall back to current state.
}
};
@@
- }, [location.search, setActiveTab, setSelectedPrId, setSelectedQueueGroupId, setSelectedRebaseItemId]);
+ }, [location.search, location.hash, setActiveTab, setSelectedPrId, setSelectedQueueGroupId, setSelectedRebaseItemId]);
React.useEffect(() => {
+ if (!didHydrateRouteRef.current) return;
const nextSearch = buildPrsRouteSearch({
activeTab,
selectedPrId,
selectedQueueGroupId,
selectedRebaseItemId,Also applies to: 100-117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/prs/PRsPage.tsx` around lines 55 - 61,
In PRsPage, prevent writing the canonical URL until route hydration completes by
adding a didHydrateRouteRef (e.g., React.useRef(false)) next to the local state
and set it true after the initial syncFromLocation run; update the write effect
that calls history.replace() (the effect paired with parsePrsRouteState /
syncFromLocation) to early-return unless didHydrateRouteRef.current is true so
the provider's default state doesn't overwrite deep links, and change the read
effect to use location.hash (not window.location.hash) and include location.hash
in its dependency array so HashRouter/hashes stay consistent with
parsePrsRouteState.
Comment on lines
+551
to
+554
| const integrationPrs = React.useMemo( | ||
| () => prs.filter((pr) => mergeContextByPrId[pr.id]?.groupType === "integration"), | ||
| [mergeContextByPrId, prs], | ||
| ); |
There was a problem hiding this comment.
Update the empty-state gate to use the filtered list.
Once this tab renders only integrationPrs, the guard at Line 1345 can't stay on prs.length. If the workspace has only normal PRs, this map renders nothing and the panel goes blank instead of showing “No integration PRs.”
Suggested fix
- {!prs.length && !proposals.length ? (
+ {!integrationPrs.length && !proposals.length ? (Also applies to: 1420-1420
🤖 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/IntegrationTab.tsx` around
lines 551 - 554, The empty-state gate currently checks prs.length but this
component now renders only the filtered list integrationPrs; change the guards
and any places where prs is mapped for the integration panel to use
integrationPrs (the React.useMemo result using mergeContextByPrId) so the "No
integration PRs" message displays when there are zero integration PRs. Also
update any related conditional renders or maps (references: integrationPrs, prs,
mergeContextByPrId) to use integrationPrs and ensure dependent hooks/arrays
reference it where necessary.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/desktop/src/renderer/components/prs/detail/PrDetailPane.issueResolver.test.tsx (1)
57-96:⚠️ Potential issue | 🟡 MinorMake scenario status explicit in every visibility case to prevent masked regressions.
Line [61] keeps
statusOverridesoptional, and cases at Lines [64]-[78] still inheritmakeStatus()defaults (failing+changes_requested). That can hide bugs where CTA visibility keys off stale status instead of the per-case checks/threads payload.💡 Suggested fixture tightening
const visibilityCases: Array<{ name: string; checks: PrCheck[]; reviewThreads: PrReviewThread[]; - statusOverrides?: Partial<PrStatus>; + statusOverrides: Partial<PrStatus>; visible: boolean; }> = [ { name: "shows for failed checks only", checks: [makeCheck()], reviewThreads: [], + statusOverrides: { checksStatus: "failing", reviewStatus: "approved" }, visible: true, }, { name: "hides while checks are still running", checks: [ makeCheck(), makeCheck({ name: "ci / lint", status: "in_progress", conclusion: null }), ], reviewThreads: [], + statusOverrides: { checksStatus: "failing", reviewStatus: "approved" }, visible: false, }, { name: "shows for unresolved review threads only", checks: [makeCheck({ conclusion: "success" })], reviewThreads: [makeThread()], - statusOverrides: { checksStatus: "passing" }, + statusOverrides: { checksStatus: "passing", reviewStatus: "approved" }, visible: true, },🤖 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.issueResolver.test.tsx` around lines 57 - 96, The visibilityCases tests rely on implicit defaults from makeStatus(), which can mask regressions—update every case in the visibilityCases array so each object explicitly sets statusOverrides (a Partial<PrStatus>) instead of leaving it undefined; for each scenario use statusOverrides with the appropriate checksStatus and reviewStatus values (e.g., "failing"/"changes_requested", "in_progress"/..., "passing"/"review_required", "passing"/"approved") so the component behavior is driven only by the provided checks (makeCheck) and reviewThreads (makeThread) fixtures rather than makeStatus() defaults.apps/desktop/src/main/services/lanes/rebaseSuggestionService.ts (1)
175-182:⚠️ Potential issue | 🟠 MajorClear deferrals when the base identity changes.
This branch still carries
existing?.deferredUntilinto the new queue-scoped state. If a lane was deferred against its direct parent, the new queue-target suggestion stays hidden until that old timer expires, so the earlier suppression-collision issue is only partially fixed.Possible fix
: { laneId: lane.id, parentLaneId: base.parentLaneId, parentHeadSha: base.parentHeadSha, behindCount, lastSuggestedAt: nowIso(), - deferredUntil: existing?.deferredUntil ?? null, + deferredUntil: null, dismissedAt: null };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/lanes/rebaseSuggestionService.ts` around lines 175 - 182, The current creation of the suggestion object copies existing?.deferredUntil blindly, which preserves deferrals even when the suggestion's base identity changes; update the logic that builds the object (the block setting laneId, parentLaneId, parentHeadSha, behindCount, lastSuggestedAt, deferredUntil, dismissedAt in rebaseSuggestionService.ts) to clear deferredUntil when the base identity changed by comparing existing?.parentLaneId and existing?.parentHeadSha to the new parentLaneId/parentHeadSha—only preserve existing.deferredUntil if both parentLaneId and parentHeadSha are unchanged, otherwise set deferredUntil to null (keep lastSuggestedAt/dismissedAt behavior unchanged).
🧹 Nitpick comments (1)
apps/desktop/src/renderer/components/prs/CreatePrModal.tsx (1)
1105-1170: Addaria-labelattributes for screen reader accessibility.The move up/down and remove buttons use
titlefor tooltips, but screen readers may not announcetitlereliably. Addingaria-labelensures these icon-only buttons are properly described.♿ Proposed accessibility fix
<button type="button" onClick={() => setQueueLaneIds((prev) => { const next = [...prev]; [next[idx - 1], next[idx]] = [next[idx], next[idx - 1]]; return next; })} style={{ display: "inline-flex", alignItems: "center", justifyContent: "center", padding: 4, background: "transparent", border: "none", color: C.textMuted, cursor: "pointer", flexShrink: 0, }} title="Move up" + aria-label="Move lane up" > <ArrowUp size={13} /> </button>Apply similar changes for the "Move down" (
aria-label="Move lane down") and "Remove" (aria-label="Remove lane from queue") buttons.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/prs/CreatePrModal.tsx` around lines 1105 - 1170, The three icon-only buttons that manipulate queue lanes (the "Move up" button, the "Move down" button, and the "Remove lane from queue" button that call setQueueLaneIds and reference queueLaneIds and laneId) need accessible labels: add aria-label attributes to each button (e.g., aria-label="Move lane up" for the ArrowUp button, aria-label="Move lane down" for the ArrowDown button, and aria-label="Remove lane from queue" for the Trash button) in addition to the existing title so screen readers can announce their purpose.
🤖 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/lanes/rebaseSuggestionService.ts`:
- Around line 100-118: resolveSuggestionBase can return queue-scoped bases
(parentLaneId like `queue:...`) but onParentHeadChanged still writes
direct-parent ids/SHAs causing queue-backed lanes to appear to have changed;
update onParentHeadChanged to call the same resolveSuggestionBase logic (or
detect when a lane has a queue override) and persist the returned parentLaneId
and parentHeadSha (including the `queue:` prefix and the comparisonRef-derived
SHA) instead of always writing the direct parent id/SHA, and ensure the same
identity check is used by listSuggestions so dismissedAt/lastSuggestedAt are not
reset for unchanged queue targets.
In `@apps/desktop/src/renderer/components/prs/prsRouteState.ts`:
- Around line 42-44: The laneId/prId/queueGroupId assignments currently use
searchParams.get(...) and hashParams.get(...) which can return an empty string
for params like "?prId=", so normalize those empty-string values to null; update
the logic that sets laneId, prId, and queueGroupId (and/or introduce a small
helper used by those assignments) to treat "" as null while preserving non-empty
values and existing null/undefined fallbacks from hashParams.get(...) and
searchParams.get(...).
---
Duplicate comments:
In `@apps/desktop/src/main/services/lanes/rebaseSuggestionService.ts`:
- Around line 175-182: The current creation of the suggestion object copies
existing?.deferredUntil blindly, which preserves deferrals even when the
suggestion's base identity changes; update the logic that builds the object (the
block setting laneId, parentLaneId, parentHeadSha, behindCount, lastSuggestedAt,
deferredUntil, dismissedAt in rebaseSuggestionService.ts) to clear deferredUntil
when the base identity changed by comparing existing?.parentLaneId and
existing?.parentHeadSha to the new parentLaneId/parentHeadSha—only preserve
existing.deferredUntil if both parentLaneId and parentHeadSha are unchanged,
otherwise set deferredUntil to null (keep lastSuggestedAt/dismissedAt behavior
unchanged).
In
`@apps/desktop/src/renderer/components/prs/detail/PrDetailPane.issueResolver.test.tsx`:
- Around line 57-96: The visibilityCases tests rely on implicit defaults from
makeStatus(), which can mask regressions—update every case in the
visibilityCases array so each object explicitly sets statusOverrides (a
Partial<PrStatus>) instead of leaving it undefined; for each scenario use
statusOverrides with the appropriate checksStatus and reviewStatus values (e.g.,
"failing"/"changes_requested", "in_progress"/..., "passing"/"review_required",
"passing"/"approved") so the component behavior is driven only by the provided
checks (makeCheck) and reviewThreads (makeThread) fixtures rather than
makeStatus() defaults.
---
Nitpick comments:
In `@apps/desktop/src/renderer/components/prs/CreatePrModal.tsx`:
- Around line 1105-1170: The three icon-only buttons that manipulate queue lanes
(the "Move up" button, the "Move down" button, and the "Remove lane from queue"
button that call setQueueLaneIds and reference queueLaneIds and laneId) need
accessible labels: add aria-label attributes to each button (e.g.,
aria-label="Move lane up" for the ArrowUp button, aria-label="Move lane down"
for the ArrowDown button, and aria-label="Remove lane from queue" for the Trash
button) in addition to the existing title so screen readers can announce their
purpose.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0bf55094-0440-4d7b-bdc3-f84a83ffff00
📒 Files selected for processing (13)
apps/desktop/src/main/services/conflicts/conflictService.tsapps/desktop/src/main/services/lanes/laneService.tsapps/desktop/src/main/services/lanes/rebaseSuggestionService.tsapps/desktop/src/main/services/prs/prService.reviewThreads.test.tsapps/desktop/src/main/services/prs/prService.tsapps/desktop/src/renderer/components/prs/CreatePrModal.test.tsxapps/desktop/src/renderer/components/prs/CreatePrModal.tsxapps/desktop/src/renderer/components/prs/detail/PrDetailPane.issueResolver.test.tsxapps/desktop/src/renderer/components/prs/prsRouteState.tsapps/desktop/src/renderer/components/prs/tabs/IntegrationTab.tsxapps/desktop/src/renderer/components/prs/tabs/QueueTab.tsxapps/desktop/src/test/setup.tsapps/ios/ADE/Resources/DatabaseBootstrap.sql
💤 Files with no reviewable changes (1)
- apps/ios/ADE/Resources/DatabaseBootstrap.sql
✅ Files skipped from review due to trivial changes (3)
- apps/desktop/src/test/setup.ts
- apps/desktop/src/renderer/components/prs/CreatePrModal.test.tsx
- apps/desktop/src/main/services/prs/prService.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/desktop/src/main/services/prs/prService.reviewThreads.test.ts
- apps/desktop/src/main/services/lanes/laneService.ts
- apps/desktop/src/main/services/conflicts/conflictService.ts
… normalize empty route params [skip ci] Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Owner
Author
Merged and closedEvery CI failure fixed. Every review comment from Copilot, Codex, and CodeRabbit (all 17 inline comments across two review passes) addressed and verified — typecheck clean, all 1334 tests passing. Fixes applied
|
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Removed