refactor: update download directory to include app subfolder in deskt…#298
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughUpdates adjust an Android constructor type reference, refactor the JVM desktop downloader to use a streaming callbackFlow with progress reporting and robust cancellation/error handling, and change desktop download paths to use a "GitHub Store Downloads" subdirectory across platforms. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DesktopDownloader
participant HTTP as "HTTP Client"
participant FS as "Filesystem"
participant Progress as "ProgressEmitter"
Client->>DesktopDownloader: request download(URL)
DesktopDownloader->>HTTP: open request stream
HTTP-->>DesktopDownloader: response stream + status
DesktopDownloader->>FS: open FileOutputStream (IO)
loop read chunks
DesktopDownloader->>HTTP: read bytes
HTTP-->>DesktopDownloader: bytes chunk
DesktopDownloader->>FS: write chunk
DesktopDownloader->>Progress: update bytes/count
Progress-->>Client: trySend(progress)
end
DesktopDownloader->>FS: close file
DesktopDownloader-->>Client: emit completion (file path)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 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. ✨ 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/data/src/jvmMain/kotlin/zed/rainxch/core/data/services/DesktopDownloader.kt`:
- Around line 41-44: The suggestedFileName is used raw to build File objects
(safeName -> outFile) in DesktopDownloader, allowing path traversal or absolute
paths; sanitize by extracting only the base filename (remove any directory
components like "../" or "\" and reject/strip drive letters or leading '/' or
'\' characters), disallow null/blank after stripping, and fall back to the
existing UUID-based name; then use that sanitized filename to construct
File(dir, safeName). Apply the same sanitization logic to the other occurrence
around the code handling lines 135-137 so both file creations use a safe,
basename-only filename.
- Around line 77-79: The write to the FileChannel
(fc.write(ByteBuffer.wrap(...))) may be partial, but the code treats bytesRead
as fully written and calls downloaded.addAndGet(bytesRead.toLong()); change this
by looping until the ByteBuffer has no remaining bytes (use while
(byteBuffer.hasRemaining()) { val written = fc.write(byteBuffer); /* handle 0 or
negative? */ downloaded.addAndGet(written.toLong()) }) so that you only
increment downloaded by the actual bytes written; update the logic around
byteBuffer, bytesRead, fc.write and downloaded.addAndGet to reflect the actual
written count.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fd8c1541-7b11-49c9-83d0-0793d5adc2b8
📒 Files selected for processing (3)
core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/AndroidDownloader.ktcore/data/src/jvmMain/kotlin/zed/rainxch/core/data/services/DesktopDownloader.ktcore/data/src/jvmMain/kotlin/zed/rainxch/core/data/services/DesktopFileLocationsProvider.kt
| val byteBuffer = ByteBuffer.wrap(buffer, 0, bytesRead) | ||
| fc.write(byteBuffer) | ||
| downloaded.addAndGet(bytesRead.toLong()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the DesktopDownloader.kt file to see the actual code around lines 77-79
cat -n core/data/src/jvmMain/kotlin/zed/rainxch/core/data/services/DesktopDownloader.kt | head -100 | tail -40Repository: OpenHub-Store/GitHub-Store
Length of output: 2139
🏁 Script executed:
# Also get more context - check the full method to understand the usage
wc -l core/data/src/jvmMain/kotlin/zed/rainxch/core/data/services/DesktopDownloader.ktRepository: OpenHub-Store/GitHub-Store
Length of output: 154
🏁 Script executed:
# Search for FileChannel write usage patterns in the file
rg -nP -C6 '\bfc\.write\s*\(' core/data/src/jvmMain/kotlin/zed/rainxch/core/data/services/DesktopDownloader.ktRepository: OpenHub-Store/GitHub-Store
Length of output: 847
🌐 Web query:
FileChannel.write(ByteBuffer) partial writes Java documentation
💡 Result:
FileChannel.write(ByteBuffer) follows the contract of WritableByteChannel.write(ByteBuffer).
From the WritableByteChannel.write(ByteBuffer) Javadoc:
- It attempts to write up to r = src.remaining() bytes. The actual number written is n, where 0 ≤ n ≤ r, and on return the buffer position is advanced by n. The method returns “the number of bytes written, possibly zero.” [1]
- “Unless otherwise specified, a write operation will return only after writing all of the r requested bytes.” It also notes that some channel types (e.g., non-blocking socket channels) may write only some or none of the bytes. [1]
From the FileChannel Javadoc for the relative write:
- Bytes are written at the channel’s current file position (or end-of-file in append mode), the file grows if needed, and the file position is updated with “the number of bytes actually written.”
- “Otherwise this method behaves exactly as specified by the
WritableByteChannelinterface.” [2]
Practical implication: even though blocking FileChannel writes are generally expected to complete the whole buffer per the “unless otherwise specified” rule, the API still allows partial writes (since n can be < r and the return value can be 0). So, if you need to be correct for all implementations/platforms, write in a loop:
while (buf.hasRemaining()) fc.write(buf);Sources:
[1] WritableByteChannel.write(ByteBuffer) Javadoc (Oracle Java SE 11) (docs.oracle.com)
[2] FileChannel.write(ByteBuffer) Javadoc (Oracle Java SE 22) (docs.oracle.com)
Citations:
- 1: https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/nio/channels/WritableByteChannel.html
- 2: https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/nio/channels/FileChannel.html
Handle partial FileChannel.write results to prevent data truncation and progress misreporting.
FileChannel.write(ByteBuffer) can write fewer bytes than requested. Although Java documentation states that blocking FileChannel writes generally complete fully, the API contract explicitly permits partial writes. The current code at lines 77-79 doesn't handle this case: it increments the downloaded progress counter by the full bytesRead amount even if fc.write() returns fewer bytes, causing inaccurate progress reporting and risking silent data truncation.
Use a while (byteBuffer.hasRemaining()) loop to ensure all bytes are written:
🛠️ Proposed fix
- val byteBuffer = ByteBuffer.wrap(buffer, 0, bytesRead)
- fc.write(byteBuffer)
+ val byteBuffer = ByteBuffer.wrap(buffer, 0, bytesRead)
+ while (byteBuffer.hasRemaining()) {
+ fc.write(byteBuffer)
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@core/data/src/jvmMain/kotlin/zed/rainxch/core/data/services/DesktopDownloader.kt`
around lines 77 - 79, The write to the FileChannel
(fc.write(ByteBuffer.wrap(...))) may be partial, but the code treats bytesRead
as fully written and calls downloaded.addAndGet(bytesRead.toLong()); change this
by looping until the ByteBuffer has no remaining bytes (use while
(byteBuffer.hasRemaining()) { val written = fc.write(byteBuffer); /* handle 0 or
negative? */ downloaded.addAndGet(written.toLong()) }) so that you only
increment downloaded by the actual bytes written; update the logic around
byteBuffer, bytesRead, fc.write and downloaded.addAndGet to reflect the actual
written count.
1 participant
…op provider
Summary by CodeRabbit