Restore database backups safely and quickly from the console - #1886
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe pull request moves database restoration from the admin web interface to a Deno CLI task. It adds backup inspection, confirmation, progress reporting, and bounded restore execution while retaining web backup listing, creation, and download features. ChangesDatabase restore workflow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant RestoreScript
participant runRestoreTask
participant runRestoreCli
participant restoreFromZip
participant Database
RestoreScript->>runRestoreTask: Load dotenv values
runRestoreTask->>runRestoreCli: Provide CLI I/O and dependencies
runRestoreCli->>restoreFromZip: Restore after typed confirmation
restoreFromZip->>Database: Inspect, reset, and import backup
Database-->>runRestoreCli: Report progress and result
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f64315df4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@README.md`:
- Line 331: Update the README restore task description to clarify that it
restores the database identified by DB_URL, using DB_TOKEN from .env, rather
than restoring DB_URL itself. Keep the command unchanged and align the wording
with the detailed restore instructions below.
In `@scripts/restore-lib.ts`:
- Around line 95-98: Update the DB_TOKEN validation condition in the restore
flow to recognize both libsql:// and https:// database URLs before checking the
token. Preserve the existing error message and return behavior for either remote
URL scheme, while leaving other DB_URL values unchanged.
In `@src/shared/db/backup.ts`:
- Around line 7-10: Update the backup documentation bullet near splitStatements
to describe its current tokenizer behavior: it recognizes quoted strings and
preserves semicolons, including those in multiline values, within the same
statement. Remove the outdated claim that statements are delimited by ";\n";
leave the other bullets unchanged.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0c0271a0-0a65-4602-995a-7c3cddde7388
⛔ Files ignored due to path filters (1)
deno.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
AGENTS.mdREADME.mddeno.jsonscripts/deploy-edge-lib.tsscripts/deploy-edge.tsscripts/restore-lib.tsscripts/restore.tsscripts/script-runner.tssrc/features/admin/backup.tssrc/locales/en/backup.jsonsrc/locales/en/guide-domains.jsonsrc/locales/en/guide-operations.jsonsrc/restore.tssrc/shared/db/backup.tssrc/ui/templates/admin/backup.tsxtest/e2e/booking.test.tstest/features/admin/backup/routes.test.tstest/features/admin/backup/server.test.tstest/integration/admin-action-cookie.test.tstest/integration/code-quality.test.tstest/integration/confirm-page-options.test.tstest/integration/restore-task.test.tstest/lib/code-quality/detectors.tstest/lib/server-backup.test.tstest/restore.test.tstest/routes/backup.test.tstest/shared/db/backup.test.tstest/shared/db/backup/restore.test.tstest/ui/templates/admin/backup.test.ts
💤 Files with no reviewable changes (3)
- test/lib/server-backup.test.ts
- test/routes/backup.test.ts
- test/shared/db/backup/restore.test.ts
stefan-burke
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6763a76ca7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/shared/db/backup.ts (1)
234-243: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRename
readManifesttoreadManifestOrNull.This function legitimately returns
nullfor an expected-absence case (older backups withoutmanifest.json, as covered by the "Backup manifest: not available" test), but doesn't follow the codebase's*OrNullnaming convention for that pattern.readRestoreDbUrlOrNullinscripts/restore-lib.tsalready follows this convention for the same kind of nullable-on-expected-absence semantics.As per coding guidelines: "Use nullable results, defaults, optional chaining, and catches only when absence or recovery is genuinely expected and documented; use
*OrNullnaming for expected absence."♻️ Proposed rename
-const readManifest = ( +const readManifestOrNull = ( files: Record<string, Uint8Array>, ): BackupManifest | null => { const manifestBytes = files["manifest.json"]; if (!manifestBytes) return null; const parsed: unknown = JSON.parse(new TextDecoder().decode(manifestBytes)); const result = v.safeParse(BackupManifestSchema, parsed); if (!result.success) throw new Error("Backup manifest is invalid"); return result.output; };- const manifest = readManifest(backup.files); + const manifest = readManifestOrNull(backup.files);Also applies to: 272-272
🤖 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 `@src/shared/db/backup.ts` around lines 234 - 243, Rename the nullable helper readManifest to readManifestOrNull and update every reference to it, including the usage around the indicated later location; preserve its existing null behavior when manifest.json is absent.Source: Coding guidelines
🤖 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 `@README.md`:
- Around line 367-369: Update the README paragraph describing restore behavior
to qualify that table, row, statement, and schema details are shown when
available, particularly for manifest-less archives. Keep the existing claims
about typed deletion confirmation, restore-step reporting, and recorded commit
display unchanged.
---
Outside diff comments:
In `@src/shared/db/backup.ts`:
- Around line 234-243: Rename the nullable helper readManifest to
readManifestOrNull and update every reference to it, including the usage around
the indicated later location; preserve its existing null behavior when
manifest.json is absent.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b925f4ed-b73d-47a6-ad0e-b89b4cabbe37
📒 Files selected for processing (9)
AGENTS.mdREADME.mdscripts/restore-lib.tsscripts/restore.tssrc/restore.tssrc/shared/db/backup.tstest/integration/restore-task.test.tstest/restore.test.tstest/shared/db/backup.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
README.md (1)
365-371: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winQualify manifest-dependent backup details.
Manifest-less archives cannot provide table, row, or schema details; only the SQL statement count is guaranteed. Change this to say those details are shown “when available,” matching the restore implementation. This repeats the unresolved prior review finding.
🤖 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 `@README.md` around lines 365 - 371, Update the restore documentation to state that table, row, and schema details are shown only when available, while keeping the SQL statement count as the guaranteed detail. Preserve the existing workflow and commit-reporting description.
🤖 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.
Duplicate comments:
In `@README.md`:
- Around line 365-371: Update the restore documentation to state that table,
row, and schema details are shown only when available, while keeping the SQL
statement count as the guaranteed detail. Preserve the existing workflow and
commit-reporting description.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cfa33fbb-555b-466c-af9d-919993314a85
📒 Files selected for processing (3)
AGENTS.mdREADME.mddeno.json
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc2a0684f3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7288dcaff2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Addressed the remaining CodeRabbit outside-diff naming note in 9e5565b: the nullable manifest helper is now |
|
Addressed the remaining outside-diff CodeRabbit finding in 9e5565b. The expected-absence helper is now named |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5f9fa60ce
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| report("resetting"); | ||
| await resetDatabase(); |
There was a problem hiding this comment.
The final progress report was moved out of the cleanup path, but this earlier report("resetting") still runs before resetDatabase() inside the try. If the progress handler fails here (for example stdout gets a broken pipe), the catch treats it as a post-reset failure and then runs the cleanup reset/rebuild, deleting the target even though no restore step had run. Gate cleanup on a flag set after resetDatabase() starts or succeeds, or keep this report outside the destructive catch.
AGENTS.md reference: AGENTS.md:L98-L100
Useful? React with 👍 / 👎.
| manifest === null | ||
| ? legacyRequiredTables | ||
| : Object.entries(manifest.tables) | ||
| .filter(([, rowCount]) => rowCount > 0) | ||
| .map(([table]) => table); |
There was a problem hiding this comment.
With a manifest present, requiredTables is derived only from whatever row counts the manifest says are positive. A partial or corrupt ZIP can include a well-formed manifest such as tables: { settings: 1 } plus only settings.sql, pass inspection, and then restoreFromSql deletes the rebuilt schema_migrations and attendee_statuses rows without importing replacements, leaving the site without migration rows or default statuses after a reported-success restore. Keep the core-table requirement for manifest archives too, or validate the manifest against the required schema before reset.
AGENTS.md reference: AGENTS.md:L101-L105
Useful? React with 👍 / 👎.
| srcTsxContents, | ||
| testContents, | ||
| ALLOWED_TEST_HOOKS, | ||
| [scriptsContents, cliContents, e2eContents], |
There was a problem hiding this comment.
Passing e2eContents (the e2e-payments harness) as a production importer weakens this guard: a src/ export used only from e2e payment tests is now treated as used in production, so the rule no longer catches dead/test-only exports for that case. Keep production entry maps to real runtime/operator entry points, and leave e2e harness imports in the test side of the check.
AGENTS.md reference: AGENTS.md:L64-L64
Useful? React with 👍 / 👎.
| // Every generated table file ends its statements with semicolons, so no | ||
| // extra separator is needed between files. | ||
| await restoreFromSql(allSql.join(""), onProgress); |
There was a problem hiding this comment.
Inspection accepts a final SQL statement without a trailing semicolon because splitStatements adds one, but the restore path concatenates raw table files with no separator. If a ZIP has two known table files and the first ends in INSERT ... without ;, preflight passes, then the import sees that statement glued to the next file's first INSERT and fails only after the target was reset; join files with a newline or restore the inspected statements instead.
Useful? React with 👍 / 👎.
| report("resetting"); | ||
| await resetDatabase(); |
There was a problem hiding this comment.
The final progress report was moved out of the cleanup path, but this earlier report("resetting") still runs before resetDatabase() inside the try. If the progress handler fails here (for example stdout gets a broken pipe), the catch treats it as a post-reset failure and then runs the cleanup reset/rebuild, deleting the target even though no restore step had run. Gate cleanup on a flag set after resetDatabase() starts or succeeds, or keep this report outside the destructive catch.
Useful? React with 👍 / 👎.
| manifest === null | ||
| ? legacyRequiredTables | ||
| : Object.entries(manifest.tables) | ||
| .filter(([, rowCount]) => rowCount > 0) | ||
| .map(([table]) => table); |
There was a problem hiding this comment.
With a manifest present, requiredTables is derived only from whatever row counts the manifest says are positive. A partial or corrupt ZIP can include a well-formed manifest such as tables: { settings: 1 } plus only settings.sql, pass inspection, and then restoreFromSql deletes the rebuilt schema_migrations and attendee_statuses rows without importing replacements, leaving the site without migration rows or default statuses after a reported-success restore. Keep the core-table requirement for manifest archives too, or validate the manifest against the required schema before reset.
Useful? React with 👍 / 👎.
| srcTsxContents, | ||
| testContents, | ||
| ALLOWED_TEST_HOOKS, | ||
| [scriptsContents, cliContents, e2eContents], |
There was a problem hiding this comment.
Passing e2eContents (the e2e-payments harness) as a production importer weakens this guard: a src/ export used only from e2e payment tests is now treated as used in production, so the rule no longer catches dead/test-only exports for that case. Keep production entry maps to real runtime/operator entry points, and leave e2e harness imports in the test side of the check.
Useful? React with 👍 / 👎.
1 participant
What changed
deno task restore <backup.zip>to restore the database named byDB_URLin.env..envdatabase settings override inherited shell values.Safety
Checks
deno task precommitdeno task mutation --source src/shared/db/backup.ts --test "test/shared/db/backup{.test.ts,/*.test.ts}" --harness(141/141 mutants killed)deno task mutation --source src/shared/update.ts --test test/shared/update.test.ts --harness(68/68 mutants killed; 2 proven equivalents suppressed)