Open
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:
📝 WalkthroughWalkthroughAdds a new Effection Api-based filesystem surface Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant FsApi as fsApi (Api<FsApiCore>)
participant Middleware as Middleware (around)
participant Impl as FS Implementation
Caller->>FsApi: readTextFile(path)
FsApi->>Middleware: invoke around hook
Middleware->>FsApi: proceed (maybe modify args/response)
FsApi->>Impl: actual readTextFile underlying call
Impl-->>FsApi: file contents / error
FsApi-->>Middleware: return to hook
Middleware-->>Caller: final response (possibly mocked)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 inconclusive)
✅ Passed checks (3 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 |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fs/package.json (1)
8-13:⚠️ Potential issue | 🟠 MajorAdd the required
filesfield for publishing.The package policy requires
filesto includedist,mod.ts, and source files. Please add the field (adjust the source path if different).📦 Proposed `files` field
"exports": { ".": { "development": "./mod.ts", "default": "./dist/mod.js" } }, + "files": [ + "dist", + "mod.ts", + "src" + ],As per coding guidelines, “package.json must include
filesfield containingdist,mod.ts, and source files.”
🤖 Fix all issues with AI agents
In `@fs/fs.test.ts`:
- Around line 207-271: The "fsApi middleware" describe block (the describe that
contains tests using fsApi.around and readTextFile) is outside the main
"@effectionx/fs" suite so it doesn't inherit the suite's cleanup; either move
that entire describe block (the "fsApi middleware" tests) inside the main
"@effectionx/fs" describe so it inherits its beforeEach cleanup, or add a local
beforeEach in the "fsApi middleware" block that performs the same directory
teardown used by the main suite; ensure references to fsApi.around,
readTextFile, and any created test files (e.g. middleware-read.txt,
middleware-scope.txt, mocked.json) are still correct after relocation.
In `@fs/package.json`:
- Around line 14-16: Bump the package version and add the missing files field:
update the "version" field in package.json to "0.3.0" (since the
peerDependencies change is breaking for a 0.x package) and add a "files" array
containing the distribution and source entries required by guidelines (include
"dist", "mod.ts", and the modified source files such as "fs/mod.ts",
"fs.test.ts" and "README.md") so package.json contains the "peerDependencies"
change plus the new "version" and the required "files" field.
In `@fs/README.md`:
- Around line 6-8: The README's version note (the sentence starting "Starting
with version 0.3.0") doesn't match the package.json version; update one of them
so they agree. Either bump the package.json "version" field to 0.3.0 (or newer)
to match the README, or change the README note to the actual package.json
version and adjust the Effection compatibility line accordingly; ensure the
README text referencing `fsApi` and `createApi` remains accurate after the
change.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
23-36: 🧹 Nitpick | 🔵 TrivialTODO comments already track this cleanup need.
The preview URL
pkg.pr.new/thefrontside/effection@1116(resolving to Effection4.1.0-alpha.3-pr) is appropriately used for development. Cleanup is already being tracked in test files—stream-helpers/valve.test.tsandeffect-ts/effect.test.tsboth include TODO comments noting that tests will need re-enabling once Effection 4.1.0 is stable. Thefspackage explicitly documents the alpha dependency requirement in itspackage.jsonandREADME.md, making the temporary nature clear.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 23 - 36, Leave the temporary Effection preview dependency in place: keep the "effection" dependency entry and the "pnpm.overrides.effection" override pointing to https://pkg.pr.new/thefrontside/effection@1116, and do not remove or change them; ensure the tests that depend on this (stream-helpers/valve.test.ts and effect-ts/effect.test.ts) retain their TODO comments about re-enabling when Effection 4.1.0 is stable and confirm the fs package documents the alpha requirement in its package.json and README.md.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@package.json`:
- Around line 23-36: Leave the temporary Effection preview dependency in place:
keep the "effection" dependency entry and the "pnpm.overrides.effection"
override pointing to https://pkg.pr.new/thefrontside/effection@1116, and do not
remove or change them; ensure the tests that depend on this
(stream-helpers/valve.test.ts and effect-ts/effect.test.ts) retain their TODO
comments about re-enabling when Effection 4.1.0 is stable and confirm the fs
package documents the alpha requirement in its package.json and README.md.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@effect-ts/effect-runtime.ts`:
- Around line 113-141: startManaged currently calls
runPromise(controller.signal) directly, so a synchronous throw can skip the
Promise-based settlement and pending bookkeeping; change the call site in
startManaged to invoke runPromise via a promise boundary (e.g.,
Promise.resolve().then(() => runPromise(controller.signal))) so any sync throw
becomes a rejected promise, assign that normalized promise to the local variable
used for execution.settled and for the returned promise, and ensure
pending.add(execution) happens after the promise variable is set so the
settled/finally handler always runs and removes the execution from pending.
In `@fs/README.md`:
- Around line 6-8: Update the README note so it states that Effection v4.1 or
greater is required for this package (not only for “full functionality”); edit
the sentence that mentions Effection v4.1+ and the fsApi/createApi reference to
assert that the package peer dependency requires Effection >= 4.1 for general
use and compatibility (replace the “for full functionality” phrasing with a
clear requirement).
In `@package.json`:
- Line 23: The global pnpm override "pnpm.overrides.effection": "4.1.0-alpha.7"
forces 24 workspace packages onto an alpha release; either scope or document
this change: replace the single global override with per-package overrides for
only the packages that require the alpha (e.g., set overrides only for
`@effectionx/timebox`, `@effectionx/worker`, etc.), or add an inline comment and a
removal plan (date or issue/PR reference) next to pnpm.overrides.effection in
package.json so reviewers know it is temporary and which rollout it supports;
ensure the change references the exact override key pnpm.overrides.effection and
the affected package names in the PR description or package.json comment.
In `@stream-helpers/valve.test.ts`:
- Around line 11-13: Re-enable the skipped test "closes and opens the valve" by
removing it.skip and replace any timing-based waits with deterministic
coordination: instrument the Valve under test to emit or expose explicit events
(or use the existing valve API) so the test waits for well-defined signals
(e.g., "closed" / "opened" events or Promises) rather than setTimeout/sleep, and
drive the test using those signals so the generator test (the function* in the
"closes and opens the valve" spec) deterministically yields on those events;
update assertions to run after the corresponding signal resolves to restore
coverage without flaky timing.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bb454650-8c13-476e-adee-cfe311b43f54
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
effect-ts/effect-runtime.tseffect-ts/package.jsonfs/README.mdfs/fs.test.tsfs/mod.tsfs/package.jsonpackage.jsonstream-helpers/valve.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fs/mod.ts (1)
311-315: 🧹 Nitpick | 🔵 Trivial
walk()uses directfsp.readdirwithwithFileTypesoption instead of routing throughFsApi.The
FsApi.operations.readdirreturnsstring[], butwalk()needsDirent[]to determine file type properties (isDirectory,isFile,isSymlink). The direct call tofsp.readdir(dir, { withFileTypes: true })at line 314 is necessary for functionality but bypasses middleware interception. Consider documenting this design decision or adding areaddirWithTypesoperation toFsApiCoreif walk operations should support middleware instrumentation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fs/mod.ts` around lines 311 - 315, walkDir (used by walk()) currently calls fsp.readdir(dir, { withFileTypes: true }) directly, bypassing FsApi middleware; add a new FsApiCore operation (e.g., readdirWithTypes) that returns Dirent[] and wire it into FsApi.operations so walkDir can yield* until(FsApi.operations.readdirWithTypes(dir)) instead of calling fsp directly, update the operation signature and any middleware hooks to accept/emit Dirent[] results, and ensure types and tests are updated accordingly; alternatively, if bypass is intentional, add a short comment in walk()/walkDir explaining why direct fsp.readdir(withFileTypes) is required and cannot be routed through the existing readdir string[] operation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@fs/mod.ts`:
- Around line 311-315: walkDir (used by walk()) currently calls fsp.readdir(dir,
{ withFileTypes: true }) directly, bypassing FsApi middleware; add a new
FsApiCore operation (e.g., readdirWithTypes) that returns Dirent[] and wire it
into FsApi.operations so walkDir can yield*
until(FsApi.operations.readdirWithTypes(dir)) instead of calling fsp directly,
update the operation signature and any middleware hooks to accept/emit Dirent[]
results, and ensure types and tests are updated accordingly; alternatively, if
bypass is intentional, add a short comment in walk()/walkDir explaining why
direct fsp.readdir(withFileTypes) is required and cannot be routed through the
existing readdir string[] operation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 97c8e016-50e7-4023-b1f0-96b0387ea42c
📒 Files selected for processing (4)
fs/api.tsfs/fs.test.tsfs/mod.tsfs/package.json
cowboyd
reviewed
Mar 16, 2026
taras
commented
Mar 27, 2026
Introduce @effectionx/context-api integration for @effectionx/fs, allowing middleware interception of filesystem operations. Use pure function handlers (not generators) per review feedback, FsHandler naming convention, bump version to 0.3.0.
3 tasks
cowboyd
reviewed
Apr 10, 2026
- Widens Fs interface params to string | URL and moves toPath into each handler so stat() and FsApi.operations.stat() share a single codepath (addresses review feedback on PR #160). - Promotes exists, ensureDir, ensureFile, emptyDir to the Fs interface so each gets its own middleware seam. ensureFile and emptyDir yield to FsApi.operations internally so nested middleware composes. - Adds cwd operation for mockable working-directory access. - Renames the api scope name to @effectionx/fs. - Collapses mod.ts: wrapper functions are gone; public operations come from destructuring FsApi.operations. walk/expandGlob/globToRegExp and URL helpers stay as-is. - Adds middleware tests for cwd, exists, ensureDir, ensureFile, emptyDir.
- Cast args[0] to String when pushing to string[] since the handler param is now string | URL. - Remove generator star from cwd middleware; cwd returns plain string so Middleware<[], string> requires a sync function, not a generator.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Add middleware support to
@effectionx/fsusing Effection'screateApipattern, enabling logging, mocking, and instrumentation of file system operations for testing and observability.This completes the series of PRs converting effectionx packages to use the new
createApipattern:@effectionx/fetch- feat(fetch): add fetchApi with middleware support #158@effectionx/process- feat(process): add processApi with middleware support #159@effectionx/fs- this PRApproach
FsApiwith core file system operations that middleware can intercept:stat,lstat- file metadatareadTextFile,writeTextFile- file I/Orm- file/directory removalreaddir- directory listing