Merged
Conversation
Contributor
|
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:
WalkthroughIntroduces snapshot dump/download capability with client-side download UI in restore form and server-side HTTP endpoint. Adds dump:started event infrastructure, path normalization utilities, and streaming support in restic operations. Refactors event subscriptions to use AbortController pattern across multiple components. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Owner
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
This was referenced Feb 19, 2026
Closed
nicotsx
force-pushed
the
02-19-feat_export_snap_as_tar_file
branch
2 times, most recently
from
3345fe2 to
eb3ecd8
Compare
|
Documentation Updates 2 document(s) were updated by changes in this PR: Real-Time Mirror Synchronization Progress, In-Progress Status Display, and Breadcrumb NavigationView Changes@@ -1,6 +1,6 @@
-# Real-Time Mirror Synchronization Progress, In-Progress Status Display, and Breadcrumb Navigation
-
-This document details three UI/UX features in the zerobyte application: real-time mirror synchronization progress indicators with animated visual feedback, the 'in progress' status display during various copy operations, and breadcrumb navigation in the repository details page.
+# Real-Time Mirror Synchronization Progress, In-Progress Status Display, Download Functionality, Minimum Loading Duration, and Breadcrumb Navigation
+
+This document details UI/UX features in the zerobyte application: real-time mirror synchronization progress indicators with animated visual feedback, the 'in progress' status display during various copy operations, snapshot download functionality, minimum loading duration pattern, and breadcrumb navigation in the repository details page.
## Real-Time Mirror Synchronization Progress Indicators
@@ -117,6 +117,14 @@
- Elapsed time
- Transfer speed (MB/s)
+**Event Listener Pattern:**
+
+The component uses AbortController/AbortSignal for event listener cleanup:
+- Creates an AbortController and passes its signal to `addEventListener` calls
+- Listens for `restore:started`, `restore:progress`, and `restore:completed` events
+- Cleanup is simplified to `abortController.abort()` in the useEffect cleanup function
+- This pattern replaces the manual unsubscribe approach, providing automatic cleanup when the component unmounts or dependencies change
+
### Mirror Copy Operations
The [`ScheduleMirrorsConfig` component](https://github.com/nicotsx/zerobyte/blob/fdd8edec702ac43721ceffce5ac9af5cf4e545d5/app/client/modules/backups/components/schedule-mirrors-config.tsx) displays copy status for mirror repositories.
@@ -133,6 +141,35 @@
**Real-Time Updates:**
[Listens to server events](https://github.com/nicotsx/zerobyte/blob/fdd8edec702ac43721ceffce5ac9af5cf4e545d5/app/client/modules/backups/components/schedule-mirrors-config.tsx#L102-L127) `mirror:started` and `mirror:completed` to update status in real-time.
+
+### RestoreForm Component
+
+The [`RestoreForm` component](https://github.com/nicotsx/zerobyte/blob/fdd8edec702ac43721ceffce5ac9af5cf4e545d5/app/client/components/restore-form.tsx) provides snapshot restore and download capabilities.
+
+**Download Functionality:**
+
+- Download button with Download icon in the header alongside Cancel and Restore buttons
+- Button text is dynamic based on selection: "Download All", "Download 1 item", or "Download X items"
+- Download is enabled only when 0 or 1 items are selected (disabled for multiple selections with tooltip explanation)
+- Downloads use the `/api/v1/repositories/{shortId}/snapshots/{snapshotId}/dump` endpoint
+- Query parameters include `path` and `kind` for single item downloads
+- Download is triggered via programmatic link creation and click
+
+**Dynamic Button Labels:**
+
+- Restore button text changes based on state: "Restoring...", "Restore All", "Restore 1 item", or "Restore X items"
+- Uses `getDownloadButtonText()` and `getRestoreButtonText()` helper functions for dynamic labels
+
+**State Management:**
+
+- New state `selectedPathKind` tracks whether selected item is "file" or "dir"
+- Uses `repository.shortId` instead of `repository.id` for consistency with API endpoints
+- AbortController/AbortSignal pattern for event listener cleanup (matching RestoreProgress component)
+
+**Responsive Layout:**
+
+- Header buttons use responsive flex layout: `flex-col` on mobile, `flex-row` on desktop
+- Tooltip component wraps the download button to explain the single-selection limitation
### Common UI Patterns
@@ -142,6 +179,38 @@
2. **StatusDot Component**: The [`StatusDot`](https://github.com/nicotsx/zerobyte/blob/fdd8edec702ac43721ceffce5ac9af5cf4e545d5/app/client/components/status-dot.tsx) with ["info" variant displays a blue dot](https://github.com/nicotsx/zerobyte/blob/fdd8edec702ac43721ceffce5ac9af5cf4e545d5/app/client/components/status-dot.tsx#L34-L38) with pulsing animation
3. **Real-Time Updates**: Server-sent events synchronize UI with backend progress without polling
4. **Consistent Status Values**: `in_progress` string used uniformly across all operation types
+
+---
+
+## Minimum Loading Duration Pattern
+
+The zerobyte application implements a minimum loading duration pattern to prevent flashing UI states for very fast operations.
+
+### useMinimumDuration Hook
+
+The [`useMinimumDuration` hook](https://github.com/nicotsx/zerobyte/blob/fdd8edec702ac43721ceffce5ac9af5cf4e545d5/app/client/hooks/useMinimumDuration.ts) ensures loading states display for a minimum duration for better UX.
+
+**Implementation:**
+
+- Accepts `isActive` boolean and `minimumDuration` number (in milliseconds)
+- Returns a `displayActive` boolean that remains true for at least the minimum duration
+- Uses `useRef` to track start time and timeout reference
+- When `isActive` becomes false, calculates elapsed time and ensures the loading state persists for the remaining duration
+
+**Usage in Button Component:**
+
+The [Button component](https://github.com/nicotsx/zerobyte/blob/fdd8edec702ac43721ceffce5ac9af5cf4e545d5/app/client/components/ui/button.tsx) implements minimum loading duration logic:
+
+- Sets `MINIMUM_LOADING_DURATION` to 300ms
+- Uses `useMinimumDuration` hook to extend the loading state
+- Displays spinner and hides button content while `isLoading` is true
+- Prevents very fast operations from causing UI flicker
+
+**Benefits:**
+
+- Smoother user experience by avoiding rapid state changes
+- Consistent loading feedback across the application
+- Reduces visual noise from operations that complete quickly
---
@@ -221,10 +290,12 @@
## Summary
-The zerobyte application implements three cohesive UI/UX features:
+The zerobyte application implements four cohesive UI/UX features:
**Mirror Synchronization**: Real-time status updates via Server-Sent Events with animated visual feedback using the StatusDot component. Mirror sync displays binary status (syncing/completed) without granular progress metrics, distinguishing it from backup and restore operations.
-**In-Progress Status Display**: Consistent status indicators across backup, restore, and mirror operations using animated spinners, color-coded status dots, and real-time event updates. Backup and restore operations provide detailed progress metrics, while mirror operations show simplified status information.
+**In-Progress Status Display**: Consistent status indicators across backup, restore, and mirror operations using animated spinners, color-coded status dots, and real-time event updates. Backup and restore operations provide detailed progress metrics, while mirror operations show simplified status information. RestoreForm component adds download functionality with dynamic button labels and responsive layout.
+
+**Minimum Loading Duration**: A pattern that ensures loading states persist for at least 300ms to prevent flashing UI states, implemented through the useMinimumDuration hook and applied to the Button component for consistent user experience.
**Breadcrumb Navigation**: Dynamic, route-based breadcrumb system using TanStack Router's `staticData` property with Radix UI components. The system provides consistent navigation context through a two-layer architecture of primitive UI components and an app-level orchestrator that dynamically renders breadcrumbs based on route configuration.Repositories in Zerobyte: Types, Architecture, and OperationsView Changes@@ -232,9 +232,30 @@
- [Browse files](https://github.com/nicotsx/zerobyte/blob/bad944a2329540088a51f7699d6f53d9ac82137c/app/server/modules/repositories/repositories.service.ts#L254-L317)
- [Delete snapshots](https://github.com/nicotsx/zerobyte/blob/bad944a2329540088a51f7699d6f53d9ac82137c/app/server/modules/repositories/repositories.service.ts#L515-L531)
- [Tag snapshots](https://github.com/nicotsx/zerobyte/blob/bad944a2329540088a51f7699d6f53d9ac82137c/app/server/modules/repositories/repositories.service.ts#L553-L575)
+- [Download snapshots](https://github.com/nicotsx/zerobyte/blob/bad944a2329540088a51f7699d6f53d9ac82137c/app/server/modules/repositories/repositories.service.ts#L394-L451)
**Restore Operations:**
- [Restore snapshot data](https://github.com/nicotsx/zerobyte/blob/bad944a2329540088a51f7699d6f53d9ac82137c/app/server/modules/repositories/repositories.service.ts#L319-L386)
+
+#### Download Snapshots
+The `dumpSnapshot()` method provides snapshot download functionality through the `/api/v1/repositories/{shortId}/snapshots/{snapshotId}/dump` endpoint:
+
+**Query Parameters:**
+- `path` (optional): Specific path within the snapshot to download
+- `kind` (optional): Type of path - `"file"` or `"dir"` (required when `path` is specified)
+
+**Behavior:**
+- Without parameters: Downloads entire snapshot as a tar archive
+- With path pointing to a directory (`kind: "dir"`): Downloads directory contents as tar archive
+- With path pointing to a file (`kind: "file"`): Downloads raw file stream with `application/octet-stream` content type
+
+**Implementation Details:**
+- Uses the `prepareSnapshotDump()` helper to process paths and determine the appropriate snapshot reference
+- Acquires a shared repository lock with key `dump:${snapshotId}` to prevent conflicts during the operation
+- Emits a `dump:started` server event containing `organizationId`, `repositoryId`, `snapshotId`, `path`, and `filename`
+- Returns a stream with completion promise, filename, and appropriate content type
+- Sets proper `Content-Type` and `Content-Disposition` headers for file downloads
+- Defaults to filename pattern `snapshot-<snapshotId>.tar` or uses the actual filename for individual files
**Health Checks:**
- [Check repository health](https://github.com/nicotsx/zerobyte/blob/bad944a2329540088a51f7699d6f53d9ac82137c/app/server/modules/repositories/repositories.service.ts#L420-L447)
@@ -258,7 +279,7 @@
### Concurrency Control
Repository operations use a [sophisticated mutex system](https://github.com/nicotsx/zerobyte/blob/bad944a2329540088a51f7699d6f53d9ac82137c/app/server/core/repository-mutex.ts) to prevent conflicts:
-- **Shared locks**: Multiple read operations (snapshots, ls, restore) can run concurrently
+- **Shared locks**: Multiple read operations (snapshots, ls, restore, dump) can run concurrently
- **Exclusive locks**: Write operations (backup, forget, check, doctor) require exclusive access
- Operations wait in a queue when locks are held
How did I do? Any feedback? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
app/server/utils/sanitize.ts (1)
28-34: Consider stripping all control characters, not just CR/LF.The current regex only removes
\rand\n, but other control characters (NUL\0, tab, etc. in the0x00–0x1Fand0x7Frange) can also cause issues in HTTP headers. For a security-sensitive header likeContent-Disposition, a more restrictive approach is safer.♻️ Suggested broader sanitization
export const sanitizeContentDispositionFilename = (filename: string): string => { const sanitized = filename - .replace(/[\r\n]/g, "") + .replace(/[\x00-\x1f\x7f]/g, "") .replace(/["\\]/g, "_") .trim(); return sanitized || "snapshot.tar"; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/utils/sanitize.ts` around lines 28 - 34, The sanitizeContentDispositionFilename function currently only strips CR/LF; update it to remove all ASCII control characters (0x00–0x1F and 0x7F) before other replacements to harden Content-Disposition handling: first replace control chars with empty string or a safe character, then continue replacing quotes/backslashes and trimming, and preserve the fallback filename "snapshot.tar" when the result is empty; target the sanitizeContentDispositionFilename symbol for the change.app/server/utils/restic.ts (2)
559-577:normalizeDumpPathdoesn't normalize..path segments.An input like
/data/../etc/shadowpasses through unchanged. While restic resolves paths within the snapshot tree (so this isn't a host filesystem traversal risk), it may produce surprising behavior or confusing error messages. Consider normalizing withpath.posix.normalizeafter adding the leading slash to collapse..segments.♻️ Optional: normalize path segments
+import pathPosix from "node:path/posix"; + const normalizeDumpPath = (pathToDump?: string): string => { const trimmedPath = pathToDump?.trim(); if (!trimmedPath) { return "/"; } const decodedPath = (() => { try { return decodeURIComponent(trimmedPath); } catch { return trimmedPath; } })(); const withLeadingSlash = decodedPath.startsWith("/") ? decodedPath : `/${decodedPath}`; - const normalizedPath = withLeadingSlash.replace(/\/+$/, ""); + const normalizedPath = pathPosix.normalize(withLeadingSlash).replace(/\/+$/, ""); return normalizedPath || "/"; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/utils/restic.ts` around lines 559 - 577, normalizeDumpPath currently decodes and ensures a leading slash but doesn't collapse .. segments; update normalizeDumpPath to, after you compute withLeadingSlash, run path.posix.normalize(withLeadingSlash) to collapse "." and ".." components (use Node's path.posix to enforce POSIX behavior), then strip any trailing slashes like the current replace(/\/+$/, "") and finally return the normalized result or "/" if empty; keep the decodeURIComponent wrapping around trimmedPath and reference normalizeDumpPath, decodedPath, withLeadingSlash, and normalizedPath when making the change.
607-649: Race betweencompletionrejection and stream consumption could cause unhandled rejection.The
completionpromise rejects (viathrow new ResticError(...)in.then()) when restic exits non-zero. If the consumer is still piping the stream and hasn't yetawaitedcompletion, Node will emit an unhandled promise rejection warning. Consider eagerly attaching a no-op.catch()to prevent this, and re-throwing when the consumer explicitly awaits:♻️ Prevent unhandled rejection while preserving error propagation
const completion = safeSpawn({ ... }) .then((result) => { if (result.exitCode === 0) { return; } const stderr = stderrTail.trim() || result.error; logger.error(`Restic dump failed: ${stderr}`); throw new ResticError(result.exitCode, stderr); }) .finally(async () => { abortController = null; await cleanup(); }); + // Prevent unhandled rejection; consumers must await .completion to observe errors + completion.catch(() => {}); + if (!stream) {The consumer still gets the error when they
await dumpStream.completion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/utils/restic.ts` around lines 607 - 649, The completion promise can reject while the stream consumer hasn't awaited it, causing unhandled rejections; immediately attach a no-op .catch() to the internal completion to suppress unhandled-rejection warnings, and expose a separate promise that preserves original semantics for callers to await. Concretely: after you create completion (the safeSpawn result), do completion.catch(() => {}); then create and return/export a new promise (e.g., completionPromise or completionPropagated) that does new Promise((res, rej) => completion.then(res, rej)) so callers still receive the ResticError when they await it; keep existing symbols (completion, ResticError, cleanup, abortController, stream, safeSpawn) and ensure the no-op catch is added immediately after completion is assigned.app/server/utils/spawn.ts (1)
71-83:onStdoutis silently ignored whenstdoutModeis"raw".If a caller passes both
stdoutMode: "raw"andonStdout, the callback will never fire. Consider either documenting this or adding a TypeScript overload/discriminated union to make the incompatibility explicit at the type level.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/utils/spawn.ts` around lines 71 - 83, The code currently ignores onStdout when stdoutMode === "raw", so callers can pass a callback that never runs; fix this by making the API explicit: update the spawn function's options type to a discriminated union (or add TS overloads) that ties onStdout to stdoutMode === "lines" (e.g., OptionsLines { stdoutMode: "lines"; onStdout?: (s: string) => void } | OptionsRaw { stdoutMode: "raw"; onStdout?: never }), update the spawn function signature and any call sites to match, and/or alternatively implement runtime behavior in spawn (in the branch handling stdoutMode === "raw" call onStdout with raw chunks) — reference stdoutMode, onStdout, child.stdout and the spawn function in spawn.ts when making the change.app/server/modules/repositories/repositories.service.ts (1)
393-434: Well-structured lock management for streaming.The idempotent
release()guard andcompletion.finally(release)pattern is clean. One minor observation:If an error occurs after
restic.dumpsucceeds but before the function returns (e.g., a synchronous throw from aserverEventslistener), the catch block releases the lock and rethrows—but the already-obtaineddumpStreamis never aborted. The underlying restic process would run until natural completion. Consider callingdumpStream.abort()in the catch block to ensure cleanup:Proposed fix
} catch (error) { + dumpStream?.abort(); release(); throw error; }This requires hoisting
dumpStream(e.g.,let dumpStream: ... | undefined;) before the try block.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/modules/repositories/repositories.service.ts` around lines 393 - 434, The dumpSnapshot function can leak the restic process if an error occurs after restic.dump succeeds but before returning; hoist a variable for dumpStream (e.g., let dumpStream = undefined) above the try block, assign it when calling restic.dump, and in the catch block if dumpStream is defined call dumpStream.abort() before calling release() and rethrowing; keep the existing completion.finally(release) logic and ensure the returned object still uses the local dumpStream and prepared filename.app/client/components/restore-form.tsx (2)
232-235: Consider disabling the download button while a download is pending.The button has
disabled={!canDownload}but notdisabled={!canDownload || isDownloadPending}. While theloadingprop provides a visual indicator andhandleDownloadhas an early return guard, screen readers and keyboard users may still perceive the button as interactive during the pending state.Proposed fix
- <Button variant="outline" onClick={handleDownload} disabled={!canDownload} loading={isDownloadPending}> + <Button variant="outline" onClick={handleDownload} disabled={!canDownload || isDownloadPending} loading={isDownloadPending}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/client/components/restore-form.tsx` around lines 232 - 235, The Download button is only disabled based on canDownload but remains focusable during an ongoing download; update the Button props (the JSX using <Button ... onClick={handleDownload} loading={isDownloadPending}>) to set disabled to {!canDownload || isDownloadPending} so it becomes non-interactive while isDownloadPending is true (also consider ensuring handleDownload still guards early and that any ARIA state such as aria-busy or aria-disabled is set if needed for assistive tech).
153-184: Anchor-based download won't surface server errors to the user.If
dumpSnapshotfails server-side (e.g., 404 for an invalid snapshot), the browser will navigate to (or download) the error response body rather than showing a toast or alert. This is a known trade-off of the programmatic-anchor approach vs.fetch()+ blob URL.Not blocking, but worth noting for future UX polish — especially if users might attempt to dump snapshots that no longer exist.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/client/components/restore-form.tsx` around lines 153 - 184, The current handleDownload uses a programmatic anchor which will surface raw server error pages to the browser; change handleDownload to fetch the dump endpoint (use repository.shortId, snapshotId, and selectedPaths to build the URL), await the response, check response.ok and show a toast/alert on error, otherwise convert the response to a blob, create an object URL, assign it to a temporary anchor to download, revoke the object URL, and preserve the existing downloadPendingTimeoutRef and setIsDownloadPending logic so the pending state is managed the same way.app/client/components/ui/button.tsx (1)
57-60:position: absoluteon the spinner withoutposition: relativeon the button.The button uses
inline-flex, which does not establish a CSS positioning context. Theabsolute-positionedLoader2will therefore be positioned relative to the nearest positioned ancestor further up the DOM tree — not the button itself. While browsers may render it close to the intended location via "static-position" fallback, this behaviour is implementation-dependent and fragile.♻️ Add `relative` to the button's class
- className={cn(buttonVariants({ variant, size, className }), "transition-all")} + className={cn(buttonVariants({ variant, size, className }), "relative transition-all")}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/client/components/ui/button.tsx` around lines 57 - 60, The spinner Loader2 is positioned absolute but the button element (rendered with cn(buttonVariants({ variant, size, className }), "transition-all")) doesn't establish a positioning context; update the button's class composition to include "relative" (e.g., add "relative" alongside "transition-all" in the cn call that wraps buttonVariants) so the absolutely positioned Loader2 is positioned relative to the button; reference the cn(buttonVariants(...), "transition-all") expression and the Loader2 component to locate where to add "relative".app/client/hooks/useMinimumDuration.ts (1)
16-16: Redundant!isActiveinelse ifcondition.Since line 16 is already the
elsebranch ofif (isActive),!isActiveis alwaystruehere. The only meaningful guard isstartTimeRef.current !== null.♻️ Simplify the condition
- } else if (!isActive && startTimeRef.current !== null) { + } else if (startTimeRef.current !== null) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/client/hooks/useMinimumDuration.ts` at line 16, The else-if condition in useMinimumDuration (the branch after if (isActive)) redundantly checks !isActive; update the branch guarding logic to remove the pointless !isActive check and only test startTimeRef.current !== null (i.e., change the else if ( !isActive && startTimeRef.current !== null ) branch to else if (startTimeRef.current !== null)), keeping all existing behavior that uses startTimeRef.current and any subsequent code in that branch unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/client/components/ui/button.tsx`:
- Around line 54-58: Destructure disabled from the incoming props (e.g., const {
disabled, ...props } = props) so it isn't included in the spread, then place the
disabled prop after the spread on the rendered component (Comp) and set it to
the loading-controlled value (disabled={isLoading}) so the loading state cannot
be silently overridden by the caller; reference Comp, isLoading, and the
{...props} spread when making this change.
In `@app/server/modules/repositories/repositories.dump.ts`:
- Around line 5-27: The function normalizeAbsolutePath currently calls
decodeURIComponent(trimmed) which can throw a URIError for malformed
percent-encodings; wrap that call in a try/catch inside normalizeAbsolutePath
and, on error, treat the input as invalid (e.g., return "/" or fall through to
the existing invalid-path handling) so the function does not propagate a 500;
ensure the catch only handles URIError (or any errors from decodeURIComponent)
and preserves the subsequent normalization logic for valid decoded input.
---
Nitpick comments:
In `@app/client/components/restore-form.tsx`:
- Around line 232-235: The Download button is only disabled based on canDownload
but remains focusable during an ongoing download; update the Button props (the
JSX using <Button ... onClick={handleDownload} loading={isDownloadPending}>) to
set disabled to {!canDownload || isDownloadPending} so it becomes
non-interactive while isDownloadPending is true (also consider ensuring
handleDownload still guards early and that any ARIA state such as aria-busy or
aria-disabled is set if needed for assistive tech).
- Around line 153-184: The current handleDownload uses a programmatic anchor
which will surface raw server error pages to the browser; change handleDownload
to fetch the dump endpoint (use repository.shortId, snapshotId, and
selectedPaths to build the URL), await the response, check response.ok and show
a toast/alert on error, otherwise convert the response to a blob, create an
object URL, assign it to a temporary anchor to download, revoke the object URL,
and preserve the existing downloadPendingTimeoutRef and setIsDownloadPending
logic so the pending state is managed the same way.
In `@app/client/components/ui/button.tsx`:
- Around line 57-60: The spinner Loader2 is positioned absolute but the button
element (rendered with cn(buttonVariants({ variant, size, className }),
"transition-all")) doesn't establish a positioning context; update the button's
class composition to include "relative" (e.g., add "relative" alongside
"transition-all" in the cn call that wraps buttonVariants) so the absolutely
positioned Loader2 is positioned relative to the button; reference the
cn(buttonVariants(...), "transition-all") expression and the Loader2 component
to locate where to add "relative".
In `@app/client/hooks/useMinimumDuration.ts`:
- Line 16: The else-if condition in useMinimumDuration (the branch after if
(isActive)) redundantly checks !isActive; update the branch guarding logic to
remove the pointless !isActive check and only test startTimeRef.current !== null
(i.e., change the else if ( !isActive && startTimeRef.current !== null ) branch
to else if (startTimeRef.current !== null)), keeping all existing behavior that
uses startTimeRef.current and any subsequent code in that branch unchanged.
In `@app/server/modules/repositories/repositories.service.ts`:
- Around line 393-434: The dumpSnapshot function can leak the restic process if
an error occurs after restic.dump succeeds but before returning; hoist a
variable for dumpStream (e.g., let dumpStream = undefined) above the try block,
assign it when calling restic.dump, and in the catch block if dumpStream is
defined call dumpStream.abort() before calling release() and rethrowing; keep
the existing completion.finally(release) logic and ensure the returned object
still uses the local dumpStream and prepared filename.
In `@app/server/utils/restic.ts`:
- Around line 559-577: normalizeDumpPath currently decodes and ensures a leading
slash but doesn't collapse .. segments; update normalizeDumpPath to, after you
compute withLeadingSlash, run path.posix.normalize(withLeadingSlash) to collapse
"." and ".." components (use Node's path.posix to enforce POSIX behavior), then
strip any trailing slashes like the current replace(/\/+$/, "") and finally
return the normalized result or "/" if empty; keep the decodeURIComponent
wrapping around trimmedPath and reference normalizeDumpPath, decodedPath,
withLeadingSlash, and normalizedPath when making the change.
- Around line 607-649: The completion promise can reject while the stream
consumer hasn't awaited it, causing unhandled rejections; immediately attach a
no-op .catch() to the internal completion to suppress unhandled-rejection
warnings, and expose a separate promise that preserves original semantics for
callers to await. Concretely: after you create completion (the safeSpawn
result), do completion.catch(() => {}); then create and return/export a new
promise (e.g., completionPromise or completionPropagated) that does new
Promise((res, rej) => completion.then(res, rej)) so callers still receive the
ResticError when they await it; keep existing symbols (completion, ResticError,
cleanup, abortController, stream, safeSpawn) and ensure the no-op catch is added
immediately after completion is assigned.
In `@app/server/utils/sanitize.ts`:
- Around line 28-34: The sanitizeContentDispositionFilename function currently
only strips CR/LF; update it to remove all ASCII control characters (0x00–0x1F
and 0x7F) before other replacements to harden Content-Disposition handling:
first replace control chars with empty string or a safe character, then continue
replacing quotes/backslashes and trimming, and preserve the fallback filename
"snapshot.tar" when the result is empty; target the
sanitizeContentDispositionFilename symbol for the change.
In `@app/server/utils/spawn.ts`:
- Around line 71-83: The code currently ignores onStdout when stdoutMode ===
"raw", so callers can pass a callback that never runs; fix this by making the
API explicit: update the spawn function's options type to a discriminated union
(or add TS overloads) that ties onStdout to stdoutMode === "lines" (e.g.,
OptionsLines { stdoutMode: "lines"; onStdout?: (s: string) => void } |
OptionsRaw { stdoutMode: "raw"; onStdout?: never }), update the spawn function
signature and any call sites to match, and/or alternatively implement runtime
behavior in spawn (in the branch handling stdoutMode === "raw" call onStdout
with raw chunks) — reference stdoutMode, onStdout, child.stdout and the spawn
function in spawn.ts when making the change.
chore(mutext): prevent double lock release
nicotsx
force-pushed
the
02-19-feat_export_snap_as_tar_file
branch
from
eb3ecd8 to
49abcc5
Compare
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
app/utils/path.ts (2)
35-39: Dead code: the else branch of thestartsWith("/")ternary is unreachable.
withoutTrailingSlashis derived fromnormalized(which starts with"/") by stripping trailing slashes. After the!withoutTrailingSlashguard on line 31, it always starts with"/", making the else branch unreachable. The whole block simplifies to a single expression.♻️ Proposed simplification
- const withSingleLeadingSlash = withoutTrailingSlash.startsWith("/") - ? `/${withoutTrailingSlash.replace(/^\/+/, "")}` - : `/${withoutTrailingSlash}`; - - return withSingleLeadingSlash || "/"; + return `/${withoutTrailingSlash.replace(/^\/+/, "")}` || "/";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/utils/path.ts` around lines 35 - 39, The ternary in the withSingleLeadingSlash assignment is dead because withoutTrailingSlash is guaranteed to start with "/" (normalized starts with "/" and you already guard !withoutTrailingSlash); simplify by removing the unreachable else branch and the temporary withSingleLeadingSlash variable and return a single expression that ensures exactly one leading slash, e.g. return `/${withoutTrailingSlash.replace(/^\/+/, "")}` (keep the existing !withoutTrailingSlash guard intact).
24-28: Dead code: the guard conditions at lines 26-28 are unreachable.After
let normalized = "/" + stack.join("/"),normalizedalways starts with"/". Therefore:
!normalized→ alwaysfalsenormalized === "."→ alwaysfalsenormalized.startsWith("..")→ alwaysfalseThe entire
ifblock is unreachable.♻️ Proposed removal
let normalized = "/" + stack.join("/"); - if (!normalized || normalized === "." || normalized.startsWith("..")) { - return "/"; - } - const withoutTrailingSlash = normalized.replace(/\/+$/, "");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/utils/path.ts` around lines 24 - 28, The guard block after computing let normalized = "/" + stack.join("/") is unreachable; change the logic so the validation runs against the raw join result or the stack length before prepending the slash: compute const raw = stack.join("/"); if (!raw || raw === "." || raw.startsWith("..")) return "/"; then set normalized = "/" + raw (or simply return "/" when stack is empty). Update references to the variable 'normalized' and the stack.join("/") expression accordingly to remove the dead if-block.app/server/core/repository-mutex.ts (1)
58-66: Fast-path releases don't usecreateRelease— inconsistent with the slow paths.Lines 65 and 119 return raw inline lambdas, while the slow paths (lines 102, 158) use the new
createReleasefactory. The underlyingreleaseShared/releaseExclusivemethods are still effectively safe for double-calls due to their own state guards, but applyingcreateReleaseconsistently to all paths would make the API surface uniform.♻️ Proposed consistency fix for fast paths
if (!state.exclusiveHolder && !hasExclusiveInQueue) { const lockId = this.generateLockId(); state.sharedHolders.set(lockId, { id: lockId, operation, acquiredAt: Date.now(), }); - return () => this.releaseShared(repositoryId, lockId); + return this.createRelease("shared", repositoryId, lockId); }if (!state.exclusiveHolder && state.sharedHolders.size === 0 && state.waitQueue.length === 0) { const lockId = this.generateLockId(); state.exclusiveHolder = { id: lockId, operation, acquiredAt: Date.now(), }; - return () => this.releaseExclusive(repositoryId, lockId); + return this.createRelease("exclusive", repositoryId, lockId); }Also applies to: 112-120
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/core/repository-mutex.ts` around lines 58 - 66, The fast-path returns currently return raw inline lambdas (e.g. () => this.releaseShared(repositoryId, lockId) and the analogous exclusive lambda) which makes the return value inconsistent with slow paths that use createRelease; change those fast-path return points to use createRelease(...) instead, passing the repositoryId and lockId and the corresponding release method (releaseShared or releaseExclusive) so all acquisition paths return the same created release function; use generateLockId, state.sharedHolders/state.exclusiveHolder and hasExclusiveInQueue to locate the exact spots to replace the inline lambdas with createRelease.app/client/components/restore-form.tsx (1)
228-240: Tooltip UX: tooltip content is hidden via CSS rather than conditionally rendered.Using
cn({ hidden: canDownload })onTooltipContentmeans the tooltip DOM is always present but hidden with CSS when download is available. This works functionally, but Radix'sTooltipmay still trigger portal/positioning logic for the hidden content. Consider conditionally rendering the tooltip wrapper instead, or this is fine if you prefer the simpler approach.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/client/components/restore-form.tsx` around lines 228 - 240, TooltipContent is always mounted and merely hidden via cn({ hidden: canDownload }), which can still trigger Radix tooltip portal/positioning; instead conditionally render the Tooltip (or at least TooltipContent) based on canDownload so it isn't in the DOM when download is allowed. Update the component around Tooltip/TooltipContent (the Tooltip wrapper, TooltipTrigger, TooltipContent, and the cn usage) to render TooltipContent only when !canDownload (or render the entire Tooltip only when !canDownload), removing the reliance on the hidden CSS class for controlling presence.app/server/utils/spawn.ts (1)
85-97: In raw mode,summaryin the result will always be empty string.Since no readline interface is attached to stdout in raw mode,
lastStdoutremains"". This meansSpawnResult.summaryis meaningless for raw-mode callers. This is fine for the current dump use case (which usescompletionpromise and stderr for error detection), but worth noting if future raw-mode callers depend onsummary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/utils/spawn.ts` around lines 85 - 97, The SpawnResult.summary is always empty in raw stdoutMode because no listener updates lastStdout; add a data listener in the branch handling raw stdoutMode (the same area that handles stdoutMode === "lines") to set child.stdout.setEncoding("utf8") and update lastStdout from incoming chunks (convert chunk to string, trim and if non-empty assign to lastStdout) and optionally invoke onStdout(chunkString) so raw-mode callers get the last output; keep the existing createInterface/line handling for stdoutMode === "lines" unchanged and update the code paths that compute SpawnResult.summary to rely on lastStdout.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/client/api-client/`@tanstack/react-query.gen.ts:
- Around line 857-874: The generated query dumpSnapshotOptions (and its key
dumpSnapshotQueryKey) returns DumpSnapshotResponse which is Blob|File and will
be cached in React Query memory; to avoid retaining large archives, modify the
consuming code to either call dumpSnapshotOptions with queryOptions overriding
gcTime: 0 and staleTime: 0 to immediately evict the blob after use, or bypass
the generated query entirely and use a direct fetch/download URL (or call
dumpSnapshot directly) so the tar/Blob is not stored in React Query cache;
update consumers that use dumpSnapshotOptions to implement one of these
approaches and ensure DumpSnapshotResponse blobs are not left in memory.
In `@app/client/components/restore-form.tsx`:
- Around line 153-181: The handleDownload function sets waitingForDownload but
it never becomes visible and can get stuck; remove the ineffective loading state
usage by eliminating setWaitingForDownload(true/false) calls from handleDownload
and move the early guard (if selectedPaths.size > 1) to run before any state
mutation so you don't set state and then return; update any UI/button that
relied on waitingForDownload to use selectedPaths.size or another explicit
condition instead, and remove waitingForDownload from the handleDownload
dependency array (and from the component state if no longer used).
In `@app/server/core/__tests__/repository-mutex.test.ts`:
- Around line 250-259: The test uses expect(...).rejects.toThrow() without
awaiting the resulting promise, so the assertions can be skipped; update the two
assertions to await the promise returned by expect for
repoMutex.acquireShared(repoId, "s1", controller.signal) and
repoMutex.acquireExclusive(repoId, "e1", controller.signal) (i.e., prepend await
to both expect(...).rejects.toThrow(...)) and apply the same fix to the other
tests that use expect(...).rejects.toThrow() (the earlier tests around the same
suite that call acquireShared/acquireExclusive or other promise-returning
assertions) so the test runner waits for the rejection assertions to execute.
In `@app/server/utils/restic.ts`:
- Around line 560-575: normalizeDumpPath currently decodes the input then passes
it to normalizeAbsolutePath which decodes again; remove the decode in
normalizeDumpPath by taking the trimmedPath (from pathToDump?.trim()), returning
"/" if empty, and directly calling normalizeAbsolutePath(trimmedPath) so
decoding is only handled inside normalizeAbsolutePath; keep the early-return for
empty input and remove the try/catch decode block and the decodedPath variable.
In `@app/utils/path.ts`:
- Line 5: Wrap the decodeURIComponent call that computes normalizedInput in a
try/catch: attempt decodeURIComponent(trimmed).replace(/\\+/g, "/") and if it
throws (URIError or any error) fall back to using the raw trimmed value (with
.replace(/\\+/g, "/") applied) so filenames with bare '%' do not crash; update
the assignment to normalizedInput and keep using the existing trimmed variable
and same replace logic in the fallback.
---
Nitpick comments:
In `@app/client/components/restore-form.tsx`:
- Around line 228-240: TooltipContent is always mounted and merely hidden via
cn({ hidden: canDownload }), which can still trigger Radix tooltip
portal/positioning; instead conditionally render the Tooltip (or at least
TooltipContent) based on canDownload so it isn't in the DOM when download is
allowed. Update the component around Tooltip/TooltipContent (the Tooltip
wrapper, TooltipTrigger, TooltipContent, and the cn usage) to render
TooltipContent only when !canDownload (or render the entire Tooltip only when
!canDownload), removing the reliance on the hidden CSS class for controlling
presence.
In `@app/server/core/repository-mutex.ts`:
- Around line 58-66: The fast-path returns currently return raw inline lambdas
(e.g. () => this.releaseShared(repositoryId, lockId) and the analogous exclusive
lambda) which makes the return value inconsistent with slow paths that use
createRelease; change those fast-path return points to use createRelease(...)
instead, passing the repositoryId and lockId and the corresponding release
method (releaseShared or releaseExclusive) so all acquisition paths return the
same created release function; use generateLockId,
state.sharedHolders/state.exclusiveHolder and hasExclusiveInQueue to locate the
exact spots to replace the inline lambdas with createRelease.
In `@app/server/utils/spawn.ts`:
- Around line 85-97: The SpawnResult.summary is always empty in raw stdoutMode
because no listener updates lastStdout; add a data listener in the branch
handling raw stdoutMode (the same area that handles stdoutMode === "lines") to
set child.stdout.setEncoding("utf8") and update lastStdout from incoming chunks
(convert chunk to string, trim and if non-empty assign to lastStdout) and
optionally invoke onStdout(chunkString) so raw-mode callers get the last output;
keep the existing createInterface/line handling for stdoutMode === "lines"
unchanged and update the code paths that compute SpawnResult.summary to rely on
lastStdout.
In `@app/utils/path.ts`:
- Around line 35-39: The ternary in the withSingleLeadingSlash assignment is
dead because withoutTrailingSlash is guaranteed to start with "/" (normalized
starts with "/" and you already guard !withoutTrailingSlash); simplify by
removing the unreachable else branch and the temporary withSingleLeadingSlash
variable and return a single expression that ensures exactly one leading slash,
e.g. return `/${withoutTrailingSlash.replace(/^\/+/, "")}` (keep the existing
!withoutTrailingSlash guard intact).
- Around line 24-28: The guard block after computing let normalized = "/" +
stack.join("/") is unreachable; change the logic so the validation runs against
the raw join result or the stack length before prepending the slash: compute
const raw = stack.join("/"); if (!raw || raw === "." || raw.startsWith(".."))
return "/"; then set normalized = "/" + raw (or simply return "/" when stack is
empty). Update references to the variable 'normalized' and the stack.join("/")
expression accordingly to remove the dead if-block.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #91 Closes #241
Summary by CodeRabbit
New Features
Tests
Chores