feat: DenoFileSystem - #6682
Merged
Merged
Conversation
🦋 Changeset detectedLatest commit: c90ffd7 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 |
|
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 with no reviewable changes (1)
📝 WalkthroughWalkthroughA reusable ChangesFilesystem conformance testing
Deno filesystem layer
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant FileSystemTest
participant DenoFileSystem
participant DenoFsFile
participant handleError
FileSystemTest->>DenoFileSystem: invoke filesystem operation
DenoFileSystem->>DenoFsFile: perform Deno filesystem call
DenoFsFile-->>DenoFileSystem: return result or error
DenoFileSystem->>handleError: translate Deno error
handleError-->>FileSystemTest: return Effect result or PlatformError
Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/effect/test/FileSystem.test-utils.ts (3)
15-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShadowed type parameter
E.
runPromise's<E, A>shadows the outer<E>fromtestLayer. Harmless, but rename for clarity.♻️ Suggested rename
- const runPromise = <E, A>(self: Effect.Effect<A, E, Fs.FileSystem>) => + const runPromise = <E2, A>(self: Effect.Effect<A, E2, Fs.FileSystem>) =>🤖 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/effect/test/FileSystem.test-utils.ts` around lines 15 - 19, Rename the inner error type parameter in the nested runPromise function within testLayer so it no longer shadows testLayer’s outer E parameter; update its Effect type annotation consistently while preserving the existing behavior.
79-80: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGuard against a missing path separator.
If neither separator is found,
separatoris-1anddirbecomes the path minus its last character, producing a confusing failure instead of a clear one.🛡️ Proposed fix
const separator = Math.max(file.lastIndexOf("/"), file.lastIndexOf("\\")) + assert(separator > 0, `expected a path separator in ${file}`) dir = file.slice(0, separator)🤖 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/effect/test/FileSystem.test-utils.ts` around lines 79 - 80, Update the path handling around the separator calculation to detect when neither "/" nor "\\" is present before slicing. Handle this missing-separator case explicitly with a clear failure, while preserving the existing directory extraction when a separator is found.
237-253: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider an opt-out for truncate cursor clamping.
POSIX
ftruncatedoesn't move the file offset, so clamping the cursor to the new length is an Effect-level guarantee that a backend may not reproduce. Given the harness already exposesTestLayerOptionsfor accepted divergences, adding a flag here would avoid a forced patch later when another runtime is wired in.🤖 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/effect/test/FileSystem.test-utils.ts` around lines 237 - 253, Add a `TestLayerOptions` opt-out for truncate cursor clamping, and update the test around `file.truncate` and the subsequent current-position `file.seek` to honor that option. Preserve the existing clamped-cursor expectation by default while allowing backends that follow POSIX `ftruncate` semantics to disable it.
🤖 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.
Nitpick comments:
In `@packages/effect/test/FileSystem.test-utils.ts`:
- Around line 15-19: Rename the inner error type parameter in the nested
runPromise function within testLayer so it no longer shadows testLayer’s outer E
parameter; update its Effect type annotation consistently while preserving the
existing behavior.
- Around line 79-80: Update the path handling around the separator calculation
to detect when neither "/" nor "\\" is present before slicing. Handle this
missing-separator case explicitly with a clear failure, while preserving the
existing directory extraction when a separator is found.
- Around line 237-253: Add a `TestLayerOptions` opt-out for truncate cursor
clamping, and update the test around `file.truncate` and the subsequent
current-position `file.seek` to honor that option. Preserve the existing
clamped-cursor expectation by default while allowing backends that follow POSIX
`ftruncate` semantics to disable it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 22ac5ca7-4180-4e41-bdd6-0a3bce29e213
📒 Files selected for processing (3)
packages/effect/test/FileSystem.test-utils.tspackages/effect/test/fixtures/text.txtpackages/platform-node-shared/test/NodeFileSystem.test.ts
Contributor
Bundle Size Analysis
|
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-deno/src/internal/error.ts`:
- Around line 21-43: Add explicit native Deno error-name branches in the error
mapping around the existing errno switch, covering BadResource, InvalidData,
TimedOut, UnexpectedEof, WouldBlock, and WriteZero before errno fallback; retain
the existing NotCapable handling. In
packages/platform-deno/src/internal/error.ts lines 21-43, map each name to its
corresponding normalized tag, and in
packages/platform-deno/test/internal/error.test.ts lines 9-17 add raw
native-error cases verifying these mappings.
🪄 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: 16471d33-949b-4f4d-87c1-2c58d0b9e04c
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
packages/effect/test/FileSystem.test-utils.tspackages/platform-deno/package.jsonpackages/platform-deno/src/internal/error.tspackages/platform-deno/test/internal/error.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/effect/test/FileSystem.test-utils.ts
github-project-automation
Bot
moved this from Discussion Ongoing
to Waiting on Author
in PR Backlog
tim-smart
commented
Jul 28, 2026
tim-smart
force-pushed
the
feat/deno-file-system
branch
from
July 28, 2026 09:42
f28533f to
d3efdd8
Compare
tim-smart
commented
Jul 28, 2026
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/effect/test/FileSystem.test-utils.ts`:
- Around line 29-40: Update the makeTempDirectory test and the corresponding
test around lines 93–108 to clean up every temporary directory or file created
during the test. Register a finalizer that removes the temporary resource after
assertions, including when assertions fail, while preserving the existing
persistence checks.
- Line 24: Replace the __dirname-based fixture paths in the test reads at lines
24, 116, 145, 161, and 185 with an ESM-safe path derived from import.meta.url,
using the existing fixture filenames and preserving the current readFile
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: eccb2a06-2df6-4986-8f65-2e0f85b3f7bd
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
.changeset/add-deno-file-system.mdpackages/effect/test/FileSystem.test-utils.tspackages/effect/test/fixtures/text.txtpackages/platform-deno/package.jsonpackages/platform-deno/src/DenoFileSystem.tspackages/platform-deno/src/index.tspackages/platform-deno/src/internal/error.tspackages/platform-deno/test/DenoFileSystem.test.tspackages/platform-deno/test/internal/error.test.tspackages/platform-node-shared/test/NodeFileSystem.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/platform-node-shared/test/NodeFileSystem.test.ts
- packages/platform-deno/package.json
- packages/platform-deno/test/internal/error.test.ts
- packages/platform-deno/src/internal/error.ts
- packages/effect/test/fixtures/text.txt
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/effect/test/FileSystem.test-utils.ts`:
- Around line 204-228: Make the interrupted-read test around readAlloc
deterministic by replacing the fixed-size file and millisecond timeout/sleep
assumptions with a controllable slow-read seam that reliably interrupts while
I/O is in flight, then waits for completion before seeking and validating cursor
restoration. If no such seam is available, move this timing-sensitive case out
of the cross-platform conformance test into an appropriate platform-specific
test.
🪄 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: 04350753-8009-4b56-ab32-209c15a970a9
📒 Files selected for processing (2)
packages/effect/test/FileSystem.test-utils.tspackages/platform-deno/src/DenoFileSystem.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/platform-deno/src/DenoFileSystem.ts
tim-smart
enabled auto-merge (squash)
July 28, 2026 10:57
This was referenced Jul 28, 2026
5 tasks
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
1 participant
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.
Summary
Testing
pnpm lint-fixpnpm --filter @effect/platform-node-shared test --run test/NodeFileSystem.test.tspnpm checkCloses EFF-139
Closes EFF-159
Summary by CodeRabbit
FileSystemimplementation and exposed it via the platform-deno package.FileSystemsuite to use it.FileSystemtests and expanded fixture, temp directory/file (including scoped cleanup), read/write/append, truncation, and cursor-behavior coverage.@std/fsdependency and published a patch release note.