Fix copy item functionality and improve path handling#128
Conversation
- Fix syntax errors in basename function for content URIs - Remove unused ext parameter from basename function - Ensure filename includes extension
WalkthroughAdds content:// URI support via a new ResolvedPath abstraction and stream-based handling, centralizes iOS path normalization, removes the optional ext parameter from basename across platforms, adds a File->NitroFileStat extension, and extends the example app with DCIM copy and document-pick features and hooks. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Hook as UseFileSystem
participant Resolver as ResolvedPath\nResolver
participant NitroFSImpl
participant AndroidCR as Content\nResolver
participant FS as File System
App->>Hook: user action (stat / copy / pick)
Hook->>Resolver: toResolvedPath(path)
alt content:// URI
Resolver-->>NitroFSImpl: ResolvedPath.Content(uri)
rect rgba(120,180,160,0.12)
NitroFSImpl->>AndroidCR: query metadata / open streams / delete
AndroidCR-->>NitroFSImpl: metadata / stream / result
end
else file path
Resolver-->>NitroFSImpl: ResolvedPath.FilePath(file)
rect rgba(120,160,220,0.12)
NitroFSImpl->>FS: file IO (read/write/delete)
FS-->>NitroFSImpl: result
end
end
NitroFSImpl-->>Hook: result / error
Hook-->>App: update UI / notify user
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/specs/nitro-fs.nitro.ts (1)
99-105: ---Update README.md documentation to reflect basename API change
The
basenamesignature has been updated across specs, implementations, and examples to remove theextparameter. However, the README.md (lines 243–251) still documents the old signature and shows examples passing theextargument. Update the documentation to show only the new single-parameter signature:
- Line 243: Change
basename(path: string, ext?: string): stringtobasename(path: string): string- Line 251: Remove the example
NitroFS.basename('/path/to/file.txt', '.txt')since theextparameter no longer existsAll codebase implementations and example code are already aligned with the breaking change; only documentation needs updating.
🧹 Nitpick comments (10)
example/src/components/action-panel.tsx (2)
132-147: Copy/Rename buttons now rely on downstream validation instead of input‑based disablingWith
disabled={!selectedFile}, users can tap Copy/Rename with an empty name and will hit the validation inside the hook instead of seeing the button disabled. That’s functionally safe but slightly noisier UX; if you want to guide users earlier, consider also disabling when!fileName.trim().
159-183: Android‑only DCIM/Document buttons correctly gated, but effectiveness ofdisabled={loading}depends on the hookThe new Android buttons and
copyImagesButtonstyle are wired cleanly and respectloadingfor opacity/disabled state. Note that this only works as intended ifcopyImagesFromDCIMToCacheandpickDocumentboth toggle the sharedloadingflag inuseFileSystem; currentlypickDocumentdoes not, so the “Pick a Document” button will never actually disable during its work.Also applies to: 240-242
example/src/hooks/use-file-system.ts (2)
415-517: DCIM→cache copy flow is solid; minor naming and UX refinements possibleThe Android‑gated DCIM scan correctly validates DCIM existence, filters by mime/extension, skips existing cache files, and tracks copy failures with aggregated logging and a summary Alert. Two small follow‑ups you might consider:
- Rename the local
filesfromreaddir(line 434) to something likedcimFilesto avoid shadowing the hook‑levelfilesstate.- Optionally refresh the visible listing after a successful run (e.g.,
await listFiles(cachePath)orcurrentPath) if you want the new copies to immediately appear in the UI.
519-566: pickDocument lacks loading/progress finalization; align it with other async actionsThe document‑picking flow is functionally correct—resolves metadata, copies into cache, derives name/extension, and uploads with progress—but it never toggles
loadingor resetsuploadProgresswhen done. This means:
- The Android button’s
disabled={loading}/opacity won’t reflect the operation.uploadProgressmay remain at the last non‑zero value after completion or failure.Bringing it in line with the other handlers will make behavior more predictable. For example:
const pickDocument = async () => { - try { - const doc = await NitroDocumentPicker.pick({ + try { + setLoading(true); + setUploadProgress(0); + const doc = await NitroDocumentPicker.pick({ types: ['all'], multiple: false, }); @@ - await NitroFS.uploadFile(nitroFile, uploadOptions, (uploadedBytes, totalBytes) => { - const progress = (uploadedBytes / totalBytes) * 100; - setUploadProgress(progress); - }); - } catch (error) { + await NitroFS.uploadFile( + nitroFile, + uploadOptions, + (uploadedBytes, totalBytes) => { + const progress = (uploadedBytes / totalBytes) * 100; + setUploadProgress(progress); + }, + ); + setUploadProgress(0); + } catch (error) { console.error('Error picking document:', error); Alert.alert('Error', `Failed to pick document: ${error}`); - } + } finally { + setLoading(false); + } };ios/NitroFSImpl+Utils.swift (1)
9-46: Path validation helper is very permissive; consider aligning docs/behavior
isValidPath(_:)currently only rejects empty strings and malformedfile://URIs; all other inputs (including obviously invalid ones) returntrue, even though the comment suggests broader validation. If that’s intentional, consider either tightening the checks (e.g., rejecting unsupported schemes) or updating the doc comment to clarify that “validation” here is minimal and thatpathToURL(_:)is mainly a normalization entry point rather than a strict validator.android/src/main/java/com/nitrofs/File+Extensions.kt (1)
6-13: File → NitroFileStat mapping is straightforward; ctime/mtime semantics to confirmThe extension is a clean, direct mapping. The only nuance is that both
ctimeandmtimeare derived fromlastModified(), which means creation and modification times are identical at this layer. If the consumer APIs or JS/TS typings treatctimeas “creation time”, it might be worth documenting this or, if needed later, switching to a source that can distinguish the two (e.g.,java.nio.fileattributes on supported API levels).ios/NitroFSImpl.swift (2)
18-29: Path normalization via getPath/pathToURL is a solid improvementRouting most filesystem operations through
getPath(path:)(and ultimatelypathToURL(_:)) is a good step toward consistent handling of raw paths andfile://URIs:exists,writeFile,readFile,copy,mkdir,stat,readdir, andrenameall benefit from the same normalization and error surface.Two points to consider:
Behavior for invalid paths differs between methods
exists(path:)now returnsfalseon invalid/unnormalizable paths (viaisValidPath/try? getPath).- Most other methods throw
NitroFSError.encodingErrorwhen normalization fails.
If JS callers expect a thrown error rather thanfalsefor invalid inputs, you might want to alignexistswith the others, or at least document thatexiststreats invalid paths as “not found”.Ensuring destination parent directories exist in
copy
- On Android,
ResolvedPath.openOutputStreamensures parent directories exist forFilePathviafile.parentFile?.mkdirs().- On iOS,
copy(source:destination:)currently assumes the destination’s parent directory already exists and will throw otherwise.For cross‑platform parity and a better DX around “copy item”, consider mirroring the Android behavior by creating the destination’s parent directory (with
withIntermediateDirectories: true) when it doesn’t exist before callingcopyItem(atPath:toPath:).Also applies to: 37-45, 50-52, 85-89, 103-104, 111-113, 117-118, 138-139, 181-189
121-123: Stat now uses normalized paths; IsDirectory still uses FileManager.defaultUsing
normalizedPathconsistently instat(path:)andIsDirectory(_:)strengthens correctness when callers passfile://URIs or other normalized forms. One minor note:IsDirectory(_:)still relies onFileManager.defaultrather than the injectedfileManager. IfNitroFSImplis ever used with a customFileManager(e.g., for sandboxing or testing), you may eventually want to delegate directory checks through that same instance for full consistency.Also applies to: 253-256
android/src/main/java/com/nitrofs/ResolvedPath.kt (1)
11-57: ResolvedPath abstraction is clean; be explicit about null/unsupported schemes handlingThe
ResolvedPathsealed class and its helpers (String.toResolvedPath,Uri.toResolvedPath,openInputStream,openOutputStream) nicely centralize handling ofcontent://vs filesystem paths and ensure parent directories exist forFilePathwrites.A couple of behavioral points to keep in mind at call sites:
String.toResolvedPath()andUri.toResolvedPath()returnnullfor unsupported schemes (e.g.http://) or malformedfile://URIs (nopath). Callers inNitroFSImplshould consistently convert thesenulls into a domain error (e.g., aNitroFSErrorvariant) rather than letting them bubble up asNullPointerExceptions.openInputStreamreturnsnullwhen aFilePathdoesn’t exist, but propagates exceptions for content resolver failures. Similarly,openOutputStreamreturnsnullif the resolver returnsnull. Callers should explicitly handle thenullcase so that JS receives a clear, intentional error instead of a late NPE.Overall, the structure is solid; the main work is making sure the use sites translate
nullinto consistent, user-facing errors.android/src/main/java/com/nitrofs/NitroFSImpl.kt (1)
148-164: Consider creating parent directories for file path destinations.Unlike
writeFile()(lines 59-64), this method doesn't ensure parent directories exist before copying to aFilePathdestination. This could cause silent failures.Consider adding parent directory validation:
fun copyFile( srcPath: String, destPath: String ) { val src = srcPath.toResolvedPath() ?: throw RuntimeException("Invalid source path: $srcPath") val dest = destPath.toResolvedPath() ?: throw RuntimeException("Invalid destination path: $destPath") // Ensure parent directory exists for FilePath destinations if (dest is ResolvedPath.FilePath) { val parentDir = dest.file.parentFile if (parentDir != null && !parentDir.exists()) { if (!parentDir.mkdirs()) { throw RuntimeException("Failed to create parent directories for: $destPath") } } } try { src.openInputStream(context)?.use { input -> dest.openOutputStream(context)?.use { output -> input.copyTo(output, DEFAULT_BUFFER_SIZE) } } } catch (e: Exception) { throw RuntimeException("Failed to copy file: ${e.message}") } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (10)
bun.lockis excluded by!**/*.lockexample/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/NitroFS-Swift-Cxx-Bridge.hppis 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 (11)
android/src/main/java/com/nitrofs/File+Extensions.kt(1 hunks)android/src/main/java/com/nitrofs/HybridNitroFS.kt(1 hunks)android/src/main/java/com/nitrofs/NitroFSImpl.kt(4 hunks)android/src/main/java/com/nitrofs/ResolvedPath.kt(1 hunks)example/package.json(1 hunks)example/src/components/action-panel.tsx(6 hunks)example/src/hooks/use-file-system.ts(1 hunks)ios/HybridNitroFs.swift(1 hunks)ios/NitroFSImpl+Utils.swift(1 hunks)ios/NitroFSImpl.swift(5 hunks)src/specs/nitro-fs.nitro.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/specs/nitro-fs.nitro.ts (1)
nitrogen/generated/android/c++/JHybridNitroFSSpec.hpp (10)
path(64-64)path(65-65)path(66-66)path(69-69)path(70-70)path(71-71)path(72-72)path(74-74)path(75-75)path(76-76)
android/src/main/java/com/nitrofs/NitroFSImpl.kt (1)
src/type.ts (1)
NitroFileStat(42-48)
ios/HybridNitroFs.swift (4)
android/src/main/java/com/nitrofs/HybridNitroFS.kt (1)
basename(161-168)android/src/main/java/com/nitrofs/NitroFSImpl.kt (1)
basename(267-290)ios/NitroFSImpl.swift (1)
basename(202-205)nitrogen/generated/android/kotlin/com/margelo/nitro/nitrofs/HybridNitroFSSpec.kt (1)
basename(123-125)
ios/NitroFSImpl.swift (1)
ios/NitroFSImpl+Utils.swift (3)
isValidPath(13-26)getPath(45-48)pathToURL(29-42)
example/src/hooks/use-file-system.ts (3)
example/src/types/index.ts (2)
FileItem(1-6)DirectoryType(8-8)example/src/utils/file-utils.ts (1)
getMimeTypeFromExtension(82-142)src/type.ts (1)
NitroUploadOptions(5-19)
🪛 detekt (1.23.8)
android/src/main/java/com/nitrofs/NitroFSImpl.kt
[warning] 161-161: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build Android Example App (new architecture)
- GitHub Check: Build Android Example App (old architecture)
- GitHub Check: Build iOS Example App (old architecture)
- GitHub Check: Build iOS Example App (new architecture)
🔇 Additional comments (9)
example/src/hooks/use-file-system.ts (2)
9-29: Initialization and directory listing look correct; consider parallelizing stats if performance mattersBootstrapping from
NitroFS.DOCUMENT_DIRvialistFilesand capturingFileItemmetadata with per‑entrystatis logically sound, and loading/error handling is consistent. If you expect large directories, you may want to parallelize thestatcalls (e.g.,await Promise.all(...)) to avoid N+1 sequential I/O, but for an example app this is acceptable as‑is.Also applies to: 31-59
266-305: Verified: No remaining callers expect older copy behaviorSearched the codebase and confirmed
copyItemhas only two reference points: exported from the hook at line 587 and passed asonCopyItemprop to a single component in App.tsx. No other callers discovered. The enhanced validation and error handling in the newcopyItemimplementation do not create compatibility issues—the function is the sole consumer of this behavior, and its interface remains unchanged from the caller's perspective.ios/HybridNitroFs.swift (1)
150-153: iOS basename wrapper matches new core implementation and error‑handling patternThe updated
basename(path: String)correctly delegates tonitroFSImpl.basename(path:)and mirrors the error logging/propagation used indirnameandextname, keeping the Swift bridge consistent with the new, simplified API.example/package.json (1)
9-16: Dependency and start script changes are correctThe
react-native-nitro-document-pickeraddition is verified—it's imported and used inexample/src/hooks/use-file-system.tsat line 521. The start script flag reordering is benign. No changes needed.android/src/main/java/com/nitrofs/HybridNitroFS.kt (1)
161-164: Basename wrapper correctly updated to new APIThe
basename(path: String)override cleanly forwards tonitroFsImpl.basename(path)and keeps the existing error-handling pattern, so the Android public API is consistent with the implementation and the cross‑platform signature change.android/src/main/java/com/nitrofs/NitroFSImpl.kt (4)
5-6: LGTM: Imports support Content URI operations.The new imports are correctly used for Content URI metadata queries in
stat()andbasename().
23-23: LGTM: Content resolver properly initialized.
25-39: LGTM: Content URI existence check is correct.The implementation properly handles both file paths and Content URIs with efficient querying and proper cursor cleanup.
41-49: LGTM: Deletion correctly handles both path types.Using
deleteRecursively()for file paths appropriately handles both files and directories, and Content URI deletion is correct.
…nction and add new file system methods
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(1 hunks)example/App.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build iOS Example App (old architecture)
- GitHub Check: Build iOS Example App (new architecture)
- GitHub Check: Build Android Example App (old architecture)
- GitHub Check: Build Android Example App (new architecture)
🔇 Additional comments (1)
example/App.tsx (1)
37-38: LGTM! ActionPanel interface correctly accepts new props.Verification confirms the ActionPanel interface has been properly updated with
onCopyImagesFromDCIMToCacheandonPickDocument(both() => void). The props are correctly destructured and used in the component. Type signatures match between App.tsx and ActionPanel—no issues found.
…thod - Enhance error handling in Android's path resolution to throw descriptive errors for invalid paths. - Update iOS's dirname method to return the correct path instead of the absolute string representation.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
android/src/main/java/com/nitrofs/NitroFSImpl.kt (3)
266-288: Tighten basename() error handling for content URIs.For file paths this correctly returns the name including extension, but for content URIs failure cases currently degrade to an empty string, which can hide real errors (query failure, missing
DISPLAY_NAME, etc.).You can make failures explicit and narrow the projection like this:
fun basename(path: String): String { - val resolved = path.toResolvedPath() ?: throw Error("Invalid path: $path") + val resolved = path.toResolvedPath() ?: throw RuntimeException("Invalid path: $path") return when(resolved) { is ResolvedPath.FilePath -> resolved.file.name is ResolvedPath.Content -> { val uri = resolved.uri - val fileName = contentResolver.query( - uri, null, null, null, null - )?.use { cursor -> - if (cursor.moveToFirst()) { - val nameIndex = cursor.getColumnIndex(OpenableColumns.DISPLAY_NAME) - if (nameIndex != -1) { - cursor.getString(nameIndex) - } else { - "" - } - } else { - "" - } - } ?: "" - fileName + val fileName = contentResolver.query( + uri, + arrayOf(OpenableColumns.DISPLAY_NAME), + null, + null, + null + )?.use { cursor -> + if (!cursor.moveToFirst()) { + null + } else { + val nameIndex = cursor.getColumnIndex(OpenableColumns.DISPLAY_NAME) + if (nameIndex != -1 && !cursor.isNull(nameIndex)) { + cursor.getString(nameIndex) + } else { + null + } + } + } ?: throw RuntimeException("Unable to get basename for content uri: $path") + + fileName } } }This keeps the “basename includes extension” behavior while failing loudly when the provider cannot supply a name.
Is `OpenableColumns.DISPLAY_NAME` the recommended column for obtaining a display name for Android content URIs (e.g., from the Storage Access Framework)?
152-163: Standardize on a single exception type instead of mixing Error and RuntimeException.Within this class some methods now throw
RuntimeException(writeFile,readFile,downloadFile), while others in these ranges throwErrorfor similar kinds of failures. Mixing these two unchecked types can be surprising for callers and makes error handling less consistent.It would be cleaner to standardize on
RuntimeExceptionfor all operational failures incopyFile,mkdir,stat,basename, andextname(and optionally older methods likereaddir/rename) so consumers see a consistent exception surface.Also applies to: 167-173, 175-177, 214-215, 266-268, 292-293
152-163: Handle null streams and preserve the original cause in copyFile.Two issues here:
openInputStream/openOutputStreamare nullable; if either returnsnull, the copy silently does nothing and still appears successful.- The catch block wraps
Exceptionin a newErrorwithout the original cause, which is what detekt flags as a swallowed exception.Consider tightening this implementation as follows:
fun copyFile( srcPath: String, destPath: String ) { - val src = srcPath.toResolvedPath() ?: throw Error("Invalid source path: $srcPath") - val dest = destPath.toResolvedPath() ?: throw Error("Invalid destination path: $destPath") + val src = srcPath.toResolvedPath() + ?: throw RuntimeException("Invalid source path: $srcPath") + val dest = destPath.toResolvedPath() + ?: throw RuntimeException("Invalid destination path: $destPath") - try { - src.openInputStream(context)?.use { input -> - dest.openOutputStream(context)?.use { output -> - input.copyTo(output, DEFAULT_BUFFER_SIZE) - } - } - } catch (e: Exception) { - throw Error("Failed to copy file: ${e.message}") - } + try { + val input = src.openInputStream(context) + ?: throw RuntimeException("Unable to open input stream for: $srcPath") + val output = dest.openOutputStream(context) + ?: throw RuntimeException("Unable to open output stream for: $destPath") + + input.use { inStream -> + output.use { outStream -> + inStream.copyTo(outStream, DEFAULT_BUFFER_SIZE) + } + } + } catch (e: Exception) { + // Keeps full stack trace and satisfies detekt's swallowed-exception check + throw RuntimeException("Failed to copy file from $srcPath to $destPath", e) + } }This makes failures explicit, standardizes on
RuntimeException, and preserves the original stack trace.#!/bin/bash # Inspect definitions of openInputStream/openOutputStream to confirm they are nullable and behave as expected. rg -n "openInputStream" -S rg -n "openOutputStream" -S
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
android/src/main/java/com/nitrofs/NitroFSImpl.kt(4 hunks)ios/NitroFSImpl.swift(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
android/src/main/java/com/nitrofs/NitroFSImpl.kt (1)
src/type.ts (1)
NitroFileStat(42-48)
ios/NitroFSImpl.swift (1)
ios/NitroFSImpl+Utils.swift (3)
isValidPath(13-26)getPath(45-48)pathToURL(29-42)
🪛 detekt (1.23.8)
android/src/main/java/com/nitrofs/NitroFSImpl.kt
[warning] 161-161: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build iOS Example App (old architecture)
- GitHub Check: Build iOS Example App (new architecture)
- GitHub Check: Build Android Example App (old architecture)
- GitHub Check: Build Android Example App (new architecture)
🔇 Additional comments (10)
ios/NitroFSImpl.swift (4)
22-29: LGTM: Defensive path validation in exists.The addition of path validation and graceful error handling with
try?is appropriate for an existence check. Returningfalsefor invalid or non-normalizable paths aligns with standardexistssemantics.
37-46: LGTM: Consistent path normalization across file operations.All file operations now consistently normalize paths via
getPathbefore processing. The pattern is applied uniformly acrosswriteFile,readFile,copy,mkdir,stat, andreaddir, which ensures reliable path handling throughout the implementation.Also applies to: 50-78, 85-88, 103-104, 111-122, 130-148
192-194: LGTM: dirname correctly returns filesystem path.The function now returns
.pathinstead of.absoluteString, which ensuresdirnamereturns a plain filesystem path (e.g.,"/foo") rather than a URL string (e.g.,"file:///foo"). This makes the return value directly consumable by other NitroFS methods and addresses the concern raised in the previous review.
202-204: Breaking change properly implemented and verified.Verification confirms all callers of
basename()have been updated to the new single-parameter signature. No calls to the old two-parameter variant remain in the codebase across ios/HybridNitroFs.swift, ios/NitroFSImpl.swift, and the generated bridge files.android/src/main/java/com/nitrofs/NitroFSImpl.kt (6)
5-6: New imports align with content-URI handling.
DocumentsContractandOpenableColumnsare appropriate for the newstat/basenameimplementations; no issues here.
23-24: CachingcontentResolveris appropriate.Storing
context.contentResolverin a private val is fine given theReactApplicationContextlifetime; avoids repeated lookups without leaking anything.
26-38: exists() logic for file vs content URIs looks correct.Resolving the path first and using
cursor.moveToFirst()as the existence test (with properuse-based closing) matches the boolean contract and cleanly handles invalid paths viafalse.
42-48: unlink() behavior is reasonable for both path types.Resolving first and returning
deleteRecursively()/delete(...) > 0gives a clear success/failure signal while treating unresolved paths asfalse.
167-172: mkdir() behavior for file and content URIs is clear.Using
mkdirs()for filesystem paths and throwing for unsupported content URIs makes the unsupported case explicit; unresolved paths returningfalseis also reasonable.
176-215: Content-URI stat implementation looks robust.The projection, helper accessors, and derivation of
size,mtime, and directory/file flags from the MIME type are all sensible; theNitroFileStatmapping matches the expected shape.
- Remove redundant fragment wrapping in ActionPanel component. - Ensure proper rendering of elements within the component.
…ieval - Update content resolver queries to throw descriptive errors when unable to retrieve file name or extension from content URIs. - Ensure null values are handled appropriately instead of returning empty strings.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
android/src/main/java/com/nitrofs/NitroFSImpl.kt (4)
167-173: Use RuntimeException for consistency.This method throws
Errorat line 171, but other methods in the class throwRuntimeException.Apply this diff:
- is ResolvedPath.Content -> throw Error("Cannot create directory from content URI: $path") + is ResolvedPath.Content -> throw RuntimeException("Cannot create directory from content URI: $path")
175-217: Use RuntimeException for consistency.This method throws
Errorat lines 176, 188, and 214, but other methods in the class throwRuntimeException. The content URI handling logic is well-implemented with appropriate fallbacks.Apply this diff:
- val resolved = path.toResolvedPath() ?: throw Error("Invalid path: $path") + val resolved = path.toResolvedPath() ?: throw RuntimeException("Invalid path: $path") return when(resolved) { is ResolvedPath.FilePath -> resolved.file.toNitroFileStat() is ResolvedPath.Content -> { // ... existing code ... contentResolver.query(uri, projection, null, null, null)?.use { cursor -> if (!cursor.moveToFirst()) { - throw Error("Unable to stat content uri (no row): $path") + throw RuntimeException("Unable to stat content uri (no row): $path") } // ... existing code ... - } ?: throw Error("Unable to stat content uri (query failed): $path") + } ?: throw RuntimeException("Unable to stat content uri (query failed): $path") } }
266-290: Use RuntimeException for consistency.This method throws
Errorat lines 267 and 285, but other methods in the class throwRuntimeException. The content URI handling viaDISPLAY_NAMEis correct.Apply this diff:
- val resolved = path.toResolvedPath() ?: throw Error("Invalid path: $path") + val resolved = path.toResolvedPath() ?: throw RuntimeException("Invalid path: $path") return when(resolved) { is ResolvedPath.FilePath -> resolved.file.name is ResolvedPath.Content -> { // ... existing code ... - } ?: throw Error("Unable to get file name from content URI: $path") + } ?: throw RuntimeException("Unable to get file name from content URI: $path") fileName } }
292-306: CRITICAL: Return extension with leading dot to match documented API contract.This critical issue was flagged in previous reviews and remains unaddressed. The documentation explicitly states that
extname('/path/to/file.txt')should return.txt(with leading dot), matching Node.js semantics. However:
- Line 296:
resolved.file.extensionreturns'txt'(no dot)- Line 300:
MimeTypeMap.getSingleton().getExtensionFromMimeType(...)returns'txt'(no dot)Both must prepend
'.'when a non-empty extension exists. Edge case: when no extension exists (e.g., hidden files like.env), return empty string''.Secondary issue: This method throws
Error(lines 293, 300, 302) instead ofRuntimeExceptionused elsewhere in the class.Apply this diff:
- val resolved = path.toResolvedPath() ?: throw Error("Invalid path: $path") + val resolved = path.toResolvedPath() ?: throw RuntimeException("Invalid path: $path") return when(resolved){ - is ResolvedPath.FilePath -> resolved.file.extension + is ResolvedPath.FilePath -> { + val ext = resolved.file.extension + if (ext.isEmpty()) "" else ".$ext" + } is ResolvedPath.Content -> { val mimeType: String? = contentResolver.getType(resolved.uri) if (mimeType != null) { - MimeTypeMap.getSingleton().getExtensionFromMimeType(mimeType) ?: throw Error("Unable to get extension from mime type: $mimeType") + val ext = MimeTypeMap.getSingleton().getExtensionFromMimeType(mimeType) + ?: throw RuntimeException("Unable to get extension from mime type: $mimeType") + if (ext.isEmpty()) "" else ".$ext" } else { - throw Error("Unable to get mime type from content URI: $path") + throw RuntimeException("Unable to get mime type from content URI: $path") } } }
🧹 Nitpick comments (1)
example/src/components/action-panel.tsx (1)
159-172: Consider removing unnecessary fragment.The Android-specific block uses a fragment to wrap a single
View. Since there's only one child element, the fragment can be removed for cleaner code.{Platform.OS === 'android' && ( - <> <View style={styles.buttonRow}> <TouchableOpacity style={[styles.button, styles.copyImagesButton, { opacity: loading ? 0.5 : 1 }]} onPress={onCopyImagesFromDCIMToCache} disabled={loading} > <Text style={styles.buttonText}>Copy DCIM Images</Text> </TouchableOpacity> </View> - </> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
android/src/main/java/com/nitrofs/NitroFSImpl.kt(4 hunks)example/src/components/action-panel.tsx(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
android/src/main/java/com/nitrofs/NitroFSImpl.kt (1)
src/type.ts (1)
NitroFileStat(42-48)
🪛 detekt (1.23.8)
android/src/main/java/com/nitrofs/NitroFSImpl.kt
[warning] 161-161: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (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 (10)
android/src/main/java/com/nitrofs/NitroFSImpl.kt (4)
5-6: LGTM!The new imports are necessary for content URI support via DocumentsContract queries and OpenableColumns metadata access.
23-23: LGTM!Caching the contentResolver reference is appropriate and improves code readability throughout the class.
25-39: LGTM!The unified path handling via
toResolvedPath()correctly supports both file paths and content URIs. The minimal projection for content URI queries is efficient.
41-49: LGTM!The implementation correctly handles both file paths and content URIs with appropriate deletion methods for each type.
example/src/components/action-panel.tsx (6)
3-3: LGTM!The
Platformimport is correctly used for the Android-specific UI check on line 159.
24-25: LGTM!The new callback props are correctly typed and the past review confirmed that all call sites have been updated.
40-41: LGTM!The props are correctly destructured and match the interface definition.
240-242: LGTM!The new button style is well-defined with a distinct color that differentiates the new actions from existing buttons.
136-136: Parent component properly validates empty names—no issue.Verification confirms that both
copyItemandrenameItemcallbacks in the parent component validate thenewNameparameter withif (!newName.trim())before executing any operations. When an empty or whitespace-only name is passed, an error alert is shown and the operation returns early. Therefore, removing the!fileNamecheck from the disabled state is safe, and the PR change is correct.
173-183: No changes required; code correctly supports cross-platform document picking.The
react-native-nitro-document-pickerlibrary supports both iOS (UIDocumentPickerViewController) and Android (Intent.ACTION_OPEN_DOCUMENT), and thepickDocumentimplementation properly handles errors across both platforms with user alerts. The button can safely render on all platforms.
|
🎉 This PR is included in version 0.8.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
1 participant
Closes #98
Summary
This PR fixes several issues and improves path handling across iOS and Android platforms.
Changes
JavaScript/TypeScript
iOS
isValidPath,pathToURL)extparameter from basename functionAndroid
extparameter from basename functionAPI Changes
basename(path: string, ext?: string)→basename(path: string)(removed optional ext parameter)Testing
Summary by CodeRabbit
New Features
Improvements
Documentation