fix: stabilize records keyset pagination across timestamp formats#6
Merged
Kzoeps merged 7 commits intofix/tap-ack-issuefrom Apr 14, 2026
Merged
fix: stabilize records keyset pagination across timestamp formats#6Kzoeps merged 7 commits intofix/tap-ack-issuefrom
Kzoeps merged 7 commits intofix/tap-ack-issuefrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughCentralized handling and normalization of the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 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. ✨ 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 |
Collaborator
Author
|
@coderabbitai please review this |
|
✅ 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: 1
🧹 Nitpick comments (1)
internal/database/repositories/records_test.go (1)
520-605: Convert this regression into a table-driven test.The new coverage is useful, but this shape makes it harder to extend with the mixed-format permutations that matter here. A case table would remove the duplicated setup/assert logic and make it straightforward to add
"YYYY-MM-DD HH:MM:SS"+ RFC3339 rows in the same dataset.As per coding guidelines, "Use table-driven tests for test functions with a test slice containing name, input, expected output, and error expectation fields".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/database/repositories/records_test.go` around lines 520 - 605, Convert TestRecordsRepository_KeysetPagination_NormalizesIndexedAtFormats into a table-driven test: create a slice of cases (fields: name, function to call or direction flag, inputCursor []string, expectedURIs []string) and iterate with t.Run per case; keep the common setup (calls to insertTestRecord and sqlDB.ExecContext to set indexed_at strings) outside the loop, then call repo.GetByCollectionSortedWithKeysetCursor or repo.GetByCollectionReversedWithKeysetCursor based on the case; replace the duplicated assertions with a single check comparing returned URIs to expectedURIs and include cases that use both RFC3339 ("2026-01-15T12:00:00Z") and SQL datetime ("2026-01-15 12:00:00") cursor formats to cover mixed-format permutations.
🤖 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/database/repositories/records.go`:
- Around line 611-629: The ORDER BY expression for indexed_at must match the
keyset predicate normalization on SQLite: update the code path that builds sort
expressions (e.g., RecordsRepository.buildSortExpr / buildSortExpr) so that when
sortField == "indexed_at" and r.db.Dialect() is SQLite it returns the same
normalized expression as keysetSortFieldExpr ("strftime('%Y-%m-%dT%H:%M:%fZ',
indexed_at)") instead of the raw column; apply the same change wherever ORDER BY
indexed_at is constructed (including the other build/ordering helpers referenced
around the keyset code) so ordering and keyset comparisons are consistent.
---
Nitpick comments:
In `@internal/database/repositories/records_test.go`:
- Around line 520-605: Convert
TestRecordsRepository_KeysetPagination_NormalizesIndexedAtFormats into a
table-driven test: create a slice of cases (fields: name, function to call or
direction flag, inputCursor []string, expectedURIs []string) and iterate with
t.Run per case; keep the common setup (calls to insertTestRecord and
sqlDB.ExecContext to set indexed_at strings) outside the loop, then call
repo.GetByCollectionSortedWithKeysetCursor or
repo.GetByCollectionReversedWithKeysetCursor based on the case; replace the
duplicated assertions with a single check comparing returned URIs to
expectedURIs and include cases that use both RFC3339 ("2026-01-15T12:00:00Z")
and SQL datetime ("2026-01-15 12:00:00") cursor formats to cover mixed-format
permutations.
🪄 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: ba0a6a7b-0bbf-4624-9eb1-4b3971e82910
📒 Files selected for processing (3)
internal/database/repositories/records.gointernal/database/repositories/records_test.gointernal/graphql/schema/builder.go
Collaborator
Author
|
@coderabbitai please review |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
internal/database/repositories/records.go (1)
617-628: Typo in function name:normalizedIndextAtExpr→normalizedIndexedAtExpr.The function name has a typo ("Indext" instead of "Indexed"). This affects readability and could cause confusion.
✏️ Suggested rename
-// normalizedIndextAtExpr returns the record-side indexed_at expression used for +// normalizedIndexedAtExpr returns the record-side indexed_at expression used for // ordering and keyset comparisons. SQLite stores indexed_at as TEXT and may // contain mixed formats (e.g. "YYYY-MM-DD HH:MM:SS" and RFC3339), so values are // normalized to a canonical sortable UTC representation. -func (r *RecordsRepository) normalizedIndextAtExpr() string { +func (r *RecordsRepository) normalizedIndexedAtExpr() string {Then update all call sites (lines 320, 350, 549, 666, 677, 913, 1270, 1284).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/database/repositories/records.go` around lines 617 - 628, Rename the function normalizedIndextAtExpr to normalizedIndexedAtExpr to fix the typo and update every call site that invokes normalizedIndextAtExpr to the new name; specifically change the method declaration on RecordsRepository and replace all references (all places where normalizedIndextAtExpr() is called) so builds and callers use normalizedIndexedAtExpr(), preserving the same return logic (PostgreSQL returns "indexed_at", others use the strftime expression).internal/tap/consumer.go (1)
117-117: Make the connection outcome self-describing.This PR fixes a swapped return-order bug, but
runOncestill exposes another positional(bool, error)pair. A tiny result type here would make the call site much harder to regress.♻️ Possible shape
+type runOnceResult struct { + connected bool +} + - connected, err := c.runOnce(ctx) - if connected { + result, err := c.runOnce(ctx) + if result.connected { backoff = minBackoff } -func (c *Consumer) runOnce(ctx context.Context) (bool, error) { +func (c *Consumer) runOnce(ctx context.Context) (runOnceResult, error) {Also applies to: 171-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tap/consumer.go` at line 117, The call site uses a positional (bool, error) return from runOnce which is easy to mis-order; change runOnce's signature to return a small named result type instead of plain bool: define a RunOnceResult struct (e.g., type RunOnceResult struct { Connected bool }) and update runOnce to return (RunOnceResult, error), then update all call sites (e.g., the c.runOnce(ctx) usages that assign to connected, err) to use res, err := c.runOnce(ctx) and check res.Connected; apply the same change to other call sites around the block mentioned (the calls at the other location referenced) so the meaning is explicit and not position-dependent..githooks/pre-commit (2)
37-38: Note: CI pins golangci-lint to v2.8.0.The hook runs whichever
golangci-lintversion is installed locally, while CI uses v2.8.0 (per.github/workflows/ci.yml). Different versions may produce different results.Consider documenting the expected version in README or updating
make toolsto install the pinned version.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.githooks/pre-commit around lines 37 - 38, The pre-commit hook prints "pre-commit: running golangci-lint on staged packages..." and then runs golangci-lint run "${packages[@]}", which can use a locally different golangci-lint version than CI's pinned v2.8.0; update the repo to either (A) document the expected golangci-lint version (v2.8.0) in README and contributing notes so developers know which version to install, or (B) modify the developer toolchain installation (e.g., the Make target that installs tools) to install golangci-lint v2.8.0 and change the hook to invoke that pinned binary (or check/gate on golangci-lint --version and fail with a message if it does not match "v2.8.0") so local runs match CI.
16-18: Consider addinggofumptfor consistency withmake fmt.The hook uses only
gofmt, butmake fmtapplies bothgo fmtandgofumpt. This inconsistency means code passing the pre-commit hook might still differ frommake fmtoutput.♻️ Add gofumpt if installed
printf 'pre-commit: formatting staged Go files with gofmt...\n' gofmt -w "${staged_go_files[@]}" +if command -v gofumpt &>/dev/null; then + gofumpt -w "${staged_go_files[@]}" +fi git add -- "${staged_go_files[@]}"Based on learnings: "Run formatting with
make fmt(go fmt + gofumpt) before committing".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.githooks/pre-commit around lines 16 - 18, The pre-commit hook currently runs only gofmt on "${staged_go_files[@]}", causing divergence from make fmt which also runs gofumpt; update the hook to ensure consistency by either (A) detecting and running gofumpt -w on the same staged_go_files (e.g., check command -v gofumpt and run it, then git add those files) or (B) simply invoke make fmt before adding files so both go fmt and gofumpt are applied; reference the staged_go_files variable and the existing gofmt invocation when adding the new step.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.githooks/pre-commit:
- Line 10: The pre-commit hook uses Bash-4+ only features (mapfile in the
staged_go_files assignment and declare -A for associative arrays) which breaks
on macOS's default Bash 3.2; either document the Bash 4+ requirement in project
docs or make the hook POSIX-compatible: replace the mapfile usage that populates
staged_go_files and any declare -A usage with portable alternatives (e.g., while
read -r -d '' loop to collect git diff --name-only -z output into an array and
use plain arrays or newline-separated strings instead of associative arrays),
and detect/bash-check at hook startup (check $BASH_VERSION or use env bash) to
provide a clear error message if an unsupported shell is running. Ensure you
update the references to staged_go_files and any associative array usages in the
hook to the new portable variables.
In `@internal/oauth/dpop.go`:
- Around line 67-74: The function that builds a JWK currently swallows errors
from PublicKey.Bytes() and returns an incomplete &JWK{Kty:"EC", Crv:"P-256"} on
failure; change its signature to return (*JWK, error) (or (JWK, error)) and
update all callers (including the JWKS endpoint handler) to propagate or handle
the error, and when PublicKey.Bytes() returns an error or when len(pubBytes) !=
1+2*p256CoordinateSize() || pubBytes[0] != 0x04 return a clear error (and do not
emit a malformed JWK); alternatively if changing the signature is infeasible,
log a warning including the specific error/invalid-length details before
returning nil or a zero-value JWK so the issue is observable.
In `@internal/tap/consumer.go`:
- Line 246: In the branch that currently does "return true, err" in
internal/tap/consumer.go, wrap the propagated dispatch/write-path error with
fmt.Errorf using the %w verb and a TAP-specific message (e.g., include "TAP
ack/write" or similar) so it matches the other error paths; also ensure fmt is
imported if not already.
---
Nitpick comments:
In @.githooks/pre-commit:
- Around line 37-38: The pre-commit hook prints "pre-commit: running
golangci-lint on staged packages..." and then runs golangci-lint run
"${packages[@]}", which can use a locally different golangci-lint version than
CI's pinned v2.8.0; update the repo to either (A) document the expected
golangci-lint version (v2.8.0) in README and contributing notes so developers
know which version to install, or (B) modify the developer toolchain
installation (e.g., the Make target that installs tools) to install
golangci-lint v2.8.0 and change the hook to invoke that pinned binary (or
check/gate on golangci-lint --version and fail with a message if it does not
match "v2.8.0") so local runs match CI.
- Around line 16-18: The pre-commit hook currently runs only gofmt on
"${staged_go_files[@]}", causing divergence from make fmt which also runs
gofumpt; update the hook to ensure consistency by either (A) detecting and
running gofumpt -w on the same staged_go_files (e.g., check command -v gofumpt
and run it, then git add those files) or (B) simply invoke make fmt before
adding files so both go fmt and gofumpt are applied; reference the
staged_go_files variable and the existing gofmt invocation when adding the new
step.
In `@internal/database/repositories/records.go`:
- Around line 617-628: Rename the function normalizedIndextAtExpr to
normalizedIndexedAtExpr to fix the typo and update every call site that invokes
normalizedIndextAtExpr to the new name; specifically change the method
declaration on RecordsRepository and replace all references (all places where
normalizedIndextAtExpr() is called) so builds and callers use
normalizedIndexedAtExpr(), preserving the same return logic (PostgreSQL returns
"indexed_at", others use the strftime expression).
In `@internal/tap/consumer.go`:
- Line 117: The call site uses a positional (bool, error) return from runOnce
which is easy to mis-order; change runOnce's signature to return a small named
result type instead of plain bool: define a RunOnceResult struct (e.g., type
RunOnceResult struct { Connected bool }) and update runOnce to return
(RunOnceResult, error), then update all call sites (e.g., the c.runOnce(ctx)
usages that assign to connected, err) to use res, err := c.runOnce(ctx) and
check res.Connected; apply the same change to other call sites around the block
mentioned (the calls at the other location referenced) so the meaning is
explicit and not position-dependent.
🪄 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: 2539c1e9-5095-42f3-92fe-14b520fd8157
📒 Files selected for processing (9)
.githooks/pre-commit.github/workflows/ci.ymlMakefileREADME.mdinternal/database/repositories/records.gointernal/database/repositories/records_filter_test.gointernal/oauth/dpop.gointernal/oauth/dpop_test.gointernal/tap/consumer.go
💤 Files with no reviewable changes (1)
- .github/workflows/ci.yml
✅ Files skipped from review due to trivial changes (2)
- README.md
- internal/database/repositories/records_filter_test.go
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
1 participant
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.
Summary
recordsby normalizingindexed_atcomparisons per database dialect (SQLite normalized text timestamps, Postgres typed timestamptz comparisons)RFC3339Nanofor more stable ordering and fewer same-second collisionsValidation
go test ./internal/database/repositories ./internal/graphql/schemago test ./...org.hypercerts.claim.activityonhttps://hyperindex-staging.up.railway.app/graphqlno longer stalls or repeats cursors across pagesSummary by CodeRabbit
Bug Fixes
Chores
Tests
Documentation
CI