feat: naming backup schedules #103
Conversation
WalkthroughThis pull request adds a new "name" field to backup schedules across the entire stack, from database schema through API types to UI components and forms. Updates include database migration, schema definitions, generated API types, React components for creating/displaying schedules, form validation, and backend service logic with duplicate-name checks. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/server/modules/backups/backups.service.ts (1)
45-59: Bug:getSchedulefilters onvolumesTable.idinstead of schedule id
getSchedulecurrently does:const schedule = await db.query.backupSchedulesTable.findFirst({ where: eq(volumesTable.id, scheduleId), ... });Filtering on
volumesTable.idhere is almost certainly wrong; this condition will never match (or will coincidentally match the wrong row) because the query is overbackupSchedulesTable. It should comparebackupSchedulesTable.idwithscheduleId.This likely breaks any caller that relies on
backupsService.getSchedule.- const schedule = await db.query.backupSchedulesTable.findFirst({ - where: eq(volumesTable.id, scheduleId), - with: { - volume: true, - repository: true, - }, - }); + const schedule = await db.query.backupSchedulesTable.findFirst({ + where: eq(backupSchedulesTable.id, scheduleId), + with: { + volume: true, + repository: true, + }, + });
🧹 Nitpick comments (3)
app/server/utils/restic.ts (1)
265-265: Consider applying os.tmpdir() consistently across the file.Using
os.tmpdir()improves cross-platform compatibility. However, temporary file creations at lines 110, 131, and 156 still use hardcoded/tmppaths. For consistency and portability, consider updating those locations as well.app/server/db/schema.ts (1)
68-96: Global uniqueness of schedule names – confirm this matches the product intent
name: text().notNull().unique()makes backup schedule names unique across the entire system, not just per volume or repository. If you expect users to reuse the same name for different volumes/repos, you may want a composite unique index on(volumeId, name)instead of a global unique onname.app/server/modules/backups/backups.service.ts (1)
61-73: Name uniqueness on create is good, but consider DB‑level race handlingThe create path now:
- Validates cron,
- Checks for an existing schedule with the same
name,- Throws
ConflictErroron duplicate,- Persists
namewhen inserting.This is correct logically and aligned with the unique index on
backup_schedules_table.name. However, two concurrent creates with the samenamecan still pass the pre‑check and race, with one failing on the DB unique constraint instead of returning a clean409.If you want fully deterministic behavior, consider also catching the DB unique‑constraint error (e.g., for
backup_schedules_table_name_unique) and mapping it to aConflictErroras a fallback.Also applies to: 90-105
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
.github/copilot-instructions.md(1 hunks)AGENTS.md(1 hunks)app/client/api-client/@tanstack/react-query.gen.ts(22 hunks)app/client/api-client/client.gen.ts(1 hunks)app/client/api-client/sdk.gen.ts(1 hunks)app/client/api-client/types.gen.ts(7 hunks)app/client/modules/backups/components/create-schedule-form.tsx(3 hunks)app/client/modules/backups/components/schedule-summary.tsx(3 hunks)app/client/modules/backups/routes/backup-details.tsx(2 hunks)app/client/modules/backups/routes/backups.tsx(1 hunks)app/client/modules/backups/routes/create-backup.tsx(1 hunks)app/drizzle/0019_secret_nomad.sql(1 hunks)app/drizzle/meta/0019_snapshot.json(1 hunks)app/drizzle/meta/_journal.json(1 hunks)app/server/db/schema.ts(1 hunks)app/server/modules/backups/backups.dto.ts(3 hunks)app/server/modules/backups/backups.service.ts(4 hunks)app/server/utils/restic.ts(2 hunks)package.json(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-03T19:48:55.097Z
Learnt from: nicotsx
Repo: nicotsx/zerobyte PR: 95
File: app/server/modules/backups/backups.service.ts:525-533
Timestamp: 2025-12-03T19:48:55.097Z
Learning: In app/server/modules/backups/backups.service.ts, the mirror copy operations use shared locks (acquireShared) for both source and mirror repositories because restic supports parallel write operations and concurrent backups with non-exclusive locks. Only maintenance operations like prune and rebuild-index require exclusive locks in restic.
Applied to files:
app/client/api-client/sdk.gen.ts
🧬 Code graph analysis (5)
app/client/api-client/@tanstack/react-query.gen.ts (2)
app/client/api-client/client/types.gen.ts (1)
Options(232-241)app/client/api-client/types.gen.ts (22)
GetMeData(79-84)GetStatusData(103-108)ListVolumesData(143-148)GetVolumeData(363-370)GetContainersUsingVolumeData(538-545)ListFilesData(638-650)BrowseFilesystemData(670-680)ListRepositoriesData(700-705)ListRcloneRemotesData(874-879)GetRepositoryData(913-920)ListSnapshotsData(1107-1116)GetSnapshotDetailsData(1155-1163)ListSnapshotFilesData(1181-1191)ListBackupSchedulesData(1279-1284)GetBackupScheduleData(1513-1520)GetBackupScheduleForVolumeData(1730-1737)GetScheduleNotificationsData(1955-1962)GetScheduleMirrorsData(2124-2131)GetMirrorCompatibilityData(2327-2334)ListNotificationDestinationsData(2349-2354)GetNotificationDestinationData(2568-2575)GetSystemInfoData(2814-2819)
app/server/modules/backups/backups.service.ts (2)
app/server/db/db.ts (1)
db(13-13)app/server/db/schema.ts (1)
backupSchedulesTable(68-96)
app/client/modules/backups/routes/backups.tsx (1)
app/client/modules/backups/components/backup-status-dot.tsx (1)
BackupStatusDot(3-30)
app/client/api-client/client.gen.ts (3)
app/client/api-client/client/index.ts (2)
createClient(12-12)createConfig(25-25)app/client/api-client/client/client.gen.ts (1)
createClient(27-301)app/client/api-client/client/utils.gen.ts (1)
createConfig(324-332)
app/client/api-client/sdk.gen.ts (3)
app/client/api-client/client/index.ts (1)
Options(18-18)app/client/api-client/client/types.gen.ts (1)
Options(232-241)app/client/api-client/client.gen.ts (1)
client(16-16)
🪛 LanguageTool
AGENTS.md
[style] ~253-~253: The double modal “Requires privileged” is nonstandard (only accepted in certain dialects). Consider “to be privileged”.
Context: ...ever expose it - Mounting: Requires privileged container or CAP_SYS_ADMIN for FUSE mou...
(NEEDS_FIXED)
.github/copilot-instructions.md
[style] ~1-~1: Consider using a more formal verb to strengthen your wording.
Context: ...This project uses the AGENTS.md file to give detailed information about the reposito...
(GIVE_INFORMATION)
🔇 Additional comments (19)
app/server/utils/restic.ts (1)
4-4: Change appears unrelated to PR objectives.While the import is technically correct, this change doesn't relate to the stated PR objective of "naming backup schedules." Please verify this change belongs in this PR.
.github/copilot-instructions.md (1)
1-1: LGTM!The instruction is clear and serves its purpose. The static analysis suggestion about replacing "give" with a more formal verb is overly pedantic for internal documentation.
app/client/api-client/client.gen.ts (1)
16-16: LGTM!The reformatting from multi-line to single-line is a cosmetic change with no semantic impact. The client initialization remains functionally identical.
AGENTS.md (1)
1-267: LGTM! Excellent documentation.This comprehensive documentation file provides valuable context about the project structure, technology stack, and development workflows. The content is well-organized and will be helpful for both AI assistants and human developers.
The static analysis hint about grammar on line 253 ("Requires privileged") is acceptable in technical documentation.
app/drizzle/0019_secret_nomad.sql (1)
1-24: Migration looks correct, with a minor note on collision risk.The migration properly:
- Uses the temp table pattern with foreign key management
- Adds the NOT NULL name column with a unique index
- Anonymizes existing schedule names using
lower(hex(randomblob(3)))The anonymization generates 6-character hex strings (3 bytes = 6 hex digits). While collision risk is low, with a very large number of existing schedules there's a small chance of duplicates causing the migration to fail on the unique constraint.
For context: 3 bytes = 16,777,216 possible values, so collision probability is negligible for typical use cases.
app/client/api-client/types.gen.ts (1)
1300-1300: LGTM! Consistent type additions.The
name: stringfield has been properly added across all backup schedule-related types:
- Required in list, create, and get responses
- Optional in update requests (line 1678)
- Required in update responses
This follows standard REST API patterns and is correctly reflected in the auto-generated types.
Also applies to: 1439-1439, 1474-1474, 1536-1536, 1678-1678, 1711-1711, 1753-1753
app/client/api-client/@tanstack/react-query.gen.ts (1)
90-90: LGTM! Formatting changes only.These are purely cosmetic changes in auto-generated code:
- String literals changed from double to single quotes
- Return array formatting adjusted
No functional impact on the query key factories or mutation options.
Also applies to: 93-93, 111-111, 146-146, 215-215, 250-250, 319-319, 337-337, 355-355, 390-390, 425-425, 460-460, 495-495, 513-513, 565-565, 617-617, 652-652, 721-721, 756-756, 791-791, 809-809, 861-861, 913-913
app/client/api-client/sdk.gen.ts (1)
24-31: LGTM! Clean refactoring to arrow function expressions.All SDK functions have been consistently refactored from block bodies with explicit returns to concise arrow function expressions. The refactoring preserves:
- All function signatures and type parameters
- URL paths and HTTP methods
- Header configurations
- Client resolution logic
This is purely a syntactic improvement with no behavioral changes.
Also applies to: 36-43, 48-48, 53-53, 58-58, 63-70, 75-75, 80-87, 92-99, 104-104, 109-109, 114-121, 126-126, 131-131, 136-136, 141-141, 146-146, 151-151, 156-156, 161-168, 173-173, 178-178, 183-183, 188-195, 200-200, 205-205, 210-210, 215-215, 220-227, 232-232, 237-237, 242-249, 254-254, 259-259, 264-271, 276-276, 281-281, 286-286, 291-291, 296-296, 301-308, 313-313, 318-325, 330-330, 335-335, 340-347, 352-352, 357-357, 362-369, 374-374, 379-379, 384-391
app/server/modules/backups/backups.dto.ts (1)
18-39: Name field wiring in DTOs looks consistentThe
namefield is now:
- Present on the schedule schema (
string),- Required and length‑bounded on create (
1 <= string <= 32),- Optional and length‑bounded on update (
(1 <= string <= 32)?),which lines up with the non‑null DB column and the update semantics (omit the field instead of sending null). No issues from an API/validation perspective.
Also applies to: 123-169
app/drizzle/meta/_journal.json (1)
131-144: Journal entry append looks correctNew entry
idx: 19with tag0019_secret_nomadmatches the existing structure and sequencing; no issues.app/client/modules/backups/routes/create-backup.tsx (1)
84-95: Create payload now correctly includes schedule namePassing
name: formValues.nameintocreateBackupScheduleMutationaligns the client request with the new server DTO and DB column. Assuming the form enforces the same 1–32 char constraint, this looks good.app/client/modules/backups/routes/backup-details.tsx (1)
37-41: Usingschedule.namein UI and update payload is consistent
- Breadcrumb label derived from
loaderData.schedule.nameimproves clarity over an ID‑based label.- Including
name: formValues.namein the update body matches the updatedUpdateBackupScheduleBodyand keeps toggling enabled state (which omitsname) from unintentionally overwriting it.No issues spotted here.
Also applies to: 153-164
app/drizzle/meta/0019_snapshot.json (1)
242-365: Snapshot matches updated backup schedule schemaThe snapshot correctly reflects the new
namecolumn and its unique index onbackup_schedules_table, consistent withbackupSchedulesTableinschema.ts. Everything else in this file looks mechanically generated and coherent.app/server/modules/backups/backups.service.ts (1)
114-135: Update path name conflict check looks correctOn update, you:
- Load the schedule and validate the (possibly new) cron expression.
- If
data.nameis provided, query for another schedule with the samenameand a differentidusingand(..., ne(backupSchedulesTable.id, scheduleId)).- Throw
ConflictErrorwhen such a schedule exists.- Then apply the update via
.set({ ...data, nextBackupAt, updatedAt: Date.now() }).This correctly prevents renaming to an already‑taken name while allowing no‑op updates where the name stays the same. The
...dataspread ensuresnameis updated only when present.Also applies to: 148-152
app/client/modules/backups/components/schedule-summary.tsx (1)
1-1: Route parameters are correct; links will work as intendedThe volume and repository links use
schedule.volume.nameandschedule.repository.name, which correctly match the route definitions (volumes/:nameandrepositories/:name). No changes needed.app/client/modules/backups/components/create-schedule-form.tsx (2)
84-84: LGTM!The name field is correctly propagated when editing an existing schedule.
26-26: Backend validation for name field is consistent with frontend constraints.The backend DTO validation in
backups.dto.tsenforces the same 1-32 character constraint on the name field for both create (line 126) and update (line 137) operations, matching the frontend schema requirement. No action needed.app/client/modules/backups/routes/backups.tsx (2)
70-81: LGTM!The header layout properly displays the schedule name with appropriate overflow handling. The use of
truncate,min-w-0, andw-0on the flex container ensures long names are truncated gracefully while keeping the status dot visible.
82-88: LGTM!The updated description provides clear visual context with icons showing the backup flow from volume to repository. The reduced icon size (h-3.5) and text size (text-xs) maintain visual hierarchy appropriately.
* docs: add agents instructions * feat: naming backup schedules * fix: wrong table for filtering
* docs: add agents instructions * feat: naming backup schedules * fix: wrong table for filtering
Summary by CodeRabbit
New Features
Documentation
Style
✏️ Tip: You can customize this high-level summary in your review settings.