feat: unify session todo source of truth#873
Conversation
Astro-Han
added
enhancement
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 40 minutes and 18 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughPR implements a unified session todo snapshot model with revision-gated acceptance, replaces timestamp-based freshness heuristics with explicit pending/invalidation flags, introduces a SolidJS hydration coordinator for tracking lifecycle, and refactors backend API/storage to return snapshots with monotonic revision numbers instead of arrays. ChangesUnified Session Todo Snapshot & Hydration Model
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
github-actions
Bot
added
the
harness
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Suggested priority: P2 (includes user-path files (packages/app/src/components/session/session-status-panel.tsx, packages/app/src/components/session/session-status-summary.tsx, packages/app/src/context/global-sync.test.ts, packages/app/src/context/global-sync.tsx, packages/app/src/context/global-sync/bootstrap.ts, packages/app/src/context/global-sync/event-reducer.test.ts, packages/app/src/context/global-sync/event-reducer.ts, packages/app/src/context/global-sync/todo-hydrate-coordinator.test.ts, packages/app/src/context/global-sync/todo-hydrate-coordinator.ts, packages/app/src/context/sync.tsx, packages/app/src/pages/layout.tsx, packages/app/src/pages/session.tsx, packages/app/src/pages/session/session-todos.test.ts, packages/app/src/pages/session/todos/todo-dock-machine.test.ts, packages/app/src/pages/session/todos/todo-dock-machine.ts, packages/app/src/pages/session/todos/todo-model.ts, packages/app/src/pages/session/todos/todo-source.test.ts, packages/app/src/pages/session/todos/todo-source.ts, packages/app/src/pages/session/todos/use-session-todos.test.ts, packages/app/src/pages/session/todos/use-session-todos.ts, packages/app/src/pages/session/use-session-refresh-effects.ts)).
P1/P0 are reserved for maintainer confirmation. Please relabel manually if this is a release blocker, security issue, data-loss risk, or updater/runtime failure.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a revision-based synchronization system for session todos, moving from simple array updates to a snapshot model that includes a monotonic revision number. Key changes include the addition of a session_todo_revision table in the backend, updates to the SDK and API to support TodoSnapshot, and the introduction of a TodoHydrateCoordinator in the frontend to manage authoritative invalidation and recovery epochs. Feedback focuses on ensuring that archived sessions are handled correctly in the API to avoid unintended UI invalidation, optimizing the cleanup of invalidated sessions across directory stores, and improving the efficiency of the frontend store reconciliation by using stable todo IDs.
Perf delta summaryComparator: pass
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/opencode/test/session/todo.test.ts (1)
58-217: 🏗️ Heavy liftPrefer
testEffect+it.livefor these new Effect service tests.These added cases run Effect services and live filesystem/git behavior, so they should be migrated to the standard test harness to match repo test runtime semantics.
As per coding guidelines, “Use
testEffect(...)fromtest/lib/effect.tsfor tests that exercise Effect services or Effect-based workflows” and “Useit.live(...)when the test depends on real time, filesystem mtimes, child processes, git, locks, or other live OS behavior.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/test/session/todo.test.ts` around lines 58 - 217, Replace the plain Jest async tests that exercise Effect services and live FS/git behavior (the tests with titles "update returns a revisioned snapshot and get persists it", "second update preserves id and persists status changes", "empty clear bumps revision and stays authoritative", "get returns rev0 empty only for known sessions that never had todos", "get reads archived sessions while update rejects them", and "get rejects unknown sessions") with the standard test harness by importing testEffect and it from test/lib/effect and wrapping each test body with testEffect(it.live(async () => { ... })), keeping the existing contents (Instance.provide, tmpdir usage, Todo.Service.use calls, Session.create/remove, etc.) unchanged; update top-of-file imports to include { testEffect, it } so the new test declarations compile.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/app/src/context/global-sync.tsx`:
- Around line 257-260: The session-trim branch currently clears child caches via
cleanupDroppedSessionCaches(store, setStore, next) but doesn't notify the
hydrate coordinator about removed session IDs; replicate the earlier pattern
used elsewhere by computing the removed IDs (e.g. compare store.session IDs to
next IDs) and call the todoHydrate "forget trimmed" API with those IDs
immediately after setStore/reconcile and cleanupDroppedSessionCaches (same
change should be applied at the other trim site around lines 294-296); reference
functions: reconcile, setStore, store.session, cleanupDroppedSessionCaches, and
todoHydrate when adding the notification call.
In `@packages/app/src/context/global-sync/todo-hydrate-coordinator.ts`:
- Around line 79-103: touch() currently evicts session IDs from the seen map but
does not fully clear per-session hydrate state, so pending-only sessions keep
stale validatedRecovery/tokenEpoch and can be missed by invalidateSession();
modify touch() (and the similar logic around lines 148-159) to call the same
cleanup used by forgetSession() (i.e. remove pending entries and clear
validatedRecovery and tokenEpoch for that (directory, sessionID)) whenever a
session is evicted or a directory is removed; ensure you reference and update
the pending map, validatedRecovery and tokenEpoch stores (and call
removeDirectoryState or the equivalent cleanup helper) for each evicted
sessionID so reopened sessions cannot inherit stale state.
In `@packages/app/src/pages/session/use-session-refresh-effects.ts`:
- Around line 166-206: Add input.validatedRecoveryEpoch to the on(...) source
tuple so the effect reruns when validated recovery changes: in the
source-returning function include id ?
(input.validatedRecoveryEpoch?.(input.directory(), id) ?? 0) : 0 as an extra
tuple element, then destructure that new value in the callback (e.g.
recoveryValidatedFromSource) and use it instead of recomputing recoveryValidated
inside the callback; this ensures recoveryDue is computed from the fresh
validated value and any pending scheduledTodo/todoFrame/todoTimer will be
cancelled/updated accordingly when validatedRecoveryEpoch changes (references:
input.validatedRecoveryEpoch, scheduledTodo, todoFrame, todoTimer,
syncTodoWithDiagnostics).
In `@packages/opencode/src/session/todo.ts`:
- Around line 99-109: The current pattern uses readSnapshot to validate the
session's active state in a separate transaction but performs the mutation
later, allowing a TOCTOU where the session can be archived between transactions;
fix by performing the active-check and the subsequent todo write inside the same
Database.transaction (or use a SELECT FOR UPDATE/row lock) so the code that
reads SessionTable (SessionTable.id / SessionTable.time_archived) and throws
NotFoundError runs in the exact transaction that performs the update (e.g., move
the active check into the transaction block used by update/updateTodo or combine
readSnapshot logic into the update transaction), ensuring the write only
proceeds when the session is confirmed active.
---
Nitpick comments:
In `@packages/opencode/test/session/todo.test.ts`:
- Around line 58-217: Replace the plain Jest async tests that exercise Effect
services and live FS/git behavior (the tests with titles "update returns a
revisioned snapshot and get persists it", "second update preserves id and
persists status changes", "empty clear bumps revision and stays authoritative",
"get returns rev0 empty only for known sessions that never had todos", "get
reads archived sessions while update rejects them", and "get rejects unknown
sessions") with the standard test harness by importing testEffect and it from
test/lib/effect and wrapping each test body with testEffect(it.live(async () =>
{ ... })), keeping the existing contents (Instance.provide, tmpdir usage,
Todo.Service.use calls, Session.create/remove, etc.) unchanged; update
top-of-file imports to include { testEffect, it } so the new test declarations
compile.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 095fc6a5-6155-452a-82b3-68c95ff2f007
⛔ Files ignored due to path filters (2)
packages/sdk/js/src/gen/types.gen.tsis excluded by!**/gen/**packages/sdk/js/src/v2/gen/types.gen.tsis excluded by!**/gen/**
📒 Files selected for processing (30)
packages/app/src/components/session/session-status-panel.tsxpackages/app/src/components/session/session-status-summary.test.tspackages/app/src/components/session/session-status-summary.tsxpackages/app/src/context/global-sync.test.tspackages/app/src/context/global-sync.tsxpackages/app/src/context/global-sync/bootstrap.tspackages/app/src/context/global-sync/event-reducer.test.tspackages/app/src/context/global-sync/event-reducer.tspackages/app/src/context/global-sync/todo-hydrate-coordinator.test.tspackages/app/src/context/global-sync/todo-hydrate-coordinator.tspackages/app/src/context/sync.tsxpackages/app/src/pages/layout.tsxpackages/app/src/pages/session.tsxpackages/app/src/pages/session/session-todos.test.tspackages/app/src/pages/session/todos/todo-model.test.tspackages/app/src/pages/session/todos/todo-model.tspackages/app/src/pages/session/todos/todo-source.test.tspackages/app/src/pages/session/todos/todo-source.tspackages/app/src/pages/session/use-session-refresh-effects.test.tspackages/app/src/pages/session/use-session-refresh-effects.tspackages/opencode/migration/20260523120000_session_todo_revision/migration.sqlpackages/opencode/src/server/instance/session.tspackages/opencode/src/session/session.sql.tspackages/opencode/src/session/todo.tspackages/opencode/src/storage/json-migration.tspackages/opencode/src/storage/schema.tspackages/opencode/src/tool/todo.tspackages/opencode/test/session/todo.test.tspackages/opencode/test/storage/json-migration.test.tspackages/sdk/openapi.json
💤 Files with no reviewable changes (2)
- packages/app/src/pages/layout.tsx
- packages/app/src/pages/session/todos/todo-model.test.ts
1 participant
Summary
Unifies session todos behind a server-stamped
{ revision, todos }snapshot and one frontend canonical cache.todo.updated,todowritemetadata, OpenAPI, and SDK types to carry the same snapshot shape.devafter refactor: retire todo composer dock #867 so the retired composer ToDo dock stays deleted.Why
Fixes the stale todo source-of-truth split from #650: late REST/SSE/tool metadata or historical transcript parts could roll a completed or empty todo state back to active because each consumer had its own freshness heuristic.
Related Issue
Closes #650.
Human Review Status
Pending
Review Focus
Please focus on the revision contract boundaries:
Todo.get/Todo.updatetransaction behavior and migration backfill.todo.updated, and completedtodowritemetadata.Risk Notes
The migration adds
session_todo_revisionand backfills sessions with existing todo rows to revision1. REST 404/not-found for the current session is now authoritative and clears canonical todo plus local placeholder sources. OpenAPI/SDK generated contracts were updated for the newTodoSnapshotresponse/event shape.After #867, visible todo UI in this PR is limited to the right-side Status summary; the old composer ToDo dock remains deleted.
Skipped checklist item: platform/packaging was left unticked because this does not touch platform, packaging, updater, signing, path, shell, or permission surfaces.
How To Verify
Screenshots or Recordings
Generated and visually reviewed local snap grids:
docs/design/preview/screenshots/todo-status-only.pngdocs/design/preview/screenshots/status-summary-todos.pngChecklist
bug,enhancement,task,documentation. Type labels are author-added; the labeler bot does NOT assign them. Add the label in the GitHub UI, then tick this.app,ui,platform,harness,ci. The labeler bot assigns these on PR open based on changed paths. Confirm the bot's choice (or override if wrong), then tick this.P0,P1,P2,P3. The priority-triage bot suggests one on PR open. Confirm or override, then tick this.Pending,Approved by @<reviewer>, orNot required: <reason>(default isPending; "not required" is restricted to bot-authored low-risk PRs).dev, and my PR title and commit messages use Conventional Commits in English.Summary by CodeRabbit
Release Notes
New Features
Changes