Backups through the CLI - #2838
Conversation
Adds 'instant-cli backup list' and 'instant-cli backup download', built on a new BackupsManager in @instantdb/platform that both surfaces the backup endpoints and documents the archive ordering contract (config.json, then entities/*.jsonl, then files/<locationId>). Also fixes the dashboard treating a 404 from the storage-files endpoint as 'no storage files': the server signals the empty case with a 200 and the done sentinel, so a 404 can only mean the backup is missing or expired, and now fails the download instead of silently omitting files.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds platform backup APIs, streamed ZIP archive creation, CLI backup listing and download commands, comprehensive tests, and web integration through ChangesBackup management
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant BackupDownloadCommand
participant BackupsManager
participant downloadBackupArchive
participant ZipWriter
BackupDownloadCommand->>BackupsManager: select backup and resolve file metadata
BackupDownloadCommand->>downloadBackupArchive: start archive download
downloadBackupArchive->>BackupsManager: fetch entity and storage resources
downloadBackupArchive->>ZipWriter: write ordered decompressed entries
ZipWriter-->>BackupDownloadCommand: return counts and zipBytes
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
|
View Vercel preview at instant-www-js-claudeprop-jsv.vercel.app. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
client/packages/cli/src/lib/backupDownload.ts (3)
140-158: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDestroy the response when the encoding is unsupported.
At Line 148 the function throws without consuming or destroying
res. The non-200 branches at Lines 222 and 245 callres.resume()for the same reason. The outercatchdoes callabortController.abort(), which tears the request down, so this is not a durable leak. Addingres.destroy()before the throw keeps the handling consistent and independent of the caller.♻️ Suggested change
} else if (encoding) { + res.destroy(); throw new Error(`Unsupported content encoding: ${encoding}`); }🤖 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 `@client/packages/cli/src/lib/backupDownload.ts` around lines 140 - 158, Update toEntryStream so it destroys the IncomingMessage response via res.destroy() before throwing for an unsupported content encoding, preserving the existing error message and handling consistency with the non-200 response branches.
300-310: 🩺 Stability & Availability | 🔵 TrivialConsider adding bounded retries for body fetches.
A backup with many storage blobs makes a long sequence of independent HTTP GETs. One transient failure discards the whole archive, including gigabytes already written. Each entry fetch is an idempotent GET against a presigned URL, so a bounded retry with backoff is safe. This is operational advice, not a defect in this PR.
🤖 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 `@client/packages/cli/src/lib/backupDownload.ts` around lines 300 - 310, In the backup download flow around the entry loop, add bounded retries with backoff for each entry’s idempotent body fetch before passing its input to zipWriter.add. Preserve the existing archive-writing behavior, but retry transient fetch failures a limited number of times and propagate the final error when retries are exhausted.
43-52: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a request timeout and reject redirects explicitly.
fetchStreamhas two gaps for long, unattended downloads:
- No timeout. If the connection stalls after headers, or before them, the download hangs until the user sends SIGINT. A multi-GB storage phase makes this likely enough to handle.
- No redirect handling.
node:http/node:httpsgetdoes not follow redirects, so a301/307from the object store surfaces asHTTP 301in the caller's error message. That message is confusing, and the callers at Lines 221 and 244 already discard the body.Add a socket/headers timeout, and keep the non-200 rejection but make the redirect case explicit.
♻️ Suggested change
+const REQUEST_TIMEOUT_MS = 60_000; + function fetchStream( url: string, signal: AbortSignal, ): Promise<IncomingMessage> { return new Promise((resolve, reject) => { const get = url.startsWith('https:') ? httpsGet : httpGet; const req = get(url, { signal }, resolve); + req.setTimeout(REQUEST_TIMEOUT_MS, () => { + req.destroy(new Error(`Timed out after ${REQUEST_TIMEOUT_MS}ms: ${url}`)); + }); req.on('error', reject); }); }🤖 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 `@client/packages/cli/src/lib/backupDownload.ts` around lines 43 - 52, Update fetchStream to apply a request timeout covering connection and header stalls, and ensure the timeout rejects the promise and cleans up the request. Preserve non-200 rejection, but detect 3xx responses and reject them with an explicit redirect-related error while consuming or destroying the response body.client/packages/platform/__tests__/src/backups.test.ts (1)
102-139: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider asserting the request URL and the non-200 error path.
The
listtest only checks row coercion. The#getJsonhelper also builds the path/dash/apps/<appId>/backupsand throwsapiErrorfor non-200 responses. Add one assertion on the stubbedfetchcall URL and one test for a 4xx response. This locks the URL contract and the error 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 `@client/packages/platform/__tests__/src/backups.test.ts` around lines 102 - 139, The list tests currently cover only response coercion; extend the `list` suite to assert the stubbed `fetch` call uses `/dash/apps/<appId>/backups`, and add a test with a non-200 response that verifies `list` propagates the `apiError` from `#getJson`. Preserve the existing successful coercion assertions.client/packages/cli/__tests__/backupDownload.test.ts (2)
15-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFail with a clear message when
zstdCompressSyncis absent.
zlib.zstdCompressSyncexists only on Node 22.15+ / 23.8+. On an older runtime the cast at Line 18 producesTypeError: zlib.zstdCompressSync is not a functionduring module evaluation, which hides the real cause. The pipeline itself gives a clear message for the same condition atbackupDownload.tsLine 84. Mirror that here, or skip the suite.♻️ Suggested change
const zstd = (s: string): Buffer => (zlib as any).zstdCompressSync(Buffer.from(s)); + +if (typeof (zlib as any).zstdCompressSync !== 'function') { + throw new Error('These tests require Node 22.15 or newer (for zstd).'); +}🤖 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 `@client/packages/cli/__tests__/backupDownload.test.ts` around lines 15 - 32, Update the test setup around the zstd helper to detect when zlib.zstdCompressSync is unavailable and fail with the same clear message used by the backupDownload pipeline, before invoking it during module evaluation. Alternatively, skip the suite when the runtime lacks zstdCompressSync; preserve the existing compressed test bodies on supported Node versions.
163-191: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the abort error, not any error.
rejects.toThrow()passes for every failure, including a failure unrelated to cancellation. The test then no longer proves the abort path. Assert the error name so a regression in abort propagation is visible.💚 Suggested change
- await expect( - downloadBackupToFile({ - manager: slowManager, - backup, - outPath, - signal: controller.signal, - onProgress: () => {}, - }), - ).rejects.toThrow(); + await expect( + downloadBackupToFile({ + manager: slowManager, + backup, + outPath, + signal: controller.signal, + onProgress: () => {}, + }), + ).rejects.toMatchObject({ name: 'AbortError' });🤖 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 `@client/packages/cli/__tests__/backupDownload.test.ts` around lines 163 - 191, Update the abort test around downloadBackupToFile to assert that the rejected error has the expected abort-related name, rather than using the broad rejects.toThrow() matcher. Keep the existing partial-file cleanup assertions unchanged so the test specifically validates abort propagation and cleanup.
🤖 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 `@client/packages/platform/src/backups.ts`:
- Around line 230-240: Update the NDJSON loop producing storage-file entries to
recognize the terminal sentinel only when line.done is exactly true. For every
non-sentinel line, validate that locationId and url are non-empty strings and
throw on invalid records instead of silently continuing; preserve yielding valid
records and add tests covering malformed sentinel and file-record inputs.
---
Nitpick comments:
In `@client/packages/cli/__tests__/backupDownload.test.ts`:
- Around line 15-32: Update the test setup around the zstd helper to detect when
zlib.zstdCompressSync is unavailable and fail with the same clear message used
by the backupDownload pipeline, before invoking it during module evaluation.
Alternatively, skip the suite when the runtime lacks zstdCompressSync; preserve
the existing compressed test bodies on supported Node versions.
- Around line 163-191: Update the abort test around downloadBackupToFile to
assert that the rejected error has the expected abort-related name, rather than
using the broad rejects.toThrow() matcher. Keep the existing partial-file
cleanup assertions unchanged so the test specifically validates abort
propagation and cleanup.
In `@client/packages/cli/src/lib/backupDownload.ts`:
- Around line 140-158: Update toEntryStream so it destroys the IncomingMessage
response via res.destroy() before throwing for an unsupported content encoding,
preserving the existing error message and handling consistency with the non-200
response branches.
- Around line 300-310: In the backup download flow around the entry loop, add
bounded retries with backoff for each entry’s idempotent body fetch before
passing its input to zipWriter.add. Preserve the existing archive-writing
behavior, but retry transient fetch failures a limited number of times and
propagate the final error when retries are exhausted.
- Around line 43-52: Update fetchStream to apply a request timeout covering
connection and header stalls, and ensure the timeout rejects the promise and
cleans up the request. Preserve non-200 rejection, but detect 3xx responses and
reject them with an explicit redirect-related error while consuming or
destroying the response body.
In `@client/packages/platform/__tests__/src/backups.test.ts`:
- Around line 102-139: The list tests currently cover only response coercion;
extend the `list` suite to assert the stubbed `fetch` call uses
`/dash/apps/<appId>/backups`, and add a test with a non-200 response that
verifies `list` propagates the `apiError` from `#getJson`. Preserve the existing
successful coercion assertions.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 463b344c-8677-4709-bbca-b6faec62c169
⛔ Files ignored due to path filters (1)
client/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
client/packages/cli/__tests__/backupDownload.test.tsclient/packages/cli/__tests__/backups.test.tsclient/packages/cli/package.jsonclient/packages/cli/src/commands/backup/download.tsclient/packages/cli/src/commands/backup/list.tsclient/packages/cli/src/index.tsclient/packages/cli/src/lib/backupDownload.tsclient/packages/cli/src/lib/backups.tsclient/packages/cli/src/lib/platformApi.tsclient/packages/cli/src/lib/webhooks.tsclient/packages/platform/__tests__/src/backups.test.tsclient/packages/platform/src/api.tsclient/packages/platform/src/backups.tsclient/packages/platform/src/index.tsclient/www/components/dash/BackupDownloadDialog.tsx
Swaps the dialog's hand-rolled fetch helpers and NDJSON parsing for the shared BackupsManager, so the dashboard and CLI consume the same protocol layer. Adds optional abort signals to the manager's JSON methods so the dialog keeps aborting in-flight API calls on cancel (and the CLI now passes its signal too).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@client/www/components/dash/BackupDownloadDialog.tsx`:
- Around line 256-259: Update the discovery completion logic around
streamStorageFiles so filesTotal is set to 0 only when storage discovery
completes without storageError. Preserve the unknown total on failed listings,
including 404 responses, while still marking storageDone true.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: df0482bb-66b2-439b-bfa6-ba9751bd3bf1
📒 Files selected for processing (3)
client/packages/cli/src/lib/backupDownload.tsclient/packages/platform/src/backups.tsclient/www/components/dash/BackupDownloadDialog.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- client/packages/cli/src/lib/backupDownload.ts
- client/packages/platform/src/backups.ts
The storage-files stream now requires an exact done sentinel and throws on records missing locationId/url instead of silently skipping them (a skipped record would mean a silently incomplete archive). A failed listing keeps the storage total unknown instead of reporting an empty-but-complete phase, and the CLI destroys the response before throwing on an unsupported content encoding.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@client/packages/cli/src/lib/backupDownload.ts`:
- Around line 190-199: Update the catch handling around
manager.streamStorageFiles so an AbortError is preserved when
opts.signal.aborted, allowing caller cancellation to propagate and preventing
completion of an incomplete storage archive. Continue ignoring AbortError only
when it was initiated by ZIP pipeline failure cleanup; retain existing
storageError handling for all other errors.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5a96a250-3955-404d-b170-cbb133b4543e
📒 Files selected for processing (4)
client/packages/cli/src/lib/backupDownload.tsclient/packages/platform/__tests__/src/backups.test.tsclient/packages/platform/src/backups.tsclient/www/components/dash/BackupDownloadDialog.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- client/packages/platform/tests/src/backups.test.ts
- client/www/components/dash/BackupDownloadDialog.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
client/packages/cli/src/lib/backupDownload.ts (1)
51-71: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueNormalize the
content-encodingvalue before matching.Line 60 compares the raw header value. A server that answers
ZSTD,zstd(trailing space), or a comma list falls into the unsupported branch at line 66 and fails the whole download. Lowercase and trim the value before the comparison.♻️ Proposed change
- const encoding = res.headers['content-encoding']; + const encoding = res.headers['content-encoding']?.trim().toLowerCase();🤖 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 `@client/packages/cli/src/lib/backupDownload.ts` around lines 51 - 71, Update fetchBody to normalize the content-encoding header before matching: trim whitespace, lowercase the value, and handle comma-separated values so equivalent zstd or gzip encodings select the existing decompression paths instead of the unsupported branch.client/packages/platform/src/backups.ts (1)
307-311: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSpread
optsbeforemanager.
{ manager: this, ...opts }lets amanagerproperty onoptsreplacethis.DownloadBackupArchiveOptsdoes not declaremanager, so TypeScript blocks this only for object literals. A caller that passes a widened object silently redirects the pipeline. Reorder the spread to make the manager authoritative.♻️ Proposed change
- return downloadBackupArchive({ manager: this, ...opts }); + return downloadBackupArchive({ ...opts, manager: this });🤖 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 `@client/packages/platform/src/backups.ts` around lines 307 - 311, Update downloadArchive so the opts spread occurs before manager, ensuring the method’s this manager always overrides any manager property supplied through widened options.
🤖 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 `@client/packages/platform/src/backupDownload.ts`:
- Around line 205-218: Update the catch handling around the discovery task in
streamStorageFiles so AbortError is ignored only when it was caused by the
pipeline’s internal abort, while an AbortError from opts.signal is assigned to
storageError. Preserve the existing handling for non-abort errors and ensure
caller cancellation makes the generator reject instead of reporting storageDone
successfully.
- Around line 262-276: Update the download error message in the file-processing
flow around currentFile to use the same path-or-locationId fallback as the
assignment above, so a null file.path never appears in the reported storage-file
name.
---
Nitpick comments:
In `@client/packages/cli/src/lib/backupDownload.ts`:
- Around line 51-71: Update fetchBody to normalize the content-encoding header
before matching: trim whitespace, lowercase the value, and handle
comma-separated values so equivalent zstd or gzip encodings select the existing
decompression paths instead of the unsupported branch.
In `@client/packages/platform/src/backups.ts`:
- Around line 307-311: Update downloadArchive so the opts spread occurs before
manager, ensuring the method’s this manager always overrides any manager
property supplied through widened options.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a352a3f5-d027-412a-953d-10a6b2ea92c9
📒 Files selected for processing (6)
client/packages/cli/src/commands/backup/download.tsclient/packages/cli/src/lib/backupDownload.tsclient/packages/platform/src/backupDownload.tsclient/packages/platform/src/backups.tsclient/packages/platform/src/index.tsclient/www/components/dash/BackupDownloadDialog.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- client/packages/cli/src/commands/backup/download.ts
# Conflicts: # client/www/components/dash/BackupDownloadDialog.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@client/packages/platform/src/backups.ts`:
- Around line 97-102: Update the byte-formatting function containing the
unit-selection loop so rounding is performed before finalizing the unit; when
the rounded value reaches 1000 and another unit exists, promote it to the next
unit and recompute the display precision. Add boundary tests for transitions at
KB/MB, MB/GB, and GB/TB, including values such as 999_500.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0802166f-f153-4301-a9fc-2c84ae575690
📒 Files selected for processing (6)
client/packages/cli/src/commands/backup/download.tsclient/packages/cli/src/commands/backup/list.tsclient/packages/platform/src/backupDownload.tsclient/packages/platform/src/backups.tsclient/packages/platform/src/index.tsclient/www/components/dash/BackupDownloadDialog.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- client/packages/platform/src/index.ts
- client/packages/platform/src/backupDownload.ts
- client/www/components/dash/BackupDownloadDialog.tsx
A caller abort that landed while no fetch was in flight surfaced only in the storage discovery stream, which swallows AbortError as expected teardown; the pipeline could then close and rename a complete-looking archive that silently omits undiscovered storage files. Check the signal explicitly in the drain loop and before closing the writer. Progress counters now increment after the writer consumes an entry, not when its fetch starts, so "N of M" reflects fully-written files. Also: name pathless storage files by locationId in errors, reject locationIds that could escape the files/ archive prefix, and fix formatFileSize rounding across a unit boundary (999,500 read as 1000 KB).
Reject a backup id combined with --latest and an output path that is a directory before downloading; randomize the .partial suffix and open it exclusively so concurrent or stale runs can't collide; fsync before the final rename; strip control characters from user-controlled descriptions printed to the terminal; and name the auth source in backups errors so a stale INSTANT_APP_ADMIN_TOKEN shadowing a fresh login is diagnosable.
Adds
instant-cli backup listandinstant-cli backup download, built on a newBackupsManagerin@instantdb/platformthat the dashboard's download dialog now consumes too.list:
download:
Platform (
@instantdb/platform)api.backups(appId)returns aBackupsManagerwithlist,listFiles,getFileUrl,streamStorageFiles(an async generator over the NDJSON storage-files stream that honors the{done: true}sentinel and throws on truncation), anddownloadArchive.downloadArchiveis the whole download pipeline, shared by the dashboard and the CLI: canonical entry order, eager draining of the storage-files discovery so the NDJSON connection isn't held open while multi-GB blobs download, progress reporting, backpressure into the caller's sink, and teardown.fetchBody(fetch and decompress a presigned URL),sink(where the bytes go), andcreateWriter(a smallBackupArchiveWriterinterface that zip.js'sZipWritersatisfies structurally, so the platform package doesn't depend on a zip implementation).config.jsonfirst, then everyentities/*.jsonlshard, thenfiles/<locationId>, with all entity files before any storage file so restore can process the archive in one streaming pass. Since the pipeline lives behinddownloadArchive, that order is enforced in one place instead of once per consumer.estimateZipSize(the min/max zip size estimate both UIs show),formatFileSize(decimal/SI byte formatting that matches how Finder reports file sizes, so both surfaces print identical numbers), andtoAppBackup(parses a server backup row).BackupDownloadDialogswaps its hand-rolled pipeline fordownloadArchive(a large net deletion); the browser-specific pieces (save picker, service worker, browser fetch, React progress UI) stay in www.Dashboard fix
{done: true}sentinel (it sends the sentinel even when there's no$filesshard), so a 404 can only mean the backup record is missing or expired (e.g. it expired mid-download). The download now fails instead of silently omitting every storage file; this behavior lives in the sharedBackupsManager.Testing
uncompressed_size, and a cleanunzip -t.@dwwoelfel @nezaj