Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
|
Warning Review limit reached
Next review available in: 27 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughAdds reactive ChangesReactive APIs and graph tracking
DOM helpers and bindings
Type-safe runtime APIs
Tooling and packaging
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Signal
participant Derived
participant ReactiveGraph
Signal->>Derived: map(fn) or pipe(fns)
Derived->>ReactiveGraph: track source dependency
ReactiveGraph-->>Derived: invalidate on source update
Derived-->>Signal: expose transformed result
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Build/src/dom/bindings.ts (1)
85-112: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSilent no-op when a non-writable
Readableis passed tobindInputValue.The
handleInputlistener silently skips writing back to the signal whensiglacks asetmethod (e.g., aDerived). This means the input→signal direction of the two-way binding is a quiet no-op, which could surprise callers who pass a read-only source. Consider either tightening the parameter type toSignal<string>or logging/warning when a non-writableReadableis detected at bind time.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Build/src/dom/bindings.ts` around lines 85 - 112, Update bindInputValue to require a writable Signal<string> instead of Readable<string>, removing the runtime set-method check in handleInput; alternatively, retain Readable support but detect non-writable signals during binding and emit a clear warning. Ensure callers and related types are updated consistently so read-only sources cannot silently no-op.Build/src/store/reactive.ts (1)
34-103: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winArray proxy
typeof prop === "number"checks are dead code — index assignment silently fails.In JavaScript Proxy traps, property keys are always
stringorsymbol; numeric indices likearr[0]arrive as the string"0". This meanstypeof prop === "number"is alwaysfalse:
gethandler (line 45): Harmless — the fallback at line 47 handles string"0"correctly.sethandler (line 56): Functional bug —arr[0] = valuefalls through toreturn false, so the assignment silently fails (or throwsTypeErrorin strict mode).This is pre-existing, but the lines were touched in this PR. Consider fixing alongside the type cleanup by checking for string-numeric indices instead.
🔧 Proposed fix for array proxy numeric index handling
set(target, prop, newValue) { if (prop === "length") { const arr = [...target.get()] as unknown as T[keyof T] & unknown[]; arr.length = newValue as number; target.set(arr as T[keyof T] & unknown[]); return true; } - if (typeof prop === "number") { + if (typeof prop === "string" && /^\d+$/.test(prop)) { const arr = [...target.get()] as unknown as T[keyof T] & unknown[]; - (arr as unknown[])[prop] = newValue; + (arr as unknown[])[Number(prop)] = newValue; target.set(arr as T[keyof T] & unknown[]); return true; } return false; },Similarly for the
gethandler:get(target, prop) { if (prop === "length") return target.get().length; - if (typeof prop === "number") return target.get()[prop]; + if (typeof prop === "string" && /^\d+$/.test(prop)) { + return target.get()[Number(prop)]; + } if (prop === "get") return () => target.get(); return (target.get() as Record<string, unknown>)[prop as string]; },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Build/src/store/reactive.ts` around lines 34 - 103, Fix array index detection in the array proxy traps within reactive: Proxy property keys are strings or symbols, so replace the typeof prop === "number" checks with handling for numeric string indices (while excluding symbols), ensuring both get and set correctly access and update array elements. Preserve existing length, get, and fallback behavior.
🧹 Nitpick comments (3)
Build/src/dom/bindings.ts (1)
96-100: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the repeated
Signal<string>casts inhandleInput.The duck-typing check casts
sigtoSignal<string>three times. Extracting a small type guard would reduce noise and make the intent clearer.♻️ Suggested refactor
+ function isWritable<T>(r: Readable<T>): r is Signal<T> { + return "set" in r && typeof (r as Signal<T>).set === "function"; + } const handleInput = (e: Event) => { const target = e.target as HTMLInputElement | HTMLTextAreaElement; - if ("set" in sig && typeof (sig as Signal<string>).set === "function") { - (sig as Signal<string>).set(target.value); + if (isWritable(sig)) { + sig.set(target.value); } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Build/src/dom/bindings.ts` around lines 96 - 100, In handleInput, replace the repeated Signal<string> casts used by the set-property check and invocation with a small type guard that identifies signals exposing a callable set method, then invoke set through the narrowed value.BUGS.md (1)
3-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider fixing the misleading docs example.
The note accurately describes the issue, but the underlying problem — a misleading
count.subscribe((value) => { ... })example in the docs — remains an open checkbox. Would you like me to search for and fix the misleading example in the documentation?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@BUGS.md` around lines 3 - 4, Update the documentation example referenced by the Subscriber note, locating the count.subscribe((value) => { ... }) usage and removing the value parameter or replacing it with the correct callback signature. Mark the corresponding checklist item in BUGS.md as completed if the misleading example is fixed.Build/src/kernel/derived.ts (1)
144-155: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider renaming the
pathparameter to avoid shadowing the importedpathfunction.The
path: PathKeyparameter shadows thepathfunction imported at line 17. While the error message string literal is unaffected, any future code added insidederived()that needs to callpath(...)would resolve to the parameter instead, causing a confusing runtime error.♻️ Optional rename
export function derived<T>( - path: PathKey, + pathKey: PathKey, fn: () => T, options?: DerivedOptions, ): Derived<T> { - if (!isPathKey(path)) { + if (!isPathKey(pathKey)) { throw new Error( - `derived() requires a valid PathKey as first argument. Got ${typeof path}. Use path(...) to create a path.`, + `derived() requires a valid PathKey as first argument. Got ${typeof pathKey}. Use path(...) to create a path.`, ); } - return new Derived(path, fn, options); + return new Derived(pathKey, fn, options); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Build/src/kernel/derived.ts` around lines 144 - 155, Rename the path parameter in derived() to a non-conflicting name such as pathKey, and update its uses in isPathKey(), the error message, and the Derived constructor call so the imported path function remains accessible.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Build/package.json`:
- Around line 6-8: Update the package metadata so the top-level types field
matches the CommonJS main entry: change types from dist/index.d.mts to
dist/index.d.cts, or use a neutral dist/index.d.ts if appropriate. Keep the
module field and exports-specific declarations unchanged.
---
Outside diff comments:
In `@Build/src/dom/bindings.ts`:
- Around line 85-112: Update bindInputValue to require a writable Signal<string>
instead of Readable<string>, removing the runtime set-method check in
handleInput; alternatively, retain Readable support but detect non-writable
signals during binding and emit a clear warning. Ensure callers and related
types are updated consistently so read-only sources cannot silently no-op.
In `@Build/src/store/reactive.ts`:
- Around line 34-103: Fix array index detection in the array proxy traps within
reactive: Proxy property keys are strings or symbols, so replace the typeof prop
=== "number" checks with handling for numeric string indices (while excluding
symbols), ensuring both get and set correctly access and update array elements.
Preserve existing length, get, and fallback behavior.
---
Nitpick comments:
In `@BUGS.md`:
- Around line 3-4: Update the documentation example referenced by the Subscriber
note, locating the count.subscribe((value) => { ... }) usage and removing the
value parameter or replacing it with the correct callback signature. Mark the
corresponding checklist item in BUGS.md as completed if the misleading example
is fixed.
In `@Build/src/dom/bindings.ts`:
- Around line 96-100: In handleInput, replace the repeated Signal<string> casts
used by the set-property check and invocation with a small type guard that
identifies signals exposing a callable set method, then invoke set through the
narrowed value.
In `@Build/src/kernel/derived.ts`:
- Around line 144-155: Rename the path parameter in derived() to a
non-conflicting name such as pathKey, and update its uses in isPathKey(), the
error message, and the Derived constructor call so the imported path function
remains accessible.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 124a9c74-9b61-4a29-817f-9e6d3037c885
⛔ Files ignored due to path filters (1)
Build/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (21)
BUGS.mdBuild/eslint.config.tsBuild/package.jsonBuild/src/async/resource.tsBuild/src/context/index.tsBuild/src/debug/index.tsBuild/src/dom/bindings.tsBuild/src/dom/selectors.tsBuild/src/flow/index.tsBuild/src/index.tsBuild/src/kernel/batch.tsBuild/src/kernel/config.tsBuild/src/kernel/dependency.tsBuild/src/kernel/derived.tsBuild/src/kernel/graph.tsBuild/src/kernel/index.tsBuild/src/kernel/signal.tsBuild/src/store/map.tsBuild/src/store/reactive.tsBuild/tests/bindings.test.tsBuild/tests/kernel.test.ts
| "main": "dist/index.cjs", | ||
| "module": "dist/index.mjs", | ||
| "types": "dist/index.d.ts", | ||
| "types": "dist/index.d.mts", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Top-level types field doesn't match main entry point module system.
main points to dist/index.cjs (CommonJS), but types points to dist/index.d.mts (ESM declaration). The top-level types field is a fallback for consumers not using the exports map (older TypeScript or moduleResolution: "node"). It should correspond to the main entry's module system. Use dist/index.d.cts (or a neutral dist/index.d.ts) instead.
🔧 Proposed fix
- "types": "dist/index.d.mts",
+ "types": "dist/index.d.cts",📝 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.
| "main": "dist/index.cjs", | |
| "module": "dist/index.mjs", | |
| "types": "dist/index.d.ts", | |
| "types": "dist/index.d.mts", | |
| "main": "dist/index.cjs", | |
| "module": "dist/index.mjs", | |
| "types": "dist/index.d.cts", |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Build/package.json` around lines 6 - 8, Update the package metadata so the
top-level types field matches the CommonJS main entry: change types from
dist/index.d.mts to dist/index.d.cts, or use a neutral dist/index.d.ts if
appropriate. Keep the module field and exports-specific declarations unchanged.
1 participant
Summary by CodeRabbit
mapandpipetransformations for reactive signals and derived values.