Conversation
🦋 Changeset detectedLatest commit: 131bca0 The changes in this PR will be included in the next version bump. 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 |
utopyin
force-pushed
the
effect-v4
branch
2 times, most recently
from
a27d8de to
02a3f06
Compare
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughMigrates the codebase to Effect v4: replaces Effect.Service/Tag and FiberRefs patterns with ServiceMap.Service and a ServiceContextBridge, updates effect execution and error mapping, converts services/tests/examples to ServiceMap + Layer, and bumps package versions and CI triggers for the effect-v4 pre-release. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant EffectBuilder
participant ServiceContextBridge
participant ManagedRuntime
participant Database
Client->>EffectBuilder: invoke RPC handler
EffectBuilder->>ServiceContextBridge: getCurrentServices()
ServiceContextBridge-->>EffectBuilder: captured services or undefined
EffectBuilder->>ManagedRuntime: merge runtime.services() with captured services
ManagedRuntime->>ManagedRuntime: run effect (runPromiseExitWith / runPromiseExit)
ManagedRuntime->>Database: call Layer-provided DatabaseService
Database-->>ManagedRuntime: return query/insert result
ManagedRuntime-->>EffectBuilder: effect Exit (success/failure)
EffectBuilder->>EffectBuilder: map Exit.cause → ORPCError (toORPCErrorFromCause) on failure
EffectBuilder->>Client: return result or mapped ORPCError
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 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🧪 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 |
utopyin
force-pushed
the
effect-v4
branch
2 times, most recently
from
13ffad5 to
8896026
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/effect-orpc/package.json (1)
57-63:⚠️ Potential issue | 🟠 MajorBound the
effectpeer range to<5.
>=4.0.0-beta.27allows future5.xreleases when published. Since the package explicitly targets Effect v4 (version1.0.0-v4.0, commit "prerelease for effect-v4") and devDependencies uses^4.0.0-beta.27to lock to v4, the peer range should match. Add an upper bound to prevent package managers from satisfying the peer with an unsupported major version.Suggested fix
"peerDependencies": { "@orpc/client": ">=1.13.0", "@orpc/contract": ">=1.13.0", "@orpc/server": ">=1.13.0", "@orpc/shared": ">=1.13.0", - "effect": ">=4.0.0-beta.27", + "effect": ">=4.0.0-beta.27 <5", "typescript": "^5" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/effect-orpc/package.json` around lines 57 - 63, Update the peerDependencies range for the effect package so it forbids Effect v5; specifically change the "effect" entry currently set to ">=4.0.0-beta.27" to a bounded range that matches devDependencies and the package intent (e.g. ">=4.0.0-beta.27 <5") so consumers cannot satisfy the peer with an incompatible major version; edit the "peerDependencies" block in package.json where "effect" is declared to apply this upper bound.README.md (1)
317-321:⚠️ Potential issue | 🟡 MinorUpdate terminology to reflect the migration.
Line 321 mentions "FiberRef state" but the code has migrated to
ServiceMap-based context. The earlier section (line 105) correctly mentions "ServiceMap.Reference-backed values". Consider updating this section to use consistent terminology:📝 Suggested wording update
-`FiberRef` state from outer middleware. +service context from outer middleware.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 317 - 321, Update the wording in the "Request-Scoped Fiber Context" section to replace references to "FiberRef state" with the current terminology "ServiceMap.Reference-backed values" (or simply "ServiceMap-based context"), so the README consistently refers to ServiceMap.Reference-backed values throughout; specifically change the sentence mentioning "FiberRef state" to mention "ServiceMap.Reference-backed values" and ensure any nearby examples/phrasing align with earlier text that references ServiceMap.Reference.
🧹 Nitpick comments (2)
.changeset/fluffy-times-shave.md (1)
5-5: Add one concrete migration note here.This body will become the release note/changelog entry, and right now it only repeats the title. For a major bump, include at least one actionable breaking-change detail so consumers know what to update first.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.changeset/fluffy-times-shave.md at line 5, Replace the placeholder body in .changeset/fluffy-times-shave.md with a concrete migration note for "migrate to effect-v4 (effect-smol)" that lists at least one actionable breaking change and how to address it (e.g., package rename, renamed/removed functions, changes to effect runtime setup or import paths); mention the exact symbol(s) to update in downstream code (for example old-package->new-package or function names that changed) and a short example migration step like which imports to replace and how to initialize the new runtime so consumers can apply the change immediately.packages/effect-orpc/src/node.ts (1)
19-25: Consider renamingwithFiberContextto reflect new semantics.The function now captures
ServiceMapviaEffect.services<R>()rather thanFiberRefs. The internal types (ServiceContextBridge,servicesStorage) have been updated, but the public function name still references "FiberContext". For API clarity, consider renaming towithServiceContextor similar.♻️ Suggested rename
-export function withFiberContext<T, R = never>( +export function withServiceContext<T, R = never>( fn: () => Promise<T>, ): Effect.Effect<T, never, R> { return Effect.flatMap(Effect.services<R>(), (services) => Effect.promise(() => servicesStorage.run(services, fn)), ); } + +/** `@deprecated` Use withServiceContext instead */ +export const withFiberContext = withServiceContext;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/effect-orpc/src/node.ts` around lines 19 - 25, The exported function withFiberContext now captures a ServiceMap via Effect.services<R>() and uses ServiceContextBridge/servicesStorage, so rename the public API and internal references to reflect services (e.g., withServiceContext) to avoid the "Fiber" misnomer; update the function name (withFiberContext → withServiceContext), any re-exports, type annotations, and all usages/imports across the codebase, and ensure the exported signature and JSDoc/comments reference Effect.services, ServiceContextBridge, and servicesStorage consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/pre.json:
- Around line 1-10: Run the project's formatter (oxfmt) on the changeset
pre-release JSON to normalize formatting and fix CI; specifically run the
formatter that fixes .changeset/pre.json, save the updated file, and commit the
changes so the keys "mode", "tag", "initialVersions", and "changesets" remain
intact and pass format:check.
- Around line 4-6: Update the prerelease metadata so Changesets computes the
correct prerelease state: open the JSON object key initialVersions.effect-orpc
in .changeset/pre.json and change its value from "0.1.1" to the latest pre-v4
release "0.1.2" to match the CHANGELOG entry.
In `@examples/hono-request-context/services/cache.ts`:
- Around line 27-29: The SQL builds a query by interpolating the variable key
into the string (in the db.query call), which can encourage SQL injection;
change the call to use a parameterized query instead (use
DatabaseService.query/db.query parameter binding rather than string
concatenation), passing the SQL with a placeholder and supplying key as a
separate parameter, and remove the template literal concatenation around key so
the query is executed with proper binding.
In `@packages/effect-orpc/src/types/index.ts`:
- Around line 135-139: Replace the non-existent Effect.gen.Return usage with
Effect.fn.Return for the generator return type in the ORPC handler signature:
update the return type currently written as Effect.gen.Return<THandlerOutput,
EffectErrorMapToUnion<TEffectErrorMap> | ORPCError<ORPCErrorCode, unknown>,
TRequirementsProvided> to use Effect.fn.Return with the same type parameters so
the generator body type for Effect.fnUntraced is correct; locate the signature
that references THandlerOutput, EffectErrorMapToUnion<TEffectErrorMap> |
ORPCError<ORPCErrorCode, unknown>, and TRequirementsProvided and swap gen.Return
for fn.Return.
In `@README.md`:
- Around line 97-131: This README contains a duplicate "Request-Scoped Context"
section that repeats the "Request-Scoped Fiber Context" content; remove the
redundant section and consolidate both into a single canonical section titled
"Request-Scoped Fiber Context" (or choose one title) keeping the example that
references withFiberContext and the effect-orpc/node entrypoint, preserving the
note about not needing the /node entrypoint when context propagation isn't
required, and update any internal links or table-of-contents references so they
point to the single retained section.
---
Outside diff comments:
In `@packages/effect-orpc/package.json`:
- Around line 57-63: Update the peerDependencies range for the effect package so
it forbids Effect v5; specifically change the "effect" entry currently set to
">=4.0.0-beta.27" to a bounded range that matches devDependencies and the
package intent (e.g. ">=4.0.0-beta.27 <5") so consumers cannot satisfy the peer
with an incompatible major version; edit the "peerDependencies" block in
package.json where "effect" is declared to apply this upper bound.
In `@README.md`:
- Around line 317-321: Update the wording in the "Request-Scoped Fiber Context"
section to replace references to "FiberRef state" with the current terminology
"ServiceMap.Reference-backed values" (or simply "ServiceMap-based context"), so
the README consistently refers to ServiceMap.Reference-backed values throughout;
specifically change the sentence mentioning "FiberRef state" to mention
"ServiceMap.Reference-backed values" and ensure any nearby examples/phrasing
align with earlier text that references ServiceMap.Reference.
---
Nitpick comments:
In @.changeset/fluffy-times-shave.md:
- Line 5: Replace the placeholder body in .changeset/fluffy-times-shave.md with
a concrete migration note for "migrate to effect-v4 (effect-smol)" that lists at
least one actionable breaking change and how to address it (e.g., package
rename, renamed/removed functions, changes to effect runtime setup or import
paths); mention the exact symbol(s) to update in downstream code (for example
old-package->new-package or function names that changed) and a short example
migration step like which imports to replace and how to initialize the new
runtime so consumers can apply the change immediately.
In `@packages/effect-orpc/src/node.ts`:
- Around line 19-25: The exported function withFiberContext now captures a
ServiceMap via Effect.services<R>() and uses
ServiceContextBridge/servicesStorage, so rename the public API and internal
references to reflect services (e.g., withServiceContext) to avoid the "Fiber"
misnomer; update the function name (withFiberContext → withServiceContext), any
re-exports, type annotations, and all usages/imports across the codebase, and
ensure the exported signature and JSDoc/comments reference Effect.services,
ServiceContextBridge, and servicesStorage consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 56e55bb5-a550-4419-bb71-c9d47a16a00a
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
.changeset/fluffy-times-shave.md.changeset/pre.json.github/workflows/publish.ymlREADME.mdexamples/README.mdexamples/hono-request-context/README.mdexamples/hono-request-context/index.tsexamples/hono-request-context/package.jsonexamples/hono-request-context/runtime.tsexamples/hono-request-context/services/cache.tsexamples/hono-request-context/services/database.tsexamples/hono-request-context/services/order.tspackages/effect-orpc/CHANGELOG.mdpackages/effect-orpc/package.jsonpackages/effect-orpc/src/effect-builder.tspackages/effect-orpc/src/effect-enhance-router.tspackages/effect-orpc/src/fiber-context-bridge.tspackages/effect-orpc/src/node.tspackages/effect-orpc/src/service-context-bridge.tspackages/effect-orpc/src/tagged-error.tspackages/effect-orpc/src/tests/effect-builder.test.tspackages/effect-orpc/src/tests/tagged-error.test.tspackages/effect-orpc/src/types/index.ts
💤 Files with no reviewable changes (1)
- packages/effect-orpc/src/fiber-context-bridge.ts
utopyin
force-pushed
the
effect-v4
branch
5 times, most recently
from
e0e28ce to
8a0a1b8
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
packages/effect-orpc/src/types/index.ts (1)
135-139:⚠️ Potential issue | 🔴 CriticalStill using the wrong Effect v4 generator return alias.
The Effect v4 API docs expose
ReturnunderEffect.fn, notEffect.gen. This exported handler type should useEffect.fn.Return<...>here rather thanEffect.gen.Return<...>. (effect-ts.github.io)Suggested fix
-) => Effect.gen.Return< +) => Effect.fn.Return<🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/effect-orpc/src/types/index.ts` around lines 135 - 139, The exported handler type is using the wrong Effect v4 generator alias; replace Effect.gen.Return with Effect.fn.Return in the handler return type so the signature becomes (... ) => Effect.fn.Return<THandlerOutput, EffectErrorMapToUnion<TEffectErrorMap> | ORPCError<ORPCErrorCode, unknown>, TRequirementsProvided>; update the type at the declaration referencing this handler (the function type that currently ends with Effect.gen.Return) to use Effect.fn.Return and ensure any imports/usage align with Effect v4's Effect.fn API.
🧹 Nitpick comments (3)
packages/effect-orpc/src/node.ts (1)
19-24: Consider renamingwithFiberContextto reflect new semantics.The function now propagates
ServiceMaprather thanFiberRefs, but retains the namewithFiberContext. This could be confusing for users. Consider renaming towithServiceContextor adding a JSDoc explaining the semantic change.💡 Optional rename suggestion
-export function withFiberContext<T, R = never>( +export function withServiceContext<T, R = never>( fn: () => Promise<T>, ): Effect.Effect<T, never, R> { return Effect.flatMap(Effect.services<R>(), (services) => servicesStorage.run(services, fn), ); } + +/** `@deprecated` Use withServiceContext instead */ +export const withFiberContext = withServiceContext;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/effect-orpc/src/node.ts` around lines 19 - 24, Rename or document the function to reflect that it propagates a ServiceMap rather than FiberRefs: either rename withFiberContext to withServiceContext (and update all references/exports/usages accordingly) or add a clear JSDoc above withFiberContext describing that it runs the provided fn with the services from servicesStorage (a ServiceMap) rather than fiber-local refs; ensure the JSDoc mentions servicesStorage and ServiceMap so callers understand the new semantics.examples/hono-request-context/services/order.ts (1)
55-57: Derive the logged order count instead of hard-coding3.This annotation will drift the next time
HARDCODED_ORDERSchanges and can make the logs misleading. Use the collection size so the telemetry stays accurate.Possible tweak
yield* Effect.logInfo("Listing all orders").pipe( - Effect.annotateLogs({ count: 3 }), + Effect.annotateLogs({ + count: Object.keys(HARDCODED_ORDERS).length, + }), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/hono-request-context/services/order.ts` around lines 55 - 57, Replace the hard-coded annotation value with the actual number of orders: compute the collection size (e.g., using HARDCODED_ORDERS.length or the variable that holds the orders result) and pass that value into Effect.annotateLogs instead of 3; update the logging call that uses Effect.logInfo(...).pipe(Effect.annotateLogs(...)) so the annotation derives from the orders collection size (reference HARDCODED_ORDERS and the Effect.annotateLogs call to locate the change).packages/effect-orpc/src/tests/effect-builder.test.ts (1)
166-298: Please keep the new coverage on Bun's test runner.These ServiceMap cases are useful, but adding them to this Vitest-based
*.test.tssuite pushes the migration further away from the repo standard. Please port the added coverage to Bun's test APIs before expanding this suite again.As per coding guidelines,
**/*.test.{ts,tsx,js,jsx}: Usebun testinstead ofjestorvitestfor running tests.Also applies to: 405-445, 605-640
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/effect-orpc/src/tests/effect-builder.test.ts` around lines 166 - 298, The new tests in effect-builder.test.ts use Vitest APIs and should be ported to Bun's test runner: extract the added ServiceMap cases (the tests exercising builder.effect/applied["~effect"].handler, withFiberContext, makeEffectORPC, ManagedRuntime, RequestId and the Counter Service class) into a Bun-compatible test file using Bun.test/Bun.assert style (or the repo's existing Bun test helpers), remove Vitest-specific imports/assertions, and ensure the tests still provide RequestId via Effect.provideService and dispose ManagedRuntime instances; keep the same assertions and behavior but replace Vitest APIs so the coverage remains runnable under bun test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/hono-request-context/package.json`:
- Around line 9-19: Update the dependency specifiers for the Effect beta
packages to exact versions to avoid prerelease drift: replace the caret ranges
for "@effect/opentelemetry" and "effect" (currently "^4.0.0-beta.27") with exact
pinned versions "4.0.0-beta.27" so installs cannot pull a newer beta; ensure you
update the entries for "@effect/opentelemetry" and "effect" in package.json
accordingly and run a reinstall to verify lockfile consistency.
In `@examples/hono-request-context/services/cache.ts`:
- Around line 1-50: The file has formatting issues causing the pipeline to fail;
run the project's formatter (oxfmt) on this file and commit the corrected
formatting. Specifically, format examples/hono-request-context/services/cache.ts
(affecting the CacheService class and its make generator, and the get/set
methods) by running oxfmt (or the repo's formatting script) to auto-fix
whitespace/line breaks and then re-run the pipeline; ensure the
CacheService.layer, make, get, and set symbols remain unchanged except for
formatting-only edits.
In `@packages/effect-orpc/src/effect-builder.ts`:
- Around line 663-672: The call to the non-existent Effect.runPromiseExitWith
should be replaced: detect when parentServices is present and either (a) provide
the merged services into the effect via Effect.provide(ServiceMap.merge(await
runtime.services(), parentServices)) on tracedEffect before running with
runtime.runPromiseExit, or (b) construct a custom runtime with merged services
and call Runtime.runPromiseExit(customRuntime)(tracedEffect, { signal:
opts.signal }); update the branch that currently uses Effect.runPromiseExitWith
to use one of these two approaches (refer to parentServices, ServiceMap.merge,
tracedEffect, runtime.runPromiseExit and Runtime.runPromiseExit) so the code
compiles against Effect v4 API.
- Around line 101-134: The function toORPCErrorFromCause incorrectly treats
Cause as having a reasons array; update it to use Effect v4's Cause API (e.g.,
use Cause.failureOption(), Cause.squash(), or Cause.match()) to extract the
underlying failure/die/interrupt instead of accessing cause.reasons[0]; then
apply the existing guards (Cause.isFailReason, Cause.isDieReason) or pattern
branches on the matched result to construct and return the proper ORPCError
(preserve existing branches for Fail → check isORPCTaggedError/instanceof
ORPCError, Die → include defect, and Interrupt → create an Interrupted error).
In `@packages/effect-orpc/src/node.ts`:
- Around line 19-24: The code calls a nonexistent Effect.services<R>() inside
withFiberContext; replace this with a supported Effect v4 pattern: either fetch
the specific services you need via Effect.all(...) or the documented helpers
(Effect.serviceOption / Effect.serviceOptional) or implement a small custom
services() helper for this bridge, then pass those retrieved services into
servicesStorage.run(..., fn); update references to Effect.services in
withFiberContext to use the chosen approach and ensure servicesStorage.run still
receives the same service map shape.
---
Duplicate comments:
In `@packages/effect-orpc/src/types/index.ts`:
- Around line 135-139: The exported handler type is using the wrong Effect v4
generator alias; replace Effect.gen.Return with Effect.fn.Return in the handler
return type so the signature becomes (... ) => Effect.fn.Return<THandlerOutput,
EffectErrorMapToUnion<TEffectErrorMap> | ORPCError<ORPCErrorCode, unknown>,
TRequirementsProvided>; update the type at the declaration referencing this
handler (the function type that currently ends with Effect.gen.Return) to use
Effect.fn.Return and ensure any imports/usage align with Effect v4's Effect.fn
API.
---
Nitpick comments:
In `@examples/hono-request-context/services/order.ts`:
- Around line 55-57: Replace the hard-coded annotation value with the actual
number of orders: compute the collection size (e.g., using
HARDCODED_ORDERS.length or the variable that holds the orders result) and pass
that value into Effect.annotateLogs instead of 3; update the logging call that
uses Effect.logInfo(...).pipe(Effect.annotateLogs(...)) so the annotation
derives from the orders collection size (reference HARDCODED_ORDERS and the
Effect.annotateLogs call to locate the change).
In `@packages/effect-orpc/src/node.ts`:
- Around line 19-24: Rename or document the function to reflect that it
propagates a ServiceMap rather than FiberRefs: either rename withFiberContext to
withServiceContext (and update all references/exports/usages accordingly) or add
a clear JSDoc above withFiberContext describing that it runs the provided fn
with the services from servicesStorage (a ServiceMap) rather than fiber-local
refs; ensure the JSDoc mentions servicesStorage and ServiceMap so callers
understand the new semantics.
In `@packages/effect-orpc/src/tests/effect-builder.test.ts`:
- Around line 166-298: The new tests in effect-builder.test.ts use Vitest APIs
and should be ported to Bun's test runner: extract the added ServiceMap cases
(the tests exercising builder.effect/applied["~effect"].handler,
withFiberContext, makeEffectORPC, ManagedRuntime, RequestId and the Counter
Service class) into a Bun-compatible test file using Bun.test/Bun.assert style
(or the repo's existing Bun test helpers), remove Vitest-specific
imports/assertions, and ensure the tests still provide RequestId via
Effect.provideService and dispose ManagedRuntime instances; keep the same
assertions and behavior but replace Vitest APIs so the coverage remains runnable
under bun test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0a668fc2-da26-4af9-8fab-275bb32911ae
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
.changeset/fluffy-times-shave.md.changeset/pre.json.github/workflows/publish.ymlREADME.mdexamples/README.mdexamples/hono-request-context/README.mdexamples/hono-request-context/index.tsexamples/hono-request-context/package.jsonexamples/hono-request-context/runtime.tsexamples/hono-request-context/services/cache.tsexamples/hono-request-context/services/database.tsexamples/hono-request-context/services/order.tspackages/effect-orpc/CHANGELOG.mdpackages/effect-orpc/package.jsonpackages/effect-orpc/src/effect-builder.tspackages/effect-orpc/src/effect-enhance-router.tspackages/effect-orpc/src/fiber-context-bridge.tspackages/effect-orpc/src/node.tspackages/effect-orpc/src/service-context-bridge.tspackages/effect-orpc/src/tagged-error.tspackages/effect-orpc/src/tests/effect-builder.test.tspackages/effect-orpc/src/tests/tagged-error.test.tspackages/effect-orpc/src/types/index.ts
💤 Files with no reviewable changes (1)
- packages/effect-orpc/src/fiber-context-bridge.ts
✅ Files skipped from review due to trivial changes (2)
- examples/hono-request-context/README.md
- packages/effect-orpc/CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (6)
- examples/README.md
- .changeset/pre.json
- packages/effect-orpc/src/tagged-error.ts
- packages/effect-orpc/src/tests/tagged-error.test.ts
- .github/workflows/publish.yml
- examples/hono-request-context/runtime.ts
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
packages/effect-orpc/src/effect-builder.ts (2)
101-134:⚠️ Potential issue | 🔴 Critical
cause.reasonsdoes not exist in Effect v4 Cause API.The
toORPCErrorFromCausefunction accessescause.reasons[0], but in Effect v4,Causeis a tree-like structure and does not expose areasonsarray. UseCause.match()for exhaustive pattern matching instead.🐛 Proposed fix using Cause.match
function toORPCErrorFromCause( cause: Cause.Cause<unknown>, ): ORPCError<string, unknown> { - const reason = cause.reasons[0]; - - if (reason === undefined) { - return new ORPCError("INTERNAL_SERVER_ERROR"); - } - - if (Cause.isDieReason(reason)) { - return new ORPCError("INTERNAL_SERVER_ERROR", { - cause: reason.defect, - }); - } - - if (Cause.isFailReason(reason)) { - const error = reason.error; - - if (isORPCTaggedError(error)) { - return error.toORPCError(); - } - if (error instanceof ORPCError) { - return error; - } - - return new ORPCError("INTERNAL_SERVER_ERROR", { - cause: error, - }); - } - - return new ORPCError("INTERNAL_SERVER_ERROR", { - cause: new Error(`${reason.fiberId} Interrupted`), - }); + return Cause.match(cause, { + onEmpty: () => new ORPCError("INTERNAL_SERVER_ERROR"), + onFail: (error) => { + if (isORPCTaggedError(error)) { + return error.toORPCError(); + } + if (error instanceof ORPCError) { + return error; + } + return new ORPCError("INTERNAL_SERVER_ERROR", { cause: error }); + }, + onDie: (defect) => + new ORPCError("INTERNAL_SERVER_ERROR", { cause: defect }), + onInterrupt: (fiberId) => + new ORPCError("INTERNAL_SERVER_ERROR", { + cause: new Error(`${String(fiberId)} Interrupted`), + }), + onSequential: (left, right) => left, + onParallel: (left, right) => left, + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/effect-orpc/src/effect-builder.ts` around lines 101 - 134, The function toORPCErrorFromCause incorrectly accesses cause.reasons; replace that logic with Cause.match to exhaustively handle Cause cases: use Cause.match(cause, { empty: () => new ORPCError("INTERNAL_SERVER_ERROR"), die: (defect) => new ORPCError("INTERNAL_SERVER_ERROR", { cause: defect }), fail: (error) => { if (isORPCTaggedError(error)) return error.toORPCError(); if (error instanceof ORPCError) return error; return new ORPCError("INTERNAL_SERVER_ERROR", { cause: error }); }, interrupt: (fiberId) => new ORPCError("INTERNAL_SERVER_ERROR", { cause: new Error(`${fiberId} Interrupted`) }) }); ensure you reference toORPCErrorFromCause, Cause.match, isORPCTaggedError, and ORPCError when making the change.
663-672:⚠️ Potential issue | 🔴 Critical
Effect.runPromiseExitWithdoes not exist in Effect v4 API and will cause a runtime error.The method
Effect.runPromiseExitWithis not part of the Effect v4 public API. To provide merged services before running the effect, wrap the mergedServiceMapwithLayer.succeedContext()and useEffect.provide():Correct fix using Layer.succeedContext
const parentServices = getCurrentServices(); const exit = parentServices - ? await Effect.runPromiseExitWith( - ServiceMap.merge(await runtime.services(), parentServices), - )(tracedEffect, { - signal: opts.signal, - }) + ? await runtime.runPromiseExit( + Effect.provide( + tracedEffect, + Layer.succeedContext( + ServiceMap.merge(await runtime.services(), parentServices), + ), + ), + { signal: opts.signal }, + ) : await runtime.runPromiseExit(tracedEffect, { signal: opts.signal, });
Layer.succeedContext()converts the mergedServiceMapinto aLayerthat can be provided to the effect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/effect-orpc/src/effect-builder.ts` around lines 663 - 672, The code incorrectly calls Effect.runPromiseExitWith (which doesn't exist); instead when parentServices is present, convert the merged ServiceMap (ServiceMap.merge(await runtime.services(), parentServices)) into a Layer via Layer.succeedContext() and provide it to the tracedEffect using Effect.provide (or Effect.provideSomeLayer) before running the effect with the runtime; keep the existing fallback that uses runtime.runPromiseExit(tracedEffect, { signal: opts.signal }) when parentServices is falsy, and ensure you still pass opts.signal to the final run call.
🧹 Nitpick comments (2)
examples/hono-request-context/package.json (1)
10-14: OpenTelemetry package versions are correctly aligned from the same coordinated release cycle; consider updating to the latest stable versions.The exporter version
0.211.0and SDK packages at2.5.0were released on the same day (2026-01-21), confirming they are from the same coordinated release cycle as intended. However, newer stable versions are available:0.213.0(exporter) and2.6.0(SDK packages), both released on 2026-03-03. While the current versions are compatible, consider updating to the latest stable releases for the latest features and patches.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/hono-request-context/package.json` around lines 10 - 14, Update the OpenTelemetry dependency versions in package.json so the exporter and SDK packages use the latest coordinated stable releases; change "@opentelemetry/exporter-trace-otlp-http" from "^0.211.0" to "^0.213.0" and update "@opentelemetry/sdk-metrics", "@opentelemetry/sdk-trace-base", "@opentelemetry/sdk-trace-node", and "@opentelemetry/sdk-trace-web" from "^2.5.0" to "^2.6.0" to align with the newer stable release cycle.examples/hono-request-context/services/order.ts (1)
50-51: Inconsistent log annotation key.
getOrderuses"service"as the annotation key (line 50), whilelistOrdersuses"app-service"(line 78). Consider using a consistent annotation key across both methods for uniform log filtering.♻️ Suggested fix for consistency
}).pipe( - Effect.annotateLogs("app-service", "OrderService"), + Effect.annotateLogs("service", "OrderService"), Effect.withSpan("OrderService.listOrders"), ),Also applies to: 78-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/hono-request-context/services/order.ts` around lines 50 - 51, The log annotation key is inconsistent between OrderService.getOrder and OrderService.listOrders: update the Effect.annotateLogs call in either getOrder or listOrders so both use the same key (e.g., change "service" to "app-service" or vice versa) to ensure uniform log filtering; locate the annotateLogs usage in the OrderService methods (Effect.annotateLogs("service", "OrderService") and Effect.annotateLogs("app-service", "OrderService")) and make them identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/hono-request-context/runtime.ts`:
- Around line 17-23: Replace the invalid Effect v4 calls Logger.layer and
Logger.consolePretty by using the pre-built logger Layer (e.g., Logger.pretty)
when constructing LoggerLive; update the AppLive merge to use that LoggerLive
and keep merging NodeSdkLive and OrderService.layer as before—specifically
change the LoggerLive definition (currently using
Logger.layer([Logger.consolePretty()])) to use Logger.pretty (or attach extra
loggers via Logger.add) so Layer.mergeAll(AppLive, LoggerLive, NodeSdkLive,
OrderService.layer) uses a valid Layer instance.
In `@packages/effect-orpc/CHANGELOG.md`:
- Line 15: The release entry uses an inconsistent version tag "1.0.0-v4.0";
update that tag to match the pattern used by later entries by renaming
"1.0.0-v4.0" to "1.0.0-effect-v4.0" in the CHANGELOG entry so it is consistent
with "1.0.0-effect-v4.1" and "1.0.0-effect-v4.2".
In `@packages/effect-orpc/src/effect-builder.ts`:
- Line 23: Update the import of IntersectPick to use the canonical package:
replace any import of IntersectPick from "@orpc/server" with an import from
"@orpc/shared" (e.g., change the import statement in effect-builder.ts and
types/variants.ts to import type { IntersectPick } from "@orpc/shared") so all
modules consistently source IntersectPick from the shared package.
In `@packages/effect-orpc/src/node.ts`:
- Around line 19-24: withFiberContext currently calls the removed
Effect.services<R>() API; replace this with the v4 service/tag pattern by
introducing (or importing) a Context.Tag/Effect.Service for the "services"
object and retrieving it via the v4 API before running servicesStorage.run.
Concretely: define a Tag (e.g., ServicesTag = Context.Tag<Services>() or export
an Effect.Service for the services type), then change the implementation to
Effect.flatMap(Effect.service(ServicesTag), (services) => Effect.promise(() =>
servicesStorage.run(services, fn))). Ensure the function signature and types
align with the new Tag/Service type and keep the calls to servicesStorage.run
and Effect.promise/Effect.flatMap intact.
---
Duplicate comments:
In `@packages/effect-orpc/src/effect-builder.ts`:
- Around line 101-134: The function toORPCErrorFromCause incorrectly accesses
cause.reasons; replace that logic with Cause.match to exhaustively handle Cause
cases: use Cause.match(cause, { empty: () => new
ORPCError("INTERNAL_SERVER_ERROR"), die: (defect) => new
ORPCError("INTERNAL_SERVER_ERROR", { cause: defect }), fail: (error) => { if
(isORPCTaggedError(error)) return error.toORPCError(); if (error instanceof
ORPCError) return error; return new ORPCError("INTERNAL_SERVER_ERROR", { cause:
error }); }, interrupt: (fiberId) => new ORPCError("INTERNAL_SERVER_ERROR", {
cause: new Error(`${fiberId} Interrupted`) }) }); ensure you reference
toORPCErrorFromCause, Cause.match, isORPCTaggedError, and ORPCError when making
the change.
- Around line 663-672: The code incorrectly calls Effect.runPromiseExitWith
(which doesn't exist); instead when parentServices is present, convert the
merged ServiceMap (ServiceMap.merge(await runtime.services(), parentServices))
into a Layer via Layer.succeedContext() and provide it to the tracedEffect using
Effect.provide (or Effect.provideSomeLayer) before running the effect with the
runtime; keep the existing fallback that uses
runtime.runPromiseExit(tracedEffect, { signal: opts.signal }) when
parentServices is falsy, and ensure you still pass opts.signal to the final run
call.
---
Nitpick comments:
In `@examples/hono-request-context/package.json`:
- Around line 10-14: Update the OpenTelemetry dependency versions in
package.json so the exporter and SDK packages use the latest coordinated stable
releases; change "@opentelemetry/exporter-trace-otlp-http" from "^0.211.0" to
"^0.213.0" and update "@opentelemetry/sdk-metrics",
"@opentelemetry/sdk-trace-base", "@opentelemetry/sdk-trace-node", and
"@opentelemetry/sdk-trace-web" from "^2.5.0" to "^2.6.0" to align with the newer
stable release cycle.
In `@examples/hono-request-context/services/order.ts`:
- Around line 50-51: The log annotation key is inconsistent between
OrderService.getOrder and OrderService.listOrders: update the
Effect.annotateLogs call in either getOrder or listOrders so both use the same
key (e.g., change "service" to "app-service" or vice versa) to ensure uniform
log filtering; locate the annotateLogs usage in the OrderService methods
(Effect.annotateLogs("service", "OrderService") and
Effect.annotateLogs("app-service", "OrderService")) and make them identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7ac82094-af3b-49eb-b6e6-fbdd012617b4
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
.changeset/fluffy-times-shave.md.changeset/pre.json.changeset/silly-doodles-cover.md.github/workflows/publish.ymlREADME.mdexamples/README.mdexamples/hono-request-context/README.mdexamples/hono-request-context/index.tsexamples/hono-request-context/package.jsonexamples/hono-request-context/runtime.tsexamples/hono-request-context/services/cache.tsexamples/hono-request-context/services/database.tsexamples/hono-request-context/services/order.tspackages/effect-orpc/CHANGELOG.mdpackages/effect-orpc/package.jsonpackages/effect-orpc/src/effect-builder.tspackages/effect-orpc/src/effect-enhance-router.tspackages/effect-orpc/src/fiber-context-bridge.tspackages/effect-orpc/src/node.tspackages/effect-orpc/src/service-context-bridge.tspackages/effect-orpc/src/tagged-error.tspackages/effect-orpc/src/tests/effect-builder.test.tspackages/effect-orpc/src/tests/tagged-error.test.tspackages/effect-orpc/src/types/index.ts
💤 Files with no reviewable changes (1)
- packages/effect-orpc/src/fiber-context-bridge.ts
✅ Files skipped from review due to trivial changes (2)
- .changeset/silly-doodles-cover.md
- .changeset/fluffy-times-shave.md
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/effect-orpc/src/tagged-error.ts
- packages/effect-orpc/src/tests/tagged-error.test.ts
- .github/workflows/publish.yml
- packages/effect-orpc/src/service-context-bridge.ts
- .changeset/pre.json
utopyin
force-pushed
the
effect-v4
branch
2 times, most recently
from
4d8224c to
56a3bbd
Compare
utopyin
force-pushed
the
effect-v4
branch
5 times, most recently
from
cc04cc1 to
131bca0
Compare
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 by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Tests