Skip to content

Fix ascending report pagination tie-breaker - #5393

Open
gcwill70 wants to merge 3 commits into
mainfrom
fix/report-pagination-sort-direction
Open

Fix ascending report pagination tie-breaker#5393
gcwill70 wants to merge 3 commits into
mainfrom
fix/report-pagination-sort-direction

Conversation

@gcwill70

@gcwill70 gcwill70 commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Summary

There is a risk that mismatched sort direction in keyset columns will lead to skipped or missed reports. This will make the sort direction in both columns the same to avoid that.

This was added so that ozone#561 can show oldest reports reliably.

Testing

  • pnpm exec eslint packages/ozone/src/mod-service/report.ts packages/ozone/tests/query-reports.test.ts
  • integration test could not run locally because Docker and DB_TEST_POSTGRES_URL are unavailable
  • package build is blocked by unrelated isolatedDeclarations errors in packages/lex/lex-schema

@claude

claude Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Claude finished @gcwill70's task in 1m 36s —— View job


Review

  • Read review guidance & repo conventions
  • Examine the diff (report.ts, query-reports.test.ts)
  • Analyze correctness of the sort-direction fix
  • Check test coverage & changeset requirements
  • Post review

The core fix is correct and the regression test is well-targeted. One mechanical gap (missing changeset). Details below.


What the fix does (and why it's right)

packages/ozone/src/mod-service/report.ts:130 — the secondary tie-breaker was hardcoded .orderBy('r.id', 'desc') while the cursor predicate for ascending sort uses r.id > id (report.ts:142-145). That mismatch is a genuine pagination bug:

For three open reports with an identical createdAt (ids 1, 2, 3), sortDirection: 'asc', limit: 2:

  • Old ordering createdAt ASC, id DESC → page returns [3, 2], cursor emits id=2.
  • Next page filter createdAt = v AND id > 2 → returns only 3 (already seen); id 1 is silently dropped.

Aligning the tie-breaker with sortDirection makes the ordering (createdAt ASC, id ASC) consistent with the id > cursorId predicate, so ties paginate without loss or duplication. Descending behavior is unchanged. ✅

I also checked index compatibility, since the PR description mentions it: every report index is (… "createdAt" DESC, id DESC) (e.g. idx_report_subject_active at packages/ozone/src/db/migrations/20260219T164523000Z-create-report-table.ts:87-89). An ASC, ASC ordering is served by a backward index scan of these same indexes in Postgres, so no new index is needed. ✅

Test

query-reports.test.ts — bob's account has exactly 3 open reports (seedReports, loop i < 3), so limit: 2 places the cursor boundary inside the tie group — this genuinely exercises the fix rather than trivially passing. The finally block restoring each createdAt is the right call to avoid leaking mutated state into sibling tests sharing the beforeAll network. Good regression coverage.

Finding (non-blocking)

Missing changeset for @atproto/ozone. Per CLAUDE.md, every package touched by a change needs a changeset entry; report.ts is a source change to the published @atproto/ozone package, but no .changeset/*.md naming @atproto/ozone is added in this PR. A patch bump is appropriate for a bug fix.

Fix this →


Nothing else stood out — the change is tightly scoped and the fix is sound.
· fix/report-pagination-sort-direction

@gcwill70

Copy link
Copy Markdown
Contributor Author

This keeps the createdAt and id sort directions aligned so ascending queries can scan the existing descending index backward and the cursor predicate matches the result ordering. In addition to avoiding an extra sort, it prevents duplicate or skipped reports when multiple reports share the same timestamp.

@claude

claude Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Claude Code is working…

I'll analyze this and get back to you.

View job run

@claude

claude Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Claude finished @gcwill70's task in 3m 17s —— View job


Re-review

  • Read review guidance & repo conventions
  • Examine updated diff (changeset added, report.ts, query-reports.test.ts)
  • Verify cursor predicate ↔ ordering alignment for both directions
  • Post review

Your explanation matches what the diff does, and the prior review's one finding is now resolved.

Changeset resolved. .changeset/tidy-report-pages.md adds a patch bump for @atproto/ozone — correct level for a bug fix, and the description is accurate.

Fix re-verified. packages/ozone/src/mod-service/report.ts:131 now ties orderBy('r.id', sortDirection) to the primary sort. For asc, the ordering (createdAt ASC, id ASC) matches the keyset predicate createdAt > v OR (createdAt = v AND r.id > id) at report.ts:143-146, and the emitted cursor uses the last (largest-id) row of the page (report.ts:167-170), so a tie group paginates without dropping or duplicating rows. desc is byte-for-byte unchanged. The inline comment at report.ts:130 accurately describes the current behavior. ✅

Index note holds. The existing (… DESC, id DESC) report indexes serve the new ASC, ASC ordering via a backward index scan, so no migration is needed — consistent with your rationale. ✅

Test. query-reports.test.ts collapses bob's 3 open reports to a single timestamp and asserts the two-page union equals the full id-ordered list; with limit: 2 the cursor boundary lands inside the tie group, so it genuinely exercises the fix. The finally restores each original createdAt, avoiding leakage into sibling tests on the shared beforeAll network. Good coverage.

Nothing else stood out — the change is tightly scoped and correct. I couldn't run the Postgres-backed integration test here (no DB_TEST_POSTGRES_URL), so that assertion is by inspection, but the logic is sound.
· fix/report-pagination-sort-direction

@gcwill70
gcwill70 requested a review from foysalit August 13, 2026 15:05
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.

1 participant