fix(frontend): preserve input focus across reruns#49
Open
sudo-vaibhav wants to merge 1 commit into
Open
Conversation
The renderer keyed every component by its value (`key={props.tree.value}`
in TreeRender), so any committed value change unmounted and remounted the
widget — dropping DOM focus, the text caret, and selection mid-interaction
on every rerun. This was visible in Backroad-based apps (e.g. code-bot)
where editing one field or nudging a slider yanked focus away.
Key components by their stable `id` instead (the parent already keys the
subtree by `path`, so the value-key only ever forced remounts). To keep
server-driven value updates visible without a remount, components now
sync the server value in place via a new `useSyncedState` hook (React's
"adjust state on prop change" pattern, guarded so it neither loops nor
clobbers in-progress local edits). The two react-select widgets move from
uncontrolled `defaultValue` to a controlled, synced value for the same
reason.
Adds an e2e regression guard: a slider must keep keyboard focus across
each commit-driven rerun (repeated arrow nudges only register if focus
survives the remount). Verified it fails on the old value-keyed renderer.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughIntroduces a ChangesFocus Retention via useSyncedState
Sequence Diagram(s)sequenceDiagram
participant User as User (keyboard)
participant Slider as Slider component
participant useSyncedState as useSyncedState
participant Socket as Backroad socket (server)
User->>Slider: ArrowRight keypress
Slider->>useSyncedState: setValue(newLocalValue)
Slider->>Socket: setBackroadValue({ id, value })
Socket-->>Slider: rerender with updated props.value
Slider->>useSyncedState: render — Object.is(serverValue, lastRef)?
alt identity changed
useSyncedState->>Slider: setValue(serverValue), update ref
else same identity
useSyncedState-->>Slider: no-op, local state preserved, focus retained
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Knip dead-code gate — no regressions
All counts at or below baseline. |
size-limit report 📦
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Every value-returning widget lost focus — and its text caret / selection — on every rerun. In Backroad-based apps (this surfaced while running code-bot), editing one field or nudging a slider yanked focus away mid-interaction.
Root cause:
TreeRenderkeyed each component by its value:The parent (
Frame) already keys each child subtree by stablepath, so the inner value-key did nothing except force an unmount+remount whenever the committed value changed. That remount replaces the DOM node → focus, caret, and selection are gone.Fix
idinstead ofvalue(tree.tsx). The node now survives a value change and only remounts when its identity actually changes.useSyncedStatehook — since we no longer remount to re-seed local state, components adopt a server-pushed value in place during render, only when it actually changes (React's documented "adjusting state on a prop change" pattern). Guarded withObject.isso it neither loops nor clobbers in-progress local edits.useState(props.value)→useSyncedState(props.value)across the controlled widgets (text_input, text_area, number_input, slider, checkbox, toggle, radio, date_input, time_input, color_picker).defaultValue(which relied on the remount to refresh) to a controlled, synced value. multiselect tracks the selected values (stable identity) and derives react-select's option objects each render, so the sync guard doesn't re-fire every render.Test
e2e/focus-retention.spec.ts— a slider must keep keyboard focus across each commit-driven rerun: focus once, then drive nudges throughpage.keyboard(targets whatever's focused; doesn't re-focus the waylocator.presswould and mask the bug). Each subsequent increment landing proves focus survived the prior rerun.Verified the guard works: reverting the key back to
props.tree.valuemakes this spec fail at the focus assertion; the fix makes it pass.Verification
typecheck(backroad-components + backroad-frontend): clean🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes