feat: complete /api/import and /api/export with asset support, tests, and docs#91
feat: complete /api/import and /api/export with asset support, tests, and docs#91elucid wants to merge 6 commits into
Conversation
Make importData mandatory on Store implementations and add listSettings for complete board exports. JsonFileStore and SqlStore now import base64-encoded assets idempotently while preserving IDs and metadata. Extend the shared store contract to cover round-trip imports, idempotency, and partial imports.
Add /api/import and /api/export coverage for auth gating, invalid JSON, ID preservation, and full round-trips. Export now includes all settings plus base64-encoded assets fetched from the store, and import responses report asset counts.
Add a release changeset for the new board import/export endpoints and document backup/migration curl examples for deployed boards. Leave the implementation handoff plan as an uncommitted local artifact.
elucid
left a comment
There was a problem hiding this comment.
Code Review
Reviewed: PR #91 import/export completion (assets, settings, store/API tests)
Verdict: NEEDS CHANGES
Summary
The PR substantially improves import/export and the happy-path tests pass, but it does not preserve the full board state yet. Session trace steps are omitted entirely, and one real asset lifecycle edge case still drops assets during export.
Findings
[P1] Session trace steps are not included in import/export
File: server/types.ts:267 / server/app.ts:622
Issue: The store persists session trace state (JsonFileStore's trace map and SqlStore's trace_steps table), and the plan/review criteria call out trace steps as part of the data that must round-trip. ImportData has no trace field, /api/export only returns sessions/surfaces/comments/assets/settings, and both importData implementations ignore traces. Exporting a board with synced trace data and importing it into another store loses every session's trace.
Suggested Fix: Add trace data to the import/export shape (for example trace?: Record<string, TraceStep[]> keyed by session id), export it with store.listTrace(session.id) for each session, and import it in both stores preserving step order. Add store-contract and API round-trip tests that set trace steps, export/import, and verify GET /api/sessions/:id/trace returns the original steps.
[P1] Export skips assets whose owning session was deleted but which are still referenced by surviving surfaces
File: server/app.ts:628
Issue: /api/export discovers assets by iterating current sessions and calling store.listAssets(session.id). That misses a valid store state this codebase explicitly supports: content-addressed assets can be referenced by surfaces in other sessions, and removeSession keeps a deleted session's asset if a surviving surface still references it. In that state, the surface is exported with an assetId, but the asset bytes are omitted, so an imported backup renders broken image/trace/file references.
Suggested Fix: Export assets independently of current sessions. The most complete fix is adding a listAllAssets()/equivalent Store method and using it in /api/export; alternatively, at minimum collect asset ids from all current and historical surface parts and getAsset() any referenced assets not found through listAssets(session.id). Add a regression test that uploads an asset in session A, references it from a surface in session B, deletes session A, exports/imports, and verifies the referenced asset is present and serveable.
What's Good
- Asset data is encoded/decoded in a cross-runtime-compatible way, with chunked export to avoid spread stack issues on larger blobs.
- Settings export now uses a generic
listSettings()instead of hard-codingtheme. - The new tests cover auth, malformed JSON, idempotent store imports, partial imports, and JSON/SQL store round-trips for the entities currently in the import shape.
Validation Run
npm test— passed (192 tests)npm run typecheck— passednpm run lint— passednpm run format:check— passed
elucid
left a comment
There was a problem hiding this comment.
Code Review
Reviewed: PR #91 import/export implementation against docs/import-export-plan.md
Verdict: NEEDS CHANGES
Summary
The PR substantially implements sessions, surfaces, comments, assets, settings, and trace import/export, and the new tests/typecheck pass locally. However it omits the persisted comment sequence high-water mark, which can make migrated sessions silently stop delivering new user feedback to agents.
Findings
[P1] Export/import drops comment sequence high-water state
File: server/app.ts:635, server/storage.ts:483, workers/sqlStore.ts:590
Issue: The export shape includes existing comments but not the store's comment sequence high-water mark (JsonFileStore.lastSeq, and SQLite's AUTOINCREMENT sequence). On import, both stores only advance their next comment seq from rows that still exist. If a source board has had comments deleted after being delivered to the agent, the exported session can still have agentSeq greater than the max exported comment seq. After import, the next new comment can reuse a seq <= agentSeq, so listComments({ afterSeq: session.agentSeq }) / agent waits never deliver that new feedback.
Concrete repro from the code path:
- create a session + surface + comment with
seq = 1 markAgentSeen(session, 1)sosession.agentSeq = 1- delete the surface, which removes the comment but leaves the session and cursor
- export/import into a fresh store
- create a new comment: it gets
seq = 1, and agent reads afteragentSeq = 1, so it is filtered out forever
This violates the feedback delivery invariant and means the board is not preserved identically across migration.
Suggested Fix: Include the comment sequence high-water mark in ImportData//api/export (or otherwise expose it via the Store interface), and restore it in both stores. At minimum, imported stores must set their next comment seq above max(imported comments.seq, imported sessions.agentSeq), but exporting/restoring the actual high-water mark is the exact preservation fix. For SQLite, also update sqlite_sequence for the comments table. Add a store/API test that exports a session whose delivered comments were deleted, imports it, creates a new user comment, and verifies its seq is greater than the imported agentSeq and is returned by an afterSeq read.
What's Good
- Asset bytes are included and base64 encoded/decoded for both store implementations.
- Settings are exported generically via
listSettings()instead of hard-codingtheme. - Trace steps are included in API/store round-trip coverage.
- Local validation:
npm testpassed (193 tests);npm run typecheckpassed.
elucid
left a comment
There was a problem hiding this comment.
Code Review
Reviewed: PR #91 import/export implementation (Store interface, JsonFileStore, SqlStore, API endpoints, tests/docs)
Verdict: NEEDS CHANGES
Summary
This substantially implements the planned import/export shape for sessions, surfaces, comments, assets, settings, and trace steps, and the happy-path coverage is strong across both store implementations. I found two material edge cases that can either silently lose imported comments or make export unusable for boards with larger asset sets.
Findings
[P1] Comment sequence collisions are silently treated as success, dropping imported feedback
File: server/storage.ts:490 and workers/sqlStore.ts:608
Issue: Import preserves each comment's seq, but both stores silently skip a comment when its sequence number already exists in the target with a different comment id. In JsonFileStore, the dedupe condition is x.id === c.id || x.seq === c.seq; in SqlStore, INSERT OR IGNORE ignores the row because seq is the primary key. This is idempotent for importing the same backup twice, but it loses data for any partial/non-empty import where the target has already allocated the same seq (very likely, since every board starts comments at 1). /api/import still reports the incoming comment count, so the operator gets a successful migration response with missing comments.
Suggested Fix: Distinguish idempotency from conflict. If the same comment id already exists, skip it only if it is the same record. If a seq exists for a different id, fail the import with a conflict (or require/validate an empty target for full-board imports). Add store-contract and API tests for a seq collision so the behavior is consistent across JsonFileStore and SqlStore.
[P1] Export materializes all raw assets and their base64 copies in memory
File: server/app.ts:622
Issue: GET /api/export calls store.listAllAssets() (which returns every asset including bytes), then maps those assets to base64 strings, then c.json() serializes the whole export. With the current board asset budget (MAX_BOARD_ASSET_BYTES is 2GB), this can hold raw bytes + base64 strings + the final JSON string at once. A board with many screenshots/trace files can OOM the Node process or Cloudflare Worker during backup, so the import/export feature does not handle the large-asset edge case called out in the plan/review criteria.
Suggested Fix: Stream the export JSON and encode each asset incrementally, or introduce an explicit export size limit/error before loading all bytes. If streaming is too large a change for this PR, lower/scope the supported export asset size and document/test the failure mode rather than crashing.
Tests Run
npm test— passed (196 tests)npm run typecheck— passednpm run lint— passednpm run format:check— passed
What's Good
- The exported shape now covers the important store entities: sessions, surfaces (including history/version), comments, retained assets, settings, trace steps, and comment seq high-water mark.
- Store contract tests exercise both JsonFileStore and SqlStore for round-trip, idempotency, partial import, assets, settings, and comment cursor behavior.
- API tests cover auth, invalid JSON, served imported assets, orphaned-but-referenced assets, and full re-export equality.
Summary
Completes the
/api/importand/api/exportendpoints so they handle the full board shape: sessions, surfaces, comments, assets (base64-encoded), and all settings — not just thethemekey.The two endpoints now round-trip cleanly: exporting from one sideshow instance and importing into another produces an identical board.
Open TODOs
seqcollisions during import without silently dropping feedback. Imports should distinguish true idempotent duplicates from a different comment with an already-used sequence number, and should fail with a clear conflict or otherwise preserve all comments.Changes
Store interface (
server/types.ts)importDatais now mandatory on allStoreimplementations (wasimportData?). Every store must support bulk import.listSettings()added to theStoreinterface — returns all key/value settings so export does not need to hard-code setting names.ImportAssettype added — mirrorsAssetbut carriesdataas a base64 string for JSON transport.ImportDataextended with an optionalassetsarray.JsonFileStore (
server/storage.ts)importDatanow decodes and inserts base64 assets, skipping duplicates by id.listSettingsreturns all persisted settings.idandseqto prevent double-insertion.SqlStore (
workers/sqlStore.ts)importDatainserts assets as BLOBs decoded from base64, usingINSERT OR IGNOREfor idempotency.SELECTby id before insert (SQLiteAUTOINCREMENTonseqmakesINSERT OR IGNOREon the PK insufficient for externally-assigned seq values).listSettingsqueries the settings table.decodeBase64helper (runtime-agnostic, noBuffer).API (
server/app.ts)GET /api/exportnow includesassets(base64-encoded via a chunkedencodeBase64that avoids stack overflow on large blobs) and usesstore.listSettings()instead of hard-codingtheme.POST /api/importresponse now reportsassetscount alongside sessions/surfaces/comments. Removed thestore.importData?optional check — the method is mandatory.Tests
Store contract (
test/storeContract.ts): three new tests run against both JsonFileStore and SqlStore:API tests (
test/importExport.test.ts): five new tests:/a/:id.Documentation
docs/deploying.md: new "Backup and migration" section with curl examples for export/import..changeset/import-export.md: minor changeset for release notes.Validation
All four checks pass: