feat: #76 myCrate query with keyset paging #77
Conversation
The database-backed suites were skipped in CI because nothing set TEST_DATABASE_URL, so "27 skipped" was passing for the whole database layer. The new `integration` job creates a throwaway PostgreSQL cluster inside the runner's temp directory, socket-only, applies the migrations, runs the server suite and deletes the cluster. No shared test database, no secret to hand out, and the application's `crate` database is never in reach. `deploy` now requires the job, so a deploy cannot ignore the database tests. The cluster alone would still have left the failure mode from "skipped" to "silently empty", so `server/scripts/assert-postgres-tests.mjs` reads the JSON report and fails when any database-backed file has zero executed or any pending assertions. Verified against both reports: with the database it prints the four files and exits 0, without it fails on the three suites that skip.
The first CI run failed with 'initdb: command not found' in 12 seconds: Debian puts the server binaries in /usr/lib/postgresql/<major>/bin and only the client on PATH, so PostgreSQL 18 being installed is not enough. Try PATH first, then the newest installed major version, and fail with a message that names the missing package if neither works.
The field has been in schema.graphql since M2 and a client that asked for it got a non-null error. The resolver follows the same shape as the paginated review list: keyset on (added_at DESC, id DESC), one extra row to detect the next page, and a cursor that rejects anything it did not write. `crate_item_user_added_at_idx` already covers (user_id, added_at, id), so the page is served by the index the schema was designed with. Note that `added_at` is a Date column while `review.created_at` is stored in string mode, so the cursor rebuilds a Date on the way back in instead of passing a string to drizzle. An anonymous request gets an empty connection, not an error: CrateConnection is non-null and reads should degrade rather than fail (architecture decision 5). The decision record now says so. Seven PostgreSQL tests: newest first with a stable tie inside the same second, a full walk across the tie with no duplicates or gaps, isolation between two users who saved the same album, the anonymous case, a malformed cursor, removal, and a deleted user.
The server half is #77; this is the page that makes the field useful, and it reuses the search result card so a saved album looks like the one that was saved. - `MY_CRATE` operation and a `/crate` route behind the same lazy-loading as the album page. The header link carries `aria-current` like Discover does. - `myCrate` gets its own cache policy: one list, `after` is paging and not part of the cache key. A new first page replaces the list; a continuation appends. - `CrateButton` evicts `myCrate` after add or remove, because the crate page caches its own list and would otherwise show an album the user just removed. - Empty, loading, error and "load more" states, with `role="status"` on the summary line. The list needed an h2: the cards start at h3, and axe reported a heading-order violation with only the page h1 above them. That is the kind of thing the axe test is for - it passed on the crate page only after the heading was added.
There was a problem hiding this comment.
Requesting changes for one paging bug; the rest looks good.
1. The cursor drops microseconds, so paging can skip items
server/src/resolvers.ts, encodeCrateCursor / decodeCrateCursor
added_at is a timestamptz with microsecond precision, filled by now(). The cursor goes through a JS Date, which only keeps milliseconds. For a row saved at 10:00:00.123900, the cursor holds 10:00:00.123, and the next page asks for added_at < .123 OR (added_at = .123 AND id < X). An item saved after .123 but before .1239 matches neither branch, so it is never returned. An exact tie at .123900 is skipped too, because the equality branch compares against .123.
I reproduced it on PostgreSQL 16 with the same WHERE clause: four rows, three of them inside one millisecond. The page after the newest row returned only id 4, when it should have returned ids 2, 1, 4.
The review list avoids this with mode: 'string' on review.created_at. One fix is to keep the column as a Date for CrateItem.addedAt but carry the exact database text in the cursor: select cursorAt: sql<string>`${crateItem.addedAt}::text` next to the row, encode that, and compare with sql`${cursor.addedAt}::timestamptz`. Switching the column to string mode, like review.created_at, works too.
The tests miss it because every fixture timestamp is a whole second. Suggested test: two or three items whose added_at differ only in microseconds inside one millisecond, walked with first: 1.
2. AGENTS.md still calls myCrate planned (small)
Rule 5 in AGENTS.md still says "the planned myCrate query", and it says anonymous reads "degrade to null or false". The PR updates docs/architecture.md but not AGENTS.md. Suggested wording: "myReview, isInCrate and myCrate rely on this identity" and "reads degrade to null, false or an empty connection".
3. The tests depend on their order (nit)
The last two tests delete fixture rows that earlier tests count on. That works with Vitest's default sequential run, but a one-line comment would stop someone from reordering them or adding .concurrent.
Looks good
- The keyset condition, the
limit + 1forhasNextPageand the ordering all match the(user_id, added_at, id)index and the review list. - The user id comes only from the request context, and the two-user test covers isolation.
- An anonymous request gets the empty non-null connection, which is consistent with decision 5 and is now documented.
CrateItem.albumgoes throughalbumById, so there is no N+1 (rule 3), and no SQL is interpolated (rule 4).- The new file is registered with the Postgres skip guard.
Review by evenkkl on #77 found a paging bug, reproduced here before the fix. The cursor was built from a JS Date, which keeps milliseconds only, so a save at .123900 produced a cursor at .123. The next page compared added_at against .123 and skipped every row in that millisecond that was not an exact match. Walking one item per page through four saves in one millisecond returned 2 of 5 items. The existing tests use whole-second timestamps, so they could not show it. The cursor now carries the database's own text for added_at: UTC with six fractional digits. It is produced with to_char, so the format does not depend on DateStyle or TimeZone. The next page compares against that text cast back to timestamptz, which stays exact and keeps the index usable. The GraphQL addedAt is still the Date, so the API output does not change. The cursor is checked before SQL sees it: the text must have the exact shape and must be a real calendar date, because Date.parse rolls 30 February over to 2 March. Malformed cursors still return "Invalid crate cursor.". Tests: a regression test that walks one item per page through four saves in one millisecond, including an exact microsecond tie. It fails on the previous code (returns 2 of 5) and passes now. Four malformed-timestamp cases; the 30 February case fails if the calendar check is removed. The two tests that change shared fixtures are now marked as order-dependent, as the review asked. Checked end to end: the built API, walked one item per page over HTTP, returned all five seeded items in order, and the cursor decodes to a microsecond timestamp.
Rule 5 still called myCrate "planned" and listed only null and false as the degraded reads for anonymous requests. Both are now wrong: the resolver exists, and an anonymous myCrate returns an empty connection.
The keyset condition was written as `added_at < $1 OR (added_at = $1 AND id <
$2)`. PostgreSQL cannot use an OR as an index condition on a multi-column index,
so EXPLAIN showed the user filter in the index cond and the cursor check as a
Filter on every row:
Index Cond: (user_id = '...'::uuid)
Filter: ((added_at < '...'::timestamptz) OR ((added_at = '...') AND (id < 89)))
For a large crate, a deep page walked every item already shown.
The same ordering is now a row comparison, `(added_at, id) < ($1, $2)`, which
PostgreSQL pushes into the index scan:
Index Cond: ((user_id = '...'::uuid) AND (ROW(added_at, id) < ROW('...', 89)))
Both columns are NOT NULL, so the two forms select the same rows; the twelve
crate tests, including the microsecond paging walk, pass unchanged.
Review by evenkkl on #77 asked for the exact database text in the cursor. That is
what this builds on: the text is cast back to timestamptz inside the row, so the
comparison stays exact at microsecond precision.
|
Thanks — all three points are fixed. Pushed as three commits; CI is green on 1. Cursor drops microseconds → fixed, with the regression test you suggested I reproduced it before changing anything: a test that saves four albums inside one millisecond ( The fix is your first suggestion: addedAtCursor: sql<string>`to_char(${crateItem.addedAt} AT TIME ZONE 'UTC', 'YYYY-MM-DD"T"HH24:MI:SS.US"Z"')`I used The GraphQL I also walked it over HTTP against the built API, not just the resolver: page 1's cursor decodes to 2. AGENTS.md — now says 3. Order dependency — added the comment above the two fixture-mutating tests: // The next two tests change fixtures that the tests above count. They must stay
// last and must not run concurrently.One related change, from your "matches the index" note. While checking that claim I ran EXPLAIN on the paging query and found it was only half true: The sql`(${crateItem.addedAt}, ${crateItem.id}) < (${cursor.addedAt}::timestamptz, ${cursor.id})`Both columns are Re-review when you have a moment. |
Two problems with paging-perf.sql, both from review by rachelks on #73. The cursor and OFFSET pages were one row apart. The anchor row was fetched with OFFSET :offset LIMIT 1, which is the row after the last one already seen, so the keyset query returned rows offset+2..offset+21 while the OFFSET query returned offset+1..offset+20. The anchor is now OFFSET :offset - 1, so both start at the same row. The script now proves it instead of asserting it: it materialises both pages and reports cursor_rows, offset_rows and shared_rows. A run at 500,000 rows prints 20 / 20 / 20, so the timings compare the same work. The keyset predicate is documented as the row comparison `(a, b) > (x, y)` that the API uses, with the reason it matters: PostgreSQL applies it as an index condition on a matching composite index, while the equivalent OR form is only a filter. That OR form was in crateForUser when this was written; #77 changed the API to the row comparison, so the benchmark and the API now run the same shape. offset is documented as "rows already scrolled past", which must be at least 1 because the first page has no cursor to page after. The script says so and stops with ON_ERROR_STOP instead of measuring a mislabelled page.
Part of #76 (server half). The client page follows in a separate PR stacked on #75, so the client types come from codegen instead of being written twice.
What was done
Query.myCratehas been inschema.graphqlsince M2 with no resolver, so any client that asked for it got a non-null error.myCrate(first, after)with keyset paging on(added_at DESC, id DESC)andtotalCount, in the same shape as the paginated review list.crate_item_user_added_at_idxon(user_id, added_at, id)already matches the ordering, so no migration.crate_item.added_atis aDatecolumn whilereview.created_atuses string mode, so the cursor rebuilds aDaterather than passing a string into drizzle.CrateConnectionis non-null and reads degrade rather than fail (architecture decision 5).docs/architecture.mdnow records that instead of saying the resolver does not exist.CrateItem.albumalready had analbumByIdloader, so a page of 20 items is not 20 queries.What was tested
Seven PostgreSQL tests, run against a real database:
They cover: newest first with a stable tie-break inside the same second; walking every page of two rows across that tie with no duplicates or gaps; two users who saved the same album never seeing each other's item; the anonymous case returning the empty shape; a malformed cursor throwing; removal and a deleted user.
The file is registered in
server/scripts/assert-postgres-tests.mjs, so CI now fails if it is skipped rather than run. The integration job lists all five database-backed files:pnpm format:check && pnpm lint && pnpm typecheck && pnpm testpasses (95 server tests with the database).