extract createValueSignal + port createBooleanSignal to use it#212
Conversation
📝 WalkthroughWalkthroughThe changes introduce a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (2 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 |
robertf224
changed the title
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
signals/value.test.ts (1)
46-51:⚠️ Potential issue | 🟠 MajorReplace
sleep(1)timeout in race with deterministic helper; preserve both assertions.The
sleep(1)used at line 49 violates the no-sleep-test-sync policy by waiting for an async result. However, the suggested rewrite removes the race entirely and loses the critical assertion at line 54 that verifies no update is emitted when the signal is set to its current value.The race pattern itself is correct for this test. The fix is to replace the
sleep(1)timeout with a deterministic timeout mechanism that preserves both assertions:
- The first assertion (line 54) validates that setting an unchanged value does not emit
- The second assertion (line 60) validates that setting a changed value does emit
Consider using a timeout helper from
@effectionx/convergeor similar deterministic mechanism instead of sleep.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@signals/value.test.ts` around lines 46 - 51, The race uses sleep(1) which violates the no-sleep-test-sync policy; replace that raw sleep call inside the race with a deterministic timeout helper (for example the timeout/tick utility from `@effectionx/converge` or your repo's deterministic timer) so the race still races subscription.next() against a fast, deterministic timeout; keep the race pattern around subscription.next(), preserve the assertion that setting the same value emits nothing (the "unchanged value does not emit" assertion) and the later assertion that changing the value does emit (the "changed value does emit" assertion), and update imports to include the chosen deterministic timeout helper while removing sleep usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@signals/mod.ts`:
- Line 2: This change added a new exported module (signals/value.ts) and
modified existing sources (signals/boolean.ts and signals/mod.ts), so bump the
package semver minor; update the version field in signals/package.json from
0.5.3 to 0.6.0 and ensure the package.json change is included in the PR so it
satisfies the .policies/version-bump.md requirement.
In `@signals/value.ts`:
- Around line 11-16: The set function must declare an explicit return type and
use Object.is for equality checks: change the signature to function set(value:
T): T and replace the inequality check (value !== ref.current) with if
(!Object.is(value, ref.current)) { ref.current = value;
signal.send(ref.current); } then return ref.current at the end; reference the
set function, ref.current and signal.send (and the ValueSignal<T> interface)
when making this change.
---
Outside diff comments:
In `@signals/value.test.ts`:
- Around line 46-51: The race uses sleep(1) which violates the
no-sleep-test-sync policy; replace that raw sleep call inside the race with a
deterministic timeout helper (for example the timeout/tick utility from
`@effectionx/converge` or your repo's deterministic timer) so the race still races
subscription.next() against a fast, deterministic timeout; keep the race pattern
around subscription.next(), preserve the assertion that setting the same value
emits nothing (the "unchanged value does not emit" assertion) and the later
assertion that changing the value does emit (the "changed value does emit"
assertion), and update imports to include the chosen deterministic timeout
helper while removing sleep usage.
🪄 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: 6b13f5c8-59a0-4984-a66b-5109f64f37e4
📒 Files selected for processing (4)
signals/boolean.tssignals/mod.tssignals/value.test.tssignals/value.ts
| @@ -1,4 +1,5 @@ | |||
| export * from "./boolean.ts"; | |||
| export * from "./value.ts"; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Changed TS source files in this PR:"
git diff --name-only origin/main...HEAD | rg -n '^signals/.*\.(ts|tsx)$' || true
echo
echo "Changed version-bearing files:"
git diff --name-only origin/main...HEAD | rg -n '(^|/)(package\.json|deno\.json|jsr\.json|CHANGELOG\.md)$' || true
echo
echo "Version field diffs (if any):"
git diff origin/main...HEAD -- '**/package.json' '**/deno.json' '**/jsr.json' | rg -n '"version"\s*:' || trueRepository: thefrontside/effectionx
Length of output: 305
🏁 Script executed:
#!/bin/bash
# First, let's understand the repo structure
echo "=== Repository structure (top level) ==="
ls -la
echo
echo "=== Git status ==="
git status
echo
echo "=== Available branches ==="
git branch -aRepository: thefrontside/effectionx
Length of output: 3306
🏁 Script executed:
#!/bin/bash
# Look for package files in the repo
echo "=== Finding package.json files ==="
fd -t f 'package\.json$'
echo
echo "=== Finding deno.json files ==="
fd -t f 'deno\.json$'
echo
echo "=== Finding jsr.json files ==="
fd -t f 'jsr\.json$'
echo
echo "=== Checking signals directory ==="
ls -la signals/ 2>/dev/null || echo "signals/ directory not found at root"Repository: thefrontside/effectionx
Length of output: 1768
🏁 Script executed:
#!/bin/bash
# Check if there's a .policies/version-bump.md file
echo "=== Checking for version-bump policy ==="
fd -t f 'version-bump.md' || echo "Policy file not found"
echo
echo "=== Checking .policies directory ==="
ls -la .policies/ 2>/dev/null || echo ".policies/ directory not found"Repository: thefrontside/effectionx
Length of output: 1656
🏁 Script executed:
#!/bin/bash
# Read the version-bump policy
echo "=== .policies/version-bump.md ==="
cat .policies/version-bump.mdRepository: thefrontside/effectionx
Length of output: 2344
🏁 Script executed:
#!/bin/bash
# Check git log to understand commits
echo "=== Recent git log ==="
git log --oneline -10
echo
echo "=== Current HEAD info ==="
git log -1 --format="%H %s"
echo
echo "=== Difference from main ==="
git log --oneline main..HEAD 2>/dev/null | head -20 || echo "No commits on current HEAD vs main"Repository: thefrontside/effectionx
Length of output: 323
🏁 Script executed:
#!/bin/bash
# Check signals/package.json version
echo "=== signals/package.json version ==="
jq '.version' signals/package.json
echo
echo "=== signals/package.json full content ==="
cat signals/package.jsonRepository: thefrontside/effectionx
Length of output: 1034
🏁 Script executed:
#!/bin/bash
# Try to get diffs using different approaches
echo "=== Files changed in HEAD ==="
git diff --name-only HEAD~1..HEAD 2>/dev/null || echo "Cannot compare HEAD~1..HEAD"
echo
echo "=== Try files changed in last commit ==="
git show --name-only --format='' HEAD 2>/dev/null | head -20 || echo "Cannot show HEAD files"
echo
echo "=== Check main vs HEAD ==="
git diff --name-only main..HEAD -- signals/ 2>/dev/null | head -20 || echo "No diff available main..HEAD"Repository: thefrontside/effectionx
Length of output: 762
🏁 Script executed:
#!/bin/bash
# Check detailed git diff for signals package
echo "=== Git diff for signals/ source files ==="
git diff main..HEAD -- signals/mod.ts signals/value.ts signals/boolean.ts | head -150
echo
echo "=== Check if package.json was modified ==="
git diff main..HEAD -- signals/package.json || echo "No changes to signals/package.json"Repository: thefrontside/effectionx
Length of output: 2598
🏁 Script executed:
#!/bin/bash
# Get a summary of what changed
echo "=== Detailed file changes ==="
for file in signals/mod.ts signals/value.ts signals/boolean.ts signals/value.test.ts; do
echo "--- $file ---"
git diff main..HEAD -- $file | head -40
echo
doneRepository: thefrontside/effectionx
Length of output: 3701
Version bump required for @effectionx/signals package.
This PR modifies source files (signals/mod.ts, signals/boolean.ts, and adds signals/value.ts) but the version in signals/package.json remains at 0.5.3. Per .policies/version-bump.md, all PRs changing package source files must include a semantic version bump.
Determine the appropriate bump level (patch, minor, or major) based on the nature of the changes and update signals/package.json accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@signals/mod.ts` at line 2, This change added a new exported module
(signals/value.ts) and modified existing sources (signals/boolean.ts and
signals/mod.ts), so bump the package semver minor; update the version field in
signals/package.json from 0.5.3 to 0.6.0 and ensure the package.json change is
included in the PR so it satisfies the .policies/version-bump.md requirement.
| function set(value: T) { | ||
| if (value !== ref.current) { | ||
| ref.current = value; | ||
|
|
||
| signal.send(ref.current); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "value.ts" | grep -E "signals|value"Repository: thefrontside/effectionx
Length of output: 85
🏁 Script executed:
# Locate the file in signals directory
git ls-files | grep "signals/value.ts"Repository: thefrontside/effectionx
Length of output: 83
🏁 Script executed:
# Read the signals/value.ts file to see the full context
cat -n signals/value.tsRepository: thefrontside/effectionx
Length of output: 1109
🏁 Script executed:
cat -n signals/types.tsRepository: thefrontside/effectionx
Length of output: 940
🏁 Script executed:
# Check if Object.is is used elsewhere in the codebase for similar patterns
rg "Object\.is" --type ts --type tsx -A 2 -B 2Repository: thefrontside/effectionx
Length of output: 94
🏁 Script executed:
# Check the coding guidelines file to confirm the explicit return type requirement
cat -n .policies/*.md | head -50Repository: thefrontside/effectionx
Length of output: 2276
🏁 Script executed:
# Search for tests of createValueSignal to understand expected behavior
rg "createValueSignal|ValueSignal" --type ts -B 2 -A 5 | head -100Repository: thefrontside/effectionx
Length of output: 4845
🏁 Script executed:
# Check if there are any tests related to value comparisons
rg "Object.is|NaN|Infinity" --type ts -B 2 -A 2Repository: thefrontside/effectionx
Length of output: 628
Add explicit return type annotation to set() function.
The set() function is missing the : T return type annotation. Per the ValueSignal<T> interface (line 13 of types.ts) and TypeScript guidelines, public functions must have explicit return types.
Additionally, replace !== with !Object.is() for change detection. The current !== operator causes surprising behavior with NaN (always triggers send, since NaN !== NaN) and -0 vs +0. Using Object.is() aligns with standard JavaScript semantics and correctly handles these edge cases in a generic value signal.
Suggested patch
function set(value: T) {
- if (value !== ref.current) {
+ if (!Object.is(value, ref.current)) {
ref.current = value;
signal.send(ref.current);
}
- return ref.current;
+ return ref.current;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@signals/value.ts` around lines 11 - 16, The set function must declare an
explicit return type and use Object.is for equality checks: change the signature
to function set(value: T): T and replace the inequality check (value !==
ref.current) with if (!Object.is(value, ref.current)) { ref.current = value;
signal.send(ref.current); } then return ref.current at the end; reference the
set function, ref.current and signal.send (and the ValueSignal<T> interface)
when making this change.
|
Hey @robertf224, this is great. I'll review it in the next 24 hours. I've been meaning to add something like this for a while. You might also find this useful when it lands #193 |
commit: |
taras
left a comment
taras
left a comment
There was a problem hiding this comment.
This is a great idea. They're a few follow up items need from coderabbit and documentation but I'll take care of them. Thank you for contributing. I'll release it shortly.
2 participants
Motivation
Needed a simple register-style signal that wasn't a boolean and had just copied the code for boolean, figured it was worth just abstracting over it a tiny bit so I extracted
createValueSignaland just portedcreateBooleanSignalto be a thin wrapper over it.