prs faster redirects -> Primary - #1053
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Warning Review limit reached
Next review available in: 33 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)
📝 WalkthroughWalkthroughChangesCoordinate-based PR deep links
Estimated code review effort: 5 (Critical) | ~120 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/components/lanes/LanesPage.tsx (1)
3339-3355: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRoute GitHub-only lane badges into the GitHub PR flow.
When
target.linkedPrIdis null, this handler opens GitHub externally. GitHub-only tags now carrygithubPrNumber,repoOwner, andrepoName, and the GitHub tab supports coordinate-only and provisional selection. This bypasses the new in-app deep-link flow.For GitHub-only tags, navigate to the GitHub PR tab with a null local ID and the repository coordinates. Keep the external URL as a separate action if needed. Add a named regression test such as
opens an unmapped lane PR badge in the GitHub tab by coordinates.As per coding guidelines,
**/*.test.{ts,tsx}must 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.tsx` around lines 3339 - 3355, Update the onActivate handler for GitHub-only targets in the lane PR badge flow: when target.linkedPrId is null but githubPrNumber, repoOwner, and repoName are available, navigate to the GitHub PR tab using a null local ID and those repository coordinates instead of opening the URL externally. Keep external URL opening as a separate fallback action, and add a named regression test such as “opens an unmapped lane PR badge in the GitHub tab by coordinates.”Source: Coding guidelines
🧹 Nitpick comments (3)
apps/desktop/src/renderer/components/prs/PRsPage.tsx (1)
291-322: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the duplicated
localSelectedPrfallback.
targetalready falls back tolocalSelectedPrat Line 291. The extralocalSelectedPr?.…fallbacks at Lines 320-322 can therefore only run whentargetis null, in which case both branches yieldnull. The branches add no behavior.♻️ Proposed simplification
- selectedPrNumber: hasPrNumber ? target?.prNumber : localSelectedPr?.githubPrNumber ?? null, - repoOwner: hasCoordinates ? target?.repoOwner : localSelectedPr?.repoOwner ?? null, - repoName: hasCoordinates ? target?.repoName : localSelectedPr?.repoName ?? null, + selectedPrNumber: target?.prNumber ?? null, + repoOwner: hasCoordinates ? target?.repoOwner ?? null : null, + repoName: hasCoordinates ? target?.repoName ?? null : null,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/components/prs/PRsPage.tsx` around lines 291 - 322, In the buildPrsRouteSearch argument construction, remove the redundant localSelectedPr fallbacks for selectedPrNumber, repoOwner, and repoName. Reuse the already-resolved target values, returning null when target lacks those fields, while preserving the existing coordinate checks and routing behavior.apps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsx (2)
451-471: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
prsByCoordinateMapinstead of a linear scan.
prsByCoordinateMap(Lines 187-193) is keyed bygithubCoordKey, which callsprRouteCoordinatesKey.prRouteCoordinatesEqualis defined as equality of those same keys. Theprs.find(...)scan at Lines 459-470 is therefore equivalent to one map lookup, but runs in O(n) on every recompute and duplicates the normalization logic.♻️ Proposed refactor
const selectedLocalPr = React.useMemo((): PrWithConflicts | null => { if (!selectedItem) return null; if (selectedItem.linkedPrId) { const linked = prsByIdMap.get(selectedItem.linkedPrId); if (linked) return linked; } // Prefer the local row by GitHub coordinates when a stale snapshot has lost // its link (or carries a foreign machine's link id). - return prs.find((pr) => prRouteCoordinatesEqual( - { - prNumber: pr.githubPrNumber, - repoOwner: pr.repoOwner, - repoName: pr.repoName, - }, - { - prNumber: selectedItem.githubPrNumber, - repoOwner: selectedItem.repoOwner, - repoName: selectedItem.repoName, - }, - )) ?? null; - }, [prs, prsByIdMap, selectedItem]); + return prsByCoordinateMap.get(githubCoordKey(selectedItem)) ?? null; + }, [prsByCoordinateMap, prsByIdMap, selectedItem]);If
prRouteCoordinatesEqualbecomes unused in this file after the change, remove its import.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsx` around lines 451 - 471, Update selectedLocalPr to use prsByCoordinateMap with the selected item's GitHub coordinate key instead of scanning prs with prs.find and prRouteCoordinatesEqual. Preserve the existing linked-pr lookup precedence and null fallback, and remove the prRouteCoordinatesEqual import if it becomes unused.
499-511: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the memo comment and dependency list.
The comment states that
prs[0]?.projectIdis captured at build time and thatsyntheticUnmappedIdkeys identity. The dependency array still listsprs, so the memo recomputes on everyprschange regardless. Either dropprsfrom the deps (keeping the eslint-disable and the stated intent), or keepprsand remove the disable comment plus the unusedsyntheticUnmappedIddependency. The current mix is misleading for future readers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsx` around lines 499 - 511, Align the selectedUnmappedPr React.useMemo dependencies with its explanatory comment: remove prs from the dependency array while retaining syntheticUnmappedId as the identity key and preserving the eslint suppression, so changes to prs do not trigger recomputation after build time.
🤖 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/prs/detail/PrDetailTimelineRails.tsx`:
- Around line 826-852: Preserve hash-based PR routes in handleVisibleEventChange
by detecting when location.hash contains the authoritative `#/prs` route and
updating or reconstructing that hash with the replacement eventId instead of
navigating only with pathname and search. Keep pathname/search navigation
unchanged for non-hash routes, and add a named regression test such as
“preserves a hash-based coordinate PR route when the visible event changes”
asserting the route retains `#/prs` and the replacement eventId.
In `@apps/desktop/src/renderer/components/prs/PRsPage.tsx`:
- Around line 232-262: Add component-test coverage in PRsPage.test.tsx for
reconciling a coordinate-only route containing pr, repoOwner, and repoName
without prId, asserting the route target and matching local PR selection are
handled correctly. Also cover the workflow-route branch around
setSelectedPrTarget so a non-null target is cleared when the route no longer has
an explicit PR selection. Keep the existing error-gating coverage unchanged.
In `@apps/desktop/src/renderer/components/prs/tabs/githubTabModel.ts`:
- Around line 224-239: Update itemMatchesSelectionTarget to require
target.repoOwner and target.repoName when matching repository coordinates,
preventing coordinate-less targets from matching externalPullRequests rows. When
target.prId is provided, return only the matching itemId with valid coordinates;
do not allow the number-based fallback to override an ID mismatch. For
coordinate-less prNumber targets, restrict the fallback to items whose scope is
"repo".
In `@apps/desktop/src/renderer/components/prs/tabs/useGitHubTabSelection.ts`:
- Around line 62-63: Update the selectedPrTarget resolution around
itemMatchesSelectionTarget so number-only targets are not resolved to the first
matching row: require repository coordinates for coordinate-based selection, or
resolve the number-only target only when exactly one displayed item matches.
Ensure ambiguous matches remain unresolved, and add a named regression test such
as does not resolve an ambiguous number-only PR route across repositories.
In `@apps/desktop/src/renderer/components/prs/tabs/useGitHubTargetHistory.ts`:
- Around line 97-129: Update requestHistory so the partial-history paging branch
only calls requestHistory when loaded.history.pageLimit is greater than the
historyPageLimit requested for the current fetch; otherwise stop paging and
invoke the existing 30-second retry path via scheduleRetry. Preserve the
existing max-page-limit handling and clearRetry behavior when history is
complete.
---
Outside diff comments:
In `@apps/desktop/src/renderer/components/lanes/LanesPage.tsx`:
- Around line 3339-3355: Update the onActivate handler for GitHub-only targets
in the lane PR badge flow: when target.linkedPrId is null but githubPrNumber,
repoOwner, and repoName are available, navigate to the GitHub PR tab using a
null local ID and those repository coordinates instead of opening the URL
externally. Keep external URL opening as a separate fallback action, and add a
named regression test such as “opens an unmapped lane PR badge in the GitHub tab
by coordinates.”
---
Nitpick comments:
In `@apps/desktop/src/renderer/components/prs/PRsPage.tsx`:
- Around line 291-322: In the buildPrsRouteSearch argument construction, remove
the redundant localSelectedPr fallbacks for selectedPrNumber, repoOwner, and
repoName. Reuse the already-resolved target values, returning null when target
lacks those fields, while preserving the existing coordinate checks and routing
behavior.
In `@apps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsx`:
- Around line 451-471: Update selectedLocalPr to use prsByCoordinateMap with the
selected item's GitHub coordinate key instead of scanning prs with prs.find and
prRouteCoordinatesEqual. Preserve the existing linked-pr lookup precedence and
null fallback, and remove the prRouteCoordinatesEqual import if it becomes
unused.
- Around line 499-511: Align the selectedUnmappedPr React.useMemo dependencies
with its explanatory comment: remove prs from the dependency array while
retaining syntheticUnmappedId as the identity key and preserving the eslint
suppression, so changes to prs do not trigger recomputation after build time.
🪄 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: c7823c37-f8a1-4e7b-905d-40235696c3d3
📒 Files selected for processing (32)
apps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/app/App.tsxapps/desktop/src/renderer/components/app/AppShell.tsxapps/desktop/src/renderer/components/chat/ChatGitToolbar.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/prs/PRsPage.tsxapps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsxapps/desktop/src/renderer/components/prs/detail/PrDetailTimelineRails.test.tsxapps/desktop/src/renderer/components/prs/detail/PrDetailTimelineRails.tsxapps/desktop/src/renderer/components/prs/prsRouteState.test.tsapps/desktop/src/renderer/components/prs/prsRouteState.tsapps/desktop/src/renderer/components/prs/state/PrsContext.test.tsxapps/desktop/src/renderer/components/prs/state/PrsContext.tsxapps/desktop/src/renderer/components/prs/tabs/GitHubTab.test.tsxapps/desktop/src/renderer/components/prs/tabs/GitHubTab.testHarness.tsxapps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsxapps/desktop/src/renderer/components/prs/tabs/GitHubTabCreateLaneDialog.tsxapps/desktop/src/renderer/components/prs/tabs/GitHubTabDeepLinks.test.tsxapps/desktop/src/renderer/components/prs/tabs/GitHubTabRowsAndMapping.test.tsxapps/desktop/src/renderer/components/prs/tabs/GitHubTabView.tsxapps/desktop/src/renderer/components/prs/tabs/githubTabModel.tsapps/desktop/src/renderer/components/prs/tabs/useGitHubTabListModel.tsapps/desktop/src/renderer/components/prs/tabs/useGitHubTabSelection.tsapps/desktop/src/renderer/components/prs/tabs/useGitHubTargetHistory.tsapps/desktop/src/renderer/components/terminals/LanePrBadge.test.tsxapps/desktop/src/renderer/components/terminals/SessionCard.test.tsxapps/desktop/src/renderer/components/terminals/useLanePrs.tsapps/desktop/src/renderer/lib/lanePrBadge.test.tsapps/desktop/src/renderer/lib/lanePrBadge.ts
1 participant
Summary by CodeRabbit