Move archive extraction to the library side, harden multi-volume handling - #809
Move archive extraction to the library side, harden multi-volume handling#809Doezer wants to merge 3 commits into
Conversation
…andling - Unpack archives at the destination instead of the downloader directory: move/copy relocate then extract in place; hardlink/symlink extract straight from the archive's original path to the final destination - Detect archives already extracted by the downloader via content comparison against loose sibling files, excluding them from transfer - Fix .partN.rar sibling detection for multi-volume RAR archives - Normalize cross-platform path separators when matching archive entries against loose files on disk - Surface stranded move/copy extraction failures as an in-app notification with recovery guidance instead of failing silently - Add ImportReviewModal hints explaining hardlink/symlink requirements Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughArchive imports now resolve archive contents and multipart volumes before transfer, extract into library destinations according to transfer mode, skip fully extracted archives, and preserve failed relocations for diagnosis. Transfer strategies support exclusions and hardlink fallback, with updated tests, UI guidance, decision records, and main-branch deployment settings. ChangesArchive import flow
Main branch deployment configuration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ImportManager
participant ArchiveService
participant PCImportStrategy
participant Library
ImportManager->>ArchiveService: resolve archive and extraction state
ArchiveService-->>ImportManager: archive resolution and excluded volumes
ImportManager->>PCImportStrategy: plan import with directory hint
PCImportStrategy-->>ImportManager: import plan
ImportManager->>Library: move or copy archive for move/copy modes
ImportManager->>Library: extract archive in destination
ImportManager->>PCImportStrategy: execute transfer with exclusions
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the game import process to perform archive unpacking on the library side rather than the downloader side, utilizing transfer-mode-specific extraction paths and adding duplication detection to skip redundant extractions. The code review identifies several critical issues and improvement opportunities: a bug in parsing split volume archives with numeric extensions, a path traversal vulnerability in the duplication detection logic, potential failures when cleaning up broken symlinks due to target resolution, an inaccurate notification message regarding the transfer mode, and a potential failure to strip file extensions from unnormalized proposed paths.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const stem = path | ||
| .basename(archivePath) | ||
| .replace(/\.part\d+\.rar$/i, "") | ||
| .replace(/\.(rar|zip|7z|gz|tar|iso|bz2)$/i, ""); |
There was a problem hiding this comment.
The current stem derivation does not handle split volume archives with numeric extensions (e.g., .7z.001 or .zip.001) correctly. Since .001 is not stripped by the second replace, the stem becomes Game.7z.001 instead of Game, which prevents findVolumeSiblings from matching other volumes like Game.7z.002.
Stripping the numeric split volume extension first resolves this issue.
const stem = path
.basename(archivePath)
.replace(/\.\d{3}$/i, "")
.replace(/\.part\d+\.rar$/i, "")
.replace(/\.(rar|zip|7z|gz|tar|iso|bz2)$/i, "");| for (const entry of entries) { | ||
| const normalizedName = entry.name.split(/[/\\]+/).join(path.sep); | ||
| const candidatePath = path.join(baseDir, normalizedName); | ||
| try { | ||
| const stats = await fs.stat(candidatePath); | ||
| if (stats.isDirectory() || stats.size !== entry.size) return false; | ||
| } catch { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
The isAlreadyExtracted method is vulnerable to path traversal if an archive contains entries with directory traversal sequences (e.g., ../../). Resolving the candidate path and verifying that it remains within the base directory prevents unauthorized file existence checks outside the intended directory.
const resolvedBase = path.resolve(baseDir);
for (const entry of entries) {
const normalizedName = entry.name.split(/[\/\\]+/).join(path.sep);
const candidatePath = path.resolve(baseDir, normalizedName);
if (candidatePath !== resolvedBase && !candidatePath.startsWith(resolvedBase + path.sep)) {
return false;
}
try {
const stats = await fs.stat(candidatePath);
if (stats.isDirectory() || stats.size !== entry.size) return false;
} catch {
return false;
}
}| } else if (mode === "symlink") { | ||
| if (await fs.pathExists(destFile)) await fs.remove(destFile); | ||
| await fs.symlink(srcFile, destFile); |
There was a problem hiding this comment.
Using fs.pathExists before fs.remove can fail to clean up broken symlinks because pathExists resolves the symlink target and returns false if the target is missing. This will cause fs.symlink to throw an EEXIST error.
Since fs.remove is idempotent and safely ignores non-existent paths, you can call it directly without the pathExists guard.
| } else if (mode === "symlink") { | |
| if (await fs.pathExists(destFile)) await fs.remove(destFile); | |
| await fs.symlink(srcFile, destFile); | |
| } else if (mode === "symlink") { | |
| await fs.remove(destFile); | |
| await fs.symlink(srcFile, destFile); |
| if (mode === "symlink") { | ||
| if (await fs.pathExists(destination)) await fs.remove(destination); | ||
| await fs.symlink(source, destination); | ||
| return "symlink"; | ||
| } |
There was a problem hiding this comment.
Using fs.pathExists before fs.remove can fail to clean up broken symlinks because pathExists resolves the symlink target and returns false if the target is missing. This will cause fs.symlink to throw an EEXIST error.
Since fs.remove is idempotent and safely ignores non-existent paths, you can call it directly without the pathExists guard.
| if (mode === "symlink") { | |
| if (await fs.pathExists(destination)) await fs.remove(destination); | |
| await fs.symlink(source, destination); | |
| return "symlink"; | |
| } | |
| if (mode === "symlink") { | |
| await fs.remove(destination); | |
| await fs.symlink(source, destination); | |
| return "symlink"; | |
| } |
| userId: game.userId ?? "", | ||
| type: "error", | ||
| title: "Import extraction failed", | ||
| message: `"${game.title}" was moved into your library, but extracting the archive failed: ${err instanceof Error ? err.message : String(err)}. The archive is left at ${archiveInDest} — extract or delete it manually to finish the import.`, |
There was a problem hiding this comment.
The notification message says the game "was moved into your library", but this notification is also triggered when the transfer mode is copy. Using "transferred" instead of "moved" makes the message accurate for both transfer modes.
| message: `"${game.title}" was moved into your library, but extracting the archive failed: ${err instanceof Error ? err.message : String(err)}. The archive is left at ${archiveInDest} — extract or delete it manually to finish the import.`, | |
| message: `"${game.title}" was transferred into your library, but extracting the archive failed: ${err instanceof Error ? err.message : String(err)}. The archive is left at ${archiveInDest} — extract or delete it manually to finish the import.`, |
| let proposedPath = overridePlan.proposedPath; | ||
| if (needsExtraction && !archiveResolution!.isDirectorySource) { | ||
| const ext = path.extname(resolvedOriginalPath); | ||
| if (ext && proposedPath.toLowerCase().endsWith(ext.toLowerCase())) { | ||
| proposedPath = proposedPath.slice(0, -ext.length); | ||
| } | ||
| } |
There was a problem hiding this comment.
If overridePlan.proposedPath contains trailing slashes or is not fully normalized, the .endsWith(ext) check might fail to strip the extension. Resolving the path first ensures it is normalized and trailing slashes are stripped before performing the extension check.
| let proposedPath = overridePlan.proposedPath; | |
| if (needsExtraction && !archiveResolution!.isDirectorySource) { | |
| const ext = path.extname(resolvedOriginalPath); | |
| if (ext && proposedPath.toLowerCase().endsWith(ext.toLowerCase())) { | |
| proposedPath = proposedPath.slice(0, -ext.length); | |
| } | |
| } | |
| let proposedPath = path.resolve(overridePlan.proposedPath); | |
| if (needsExtraction && !archiveResolution!.isDirectorySource) { | |
| const ext = path.extname(resolvedOriginalPath); | |
| if (ext && proposedPath.toLowerCase().endsWith(ext.toLowerCase())) { | |
| proposedPath = proposedPath.slice(0, -ext.length); | |
| } | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@server/services/ImportManager.ts`:
- Around line 693-699: Update the resolvedRoot calculation in the proposed-path
containment check to apply the same config.libraryRoot fallback used by
processImport and planConfirmImport, defaulting to "/data" when empty or unset.
Keep the existing resolvedTarget and insideRoot validation unchanged.
- Around line 239-241: Update the siblingsInDest construction to use
Array.from(resolution.excludePaths) instead of spreading the Set, while
preserving the existing filter and map behavior.
🪄 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: d5322cfd-76fa-41a4-8d97-65c1bb4ba193
📒 Files selected for processing (9)
client/src/components/ImportReviewModal.tsxmemory/decisions.mdmemory/preferences.mdserver/__tests__/archive_service.test.tsserver/__tests__/import_manager.test.tsserver/__tests__/import_manager_additional.test.tsserver/services/ArchiveService.tsserver/services/ImportManager.tsserver/services/ImportStrategies.ts
| siblingsInDest = [...resolution.excludePaths] | ||
| .filter((p) => p !== resolvedArchive) | ||
| .map((p) => path.join(destDir, path.basename(p))); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether the repo's TS target/downlevelIteration permits Set spread.
fd -H -t f 'tsconfig*.json' --exec sh -c 'echo "== {} =="; cat "{}"'Repository: Doezer/Questarr
Length of output: 1116
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== server/services/ImportManager.ts (around lines 220-250) ==\n'
sed -n '220,250p' server/services/ImportManager.ts | cat -n
printf '\n== occurrences of excludePaths ==\n'
rg -n "excludePaths" server shared clientRepository: Doezer/Questarr
Length of output: 3102
Use Array.from here
[...resolution.excludePaths] trips TS2802 under the current tsconfig.json because target and downlevelIteration are unset. Array.from(resolution.excludePaths) avoids the spread-on-Set type error.
Suggested change
- siblingsInDest = [...resolution.excludePaths]
+ siblingsInDest = Array.from(resolution.excludePaths)
.filter((p) => p !== resolvedArchive)
.map((p) => path.join(destDir, path.basename(p)));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| siblingsInDest = [...resolution.excludePaths] | |
| .filter((p) => p !== resolvedArchive) | |
| .map((p) => path.join(destDir, path.basename(p))); | |
| siblingsInDest = Array.from(resolution.excludePaths) | |
| .filter((p) => p !== resolvedArchive) | |
| .map((p) => path.join(destDir, path.basename(p))); |
🤖 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 `@server/services/ImportManager.ts` around lines 239 - 241, Update the
siblingsInDest construction to use Array.from(resolution.excludePaths) instead
of spreading the Set, while preserving the existing filter and map behavior.
Source: Learnings
| const resolvedRoot = path.resolve(config.libraryRoot); | ||
| const resolvedTarget = path.resolve(overridePlan.proposedPath); | ||
| const resolvedTarget = path.resolve(proposedPath); | ||
| const insideRoot = | ||
| resolvedTarget === resolvedRoot || resolvedTarget.startsWith(resolvedRoot + path.sep); | ||
| if (!insideRoot) { | ||
| throw new Error("Proposed path is outside configured library root"); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the same libraryRoot fallback as elsewhere.
path.resolve(config.libraryRoot) omits the || "/data" fallback that both processImport (Line 486) and planConfirmImport (Line 584) apply. If libraryRoot is empty, resolvedRoot collapses to the process CWD, and a client-supplied proposedPath under the effective /data root would fail this containment check and block the import.
🛠️ Suggested change
- const resolvedRoot = path.resolve(config.libraryRoot);
+ const resolvedRoot = path.resolve(config.libraryRoot || "/data");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const resolvedRoot = path.resolve(config.libraryRoot); | |
| const resolvedTarget = path.resolve(overridePlan.proposedPath); | |
| const resolvedTarget = path.resolve(proposedPath); | |
| const insideRoot = | |
| resolvedTarget === resolvedRoot || resolvedTarget.startsWith(resolvedRoot + path.sep); | |
| if (!insideRoot) { | |
| throw new Error("Proposed path is outside configured library root"); | |
| } | |
| const resolvedRoot = path.resolve(config.libraryRoot || "/data"); | |
| const resolvedTarget = path.resolve(proposedPath); | |
| const insideRoot = | |
| resolvedTarget === resolvedRoot || resolvedTarget.startsWith(resolvedRoot + path.sep); | |
| if (!insideRoot) { | |
| throw new Error("Proposed path is outside configured library root"); | |
| } |
🤖 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 `@server/services/ImportManager.ts` around lines 693 - 699, Update the
resolvedRoot calculation in the proposed-path containment check to apply the
same config.libraryRoot fallback used by processImport and planConfirmImport,
defaulting to "/data" when empty or unset. Keep the existing resolvedTarget and
insideRoot validation unchanged.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.github/workflows/deploy.yml:
- Around line 34-37: Add concurrency controls to the deployment workflow
triggered by the main branch, using a group key that identifies the dev
deployment environment so runs targeting the mutable dev tag cannot overlap.
Configure the policy to serialize queued dev deployments rather than allowing
out-of-order completion, while preserving the existing main-branch trigger.
- Line 78: Align the default environment in the workflow’s environment
expression with the deployment target configured around ENVIRONMENT at lines
101-106. For non-main, non-scheduled runs without inputs.environment, use the
same default on both paths, or require inputs.environment instead of falling
back independently, so GitHub protections, secrets, and published artifacts
target one environment.
🪄 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: d9ad0305-a5e2-4389-b3c8-c99149ad913d
📒 Files selected for processing (2)
.github/dependabot.yml.github/workflows/deploy.yml
| # Trigger on main branch for dev release | ||
| push: | ||
| branches: | ||
| - "release/1.4.1" | ||
| - "main" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Serialize deployments targeting the mutable dev tag.
With main now triggering deployments, overlapping runs can finish out of order and leave dev pointing to an older build. Add workflow/job concurrency keyed by deployment environment, choosing whether to serialize or cancel superseded dev runs.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 3-37: insufficient job-level concurrency limits (concurrency-limits): workflow is missing concurrency setting
(concurrency-limits)
🤖 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 @.github/workflows/deploy.yml around lines 34 - 37, Add concurrency controls
to the deployment workflow triggered by the main branch, using a group key that
identifies the dev deployment environment so runs targeting the mutable dev tag
cannot overlap. Configure the policy to serialize queued dev deployments rather
than allowing out-of-order completion, while preserving the existing main-branch
trigger.
Source: Linters/SAST tools
| build-and-push: | ||
| runs-on: ubuntu-latest | ||
| environment: ${{ (github.event_name == 'schedule' || github.ref == 'refs/heads/release/1.4.1') && 'dev' || inputs.environment || 'production' }} | ||
| environment: ${{ (github.event_name == 'schedule' || github.ref == 'refs/heads/main') && 'dev' || inputs.environment || 'production' }} |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files .github/workflows/deploy.yml
echo '---'
cat -n .github/workflows/deploy.yml | sed -n '60,120p'Repository: Doezer/Questarr
Length of output: 3078
Keep the workflow environment and deployment target aligned.
At lines 78 and 101-106, a non-main, non-scheduled run with no inputs.environment uses the GitHub production environment but sets ENVIRONMENT=dev. That can pull production protections/secrets while publishing dev artifacts. Make both defaults match or require inputs.environment.
🤖 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 @.github/workflows/deploy.yml at line 78, Align the default environment in
the workflow’s environment expression with the deployment target configured
around ENVIRONMENT at lines 101-106. For non-main, non-scheduled runs without
inputs.environment, use the same default on both paths, or require
inputs.environment instead of falling back independently, so GitHub protections,
secrets, and published artifacts target one environment.
|
1 participant
Summary
.partN.rarsibling detection for multi-volume RAR archives (stem derivation previously left.part1attached, blocking.part2.rar/.part10.rarfrom matching)ImportReviewModalhints explaining hardlink/symlink volume requirementsKnown limitation (reported, not fixed — explicitly scoped out)
fs-extra's cross-device (EXDEV) move fallback is copy-then-remove, not atomic. A failure mid-copy can leave a partial file at the library destination. Flagged as future work; not addressed in this PR.Test plan
server/__tests__/archive_service.test.ts— 20/20 passingserver/__tests__/import_manager_additional.test.ts— 22/22 passingserver/__tests__/import_manager.test.ts— 28/28 passingnpm run check— clean, no type errorsImportReviewModalhint text in a browser🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
hardlinkandsymlink.Bug Fixes
Chores
mainbranch.