fix: harden routing, focus, keybindings, and forms#267
Merged
RtlZeroMemory merged 4 commits intomainfrom Mar 6, 2026
Merged
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR adds keybinding registration validation with error handling, implements focus trap stacking with Tab traversal constraints and ESC handling, extends form APIs with readOnly field support and text-bindable type safety, refactors wheel routing to check multiple scrollable ancestors, and updates widget typings and documentation accordingly. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/core/src/app/createApp.ts (1)
41-52:⚠️ Potential issue | 🟡 MinorFix import ordering to clear CI
organizeImportsfailure.Line 41-Line 52 currently fail the import-order check reported by CI, so this PR will remain red until the block is organized.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/app/createApp.ts` around lines 41 - 52, Reorder the named imports in the import from "../keybindings/index.js" to satisfy the project's organizeImports rule: alphabetize the specifiers so they read createManagerState, getBindings, getMode, getPendingChord, registerBindings, registerModes, removeBindingsBySource, resetChordState, routeKeyEvent, setMode; leave the module path unchanged.packages/core/src/forms/useForm.ts (1)
1-1551:⚠️ Potential issue | 🟡 MinorFormatter mismatch is still failing CI for this file.
The pipeline reports Prettier/formatter mismatch in
packages/core/src/forms/useForm.ts; please run the formatter and commit the result.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/forms/useForm.ts` around lines 1 - 1551, The file packages/core/src/forms/useForm.ts is failing CI due to formatting (Prettier) mismatch; fix by running the project's formatter (e.g. run the repo's format script or Prettier: prettier --write packages/core/src/forms/useForm.ts or npm/yarn run format), review changes (especially around the exported useForm and helper functions), stage the formatted file, and commit/push the updated file so the Prettier check passes in CI.
🧹 Nitpick comments (1)
packages/core/src/keybindings/manager.ts (1)
137-140: Tighten parse result typing sosourceTagis preserved at compile time.
parseBindingsWithOptionscan return source-tagged bindings, butParseBindingsResultstill exposesKeyBinding[], which hides that metadata in the type system.Type-only refactor sketch
-export type ParseBindingsResult<C> = Readonly<{ - bindings: readonly KeyBinding<C>[]; +export type ParseBindingsResult<C, B extends KeyBinding<C> = KeyBinding<C>> = Readonly<{ + bindings: readonly B[]; invalidKeys: readonly InvalidKey[]; }>; -function parseBindingsWithOptions<C>( +function parseBindingsWithOptions<C>( map: BindingMap<C>, options?: RegisterBindingsOptions, -): ParseBindingsResult<C> { +): ParseBindingsResult<C, ManagedBinding<C>> {Also applies to: 189-211
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/keybindings/manager.ts` around lines 137 - 140, The ParseBindingsResult type erases sourceTag metadata by returning KeyBinding<C>[]; update ParseBindingsResult to be generic over the binding item so it preserves any source tagging (e.g., ParseBindingsResult<C, B extends KeyBinding<C> = KeyBinding<C>>) and use that generic when returning results from parseBindingsWithOptions and related functions; specifically change the bindings field to readonly B[] (or readonly B[]) and update callsites/other exported types (the parseBindingsWithOptions return type and any references around lines referenced) so the concrete source-tagged binding type flows through the result type instead of being widened to KeyBinding<C>.
🤖 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/core/src/app/__tests__/widgetRenderer.integration.test.ts`:
- Around line 512-579: The tests claim readOnly widgets remain focusable but
never assert focus movement; after sending the TAB event via
renderer.routeEngineEvent(keyEvent(3 /* TAB */)) assert the renderer's focused
id equals the widget id ("inp" for the input test, "ta" for the textarea test)
to ensure focus traversal includes readOnly fields (use the renderer API used
elsewhere in tests to read focus state, e.g. renderer.focusedId or
renderer.getFocusedId()). Add these assertions immediately after the TAB routing
and before simulating typing/enter/undo.
In `@packages/core/src/app/createApp.ts`:
- Around line 790-798: The current replaceRouteBindings removes existing route
bindings via applyKeybindingState(removeBindingsBySource(...)) before
registering new ones, so if registerAppBindings(..., method: "replaceRoutes")
throws, the old shortcuts are lost; change this to be transactional by capturing
the current keybindingState (snapshot) then attempt to register the new bindings
first (or validate them) and only apply the removal+replacement on success, or
wrap the operations in try/catch and on any error restore the snapshot via
applyKeybindingState(snapshot); update the code paths around
replaceRouteBindings, applyKeybindingState, removeBindingsBySource, and
registerAppBindings to perform the snapshot/restore or deferred removal so
failures do not delete existing ROUTE_KEYBINDING_SOURCE bindings.
In `@packages/core/src/app/widgetRenderer/inputEditing.ts`:
- Around line 224-226: The read-only guard only prevents edits produced by
applyInputEditEvent(...) but doesn't stop the manual cut (Ctrl+X) branch from
mutating value/history; update the manual cut branch to check meta.readOnly and
short-circuit (return ROUTE_NO_RENDER_CONSUMED) before performing any mutation.
Specifically, locate the manual cut handling code/path (the branch that performs
the cut and updates value/history) and add the same meta.readOnly guard as used
with edit.action so no destructive changes occur in read-only mode.
In `@packages/core/src/forms/useForm.ts`:
- Around line 1442-1446: If options.onSubmit throws synchronously,
submittingRef.current is never cleared which blocks future submits; in the
try/catch around options.onSubmit (where submitResult is assigned) ensure you
reset submittingRef.current = false in the catch before calling
failSubmit(error) and returning, and apply the same pattern for the later block
around the async submit (the code around where submittingRef.current is set to
true at the 1480-1484 region) so any thrown errors always clear
submittingRef.current (use a catch or finally to restore the ref).
- Around line 1463-1472: The submit flow currently uses Promise.then chains
(notably the submitResult.then call and the subsequent .then/.catch blocks
around lines referenced) — change this to async/await: mark the surrounding
function (where submitResult is created/used) as async, await the submitResult
call, set submittingRef.current = false and call finishSuccessfulSubmit() after
the await on success, and wrap the await in try/catch to call failSubmit(error)
on error; update all other .then/.catch usages in the same submit flow (the
blocks around the later 1486-1509 range) to the same async/try/catch pattern so
state updates and error handling use await instead of .then chains, keeping the
same calls to submittingRef.current, finishSuccessfulSubmit, and failSubmit.
In `@packages/core/src/runtime/__tests__/widgetMeta.test.ts`:
- Around line 301-305: The test assertion formatting in the expression using
traps.get("trap1")?.focusableIds has drifted; run the project's formatter (e.g.,
npm/yarn run format or Prettier) on
packages/core/src/runtime/__tests__/widgetMeta.test.ts or the changed hunk to
normalize spacing/line breaks so the assertion line matches the repo style, then
re-run CI.
In `@packages/core/src/runtime/router/layer.ts`:
- Around line 42-45: The code incorrectly treats falsy layer IDs as absent by
using "if (!layerId)" when reading const layerId = layerStack[layerStack.length
- 1]; — change this to explicitly check for stack emptiness or undefined (e.g.,
if (layerStack.length === 0) or if (layerId === undefined)) so valid falsy IDs
(0, '', false) are not treated as missing; update the conditional that returns
Object.freeze({ consumed: false }) accordingly in the layer lookup logic that
references layerStack and layerId.
- Around line 52-55: The branch that handles missing close callbacks currently
returns Object.freeze({ consumed: true }) which lets ESC be consumed without
closing a closable top layer; update the logic around onClose.get(layerId) /
closeCallback in layer handling to instead return an object that includes
closedLayerId when the layer is closable (or fall through to trigger the close
path), e.g. detect that layerId represents a closable top layer and, if
closeCallback is absent, invoke the standard close handling or return {
consumed: true, closedLayerId: layerId } so ESC isn't swallowed; adjust the code
that reads closeCallback and the consumers of the returned object to expect
closedLayerId when applicable.
In `@packages/core/src/runtime/widgetMeta.ts`:
- Around line 831-843: The current double-scan over _containerStack causes
focusables inside a trap to be added to both _trapFocusables and the nearest
outer _zoneFocusables; change to a single backward scan that examines each
container once and: if container.kind === "trap" push focusableId into
_trapFocusables.get(container.id) and break (stop scanning), else if
container.kind === "zone" push into _zoneFocusables.get(container.id) and break;
update the loop that references _containerStack, _zoneFocusables,
_trapFocusables and focusableId accordingly so trap descendants are not also
added to outer zones.
---
Outside diff comments:
In `@packages/core/src/app/createApp.ts`:
- Around line 41-52: Reorder the named imports in the import from
"../keybindings/index.js" to satisfy the project's organizeImports rule:
alphabetize the specifiers so they read createManagerState, getBindings,
getMode, getPendingChord, registerBindings, registerModes,
removeBindingsBySource, resetChordState, routeKeyEvent, setMode; leave the
module path unchanged.
In `@packages/core/src/forms/useForm.ts`:
- Around line 1-1551: The file packages/core/src/forms/useForm.ts is failing CI
due to formatting (Prettier) mismatch; fix by running the project's formatter
(e.g. run the repo's format script or Prettier: prettier --write
packages/core/src/forms/useForm.ts or npm/yarn run format), review changes
(especially around the exported useForm and helper functions), stage the
formatted file, and commit/push the updated file so the Prettier check passes in
CI.
---
Nitpick comments:
In `@packages/core/src/keybindings/manager.ts`:
- Around line 137-140: The ParseBindingsResult type erases sourceTag metadata by
returning KeyBinding<C>[]; update ParseBindingsResult to be generic over the
binding item so it preserves any source tagging (e.g., ParseBindingsResult<C, B
extends KeyBinding<C> = KeyBinding<C>>) and use that generic when returning
results from parseBindingsWithOptions and related functions; specifically change
the bindings field to readonly B[] (or readonly B[]) and update callsites/other
exported types (the parseBindingsWithOptions return type and any references
around lines referenced) so the concrete source-tagged binding type flows
through the result type instead of being widened to KeyBinding<C>.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 550f9b3d-71c6-404f-bf42-9cb754c24dff
📒 Files selected for processing (36)
docs/guide/concepts.mddocs/guide/hooks-reference.mddocs/guide/input-and-focus.mddocs/guide/lifecycle-and-updates.mddocs/recipes/form-validation.mdpackages/core/src/app/__tests__/keybindings.api.test.tspackages/core/src/app/__tests__/widgetRenderer.integration.test.tspackages/core/src/app/createApp.tspackages/core/src/app/widgetRenderer.tspackages/core/src/app/widgetRenderer/inputEditing.tspackages/core/src/app/widgetRenderer/mouseRouting.tspackages/core/src/forms/__tests__/form.disabled.test.tspackages/core/src/forms/__tests__/form.wizard.test.tspackages/core/src/forms/__tests__/useForm.test.tspackages/core/src/forms/index.tspackages/core/src/forms/types.tspackages/core/src/forms/useForm.tspackages/core/src/index.tspackages/core/src/keybindings/__tests__/keybinding.conflicts.test.tspackages/core/src/keybindings/__tests__/keybinding.modes.test.tspackages/core/src/keybindings/index.tspackages/core/src/keybindings/manager.tspackages/core/src/router/__tests__/keybindings.test.tspackages/core/src/router/__tests__/router.test.tspackages/core/src/router/keybindings.tspackages/core/src/router/router.tspackages/core/src/runtime/__tests__/focus.layers.test.tspackages/core/src/runtime/__tests__/focus.traps.test.tspackages/core/src/runtime/__tests__/wheelRouting.test.tspackages/core/src/runtime/__tests__/widgetMeta.test.tspackages/core/src/runtime/focus.tspackages/core/src/runtime/router/layer.tspackages/core/src/runtime/widgetMeta.tspackages/core/src/widgets/__tests__/layers.golden.test.tspackages/core/src/widgets/__tests__/modal.focus.test.tspackages/core/src/widgets/types.ts
RtlZeroMemory
force-pushed
the
fix/routing-focus-forms-hardening-clean
branch
from
1b758ed to
b113374
Compare
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.
Summary
useFormtyping and runtime behavior around same-turn state reads, async wizard gating, submit/edit locking, and field prop forwardingVerification
npm run buildnpm run build:nativenode scripts/run-tests.mjsTSX_DISABLE_CACHE=1 node --import tsx --test packages/core/src/app/__tests__/keybindings.api.test.ts packages/core/src/app/__tests__/widgetRenderer.integration.test.ts packages/core/src/forms/__tests__/form.disabled.test.ts packages/core/src/forms/__tests__/form.wizard.test.ts packages/core/src/forms/__tests__/useForm.test.ts packages/core/src/keybindings/__tests__/keybinding.conflicts.test.ts packages/core/src/keybindings/__tests__/keybinding.modes.test.ts packages/core/src/router/__tests__/keybindings.test.ts packages/core/src/router/__tests__/router.test.ts packages/core/src/runtime/__tests__/focus.layers.test.ts packages/core/src/runtime/__tests__/focus.traps.test.ts packages/core/src/runtime/__tests__/wheelRouting.test.ts packages/core/src/runtime/__tests__/widgetMeta.test.tsPTY Frame Audit
2,3,t,qnode scripts/frame-audit-report.mjs /tmp/rezi-routing-focus-audit.ndjson --latest-pid) reported:backend_submitted=1914worker_payload=1914worker_accepted=1914worker_completed=1914hash_mismatch_backend_vs_worker=0bridge,engineering, andcrewcrewproduced distinct drawlist hashes in the audit log while command streams remained validSummary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation