Skip to content

feat: cursor-based pagination for GET /comments (#263) - #268

Open
cass-clearly wants to merge 2 commits into
mainfrom
feature/cursor-pagination-263
Open

feat: cursor-based pagination for GET /comments (#263)#268
cass-clearly wants to merge 2 commits into
mainfrom
feature/cursor-pagination-263

Conversation

@cass-clearly

Copy link
Copy Markdown
Owner

Summary

Adds optional cursor-based pagination to the GET /comments endpoint.

Changes

  • New query params: limit (1–200) and after (opaque cursor)
  • When limit is omitted, all results are returned (fully backward compatible)
  • Response includes next_cursor when paginated and more pages exist; null on last page
  • Cursor is an opaque base64url-encoded offset value
  • Updated OpenAPI spec with new parameters and enriched 200 response schema
  • 7 new tests covering paginated requests, cursor chaining, last-page detection, and error cases

Breaking Changes

None — existing callers that omit limit get the same behavior as before.

Testing

node --test server/test.mjs

146 tests, 0 failures.

Closes #263

- Add optional 'limit' (1-200) and 'after' cursor query params
- When 'limit' is omitted, all results returned (backward compatible)
- Response includes 'next_cursor' when paginated and more pages exist
- Cursor is an opaque base64url-encoded offset value
- Updated OpenAPI spec with new parameters and response schema
- Added 7 tests covering pagination, cursors, and error cases
@cass-clearly

Copy link
Copy Markdown
Owner Author

Cass's Review: feat: cursor-based pagination for GET /comments

Verdict: Approve

What's Good

  • Backward compatible by designlimit omitted = all results returned, no existing integration breaks
  • Clean offset-based cursor — uses base64url-encoded offset, opaque to callers, simple to reason about
  • Robust 400 validation — invalid cursor, limit=0, limit>200 all return correct errors
  • OpenAPI spec updated — parameters and next_cursor field documented in response schema
  • 146 tests, 0 failures — 7 new tests cover the happy path, cursor chaining, last page, and error cases

Observations

  1. Offset cursors are not deletion-safe — if a comment is deleted between page 1 and page 2 fetches, the page 2 cursor may skip or duplicate an item. This is documented behavior for offset pagination and acceptable for MVP. A keyset cursor using (created_at, inserted_rownum) would fix this for a future iteration.
  2. No cursor on non-paginated responsesnext_cursor key is absent (not null) when limit is omitted. This is consistent and correct.
  3. Default limit of 50 when only after is provided — good UX default, clearly documented.

Solid, clean implementation. Approve.

@cass-clearly cass-clearly left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: feat: cursor-based pagination for GET /comments (#263)

Verdict: Approve (posted as comment — self-approval blocked)

What's Good

  • Cursor implemented as opaque base64url-encoded offset — clean API contract
  • Fully backward compatible: no limit param = all results returned as before
  • next_cursor: null on last page is explicit and easy to handle in clients
  • 7 new tests cover paginated, cursor-based, and edge cases
  • OpenAPI spec updated

Observations (non-blocking)

  1. Offset-based cursor: Cursor encodes an offset rather than a stable record ID. This means concurrent inserts during pagination can cause items to shift. Acceptable for v1 — true keyset pagination is a follow-up if needed.
  2. Default limit when only cursor provided: Defaults to 50 when after is passed without limit. This is reasonable behavior.

Solid implementation. Approve.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add cursor-based pagination to /comments endpoint

1 participant