feat: expand snapshot details with additional info#505
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR extends the backup reporting system with summary data, introducing new DTO schemas for Restic and backup events, adding a UI component to display backup summaries, creating a byte formatting utility, and propagating summary information through the backup execution and notification flows. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/server/modules/notifications/notifications.service.ts`:
- Around line 487-491: The notification body currently repeats metrics because
derivedDuration, derivedFilesProcessed, derivedBytesProcessed, and
derivedSnapshotId are always appended alongside summaryLines; update the logic
in notifications.service.ts to avoid duplicates by conditionally omitting the
derived* lines when summaryLines is non-empty (or alternatively adjust
buildBackupSummaryLines to exclude overlapping fields), e.g., check if
summaryLines.length > 0 before pushing `Duration: ...`, `Files: ...`, `Size:
...`, `Snapshot: ...` so only one set of metric lines appears; reference the
derivedDuration/derivedFilesProcessed/derivedBytesProcessed/derivedSnapshotId
variables and the summaryLines array or the buildBackupSummaryLines helper to
implement this change.
🧹 Nitpick comments (6)
app/server/modules/repositories/repositories.controller.ts (2)
102-121: Duration calculation is duplicated betweenlistSnapshotsandgetSnapshotDetails.Lines 105–109 here and lines 135–139 in the
getSnapshotDetailshandler compute duration identically. Consider extracting a small helper to keep them in sync.♻️ Optional extraction
+function getSnapshotDuration(summary?: { backup_start: string; backup_end: string }): number { + if (!summary) return 0; + return new Date(summary.backup_end).getTime() - new Date(summary.backup_start).getTime(); +} +
116-117: Prefer?? 0over|| 0for numeric fallback.
|| 0also coerces0itself to the fallback (though the result is the same here). Using?? 0is more precise and idiomatic for nullish checks. Same applies to Line 147.♻️ Suggested fix
- size: summary?.total_bytes_processed || 0, + size: summary?.total_bytes_processed ?? 0,app/client/modules/backups/components/backup-progress-card.tsx (1)
8-8: Inconsistent import path style — use tilde alias for consistency.Other files in this PR (e.g.,
notifications.service.tsline 19) import from~/utils/format-bytes. This relative path traversal is fragile and inconsistent.♻️ Suggested fix
-import { formatBytes } from "../../../../utils/format-bytes"; +import { formatBytes } from "~/utils/format-bytes";app/server/utils/restic.ts (1)
366-377: Pre-existing:summaryLinetype confusion between string and parsed object.
summaryLineis declared asstringbut line 370 assigns the parsed JSON object to it. In the success path,resticBackupOutputSchemareceives an object (correct), but in the catch path it receives the string"{}"(will fail validation and hit the null fallback). This is a pre-existing pattern, not introduced by this PR, but worth noting for a future cleanup.The same pattern exists in the restore path (lines 443–454).
app/server/modules/backups/backups.execution.ts (1)
135-138:buildBackupSummaryis a no-op identity function.This helper returns
undefinedfor falsy input or the input itself unchanged — it performs no transformation. Consider inliningresult ?? undefinedat the call site (line 147), or if the intent is to eventually map/transform the result into a different summary shape, add a TODO comment clarifying the planned logic.♻️ Proposed inline simplification
-const buildBackupSummary = (result: ResticBackupOutputDto | null | undefined) => { - if (!result) return undefined; - return result; -}; - const finalizeSuccessfulBackup = async ( ctx: BackupContext, scheduleId: number, exitCode: number, result: ResticBackupOutputDto | null, ) => { const finalStatus = exitCode === 0 ? "success" : "warning"; - const summary = buildBackupSummary(result); + const summary = result ?? undefined;app/schemas/events-dto.ts (1)
20-25: Note:summarytype is broader than what's actually passed.The event schema declares
summaryasresticBackupRunSummarySchema.optional(), butfinalizeSuccessfulBackupinbackups.execution.ts(line 186) passes aResticBackupOutputDto(which addsmessage_type: 'summary'). This is structurally compatible in TypeScript, so it works — but the consumer will receive the extramessage_typefield that isn't reflected in the event's type. This is fine if intentional; just noting the slight type widening.
1 participant
Closes #385
Summary by CodeRabbit
Release Notes
New Features
Improvements