fix(surface-client): cursor pagination actually paginates; retire stale notes-ui pages deploy - #197
Merged
Conversation
Three compounding bugs broke pagination against both doors: the
bootstrap call never sent `?cursor=` at all (cursor mode is
presence-based, so page 1 could never get a watermark), the response
body was cast straight to `Note[]` instead of parsed as the
`{notes, next_cursor}` envelope, and `nextCursor` was read from
`X-Next-Cursor`, a header the self-hosted bun vault never emits (only
cloud mirrors it, additively). Grounded against the real wire contract
on both doors (bun routes.ts:1383-1394,1729-1735; cloud
notes.ts:256-268,379-383 + parse.ts:50-53).
Also adds a client-side guard: cursor pagination forces ascending
updated_at order server-side, so `orderBy`/`sort:"desc"` alongside
`cursor` always 400s — now caught before the round trip instead of
after.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XLZtmuSs1RirWGMGyCB1QB
notes.parachute.computer is now a 301 redirect worker on Cloudflare (parachute-cloud/workers/notes-redirect) — the GitHub Pages target this workflow deployed to is DNS-shadowed and unreachable, so it's been burning CI minutes on every push to main touching notes-ui or surface-client for no visible effect. notes-ui itself is untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XLZtmuSs1RirWGMGyCB1QB
Review fold on PR #197: next_cursor is never null on the wire — core's queryNotesPaged (parachute-vault/core/src/notes.ts:1741-1748) unconditionally encodes a watermark, holding it at the prior value on an empty page so a caller can persist one cursor and keep polling ("since last checked"; QueryNotesPage.next_cursor is a non-nullable string, core/src/types.ts:327-330). The bootstrap fix (empty-string cursor on page 1) was already correct and canonical (core.test.ts:1960 documents it as the wire contract) — this only corrects the termination side. - JSDoc now states the real contract: stop draining on items.length === 0, not on a falsy nextCursor (it's never falsy in practice). - Re-mocked the "final page" and "full pagination walk" tests to terminate on an empty-items page carrying a real next_cursor, matching ground truth. - The next_cursor: null → undefined parsing branch stays (a real malformed/future response could still send it) but is now commented as defensive, not a wire shape either door emits — with its own dedicated test making that explicit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XLZtmuSs1RirWGMGyCB1QB
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
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
Build task S2 from the contract-drift brief (§3).
queryNotesCursorinpackages/surface-client/src/vault-client.tswas broken against both doors — pagination silently stopped after page 1 everywhere. Plus a housekeeping rider: retiring a GitHub Pages deploy workflow that's been shipping to a dead target since thenotes.parachute.computercutover.Part 1 — the cursor fix (commit 1)
Grounded against the real wire contract on both doors (not just the brief):
parachute-vault/src/routes.ts:1383-1394(presence-based cursor mode + bootstrap) and:1729-1735({notes, next_cursor}envelope emission)parachute-cloud/workers/vault/src/rest/notes.ts:256-268,379-383(same presence-based gate) andrest/parse.ts:50-53(X-Next-Cursor— an ADDITIVE cloud-only mirror; bun never emits it)parachute-vault/core/src/notes.ts:1320-1338(cursor+orderBy/sort:"desc"→INVALID_QUERY)Three compounding bugs, all in
queryNotesCursor/requestCursorWithRetry::567old) —if (cursor) qs.set("cursor", cursor)never sent?cursor=on the first call. Cursor mode is presence-based on both doors: an entirely-omittedcursorparam never engages the envelope, so page 1 could never obtain a watermark. Fixed: every call now doesqs.set("cursor", cursor ?? "").:643old) —const items = (await res.json()) as Note[]cast the whole body to an array, but cursor mode answers{notes, next_cursor}on both doors. Fixed: the body is parsed as the envelope (with a defensive bare-array fallback for a server that ignorescursor).:644old) — readX-Next-Cursor, a header the self-hosted bun vault never emits (only cloud mirrors the same body field, additively). Fixed:next_cursoris read from the body first; the header is now only a fallback for a response whose body omits it.Also added a client-side guard:
orderByorsort: "desc"alongsidecursoralways 400s server-side (INVALID_QUERY— cursor mode forces ascendingupdated_atorder), soqueryNotesCursornow throws that synchronously instead of spending a round trip on it.Wire-visible changes (called out per the compatibility note):
queryNotesCursorrequests now always carry acursorparam (previously omitted on the bootstrap call), and its body-parsing changed from "cast toNote[]" to "parse the envelope." The types are unchanged ({ items: Note[]; nextCursor?: string }) — this API was demonstrably broken in practice (pagination never worked), so the runtime-behavior fix is the point.queryNotes(the non-cursor path) is untouched — same request shape, same bare-array response, byte-compatible; pinned by a new test.Tests (
src/__tests__/vault-client.test.ts)cursorarg) sends?cursor=(empty, present){notes, next_cursor}body → correctitems/nextCursornext_cursor; body wins over a stale headernext_cursor: null) ends the loopnext_cursor→ page 2 (?cursor=) → final page (next_cursor: null) → loop endsorderByandsort: "desc"alongside cursor both throw client-side;sort: "asc"(the forced order) is allowedqueryNotes(non-cursor) never sends acursorparam and stays byte-compatibleUpdated two pre-existing tests that encoded the old (broken) contract — mocked bare arrays +
X-Next-Cursorheader instead of the real envelope — to match ground truth; one test that combinedcursor+sort: "desc"(a combination that's actually invalid per the wire contract) now uses a valid combination.Part 2 — housekeeping rider (commit 2)
Removed
.github/workflows/deploy-notes-ui.yml(fired again on today's merge to main).notes.parachute.computeris now a 301 redirect worker on Cloudflare (parachute-cloud/workers/notes-redirect) — the GitHub Pages target is DNS-shadowed and unreachable, so the workflow was building+deploying to a dead target on every push touchingnotes-ui/surface-client.notes-uipackage itself is untouched.Version
surface-client0.3.5 → 0.3.6 (stable patch, no rc — repo convention). CHANGELOG updated.Gates (literal verdicts)
bun run --filter "@openparachute/surface-client" test: 310 pass, 0 failbun run test(surface-host + surface-client + account-client + surface-server + pebble-config + doc-schema + docs-editor + meeting-ingest + meeting-mcp + notes-ui + surface-render): 1607 + 1140 + 61 = 2808 tests, 0 fail (one notes-ui route test flaked on a prior run —App > catch-all redirects to the root list— confirmed pre-existing/unrelated: passes in isolation and on a clean re-run of the full suite; notes-ui package is untouched by this PR)bun run typecheck: exit 0bun run typecheck:all: exit 0bun run lint: 26 errors / 8 warnings — identical count on cleanmainbefore this PR (verified viagit stash); zero new lint errors introduced (the one formatting issue in my own new test code was fixed before commit)bun run build:surface-client: exit 0 (prebuild regeneratessrc/version.ts→ confirmed0.3.6)Which of the brief's 3 claims survived contact with the code
All three survived verification against the actual vault/cloud source (not just the brief's citations), with one clarification: the brief's own wording on bug (1) was slightly ambiguous ("never bootstraps... on the first call" vs "never sends... on subsequent calls") — ground truth is that cursor mode is presence-based, so the bug is specifically that
qs.set("cursor", cursor)underif (cursor)skips the param entirely whenevercursoris falsy, which includes both the first call (undefined) and any manual re-bootstrap with"". The fix (qs.set("cursor", cursor ?? "")unconditionally) closes both readings at once.Co-Authored-By: Claude Fable 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01XLZtmuSs1RirWGMGyCB1QB