feat: Add cursor pagination to list endpoints; remove offset pagination#42
Merged
Conversation
✱ Stainless preview buildsThis PR will update the This comment is auto-generated by GitHub Actions and is automatically kept up to date as you push. |
WalkthroughThis PR migrates list endpoints from offset-based to keyset cursor pagination. It adds a pkg/cursor implementation (encode/decode, ErrInvalidCursor), replaces offset fields with cursor parameters in models and service/handler signatures, and introduces ListAfterCursor methods in repositories and services to run keyset queries (timestamp DESC, id ASC). Handlers map cursor decoding errors to 400 Bad Request. A pagination helper builds NextCursor from the last record. SQL migrations add composite indexes to support keyset scans. OpenAPI and tests are updated to reflect cursor parameters and next_cursor responses. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/repository/feedback_records_repository.go`:
- Around line 245-314: List and ListAfterCursor duplicate the SELECT statement
and the rows.Scan loop which risks drift; extract a shared helper such as
fetchFeedbackRecords(ctx, query string, args ...interface{})
([]models.FeedbackRecord, error) or split into buildBaseSelect() string plus
scanRowsToFeedbackRecords(rows pgx.Rows) ([]models.FeedbackRecord, error). Move
the SELECT column list into the shared buildBaseSelect or use it inside the
helper, have both List and ListAfterCursor call the helper (pass the assembled
WHERE/LIMIT parts and args), and ensure the helper handles query execution, rows
iteration, scanning into models.FeedbackRecord, error wrapping, and rows.Close
to preserve existing behavior.
In `@internal/service/feedback_records_service.go`:
- Around line 168-188: Extract the duplicated pagination assembly into a shared
helper to avoid repeating the same pointer/next-cursor logic used in
ListFeedbackRecords (internal/service/feedback_records_service.go) and
ListWebhooks (internal/service/webhooks_service.go): create a helper (e.g.,
buildPaginatedMeta or buildPaginationResponse) that accepts the records slice,
total (int), filters (with Limit and Offset), and a cursor encoder function
(cursor.Encode) and returns the computed Total *int, Limit int, Offset *int, and
NextCursor string (or a small struct with those fields); then call this helper
from ListFeedbackRecords and ListWebhooks to populate
models.ListFeedbackRecordsResponse and the webhooks response type respectively,
keeping the existing error handling for cursor.Encode in the helper.
In `@openapi.yaml`:
- Around line 158-165: Add an explicit 400 response for invalid cursor on the
list endpoints that accept the query parameter named "cursor" (e.g., the
/v1/feedback-records and /v1/webhooks paths) so generated clients can surface
cursor validation errors; update each operation object to include a 400 response
entry with a clear description like "Invalid cursor" and a response schema
matching your existing error model (reuse the same error schema used by other
400 responses in the spec) to keep the OpenAPI contract aligned with runtime
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 49ef504b-5798-4047-b51c-e348d8dfbcdd
📒 Files selected for processing (15)
internal/api/handlers/feedback_records_handler.gointernal/api/handlers/webhooks_handler.gointernal/models/feedback_records.gointernal/models/webhooks.gointernal/repository/feedback_records_repository.gointernal/repository/webhooks_repository.gointernal/service/feedback_records_service.gointernal/service/webhook_provider_test.gointernal/service/webhook_sender_test.gointernal/service/webhooks_service.gointernal/service/webhooks_service_test.gomigrations/006_list_keyset_indexes.sqlopenapi.yamlpkg/cursor/cursor.gotests/integration_test.go
Contributor
Author
|
@coderabbitai can you re-review? |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/repository/webhooks_repository.go`:
- Around line 149-185: The List and ListAfterCursor methods duplicate query
execution and row-to-model mapping; extract the shared logic into a single
helper such as fetchWebhooks or scanWebhooksRows that accepts ctx, query and
args (and returns ([]models.Webhook, error)), centralizes rows scanning
(including scanning into dbEventTypes, calling parseDBEventTypes, and mapping
fields
ID/URL/SigningKey/Enabled/TenantID/CreatedAt/UpdatedAt/DisabledReason/DisabledAt),
closes rows and checks rows.Err(), and is called by both List and
ListAfterCursor to avoid drift when selected columns change.
In `@internal/service/feedback_records_service.go`:
- Around line 121-126: The code dereferences the filters parameter
(filters.Limit and filters.Cursor) without a nil check; add a defensive nil
guard at the start of the function that owns this code in
internal/service/feedback_records_service.go: if filters == nil, initialize a
default filters value (or assign a local default struct) before using it so the
subsequent logic that sets filters.Limit (default 100) and uses
strings.TrimSpace(filters.Cursor) cannot panic; ensure the rest of the function
uses that non-nil filters variable.
In `@internal/service/pagination.go`:
- Around line 11-26: BuildListPaginationMeta currently treats recordCount==limit
as "more pages" which causes a spurious NextCursor when the final page contains
exactly limit items; to fix, change the callers (e.g., the repository methods
such as in webhooks_repository.go) to fetch limit+1 rows as a sentinel, trim the
returned results down to limit before returning, and pass the full fetched count
into BuildListPaginationMeta; then update BuildListPaginationMeta usage to only
emit NextCursor when recordCount > limit (i.e., a true extra item was fetched)
and keep meta.Limit as the original limit. Ensure encodeLast() is only called
when recordCount > limit and repositories drop the extra item before returning
the result set.
In `@migrations/006_list_keyset_indexes.sql`:
- Around line 1-21: Do not modify migrations/006_list_keyset_indexes.sql;
instead create a new sequential migration file (e.g.,
007_keyset_pagination_indexes.sql) containing the same DDL changes (the
CONCURRENTLY DROP/CREATE for idx_feedback_records_tenant_collected_at_id,
idx_webhooks_tenant_created_at_id and recreation of
idx_feedback_records_tenant_collected_at in the Down), include the same goose
headers (+goose Up and +goose Down and NO TRANSACTION), and leave migration 006
unchanged so environments that already ran 006 will receive these index
operations via the new migration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5b592d3a-e6f3-4cd6-8ea1-52671d3dd6db
📒 Files selected for processing (21)
internal/api/handlers/feedback_records_handler.gointernal/api/handlers/search_handler.gointernal/api/handlers/search_handler_test.gointernal/api/handlers/webhooks_handler.gointernal/models/feedback_records.gointernal/models/webhooks.gointernal/repository/embeddings_repository.gointernal/repository/feedback_records_repository.gointernal/repository/webhooks_repository.gointernal/service/feedback_records_service.gointernal/service/pagination.gointernal/service/search_service.gointernal/service/search_service_test.gointernal/service/webhook_provider_test.gointernal/service/webhook_sender_test.gointernal/service/webhooks_service.gointernal/service/webhooks_service_test.gomigrations/006_list_keyset_indexes.sqlopenapi.yamlpkg/cursor/cursor.gotests/integration_test.go
xernobyl
changed the title
mattinannt
requested changes
Mar 4, 2026
Member
mattinannt
left a comment
mattinannt
left a comment
There was a problem hiding this comment.
This is a Codex AI review.
Thanks for the cursor migration work overall. I’m requesting changes for two release-blocking issues:
ListEnabled()is now implicitly capped to 100 rows because it delegates to the paginatedList()default limit. This can silently drop enabled webhooks from delivery.- Removing
offsetwithout rejecting unknown query params means existing clients can sendoffsetand get page 1 repeatedly with no explicit error.
Please address both before public release.
mattinannt
requested changes
Mar 5, 2026
Member
mattinannt
left a comment
mattinannt
left a comment
There was a problem hiding this comment.
This is a Codex review with human approval.
Requesting changes for one remaining issue before merge.
mattinannt
approved these changes
Mar 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Removes offset+limit pagination and keeps only cursor-based (keyset) pagination across:
GET /v1/feedback-records)GET /v1/webhooks)POST /v1/feedback-records/search/semantic)GET /v1/feedback-records/{id}/similar)Summary of changes
Offsetfrom filters andTotal/Offsetfrom responses; addedCursorandNextCursor.cursorempty → first page (List/Nearest);cursorset →ListAfterCursor/NearestFeedbackRecordsByEmbeddingAfterCursor.ListAfterCursorfor feedback records and webhooks; removedCount()from feedback records; removedoffsetfrom embeddings nearest-neighbor.cursor.ErrInvalidCursorhandling for 400 responses; removed offset parsing.offsetquery params andtotal/offsetfrom response schemas.Breaking changes
offsetquery param is no longer supported; clients must usecursorandnext_cursor.totalandoffsetare removed.API examples
Before (removed):
After:
Response (before):
{"data": [...], "total": 500, "limit": 100, "offset": 200}Response (after):
{"data": [...], "limit": 100, "next_cursor": "eyJjIjoiMjAyNC0wMS0xNSIsImkiOiIuLi4ifQ=="}How should this be tested?
make buildandmake fmt— should succeed.make tests— integration tests pass (including cursor pagination).go test ./internal/... ./pkg/...— unit tests pass.offset; usenext_cursorfor subsequent pages.Checklist
Required
make buildmake tests(integration tests intests/)make fmtandmake lint; no new warningsgit pull origin mainAppreciated
docs/if changes were necessarymake tests-coveragefor meaningful logic changes