Multi-PR Lane Links -> Primary - #1046
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Warning Review limit reached
Next review available in: 31 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughChangesChat-linked pull requests and lane history
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
There was a problem hiding this comment.
Actionable comments posted: 6
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/renderer/components/chat/ChatGitToolbar.tsx (1)
234-247: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBoth PR surfaces subscribe to the whole
lanesarray to read one lane. Each component needs lane metadata forselectPrimaryLanePr, and each addeduseAppStore((s) => s.lanes)plus alanesentry in its refresh callback dependency list. The store replaces that array on every lane refresh and cross-machine merge, so both refresh callbacks change identity on a timer, which resets PR state and re-creates theprs.onEventsubscriptions. Both files already solve this class of problem forruntimePinby keying onruntimePinKeyand reading the object through a ref.
apps/desktop/src/renderer/components/chat/ChatGitToolbar.tsx#L234-L247: select the single lane bylaneId, read it inrefreshPrthrough a ref, and replacelanesin the dependency list with a primitive key built from the lane fieldsselectPrimaryLanePruses (laneType,branchRef,baseRef).apps/desktop/src/renderer/components/chat/ChatPrPane.tsx#L444-L519: apply the same change torefresh, so the reconcile subscription at lines 555-580 and theprs-updatedsubscription at lines 593-610 keep the stable-dependency guarantee their comments describe.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/components/chat/ChatGitToolbar.tsx` around lines 234 - 247, In apps/desktop/src/renderer/components/chat/ChatGitToolbar.tsx lines 234-247, replace the whole-lanes subscription with the single lane selected by laneId, read that lane through a ref inside refreshPr, and replace lanes in its dependencies with a primitive key containing laneType, branchRef, and baseRef. Apply the same change to refresh in apps/desktop/src/renderer/components/chat/ChatPrPane.tsx lines 444-519; the later reconcile and prs-updated subscriptions require no direct changes and must retain stable dependencies.apps/desktop/src/main/services/prs/prChatCards.ts (1)
543-549: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the failure count denominator after the fan-out change.
resultsnow holdssessions.length * cards.lengthentries, but the message still divides bycards.length. With 2 sessions and 1 card, a total failure reports "Failed to emit 2 of 1 PR chat cards."🐛 Proposed fix
const failures = results.filter((result) => result.status === "rejected"); if (failures.length > 0) { throw new AggregateError( failures.map((failure) => failure.reason), - `Failed to emit ${failures.length} of ${cards.length} PR chat cards.`, + `Failed to emit ${failures.length} of ${results.length} PR chat cards.`, ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/prs/prChatCards.ts` around lines 543 - 549, Update the AggregateError message in the fan-out result handling to use the total number of emitted card-session operations, based on results.length, as the denominator instead of cards.length. Keep the existing failure collection and error propagation unchanged.
🧹 Nitpick comments (7)
apps/desktop/src/renderer/components/chat/ChatPrInlineCreator.test.tsx (1)
182-182: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case that asserts
sessionIdis omitted when no chat owns the creator.The implementation uses a conditional spread, so the key must be absent rather than
nullfor non-chat surfaces such as the Work grid. Only the positive case is covered. Add a second case that renders withoutsessionIdand assertsexpect(createFromLane.mock.calls[0][0]).not.toHaveProperty("sessionId").As per coding guidelines: "Record a named regression test or exact alternate verification for every accepted correctness finding."
Also applies to: 197-197
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/components/chat/ChatPrInlineCreator.test.tsx` at line 182, Add a named regression test alongside the existing positive case in the ChatPrInlineCreator tests that renders without sessionId, triggers creation, and asserts the createFromLane payload does not have a sessionId property. Preserve the existing session-owned case and use the exact absence assertion requested.Source: Coding guidelines
apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts (1)
96-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the linked-chat-id normalization into one shared helper.
The same normalization now exists here and in
prChatCards.ts(lines 199-201), andprChatScope.tsperforms an unnormalizedincludescheck on the same field. Three copies of one contract will drift. A single exported helper, for examplenormalizeLinkedChatSessionIds(pr.chatSessionIds), keeps trimming rules identical everywhere.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts` around lines 96 - 98, Extract the chat-session ID normalization currently used in the auto-settlement flow into one exported helper, such as normalizeLinkedChatSessionIds, and reuse it in prMergeAutoSettlementService, prChatCards, and prChatScope. Ensure the helper preserves the existing null-safe conversion, trimming, and empty-value filtering, and update prChatScope’s membership check to use the normalized IDs.apps/desktop/src/renderer/components/chat/ChatGitToolbar.test.tsx (1)
140-149: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd component-level coverage for the new
sessionIdprop.This test exercises the pure helper only. The changed behavior in
ChatGitToolbar.tsxis broader:refreshPrnow callswindow.ade.prs.listAll, filters detached rows, appliesselectPrsForChat, and picks a primary PR withselectPrimaryLanePr; theprs-updatedhandler also filters by chat membership. None of that is covered.Add a render test that mounts
ChatGitToolbarwithsessionIdset and a mockedwindow.ade.prs.listAllreturning two lane PRs with differentchatSessionIds, then assert that the pill shows the linked PR and that the+Ncounter appears only when more than one scoped PR exists.As per coding guidelines: "Record a named regression test or exact alternate verification for every accepted correctness finding."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/components/chat/ChatGitToolbar.test.tsx` around lines 140 - 149, Add a component-level regression test for ChatGitToolbar that renders it with sessionId, mocks window.ade.prs.listAll to return lane PRs assigned to different chatSessionIds, and verifies only the current chat’s PR appears in the pill. Also verify the +N counter is absent for one scoped PR and appears when multiple PRs belong to the session, covering refreshPr and prs-updated filtering behavior.Source: Coding guidelines
apps/desktop/src/renderer/components/lanes/LanePrBadgePopover.tsx (1)
101-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the duplicated attention reduce.
Lines 102-104 repeat the reduce from lines 96-98 with the same inputs, so
aggregatealways equalsprimaryPr. Derive the color fromprimaryPr.♻️ Proposed refactor
if (allPrs.length > 1) { - const aggregate = allPrs.reduce((best, candidate) => ( - lanePrAttentionRank(candidate) > lanePrAttentionRank(best) ? candidate : best - ), allPrs[0]!); - const aggregateColor = lanePrAttentionColor(lanePrAttention(aggregate)); + const aggregateColor = lanePrAttentionColor(lanePrAttention(primaryPr));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/components/lanes/LanePrBadgePopover.tsx` around lines 101 - 105, Remove the redundant reduce in the allPrs aggregation block and derive aggregateColor directly from primaryPr using lanePrAttentionColor and lanePrAttention.apps/desktop/src/renderer/components/lanes/lanePageModel.ts (1)
448-474: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCompute each GitHub match once.
Line 451 and line 469 run the same
laneGithubPrs.find((candidate) => githubPrMatchesAdePr(mappedPr, candidate))lookup for every mapped PR. One pass can build both the tags and the matched-id set.♻️ Proposed refactor
- const mappedTags = mappedPrs.map((mappedPr) => { - const githubPr = laneGithubPrs.find((candidate) => githubPrMatchesAdePr(mappedPr, candidate)) ?? null; + const githubPrByMappedPrId = new Map<string, GitHubPrListItem>(); + for (const mappedPr of mappedPrs) { + const match = laneGithubPrs.find((candidate) => githubPrMatchesAdePr(mappedPr, candidate)); + if (match) githubPrByMappedPrId.set(mappedPr.id, match); + } + const mappedTags = mappedPrs.map((mappedPr) => { + const githubPr = githubPrByMappedPrId.get(mappedPr.id) ?? null; // The PrSummary carries diff/checks/reviews; the matching GitHub item carries // labels/author. Merge so the popover card has the richest data available. @@ - const mappedGithubKeys = new Set(mappedPrs.map((mappedPr) => ( - laneGithubPrs.find((candidate) => githubPrMatchesAdePr(mappedPr, candidate))?.id - )).filter((id): id is string => Boolean(id))); + const mappedGithubKeys = new Set([...githubPrByMappedPrId.values()].map((item) => item.id));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/components/lanes/lanePageModel.ts` around lines 448 - 474, Refactor the mapped PR processing around mappedTags and mappedGithubKeys to perform each laneGithubPrs.find match only once per mappedPr. Reuse the computed GitHub match both when constructing the mapped tag and when collecting matched IDs, while preserving the existing terminal-update, preference, merge, and unmapped-tag behavior.apps/desktop/src/renderer/components/lanes/LanesPage.test.ts (1)
382-399: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the combined multi-tag selection.
These tests cover
selectLanePrsandlanePrRole. The newselectLaneTabPrTagsbehavior is not covered: it merges mapped ADE PRs with unmatched GitHub PRs, assignslaneRole, and orders results withcompareLaneTabPrTags. Add a case with one active mapped PR, one previous mapped PR, and one GitHub-only PR, then assert the returned order and eachlaneRole.As per path instructions: "Record a named regression test or exact alternate verification for every accepted correctness finding."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/components/lanes/LanesPage.test.ts` around lines 382 - 399, Extend the existing lane PR test coverage with a regression case for selectLaneTabPrTags using one active mapped PR, one previous mapped PR, and one unmatched GitHub PR. Assert the merged result order from compareLaneTabPrTags and verify each returned entry has the expected laneRole, including the GitHub-only entry.Source: Path instructions
apps/desktop/src/renderer/components/lanes/LanesPage.tsx (1)
644-659: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDerive the primary map from the tags map.
Both memos call
selectLaneTabPrTags(lane, lanePrTags, laneGithubPrTags)with identical arguments, so the full per-lane selection runs twice for every lane on each PR data change. Compute the tags map first, then take the first entry of each list.♻️ Proposed refactor
- const lanePrByLaneId = useMemo(() => { - const map = new Map<string, LaneTabPrTag>(); - for (const lane of sortedLanes) { - const pr = selectLaneTabPrTags(lane, lanePrTags, laneGithubPrTags)[0] ?? null; - if (pr) map.set(lane.id, pr); - } - return map; - }, [sortedLanes, lanePrTags, laneGithubPrTags]); const lanePrTagsByLaneId = useMemo(() => { const map = new Map<string, LaneTabPrTag[]>(); for (const lane of sortedLanes) { const tags = selectLaneTabPrTags(lane, lanePrTags, laneGithubPrTags); if (tags.length > 0) map.set(lane.id, tags); } return map; }, [sortedLanes, lanePrTags, laneGithubPrTags]); + const lanePrByLaneId = useMemo(() => { + const map = new Map<string, LaneTabPrTag>(); + for (const [laneId, tags] of lanePrTagsByLaneId) { + const pr = tags[0]; + if (pr) map.set(laneId, pr); + } + return map; + }, [lanePrTagsByLaneId]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/components/lanes/LanesPage.tsx` around lines 644 - 659, Update the lanePrByLaneId and lanePrTagsByLaneId memoization flow so selectLaneTabPrTags is invoked only while building lanePrTagsByLaneId; derive lanePrByLaneId from that tags map by selecting each lane’s first tag, preserving the existing omission of lanes without tags and the memoization dependencies.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx`:
- Line 11801: Update the PR scoping prop at the shown call site to use
renderedSessionId instead of selectedSessionId, and apply the same replacement
to the ChatPrPane sessionId flow that reaches ChatPrInlineCreator and
prs.createFromLane. Preserve selectedSessionId for unrelated internal state
usage while ensuring header, PR pane, and PR creation target the currently
rendered chat during prop-driven switches.
In `@apps/desktop/src/renderer/components/chat/ChatPrPane.tsx`:
- Line 444: Replace the full lanes-array subscription used by refresh with a
selector for the current lane’s stable identifying fields, such as laneId and
projectRoot, following the existing runtimePinKey pattern. Update refresh and
its dependency inputs so lane-array refreshes do not change refresh identity,
preserving the event subscriptions’ stable-dependency invariant while retaining
updates when the selected lane’s stable fields change.
In `@apps/desktop/src/renderer/components/lanes/LanesPage.tsx`:
- Around line 887-890: Bound the PR IDs used by the refresh flow around
matchedPrIds and refreshPrsCoalesced to active PRs plus a small historical
window, rather than every ID returned by selectLanePrs across sortedLanesRef.
Preserve deduplication and the existing early return when no IDs remain, and
apply the cap either before calling refreshPrsCoalesced or within that
coalescer.
In `@apps/desktop/src/renderer/components/terminals/LanePrBadge.tsx`:
- Around line 166-175: Update the status container in LanePrBadge.tsx to use a
semantic status element with an accessible name that includes the specific CI
and review values, such as “CI failing; review changes requested,” rather than
exposing only the generic label. In LanePrBadge.test.tsx, add a named regression
test that queries and verifies this specific combined accessible name.
In `@apps/desktop/src/renderer/components/terminals/useLanePrs.ts`:
- Around line 74-80: Update the GitHub PR lookup in the mapped PR transformation
within useLanePrs to match by both repository identity (owner/repo) and
githubPrNumber, consistent with the identity logic used later in the function.
Preserve the existing stack fallback chain and null behavior after selecting the
repository-aware match.
In `@apps/ios/ADE/Services/Database.swift`:
- Around line 2982-2996: Remove the duplicate pull_request_chat_sessions table
creation and its three indexes from ensureHydrationProjectionColumns, keeping
the canonical definitions in ensurePullRequestProjectionTables. Leave the
worker_agents linear_identity_json column migration unchanged.
---
Outside diff comments:
In `@apps/desktop/src/main/services/prs/prChatCards.ts`:
- Around line 543-549: Update the AggregateError message in the fan-out result
handling to use the total number of emitted card-session operations, based on
results.length, as the denominator instead of cards.length. Keep the existing
failure collection and error propagation unchanged.
In `@apps/desktop/src/renderer/components/chat/ChatGitToolbar.tsx`:
- Around line 234-247: In
apps/desktop/src/renderer/components/chat/ChatGitToolbar.tsx lines 234-247,
replace the whole-lanes subscription with the single lane selected by laneId,
read that lane through a ref inside refreshPr, and replace lanes in its
dependencies with a primitive key containing laneType, branchRef, and baseRef.
Apply the same change to refresh in
apps/desktop/src/renderer/components/chat/ChatPrPane.tsx lines 444-519; the
later reconcile and prs-updated subscriptions require no direct changes and must
retain stable dependencies.
---
Nitpick comments:
In `@apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts`:
- Around line 96-98: Extract the chat-session ID normalization currently used in
the auto-settlement flow into one exported helper, such as
normalizeLinkedChatSessionIds, and reuse it in prMergeAutoSettlementService,
prChatCards, and prChatScope. Ensure the helper preserves the existing null-safe
conversion, trimming, and empty-value filtering, and update prChatScope’s
membership check to use the normalized IDs.
In `@apps/desktop/src/renderer/components/chat/ChatGitToolbar.test.tsx`:
- Around line 140-149: Add a component-level regression test for ChatGitToolbar
that renders it with sessionId, mocks window.ade.prs.listAll to return lane PRs
assigned to different chatSessionIds, and verifies only the current chat’s PR
appears in the pill. Also verify the +N counter is absent for one scoped PR and
appears when multiple PRs belong to the session, covering refreshPr and
prs-updated filtering behavior.
In `@apps/desktop/src/renderer/components/chat/ChatPrInlineCreator.test.tsx`:
- Line 182: Add a named regression test alongside the existing positive case in
the ChatPrInlineCreator tests that renders without sessionId, triggers creation,
and asserts the createFromLane payload does not have a sessionId property.
Preserve the existing session-owned case and use the exact absence assertion
requested.
In `@apps/desktop/src/renderer/components/lanes/lanePageModel.ts`:
- Around line 448-474: Refactor the mapped PR processing around mappedTags and
mappedGithubKeys to perform each laneGithubPrs.find match only once per
mappedPr. Reuse the computed GitHub match both when constructing the mapped tag
and when collecting matched IDs, while preserving the existing terminal-update,
preference, merge, and unmapped-tag behavior.
In `@apps/desktop/src/renderer/components/lanes/LanePrBadgePopover.tsx`:
- Around line 101-105: Remove the redundant reduce in the allPrs aggregation
block and derive aggregateColor directly from primaryPr using
lanePrAttentionColor and lanePrAttention.
In `@apps/desktop/src/renderer/components/lanes/LanesPage.test.ts`:
- Around line 382-399: Extend the existing lane PR test coverage with a
regression case for selectLaneTabPrTags using one active mapped PR, one previous
mapped PR, and one unmatched GitHub PR. Assert the merged result order from
compareLaneTabPrTags and verify each returned entry has the expected laneRole,
including the GitHub-only entry.
In `@apps/desktop/src/renderer/components/lanes/LanesPage.tsx`:
- Around line 644-659: Update the lanePrByLaneId and lanePrTagsByLaneId
memoization flow so selectLaneTabPrTags is invoked only while building
lanePrTagsByLaneId; derive lanePrByLaneId from that tags map by selecting each
lane’s first tag, preserving the existing omission of lanes without tags and the
memoization dependencies.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 516457d1-b59f-4c18-83cb-b86e5d65ba2e
⛔ Files ignored due to path filters (4)
docs/features/lanes/README.mdis excluded by!docs/**docs/features/pull-requests/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/ios-companion.mdis excluded by!docs/**
📒 Files selected for processing (38)
apps/ade-cli/src/services/sync/syncRemoteCommandService.tsapps/desktop/src/main/services/lanes/laneService.test.tsapps/desktop/src/main/services/lanes/laneService.tsapps/desktop/src/main/services/prs/prAsync.test.tsapps/desktop/src/main/services/prs/prChatCards.test.tsapps/desktop/src/main/services/prs/prChatCards.tsapps/desktop/src/main/services/prs/prMergeAutoSettlementService.tsapps/desktop/src/main/services/prs/prService.test.tsapps/desktop/src/main/services/prs/prService.tsapps/desktop/src/main/services/prs/pullRequestRowCleanup.tsapps/desktop/src/main/services/state/kvDb.tsapps/desktop/src/main/services/sync/syncRemoteCommandService.test.tsapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/ChatGitToolbar.test.tsxapps/desktop/src/renderer/components/chat/ChatGitToolbar.tsxapps/desktop/src/renderer/components/chat/ChatPrInlineCreator.test.tsxapps/desktop/src/renderer/components/chat/ChatPrInlineCreator.tsxapps/desktop/src/renderer/components/chat/ChatPrPane.tsxapps/desktop/src/renderer/components/lanes/LanePrBadgePopover.tsxapps/desktop/src/renderer/components/lanes/LaneWorkPane.tsxapps/desktop/src/renderer/components/lanes/LanesPage.test.tsapps/desktop/src/renderer/components/lanes/LanesPage.tsxapps/desktop/src/renderer/components/lanes/lanePageModel.tsapps/desktop/src/renderer/components/terminals/CliSessionWorkSurfaceHeader.tsxapps/desktop/src/renderer/components/terminals/LanePrBadge.test.tsxapps/desktop/src/renderer/components/terminals/LanePrBadge.tsxapps/desktop/src/renderer/components/terminals/SessionCard.tsxapps/desktop/src/renderer/components/terminals/SessionListPane.tsxapps/desktop/src/renderer/components/terminals/useLanePrs.test.tsapps/desktop/src/renderer/components/terminals/useLanePrs.tsapps/desktop/src/renderer/components/work/WorkSurfaceHeader.tsxapps/desktop/src/renderer/lib/lanePrBadge.test.tsapps/desktop/src/renderer/lib/lanePrBadge.tsapps/desktop/src/renderer/lib/prChatScope.tsapps/desktop/src/shared/types/prs.tsapps/ios/ADE/Resources/DatabaseBootstrap.sqlapps/ios/ADE/Services/Database.swiftapps/ios/ADE/Services/SyncService.swift
Summary by CodeRabbit
New Features
Bug Fixes