Feat/upload ios#35
Conversation
…NitroFSFileDownloader
… multipart support
…pes and simplify downloadFile method signature
|
""" WalkthroughThe changes update the file download and upload flows, error handling, and platform support. The downloadFile API is simplified by removing the filename parameter, and error enums are consolidated for consistency. The iOS uploader is refactored to stream multipart uploads from disk, and platform support for visionOS is removed from the podspec. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant NitroFS (JS/TS)
participant HybridNitroFS (Swift)
participant NitroFSImpl
participant NitroFSFileDownloader
App->>NitroFS: downloadFile(serverUrl, destinationPath, onProgress)
NitroFS->>HybridNitroFS: downloadFile(serverUrl, destinationPath, onProgress)
HybridNitroFS->>NitroFSImpl: downloadFile(serverUrl, destinationPath, onProgress)
NitroFSImpl->>NitroFSFileDownloader: downloadFile(serverUrl, destinationPath, onProgress)
NitroFSFileDownloader-->>NitroFSImpl: NitroFile (or error)
NitroFSImpl-->>HybridNitroFS: NitroFile (or error)
HybridNitroFS-->>NitroFS: NitroFile (or error)
NitroFS-->>App: NitroFile (or error)
sequenceDiagram
participant App
participant NitroFS (JS/TS)
participant HybridNitroFS (Swift)
participant NitroFSImpl
participant NitroFSFileUploader
App->>NitroFS: uploadFile(file, options, onProgress)
NitroFS->>HybridNitroFS: uploadFile(file, options, onProgress)
HybridNitroFS->>NitroFSImpl: uploadFile(file, options, onProgress)
NitroFSImpl->>NitroFSFileUploader: uploadFile(file, options, onProgress)
NitroFSFileUploader->>NitroFSFileUploader: createMultipartBodyFile(file, ...)
NitroFSFileUploader->>Server: Upload multipart file
NitroFSFileUploader-->>NitroFSImpl: Success or error
NitroFSImpl-->>HybridNitroFS: Success or error
HybridNitroFS-->>NitroFS: Success or error
NitroFS-->>App: Success or error
Possibly related PRs
Suggested labels
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
ios/NitroFSImpl.swift (1)
115-116: Misleading error message fordownloadFileunavailabilityThe thrown message references “stat file”, which is unrelated to the
downloadFileoperation and will confuse consumers troubleshooting download failures.- throw NitroFSError.unavailable(message: "Failed to stat file. fileDownloader is unavailable") + throw NitroFSError.unavailable(message: "Failed to download file. fileDownloader is unavailable")ios/Utils.swift (1)
24-28: Consider surfacinglocalizedDescriptionfor unified error reportingEach case already carries a message. Conforming
NitroFSErrortoLocalizedError(orCustomStringConvertible) would let callers accesserror.localizedDescriptioninstead of manually switching on the enum.extension NitroFSError: LocalizedError { var errorDescription: String? { switch self { case let .unavailable(msg), .fileError(msg), .networkError(msg), .encodingError(msg): return msg } } }ios/NitroFSFileDownloader.swift (2)
56-65: Validate & percent-encode the URL
URL(string:)assumes the input is already percent-encoded. IfserverUrlcomes from user input it may contain spaces or other illegal characters and the request will fail at runtime.Consider:
guard let encoded = serverUrl.addingPercentEncoding(withAllowedCharacters: .urlQueryAllowed), let url = URL(string: encoded) else { throw URLError(.badURL) }
73-82: Differentiate network vs server-side errorsThe same
networkErrorcase is used for both transport failures and non-2xx HTTP codes. Distinguishing them (transportErrorvshttpError(code:)) can simplify client-side handling (e.g., retry policy vs user-visible error).ios/NitroFSFileUploader.swift (2)
135-140: SwiftLint: replace unused closure parameters with “_”
session,task, andbytesSentare unused, triggering the SwiftLint warning reported by static analysis.- func urlSession(_ session: URLSession, task: URLSessionTask, didSendBodyData bytesSent: Int64, - totalBytesSent: Int64, totalBytesExpectedToSend: Int64) { + func urlSession(_ _: URLSession, task _: URLSessionTask, didSendBodyData _: Int64, + totalBytesSent: Int64, totalBytesExpectedToSend: Int64) {
72-104:createMultipartBodyFilemarkedasyncbut performs no asynchronous workRemoving
asyncsimplifies call-sites and avoids unnecessary suspension points.- private func createMultipartBodyFile(... ) async throws -> URL { + private func createMultipartBodyFile(... ) throws -> URL { - let multipartFile = try await createMultipartBodyFile( + let multipartFile = try createMultipartBodyFile(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
example/ios/Podfile.lockis excluded by!**/*.locknitrogen/generated/android/c++/JHybridNitroFSSpec.cppis excluded by!**/generated/**nitrogen/generated/android/c++/JHybridNitroFSSpec.hppis excluded by!**/generated/**nitrogen/generated/android/kotlin/com/margelo/nitro/nitrofs/HybridNitroFSSpec.ktis excluded by!**/generated/**nitrogen/generated/ios/c++/HybridNitroFSSpecSwift.hppis excluded by!**/generated/**nitrogen/generated/ios/swift/HybridNitroFSSpec.swiftis excluded by!**/generated/**nitrogen/generated/ios/swift/HybridNitroFSSpec_cxx.swiftis excluded by!**/generated/**nitrogen/generated/shared/c++/HybridNitroFSSpec.hppis excluded by!**/generated/**
📒 Files selected for processing (8)
NitroFS.podspec(1 hunks)example/App.tsx(2 hunks)ios/HybridNitroFs.swift(6 hunks)ios/NitroFSFileDownloader.swift(3 hunks)ios/NitroFSFileUploader.swift(1 hunks)ios/NitroFSImpl.swift(4 hunks)ios/Utils.swift(1 hunks)src/specs/nitro-fs.nitro.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
ios/NitroFSFileDownloader.swift (2)
ios/NitroFSImpl.swift (1)
downloadFile(109-122)ios/HybridNitroFs.swift (1)
downloadFile(126-139)
🪛 SwiftLint (0.57.0)
ios/NitroFSFileUploader.swift
[Warning] 48-48: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build Android Example App (new architecture)
- GitHub Check: Build iOS Example App (old architecture)
- GitHub Check: Build iOS Example App (new architecture)
🔇 Additional comments (8)
NitroFS.podspec (1)
13-13: Verify the intentional removal of visionOS support.This change removes visionOS platform support, which is a breaking change for any existing users targeting visionOS. Please ensure this removal is intentional and consider documenting this breaking change in release notes.
example/App.tsx (2)
12-14: LGTM! URL structure updated to include filename.The download URLs now properly include the filename as part of the path, which aligns with the simplified
downloadFileAPI that no longer requires a separate filename parameter.
122-122: LGTM! Minor formatting improvement.Adding the space before the
urlkey improves code readability and maintains consistency.src/specs/nitro-fs.nitro.ts (2)
118-118: LGTM! Documentation updated to reflect new API.The example code correctly demonstrates the simplified
downloadFilemethod without the separate filename parameter.
124-124: LGTM! Method signature simplified effectively.Removing the
fileNameparameter simplifies the API by embedding the filename directly in theserverUrl. This reduces complexity and makes the interface more intuitive.ios/HybridNitroFs.swift (3)
33-33: LGTM! Error handling consolidated for consistency.The consolidation of specific error types into more general
NitroFSError.fileErrorimproves consistency across the codebase, despite the message issues noted separately.Also applies to: 44-44, 55-55, 66-66
102-102: LGTM! Appropriate error type for stat operation.Using
NitroFSError.encodingErrorfor stat operation failures is more semantically appropriate than the previous error type.
126-126: LGTM! Method signature updated to match TypeScript interface.The
downloadFilemethod signature correctly removes thefileNameparameter, aligning with the simplified API design across the codebase.
| } catch { | ||
| os_log("Failed to read file content: \(error.localizedDescription)") | ||
| throw NitroFSError.nitroFSWriteFailed(message: "Failed to write file content to disk") | ||
| throw NitroFSError.fileError(message: "Failed to write file content to disk") |
There was a problem hiding this comment.
Fix incorrect error message in readFile method.
The error message says "write" but this is in the readFile method, so it should say "read".
- throw NitroFSError.fileError(message: "Failed to write file content to disk")
+ throw NitroFSError.fileError(message: "Failed to read file content from disk")📝 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.
| throw NitroFSError.fileError(message: "Failed to write file content to disk") | |
| throw NitroFSError.fileError(message: "Failed to read file content from disk") |
🤖 Prompt for AI Agents
In ios/HybridNitroFs.swift at line 44, the error message in the readFile method
incorrectly states "Failed to write file content to disk." Change the message to
"Failed to read file content from disk" to accurately reflect the read operation
failure.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
android/src/main/java/com/nitrofs/NitroFSImpl.kt (1)
93-108: API refactoring looks good with a minor improvement suggestion.The method signature changes align well with the cross-platform API simplification. The null checking logic is appropriate, and the return type change improves the API consistency.
Consider using a more specific exception type for better error handling:
- } else { - throw Error("Error downloading file") - } + } else { + throw RuntimeException("Failed to download file from: $serverUrl") + }android/src/main/java/com/nitrofs/FileDownloader.kt (1)
43-54: Excellent content-Type extraction and NitroFile construction.The implementation correctly extracts the MIME type from the HTTP response with a sensible fallback, and constructs the NitroFile object with all required fields from the TypeScript interface.
Consider adding HTTP status code validation to handle error responses:
}.execute { response -> + if (!response.status.isSuccess()) { + throw RuntimeException("HTTP ${response.status.value}: Failed to download file") + } contentType = response.headers["Content-Type"] ?: "application/octet-stream" val channel: ByteReadChannel = response.body() channel.copyAndClose(outputFile.writeChannel()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
android/src/main/java/com/nitrofs/FileDownloader.kt(3 hunks)android/src/main/java/com/nitrofs/HybridNitroFS.kt(1 hunks)android/src/main/java/com/nitrofs/NitroFSImpl.kt(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
android/src/main/java/com/nitrofs/FileDownloader.kt (1)
src/type.ts (1)
NitroFile(36-40)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build iOS Example App (old architecture)
- GitHub Check: Build iOS Example App (new architecture)
- GitHub Check: Build Android Example App (new architecture)
- GitHub Check: Build Android Example App (old architecture)
🔇 Additional comments (4)
android/src/main/java/com/nitrofs/FileDownloader.kt (3)
3-3: Good addition of NitroFile import.The import is correctly added to support the new return type.
18-26: Method signature update aligns with API simplification.The removal of the fileName parameter and addition of NitroFile? return type correctly implements the new API design.
31-31: Correct use of full serverUrl.Using the full serverUrl directly in the HTTP request is the right approach for the simplified API.
android/src/main/java/com/nitrofs/HybridNitroFS.kt (1)
135-135: Correct API signature update.The method call correctly matches the updated
nitroFsImpl.downloadFilesignature by removing the fileName parameter while preserving all other functionality.
…pdate file upload methods in NitroFSFileUploader for better error management
…tailed runtime exceptions
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
ios/Utils.swift (1)
43-67: Partial writes not handled correctly - same issue as previous review.The
generateLargeFilefunction has the same issue identified in previous reviews:OutputStream.writecan legitimately return fewer bytes than requested without signifying an error, but the current implementation treats partial writes as complete success, potentially resulting in incomplete file generation.The fix remains the same - implement a retry loop for partial writes:
- let written = outputStream.write(buffer, maxLength: toWrite) - if written <= 0 { - throw NSError(domain: "FileError", code: 2, userInfo: [NSLocalizedDescriptionKey: "Failed to write data"]) - } - bytesWritten += written + var totalWritten = 0 + while totalWritten < toWrite { + let written = outputStream.write(buffer.advanced(by: totalWritten), + maxLength: toWrite - totalWritten) + if written <= 0 { + throw NSError(domain: "FileError", + code: 2, + userInfo: [NSLocalizedDescriptionKey: "Failed to write data"]) + } + totalWritten += written + } + bytesWritten += totalWrittenios/NitroFSFileUploader.swift (1)
45-66: Session leakage and retain cycle risk - same issue as previous review.The issue identified in previous reviews persists: the
URLSessionretains its delegate (self), and without proper session invalidation, resources may leak. The[weak self]capture helps but doesn't fully resolve the session retention issue.Add session invalidation:
let task = session.uploadTask(with: request, fromFile: multipartFile) { data, response, error in try? FileManager.default.removeItem(at: multipartFile) + session.finishTasksAndInvalidate() if let error = error { + session.invalidateAndCancel() continuation.resume(throwing: error) return }🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 48-48: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
🧹 Nitpick comments (1)
ios/NitroFSFileUploader.swift (1)
48-48: Remove unused parameter to address static analysis warning.The
dataparameter in the upload task completion handler is not used.Apply this diff to fix the warning:
-let task = session.uploadTask(with: request, fromFile: multipartFile) { data, response, error in +let task = session.uploadTask(with: request, fromFile: multipartFile) { _, response, error in🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 48-48: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
android/src/main/java/com/nitrofs/FileDownloader.kt(3 hunks)android/src/main/java/com/nitrofs/NitroFSImpl.kt(1 hunks)ios/HybridNitroFs.swift(6 hunks)ios/NitroFSFileDownloader.swift(3 hunks)ios/NitroFSFileUploader.swift(1 hunks)ios/NitroFSImpl.swift(4 hunks)ios/Utils.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- android/src/main/java/com/nitrofs/NitroFSImpl.kt
- ios/HybridNitroFs.swift
- ios/NitroFSFileDownloader.swift
- android/src/main/java/com/nitrofs/FileDownloader.kt
- ios/NitroFSImpl.swift
🧰 Additional context used
🪛 SwiftLint (0.57.0)
ios/NitroFSFileUploader.swift
[Warning] 48-48: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build iOS Example App (new architecture)
- GitHub Check: Build iOS Example App (old architecture)
- GitHub Check: Build Android Example App (new architecture)
- GitHub Check: Build Android Example App (old architecture)
🔇 Additional comments (3)
ios/Utils.swift (1)
23-40: LGTM! Well-designed error consolidation.The refactoring of
NitroFSErrorwith associated values provides better error messaging flexibility, and theLocalizedErrorconformance enhances user experience with proper error descriptions.ios/NitroFSFileUploader.swift (2)
73-104: Well-implemented multipart body creation.The
createMultipartBodyFilemethod properly handles resource management with defer statements and creates correctly formatted multipart data with CRLF line endings.
145-169: Excellent partial write handling in OutputStream extension.The
OutputStream.write(_:String)extension properly handles partial writes with a retry loop, which is the correct approach for robust stream writing.
| if bytesRead > 0 { | ||
| let bytesWritten = output.write(buffer, maxLength: bytesRead) | ||
| if bytesWritten != bytesRead { | ||
| throw NitroFSError.encodingError(message: "Failed to write file data to output stream") | ||
| } |
There was a problem hiding this comment.
Handle partial writes correctly in file streaming.
Similar to the issue in generateLargeFile, the comparison bytesWritten != bytesRead doesn't handle partial writes correctly. OutputStream.write can legitimately return fewer bytes than requested.
Apply this fix to handle partial writes:
- if bytesRead > 0 {
- let bytesWritten = output.write(buffer, maxLength: bytesRead)
- if bytesWritten != bytesRead {
- throw NitroFSError.encodingError(message: "Failed to write file data to output stream")
- }
- }
+ if bytesRead > 0 {
+ var totalWritten = 0
+ while totalWritten < bytesRead {
+ let written = output.write(buffer.advanced(by: totalWritten), maxLength: bytesRead - totalWritten)
+ if written <= 0 {
+ throw NitroFSError.encodingError(message: "Failed to write file data to output stream")
+ }
+ totalWritten += written
+ }
+ }📝 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.
| if bytesRead > 0 { | |
| let bytesWritten = output.write(buffer, maxLength: bytesRead) | |
| if bytesWritten != bytesRead { | |
| throw NitroFSError.encodingError(message: "Failed to write file data to output stream") | |
| } | |
| if bytesRead > 0 { | |
| var totalWritten = 0 | |
| while totalWritten < bytesRead { | |
| let written = output.write(buffer.advanced(by: totalWritten), maxLength: bytesRead - totalWritten) | |
| if written <= 0 { | |
| throw NitroFSError.encodingError(message: "Failed to write file data to output stream") | |
| } | |
| totalWritten += written | |
| } | |
| } |
🤖 Prompt for AI Agents
In ios/NitroFSFileUploader.swift around lines 120 to 124, the code incorrectly
treats any partial write to the output stream as an error by comparing
bytesWritten directly to bytesRead. To fix this, implement a loop that continues
writing the remaining bytes until all bytesRead are written or an error occurs,
properly handling partial writes by adjusting the buffer pointer and remaining
length accordingly.
…itroFSFileUploader after error handling
…troFSFileUploader for cleaner code
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
ios/NitroFSFileUploader.swift (1)
123-127:⚠️ Potential issueCritical: Partial write handling still not addressed
This is the same issue flagged in previous reviews. The comparison
bytesWritten != bytesReaddoesn't handle partial writes correctly, asOutputStream.writecan legitimately write fewer bytes than requested.Apply this fix to handle partial writes correctly:
- if bytesRead > 0 { - let bytesWritten = output.write(buffer, maxLength: bytesRead) - if bytesWritten != bytesRead { - throw NitroFSError.encodingError(message: "Failed to write file data to output stream") - } - } + if bytesRead > 0 { + var totalWritten = 0 + while totalWritten < bytesRead { + let written = output.write(buffer.advanced(by: totalWritten), maxLength: bytesRead - totalWritten) + if written <= 0 { + throw NitroFSError.encodingError(message: "Failed to write file data to output stream") + } + totalWritten += written + } + }
🧹 Nitpick comments (1)
ios/NitroFSFileUploader.swift (1)
19-70: Address unused parameter warning and improve code clarityThe implementation looks solid overall with proper async/await handling, session management, and cleanup. However, there's an unused parameter in the completion closure.
Apply this diff to address the SwiftLint warning:
- let task = session.uploadTask(with: request, fromFile: multipartFile) { data, response, error in + let task = session.uploadTask(with: request, fromFile: multipartFile) { _, response, error in🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 48-48: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ios/NitroFSFileDownloader.swift(5 hunks)ios/NitroFSFileUploader.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ios/NitroFSFileDownloader.swift
🧰 Additional context used
🪛 SwiftLint (0.57.0)
ios/NitroFSFileUploader.swift
[Warning] 48-48: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
🔇 Additional comments (4)
ios/NitroFSFileUploader.swift (4)
10-10: LGTM: Protocol conformance updateGood change to implement
URLSessionDataDelegateto support upload progress reporting.
76-107: LGTM: Multipart body creationThe multipart/form-data format implementation is correct with proper headers, CRLF line endings, and boundary formatting. The temporary file approach efficiently handles large uploads without loading everything into memory.
138-143: LGTM: Progress reporting implementationThe progress callback properly dispatches to the main queue and provides both current and total bytes for upload progress tracking.
148-172: LGTM: Robust OutputStream extensionExcellent implementation that properly handles partial writes for string data. The use of
withUnsafeBytesand the loop to handle remaining bytes ensures all data is written correctly.
|
🎉 This PR is included in version 0.5.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation