fix(platform): construct NodeCompatibleFileSystemAdapter safely when node:fs constants are absent - #3672
Conversation
…node:fs constants are absent Guard the `node:fs` constants deref so the adapter constructor doesn't throw in a non-Node runtime (e.g. a browser bundle that transitively imports it). Refs #3661.
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe filesystem adapter constructor now treats unavailable ChangesFilesystem capability handling
Estimated code review effort: 1 (Trivial) | ~5 minutes Mergeability Score: ⚪ Minimal · up to The change prevents adapter construction from crashing in runtimes without node:fs constants while preserving unavailable no-follow behavior; no actionable merge-blocking risk remains after normal checks and review. Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
… resolver Extract the default-noFollow resolution into `resolveNoFollowFlag(options, constants)` so the "constants unavailable" branch is testable without a `node:fs` mock: passing `undefined` for `constants` reproduces a non-Node runtime directly. Without the guard it throws `Cannot read properties of undefined (reading 'O_NOFOLLOW')`; with it, the flag degrades to `undefined`. Adds unit tests for the resolver (absent / present / own-seam) and an adapter-construction test asserting it builds without exact-snapshot support when the constants are absent. Refs #3661.
Bug
NodeCompatibleFileSystemAdapter's constructor reads anode:fsconstant unconditionally:In a non-Node runtime (e.g. a browser bundle that transitively imports this adapter),
nodeFsConstantsisundefined, so construction throws:…which aborts client hydration.
Issue
Surfaced by #3661 (a client-tree route that value-imports a server helper drags the runtime adapter graph into the browser bundle). Whether or not that leak should happen, the adapter constructor should not crash when
node:fsconstants are unavailable — the code already models this:NodeFileSystemCapabilityOptions.noFollowis documented as "An own undefined value means unavailable."Reproduction
Against this repo's source via LOCALDEV,
pages-server-import-leak/vector-ain mattboon/veryfront-router-testing:Fix
Optional-chain the constant deref, matching the documented "undefined means unavailable" contract. When absent,
noFollowisundefined→canOpenExactSnapshotisfalse(correct in a browser), and the constructor no longer throws.Validation
deno test src/platform/adapters/runtime/shared/node-filesystem-adapter.test.ts→ 1 passed (37 steps), 0 failed.Defense-in-depth only: the deeper server→client leak that pulls this adapter into the browser at all is tracked separately in #3661.
Refs #3661.
Summary by CodeRabbit