Feat/download file#30
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes introduce and refine file upload and download capabilities across Android and iOS for the NitroFS module, including new Kotlin and Swift implementations. The README is expanded with comprehensive usage and API documentation. Exports are standardized, configuration files updated, and server handlers improved for correctness and metadata. Minor code cleanups and interface adjustments are also present. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant NitroFS (JS)
participant NativeModule (Android/iOS)
participant FileUploader/Downloader
participant Server
App->>NitroFS (JS): call downloadFile(serverUrl, fileName, destPath, onProgress)
NitroFS (JS)->>NativeModule: downloadFile(...)
NativeModule->>FileDownloader/Downloader: downloadFile(...)
FileDownloader/Downloader->>Server: HTTP GET /download
Server-->>FileDownloader/Downloader: File stream + Content-Length
FileDownloader/Downloader-->>NativeModule: Save file, report progress
NativeModule-->>NitroFS (JS): Return NitroFile
NitroFS (JS)-->>App: Resolve promise with file info
sequenceDiagram
participant App
participant NitroFS (JS)
participant NativeModule (Android/iOS)
participant FileUploader
participant Server
App->>NitroFS (JS): call uploadFile(file, options, onProgress)
NitroFS (JS)->>NativeModule: uploadFile(...)
NativeModule->>FileUploader: uploadFile(...)
FileUploader->>Server: HTTP POST /upload (multipart/form-data)
Server-->>FileUploader: HTTP 200 OK
FileUploader-->>NativeModule: Report progress
NativeModule-->>NitroFS (JS): Resolve promise
NitroFS (JS)-->>App: Notify upload complete
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ 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: 3
🧹 Nitpick comments (7)
ios/HybridNitroFs.swift (1)
136-136: Fix misleading log message.The log message mentions "failed to upload file" but this is in the download method.
- os_log("failed to upload file: \(error.localizedDescription)") + os_log("failed to download file: \(error.localizedDescription)")android/src/main/java/com/nitrofs/HybridNitroFS.kt (1)
131-143: Implementation looks good!The implementation of the
downloadFilemethod correctly follows the established pattern in this class:
- Using
Promise.asyncfor asynchronous execution- Proper error handling with logging and error propagation
- Returning the appropriate
NitroFileobject with file detailsOne potential enhancement would be to determine the actual MIME type of the downloaded file instead of returning an empty string.
Consider adding MIME type detection for the downloaded file:
nitroFsImpl.downloadFile(serverUrl, fileName, destinationPath, onProgress) +// Determine MIME type based on file extension or content +val mimeType = getMimeType(fileName) NitroFile( name = fileName, path = destinationPath, - mimeType = "", + mimeType = mimeType, )server/main.go (1)
99-99: Memory limit increased for multipart form parsing.Increasing the memory limit from 1GB to 2GB (2 << 30) allows for handling larger file uploads.
However, the comment still incorrectly states "1GB max memory" while the actual value is 2GB.
-err := r.ParseMultipartForm(2 << 30) // 1GB max memory +err := r.ParseMultipartForm(2 << 30) // 2GB max memoryREADME.md (1)
60-66: Fix formatting in constants listThere appears to be inconsistent formatting in the list of constants. The list items are missing line breaks or proper spacing.
-The module provides several directory constants: - `BUNDLE_DIR`: Directory for storing bundle files +The module provides several directory constants: + +- `BUNDLE_DIR`: Directory for storing bundle files🧰 Tools
🪛 LanguageTool
[uncategorized] ~62-~62: Loose punctuation mark.
Context: ...ral directory constants: -BUNDLE_DIR: Directory for storing bundle files - `D...(UNLIKELY_OPENING_PUNCTUATION)
ios/NitroFSImpl.swift (1)
115-117: Fix error message in downloadFile methodThe error message refers to "stat file" which appears to be copied from another method and should be updated to reference downloading.
- throw NitroFSError.nitroFSUnavailable(message: "Failed to stat file. fileDownloader is unavailable") + throw NitroFSError.nitroFSUnavailable(message: "Failed to download file. fileDownloader is unavailable")ios/NitroFSFileUploader.swift (2)
29-30:fileURLandfieldNameare computed but never usedSwift will warn about both of these being unused once the TODO is resolved. Either remove them or integrate them into the real upload logic (see previous suggestion).
40-44: Progress callback could expose a percentageYou currently pass raw byte counts:
self.onProgress?(Double(totalBytesSent), Double(totalBytesExpectedToSend))Most callers care about “0.0…1.0”. Consider adding a computed percentage (or switching the signature) to avoid repeated boilerplate on every consumer side.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (9)
example/ios/Podfile.lockis excluded by!**/*.locknitrogen/generated/android/c++/JHybridNitroFSSpec.cppis excluded by!**/generated/**nitrogen/generated/android/c++/JNitroUploadOptions.hppis excluded by!**/generated/**nitrogen/generated/android/kotlin/com/margelo/nitro/nitrofs/NitroUploadOptions.ktis excluded by!**/generated/**nitrogen/generated/ios/NitroFS-Swift-Cxx-Bridge.hppis excluded by!**/generated/**nitrogen/generated/ios/NitroFS-Swift-Cxx-Umbrella.hppis excluded by!**/generated/**nitrogen/generated/ios/c++/HybridNitroFSSpecSwift.hppis excluded by!**/generated/**nitrogen/generated/ios/swift/NitroUploadOptions.swiftis excluded by!**/generated/**nitrogen/generated/shared/c++/NitroUploadOptions.hppis excluded by!**/generated/**
📒 Files selected for processing (16)
.github/workflows/release.yml(1 hunks)NitroFs.podspec(1 hunks)README.md(2 hunks)android/src/main/java/com/nitrofs/FileDownloader.kt(1 hunks)android/src/main/java/com/nitrofs/HybridNitroFS.kt(1 hunks)android/src/main/java/com/nitrofs/NitroFSImpl.kt(2 hunks)android/src/main/java/com/nitrofs/NitroFileUploader.kt(1 hunks)android/src/main/java/com/nitrofs/NitroHttpClient.kt(0 hunks)example/App.tsx(5 hunks)ios/HybridNitroFs.swift(2 hunks)ios/NitroFSFileUploader.swift(1 hunks)ios/NitroFSImpl.swift(2 hunks)release.config.js(1 hunks)server/main.go(3 hunks)src/index.ts(1 hunks)src/type.ts(1 hunks)
💤 Files with no reviewable changes (1)
- android/src/main/java/com/nitrofs/NitroHttpClient.kt
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/index.ts (1)
src/specs/nitro-fs.nitro.ts (1)
NitroFS(5-126)
android/src/main/java/com/nitrofs/HybridNitroFS.kt (1)
src/type.ts (1)
NitroFile(36-40)
example/App.tsx (1)
src/specs/nitro-fs.nitro.ts (1)
NitroFS(5-126)
ios/HybridNitroFs.swift (3)
ios/NitroFSFileUploader.swift (1)
uploadFile(19-37)ios/NitroFSImpl.swift (2)
uploadFile(97-107)downloadFile(109-124)ios/NitroFSFileDownloader.swift (1)
downloadFile(26-53)
🪛 LanguageTool
README.md
[uncategorized] ~62-~62: Loose punctuation mark.
Context: ...ral directory constants: - BUNDLE_DIR: Directory for storing bundle files - `D...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 SwiftLint (0.57.0)
ios/NitroFSFileUploader.swift
[Warning] 32-32: Prefer at least one space after slashes for comments
(comment_spacing)
[Warning] 32-32: TODOs should be resolved (Implement upload file)
(todo)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build iOS Example App (old architecture)
🔇 Additional comments (27)
.github/workflows/release.yml (1)
6-7: Good addition of the 'next' branch to the release triggersThis change ensures that releases are automatically triggered when pushing to either the main branch or the next branch, which aligns well with the branch configuration in your semantic-release setup.
🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 6-6: wrong indentation: expected 10 but found 8
(indentation)
release.config.js (1)
42-47: Improved changelog configuration with explicit file pathConverting from a simple string format to the array format with explicit options makes the configuration more maintainable and clearer. This is consistent with how you've configured the other plugins in this file.
NitroFs.podspec (1)
29-29: LGTM: Code formatting change.The added whitespace doesn't affect functionality and maintains consistent formatting.
src/index.ts (1)
5-8: Export style change from named to default export.This change aligns with the updated usage in the example app. Note that any code importing this module will need to update their import statements from
import { NitroFS } from '...'toimport NitroFS from '...'.src/type.ts (1)
15-18:Details
✅ Verification successful
API contract changed from body to field property.
This change simplifies the API by focusing on the field name for file upload rather than allowing a full customizable body. The JSDoc has been properly updated with the default value.
🏁 Script executed:
#!/bin/bash # Check if all consumers of NitroUploadOptions have been updated to use field instead of body # Find all TypeScript files that might use NitroUploadOptions echo "Searching for files that might use NitroUploadOptions..." rg -l "NitroUploadOptions" --type ts # Check for any remaining references to uploadOptions.body echo "Checking for any remaining references to uploadOptions.body..." rg "uploadOptions\.body" --type ts # Checking native implementations echo "Checking iOS implementation..." rg "uploadOptions\.field" --type swift echo "Checking Android implementation..." rg "uploadOptions\.field" --type kotlinLength of output: 792
API contract change verified:
bodyreplaced withfieldAll references to the old
bodyproperty have been removed and replaced withfield, with the default value correctly applied:
- No remaining
uploadOptions.bodyreferences in TypeScript (checked acrosssrc/).- Native implementations in
ios/NitroFSFileUploader.swiftandandroid/src/main/java/com/nitrofs/NitroFileUploader.ktnow useuploadOptions.field(falling back to"file").ios/HybridNitroFs.swift (2)
114-118: Code cleanup: Delegating file operations to nitroFSImpl.Good refactoring that simplifies the class and improves code organization by consistently delegating operations to the implementation class.
129-134: Code cleanup: Delegating file download to nitroFSImpl.Good refactoring that follows the same pattern as the upload method, centralizing implementation details in the NitroFSImpl class.
android/src/main/java/com/nitrofs/NitroFileUploader.kt (3)
7-9: Imports updated for local client creation.The imports have been appropriately updated to support creating local HttpClient instances.
25-27: Good practice: Creating a local HttpClient instance.Creating a dedicated HttpClient instance for each upload operation is a good practice compared to using a shared instance, as it ensures proper lifecycle management and resource cleanup.
28-42: Improved resource management and field name handling.The implementation has two important improvements:
- Using
client.use { }ensures the HttpClient is properly closed after the operation- Using
uploadOptions.field ?: "file"allows for custom form field names to be specifiedBoth changes enhance the flexibility and resource management of the uploader.
server/main.go (4)
73-79: Good addition: File size retrieval for Content-Length header.Retrieving and using the file size for the Content-Length header is a good practice as it:
- Allows clients to show accurate download progress
- Helps clients pre-allocate resources
- Enables proper range requests support
135-135: Fixed file copy source.The change correctly uses the uploaded file as the source for copying to the destination file.
140-144: MIME type detection for uploaded files.Adding MIME type detection based on file extension helps set the appropriate Content-Type header for the uploaded file.
146-147: Improved response handling.Setting the Content-Type header and explicitly writing the HTTP status code before sending the response body follows best practices for HTTP responses.
README.md (3)
3-16: Great introduction and feature overview!The image and feature list provide a clear overview of the module's capabilities, including the newly added download functionality. This introduction helps users quickly understand what the module offers.
31-54: Well-documented Quick Start section!The Quick Start section provides clear and concise examples of the module's core functionality, including the new download feature with progress tracking. This helps users quickly integrate the module into their projects.
155-172: Complete and accurate downloadFile documentationThe documentation for the downloadFile method accurately reflects the implementation and provides a clear example of usage with progress tracking. This will help users properly integrate the download functionality.
example/App.tsx (4)
3-3: Updated import style for NitroFSThe import has been changed from a named import to a default import with a separate type import, which aligns with the module's export style.
11-15: Good platform-specific download URL configurationThe downloadURL constant follows the same pattern as uploadURL, correctly handling platform-specific URL differences between iOS and Android emulator environments.
114-127: Improved user experience with button state managementDisabling the upload button during active uploads prevents multiple concurrent uploads and provides clear visual feedback to users about the ongoing operation.
139-156: Well-implemented download functionality with progress trackingThe download implementation properly uses the new API, includes progress tracking, and disables the button during downloads to prevent multiple concurrent operations.
ios/NitroFSImpl.swift (3)
12-13: Good implementation of uploader and downloader dependenciesThe fileUploader and fileDownloader properties are properly initialized in the constructor, following a clean dependency injection pattern.
Also applies to: 17-18
97-107: Well-structured uploadFile methodThe uploadFile method properly delegates to the fileUploader instance, passing all necessary parameters including the progress callback.
109-124: Good implementation of downloadFile method with proper error handlingThe downloadFile method correctly checks for fileDownloader availability and delegates the operation with all required parameters. The method signature aligns well with the interface defined in the spec.
android/src/main/java/com/nitrofs/NitroFSImpl.kt (2)
17-17: FileDownloader property properly initializedThe fileDownloader property is correctly initialized as a private val, making it immutable and encapsulated.
93-105: Well-implemented downloadFile methodThe downloadFile method properly delegates to the fileDownloader component with all necessary parameters. The implementation is clean and follows good separation of concerns.
ios/NitroFSFileUploader.swift (1)
11-12: Question: shouldfileManagerreally beweak?
FileManageris not reference-counted by default, so marking itweakoften leaves you with anilinstance the first time you need it. Unless you purposely inject a shared singleton (which lives forever anyway), consider making it a strong reference:- weak var fileManager: FileManager? + private let fileManager: FileManagerIf retaining it is undesirable, document why so future maintainers know the intent.
| package com.nitrofs | ||
|
|
||
| import android.os.Handler | ||
| import android.os.Looper | ||
| import io.ktor.client.HttpClient | ||
| import io.ktor.client.call.body | ||
| import io.ktor.client.engine.okhttp.OkHttp | ||
| import io.ktor.client.plugins.onDownload | ||
| import io.ktor.client.request.prepareGet | ||
| import io.ktor.http.HttpMethod | ||
| import io.ktor.util.cio.writeChannel | ||
| import io.ktor.utils.io.ByteReadChannel | ||
| import io.ktor.utils.io.copyAndClose | ||
| import java.io.File | ||
|
|
||
| class FileDownloader { | ||
| suspend fun downloadFile( | ||
| serverUrl: String, | ||
| fileName: String, | ||
| destinationPath: String, | ||
| onProgress: ((Double, Double) -> Unit)? | ||
| ) { | ||
| val outputFile = File(destinationPath) | ||
| outputFile.parentFile?.mkdirs() | ||
| val client = HttpClient(OkHttp) | ||
|
|
||
| client.use { it | ||
| it.prepareGet("$serverUrl/$fileName") { | ||
| method = HttpMethod.Get | ||
| onDownload { bytesSentTotal, contentLength -> | ||
| if (bytesSentTotal > 0){ | ||
| onProgress?.let { | ||
| Handler(Looper.getMainLooper()).post { | ||
| onProgress.invoke(bytesSentTotal.toDouble(), contentLength.toDouble()) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }.execute { response -> | ||
| val channel: ByteReadChannel = response.body() | ||
| channel.copyAndClose(outputFile.writeChannel()) | ||
| } | ||
| } | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Well-implemented file download functionality.
The new FileDownloader class is well-structured with several good practices:
- Creating parent directories if they don't exist (line 24)
- Properly using HttpClient with resource management via
useblock (lines 25-43) - Setting up download progress callbacks on the main thread (lines 30-38)
- Using efficient streaming with
copyAndCloseto write directly to file (line 41)
However, there are a few opportunities for improvement:
- Missing validation for the server URL and file name parameters
- No handling for HTTP error responses (non-200 status codes)
- Missing timeout configuration for the HTTP client
Consider adding error handling for HTTP responses and timeouts:
val client = HttpClient(OkHttp) {
+ engine {
+ config {
+ connectTimeout(30, TimeUnit.SECONDS)
+ readTimeout(30, TimeUnit.SECONDS)
+ }
+ }
}
client.use { it
it.prepareGet("$serverUrl/$fileName") {
method = HttpMethod.Get
onDownload { bytesSentTotal, contentLength ->
// ... existing code ...
}
}.execute { response ->
+ if (!response.status.isSuccess()) {
+ throw IOException("Download failed with status: ${response.status}")
+ }
val channel: ByteReadChannel = response.body()
channel.copyAndClose(outputFile.writeChannel())
}
}📝 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.
| package com.nitrofs | |
| import android.os.Handler | |
| import android.os.Looper | |
| import io.ktor.client.HttpClient | |
| import io.ktor.client.call.body | |
| import io.ktor.client.engine.okhttp.OkHttp | |
| import io.ktor.client.plugins.onDownload | |
| import io.ktor.client.request.prepareGet | |
| import io.ktor.http.HttpMethod | |
| import io.ktor.util.cio.writeChannel | |
| import io.ktor.utils.io.ByteReadChannel | |
| import io.ktor.utils.io.copyAndClose | |
| import java.io.File | |
| class FileDownloader { | |
| suspend fun downloadFile( | |
| serverUrl: String, | |
| fileName: String, | |
| destinationPath: String, | |
| onProgress: ((Double, Double) -> Unit)? | |
| ) { | |
| val outputFile = File(destinationPath) | |
| outputFile.parentFile?.mkdirs() | |
| val client = HttpClient(OkHttp) | |
| client.use { it | |
| it.prepareGet("$serverUrl/$fileName") { | |
| method = HttpMethod.Get | |
| onDownload { bytesSentTotal, contentLength -> | |
| if (bytesSentTotal > 0){ | |
| onProgress?.let { | |
| Handler(Looper.getMainLooper()).post { | |
| onProgress.invoke(bytesSentTotal.toDouble(), contentLength.toDouble()) | |
| } | |
| } | |
| } | |
| } | |
| }.execute { response -> | |
| val channel: ByteReadChannel = response.body() | |
| channel.copyAndClose(outputFile.writeChannel()) | |
| } | |
| } | |
| } | |
| } | |
| package com.nitrofs | |
| import android.os.Handler | |
| import android.os.Looper | |
| import io.ktor.client.HttpClient | |
| import io.ktor.client.call.body | |
| import io.ktor.client.engine.okhttp.OkHttp | |
| import io.ktor.client.plugins.onDownload | |
| import io.ktor.client.request.prepareGet | |
| import io.ktor.http.HttpMethod | |
| import io.ktor.util.cio.writeChannel | |
| import io.ktor.utils.io.ByteReadChannel | |
| import io.ktor.utils.io.copyAndClose | |
| import java.io.File | |
| class FileDownloader { | |
| suspend fun downloadFile( | |
| serverUrl: String, | |
| fileName: String, | |
| destinationPath: String, | |
| onProgress: ((Double, Double) -> Unit)? | |
| ) { | |
| val outputFile = File(destinationPath) | |
| outputFile.parentFile?.mkdirs() | |
| val client = HttpClient(OkHttp) { | |
| engine { | |
| config { | |
| connectTimeout(30, TimeUnit.SECONDS) | |
| readTimeout(30, TimeUnit.SECONDS) | |
| } | |
| } | |
| } | |
| client.use { it | |
| it.prepareGet("$serverUrl/$fileName") { | |
| method = HttpMethod.Get | |
| onDownload { bytesSentTotal, contentLength -> | |
| if (bytesSentTotal > 0){ | |
| onProgress?.let { | |
| Handler(Looper.getMainLooper()).post { | |
| onProgress.invoke(bytesSentTotal.toDouble(), contentLength.toDouble()) | |
| } | |
| } | |
| } | |
| } | |
| }.execute { response -> | |
| if (!response.status.isSuccess()) { | |
| throw IOException("Download failed with status: ${response.status}") | |
| } | |
| val channel: ByteReadChannel = response.body() | |
| channel.copyAndClose(outputFile.writeChannel()) | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In android/src/main/java/com/nitrofs/FileDownloader.kt lines 1 to 45, add
validation checks for serverUrl and fileName parameters to ensure they are not
empty or malformed before proceeding. Implement error handling for HTTP
responses by checking the status code after executing the request and throwing
an exception or returning an error if it is not successful (non-200). Configure
the HttpClient with appropriate timeout settings to avoid indefinite waits
during download. These changes will improve robustness by validating inputs,
handling HTTP errors gracefully, and preventing hangs due to network issues.
| func uploadFile( | ||
| file: NitroFile, | ||
| uploadOptions: NitroUploadOptions, | ||
| onProgress: ((Double, Double) -> Void)? = nil | ||
| ) async throws -> String { | ||
| ) async throws { | ||
| self.onProgress = onProgress | ||
|
|
||
| if let fileExist = fileManager?.fileExists(atPath: file.path), !fileExist { | ||
| throw NitroFSError.nitroFSFileNotFound(message: "Failed to upload file. File does not exist on path \(file.path)") | ||
| guard let uploadURL = URL(string: uploadOptions.url) else { | ||
| throw NetworkError.invalidURL(message: "Invalid URL") | ||
| } | ||
|
|
||
| let fileURL = URL(fileURLWithPath: file.path) | ||
|
|
||
| let newHeaders = ["X-Filename": fileURL.lastPathComponent] | ||
|
|
||
| return try await makeUploadRequest( | ||
| url: uploadOptions.url, | ||
| method: uploadOptions.method?.stringValue ?? "POST", | ||
| headers: newHeaders, | ||
| fileURL: fileURL | ||
| ) | ||
| } | ||
|
|
||
| func cancelUpload() { | ||
| uploadTask?.cancel() | ||
| } | ||
|
|
||
| private func makeUploadRequest( | ||
| url: String, | ||
| method: String, | ||
| headers: [String: String], | ||
| fileURL: URL | ||
| ) async throws -> String { | ||
| guard let uploadURL = URL(string: url) else { | ||
| throw NetworkError.invalidURL(message: "The URL '\(url)' is not valid.") | ||
| } | ||
|
|
||
| var req = URLRequest(url: uploadURL) | ||
| req.httpMethod = method | ||
| req.timeoutInterval = 60 | ||
|
|
||
| for (key, value) in headers { | ||
| req.setValue(value, forHTTPHeaderField: key) | ||
| } | ||
|
|
||
| let session = URLSession(configuration: .default, delegate: self, delegateQueue: nil) | ||
|
|
||
| return try await withCheckedThrowingContinuation { continuation in | ||
| self.uploadTask = session.uploadTask(with: req, fromFile: fileURL) { data, response, error in | ||
| if let error = error as NSError? { | ||
| if error.code == NSURLErrorCancelled { | ||
| if error.userInfo[NSURLSessionDownloadTaskResumeData] is Data { | ||
| // TODO: Store resume data for later use if needed | ||
| print("Upload cancelled with resume data available") | ||
| } | ||
| } | ||
| continuation.resume(throwing: error) | ||
| return | ||
| } | ||
|
|
||
| guard let httpResponse = response as? HTTPURLResponse, | ||
| (200...299).contains(httpResponse.statusCode), | ||
| let data = data, | ||
| let responseString = String(data: data, encoding: .utf8) else { | ||
| let res = (response as? HTTPURLResponse) | ||
| print("Upload failed with status code:", res?.statusCode ?? 0) | ||
| print("Response data:", String(data: data ?? Data(), encoding: .utf8) ?? "No data") | ||
| continuation.resume(throwing: NetworkError.invalidResponse) | ||
| return | ||
| } | ||
|
|
||
| continuation.resume(returning: responseString) | ||
| } | ||
| self.uploadTask?.resume() | ||
| } | ||
| } | ||
|
|
||
| func resumeUpload(with resumeData: Data, headers: [String: String]) async throws -> String { | ||
| guard let uploadURL = URL(string: headers["url"] ?? "") else { | ||
| throw NetworkError.invalidURL(message: "Invalid URL for resume") | ||
| } | ||
|
|
||
| var req = URLRequest(url: uploadURL) | ||
| req.httpMethod = "PUT" | ||
| req.timeoutInterval = 60 | ||
|
|
||
| for (key, value) in headers { | ||
| req.setValue(value, forHTTPHeaderField: key) | ||
| } | ||
|
|
||
| let session = URLSession(configuration: .default, delegate: self, delegateQueue: nil) | ||
| let fileURL = URL(fileURLWithPath: file.path) | ||
| _ = uploadOptions.field ?? "file" | ||
|
|
||
| //TODO: Implement upload file | ||
|
|
||
| return try await withCheckedThrowingContinuation { continuation in | ||
| if #available(iOS 17.0, *) { | ||
| self.uploadTask = session.uploadTask(withResumeData: resumeData) { data, response, error in | ||
| if let error = error { | ||
| continuation.resume(throwing: error) | ||
| return | ||
| } | ||
|
|
||
| guard let httpResponse = response as? HTTPURLResponse, | ||
| (200...299).contains(httpResponse.statusCode), | ||
| let data = data, | ||
| let responseString = String(data: data, encoding: .utf8) else { | ||
| continuation.resume(throwing: NetworkError.invalidResponse) | ||
| return | ||
| } | ||
|
|
||
| continuation.resume(returning: responseString) | ||
| } | ||
| } else { | ||
| continuation.resume(throwing: NitroFSError.nitroFSUnavailable(message: "Resume upload is not supported on iOS versions below 17.0")) | ||
| } | ||
| self.uploadTask?.resume() | ||
| continuation.resume() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Upload stub leaves the feature non-functional and leaks unused variables
uploadFile currently
• validates uploadOptions.url and creates fileURL,
• ignores both of them,
• immediately returns via a continuation, and
• leaves a //TODO marker that SwiftLint is already flagging.
Until a real body is provided every call site will “succeed” without uploading anything – a silent functional bug that will only surface at runtime.
Suggested minimal implementation (keeps the delegate-based progress handling you already have):
@@
- let fileURL = URL(fileURLWithPath: file.path)
- _ = uploadOptions.field ?? "file"
-
- //TODO: Implement upload file
-
- return try await withCheckedThrowingContinuation { continuation in
- continuation.resume()
- }
+ let fileURL = URL(fileURLWithPath: file.path)
+ let fieldName = uploadOptions.field ?? "file"
+
+ var request = URLRequest(url: uploadURL)
+ request.httpMethod = "POST"
+
+ // Very small multipart helper – replace with a proper one if you support metadata
+ let boundary = UUID().uuidString
+ request.setValue("multipart/form-data; boundary=\(boundary)", forHTTPHeaderField: "Content-Type")
+
+ let body = try MultipartBody(boundary: boundary)
+ .append(fileURL: fileURL, name: fieldName)
+ .build()
+
+ let session = URLSession(configuration: .default,
+ delegate: self,
+ delegateQueue: nil)
+
+ let (_, response) = try await session.upload(for: request, from: body)
+ guard let http = response as? HTTPURLResponse,
+ 200..<300 ~= http.statusCode else {
+ throw NetworkError.httpError(statusCode: (response as? HTTPURLResponse)?.statusCode ?? -1)
+ }(You may already have a multipart helper; if so, wire it in here.)
At the very least, replace the continuation with a plain return to avoid misleading callers.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 32-32: Prefer at least one space after slashes for comments
(comment_spacing)
[Warning] 32-32: TODOs should be resolved (Implement upload file)
(todo)
🤖 Prompt for AI Agents
In ios/NitroFSFileUploader.swift lines 19 to 36, the uploadFile function
currently validates the URL and file path but does not perform any upload,
leaving the feature non-functional and causing silent runtime bugs. To fix this,
remove the withCheckedThrowingContinuation block and the TODO comment, then
implement the actual file upload logic using the validated uploadURL and
fileURL. If a multipart helper exists, integrate it here; otherwise, at minimum,
replace the continuation with a simple return to avoid misleading callers about
the upload success.
|
🎉 This PR is included in version 0.4.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
1 participant
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Refactor
Chores