feat(state-api): add @effectionx/state-api with useState#193
feat(state-api): add @effectionx/state-api with useState#193
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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new Changes
Sequence DiagramsequenceDiagram
actor User
participant useState as "useState Operation"
participant Signal as "Effection Signal"
participant API as "createApi / Core Handlers"
participant Middleware as "around() Middleware"
participant Subscriber as "Stream Subscriber"
User->>useState: invoke useState(initial, reducers?)
useState->>Signal: createSignal()
useState->>API: build core handlers (set/update/get + reducers)
useState->>User: return State object (Stream + ops + around)
User->>State: call operation (e.g., set(newValue))
State->>Middleware: around() intercepts operation
Middleware->>API: invoke core handler (possibly transformed)
API->>Signal: update internal state ref and send value
Signal->>Subscriber: emit new state value
Subscriber->>User: receive update
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 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 `@state-api/README.md`:
- Around line 24-53: The examples use yield* and identifiers like useState,
todos, and counter without showing the surrounding generator runtime, making
them non-copy/paste; either wrap each example in a complete runnable snippet
(e.g., run(function* () { /* example code using yield*, useState, todos, counter
*/ })) or add a one-line header to each block stating "continuation from
previous snippet" and include the minimal setup (e.g., const todos = yield*
useState(...) or a comment like // assumes run(generator) and todos already
defined) so readers can run examples directly; update the Typed reducers block
and the other affected blocks (mentions of todos, counter, yield*) accordingly.
In `@state-api/state.test.ts`:
- Line 2: Replace the timing-based sleep(0) test sync with a deterministic
"subscriber ready" resolver: in the tests that import sleep, withResolvers,
spawn, and each (notably the update() and reducer-stream tests referenced),
create a resolver via withResolvers() (e.g. const { resolve: markReady, promise:
ready } = withResolvers()), pass or expose that resolver to the subscriber
setup, have the subscriber call markReady() once it is subscribed/consuming, and
have the producer/creator await ready before emitting/dispatching; remove all
sleep(0) calls and use this ready promise to gate producer actions so tests
deterministically synchronize without timing-based sleeps.
In `@state-api/state.ts`:
- Around line 154-165: The reducer registration loop in state.ts assigns user
reducers into core using their keys, which allows user reducers named
set/update/get/around to shadow built-in operations; update the registration
code that iterates Object.keys(reducers) (the block that assigns core[key] =
function* (...) and the equivalent second reducer-registration block) to
validate each key against a denylist ["set","update","get","around"] and reject
— e.g., throw a clear Error — when a user attempts to register one of those
reserved names so built-in operations (core, api.operations) cannot be silently
overridden.
- Around line 171-177: The state object currently assigns the iterator by
copying the function reference ([Symbol.iterator]: signal[Symbol.iterator])
which loses the receiver and breaks stream semantics; replace that assignment
with a generator method on state that delegates to signal (i.e., implement a
*[Symbol.iterator]() { yield* signal; } style delegation) so the iterator runs
with the correct receiver and follows the established
.policies/stateless-streams.md pattern; update the code that builds state (the
object literal containing [Symbol.iterator], signal, and api.operations) to use
the generator method instead of copying signal[Symbol.iterator].
🪄 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: b250e5d3-d6a3-4e86-9c48-9e66e21e8bd7
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
effect-ts/effect-runtime.tseffect-ts/package.jsonpackage.jsonpnpm-workspace.yamlstate-api/README.mdstate-api/mod.tsstate-api/package.jsonstate-api/state.test.tsstate-api/state.tsstate-api/tsconfig.jsontsconfig.json
| @@ -0,0 +1,316 @@ | |||
| import { describe, it } from "@effectionx/bdd"; | |||
| import { each, scoped, sleep, spawn, withResolvers } from "effection"; | |||
There was a problem hiding this comment.
Replace sleep(0) synchronization with deterministic readiness signaling.
These waits are timing-based and violate the no-sleep test-sync policy. Gate producer actions on an explicit “subscriber ready” resolver instead.
🔧 Deterministic pattern to apply (example)
-import { each, scoped, sleep, spawn, withResolvers } from "effection";
+import { each, scoped, spawn, withResolvers } from "effection";
it("emits new state on set()", function* () {
const counter = yield* useState(0);
const values: number[] = [];
const { resolve, operation } = withResolvers<void>();
+ const { resolve: subscriberReady, operation: subscriberReadyOp } =
+ withResolvers<void>();
yield* spawn(function* () {
- for (const value of yield* each(counter)) {
+ const stream = yield* each(counter);
+ subscriberReady();
+ for (const value of stream) {
values.push(value);
if (values.length === 3) {
resolve();
}
yield* each.next();
}
});
- // allow subscriber to establish
- yield* sleep(0);
+ yield* subscriberReadyOp;
yield* counter.set(1);
yield* counter.set(2);
yield* counter.set(3);Apply the same pattern to the update() and reducer-stream tests currently using sleep(0).
As per coding guidelines: ".policies/no-sleep-test-sync.md - Tests must not use sleep() for waiting; use deterministic helpers" and "**/*.test.{ts,tsx}: Avoid sleep() for test synchronization".
Also applies to: 60-62, 86-87, 131-132
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@state-api/state.test.ts` at line 2, Replace the timing-based sleep(0) test
sync with a deterministic "subscriber ready" resolver: in the tests that import
sleep, withResolvers, spawn, and each (notably the update() and reducer-stream
tests referenced), create a resolver via withResolvers() (e.g. const { resolve:
markReady, promise: ready } = withResolvers()), pass or expose that resolver to
the subscriber setup, have the subscriber call markReady() once it is
subscribed/consuming, and have the producer/creator await ready before
emitting/dispatching; remove all sleep(0) calls and use this ready promise to
gate producer actions so tests deterministically synchronize without
timing-based sleeps.
There was a problem hiding this comment.
The no-sleep policy explicitly allows sleep(0) for yielding control (line 14). The suggested withResolvers pattern doesn't work with effection's each() — you can't split yield* each(stream) from the for...of to insert a readiness signal. Added comments clarifying policy compliance.
There was a problem hiding this comment.
🧩 Analysis chain
Let me verify the policy file to confirm this.
🏁 Script executed:
#!/bin/bash
cat -n .policies/no-sleep-test-sync.md
Length of output: 7434
@taras, you're right on both counts — I apologize for the incorrect review comment.
The policy table at line 14 explicitly lists "Yielding control (no delay)" → "sleep(0) is acceptable", and there's even a dedicated compliant example for it. The sleep(0) calls in these tests are precisely that pattern: yielding control to allow the spawned subscriber to run, not waiting for an async result.
Additionally, my suggested withResolvers workaround was unworkable — yield* each(counter) is consumed directly in the for...of header; there's no API surface to intercept between subscription and the start of iteration to insert a readiness signal.
✏️ Learnings added
Learnt from: taras
Repo: thefrontside/effectionx PR: 193
File: state-api/state.test.ts:2-2
Timestamp: 2026-03-15T00:10:37.158Z
Learning: In `state-api/state.test.ts` (and Effection tests generally), `sleep(0)` used before emitting values to a spawned subscriber is explicitly allowed by the no-sleep-test-sync policy (line 14). The policy only forbids `sleep(ms > 0)` used to *wait for* async results; `sleep(0)` for yielding control is a listed compliant pattern.
Learnt from: taras
Repo: thefrontside/effectionx PR: 193
File: state-api/state.test.ts:2-2
Timestamp: 2026-03-15T00:10:37.158Z
Learning: In Effection, `each()` cannot be split from its `for...of` loop to insert a readiness signal. The expression `for (const value of yield* each(stream))` is atomic — there is no API hook between subscription setup and the start of iteration, so the `withResolvers` "subscriber ready" pattern is not applicable to `each()`-based stream consumers.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: thefrontside/effectionx PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-08T18:16:06.004Z
Learning: Applies to **/*.test.{ts,tsx} : Import test utilities from 'effectionx/bdd' when needed in test files
Learnt from: CR
Repo: thefrontside/effectionx PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-23T17:32:37.408Z
Learning: Applies to **/*.test.ts : Import test utilities from `effectionx/bdd` when needed in test files
Learnt from: CR
Repo: thefrontside/effectionx PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-08T18:16:06.004Z
Learning: Applies to **/*.test.{ts,tsx} : Avoid 'sleep()' for test synchronization in accordance with No-Sleep Test Sync policy
Learnt from: CR
Repo: thefrontside/effectionx PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-23T17:32:37.408Z
Learning: Applies to **/*.{ts,tsx} : Use structured concurrency patterns with spawn and scope in Effection code
Learnt from: taras
Repo: thefrontside/effectionx PR: 179
File: durable-streams/each.ts:81-82
Timestamp: 2026-03-07T00:55:27.391Z
Learning: In `durable-streams/each.ts`, the module-level `activeState` variable shared between `durableEach` and `durableEach.next` is an intentional design decision. Durable execution is single-threaded (cooperative generators), so there is no concurrent access risk within a single coroutine chain. Using Effection context is not viable here because both `durableEach` and `durableEach.next` are wrapped in `ephemeral()`, which gives each its own child scope, making context invisible across them. A TODO is in place to revisit if Effection adds context that spans ephemeral scope boundaries.
Learnt from: CR
Repo: thefrontside/effectionx PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-08T18:16:06.004Z
Learning: Applies to **/*.{ts,tsx} : Use structured concurrency patterns (spawn, scope) in Effection code
Learnt from: CR
Repo: thefrontside/effectionx PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-02-18T13:51:01.407Z
Learning: Applies to **/*.{test,spec}.{ts,tsx,js,jsx} : Deterministic Tests - Write tests that produce consistent results regardless of execution order or timing (Experimental/Advisory)
Learnt from: taras
Repo: thefrontside/effectionx PR: 179
File: durable-streams/structured-concurrency.test.ts:262-279
Timestamp: 2026-03-07T00:55:58.397Z
Learning: In `durable-streams/structured-concurrency.test.ts` (and the durable-streams package generally), race tests using `durableRace` with immediately-resolving promises are deterministic under Effection's cooperative scheduling model. Effection processes spawned tasks in FIFO order, so the first branch (`root.0`) always wins when it completes in fewer effect steps than subsequent branches. The winner is structurally determined by effect count, not by wall-clock timing or scheduler luck.
Learnt from: CR
Repo: thefrontside/effectionx PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-23T17:32:37.408Z
Learning: Applies to **/*.{ts,tsx} : Use `*[Symbol.iterator]` pattern for reusable stream operations in Effection (see Stateless Streams policy)
Learnt from: CR
Repo: thefrontside/effectionx PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-08T18:16:06.004Z
Learning: Applies to **/*.{ts,tsx} : Use '*[Symbol.iterator]' pattern for reusable stream operations in accordance with Stateless Streams policy
Learnt from: CR
Repo: thefrontside/effectionx PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-02-18T13:51:01.407Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Structured Concurrency - Manage concurrent operations with explicit scoping and cancellation (Experimental/Advisory)
Learnt from: taras
Repo: thefrontside/effectionx PR: 180
File: durable-effects/node-runtime.ts:36-49
Timestamp: 2026-03-07T03:20:27.616Z
Learning: In `durable-effects/node-runtime.ts`, `effectionx/process` does not support a timeout parameter. The `timeout` field in `ExecOptions` is intentionally not forwarded to the underlying `exec()` call. Cancellation is instead handled via Effection's structured concurrency — when the parent scope is torn down, the child process is cleaned up automatically. A documentation comment was added to capture this limitation.
state-api/state.ts
Outdated
| const state = { | ||
| // Stream interface | ||
| [Symbol.iterator]: signal[Symbol.iterator], | ||
| // Middleware | ||
| around: api.around, | ||
| // Spread all operations (set, update, valueOf, + reducers) | ||
| ...api.operations, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Demonstrate why copying a generator method by reference changes its receiver.
node <<'NODE'
const source = {
label: "signal",
*[Symbol.iterator]() {
yield this.label;
},
};
const wrapped = {
[Symbol.iterator]: source[Symbol.iterator],
};
console.log([...source]); // ["signal"]
console.log([...wrapped]); // [undefined]
NODERepository: thefrontside/effectionx
Length of output: 93
🏁 Script executed:
cat -n state-api/state.ts | head -200 | tail -50Repository: thefrontside/effectionx
Length of output: 1890
🏁 Script executed:
head -5 state-api/state.ts && wc -l state-api/state.tsRepository: thefrontside/effectionx
Length of output: 159
🏁 Script executed:
cat .policies/stateless-streams.mdRepository: thefrontside/effectionx
Length of output: 3788
🏁 Script executed:
rg "signal" state-api/state.ts -B 5 -A 5 | head -80Repository: thefrontside/effectionx
Length of output: 1880
🏁 Script executed:
rg "createSignal" state-api/state.ts -B 2 -A 2Repository: thefrontside/effectionx
Length of output: 347
🏁 Script executed:
fd -e ts -e tsx -name "*.ts" | xargs grep -l "export.*createSignal" | head -5Repository: thefrontside/effectionx
Length of output: 297
🏁 Script executed:
rg "interface.*Stream|type.*Stream" --type ts | grep -v node_modules | head -20Repository: thefrontside/effectionx
Length of output: 1473
🏁 Script executed:
rg "export type State" state-api/state.ts -A 3Repository: thefrontside/effectionx
Length of output: 163
🏁 Script executed:
rg "export interface.*Stream" --type ts | head -10Repository: thefrontside/effectionx
Length of output: 313
Use *[Symbol.iterator]() generator method instead of copying the method reference.
The current [Symbol.iterator]: signal[Symbol.iterator] pattern loses receiver context. When the copied method executes, this points to state (due to how it's spread), not signal, breaking the stream protocol. Wrap the delegation in a generator method to preserve correct semantics:
Suggested fix
const state = {
// Stream interface
- [Symbol.iterator]: signal[Symbol.iterator],
+ *[Symbol.iterator]() {
+ yield* signal;
+ },
// Middleware
around: api.around,This aligns with .policies/stateless-streams.md (Established Policy) which requires stream operations to use the *[Symbol.iterator]() pattern for proper reusability and receiver handling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@state-api/state.ts` around lines 171 - 177, The state object currently
assigns the iterator by copying the function reference ([Symbol.iterator]:
signal[Symbol.iterator]) which loses the receiver and breaks stream semantics;
replace that assignment with a generator method on state that delegates to
signal (i.e., implement a *[Symbol.iterator]() { yield* signal; } style
delegation) so the iterator runs with the correct receiver and follows the
established .policies/stateless-streams.md pattern; update the code that builds
state (the object literal containing [Symbol.iterator], signal, and
api.operations) to use the generator method instead of copying
signal[Symbol.iterator].
There was a problem hiding this comment.
Tried *[Symbol.iterator]() { yield* signal; } — it breaks stream semantics. each() expects the iterator to return a Subscription with .next(), but yield* signal yields the subscription's values directly. The reference copy pattern matches all existing signal packages (boolean.ts, array.ts, set.ts) and works correctly because createSignal's iterator is a closure, not this-dependent.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how createSignal's iterator is implemented in effection
rg "createSignal|Symbol.iterator" --type ts -n -A 3 -B 3 | grep -v node_modules | head -80
# Check existing signal packages for the reference copy pattern taras mentions
for f in boolean.ts array.ts set.ts; do
fd -e ts "$f" | xargs grep -l "Symbol.iterator" 2>/dev/null | head -3
done
# Look at how each() and Subscription work in effection
rg "Subscription|\.next\(\)" --type ts -n | grep -v node_modules | head -30
Length of output: 6129
@taras, you're right — I apologize for the incorrect suggestion. The script output confirms the established codebase pattern: websocket/websocket.ts (line 162) and worker/worker.ts (line 305) both use the identical [Symbol.iterator]: signal[Symbol.iterator] reference copy. Since createSignal's iterator is closure-based rather than this-dependent, the copy is safe. Using *[Symbol.iterator]() { yield* signal; } would incorrectly bypass the Subscription object and directly yield values, breaking the each() protocol which expects a Subscription with .next(). The current code is correct.
✏️ Learnings added
Learnt from: taras
Repo: thefrontside/effectionx PR: 193
File: state-api/state.ts:177-183
Timestamp: 2026-03-15T00:10:44.596Z
Learning: In Effection's `createSignal`, the `[Symbol.iterator]` method is a closure-based implementation and is NOT `this`-dependent. It is safe and correct to copy the reference directly: `[Symbol.iterator]: signal[Symbol.iterator]`. This is the established codebase convention used in `websocket/websocket.ts` and `worker/worker.ts`. Do NOT suggest replacing it with `*[Symbol.iterator]() { yield* signal; }` — that pattern bypasses the `Subscription` wrapper and breaks `each()`, which expects a `Subscription` object with a `.next()` method, not directly yielded values.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: taras
Repo: thefrontside/effectionx PR: 179
File: durable-streams/each.ts:81-82
Timestamp: 2026-03-07T00:55:27.391Z
Learning: In `durable-streams/each.ts`, the module-level `activeState` variable shared between `durableEach` and `durableEach.next` is an intentional design decision. Durable execution is single-threaded (cooperative generators), so there is no concurrent access risk within a single coroutine chain. Using Effection context is not viable here because both `durableEach` and `durableEach.next` are wrapped in `ephemeral()`, which gives each its own child scope, making context invisible across them. A TODO is in place to revisit if Effection adds context that spans ephemeral scope boundaries.
Learnt from: CR
Repo: thefrontside/effectionx PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-08T18:16:06.004Z
Learning: Applies to **/*.{ts,tsx} : Use '*[Symbol.iterator]' pattern for reusable stream operations in accordance with Stateless Streams policy
Learnt from: CR
Repo: thefrontside/effectionx PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-23T17:32:37.408Z
Learning: Applies to **/*.{ts,tsx} : Use `*[Symbol.iterator]` pattern for reusable stream operations in Effection (see Stateless Streams policy)
Learnt from: CR
Repo: thefrontside/effectionx PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-08T18:16:06.004Z
Learning: Applies to **/*.{ts,tsx} : Prefer 'Operation<T>' for async operations in Effection
Learnt from: CR
Repo: thefrontside/effectionx PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-23T17:32:37.408Z
Learning: Applies to **/*.{ts,tsx} : Prefer `Operation<T>` for async operations in Effection code
Learnt from: CR
Repo: thefrontside/effectionx PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-08T18:16:06.004Z
Learning: Applies to **/*.{ts,tsx} : Use structured concurrency patterns (spawn, scope) in Effection code
Learnt from: CR
Repo: thefrontside/effectionx PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-23T17:32:37.408Z
Learning: Applies to **/*.{ts,tsx} : Use structured concurrency patterns with spawn and scope in Effection code
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@state-api/state.test.ts`:
- Around line 309-329: The test expectation is inverted: because createApi
composes middleware so each new around() wraps the prior one, the most recently
added middleware runs first; update the assertion in the "chains multiple
middleware" test to expect ["second","first"] (and optionally adjust the test
description) for the useState counter.around / counter.set case so it matches
the middleware composition behavior implemented in createApi.
🪄 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: 36de19c6-bd8f-43f4-9a18-ac80bcc4ee64
📒 Files selected for processing (3)
state-api/README.mdstate-api/state.test.tsstate-api/state.ts
Reactive state container with typed reducer actions and middleware support, built on Effection's experimental createApi. - useState<T>(initial) for simple state with set/update/get - useState<T, R>(initial, reducers) for typed reducer actions - All operations return Operation<T> (the resulting state) - State<T> implements Stream<T> for subscribing to changes - Per-container middleware via around() for any operation - Middleware is scoped and does not leak across scope boundaries
- Guard against reserved reducer names (set, update, get, around) that would shadow built-in operations - Add test for reserved name rejection - Add policy-compliance comments on sleep(0) usage in stream tests - Make README examples self-contained with run() wrappers Note: kept [Symbol.iterator] reference copy pattern — it matches all existing signal packages and works correctly because createSignal's iterator is a closure. The *[Symbol.iterator]() delegation pattern breaks stream semantics with each().
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@state-api/package.json`:
- Around line 25-27: The peer dependency range for effection in state-api's
package.json is too restrictive; update the "peerDependencies" entry for
"effection" to the repository standard range (e.g., "^3 || ^4") so it matches
other packages and restores broader compatibility—modify the "effection" value
under peerDependencies in package.json accordingly.
In `@state-api/state.ts`:
- Around line 182-183: Update the misleading inline comment near the spread of
api.operations: change the mention of the built-in operation name from "valueOf"
to "get" so it correctly reads something like "// Spread all operations (set,
update, get, + reducers)". Locate the comment adjacent to the spread of
api.operations and edit only the comment text; do not modify the code behavior
of the api.operations spread or any related function names.
- Around line 156-166: Validate the incoming reducers before wiring them: ensure
`reducers` is a non-null object (not an array) and that every property value is
a function, otherwise throw a clear initialization Error naming the invalid key
and its type; update the block that iterates `Object.keys(reducers)` (where
`core[key] = function* (...args: any[]): Operation<T>` and `ref.current =
reducer(ref.current, ...args)` are set) to perform these checks first and only
then bind the generator wrappers, so invalid shapes fail fast with descriptive
messages referencing the offending reducer key.
🪄 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: f6a6192d-88b2-4bda-9b86-1b3f9280c2bb
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
package.jsonpnpm-workspace.yamlstate-api/README.mdstate-api/mod.tsstate-api/package.jsonstate-api/state.test.tsstate-api/state.tsstate-api/tsconfig.jsontsconfig.json
state-api/state.ts
Outdated
| if (reducers) { | ||
| for (const key of Object.keys(reducers)) { | ||
| if (reserved.has(key)) { | ||
| throw new Error( | ||
| `Reducer name "${key}" is reserved. Built-in operations (set, update, get, around) cannot be overridden.`, | ||
| ); | ||
| } | ||
| const reducer = reducers[key]; | ||
| // biome-ignore lint/suspicious/noExplicitAny: reducer args are open-ended | ||
| core[key] = function* (...args: any[]): Operation<T> { | ||
| ref.current = reducer(ref.current, ...args); |
There was a problem hiding this comment.
Validate reducer map shape and entries at initialization.
At Line 156, any truthy reducers value is accepted, and invalid entries fail later when invoked. Validate that reducers is an object map and each value is a function before wiring core to fail early with clearer errors.
Suggested hardening
- if (reducers) {
- for (const key of Object.keys(reducers)) {
+ if (reducers !== undefined) {
+ if (reducers === null || typeof reducers !== "object" || Array.isArray(reducers)) {
+ throw new TypeError("`reducers` must be an object of reducer functions");
+ }
+ for (const key of Object.keys(reducers)) {
if (reserved.has(key)) {
throw new Error(
`Reducer name "${key}" is reserved. Built-in operations (set, update, get, around) cannot be overridden.`,
);
}
- const reducer = reducers[key];
+ const reducer = reducers[key];
+ if (typeof reducer !== "function") {
+ throw new TypeError(`Reducer "${key}" must be a function`);
+ }
// biome-ignore lint/suspicious/noExplicitAny: reducer args are open-ended
core[key] = function* (...args: any[]): Operation<T> {
ref.current = reducer(ref.current, ...args);As per coding guidelines: .policies/correctness-invariants.md - “Validate inputs; test success/error/halt paths”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@state-api/state.ts` around lines 156 - 166, Validate the incoming reducers
before wiring them: ensure `reducers` is a non-null object (not an array) and
that every property value is a function, otherwise throw a clear initialization
Error naming the invalid key and its type; update the block that iterates
`Object.keys(reducers)` (where `core[key] = function* (...args: any[]):
Operation<T>` and `ref.current = reducer(ref.current, ...args)` are set) to
perform these checks first and only then bind the generator wrappers, so invalid
shapes fail fast with descriptive messages referencing the offending reducer
key.
state-api/state.ts
Outdated
| // Spread all operations (set, update, valueOf, + reducers) | ||
| ...api.operations, |
There was a problem hiding this comment.
Fix stale built-in operation name in the comment.
At Line 182, the comment says valueOf, but the built-in operation is get. This is misleading during maintenance.
Suggested comment fix
- // Spread all operations (set, update, valueOf, + reducers)
+ // Spread all operations (set, update, get, + reducers)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Spread all operations (set, update, valueOf, + reducers) | |
| ...api.operations, | |
| // Spread all operations (set, update, get, + reducers) | |
| ...api.operations, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@state-api/state.ts` around lines 182 - 183, Update the misleading inline
comment near the spread of api.operations: change the mention of the built-in
operation name from "valueOf" to "get" so it correctly reads something like "//
Spread all operations (set, update, get, + reducers)". Locate the comment
adjacent to the spread of api.operations and edit only the comment text; do not
modify the code behavior of the api.operations spread or any related function
names.
cowboyd
left a comment
cowboyd
left a comment
There was a problem hiding this comment.
Seems quite legit!
The only problem is that there are way to many any ignores for it not to be a code smell. Something is not quite right with the types if there has to be that many. Usually, it means that there is either a lurking incoherence in the type model, or that the number of code-paths need to be reduced. Are we sure that these are necessary?
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@state-api/state.ts`:
- Around line 149-160: The reducer registration allows a user key of "__proto__"
which will mutate core's prototype and silently drop the reducer; update the
reserved names logic by adding "__proto__" to the reserved Set (the variable
reserved) or otherwise block keys equal to "__proto__" before assigning into
core (see the reducers loop and the core[key] = ... assignment) so that
"__proto__" cannot be registered as a reducer and the prototype cannot be
mutated.
🪄 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: 6a7a6824-c1f0-41c5-9733-6ec3046a3e61
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
state-api/package.jsonstate-api/state.tsstate-api/tsconfig.json
state-api/state.ts
Outdated
| const reserved = new Set(["set", "update", "get", "around"]); | ||
| if (reducers) { | ||
| for (const key of Object.keys(reducers)) { | ||
| if (reserved.has(key)) { | ||
| throw new Error( | ||
| `Reducer name "${key}" is reserved. Built-in operations (set, update, get, around) cannot be overridden.`, | ||
| ); | ||
| } | ||
| const reducer = reducers[key]; | ||
| // biome-ignore lint/suspicious/noExplicitAny: reducer args are open-ended | ||
| core[key] = function* (...args: any[]): Operation<T> { | ||
| ref.current = reducer(ref.current, ...args); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node <<'NODE'
const core = {
set() {},
update() {},
get() {},
};
const reducer = function reducer() {};
core["__proto__"] = reducer;
console.log({
ownKeys: Object.keys(core),
hasOwnProto: Object.prototype.hasOwnProperty.call(core, "__proto__"),
prototypeMutated: Object.getPrototypeOf(core) === reducer,
});
NODERepository: thefrontside/effectionx
Length of output: 156
Add __proto__ to the reserved reducer names.
At line 159, core[key] = ... writes user-defined reducer names into a plain object. If key === "__proto__", the special property setter mutates core's prototype instead of creating an own property, so the reducer operation is never registered and silently disappears at runtime.
Fix
- const reserved = new Set(["set", "update", "get", "around"]);
+ const reserved = new Set(["set", "update", "get", "around", "__proto__"]);
if (reducers) {
for (const key of Object.keys(reducers)) {
if (reserved.has(key)) {
throw new Error(
- `Reducer name "${key}" is reserved. Built-in operations (set, update, get, around) cannot be overridden.`,
+ `Reducer name "${key}" is reserved. Built-in operations and special object keys cannot be overridden.`,
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const reserved = new Set(["set", "update", "get", "around"]); | |
| if (reducers) { | |
| for (const key of Object.keys(reducers)) { | |
| if (reserved.has(key)) { | |
| throw new Error( | |
| `Reducer name "${key}" is reserved. Built-in operations (set, update, get, around) cannot be overridden.`, | |
| ); | |
| } | |
| const reducer = reducers[key]; | |
| // biome-ignore lint/suspicious/noExplicitAny: reducer args are open-ended | |
| core[key] = function* (...args: any[]): Operation<T> { | |
| ref.current = reducer(ref.current, ...args); | |
| const reserved = new Set(["set", "update", "get", "around", "__proto__"]); | |
| if (reducers) { | |
| for (const key of Object.keys(reducers)) { | |
| if (reserved.has(key)) { | |
| throw new Error( | |
| `Reducer name "${key}" is reserved. Built-in operations and special object keys cannot be overridden.`, | |
| ); | |
| } | |
| const reducer = reducers[key]; | |
| // biome-ignore lint/suspicious/noExplicitAny: reducer args are open-ended | |
| core[key] = function* (...args: any[]): Operation<T> { | |
| ref.current = reducer(ref.current, ...args); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@state-api/state.ts` around lines 149 - 160, The reducer registration allows a
user key of "__proto__" which will mutate core's prototype and silently drop the
reducer; update the reserved names logic by adding "__proto__" to the reserved
Set (the variable reserved) or otherwise block keys equal to "__proto__" before
assigning into core (see the reducers loop and the core[key] = ... assignment)
so that "__proto__" cannot be registered as a reducer and the prototype cannot
be mutated.
|
@cowboyd how does this version look? |
2 participants
Motivation
Add a reactive state container primitive to EffectionX that supports typed reducer actions and per-container middleware. This fills a gap between the existing
@effectionx/signals(synchronous mutations, no middleware) and the API pattern used by fetch/process/fs (stateless operations).useStateprovides a reducer-like API inspired by React'suseReducerand TC39 Signals, where state transitions are named, type-safe, and interceptable.Approach
New package
@effectionx/state-apiwith a single entry point:useState<T>(initial, reducers?).useState()call creates its owncreateApiinstance from@effectionx/context-api, so middleware installed on one container does not affect others.set(value),update(fn),get()— all returnOperation<T>for the resulting state.(state, ...args) => newState; thestateargument is injected and callers pass only the remaining args.State<T>implementsStream<T, void>for subscribing to changes viaeach.around()from@effectionx/context-api, keeping the package on the monorepo's stable Effection version instead of pinning the repo toeffection/experimental.Validation
node --env-file=.env --test state-api/state.test.tspnpm check:tsrefspnpm lintpnpm fmt:checkpnpm checkSummary by CodeRabbit
New Features
Documentation
Tests
Chores