refactor(file): split monolithic download module into focused submodules#26
Conversation
Extract the file download implementation into separate modules under download/ for better testability and maintainability: - errors.ts: download-specific error definitions - file-system.ts: temporary file and finalization logic - input.ts: URL, name, and extension option parsing - plan.ts: download session key creation and plan resolution - progress.ts: download progress reporting - request.ts: HTTP request handling with range support - session.ts: download session persistence - types.ts: shared type definitions Each module has its own dedicated test file. The top-level download.test.ts now covers integration-level scenarios that exercise the full command handler. Signed-off-by: Kevin Cui <bh@bugs.cc>
Summary by CodeRabbit
WalkthroughThe pull request refactors the file download command handler by extracting large in-file implementations into dedicated modules. The monolithic Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes DetailsComplexity factors:
Review focus areas:
🚥 Pre-merge checks | ✅ 2✅ 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 |
![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
🧹 Nitpick comments (2)
src/application/commands/file/download/input.test.ts (1)
9-28: Add direct cases for the other exported input helpers.
src/application/commands/file/download/input.tsalso introducesparseFileDownloadUrl()andensureOutputDirectory(), but this suite only locks down the name/extension validators. A few focused cases for malformed URLs, non-HTTP schemes, existing-file out-dir paths, and mkdir/stat failures would make regressions fail much closer to the source.Also applies to: 30-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/commands/file/download/input.test.ts` around lines 9 - 28, Add focused unit tests for the other exported input helpers in src/application/commands/file/download/input.ts: cover parseFileDownloadUrl() with malformed URL strings and non-HTTP/HTTPS schemes (assert it throws the expected CLI user error/key), and cover ensureOutputDirectory() for cases where the target path is an existing file (not a directory), for when mkdir fails, and when stat fails (simulate errors via stubbing fs methods) to assert the correct error keys are thrown; reference the functions parseFileDownloadUrl and ensureOutputDirectory when adding tests so malformed URLs, non-http schemes, existing-file out-dir paths, and mkdir/stat failure paths are exercised.src/application/commands/file/download/file-system.ts (1)
146-170: Consider fallback for cross-filesystem scenarios.The
link()+unlink()approach provides atomicity on the same filesystem, but will fail withEXDEViftemporaryFilePathanddirectoryPathare on different mount points. Since the temp file is created insession.outDirPathwhich equals the output directory, this should be fine in practice—but if the design ever allows separate temp directories, a fallback torename()or copy-and-delete would be needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/commands/file/download/file-system.ts` around lines 146 - 170, The finalizeDownloadedFile function currently uses link() followed by unlink(), which will throw on cross-filesystem moves (EXDEV); update the catch to detect error.code === 'EXDEV' (or equivalent) and in that case attempt a fallback: try fs.rename(temporaryFilePath, candidateFilePath) first, and if rename fails or is not permitted, fall back to fs.copyFile(temporaryFilePath, candidateFilePath) then fs.unlink(temporaryFilePath); ensure all attempts still return candidateFilePath on success and otherwise throw createDownloadFailedError(candidateFilePath, ...) as before; reference functions and symbols: finalizeDownloadedFile, link, unlink, rename, copyFile, resolveAvailableFileName, createDownloadFailedError.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/application/commands/file/download/request.ts`:
- Around line 42-48: The debug logs are currently emitting full URLs and query
strings (requestUrl, finalUrl, query) which can contain presigned tokens or
user:pass credentials; update the logging in the context.logger.debug calls in
src/application/commands/file/download/request.ts to log only the host and
pathname (use withRequestTarget(requestUrl.host, requestUrl.pathname) as-is) and
replace the full query/url/finalUrl fields with redacted values (e.g., log a
boolean or "REDACTED" for query presence or a masked URL that strips credentials
and query string). Apply the same change to the other context.logger.debug/warn
sites referenced (the blocks around the other occurrences at lines 57-64, 72-80,
91-98) and use requestUrl and finalUrl identifiers so the maintainers can find
and change those exact fields.
- Around line 119-127: The code currently sets a "Range" header regardless of
If-Range validation; change the logic in the download request flow (around
buildRequestHeaders, resolveIfRangeHeader, and headers usage) to refuse/rescind
resume attempts when resolveIfRangeHeader(session) returns undefined while there
is a non-zero localBytes (i.e., session.entityTag and session.lastModified are
both empty). Specifically: do NOT set the "Range" header or issue a partial
request when ifRangeValue is undefined and localBytes > 0 — instead reject the
resume (throw or return an error/flag so the caller restarts a fresh download)
so partial-append corruption cannot occur. Ensure any downstream callers handle
the new rejection path.
---
Nitpick comments:
In `@src/application/commands/file/download/file-system.ts`:
- Around line 146-170: The finalizeDownloadedFile function currently uses link()
followed by unlink(), which will throw on cross-filesystem moves (EXDEV); update
the catch to detect error.code === 'EXDEV' (or equivalent) and in that case
attempt a fallback: try fs.rename(temporaryFilePath, candidateFilePath) first,
and if rename fails or is not permitted, fall back to
fs.copyFile(temporaryFilePath, candidateFilePath) then
fs.unlink(temporaryFilePath); ensure all attempts still return candidateFilePath
on success and otherwise throw createDownloadFailedError(candidateFilePath, ...)
as before; reference functions and symbols: finalizeDownloadedFile, link,
unlink, rename, copyFile, resolveAvailableFileName, createDownloadFailedError.
In `@src/application/commands/file/download/input.test.ts`:
- Around line 9-28: Add focused unit tests for the other exported input helpers
in src/application/commands/file/download/input.ts: cover parseFileDownloadUrl()
with malformed URL strings and non-HTTP/HTTPS schemes (assert it throws the
expected CLI user error/key), and cover ensureOutputDirectory() for cases where
the target path is an existing file (not a directory), for when mkdir fails, and
when stat fails (simulate errors via stubbing fs methods) to assert the correct
error keys are thrown; reference the functions parseFileDownloadUrl and
ensureOutputDirectory when adding tests so malformed URLs, non-http schemes,
existing-file out-dir paths, and mkdir/stat failure paths are exercised.
🪄 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: CHILL
Plan: Pro
Run ID: a9bc81ab-1a05-4cd3-a4b8-06a50f8249b7
📒 Files selected for processing (16)
src/application/commands/file/download.test.tssrc/application/commands/file/download.tssrc/application/commands/file/download/__tests__/helpers.tssrc/application/commands/file/download/errors.tssrc/application/commands/file/download/file-system.test.tssrc/application/commands/file/download/file-system.tssrc/application/commands/file/download/input.test.tssrc/application/commands/file/download/input.tssrc/application/commands/file/download/plan.tssrc/application/commands/file/download/progress.test.tssrc/application/commands/file/download/progress.tssrc/application/commands/file/download/request.test.tssrc/application/commands/file/download/request.tssrc/application/commands/file/download/session.test.tssrc/application/commands/file/download/session.tssrc/application/commands/file/download/types.ts
| context.logger.debug( | ||
| { | ||
| method: "GET", | ||
| ...withRequestTarget(requestUrl.host, requestUrl.pathname), | ||
| query: requestUrl.searchParams.toString(), | ||
| url: requestUrl.toString(), | ||
| }, |
There was a problem hiding this comment.
Redact full URLs and query strings before logging.
query, url, and finalUrl will record pre-signed tokens or user:pass@host credentials whenever users download from signed/private endpoints. That leaks sensitive data into routine debug/warn logs; keep the host/path fields and log only redacted URL details or query presence instead.
Also applies to: 57-64, 72-80, 91-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/application/commands/file/download/request.ts` around lines 42 - 48, The
debug logs are currently emitting full URLs and query strings (requestUrl,
finalUrl, query) which can contain presigned tokens or user:pass credentials;
update the logging in the context.logger.debug calls in
src/application/commands/file/download/request.ts to log only the host and
pathname (use withRequestTarget(requestUrl.host, requestUrl.pathname) as-is) and
replace the full query/url/finalUrl fields with redacted values (e.g., log a
boolean or "REDACTED" for query presence or a masked URL that strips credentials
and query string). Apply the same change to the other context.logger.debug/warn
sites referenced (the blocks around the other occurrences at lines 57-64, 72-80,
91-98) and use requestUrl and finalUrl identifiers so the maintainers can find
and change those exact fields.
| const headers = buildRequestHeaders(); | ||
|
|
||
| headers.set("Range", `bytes=${localBytes}-`); | ||
|
|
||
| const ifRangeValue = resolveIfRangeHeader(session); | ||
|
|
||
| if (ifRangeValue !== undefined) { | ||
| headers.set("If-Range", ifRangeValue); | ||
| } |
There was a problem hiding this comment.
Do not issue resume requests without an If-Range validator.
When both session.entityTag and session.lastModified are empty, this still sends a bare Range request. A changed upstream object can then return 206 for a different representation, and the later append will silently corrupt the partial download. Reject resume here, or make the caller restart fresh instead.
Also applies to: 132-146
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/application/commands/file/download/request.ts` around lines 119 - 127,
The code currently sets a "Range" header regardless of If-Range validation;
change the logic in the download request flow (around buildRequestHeaders,
resolveIfRangeHeader, and headers usage) to refuse/rescind resume attempts when
resolveIfRangeHeader(session) returns undefined while there is a non-zero
localBytes (i.e., session.entityTag and session.lastModified are both empty).
Specifically: do NOT set the "Range" header or issue a partial request when
ifRangeValue is undefined and localBytes > 0 — instead reject the resume (throw
or return an error/flag so the caller restarts a fresh download) so
partial-append corruption cannot occur. Ensure any downstream callers handle the
new rejection path.
1 participant
Extract the file download implementation into separate modules under download/ for better testability and maintainability:
Each module has its own dedicated test file. The top-level download.test.ts now covers integration-level scenarios that exercise the full command handler.