Fix: node to use chunking for uploads #1291
Conversation
WalkthroughNode language type for file parameters changed from Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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). (18)
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: 1
🤖 Fix all issues with AI agents
In @templates/node/src/inputFile.ts.twig:
- Around line 2-4: The module imports Node fs APIs at top-level (basename,
realpathSync, promises as fs) which breaks edge runtimes; change to
conditional/dynamic loading: remove top-level fs/realpathSync imports and
instead import or require them inside methods that need filesystem access (e.g.,
in InputFile.fromPath, InputFile.fromFileSystem) guarded by a runtime check/flag
similar to client.ts.twig, and provide safe fallbacks for edge environments
(e.g., have fromBuffer remain usable without fs, have realpathSync return the
input path or undefined in edge, and throw a clear error when file-system-only
methods are invoked in an edge runtime). Ensure references to
basename/realpathSync/fs are only used after the runtime check or dynamic import
so edge environments never attempt to load the Node fs module.
🧹 Nitpick comments (1)
templates/node/src/client.ts.twig (1)
217-261: LGTM - consider extracting shared chunking logic.The
InputFilechunked upload implementation correctly handles size detection, chunk slicing, progress tracking, and header propagation. The async/await usage withInputFilemethods is appropriate.The chunking logic between
InputFile(lines 217-261) andFile(lines 263-301) is largely duplicated. Consider extracting a shared helper, though this is optional given the different sync/async APIs.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/SDK/Language/Node.phptemplates/node/src/client.ts.twigtemplates/node/src/inputFile.ts.twigtemplates/node/src/services/template.ts.twig
🔇 Additional comments (6)
src/SDK/Language/Node.php (1)
76-77: LGTM!The union type
File | InputFilecorrectly allows both the standardFiletype and the newInputFiletype for file parameters, enabling the chunked upload functionality while maintaining backward compatibility.templates/node/src/services/template.ts.twig (1)
4-15: LGTM!The conditional import logic correctly detects file parameters across all service methods and only imports
InputFilewhen necessary, keeping the generated code clean and avoiding unused imports.templates/node/src/client.ts.twig (1)
209-211: LGTM!The file detection correctly handles both
FileandInputFileinstances, enabling the chunked upload to work with either type.templates/node/src/inputFile.ts.twig (3)
25-50: LGTM!The
fromBufferfactory correctly handles multiple input types with appropriate type detection:
- BlobLike via duck-typing (
arrayBuffermethod check)- Buffer instances
- Strings (encoded to Buffer)
- ArrayBuffer (wrapped in Buffer)
- TypedArrays (extracted with proper offset/length)
The fallback error for unsupported types provides clear feedback.
72-93: LGTM!The
slicemethod efficiently handles all source types:
- Path-based: Uses file handles with proper cleanup via
try/finally, and correctly handles partial reads- Buffer: Uses
subarrayfor zero-copy slicing- Blob: Converts slice to Buffer via
arrayBufferThis enables true streaming for file-based sources without loading the entire file into memory.
95-109: LGTM!The
toFileandtoBuffermethods correctly materialize the content for scenarios requiring full file access, such as small uploads that bypass chunking.
| import { basename } from "path"; | ||
| import { realpathSync } from "fs"; | ||
| import { promises as fs } from "fs"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine the file in question
fd "inputFile.ts.twig" --type fRepository: appwrite/sdk-generator
Length of output: 139
🏁 Script executed:
# Check the repository structure to understand the project
ls -la
cat README.md | head -100Repository: appwrite/sdk-generator
Length of output: 5331
🏁 Script executed:
# Look for any edge runtime or environment-specific configurations
rg -i "edge|cloudflare|vercel|worker" --max-count 20Repository: appwrite/sdk-generator
Length of output: 1812
🏁 Script executed:
# Check if there's package.json to understand the project
cat package.json | head -50Repository: appwrite/sdk-generator
Length of output: 110
🏁 Script executed:
# Read the inputFile.ts.twig file to see realpathSync usage
cat -n templates/node/src/inputFile.ts.twigRepository: appwrite/sdk-generator
Length of output: 4153
🏁 Script executed:
# Read the client.ts.twig to see how edge runtimes are handled
cat -n templates/node/src/client.ts.twig | head -150Repository: appwrite/sdk-generator
Length of output: 5378
🏁 Script executed:
# Check if there's any documentation about edge runtime support or Node.js version compatibility
rg -i "edge.runtime|nodejs|node version|realpathSync" templates/node --type ts -A 3 -B 3Repository: appwrite/sdk-generator
Length of output: 48
🏁 Script executed:
# Check the templates directory structure to understand the organization
find templates/node -type f -name "*.twig" | head -20Repository: appwrite/sdk-generator
Length of output: 576
🏁 Script executed:
# Check the package.json template for export conditions or edge runtime support
cat -n templates/node/package.json.twig | head -60Repository: appwrite/sdk-generator
Length of output: 1638
🏁 Script executed:
# Check the index.ts to see what's exported
cat -n templates/node/src/index.ts.twigRepository: appwrite/sdk-generator
Length of output: 779
🏁 Script executed:
# Check README for edge runtime or browser support documentation
cat -n templates/node/README.md.twig | head -100Repository: appwrite/sdk-generator
Length of output: 1944
🏁 Script executed:
# Look for any conditional imports or environment checks in the Node.js template
rg "import|require" templates/node/src --type ts -B 2 -A 2Repository: appwrite/sdk-generator
Length of output: 48
🏁 Script executed:
# Check if there's any mention of edge runtime or browser support in Node SDK docs/code
rg -i "edge|browser|cloudflare|vercel" templates/node --type ts --type twigRepository: appwrite/sdk-generator
Length of output: 543
🏁 Script executed:
# Check tsconfig to understand compilation targets
cat -n templates/node/tsconfig.json.twigRepository: appwrite/sdk-generator
Length of output: 656
🏁 Script executed:
# Look for any tests or examples showing InputFile usage
find templates/node -name "*test*" -o -name "*example*" -o -name "*spec*" 2>/dev/null | head -10Repository: appwrite/sdk-generator
Length of output: 101
🏁 Script executed:
# Check if node-fetch-native-with-agent documents edge runtime support
rg -i "edge|cloudflare|worker" . --max-count 5Repository: appwrite/sdk-generator
Length of output: 1844
The entire inputFile.ts module is incompatible with edge runtimes due to fs module imports, not just realpathSync.
The issue extends beyond realpathSync blocking: the fs module import itself (lines 2-4) will fail in edge runtimes like Cloudflare Workers or Vercel Edge Functions that don't support Node.js filesystem APIs. This makes the entire InputFile class unavailable in these environments, even for methods like fromBuffer that don't require filesystem access.
Since client.ts.twig already includes edge runtime detection and warnings (lines 58-59, 119-122), the SDK is designed to support edge runtimes. Either add conditional imports for fs and realpathSync (gated by runtime detection) or provide fallback implementations for edge environments.
🤖 Prompt for AI Agents
In @templates/node/src/inputFile.ts.twig around lines 2 - 4, The module imports
Node fs APIs at top-level (basename, realpathSync, promises as fs) which breaks
edge runtimes; change to conditional/dynamic loading: remove top-level
fs/realpathSync imports and instead import or require them inside methods that
need filesystem access (e.g., in InputFile.fromPath, InputFile.fromFileSystem)
guarded by a runtime check/flag similar to client.ts.twig, and provide safe
fallbacks for edge environments (e.g., have fromBuffer remain usable without fs,
have realpathSync return the input path or undefined in edge, and throw a clear
error when file-system-only methods are invoked in an edge runtime). Ensure
references to basename/realpathSync/fs are only used after the runtime check or
dynamic import so edge environments never attempt to load the Node fs module.
There was a problem hiding this comment.
@copilot Based on this comment, make a PR that makes changes compatible with Edge. Goal is to prevent entire InputFile from breaking - instead, we just want exception at runtime if trying to use methods that require fs
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/Node18Test.php (1)
30-33: Consider documenting the expected chunk count.The test now expects exactly 4 large file upload responses, which aligns with the PR's chunking implementation. However, the hardcoded count of 4 chunks lacks context. If the test file size or CHUNK_SIZE constant changes, this expectation could silently break.
Consider adding a comment explaining why 4 chunks are expected (e.g., test file size ÷ chunk size), or better yet, verifying that the test file and chunk size configuration are documented to produce exactly 4 chunks.
💡 Optional: Improve comment clarity
- ...Base::LARGE_FILE_RESPONSES, // Large file uploads + ...Base::LARGE_FILE_RESPONSES, // Large file upload (4 chunks expected)tests/Node20Test.php (1)
30-33: Consider documenting the expected chunk count.The test expects exactly 4 large file upload responses for chunked uploads. While this aligns with the PR's implementation, the hardcoded count lacks documentation about why 4 chunks are expected.
Consider adding context about the test file size and chunk size that result in exactly 4 chunks, or document this relationship to prevent silent test failures if configurations change.
💡 Optional: Improve comment clarity
- ...Base::LARGE_FILE_RESPONSES, // Large file uploads + ...Base::LARGE_FILE_RESPONSES, // Large file upload (4 chunks expected)Note: The change is consistent across all Node version tests (16, 18, 20), which is good for maintainability.
tests/Node16Test.php (1)
30-33: Consider documenting the expected chunk count and reducing duplication.The test expects exactly 4 large file upload responses for chunked uploads, matching the pattern in Node18Test and Node20Test. While consistency across Node versions is excellent, the hardcoded count of 4 lacks documentation.
Additionally, since all three Node test classes (16, 18, 20) have identical chunk count expectations, consider:
- Documenting why 4 chunks are expected (test file size relationship to CHUNK_SIZE)
- Defining a constant in the Base class (e.g.,
LARGE_FILE_CHUNK_COUNT = 4) to make the expectation explicit and easier to maintain♻️ Optional: Improve comment clarity and reduce duplication
Option 1: Improve the comment
- ...Base::LARGE_FILE_RESPONSES, // Large file uploads + ...Base::LARGE_FILE_RESPONSES, // Large file upload (4 chunks expected)Option 2: Use a constant in Base class
In
tests/Base.php, add:protected const LARGE_FILE_CHUNK_COUNT = 4; // Large file test expects 4 chunks based on file size and CHUNK_SIZEThen in all Node test files:
...array_fill(0, Base::LARGE_FILE_CHUNK_COUNT, ...Base::LARGE_FILE_RESPONSES), // Large file upload chunks
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/Node16Test.phptests/Node18Test.phptests/Node20Test.php
🧰 Additional context used
🧬 Code graph analysis (2)
tests/Node18Test.php (1)
tests/Base.php (1)
Base(17-346)
tests/Node20Test.php (1)
tests/Base.php (1)
Base(17-346)
⏰ 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). (17)
- GitHub Check: build (8.3, Python39)
- GitHub Check: build (8.3, WebChromium)
- GitHub Check: build (8.3, PHP80)
- GitHub Check: build (8.3, Ruby30)
- GitHub Check: build (8.3, Node20)
- GitHub Check: build (8.3, PHP83)
- GitHub Check: build (8.3, WebNode)
- GitHub Check: build (8.3, Go118)
- GitHub Check: build (8.3, KotlinJava17)
- GitHub Check: build (8.3, Android5Java17)
- GitHub Check: build (8.3, DartBeta)
- GitHub Check: build (8.3, Android14Java17)
- GitHub Check: build (8.3, DotNet90)
- GitHub Check: build (8.3, FlutterBeta)
- GitHub Check: apple (client)
- GitHub Check: swift (server)
- GitHub Check: android (client)
What does this PR do?
Was technically pulling everything into memory.
Test Plan
N/A.
Related PRs and Issues
N/A.
Have you read the Contributing Guidelines on issues?
Yes.
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.