feat: added database pagination#725
Conversation
|
CodeAnt AI is reviewing your PR. |
|
@madsysharma is attempting to deploy a commit to the Karan Mani Tripathi 's projects Team on Vercel. A member of the Team first needs to authorize it. |
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
@coderabbitai review |
WalkthroughAdds cursor-based pagination for the doubts feed, with new cursor helpers, query-path branching in ChangesDoubts Feed Pagination
Test Updates
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
|
Hi @knoxiboy , please review this PR. Thank you. |
|
CodeAnt AI finished reviewing your PR. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/app/api/doubts/route.ts`:
- Around line 230-250: In the cursor pagination path of doubts/route.ts, the
current use of query.limit(limit) in the useCursor branch can miscompute hasMore
and produce a dead next cursor when the last page is exactly full. Update the
cursor-mode logic in the doubts query and the subsequent nextCursor/hasMore
handling to fetch limit + 1 rows, use the extra row to decide whether another
page exists, then trim the results back to limit before returning and
constructing the cursor.
- Around line 210-228: The pagination logic in the doubts route mixes two
different orderings: offset mode uses pinned-first sorting while cursor mode
resumes with only createdAt/id, so do not emit nextCursor from responses ordered
by the offset path. Update the route handler in the doubts API so nextCursor is
only returned when useCursor is already true, or change the offset and cursor
branches to share the exact same ordering in orderByFields and the cursor
creation logic. Make the fix around the pagination response near the nextCursor
handling to keep the ordering consistent.
In `@src/configs/schema.ts`:
- Around line 215-228: The new filtered indexes in schema.ts for
classroomTypeIndex and classroomSolvedIndex do not include the recency key used
by the feed query. Update the index definitions in the doubts schema so the
composite keys match GET /api/doubts filtering and ordering, using classroomId +
type/isSolved + createdAt with createdAt last; then mirror the same composite
shape in drizzle/0016_doubts_composite_indexes.sql so the migration matches the
schema.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 01ae8a17-39e3-407d-bc85-01f1603becb7
📒 Files selected for processing (6)
drizzle/0016_doubts_composite_indexes.sqldrizzle/meta/_journal.jsonsrc/__tests__/lib/pagination.test.tssrc/app/api/doubts/route.tssrc/configs/schema.tssrc/lib/pagination.ts
|
@madsysharma look into ci test failures and try to fix that |
Refactor cursor-based pagination logic for clarity and correctness, ensuring proper handling of cursor tokens and sentinel rows.
Fix missing newline at the end of the file.
Added a newline at the end of the pagination test file.
Added composite indexes for filtering and ordering doubts feed.
56d6547 to
743a40c
Compare
|
CodeAnt AI is running Incremental review |
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
CodeAnt AI Incremental review completed. |
|
Hi @knoxiboy , have included fixes for the CI failures - although the remaining failure is coming from DB migration (duplicate ID), which has nothing to do with my changes. Please review this new PR. Thank you. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/app/api/doubts/[id]/accept/route.test.ts (1)
40-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the select mock branch on query shape, not call count.
mockSelectCallCountonly works while the route keeps the same twoselects in the same order; if anotherselectis added or the lookups are reordered, the test will return the wrong fixture instead of failing. Key off the.from()target (or another query-specific field) so each lookup resolves to the right response.🤖 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 `@src/app/api/doubts/`[id]/accept/route.test.ts around lines 40 - 62, The select mock in the doubted accept route test is tied to invocation order via mockSelectCallCount, so it can return the wrong fixture if the route changes its select sequence. Update the jest.mock for db.select to branch on query shape instead of call count, using the .from() target or another query-specific signal so the doubt lookup and reply lookup each resolve to the correct mocked result. Keep the change localized in the mock helpers like makeSelectChain and db.select so the test stays stable if queries are reordered or additional selects are added.src/__tests__/inngest/digest-functions.test.ts (1)
64-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffAvoid calling the internal
.fnhandler
createFunction()doesn’t document.fnas a public API, so this test is coupled to an Inngest internal and may break on SDK upgrades. Prefer a testable wrapper or the official Inngest test engine instead of reaching into the returned object.🤖 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 `@src/__tests__/inngest/digest-functions.test.ts` around lines 64 - 72, The test helper runHandler is reaching into Inngest’s internal .fn property, which couples the test to a non-public API. Refactor src/__tests__/inngest/digest-functions.test.ts to avoid invoking createFunction() output via .fn; instead test through an exported wrapper around the handler logic or use the official Inngest test engine/SDK test utilities. Keep the handler-specific assertions in the digest function tests, but route execution through supported interfaces such as the function-under-test itself or a helper around makeStep rather than the internal function object.
🤖 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 `@src/__tests__/inngest/digest-functions.test.ts`:
- Around line 64-72: The test helper runHandler is reaching into Inngest’s
internal .fn property, which couples the test to a non-public API. Refactor
src/__tests__/inngest/digest-functions.test.ts to avoid invoking
createFunction() output via .fn; instead test through an exported wrapper around
the handler logic or use the official Inngest test engine/SDK test utilities.
Keep the handler-specific assertions in the digest function tests, but route
execution through supported interfaces such as the function-under-test itself or
a helper around makeStep rather than the internal function object.
In `@src/app/api/doubts/`[id]/accept/route.test.ts:
- Around line 40-62: The select mock in the doubted accept route test is tied to
invocation order via mockSelectCallCount, so it can return the wrong fixture if
the route changes its select sequence. Update the jest.mock for db.select to
branch on query shape instead of call count, using the .from() target or another
query-specific signal so the doubt lookup and reply lookup each resolve to the
correct mocked result. Keep the change localized in the mock helpers like
makeSelectChain and db.select so the test stays stable if queries are reordered
or additional selects are added.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7f19114c-a3c5-400a-9c38-7f736bcddaa3
📒 Files selected for processing (10)
drizzle/0016_doubts_composite_indexes.sqldrizzle/meta/_journal.jsonsrc/__tests__/api/teacher-insights.test.tssrc/__tests__/inngest/digest-functions.test.tssrc/__tests__/lib/anonymity.test.tssrc/__tests__/lib/pagination.test.tssrc/app/api/doubts/[id]/accept/route.test.tssrc/app/api/doubts/route.tssrc/configs/schema.tssrc/lib/pagination.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/tests/lib/pagination.test.ts
- src/lib/pagination.ts
- drizzle/meta/_journal.json
- drizzle/0016_doubts_composite_indexes.sql
- src/configs/schema.ts
- src/app/api/doubts/route.ts
Updated comment formatting for clarity.
knoxiboy
left a comment
There was a problem hiding this comment.
All good just resolve merge conflicts
|
CodeAnt AI is running Incremental review |
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
Hi @knoxiboy , have resolved the merge conflicts. Please review this PR. Thanks. |
|
CodeAnt AI Incremental review completed. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@drizzle/meta/_journal.json`:
- Around line 21-22: The migration journal JSON is invalid because the entry
objects in _journal.json are not separated by a comma. Update the journal
entries around the affected objects so each migration record in the array/list
is properly comma-delimited, preserving the existing schema used by Drizzle’s
migration runner. Verify the file parses as valid JSON after adjusting the
sequence around the 0016_doubts_composite_indexes and 0017_video_jobs entries.
In `@src/app/api/doubts/`[id]/accept/route.test.ts:
- Around line 40-41: Remove the duplicate module-scope mockSelectCallCount
declaration in the route.test setup so the test file loads cleanly. Keep a
single shared counter for the select mock and make sure the mock implementation
and any reset logic in the test cases reference that one declaration, using the
existing mockSelectCallCount symbol to locate the affected setup.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aea7b663-0a31-4ff3-b27a-309563a8ae68
📒 Files selected for processing (6)
drizzle/meta/_journal.jsonsrc/__tests__/lib/anonymity.test.tssrc/app/api/doubts/[id]/accept/route.test.tssrc/app/api/doubts/route.tssrc/configs/schema.tssrc/lib/pagination.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/configs/schema.ts
- src/tests/lib/anonymity.test.ts
- src/lib/pagination.ts
- src/app/api/doubts/route.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 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 `@drizzle/meta/_journal.json`:
- Around line 21-22: The migration journal JSON is invalid because the entry
objects in _journal.json are not separated by a comma. Update the journal
entries around the affected objects so each migration record in the array/list
is properly comma-delimited, preserving the existing schema used by Drizzle’s
migration runner. Verify the file parses as valid JSON after adjusting the
sequence around the 0016_doubts_composite_indexes and 0017_video_jobs entries.
In `@src/app/api/doubts/`[id]/accept/route.test.ts:
- Around line 40-41: Remove the duplicate module-scope mockSelectCallCount
declaration in the route.test setup so the test file loads cleanly. Keep a
single shared counter for the select mock and make sure the mock implementation
and any reset logic in the test cases reference that one declaration, using the
existing mockSelectCallCount symbol to locate the affected setup.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aea7b663-0a31-4ff3-b27a-309563a8ae68
📒 Files selected for processing (6)
drizzle/meta/_journal.jsonsrc/__tests__/lib/anonymity.test.tssrc/app/api/doubts/[id]/accept/route.test.tssrc/app/api/doubts/route.tssrc/configs/schema.tssrc/lib/pagination.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/configs/schema.ts
- src/tests/lib/anonymity.test.ts
- src/lib/pagination.ts
- src/app/api/doubts/route.ts
🛑 Comments failed to post (2)
drizzle/meta/_journal.json (1)
21-22: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Invalid JSON: missing comma breaks the migration journal.
Line 21's entry object is not followed by a comma before line 22's entry object begins, making the whole file invalid JSON. Static analysis confirms this parse error. Drizzle's migration runner relies on this file to track applied migrations — an invalid JSON file will hard-fail any migration command.
🐛 Proposed fix
- { "idx": 16, "version": "7", "when": 1781400000000, "tag": "0016_doubts_composite_indexes", "breakpoints": true } + { "idx": 16, "version": "7", "when": 1781400000000, "tag": "0016_doubts_composite_indexes", "breakpoints": true }, { "idx": 17, "version": "7", "when": 1781400000000, "tag": "0017_video_jobs", "breakpoints": true }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.{ "idx": 16, "version": "7", "when": 1781400000000, "tag": "0016_doubts_composite_indexes", "breakpoints": true }, { "idx": 17, "version": "7", "when": 1781400000000, "tag": "0017_video_jobs", "breakpoints": true }🧰 Tools
🪛 Biome (2.5.1)
[error] 22-22: expected
,but instead found{(parse)
🤖 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 `@drizzle/meta/_journal.json` around lines 21 - 22, The migration journal JSON is invalid because the entry objects in _journal.json are not separated by a comma. Update the journal entries around the affected objects so each migration record in the array/list is properly comma-delimited, preserving the existing schema used by Drizzle’s migration runner. Verify the file parses as valid JSON after adjusting the sequence around the 0016_doubts_composite_indexes and 0017_video_jobs entries.Source: Linters/SAST tools
src/app/api/doubts/[id]/accept/route.test.ts (1)
40-41: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash rg -n 'mockSelectCallCount' src/app/api/doubts/\[id\]/accept/route.test.tsRepository: knoxiboy/DoubtDesk
Length of output: 391
🏁 Script executed:
#!/bin/bash sed -n '1,190p' src/app/api/doubts/\[id\]/accept/route.test.ts | cat -nRepository: knoxiboy/DoubtDesk
Length of output: 8038
Remove the duplicate
mockSelectCallCountdeclaration. The counter is used in the mock and reset in tests, but it’s declared twice at module scope, which breaks the file before it runs.🤖 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 `@src/app/api/doubts/`[id]/accept/route.test.ts around lines 40 - 41, Remove the duplicate module-scope mockSelectCallCount declaration in the route.test setup so the test file loads cleanly. Keep a single shared counter for the select mock and make sure the mock implementation and any reset logic in the test cases reference that one declaration, using the existing mockSelectCallCount symbol to locate the affected setup.
User description
Description
Database-level pagination (
ORDER BY/LIMIT/OFFSET), the reply-count subquery, and the total count are already onmain, so the JS-memory pagination the issue describes is already resolved. This PR finishes the remaining items:EXISTSsubqueries: folded into the main query instead of two extra per-request round-trips.(classroomId, createdAt),(classroomId, type),(classroomId, isSolved)plus a migration.Related Issue
Closes #319.
Type of Change
What changed
EXISTSsubqueries (src/app/api/doubts/route.ts)Previously
hasLiked/hasBookmarkedwere computed with two additional queries (SELECT doubtId FROM likes/bookmarks WHERE userEmail = ?) andSet.has()in JS. They're now resolved inline on the main query:Anonymous viewers resolve to
false. Both fields were already booleans in the response; they're now always present (logged-out users getfalseinstead ofundefined: same truth value, so no client breakage).Keyset cursor pagination (
src/lib/pagination.tsand route)A new optional
cursorquery param enables keyset paging:This is O(log n) on the new
(classroomId, createdAt)index rather than O(n) at deep offsets. It is opt-in and conservative: honored only for the default recency sort with no search, where(createdAt, id)is a stable monotonic key. Offset/pagepagination is unchanged and remains the default; pinned-first ordering and thepopular/most-replied/search-rank sorts keep using offset (their ordering isn't a monotonic keyset). The response gains an additivenextCursorfor recency feeds (returned whether the request used offset or cursor, so clients can switch to cursor for deep scroll).encodeCursor/decodeCursorlive insrc/lib/pagination.ts;decodeCursorreturnsnullfor any malformed/garbage input so the route safely falls back to offset.Composite indexes (
src/configs/schema.ts+drizzle/0016_doubts_composite_indexes.sql)Added
(classroomId, createdAt),(classroomId, type),(classroomId, isSolved)to support the feed's filter + recency-ordering access pattern. The migration is a hand-written idempotentCREATE INDEX IF NOT EXISTSfile matching the repo's existing manual-migration convention (e.g.0009_query_indexes.sql, run bydrizzle/migrate.ts).Backward compatibility
The response object is preserved (
doubts,hasMore,totalCount,page,limit) withnextCursoradded. Existingoffset/page/limitparams behave exactly as before. No removed fields.Testing
src/__tests__/lib/pagination.test.ts: cursor round-trips (Date / ISO / epoch inputs), produces an opaque base64 token, and returnsnullfor null/empty/garbage/non-numeric-id/invalid-date inputs (safe offset fallback).doubts.test.ts/doubts-id.test.tspass unchanged.npx tsc --noEmitintroduces no new errors, and the full suite shows no new failures, versus a cleanmaincheckout. (Upstreammaincurrently has 2 pre-existingtscerrors and 4 failing suites unrelated to this PR: verified identical before/after.)Notes / scope
rooms/route.tsis intentionally left out to keep this PR focused; it does not exhibit the fetch-all-then-slice pattern the issue targets. Happy to do it as a follow-up.drizzle/0016_doubts_composite_indexes.sql(idempotentCREATE INDEX IF NOT EXISTS) plus a_journal.jsonentry (idx 16) so the migration-integrity test added by PR fix(devops): resolve 14 duplicate version-number migration files in drizzle/ #722 passes.npx drizzle-kit generatestill cannot run non-interactively onmain(a pre-existingschema.ts<-> snapshot column conflict prompts for input), so that gate remains red independent of this PR; the hand-written migration follows the repo's manual approach.Note: PR [ADVANCED] Refactor Video Generation Pipeline to Async Background Job with Progress Tracking #321 also adds a
0016_*migration; whichever lands second must bump to0017(filename + journal idx) to keep numbers unique.main) wraps the doubts responses intoPublicDoubtbut kept the two extra like/bookmark queries. This PR replaces those with theEXISTSsubqueries on the mainSELECT;toPublicDoubtpreserves the resultinghasLiked/hasBookmarkedfields, so the two changes compose cleanly.Checklist
Summary by CodeRabbit
New Features
nextCursorand improvedhasMorebehavior.Bug Fixes
Tests
CodeAnt-AI Description
Speed up doubts feed loading and add cursor-based pagination
What Changed
falsefor signed-out viewersImpact
✅ Faster infinite scroll on large doubts feeds✅ Fewer extra lookups per feed request✅ Quicker classroom-filtered feed pages💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.