Restore FileSystem.watch recursive control - #6705
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRestores optional recursive control to ChangesFilesystem watch options
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant FileSystem
participant NodeFileSystem
participant NativeWatcher
Caller->>FileSystem: watch(path, { recursive })
FileSystem->>NodeFileSystem: forward watch options
NodeFileSystem->>NativeWatcher: configure recursive watcher
NativeWatcher-->>FileSystem: emit filesystem event
FileSystem-->>Caller: emit WatchEvent
Suggested labels: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Comment |
🦋 Changeset detectedLatest commit: a15fc71 The changes in this PR will be included in the next version bump. This PR includes changesets to release 29 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Fixed the actionable Check failures in 977d23e: updated the new tests to the v4 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/platform-node-shared/test/NodeFileSystem.test.ts (1)
19-22: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the omitted-options default.
This verifies only
recursive: false; add a case withfs.watch(root)and confirm nested changes remain excluded, since omitted options are also required to default to non-recursive watching.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/platform-node-shared/test/NodeFileSystem.test.ts` around lines 19 - 22, Add a test alongside the existing fs.watch coverage that calls fs.watch(root) without options and verifies changes in nested paths are not reported, preserving the expected non-recursive default behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/platform-node-shared/test/NodeFileSystem.test.ts`:
- Around line 19-24: Add an explicit readiness barrier after starting the
watcher in both tests before any file writes occur. Update the watcher setup
around fs.watch, Stream.runHead, and Effect.forkChild so the test waits until
NFS.watch has registered and the stream is actively pulling, then proceed with
the existing write operations.
---
Nitpick comments:
In `@packages/platform-node-shared/test/NodeFileSystem.test.ts`:
- Around line 19-22: Add a test alongside the existing fs.watch coverage that
calls fs.watch(root) without options and verifies changes in nested paths are
not reported, preserving the expected non-recursive default behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 520eecd6-6929-43b9-9f6f-3daa20f93917
📒 Files selected for processing (2)
packages/effect/src/FileSystem.tspackages/platform-node-shared/test/NodeFileSystem.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/effect/src/FileSystem.ts
github-project-automation
Bot
moved this from Discussion Ongoing
to Waiting on Author
in PR Backlog
|
Addressed the latest review in 5724b7c: the watch tests now establish a real readiness handshake by repeatedly touching a sentinel until the stream observes it, and the sentinel is filtered from assertions. I also added explicit coverage for |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/platform-node-shared/test/NodeFileSystem.test.ts`:
- Around line 18-35: Update the watcher setup around the `fiber`,
`Deferred.await(ready)`, and `signalFiber` lifecycle so watcher errors or normal
closure before observing `readyName` cause the readiness wait to fail or exit
instead of hanging. Link the readiness wait to `fiber` completion, and ensure
`signalFiber` is interrupted whether readiness succeeds or the watcher
terminates unexpectedly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7e690f2e-2ca5-4a57-8dcc-462b66a6e98a
📒 Files selected for processing (1)
packages/platform-node-shared/test/NodeFileSystem.test.ts
|
Addressed the watcher-lifecycle review in d67acc0. The readiness wait now races the watcher fiber’s completion, so startup errors or early closure surface immediately, and an |
Head branch was pushed to by a user without write access
|
Fixed the Node CI timeout in a15fc71. The readiness signal used the test clock, so after its first pre-watcher write it never retried; it now delays on the live clock before writing, giving the watcher fiber time to acquire. Verified the focused NodeFileSystem suite 10 consecutive times on Linux/Node 26 (18/18 each), plus oxlint, dprint, and the package TypeScript build. |
Bundle Size Analysis
|
2 participants
Summary
WatchOptionsAPI onFileSystem.watchWatchBackendand the Node/Deno native implementationsfalseoptions to non-recursive watchingWhy
The v4 migration removed the watch options and hard-coded recursive watching in the Node-compatible implementation used by Node and Bun. That silently widened existing watchers to entire directory trees and could trigger extra rebuilds or feedback loops.
Root cause
The established option from #5174 was dropped while the filesystem API moved into
effect, and native watchers were changed torecursive: trueinstead of forwarding caller intent.Impact
fs.watch(path)andfs.watch(path, { recursive: false })again observe only direct children. Passing{ recursive: true }opts into subtree events.Verification
recursive: falseandrecursive: truemainCloses #6698
Summary by CodeRabbit
FileSystem.watchto accept optionalWatchOptions(includingrecursive?: boolean) and threaded options through the Node and Deno implementations.recursivehandling for filesystem watching; non-recursive watching is now the default whenrecursiveisn’t specified.recursive: truevs non-recursive (including the default).