Conversation
Refactor AsyncDerivedCell into a base AsyncCell class to support the new AsyncTaskCell. Unlike derived cells, tasks are manually triggered via runWith(input) while providing reactive pending and error states.
Update runWith to trigger a new execution for every call instead of sharing the promise from an active execution.
📝 WalkthroughWalkthroughAdds a new AsyncTaskCell API (accessible via Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant TaskCell as AsyncTaskCell
participant TaskFn as Task Function
participant Composite as Composite / Derived
participant Context as LocalContext
Caller->>TaskCell: call runWith(input)
activate TaskCell
TaskCell->>TaskCell: create AbortController + set pending=true
TaskCell->>TaskFn: invoke fn(input, signal)
activate TaskFn
TaskFn-->>TaskCell: resolves/rejects (may observe signal)
deactivate TaskFn
alt resolved
TaskCell->>TaskCell: set value, clear error, pending=false
TaskCell->>Composite: propagate value/pending change
else rejected/aborted
TaskCell->>TaskCell: set error or keep previous value, pending=false
TaskCell->>Composite: propagate error/pending change
end
deactivate TaskCell
Note over Context, TaskCell: Context.destroy -> DisposeAsyncCell -> cancels/disposes TaskCell
Context->>TaskCell: dispose / abort in-flight runs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@library/classes.js`:
- Around line 1339-1356: Update the AsyncTaskCell JSDoc to remove the stale
claim about deduplication: delete or reword the line "Caches concurrent calls
with the same input to avoid duplicate executions" and instead state that
AsyncTaskCell does NOT deduplicate concurrent runWith(input) calls; mention that
concurrent calls are independent (no caching) and that cancellation must be
handled via AbortSignal. Ensure you update references to AsyncTaskCell, runWith,
pending, and error in the comment so the docs accurately reflect current
behavior.
- Around line 1418-1426: The code uses currentInput = null as a sentinel so
computedFn skips calling fn when currentInput === null, which incorrectly drops
legitimate inputs like runWith(null); change the sentinel to an explicit boolean
flag (e.g., hasInput) alongside a separate input variable (currentInput) so
computedFn checks hasInput rather than currentInput === null, and ensure runWith
sets both currentInput and hasInput and computedFn captures currentInput into
capturedInput before calling fn(this._signal) via fn(capturedInput,
this._signal); update any JSDoc/type comments accordingly to reflect that
currentInput may be null but hasInput signals presence.
🧹 Nitpick comments (1)
tests/index.test.js (1)
1807-1857: Remove leftoverconsole.logdebug statements from this test.Lines 1811, 1817, 1823, and 1833 have
console.logcalls ('\nderiving a','deriving b','deriving c','deriving d') that appear to be debugging artifacts from development. They add noise to test output.Suggested cleanup
const a = Cell.derivedAsync(async (get) => { - console.log('\nderiving a'); await new Promise((r) => setTimeout(r, 10)); return get(source); }); const b = Cell.derivedAsync(async (get) => { - console.log('deriving b'); const val = await get(a); await new Promise((r) => setTimeout(r, 15)); return val * 10; }); const c = Cell.derivedAsync(async (get) => { - console.log('deriving c'); const val = await get(a); await new Promise((r) => setTimeout(r, 5)); return val + 100; }); const d = Cell.derivedAsync(async (get) => { const bVal = await get(b); const cVal = await get(c); - console.log('deriving d'); await new Promise((r) => setTimeout(r, 10)); return bVal + cVal; });
Use a flag to track execution status, allowing null as a valid input. Clarify that concurrent calls are not deduplicated or cached.
commit: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@library/classes.js`:
- Around line 1466-1492: The first-execution .then() handler in runWith may act
on a stale run if a subsequent runWith replaced this.wvalue; to fix, capture a
local run identity or the returned promise (e.g., const runId =
this.currentRunId or const myValue = value) inside runWith and, at the start of
the .then() handler, bail out if the run is stale (for example check that
this.wvalue === myValue or this.currentRunId === runId); only call
this.update(), iterate this.derivations and push children to UPDATE_BUFFER and
call triggerUpdate() when the captured identity still matches the cell, keeping
the existing AsyncDerivedCell and IsScheduled checks intact.
| this.runWith = async (input) => { | ||
| const isFirstExecution = !hasExecuted; | ||
| this.abort(); | ||
| currentInput = input; | ||
| hasInput = true; | ||
| const value = this.computedFn(); | ||
| hasExecuted = true; | ||
|
|
||
| // For the first execution, we need to manually trigger an update | ||
| // since AsyncCell skips update() for the initial state. | ||
| // We also need to schedule AsyncDerivedCell children for recomputation. | ||
| if (isFirstExecution) { | ||
| value.then(() => { | ||
| this.update(); | ||
| // Schedule AsyncDerivedCell children for recomputation | ||
| for (const child of this.derivations) { | ||
| if (child instanceof AsyncDerivedCell && !child[IsScheduled]) { | ||
| UPDATE_BUFFER.push(child); | ||
| child[IsScheduled] = true; | ||
| } | ||
| } | ||
| if (!IS_UPDATING) triggerUpdate(); | ||
| }); | ||
| } | ||
|
|
||
| return value; | ||
| }; |
There was a problem hiding this comment.
First-execution .then() handler lacks a staleness guard.
If a second runWith is called before the first completes, the first execution's .then() at line 1478 still fires (because the tripwire resolves the aborted promise). It calls this.update() and schedules children even though it belongs to a superseded run. In practice, this.wvalue will already point to the second execution's promise and !child[IsScheduled] prevents double-scheduling, so this is unlikely to cause visible bugs—but it's fragile and inconsistent with the staleness guards used elsewhere in AsyncCell (e.g., currentRunId !== runId checks in .finally()).
Consider capturing a local snapshot of the run state to skip the handler when stale:
Proposed fix
this.runWith = async (input) => {
const isFirstExecution = !hasExecuted;
this.abort();
currentInput = input;
hasInput = true;
const value = this.computedFn();
hasExecuted = true;
+ // Capture a reference to detect if a newer runWith supersedes this one.
+ const capturedValue = this.wvalue;
if (isFirstExecution) {
value.then(() => {
+ // If another runWith was called, this execution is stale.
+ if (this.wvalue !== capturedValue) return;
this.update();
for (const child of this.derivations) {
if (child instanceof AsyncDerivedCell && !child[IsScheduled]) {
UPDATE_BUFFER.push(child);
child[IsScheduled] = true;
}
}
if (!IS_UPDATING) triggerUpdate();
});
}
return value;
};🤖 Prompt for AI Agents
In `@library/classes.js` around lines 1466 - 1492, The first-execution .then()
handler in runWith may act on a stale run if a subsequent runWith replaced
this.wvalue; to fix, capture a local run identity or the returned promise (e.g.,
const runId = this.currentRunId or const myValue = value) inside runWith and, at
the start of the .then() handler, bail out if the run is stale (for example
check that this.wvalue === myValue or this.currentRunId === runId); only call
this.update(), iterate this.derivations and push children to UPDATE_BUFFER and
call triggerUpdate() when the captured identity still matches the cell, keeping
the existing AsyncDerivedCell and IsScheduled checks intact.
Renames `Cell.createComposite` to `Cell.composite` and adds a `loaded` property to track initial load completion. Also refines `AsyncCell` types to return `Promise<T>` instead of `Promise<T | null>`.
Rewrite the README with a quick start guide, API reference, and advanced usage patterns. Also remove the `name` debugging option from effects and reformat the test suite for consistent indentation.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@library/classes.js`:
- Around line 1009-1028: The JSDoc for abort() contradicts the PR summary: if
AsyncCell is intended to expose abort() publicly, remove the `@protected` tag from
the abort() JSDoc (and ensure the description still returns {void}) so the
method is documented as public; if abort() should stay protected, update the PR
description to not claim a public abort() API—refer to the abort() method on
AsyncCell when making the change.
- Around line 1046-1048: The constructor currently seeds AsyncCell (constructor)
and AsyncTaskCell with Promise.resolve(null) but types promise Promise<T>, which
hides a real null until first computation; update the types to reflect the
actual runtime by changing the initial state typing to Promise<T|null> (and
update any related JSDoc return types for get() in AsyncCell/AsyncTaskCell from
Promise<T> to Promise<T|null>) or alternatively replace the pre-resolved null
with an unresolving placeholder Promise created inside the AsyncCell
constructors and keep get() as Promise<T>; pick one approach and apply
consistently to the AsyncCell constructor, AsyncTaskCell, and their get() JSDoc
signatures so callers see the accurate contract.
In `@README.md`:
- Around line 45-48: Update the "Requirements" section in the README to reflect
accurate Node.js support: replace "Node.js 14+" with a safer minimum (e.g.,
"Node.js 14.6+" or "Node.js 16+") because the library relies on
WeakRef/FinalizationRegistry; reference the "Requirements" header and the
current "Node.js 14+" text and explicitly mention WeakRef/FinalizationRegistry
compatibility in the note so users know why the minimum was raised.
- Around line 183-189: The listener passed to dashboard.loaded.listen uses await
but isn't declared async; update the callback (the function passed to
loaded.listen) to be async or remove await and chain promises instead —
specifically make the callback async so you can await
dashboard.values.profile.get() and dashboard.values.posts.get() before calling
renderDashboard(profileData, postsData), or alternatively replace awaits with
Promise.all on dashboard.values.profile.get() and dashboard.values.posts.get()
and then call renderDashboard with the resolved values.
🧹 Nitpick comments (1)
library/classes.js (1)
1373-1390:AsyncTaskCellinheritsAsyncCell's constructor which immediately callscomputedFn()— review the double-invocation path.
super(computedFn)at Line 1387 invokesAsyncCell's constructor, which callsthis.computedFn()at Line 1147. At that pointhasInputisfalse, socomputedFnreturnsPromise.resolve(null). Then the fullAsyncCellmachinery (runId tracking, abort controller, tripwire,.finally()handler) all run for this no-op initial promise.Immediately after,
this.pending.set(false)(Line 1390) overrides thepending = truethatAsyncCelljust set. This works but is wasteful — it creates and discards anAbortController, allocates a tripwire promise, and triggers the entire.finally()chain for a dummy value. If this is intentional scaffolding, a brief comment would help future maintainers understand why.
| /** | ||
| * @protected | ||
| * Aborts the current computation if one is running. | ||
| * @returns {void} | ||
| */ | ||
| abort() { | ||
| this.#controller?.abort(); | ||
| } | ||
|
|
||
| /** | ||
| * Gets the AbortSignal for the current computation. | ||
| * @protected | ||
| * @returns {AbortSignal} | ||
| */ | ||
| get _signal() { | ||
| if (!this.#controller) { | ||
| this.#controller = new AbortController(); | ||
| } | ||
| return this.#controller.signal; | ||
| } |
There was a problem hiding this comment.
abort() is marked @protected but documented as public in the PR summary.
The PR summary states "AsyncCell exposes a public abort() method for external cancellation," but the JSDoc says @protected. If external cancellation is intended, remove @protected; otherwise, update the PR description.
🤖 Prompt for AI Agents
In `@library/classes.js` around lines 1009 - 1028, The JSDoc for abort()
contradicts the PR summary: if AsyncCell is intended to expose abort() publicly,
remove the `@protected` tag from the abort() JSDoc (and ensure the description
still returns {void}) so the method is documented as public; if abort() should
stay protected, update the PR description to not claim a public abort()
API—refer to the abort() method on AsyncCell when making the change.
| constructor(fn) { | ||
| /** @type {Promise<T | null>} */ | ||
| const initialState = Promise.resolve(null); | ||
| const initialState = /** @type {Promise<T>} */ (Promise.resolve(null)); | ||
| super(() => initialState); |
There was a problem hiding this comment.
Initial state Promise.resolve(null) is type-cast to Promise<T>.
Both AsyncCell (Line 1047) and AsyncTaskCell (Line 1382) use Promise.resolve(null) cast to Promise<T>. Consumers calling get() before the first real computation completes will receive null disguised as T. The return type is declared as Promise<T> (Lines 1206, 1226), not Promise<T | null>.
This is a known trade-off in the JS/JSDoc world, but it contradicts the commit message "Refined AsyncCell types to return Promise<T> (no longer Promise<T | null>)." Consider documenting this pre-resolution null behavior or typing the initial state more honestly as Promise<T | null>.
🤖 Prompt for AI Agents
In `@library/classes.js` around lines 1046 - 1048, The constructor currently seeds
AsyncCell (constructor) and AsyncTaskCell with Promise.resolve(null) but types
promise Promise<T>, which hides a real null until first computation; update the
types to reflect the actual runtime by changing the initial state typing to
Promise<T|null> (and update any related JSDoc return types for get() in
AsyncCell/AsyncTaskCell from Promise<T> to Promise<T|null>) or alternatively
replace the pre-resolved null with an unresolving placeholder Promise created
inside the AsyncCell constructors and keep get() as Promise<T>; pick one
approach and apply consistently to the AsyncCell constructor, AsyncTaskCell, and
their get() JSDoc signatures so callers see the accurate contract.
README.md
Outdated
| ### Requirements | ||
|
|
||
| Source cells are the building blocks of your reactive state. They hold values that can change over time, automatically notifying dependents when updates occur. | ||
| - ES2020+ environment (browsers, Node.js 14+, Deno, Bun) | ||
| - TypeScript 4.5+ (optional, for type definitions) |
There was a problem hiding this comment.
Node.js 14 may not fully support ES2020+.
Node.js 14 has partial ES2020 support (e.g., optional chaining landed in 14.0, but WeakRef / FinalizationRegistry arrived in 14.6). Since the library uses WeakRef extensively, consider bumping the stated minimum to Node.js 14.6+ or Node.js 16+ for safety.
🤖 Prompt for AI Agents
In `@README.md` around lines 45 - 48, Update the "Requirements" section in the
README to reflect accurate Node.js support: replace "Node.js 14+" with a safer
minimum (e.g., "Node.js 14.6+" or "Node.js 16+") because the library relies on
WeakRef/FinalizationRegistry; reference the "Requirements" header and the
current "Node.js 14+" text and explicitly mention WeakRef/FinalizationRegistry
compatibility in the note so users know why the minimum was raised.
Add missing async keyword to the dashboard.loaded listener.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@README.md`:
- Around line 133-139: The snippet uses top-level await with user.get(), which
can break in non-ESM contexts; update the example to avoid top-level await by
wrapping the call in an async function or using a Promise callback.
Specifically, change the usage around user.get() (and keep user.pending.listen /
user.error.listen as-is) so the data retrieval is performed inside an async
function or via user.get().then(...) to ensure the example runs in CommonJS and
browser scripts without top-level-await support.
| // Built-in status tracking | ||
| user.pending.listen((isLoading) => console.log(isLoading ? 'Loading...' : 'Done')); | ||
| user.error.listen((err) => err && console.error(err)); | ||
|
|
||
| // Access the data | ||
| const data = await user.get(); | ||
| ``` |
There was a problem hiding this comment.
Avoid top‑level await in the generic JS snippet.
Unless you’re explicitly documenting ESM + top‑level await, this will be copied into non‑module contexts and fail. Wrap in an async function or use .then.
Suggested fix
-// Access the data
-const data = await user.get();
+// Access the data
+(async () => {
+ const data = await user.get();
+ // use data
+})();📝 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.
| // Built-in status tracking | |
| user.pending.listen((isLoading) => console.log(isLoading ? 'Loading...' : 'Done')); | |
| user.error.listen((err) => err && console.error(err)); | |
| // Access the data | |
| const data = await user.get(); | |
| ``` | |
| // Built-in status tracking | |
| user.pending.listen((isLoading) => console.log(isLoading ? 'Loading...' : 'Done')); | |
| user.error.listen((err) => err && console.error(err)); | |
| // Access the data | |
| (async () => { | |
| const data = await user.get(); | |
| // use data | |
| })(); |
🤖 Prompt for AI Agents
In `@README.md` around lines 133 - 139, The snippet uses top-level await with
user.get(), which can break in non-ESM contexts; update the example to avoid
top-level await by wrapping the call in an async function or using a Promise
callback. Specifically, change the usage around user.get() (and keep
user.pending.listen / user.error.listen as-is) so the data retrieval is
performed inside an async function or via user.get().then(...) to ensure the
example runs in CommonJS and browser scripts without top-level-await support.
1 participant
Introduces
AsyncTaskCell, a new cell type for one-time, user-triggered asynchronous operations. UnlikeCell.derivedAsync()which automatically re-computes when dependencies change,Cell.task()creates a cell that only executes when explicitly invoked viarunWith(input).Motivation
Cell.derivedAsync()is designed for declarative, reactive data fetching — it watches dependencies and re-runs automatically. However, many async operations are inherently imperative:These operations should not auto-execute and need explicit invocation with dynamic input. Previously, there was no first-class way to model these within the cell system while still benefiting from built-in
pending/errorstate tracking and integration withCell.createComposite.Changes
Refactored class hierarchy
Extracted a shared
AsyncCellbase class fromAsyncDerivedCellto enable code reuse:AsyncCellcontains the common async machinery: pending/error state management, abort controller lifecycle, upstream promise tracking, and the core computation/cancellation logic.New
AsyncTaskCell(Cell.task())Key behaviors:
runWith()call — unlikederivedAsync, which starts pending immediately.runWith()call aborts the previous in-flight execution viaAbortSignal.Cell.createCompositefor grouped pending/error tracking across multiple tasks.Cell.derivedAsync— derived cells recompute when the task completes.Public
abort()method onAsyncCellBoth
AsyncDerivedCellandAsyncTaskCellnow expose anabort()method for external cancellation of in-flight computations.Broadened
instanceofchecksUpdated
LocalContextdisposal andCell.createCompositeto checkinstanceof AsyncCellinstead ofinstanceof AsyncDerivedCell, so both derived and task cells are handled correctly for cleanup and composite state aggregation.Tests
Added comprehensive test coverage for
Cell.task():Cell.derivedfor dependent computationsCell.createComposite(single task, multiple tasks, mixed with sync cells)Cell.derivedAsync(dependency tracking, recomputation on re-execution, error propagation, multiple task dependencies)Summary by CodeRabbit