Fix duplicate partners in paginated list and API#4095
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughPartner pagination now uses a secondary ChangesPartners Pagination Fix
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: 1
🤖 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/web/tests/partners/pagination.test.ts`:
- Around line 18-30: The pagination test setup in beforeAll for
pagination.test.ts does not verify that the fixture is large enough to cover
three full pages, so it can pass without exercising the boundary case. Update
the shared setup and the page assertions around the /partners requests to
confirm the baseline contains at least 15 enrolled partners, then explicitly
assert that pages 1, 2, and 3 each return 5 rows using the existing h.init(),
http.get, baseline, and baselineIds flow so the duplicate issue is actually
covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2df3878d-6722-4523-abee-79c990942964
📒 Files selected for processing (3)
apps/web/lib/api/pagination.tsapps/web/lib/api/partners/get-partners.tsapps/web/tests/partners/pagination.test.ts
|
@coderabbitai full review, any breaking changes? |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Changed files vs origin/main =="
git diff --name-only origin/main...HEAD
echo
echo "== Diffstat =="
git diff --stat origin/main...HEAD
echo
echo "== Diff: apps/web/lib/api/pagination.ts =="
git diff --unified=60 origin/main...HEAD -- apps/web/lib/api/pagination.ts
echo
echo "== Diff: apps/web/lib/api/partners/get-partners.ts =="
git diff --unified=80 origin/main...HEAD -- apps/web/lib/api/partners/get-partners.ts
echo
echo "== Diff: apps/web/tests/partners/pagination.test.ts =="
git diff --unified=80 origin/main...HEAD -- apps/web/tests/partners/pagination.test.ts
echo
echo "== AST outline: apps/web/lib/api/pagination.ts =="
ast-grep outline apps/web/lib/api/pagination.ts --view expanded || true
echo
echo "== AST outline: apps/web/lib/api/partners/get-partners.ts =="
ast-grep outline apps/web/lib/api/partners/get-partners.ts --view expanded || true
echo
echo "== Usages of buildPaginationQuery =="
rg -n -C2 '\bbuildPaginationQuery\s*\(' apps/web
echo
echo "== Relevant slice: apps/web/lib/api/pagination.ts =="
sed -n '1,240p' apps/web/lib/api/pagination.ts
echo
echo "== Relevant slice: apps/web/lib/api/partners/get-partners.ts =="
sed -n '1,260p' apps/web/lib/api/partners/get-partners.tsLength of output: 16708 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Diff: apps/web/app/api/ai/support-chat/route.ts =="
git diff --unified=80 origin/main...HEAD -- apps/web/app/api/ai/support-chat/route.ts
echo
echo "== Diff: apps/web/lib/ai/build-system-prompt.ts =="
git diff --unified=120 origin/main...HEAD -- apps/web/lib/ai/build-system-prompt.ts
echo
echo "== Diff: apps/web/lib/ai/get-plan-comparison.ts =="
git diff --unified=120 origin/main...HEAD -- apps/web/lib/ai/get-plan-comparison.ts
echo
echo "== Diff: apps/web/lib/ai/plan-context.ts =="
git diff --unified=160 origin/main...HEAD -- apps/web/lib/ai/plan-context.ts
echo
echo "== Relevant slices of shared-pagination consumers =="
for f in \
apps/web/lib/customers/api/get-customers.ts \
apps/web/lib/api/links/get-links-for-workspace.ts \
apps/web/lib/api/commissions/get-commissions.ts
do
echo
echo "-- $f --"
sed -n '1,220p' "$f"
done
echo
echo "== Search for assumptions about exact tied-order pagination =="
rg -n -C2 'pageSize|startingAfter|endingBefore|sortBy|sortOrder|baselineIds|expectNoOverlap' apps/web/tests apps/web/libLength of output: 149453
Short answer: no obvious breaking changes to the public API contract. What I found:
One compatibility note:
So I’d classify it as:
That tie-break change is probably the right fix here, but if any client depends on the exact previous order of equal-valued rows, they could observe different pagination results. ✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/lib/api/partners/get-partners.ts (1)
57-59: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftReuse the shared pagination helper here.
get-partners.tsnow duplicates the offset-paginationtake/skip/orderBylogic and adds its ownidtiebreak, whilebuildPaginationQuerystill only returns a single-fieldorderByfor offset pagination. If partners needs that secondary sort, extend the shared helper to support it and use it here to avoid drift from the other list APIs.🤖 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/web/lib/api/partners/get-partners.ts` around lines 57 - 59, `get-partners.ts` is duplicating the offset pagination query shape instead of using the shared `buildPaginationQuery` helper. Update `buildPaginationQuery` to support an optional secondary sort/tiebreak field for offset pagination, then switch `get-partners` to call that helper so the `take`/`skip`/`orderBy` logic stays consistent with the other list APIs. Use the shared pagination symbols (`buildPaginationQuery`, the partners list query builder) to keep the sorting behavior centralized and avoid drift.
🤖 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.
Nitpick comments:
In `@apps/web/lib/api/partners/get-partners.ts`:
- Around line 57-59: `get-partners.ts` is duplicating the offset pagination
query shape instead of using the shared `buildPaginationQuery` helper. Update
`buildPaginationQuery` to support an optional secondary sort/tiebreak field for
offset pagination, then switch `get-partners` to call that helper so the
`take`/`skip`/`orderBy` logic stays consistent with the other list APIs. Use the
shared pagination symbols (`buildPaginationQuery`, the partners list query
builder) to keep the sorting behavior centralized and avoid drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d4c0bf31-6f33-4b7c-b6c7-bb1dc95524bb
📒 Files selected for processing (2)
apps/web/lib/api/pagination.tsapps/web/lib/api/partners/get-partners.ts
✅ Files skipped from review due to trivial changes (1)
- apps/web/lib/api/pagination.ts
1 participant
Summary by CodeRabbit
/partnersoffset pagination to return results in a deterministic, stable order across pages by adding anidtie-breaker when multiple records share the same sort value./partnerspagination that request multiple offset pages, verify item counts, ensure no overlap between pages, and confirm the combined ordering matches the expected baseline sequence.