feat: paginate thread loading with user-anchored turn windows - #5493
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
There was a problem hiding this comment.
Effect service conventions review of the changed TypeScript. One finding: the new UI-to-state-machine channel in packages/client-runtime/src/state/threads.ts routes through mutable module-global state instead of the Effect environment. Server-side additions (threadDetailCursor.ts, windowed ProjectionSnapshotQuery queries, contract/schema additions) follow the import, error, and dependency-acquisition conventions; test harnesses pass service instances explicitly, which is an allowed test seam.
Posted via Macroscope — Effect Service Conventions
ApprovabilityVerdict: Needs human review This PR introduces a substantial new pagination feature for thread loading with complex state management, new API parameters, and multi-platform UI changes. Additionally, there is an open bug report about the loading state getting stuck on disconnect. Human review is appropriate for this scope. No code changes detected at You can customize Macroscope's approvability policy. Learn more. |
d4c55c3 to
2c8a2e3
Compare
| const pendingOlderPage = yield* Ref.make<{ | ||
| readonly snapshot: OrchestrationThreadDetailSnapshot; | ||
| readonly epoch: number; | ||
| } | null>(null); |
There was a problem hiding this comment.
Parked page sticks loading offline
Medium Severity
The pendingOlderPage state, which holds a parked page and keeps loadingOlder true, isn't cleared when the connection disconnects or encounters a stream error. This leaves the UI stuck on "Loading earlier turns..." and prevents subsequent attempts to fetch older history.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 9bc5966. Configure here.
…n pages Adds opt-in pagination to thread detail reads. A windowed request returns everything from the Nth-last user-anchored turn onward (subagent/fan-out turns ride along) plus page metadata with an opaque exclusive cursor for disjoint older slices. Requests without a window keep the full-snapshot behavior on both HTTP and the WS fallback, so pre-pagination clients are unaffected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Clients gate window requests on threadSnapshotPagination in the server config, so new clients never send window fields to pre-pagination servers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ge merges Thread state gains page metadata and a loadOlderTurns flow implementing the consistency rules: fresh snapshots replace all loaded history, in-flight older pages are discarded when a revert/snapshot/deletion rewrites history (epoch check) or when the page was read from a projection behind the loaded state, and merged pages never advance the live-event dedupe sequence. Windowed loads are gated on the server's threadSnapshotPagination capability; servers without it keep full snapshots. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Both timelines gain a plain load-more row as the list header, driven by the shared thread state's page metadata. LegendList's maintainVisibleContentPosition anchors the scroll position on prepend. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ext.Reference Effect Service Conventions check flagged the module-global handler Map as hiding the UI-action-to-state-machine dependency. The registry is now a Context.Reference the machines resolve from the environment (overridable in tests), with a shared default instance backing the sync requestOlderThreadTurns entry point so app wiring is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…omic page merges Addresses three review findings on the pagination PR: - Stale cursors after revert (high): the server's revert projector rewrites projection_turns row ids, invalidating the stored page cursor. On a windowed thread, a revert now triggers a fresh windowed snapshot fetch (sequence-checked so a lagging projection cannot resurrect reverted turns), minting a valid cursor. - Windowed cache vs pre-pagination server (medium): resuming a windowed cache via afterSequence against a server without threadSnapshotPagination would render only the window forever. The subscription now drops the windowed cache and takes a full snapshot; loadOlderTurns is gated on the capability so window params are never sent to old servers. - Epoch TOCTOU (medium): staleness check and page merge now run under the same semaphore as stream-item application, closing the window where a revert could land between check and merge. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…te update The merge previously read the loaded thread outside SubscriptionRef.update and committed the result inside it, so a concurrent setThread between read and commit could be overwritten. The merge now composes with the value the update callback receives. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ility, no-op refresh when fully loaded Second round of review findings on the revert-refresh path: - The refresh's staleness check and snapshot application now share one applyLock acquisition (via applyItemLocked), so a live event cannot advance lastSequence between check and apply and be swallowed by a regressing watermark. - paginationSupported is reset on disconnect: the capability belongs to the session that advertised it, and a stale true during reconnect could send window params to a newly prepared pre-pagination server. - The post-revert refresh is skipped when hasMore is false: there is no cursor to re-mint and the refresh would discard already-merged older pages for nothing. The revert reducer's own filtering handles history. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Third-party review round on the pagination design: - Cursors are now an (anchor timestamp, turn id) keyset instead of projection_turns.row_id. Row ids are rewritten by the revert projector and by projection rebuilds, silently invalidating persisted cursors; the keyset is derived from event content and survives both. This deletes the client's entire revert-refresh machinery (refresh queue, refreshWindowedSnapshot, revert-triggers-refresh wiring) — the revert reducer's turn filtering is sufficient on its own. Pinned by a server test that rewrites all turn row ids and re-pages with the old cursor. - Thread cache schema bumped to 3 on web and mobile (rollback safety): a pre-pagination client would decode a windowed v2 record, silently drop the unknown page field, and treat the partial thread as complete forever. v3 records fail its literal match and cold-load instead. - The window query applies the keyset bound and 150-turn LIMIT in a candidates CTE before the window functions run, so a page over a huge thread scans a bounded number of turns instead of every older turn. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The anchor is COALESCE(requested_at, started_at, '') and the turn key is COALESCE(turn_id, ''), so a server-minted cursor can legitimately carry empty strings; the decoder rejected them as malformed, degrading a valid cursor to a first-page request that repeats recent history. Adds a codec test file covering round-trips (including empty boundaries) and malformed input. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two blockers from external review: - Pages now carry threadSequence, the highest thread-detail event sequence applied at read time (filtered to the exact event types the subscription delivers, so the watermark is always reachable). A page read ahead of the client's live state parks until events catch up, closing the race where a streaming turn outside the loaded window had its deltas replayed on top of page content that already included them, duplicating text. Pages from pre-watermark servers merge immediately (old behavior). - The window query's candidates CTE now orders by raw (requested_at, turn_id) — requested_at is NOT NULL by schema — and migration 037 adds a (thread_id, requested_at, turn_id) index, so the keyset range and order are both index-served with no temp B-tree: the scan is genuinely bounded by the page LIMIT. Keyset comparisons keep COALESCE only on the turn_id tiebreak, which does not affect index use. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
9bc5966 to
95cd34a
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 95cd34a. Configure here.
| ? { minAnchorAt: "", minTurnKey: "", beforeAnchorAt: "", beforeTurnKey: "" } | ||
| : undefined; | ||
|
|
||
| const thread = yield* getThreadDetailByIdBounded(threadId, emptyBounds ?? bounds); |
There was a problem hiding this comment.
Pre-turn rows dropped from pages
Medium Severity
Windowed reads only open the lower bound for turnless rows when the thread has no turns at all. If any turns exist, minAnchorAt stays at the oldest returned turn’s requested_at even when hasMore is false, so messages and activities with turn_id null and created_at before that first turn never appear on any page. That breaks the claimed full page-walk parity with an unwindowed fetch for pre-turn content such as early context-window.updated activities.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 95cd34a. Configure here.
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…ffect beta.103 67 upstream commits (through 6b73b3d): paginated thread loading with user-anchored turn windows (pingdotgg#5493), contract-backed fonts (pingdotgg#5103), subagent/workflow observability agents panel (pingdotgg#5219), sidebar-v2 thread pinning (pingdotgg#5312, migrations 036/037), scannable pairing QR (pingdotgg#5360), MCP tool-result trimming (pingdotgg#5482), renderer OOM containment (pingdotgg#5148), tunnel survives updates/stops, terminal polish batch, forward-compat ServerProviders decode (pingdotgg#5327), Effect beta.103, many fixes. Forma preserved: composer surface/footer, ComposerMetaBar, diff panel skin, RightPanelTabs Forma strip/icons with BOTH componentPreview and agents surfaces, sidebar tone map, forma:// CORS origins, ~/.forma, fork migrations 935-941 after upstream 036/037, fork contract fields alongside fonts/threadPinning. Fonts unification starts (terminal now settings-driven); fix-forward follows. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Eight commits, headlined by pingdotgg#5493 "paginate thread loading with user-anchored turn windows" -- 2093 insertions plus a keyset index migration. Candidate #29 ("perf: import bounded web thread history", upstream pingdotgg#4018) is confirmed superseded: upstream never merged pingdotgg#4018 and ships loadOlderTurns instead. The candidate is removed from web, mobile and client-runtime -- olderThreadActivities.ts deleted, ChatView and the mobile composer/feed/screens rewired to upstream's loadEarlier model. The SERVER half stays: deployed mobile builds still call orchestration.getThreadActivities, so the RPC, its schemas and the activity window constant are retained as a compatibility surface and marked as such. All 24 textual conflicts resolved to upstream. The expensive work was what git auto-merged wrong or left dangling, found by typecheck and tests, not markers: - threads.ts: upstream's applyItemLocked header welded onto the fork's batch reducer body, referencing an out-of-scope identifier and silently dropping upstream's synchronized branch. The fork's batching layer (groupedWithin, reduceThreadStreamItems, eventBatchSize) is removed with its two tests; the fork's load-once HTTP fallback guard is reimplemented on upstream's model and its regression test passes again. - ws.ts: the fork's reuseBaseBranch worktree flow and upstream's pingdotgg#5556 no-origin fallback are combined; neither side alone compiled. - ProjectionSnapshotQuery: upstream's new windowed message query lacked the fork's source_json column, failing decode on every windowed read; the bounded detail query destructured nine results from seven queries -- the fork's queued-messages and pending-turn-start members are restored. - BranchToolbar/SidebarV2/MessagesTimeline/ChatView: prop and rename skews reconciled; fork surface-existence assertion updated for the new feed call. - Migration ledger fixtures extended for upstream migration 037, which lands in the upstream namespace and does not collide with the fork's renumbered 037. - Upstream's new tests adapted to fork-required fields (queuedMessages, pendingTurnStart) and the fork's projection-wait in bootstrap. Verified: full recursive typecheck clean across 17 packages; 2264 tests pass including upstream's 11 pagination and 7 windowed-detail tests. The single failure (CodexTextGeneration structured output) predates this merge. fork/tim touches threads.ts, ProjectionSnapshotQuery and contracts; no tim commit is merged upstream, so review those diffs with tim provenance in mind. Co-authored-by: Patrick Roza <42661+patroza@users.noreply.github.com>
Eight commits, headlined by pingdotgg#5493 "paginate thread loading with user-anchored turn windows" -- 2093 insertions plus a keyset index migration. Candidate #29 ("perf: import bounded web thread history", upstream pingdotgg#4018) is confirmed superseded: upstream never merged pingdotgg#4018 and ships loadOlderTurns instead. The candidate is removed from web, mobile and client-runtime -- olderThreadActivities.ts deleted, ChatView and the mobile composer/feed/screens rewired to upstream's loadEarlier model. The SERVER half stays: deployed mobile builds still call orchestration.getThreadActivities, so the RPC, its schemas and the activity window constant are retained as a compatibility surface and marked as such. All 24 textual conflicts resolved to upstream. The expensive work was what git auto-merged wrong or left dangling, found by typecheck and tests, not markers: - threads.ts: upstream's applyItemLocked header welded onto the fork's batch reducer body, referencing an out-of-scope identifier and silently dropping upstream's synchronized branch. The fork's batching layer (groupedWithin, reduceThreadStreamItems, eventBatchSize) is removed with its two tests; the fork's load-once HTTP fallback guard is reimplemented on upstream's model and its regression test passes again. - ws.ts: the fork's reuseBaseBranch worktree flow and upstream's pingdotgg#5556 no-origin fallback are combined; neither side alone compiled. - ProjectionSnapshotQuery: upstream's new windowed message query lacked the fork's source_json column, failing decode on every windowed read; the bounded detail query destructured nine results from seven queries -- the fork's queued-messages and pending-turn-start members are restored. - BranchToolbar/SidebarV2/MessagesTimeline/ChatView: prop and rename skews reconciled; fork surface-existence assertion updated for the new feed call. - Migration ledger fixtures extended for upstream migration 037, which lands in the upstream namespace and does not collide with the fork's renumbered 037. - Upstream's new tests adapted to fork-required fields (queuedMessages, pendingTurnStart) and the fork's projection-wait in bootstrap. Verified: full recursive typecheck clean across 17 packages; 2264 tests pass including upstream's 11 pagination and 7 windowed-detail tests. The single failure (CodexTextGeneration structured output) predates this merge. fork/tim touches threads.ts, ProjectionSnapshotQuery and contracts; no tim commit is merged upstream, so review those diffs with tim provenance in mind. Co-authored-by: Patrick Roza <42661+patroza@users.noreply.github.com>
Eight commits, headlined by pingdotgg#5493 "paginate thread loading with user-anchored turn windows" -- 2093 insertions plus a keyset index migration. Candidate #29 ("perf: import bounded web thread history", upstream pingdotgg#4018) is confirmed superseded: upstream never merged pingdotgg#4018 and ships loadOlderTurns instead. The candidate is removed from web, mobile and client-runtime -- olderThreadActivities.ts deleted, ChatView and the mobile composer/feed/screens rewired to upstream's loadEarlier model. The SERVER half stays: deployed mobile builds still call orchestration.getThreadActivities, so the RPC, its schemas and the activity window constant are retained as a compatibility surface and marked as such. All 24 textual conflicts resolved to upstream. The expensive work was what git auto-merged wrong or left dangling, found by typecheck and tests, not markers: - threads.ts: upstream's applyItemLocked header welded onto the fork's batch reducer body, referencing an out-of-scope identifier and silently dropping upstream's synchronized branch. The fork's batching layer (groupedWithin, reduceThreadStreamItems, eventBatchSize) is removed with its two tests; the fork's load-once HTTP fallback guard is reimplemented on upstream's model and its regression test passes again. - ws.ts: the fork's reuseBaseBranch worktree flow and upstream's pingdotgg#5556 no-origin fallback are combined; neither side alone compiled. - ProjectionSnapshotQuery: upstream's new windowed message query lacked the fork's source_json column, failing decode on every windowed read; the bounded detail query destructured nine results from seven queries -- the fork's queued-messages and pending-turn-start members are restored. - BranchToolbar/SidebarV2/MessagesTimeline/ChatView: prop and rename skews reconciled; fork surface-existence assertion updated for the new feed call. - Migration ledger fixtures extended for upstream migration 037, which lands in the upstream namespace and does not collide with the fork's renumbered 037. - Upstream's new tests adapted to fork-required fields (queuedMessages, pendingTurnStart) and the fork's projection-wait in bootstrap. Verified: full recursive typecheck clean across 17 packages; 2264 tests pass including upstream's 11 pagination and 7 windowed-detail tests. The single failure (CodexTextGeneration structured output) predates this merge. fork/tim touches threads.ts, ProjectionSnapshotQuery and contracts; no tim commit is merged upstream, so review those diffs with tim provenance in mind. Co-authored-by: Patrick Roza <42661+patroza@users.noreply.github.com>
Eight commits, headlined by pingdotgg#5493 "paginate thread loading with user-anchored turn windows" -- 2093 insertions plus a keyset index migration. Candidate #29 ("perf: import bounded web thread history", upstream pingdotgg#4018) is confirmed superseded: upstream never merged pingdotgg#4018 and ships loadOlderTurns instead. The candidate is removed from web, mobile and client-runtime -- olderThreadActivities.ts deleted, ChatView and the mobile composer/feed/screens rewired to upstream's loadEarlier model. The SERVER half stays: deployed mobile builds still call orchestration.getThreadActivities, so the RPC, its schemas and the activity window constant are retained as a compatibility surface and marked as such. All 24 textual conflicts resolved to upstream. The expensive work was what git auto-merged wrong or left dangling, found by typecheck and tests, not markers: - threads.ts: upstream's applyItemLocked header welded onto the fork's batch reducer body, referencing an out-of-scope identifier and silently dropping upstream's synchronized branch. The fork's batching layer (groupedWithin, reduceThreadStreamItems, eventBatchSize) is removed with its two tests; the fork's load-once HTTP fallback guard is reimplemented on upstream's model and its regression test passes again. - ws.ts: the fork's reuseBaseBranch worktree flow and upstream's pingdotgg#5556 no-origin fallback are combined; neither side alone compiled. - ProjectionSnapshotQuery: upstream's new windowed message query lacked the fork's source_json column, failing decode on every windowed read; the bounded detail query destructured nine results from seven queries -- the fork's queued-messages and pending-turn-start members are restored. - BranchToolbar/SidebarV2/MessagesTimeline/ChatView: prop and rename skews reconciled; fork surface-existence assertion updated for the new feed call. - Migration ledger fixtures extended for upstream migration 037, which lands in the upstream namespace and does not collide with the fork's renumbered 037. - Upstream's new tests adapted to fork-required fields (queuedMessages, pendingTurnStart) and the fork's projection-wait in bootstrap. Verified: full recursive typecheck clean across 17 packages; 2264 tests pass including upstream's 11 pagination and 7 windowed-detail tests. The single failure (CodexTextGeneration structured output) predates this merge. fork/tim touches threads.ts, ProjectionSnapshotQuery and contracts; no tim commit is merged upstream, so review those diffs with tim provenance in mind. Co-authored-by: Patrick Roza <42661+patroza@users.noreply.github.com>
Bring upstream pingdotgg#5493 thread turn windows, loadEarlier/keyset reads, and client-runtime thread sync onto current fork/dev as a single linear commit (rebase-mergeable). Drop superseded getThreadActivities/olderThreadActivities; keep fork queue/identity/pendingTurnStart and surface markers. Includes the green CI tip fixes (migration 37 ledger, source_json projection, remoteExists bootstrap mock, mobile surface tests).


Problem
Long threads are heavy to open, especially on mobile. The heaviest observed thread (161 turns, 23k activities) ships 8.4MB of JSON on every open, all of which must be parsed, held in client state, and persisted to the mobile cache. On many-turn threads, the most recent 10 turns are only 2-6% of the bytes.
Solution
Opt-in pagination of the thread detail snapshot, cut on user-anchored turn boundaries:
GET /api/orchestration/threads/:idacceptsturnLimit+beforeCursor. The window is everything from the Nth-last turn-with-a-user-pending-message onward, so subagent/fan-out turns ride along and the first page always contains the last N user prompts (verified against real data: fan-out bursts run 35+ consecutive subagent turns). Responses carrypage: { beforeCursor, hasMore, snapshotSequence }with an opaque exclusive cursor returning disjoint older slices. A 150-raw-turn ceiling bounds pathological fan-out. The WS fallback snapshot honors the same opt-in via the subscription input.threadSnapshotPaginationin server config). Old client + new server and new client + old server both keep full-snapshot behavior.loadOlderTurnsfetches 20 more per call. Consistency rules: a fresh snapshot replaces all loaded history (no stale-revert resurrection), in-flight pages are discarded when a revert/snapshot/deletion rewrites history or when the page was read from a projection behind the loaded state, and merged pages never advance the live-event dedupe sequence. All covered by state-machine tests.maintainVisibleContentPositionanchors scroll on prepend.No schema migration: the migration-029 indexes cover the bounded queries (verified with EXPLAIN QUERY PLAN against a 45k-activity fixture; worst-case bounded reads run in single-digit ms).
Measured on the heaviest real thread: initial fetch drops from 8.4MB to 1.0MB wire (~1/8th), and a full page-by-page walk reproduces the exact row set of an unwindowed fetch with zero overlap between pages.
Testing
Implemented by Claude Fable 5 via Claude Code.
🤖 Generated with Claude Code
Note
Medium Risk
Touches core thread sync, projection queries, and cache semantics; mistakes could drop history, duplicate streaming text, or serve wrong pages, but behavior is opt-in, capability-gated, and heavily tested.
Overview
Adds opt-in, capability-gated thread detail pagination so long threads open with a small recent window instead of the full history.
Server exposes
turnLimitandbeforeCursoron HTTP thread snapshots and WS subscribe fallbacks. Windowing walks back user-anchored turns (subagent turns ride along), returnspagemetadata with an opaque keyset cursor, and uses a newprojection_turnskeyset index. Malformed or foreign cursors degrade to the first page.Client-runtime loads the last 10 user turns initially and fetches 20 more per “load earlier” via
requestOlderThreadTurns, with epoch/lock/watermark rules so reverts, fresh snapshots, and stale pages cannot corrupt merged history. Thread cache schema bumps to v3 so old clients cannot treat a partial window as complete.Web and mobile show a Load earlier turns control in the thread feed when
hasMoreis set.Reviewed by Cursor Bugbot for commit 95cd34a. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Paginate thread loading with user-anchored turn windows on client and server
ProjectionSnapshotQuery, HTTP and WS handlers), returning only the last 10 user-anchored turns initially and supporting cursor-based older-page fetches of 20 turns at a time.EnvironmentThreadStatewith pagination state (page,loadingOlder,hasMore, cursor) and addsrequestOlderThreadTurns/threadHasOlderTurnshelpers in the client-runtime state machine.applyLock) and epoch/watermark guards in the thread state machine to prevent stale or interleaved older-page merges from corrupting live history.MessagesTimeline) and mobile (ThreadFeed) UIs, wired torequestOlderThreadTurnsand reflecting loading state.projection_turns(thread_id, requested_at, turn_id)to support efficient paginated queries.threadSnapshotPaginationinServerConfigwill have any windowed cache discarded and reload the full thread history.Macroscope summarized 95cd34a.