Skip to content

Backups through the CLI - #2838

Merged
stopachka merged 19 commits into
mainfrom
claudeprop
Aug 5, 2026
Merged

Backups through the CLI#2838
stopachka merged 19 commits into
mainfrom
claudeprop

Conversation

@stopachka

@stopachka stopachka commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Adds instant-cli backup list and instant-cli backup download, built on a new BackupsManager in @instantdb/platform that the dashboard's download dialog now consumes too.

list:

CleanShot 2026-08-05 at 14 54 12@2x

download:

CleanShot 2026-08-05 at 14 54 41@2x

Platform (@instantdb/platform)

  • api.backups(appId) returns a BackupsManager with list, listFiles, getFileUrl, streamStorageFiles (an async generator over the NDJSON storage-files stream that honors the {done: true} sentinel and throws on truncation), and downloadArchive.
  • downloadArchive is 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.
    • The runtime-specific pieces are injected: fetchBody (fetch and decompress a presigned URL), sink (where the bytes go), and createWriter (a small BackupArchiveWriter interface that zip.js's ZipWriter satisfies structurally, so the platform package doesn't depend on a zip implementation).
  • The manager's docs are the canonical statement of the archive ordering contract: config.json first, then every entities/*.jsonl shard, then files/<locationId>, with all entity files before any storage file so restore can process the archive in one streaming pass. Since the pipeline lives behind downloadArchive, that order is enforced in one place instead of once per consumer.
  • Also shared: 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), and toAppBackup (parses a server backup row).
  • The dashboard's BackupDownloadDialog swaps its hand-rolled pipeline for downloadArchive (a large net deletion); the browser-specific pieces (save picker, service worker, browser fetch, React progress UI) stay in www.

