Feat/upgrade nitro#40
Conversation
- Bump versions of nitro-codegen and react-native-nitro-modules to 0.26.1 - Update Podfile.lock to reflect NitroFS and NitroModules version changes - Refactor code generation command in package.json to include a new 'postcodegen' script - Adjust file copy logic in App.tsx for better clarity and functionality
WalkthroughThe updates include dependency version bumps for "react-native-nitro-modules" and "nitro-codegen" in both the main and example package files, a new npm script addition, a correction to an error message in Swift code, and a change in file path handling and logging within an example TypeScript file. Changes
Possibly related PRs
Poem
📜 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 (2)
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. 🪧 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 (
|
…t-native-nitro-modules version
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
bun.lockis excluded by!**/*.lockexample/ios/Podfile.lockis excluded by!**/*.locknitrogen/generated/ios/NitroFS-Swift-Cxx-Umbrella.hppis excluded by!**/generated/**
📒 Files selected for processing (4)
example/App.tsx(1 hunks)example/package.json(1 hunks)ios/HybridNitroFs.swift(1 hunks)package.json(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
example/App.tsx (1)
src/specs/nitro-fs.nitro.ts (1)
NitroFS(5-125)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Android Example App (new architecture)
- GitHub Check: Build Android Example App (old architecture)
🔇 Additional comments (4)
package.json (2)
16-17: LGTM: Script improvements enhance the build process.The addition of the trailing comma in the codegen script follows JSON formatting conventions, and the new
postcodegenscript automates pod installation in the example directory after code generation, which streamlines the development workflow.
60-60:Details
❓ Verification inconclusive
Verify dependency versions for security and compatibility.
The upgrades from 0.25.2 to 0.26.1 for both
nitro-codegenandreact-native-nitro-modulesalign with the PR objectives. However, please verify these versions are valid and free from security vulnerabilities.Also applies to: 64-64
🏁 Script executed:
#!/bin/bash # Description: Verify the dependency versions exist and check for security advisories echo "Checking nitro-codegen version 0.26.1..." npm view nitro-codegen@0.26.1 version 2>/dev/null || echo "Version 0.26.1 not found for nitro-codegen" echo "Checking react-native-nitro-modules version 0.26.1..." npm view react-native-nitro-modules@0.26.1 version 2>/dev/null || echo "Version 0.26.1 not found for react-native-nitro-modules" echo "Checking for security advisories..." npm audit --audit-level=moderate --json | jq '.vulnerabilities | keys[]' 2>/dev/null || echo "No audit data available"Length of output: 876
Confirm dependency availability and audit vulnerabilities
- nitro-codegen@0.26.1 and react-native-nitro-modules@0.26.1 are both published on npm and resolve correctly.
- No security advisories were reported because there is no lockfile in the sandbox. Please generate one and rerun the audit to ensure there are no moderate-or-higher vulnerabilities.
Next steps:
- At the repo root, run:
npm install --package-lock-only npm audit --audit-level=moderate- Address any findings before merging.
Locations to verify:
- package.json: line 60 (
"nitro-codegen": "0.26.1",)- package.json: line 64 (
"react-native-nitro-modules": "0.26.1",)example/package.json (1)
16-16: LGTM: Dependency version aligned with root package.The upgrade of
react-native-nitro-modulesto 0.26.1 maintains consistency with the root package.json dependency versions.ios/HybridNitroFs.swift (1)
44-44: LGTM: Error message correction improves debugging accuracy.The error message now correctly reflects the actual operation (reading vs writing), which will make debugging file operation failures much clearer.
| NitroFS.copy( | ||
| fileToUpload, | ||
| NitroFS.DOCUMENT_DIR + `/testDir/test${Date.now()}.txt`, | ||
| NitroFS.DOCUMENT_DIR + `/testDir/${fileToUpload}`, |
There was a problem hiding this comment.
Fix destination path construction to avoid nested directories.
Using the full fileToUpload path as the filename will create unintended nested directory structures. For example, if fileToUpload is /documents/largefile.txt, the destination becomes /documents/testDir//documents/largefile.txt.
Extract just the filename from the source path:
- NitroFS.DOCUMENT_DIR + `/testDir/${fileToUpload}`,
+ NitroFS.DOCUMENT_DIR + `/testDir/${fileToUpload.split('/').pop()}`,Or alternatively, use a library like path for more robust path handling:
+import { basename } from 'path';
// Then in the copy call:
- NitroFS.DOCUMENT_DIR + `/testDir/${fileToUpload}`,
+ NitroFS.DOCUMENT_DIR + `/testDir/${basename(fileToUpload)}`,🤖 Prompt for AI Agents
In example/App.tsx at line 81, the destination path is constructed by
concatenating NitroFS.DOCUMENT_DIR with the full fileToUpload path, causing
unintended nested directories. To fix this, extract only the filename from
fileToUpload before appending it to the destination path, for example by using a
path handling library like 'path' to get the basename. Replace the current
concatenation with one that uses just the filename to avoid creating nested
directory structures.
|
🎉 This PR is included in version 0.5.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
1 participant
Summary by CodeRabbit