fix: update string input value in real-time#603
fix: update string input value in real-time#603dinakars777 wants to merge 4 commits intopmndrs:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 2d84cdc The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 2d84cdc:
|
|
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:
📝 WalkthroughWalkthrough
Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 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 `@packages/leva/src/components/ValueInput/ValueInput.tsx`:
- Around line 87-90: The current onChange handler in ValueInput unconditionally
calls onUpdate(value) on every keystroke which causes NumberInput (which reuses
ValueInput) to also commit on each keystroke; change the handler in ValueInput
so it only calls onUpdate for string inputs (e.g., check props.inputType !==
'number' or typeof value === 'string') and leave numeric inputs to retain their
original commit semantics; update the onChange={(value) => { onChange(value); if
(<guard-for-non-number-or-string>) onUpdate(value); }} logic so NumberInput
behavior is unchanged while string realtime updates remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 148ee752-814f-4729-b100-0ebea143c9bd
📒 Files selected for processing (1)
packages/leva/src/components/ValueInput/ValueInput.tsx
dinakars777
force-pushed
the
fix/string-input-realtime-update
branch
from
8470ecd to
5b335d2
Compare
travisbreaks
left a comment
travisbreaks
left a comment
There was a problem hiding this comment.
The change is small and targeted, but the behavioral implications are worth discussing.
Performance concern
Calling onUpdate on every keystroke means the leva store updates on each character typed. If downstream consumers have expensive side effects (e.g., shader recompilation, network requests, scene updates in R3F), this could cause noticeable jank. The previous blur-on-commit pattern existed for a reason.
Possible alternatives:
- Debounced
onUpdate: FireonUpdateafter a short delay (e.g., 150ms) since the last keystroke. This gives real-time feel without per-character store updates. - Opt-in behavior: Add a
liveUpdateoption to string inputs so consumers can choose between immediate and commit-on-blur.
Undo/redo
If leva tracks history for undo, each keystroke now creates a history entry. Typing "hello" would produce 5 undo steps instead of 1. Is that the intended behavior?
The type !== 'number' guard
Makes sense since number inputs have their own drag/scrub update logic. But worth a comment explaining why numbers are excluded.
The fix solves a real usability issue (string inputs feeling "dead" until blur), but the implementation could benefit from a debounce to avoid downstream performance surprises.
Previously, string inputs only committed changes to the store on blur. This change makes onUpdate fire on every keystroke for string inputs only - number inputs retain their original commit behavior (on blur/enter). Added comment explaining the type !== 'number' guard. Fixes pmndrs#599
dinakars777
force-pushed
the
fix/string-input-realtime-update
branch
from
5b335d2 to
963cf15
Compare
|
Thanks for the review @travisbreaks! The concerns are valid. Here's my thinking:
The type !== 'number' guard already ensures number inputs keep their original behavior - only string/text inputs get the real-time update. |
- Only call onUpdate for non-number inputs during realtime updates - Add liveUpdate prop for explicit opt-in (for String plugin) - Number inputs retain original commit behavior (on blur/Enter)
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/leva/src/plugins/String/String.tsx (1)
16-16: EnsureliveUpdatecannot be accidentally overriddenOn Line 16,
liveUpdateis set before{...props}, soprops.liveUpdate={false}would win and reintroduce blur-only commits. If editable strings must always update live, placeliveUpdatelast.Proposed fix
- if (editable) return <ValueInput value={displayValue} onUpdate={onUpdate} onChange={onChange} liveUpdate {...props} /> + if (editable) return <ValueInput value={displayValue} onUpdate={onUpdate} onChange={onChange} {...props} liveUpdate />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/leva/src/plugins/String/String.tsx` at line 16, The editable string branch currently renders ValueInput with liveUpdate before spreading {...props}, so a passed prop like props.liveUpdate=false can override it; in the editable branch (the conditional that returns <ValueInput ... />) ensure liveUpdate is placed after the spread (or move the spread before and then append liveUpdate) so that ValueInput(..., {...props}, liveUpdate) always forces live updates—update the JSX rendering of ValueInput in the editable branch (referencing ValueInput, displayValue, onUpdate, onChange, and props) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/leva/src/plugins/String/String.tsx`:
- Line 16: The editable string branch currently renders ValueInput with
liveUpdate before spreading {...props}, so a passed prop like
props.liveUpdate=false can override it; in the editable branch (the conditional
that returns <ValueInput ... />) ensure liveUpdate is placed after the spread
(or move the spread before and then append liveUpdate) so that ValueInput(...,
{...props}, liveUpdate) always forces live updates—update the JSX rendering of
ValueInput in the editable branch (referencing ValueInput, displayValue,
onUpdate, onChange, and props) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2d0bb799-0f8f-4c34-be7c-18adfc35e218
📒 Files selected for processing (2)
packages/leva/src/components/ValueInput/ValueInput.tsxpackages/leva/src/plugins/String/String.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/leva/src/components/ValueInput/ValueInput.tsx
|
Thanks for the review! Fixed by adding |
2 participants
Summary
Previously, string inputs in Leva only committed changes to the store when the input lost focus (on blur). This meant that
useEffecthooks depending on the value would only fire after clicking away from the input.This fix makes
onUpdatefire on every keystroke, so the store value updates in real-time.Changes
ValueInput.tsxto call bothonChangeandonUpdateon everyonChangeeventTesting
Build passes successfully. The change maintains backward compatibility -
onUpdatewas already being called on blur, and now it's also called on every keystroke.Fixes #599
Summary by CodeRabbit