Centralize app persistence in SQLite and harden state/event flows - #86
Conversation
|
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 You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughIntroduces a centralized SQLite persistence layer (.t3/state.sqlite) with a new PersistenceService for durable server-side state management of projects, threads, messages, and turn summaries. Establishes state event protocol (bootstrap, catchUp, listMessages) for client-server synchronization, removes legacy localStorage migration paths, and updates desktop/web clients to use server-authoritative persistence via new API surfaces. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Web Client
participant WS as WebSocket Server
participant Persistence as PersistenceService
participant StateDb as SQLite State DB
Client->>WS: state.bootstrap()
WS->>Persistence: loadSnapshot()
Persistence->>StateDb: Query projects, threads, messages
StateDb-->>Persistence: Return snapshot data
Persistence-->>WS: StateBootstrapResult
WS-->>Client: Bootstrap complete
Client->>Client: HYDRATE_FROM_SERVER
loop Event Synchronization
Persistence->>StateDb: Emit stateEvent (upsert/delete)
StateDb-->>Persistence: Transaction committed
Persistence->>WS: emit('stateEvent')
WS->>Client: broadcast stateEvent
Client->>Client: APPLY_STATE_EVENT
end
sequenceDiagram
participant Provider as Provider/ProviderManager
participant Persistence as PersistenceService
participant StateDb as SQLite State DB
participant WS as WebSocket Server
Provider->>Persistence: ingestProviderEvent(event)
Persistence->>StateDb: Transactional write (message, turn summary, state event)
StateDb-->>Persistence: Transaction committed
Persistence->>Persistence: Emit stateEvent
Persistence->>WS: Broadcast stateEvent
WS->>Client: state.event (with seq metadata)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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)
Comment |
Centralize server state in SQLite via
|
Greptile SummaryMigrated canonical app state from client-side Key Changes:
Implementation Quality:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client as Web Client
participant WS as WebSocket Server
participant PS as PersistenceService
participant DB as SQLite DB
participant PM as ProviderManager
Note over Client,DB: Initial Bootstrap Flow
Client->>WS: state.bootstrap()
WS->>PS: loadSnapshot()
PS->>DB: SELECT projects, threads, messages
DB-->>PS: State snapshot + lastStateSeq
PS-->>WS: StateBootstrapResult
WS-->>Client: {projects, threads, lastStateSeq}
Client->>Client: dispatch(HYDRATE_FROM_SERVER)
Note over Client,DB: Provider Event Ingestion
PM->>PS: ingestProviderEvent(event)
PS->>DB: INSERT OR IGNORE into provider_events
PS->>DB: UPDATE thread, message, turn_summary
PS->>DB: INSERT state_event
DB-->>PS: event.seq
PS->>PS: emit('stateEvent', event)
PS-->>PM: ProviderEvent with seq
PM->>WS: emit('event', providerEvent)
WS->>Client: push providers.event
WS->>Client: push state.event
Client->>Client: dispatch(APPLY_STATE_EVENT)
Note over Client,DB: Catch-Up After Reconnect
Client->>WS: state.catchUp({afterSeq: lastSeq})
WS->>PS: catchUp(afterSeq)
PS->>DB: SELECT from state_events WHERE seq > ?
DB-->>PS: Missing events
PS-->>WS: {events, lastStateSeq}
WS-->>Client: StateCatchUpResult
Client->>Client: Replay missing events
Note over Client,DB: Thread Operations
Client->>WS: threads.create({projectId, title})
WS->>PS: createThread(input)
PS->>DB: BEGIN IMMEDIATE
PS->>DB: INSERT thread document
PS->>DB: INSERT state_event
PS->>DB: COMMIT
PS->>PS: emit('stateEvent')
PS-->>WS: {thread}
WS->>Client: push state.event
WS-->>Client: ThreadsUpdateResult
Client->>Client: dispatch(APPLY_STATE_EVENT)
Last reviewed commit: 451b295 |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| const input = stateCatchUpInputSchema.parse(raw); | ||
| const rows = this.db | ||
| .prepare( | ||
| "SELECT seq, event_type, entity_id, payload_json, created_at FROM state_events WHERE seq > ? ORDER BY seq ASC;", |
There was a problem hiding this comment.
🟡 Medium
src/persistenceService.ts:775 This query has no LIMIT clause, so a low afterSeq could load the entire event history into memory. Consider adding pagination or a LIMIT to prevent potential OOM issues, or document why unbounded results are acceptable here.
🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file apps/server/src/persistenceService.ts around line 775:
This query has no `LIMIT` clause, so a low `afterSeq` could load the entire event history into memory. Consider adding pagination or a `LIMIT` to prevent potential OOM issues, or document why unbounded results are acceptable here.
Evidence trail:
apps/server/src/persistenceService.ts lines 773-778 at commit 451b295d56e35deb8970760592be311e0f4c457c - shows SQL query `SELECT seq, event_type, entity_id, payload_json, created_at FROM state_events WHERE seq > ? ORDER BY seq ASC;` with no LIMIT clause, results loaded via `.all(input.afterSeq)`.
| const raw = fs.readFileSync(normalizedPath, "utf8"); | ||
| const payload = JSON.parse(raw) as { projects?: unknown }; | ||
| const candidates = Array.isArray(payload.projects) ? payload.projects : []; | ||
| for (const candidate of candidates) { |
There was a problem hiding this comment.
🟡 Medium
src/persistenceService.ts:1386 The try/catch wraps the entire loop, so one failing project aborts all remaining imports. Consider moving the try/catch inside the for loop so each candidate is handled independently.
🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file apps/server/src/persistenceService.ts around line 1386:
The `try/catch` wraps the entire loop, so one failing project aborts all remaining imports. Consider moving the `try/catch` inside the `for` loop so each candidate is handled independently.
Evidence trail:
apps/server/src/persistenceService.ts lines 1382-1405 at commit 451b295d56e35deb8970760592be311e0f4c457c. The `try` block starts at line 1382, the `for (const candidate of candidates)` loop runs from approximately line 1387-1402, and the `catch` block is at line 1403. This confirms the try/catch wraps the entire loop.
| } | ||
| } | ||
|
|
||
| if (!hasOptimisticWriteSinceHydration()) { |
There was a problem hiding this comment.
🟠 High
src/appSettings.ts:169 Race condition: legacy settings are removed unconditionally at line 184, but the migration that persists them (lines 172-176) may be skipped if hasOptimisticWriteSinceHydration() is true. Consider only removing legacy settings when the migration actually ran.
🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file apps/web/src/appSettings.ts around line 169:
Race condition: legacy settings are removed unconditionally at line 184, but the migration that persists them (lines 172-176) may be skipped if `hasOptimisticWriteSinceHydration()` is true. Consider only removing legacy settings when the migration actually ran.
Evidence trail:
apps/web/src/appSettings.ts lines 160-195 at commit 451b295d56e35deb8970760592be311e0f4c457c. The `persistLocalSettings` call (lines 172-176) is inside the `if (!hasOptimisticWriteSinceHydration())` block (line 169), while `window.localStorage.removeItem(LEGACY_APP_SETTINGS_STORAGE_KEY)` (around line 184) is outside that conditional, after `backendHydrated = true` is set.
| } catch (error) { | ||
| db.exec("ROLLBACK;"); |
There was a problem hiding this comment.
🟡 Medium
src/stateMigrations.ts:92 If ROLLBACK throws (e.g., closed connection), the original error is masked. Consider wrapping the rollback in a try-catch to preserve the original error.
- } catch (error) {
- db.exec("ROLLBACK;");
+ } catch (error) {
+ try {
+ db.exec("ROLLBACK;");
+ } catch {
+ // Ignore rollback failure to preserve original error
+ }🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file apps/server/src/stateMigrations.ts around lines 92-93:
If `ROLLBACK` throws (e.g., closed connection), the original error is masked. Consider wrapping the rollback in a try-catch to preserve the original error.
Evidence trail:
apps/server/src/stateMigrations.ts lines 91-95 at commit 451b295d56e35deb8970760592be311e0f4c457c. The catch block at line 92-95 shows `db.exec("ROLLBACK;");` followed by `throw error;` with no try-catch around the ROLLBACK call.
| export function applyStateDbPragmas(db: SqliteDatabase): void { | ||
| db.exec("PRAGMA journal_mode=WAL;"); | ||
| db.exec("PRAGMA synchronous=FULL;"); | ||
| db.exec("PRAGMA busy_timeout=5000;"); | ||
| db.exec("PRAGMA foreign_keys=ON;"); | ||
| } |
There was a problem hiding this comment.
🟡 Medium
src/stateMigrations.ts:5 Consider setting busy_timeout before journal_mode=WAL since changing to WAL requires an exclusive lock and will fail immediately with SQLITE_BUSY if another process holds the lock.
| export function applyStateDbPragmas(db: SqliteDatabase): void { | |
| db.exec("PRAGMA journal_mode=WAL;"); | |
| db.exec("PRAGMA synchronous=FULL;"); | |
| db.exec("PRAGMA busy_timeout=5000;"); | |
| db.exec("PRAGMA foreign_keys=ON;"); | |
| } | |
| export function applyStateDbPragmas(db: SqliteDatabase): void { | |
| db.exec("PRAGMA busy_timeout=5000;"); | |
| db.exec("PRAGMA journal_mode=WAL;"); | |
| db.exec("PRAGMA synchronous=FULL;"); | |
| db.exec("PRAGMA foreign_keys=ON;"); | |
| } |
🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file apps/server/src/stateMigrations.ts around lines 5-10:
Consider setting `busy_timeout` before `journal_mode=WAL` since changing to WAL requires an exclusive lock and will fail immediately with `SQLITE_BUSY` if another process holds the lock.
Evidence trail:
apps/server/src/stateMigrations.ts lines 5-10 at commit 451b295d56e35deb8970760592be311e0f4c457c - shows `journal_mode=WAL` on line 6 and `busy_timeout=5000` on line 8, confirming the ordering issue described in the claim.
|
|
||
| updateProjectScripts(raw: ProjectUpdateScriptsInput): ProjectUpdateScriptsResult { | ||
| const input = projectUpdateScriptsInputSchema.parse(raw); | ||
| const existing = this.getProjectById(input.id); |
There was a problem hiding this comment.
🟡 Medium
src/persistenceService.ts:588 Suggestion: Make read–modify–write operations atomic. getProjectById and getThreadById are called before entering withTransaction, so another writer can change the row between the read and the write, losing updates. Consider moving the reads into the same withTransaction block (or using a single atomic upsert) to avoid races.
🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file apps/server/src/persistenceService.ts around line 588:
Suggestion: Make read–modify–write operations atomic. `getProjectById` and `getThreadById` are called before entering `withTransaction`, so another writer can change the row between the read and the write, losing updates. Consider moving the reads into the same `withTransaction` block (or using a single atomic upsert) to avoid races.
Evidence trail:
apps/server/src/persistenceService.ts line 587: `const existing = this.getProjectById(input.id);` (read before transaction)
apps/server/src/persistenceService.ts line 597: `this.withTransaction((pendingEvents) => {` (transaction starts after read)
apps/server/src/persistenceService.ts lines 1343-1358: `updateThreadWith` method definition
apps/server/src/persistenceService.ts line 1347: `const existing = this.getThreadById(threadId);` (read before transaction)
apps/server/src/persistenceService.ts line 1353: `this.withTransaction((pendingEvents) => {` (transaction starts after read)
Add durable provider event sequencing backed by SQLite and expose
|
- Persist provider event `seq` metadata and return persisted events from ingestion - Add `providers.catchUp` WS/API contract and server persistence query - Replay missing provider events on web reconnect using ordered sequence catch-up - Extend server/contracts tests for sequence ordering and catch-up behavior
451b295 to
f8aadcf
Compare
| } | ||
| }; | ||
|
|
||
| const enqueueProviderWork = (work: () => Promise<void>) => { |
There was a problem hiding this comment.
🟠 High routes/__root.tsx:260
The queued async work in providerQueueRef continues executing after effect cleanup. Consider adding a disposed flag (similar to the bootstrap effect) and checking it before calling applyProviderEvent or updating refs, to prevent stale closures from dispatching after unmount or re-subscription.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/web/src/routes/__root.tsx around line 260:
The queued async work in `providerQueueRef` continues executing after effect cleanup. Consider adding a `disposed` flag (similar to the bootstrap effect) and checking it before calling `applyProviderEvent` or updating refs, to prevent stale closures from dispatching after unmount or re-subscription.
Evidence trail:
apps/web/src/routes/__root.tsx lines 144, 214-298 (commit f8aadcff890d5bc7a8741d048d9c1d034bb91333): `providerQueueRef` defined at line 144, `enqueueProviderWork` at lines 260-262 queues async work, `replayProviderCatchUp` at lines 239-257 calls `applyProviderEvent()` after async operations, cleanup at lines 294-297 only calls unsubscribe functions with no disposed flag. Compare to bootstrap effect at lines 146-179 which has `disposed` flag at line 148 and checks at lines 155, 162.
| } | ||
|
|
||
| if (lastProviderSeqRef.current < catchUp.lastProviderSeq) { | ||
| await replayProviderCatchUp(); |
There was a problem hiding this comment.
🟠 High routes/__root.tsx:253
If all events in catchUp.events are skipped (filtered out at lines 249-250), lastProviderSeqRef.current stays unchanged while the recursion check at line 255 still triggers, causing infinite recursion. Consider updating lastProviderSeqRef.current to catchUp.lastProviderSeq after the loop to ensure progress.
- lastProviderSeqRef.current = missingSeq;
- }
-
- if (lastProviderSeqRef.current < catchUp.lastProviderSeq) {
+ lastProviderSeqRef.current = missingSeq;
+ }
+ lastProviderSeqRef.current = Math.max(lastProviderSeqRef.current, catchUp.lastProviderSeq);
+
+ if (lastProviderSeqRef.current < catchUp.lastProviderSeq) {🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/web/src/routes/__root.tsx around lines 253-256:
If all events in `catchUp.events` are skipped (filtered out at lines 249-250), `lastProviderSeqRef.current` stays unchanged while the recursion check at line 255 still triggers, causing infinite recursion. Consider updating `lastProviderSeqRef.current` to `catchUp.lastProviderSeq` after the loop to ensure progress.
Evidence trail:
apps/web/src/routes/__root.tsx lines 240-257 at commit f8aadcff890d5bc7a8741d048d9c1d034bb91333: Line 243-246 handles empty events case with early return and update. Lines 248-253 show loop with `continue` condition that can skip events without updating `lastProviderSeqRef.current`. Lines 254-256 show recursion check that can trigger when `lastProviderSeqRef.current < catchUp.lastProviderSeq` even if no events were processed in the loop.
1 participant
Summary
~/.t3/state.sqlite) via a newPersistenceService, covering projects, threads, messages, app settings, and state event sequencing.stateDb, migrations, sqlite adapter) and wire server startup/runtime to the new persistence path.localStoragemigration/schema paths; keep browser-scoped UX preferences separate from canonical state.Testing
bun run lintbun --cwd apps/server testbun --cwd apps/web testbun --cwd packages/contracts testNote
Medium Risk
Touches core provider streaming/persistence paths and introduces new replay logic; bugs could cause dropped/duplicated provider events or inconsistent UI state on reconnect.
Overview
Adds durable provider-event sequencing and replay.
ProviderEventnow optionally includes a persistedseq,PersistenceService.ingestProviderEventreturns the stored event (ornullwhen ignored), and a newPersistenceService.providerCatchUpendpoint pages provider events by sequence and reportslastProviderSeq.Wires catch-up through server + client. WebSocket RPC adds
providers.catchUp,ProviderManagercentralizes provider event publishing to emit the persisted/seq-tagged version (and suppress emission when persistence dedupes), and the web client now trackslastProviderSeqto replay missing provider events on reconnect/gaps for ordered, idempotent application.Tests update/expand to assert monotonic provider
seqassignment, correct catch-up ordering/filters, and propagation of persisted sequence metadata throughProviderManagerand WebSocket.Written by Cursor Bugbot for commit f8aadcf. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Release Notes