Dashboard fix

  • A 404 from the storage-files endpoint was treated as "no storage files" and produced a complete-looking zip. The server signals the empty case with a 200 + {done: true} sentinel (it sends the sentinel even when there's no $files shard), 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 shared BackupsManager.

Testing

  • 28 new tests: NDJSON sentinel/truncation semantics and pipeline-level tests (cancellation race, counter timing, entry order, locationId validation) in the platform package, CLI command tests mocked at the SDK boundary, and an integration test that runs the real shared pipeline against a local HTTP server serving zstd-encoded bodies, then reads the zip back and asserts entry order and contents.
  • Real-world run against production with a ~4.9 GB backup: 65 namespaces -> 1.54 GB zip in ~3 minutes, memory flat at ~133 MB the whole run, the sum of uncompressed entry sizes exactly equal to the recorded uncompressed_size, and a clean unzip -t.

@dwwoelfel @nezaj

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.
@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 2d71c970-04a0-4700-8620-0ec54a55548a

📥 Commits

Reviewing files that changed from the base of the PR and between 0a93755 and fe6fd8f.

📒 Files selected for processing (1)
  • client/packages/version/src/version.ts

📝 Walkthrough

Walkthrough

The PR adds platform backup APIs, streamed ZIP archive creation, CLI backup listing and download commands, comprehensive tests, and web integration through BackupsManager.

Changes

Backup management

Layer / File(s) Summary
Platform backup API
client/packages/platform/src/backups.ts, client/packages/platform/src/api.ts, client/packages/platform/src/index.ts, client/packages/platform/__tests__/src/backups.test.ts
Adds typed backup models, authenticated listing, entity-file access, storage-file streaming, archive helpers, and public exports.
Shared backup archive pipeline
client/packages/platform/src/backupDownload.ts, client/packages/platform/__tests__/src/backupDownload.test.ts
Discovers files, writes ordered ZIP entries with backpressure, reports progress, supports cancellation, and aborts incomplete work.
CLI backup download flow
client/packages/cli/src/lib/backupDownload.ts, client/packages/cli/package.json, client/packages/cli/__tests__/backupDownload.test.ts
Fetches and decompresses responses, writes ZIP64 output through a partial file, renames successful archives, and cleans up failures.
CLI commands and web integration
client/packages/cli/src/commands/backup/*, client/packages/cli/src/index.ts, client/packages/cli/src/lib/*, client/packages/cli/__tests__/backups.test.ts, client/www/components/dash/BackupDownloadDialog.tsx
Adds authenticated list and download commands. The web dialog uses shared backup models, archive helpers, progress, and download handling.

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
Loading

Suggested reviewers: nezaj

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 30.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the primary change: adding backup functionality through the CLI.
Description check ✅ Passed The description directly explains the new CLI commands, shared platform manager, dashboard changes, and tests.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claudeprop

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

View Vercel preview at instant-www-js-claudeprop-jsv.vercel.app.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (6)
client/packages/cli/src/lib/backupDownload.ts (3)

140-158: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Destroy 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 call res.resume() for the same reason. The outer catch does call abortController.abort(), which tears the request down, so this is not a durable leak. Adding res.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 | 🔵 Trivial

Consider 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 win

Add a request timeout and reject redirects explicitly.

fetchStream has two gaps for long, unattended downloads:

  1. 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.
  2. No redirect handling. node:http/node:https get does not follow redirects, so a 301/307 from the object store surfaces as HTTP 301 in 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 value

Consider asserting the request URL and the non-200 error path.

The list test only checks row coercion. The #getJson helper also builds the path /dash/apps/<appId>/backups and throws apiError for non-200 responses. Add one assertion on the stubbed fetch call 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 value

Fail with a clear message when zstdCompressSync is absent.

zlib.zstdCompressSync exists only on Node 22.15+ / 23.8+. On an older runtime the cast at Line 18 produces TypeError: zlib.zstdCompressSync is not a function during module evaluation, which hides the real cause. The pipeline itself gives a clear message for the same condition at backupDownload.ts Line 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 win

Assert 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

📥 Commits

Reviewing files that changed from the base of the PR and between b33aadc and 8fed07e.

⛔ Files ignored due to path filters (1)
  • client/pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (15)
  • client/packages/cli/__tests__/backupDownload.test.ts
  • client/packages/cli/__tests__/backups.test.ts
  • client/packages/cli/package.json
  • client/packages/cli/src/commands/backup/download.ts
  • client/packages/cli/src/commands/backup/list.ts
  • client/packages/cli/src/index.ts
  • client/packages/cli/src/lib/backupDownload.ts
  • client/packages/cli/src/lib/backups.ts
  • client/packages/cli/src/lib/platformApi.ts
  • client/packages/cli/src/lib/webhooks.ts
  • client/packages/platform/__tests__/src/backups.test.ts
  • client/packages/platform/src/api.ts
  • client/packages/platform/src/backups.ts
  • client/packages/platform/src/index.ts
  • client/www/components/dash/BackupDownloadDialog.tsx

Comment thread client/packages/platform/src/backups.ts
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).

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 8fed07e and 1f72cc0.

📒 Files selected for processing (3)
  • client/packages/cli/src/lib/backupDownload.ts
  • client/packages/platform/src/backups.ts
  • client/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

Comment thread client/www/components/dash/BackupDownloadDialog.tsx Outdated
@stopachka stopachka changed the title Claude proposal Backups through the CLI Aug 5, 2026
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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 1f72cc0 and 5a51250.

📒 Files selected for processing (4)
  • client/packages/cli/src/lib/backupDownload.ts
  • client/packages/platform/__tests__/src/backups.test.ts
  • client/packages/platform/src/backups.ts
  • client/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

Comment thread client/packages/cli/src/lib/backupDownload.ts Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (2)
client/packages/cli/src/lib/backupDownload.ts (1)

51-71: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

Normalize the content-encoding value 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 value

Spread opts before manager.

{ manager: this, ...opts } lets a manager property on opts replace this. DownloadBackupArchiveOpts does not declare manager, 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

📥 Commits

Reviewing files that changed from the base of the PR and between 5a51250 and fa9cf3f.

📒 Files selected for processing (6)
  • client/packages/cli/src/commands/backup/download.ts
  • client/packages/cli/src/lib/backupDownload.ts
  • client/packages/platform/src/backupDownload.ts
  • client/packages/platform/src/backups.ts
  • client/packages/platform/src/index.ts
  • client/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

Comment thread client/packages/platform/src/backupDownload.ts
Comment thread client/packages/platform/src/backupDownload.ts Outdated
# Conflicts:
#	client/www/components/dash/BackupDownloadDialog.tsx

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between fa9cf3f and ab4afed.

📒 Files selected for processing (6)
  • client/packages/cli/src/commands/backup/download.ts
  • client/packages/cli/src/commands/backup/list.ts
  • client/packages/platform/src/backupDownload.ts
  • client/packages/platform/src/backups.ts
  • client/packages/platform/src/index.ts
  • client/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

Comment thread client/packages/platform/src/backups.ts
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.

@dwwoelfel dwwoelfel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@stopachka
stopachka merged commit cbb3c41 into main Aug 5, 2026
30 checks passed
@stopachka
stopachka deleted the claudeprop branch August 5, 2026 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants