fix: repair Cloudflare device cursor pagination#1941
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 4 minutes and 57 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe pull request refactors query construction in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/cloudflare-device-pagination.unit.test.ts (1)
5-5: Preferit.concurrent()for these independent unit tests.Both cases are pure query-string assertions and can run safely in parallel.
♻️ Suggested change
- it('applies descending cursor pagination after device grouping', () => { + it.concurrent('applies descending cursor pagination after device grouping', () => { @@ - it('applies ascending cursor pagination after device grouping', () => { + it.concurrent('applies ascending cursor pagination after device grouping', () => {As per coding guidelines, "Use
it.concurrent()instead ofit()when possible to run tests in parallel within the same file, maximizing parallelism for faster CI/CD".Also applies to: 22-22
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cloudflare-device-pagination.unit.test.ts` at line 5, Replace the two independent unit tests that use it(...) with it.concurrent(...) so they run in parallel; specifically change the test with the description "applies descending cursor pagination after device grouping" and the other standalone test at the second occurrence in this file to use it.concurrent(...) instead of it(...), preserving the test bodies and assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@supabase/functions/_backend/utils/cloudflare.ts`:
- Around line 708-714: The cloudlog calls currently emit raw params.deviceIds
and params.search; instead redact or summarize these sensitive fields before
logging—modify the block that calls cloudlog (referencing cloudlog,
params.deviceIds, params.search and c.get('requestId')) to log non-sensitive
representations such as deviceIds.length or a hashed/first-N-masked sample and a
truncated or sanitized summary of params.search (e.g., "<redacted>" or first 20
chars + ellipsis), ensuring no raw identifiers or user input are written to logs
while preserving useful context like the requestId.
In `@tests/cloudflare-device-pagination.unit.test.ts`:
- Around line 13-20: Add explicit assertions to the test that verify the
secondary tie-breaker on device_id is present in the cursor predicate: after
computing query, assert that the query string contains the `AND device_id >`
clause (e.g., `AND device_id > '...') and that this clause appears after the
cursor predicate (use the existing `cursorIndex` / `groupByIndex` logic if
helpful). Update both occurrences noted in the comment (the current block around
the `groupByIndex`/`cursorIndex` assertions and the analogous checks at lines
30–31) so the test fails if the `device_id` tie-breaker is omitted.
---
Nitpick comments:
In `@tests/cloudflare-device-pagination.unit.test.ts`:
- Line 5: Replace the two independent unit tests that use it(...) with
it.concurrent(...) so they run in parallel; specifically change the test with
the description "applies descending cursor pagination after device grouping" and
the other standalone test at the second occurrence in this file to use
it.concurrent(...) instead of it(...), preserving the test bodies and
assertions.
🪄 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: 0ff7a171-d834-4d7b-8aa7-63cda2424fb7
📒 Files selected for processing (2)
supabase/functions/_backend/utils/cloudflare.tstests/cloudflare-device-pagination.unit.test.ts
|
1 participant
Summary (AI generated)
/private/devicescursor pagination sonextCursoradvances after per-device groupingMotivation (AI generated)
The Cloudflare/workerd implementation applied cursor filtering before the per-device aggregation, which could repeat the same device on the next page and strand later rows behind a non-advancing cursor.
Business Impact (AI generated)
This removes a production pagination defect in device administration flows, avoiding duplicate-page loops, unreachable devices, and wasted operational work in exports or analytics workflows.
Test Plan (AI generated)
bunx eslint supabase/functions/_backend/utils/cloudflare.ts tests/cloudflare-device-pagination.unit.test.tsbunx vitest run tests/cloudflare-device-pagination.unit.test.ts tests/cloudflare-datetime.unit.test.tsGenerated with AI
Summary by CodeRabbit
Refactor
Tests