refactor(core): split widget metadata collection by concern#286
Merged
RtlZeroMemory merged 1 commit intomainfrom Mar 17, 2026
Merged
refactor(core): split widget metadata collection by concern#286RtlZeroMemory merged 1 commit intomainfrom
RtlZeroMemory merged 1 commit intomainfrom
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThis pull request restructures the widget metadata collection system by extracting implementation from a monolithic 1122-line file into four focused internal modules (collector, focusContainers, focusInfo, helpers). The public API surface remains unchanged through re-exports. No behavioral changes to runtime logic. 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)
📝 Coding Plan
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: 2
🧹 Nitpick comments (4)
packages/core/src/runtime/widgetMeta/focusContainers.ts (1)
85-107: Subtle difference: trap subtree allows nested zones.The
collectFocusableIdsInTrapSubtreefunction only stops at nested traps/modals (lines 93-95), allowing zones within traps. This is the correct behavior per the comment on line 237-238, but differs fromcollectFocusableIdsInSubtree. Consider adding a brief comment here explaining why zones are included.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/runtime/widgetMeta/focusContainers.ts` around lines 85 - 107, Add a short inline comment above the collectFocusableIdsInTrapSubtree function that explains why zones are intentionally not treated like nested traps/modals (i.e., zones inside a trap should be traversed and their focusable IDs collected), and note that this differs from collectFocusableIdsInSubtree which stops at zones; reference the function name collectFocusableIdsInTrapSubtree and the contrasting collectFocusableIdsInSubtree so future readers understand the deliberate behavior.packages/core/src/runtime/widgetMeta/helpers.ts (1)
193-210: Consider simplifying InputMeta construction.The conditional spread pattern creates two code paths. Since
Object.freezeis called regardless, you could simplify by always including the optional callbacks inmetaBasewhen defined.✨ Optional simplification
- const metaBase: InputMeta = { + const meta: InputMeta = Object.freeze({ instanceId: node.instanceId, value, disabled, readOnly, multiline, rows, wordWrap, - }; - const meta: InputMeta = - onInput || onBlur - ? Object.freeze({ - ...metaBase, - ...(onInput ? { onInput } : {}), - ...(onBlur ? { onBlur } : {}), - }) - : Object.freeze(metaBase); + ...(onInput && { onInput }), + ...(onBlur && { onBlur }), + }); m.set(id, meta);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/runtime/widgetMeta/helpers.ts` around lines 193 - 210, The InputMeta creation unnecessarily branches to either freeze metaBase or a spread including optional callbacks; simplify by always constructing a single object that starts from metaBase and conditionally spreads onInput/onBlur when present, then call Object.freeze once and store it (replace the ternary that defines meta with a single Object.freeze({ ...metaBase, ...(onInput ? { onInput } : {}), ...(onBlur ? { onBlur } : {}) }) and keep the subsequent m.set(id, meta) call); update references to metaBase/meta in this function accordingly.packages/core/src/runtime/widgetMeta/focusInfo.ts (1)
96-135: Consider addinginputto the visible label switch.The
kindToAnnouncementPrefixhandlesinput(line 102-103), butreadFocusableVisibleLabeldoesn't have aninputcase. Input widgets often have aplaceholderprop that could serve as a visible label fallback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/runtime/widgetMeta/focusInfo.ts` around lines 96 - 135, readFocusableVisibleLabel currently omits a branch for vnode.kind === "input" even though kindToAnnouncementPrefix recognizes "input"; update readFocusableVisibleLabel to handle the "input" kind by using the input's visible label sources (e.g., label element, aria-label, aria-labelledby) and fall back to the vnode.props.placeholder when no other visible label exists so placeholders serve as fallback visible labels for input widgets.packages/core/src/runtime/widgetMeta/collector.ts (1)
196-243: Consider extracting shared input metadata logic.The input metadata collection here (lines 196-243) duplicates the logic from
collectInputMetaByIdin helpers.ts (lines 157-221). Consider extracting a shared helper function to avoid drift between implementations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/runtime/widgetMeta/collector.ts` around lines 196 - 243, The block in the vnode.kind === "input" branch duplicates logic from helpers.ts:collectInputMetaById; extract the shared logic into a single helper (e.g., buildInputMeta or extractInputMeta) that accepts the props and node.instanceId and returns an InputMeta or null, then replace the duplicated code in collector.ts to call that helper before setting this._inputById; ensure the helper reproduces the same behavior (id/value validation, disabled/readOnly/multiline/rows/wordWrap parsing, function casting for onInput/onBlur, Object.freeze) and keep existing symbols like InputMeta, collectInputMetaById, vnode, and this._inputById to locate where to wire it in.
🤖 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/runtime/widgetMeta/collector.ts`:
- Around line 176-185: The current logic in collector.ts lets proto.openGated
overwrite the result from proto.disableable; update the checks so they are
combined instead of overwritten: when handling proto.disableable set enabled =
disabled !== true as now, and when handling proto.openGated update enabled by
combining with the prior value (e.g., enabled = enabled && (open === true))
before calling this._enabledById.set(interactiveId, enabled); reference the
proto.disableable/proto.openGated branches, vnode.props disabled/open reads,
interactiveId, and this._enabledById.set to locate where to apply the fix.
In `@packages/core/src/runtime/widgetMeta/helpers.ts`:
- Around line 101-113: The current logic in the block that reads id =
readInteractiveId(node.vnode) and uses getWidgetProtocol(node.vnode.kind) lets
the openGated check overwrite the prior disableable check; change the logic so
both checks are combined and a widget is enabled only if it passes all
applicable gates. Concretely, compute enabled by evaluating both gates (for
example: if proto.disableable, require (props.disabled !== true); if
proto.openGated, require (props.open === true)) and set enabled to the AND of
those results before calling m.set(id, enabled), referencing readInteractiveId,
getWidgetProtocol, node.vnode.props and m.set to locate the code.
---
Nitpick comments:
In `@packages/core/src/runtime/widgetMeta/collector.ts`:
- Around line 196-243: The block in the vnode.kind === "input" branch duplicates
logic from helpers.ts:collectInputMetaById; extract the shared logic into a
single helper (e.g., buildInputMeta or extractInputMeta) that accepts the props
and node.instanceId and returns an InputMeta or null, then replace the
duplicated code in collector.ts to call that helper before setting
this._inputById; ensure the helper reproduces the same behavior (id/value
validation, disabled/readOnly/multiline/rows/wordWrap parsing, function casting
for onInput/onBlur, Object.freeze) and keep existing symbols like InputMeta,
collectInputMetaById, vnode, and this._inputById to locate where to wire it in.
In `@packages/core/src/runtime/widgetMeta/focusContainers.ts`:
- Around line 85-107: Add a short inline comment above the
collectFocusableIdsInTrapSubtree function that explains why zones are
intentionally not treated like nested traps/modals (i.e., zones inside a trap
should be traversed and their focusable IDs collected), and note that this
differs from collectFocusableIdsInSubtree which stops at zones; reference the
function name collectFocusableIdsInTrapSubtree and the contrasting
collectFocusableIdsInSubtree so future readers understand the deliberate
behavior.
In `@packages/core/src/runtime/widgetMeta/focusInfo.ts`:
- Around line 96-135: readFocusableVisibleLabel currently omits a branch for
vnode.kind === "input" even though kindToAnnouncementPrefix recognizes "input";
update readFocusableVisibleLabel to handle the "input" kind by using the input's
visible label sources (e.g., label element, aria-label, aria-labelledby) and
fall back to the vnode.props.placeholder when no other visible label exists so
placeholders serve as fallback visible labels for input widgets.
In `@packages/core/src/runtime/widgetMeta/helpers.ts`:
- Around line 193-210: The InputMeta creation unnecessarily branches to either
freeze metaBase or a spread including optional callbacks; simplify by always
constructing a single object that starts from metaBase and conditionally spreads
onInput/onBlur when present, then call Object.freeze once and store it (replace
the ternary that defines meta with a single Object.freeze({ ...metaBase,
...(onInput ? { onInput } : {}), ...(onBlur ? { onBlur } : {}) }) and keep the
subsequent m.set(id, meta) call); update references to metaBase/meta in this
function accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1117dede-2602-4115-90e1-09711ed69110
📒 Files selected for processing (5)
packages/core/src/runtime/widgetMeta.tspackages/core/src/runtime/widgetMeta/collector.tspackages/core/src/runtime/widgetMeta/focusContainers.tspackages/core/src/runtime/widgetMeta/focusInfo.tspackages/core/src/runtime/widgetMeta/helpers.ts
Comment on lines
+176
to
+185
| if (proto.disableable) { | ||
| const disabled = (vnode.props as { disabled?: unknown }).disabled; | ||
| enabled = disabled !== true; | ||
| } | ||
| if (proto.openGated) { | ||
| const open = (vnode.props as { open?: unknown }).open; | ||
| enabled = open === true; | ||
| } | ||
| this._enabledById.set(interactiveId, enabled); | ||
| } |
There was a problem hiding this comment.
Same logic issue: openGated overwrites disableable result.
This has the same bug as collectEnabledMap in helpers.ts (lines 101-113). The enabled variable should combine both checks.
🐛 Proposed fix
if (proto.disableable) {
const disabled = (vnode.props as { disabled?: unknown }).disabled;
enabled = disabled !== true;
}
if (proto.openGated) {
const open = (vnode.props as { open?: unknown }).open;
- enabled = open === true;
+ enabled = enabled && open === true;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/runtime/widgetMeta/collector.ts` around lines 176 - 185,
The current logic in collector.ts lets proto.openGated overwrite the result from
proto.disableable; update the checks so they are combined instead of
overwritten: when handling proto.disableable set enabled = disabled !== true as
now, and when handling proto.openGated update enabled by combining with the
prior value (e.g., enabled = enabled && (open === true)) before calling
this._enabledById.set(interactiveId, enabled); reference the
proto.disableable/proto.openGated branches, vnode.props disabled/open reads,
interactiveId, and this._enabledById.set to locate where to apply the fix.
Comment on lines
+101
to
+113
| const id = readInteractiveId(node.vnode); | ||
| if (id !== null && !m.has(id)) { | ||
| let enabled = true; | ||
| const proto = getWidgetProtocol(node.vnode.kind); | ||
| if (proto.disableable) { | ||
| const disabled = (node.vnode.props as { disabled?: unknown }).disabled; | ||
| enabled = disabled !== true; | ||
| } | ||
| if (proto.openGated) { | ||
| const open = (node.vnode.props as { open?: unknown }).open; | ||
| enabled = open === true; | ||
| } | ||
| m.set(id, enabled); |
There was a problem hiding this comment.
Potential logic issue: openGated check overwrites disableable result.
When a widget is both disableable and openGated, the enabled variable is first set based on disabled !== true, then unconditionally overwritten by open === true. This means a widget that is open=true but disabled=true would incorrectly show as enabled.
Compare with isEnabledInteractive (lines 23-39) which returns null (disabled) if either condition fails. The logic here should likely use && or early returns to preserve both checks.
🐛 Proposed fix to preserve both checks
if (proto.disableable) {
const disabled = (node.vnode.props as { disabled?: unknown }).disabled;
enabled = disabled !== true;
}
if (proto.openGated) {
const open = (node.vnode.props as { open?: unknown }).open;
- enabled = open === true;
+ enabled = enabled && open === true;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/runtime/widgetMeta/helpers.ts` around lines 101 - 113, The
current logic in the block that reads id = readInteractiveId(node.vnode) and
uses getWidgetProtocol(node.vnode.kind) lets the openGated check overwrite the
prior disableable check; change the logic so both checks are combined and a
widget is enabled only if it passes all applicable gates. Concretely, compute
enabled by evaluating both gates (for example: if proto.disableable, require
(props.disabled !== true); if proto.openGated, require (props.open === true))
and set enabled to the AND of those results before calling m.set(id, enabled),
referencing readInteractiveId, getWidgetProtocol, node.vnode.props and m.set to
locate the code.
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
packages/core/src/runtime/widgetMeta.tsinto internal helper modules.Why
Validation
npm installnpm run build:nativenpm run lintnpm run typechecknpm run buildnode scripts/run-tests.mjs --filter "packages/core/dist/runtime/__tests__/widgetMeta"node scripts/run-tests.mjs --filter "packages/core/dist/runtime/__tests__/focus"node scripts/run-tests.mjs --filter "packages/core/dist/app/__tests__/focusDispatcher|packages/core/dist/app/__tests__/commandPaletteRouting"(matched 0 files becauserun-tests.mjstreats--filteras a literal substring)node scripts/run-tests.mjs --filter "packages/core/dist/app/__tests__/commandPaletteRouting.test.js"node scripts/run-tests.mjs --filter "packages/core/dist/app/__tests__/focusDispatcher.controller.test.js"node scripts/run-tests.mjsPTY / frame-audit evidence
docs/dev/live-pty-debugging.mdwith deterministic viewport runs.npm run build:native.stty rows 40 cols 120 && : > /tmp/rezi-widget-meta-frame.ndjson && : > /tmp/rezi-widget-meta-starship.log && env -u NO_COLOR REZI_STARSHIP_EXECUTION_MODE=worker REZI_STARSHIP_DEBUG=1 REZI_STARSHIP_DEBUG_LOG=/tmp/rezi-widget-meta-starship.log REZI_FRAME_AUDIT=1 REZI_FRAME_AUDIT_LOG=/tmp/rezi-widget-meta-frame.ndjson npx tsx packages/create-rezi/templates/starship/src/main.tsnode scripts/frame-audit-report.mjs /tmp/rezi-widget-meta-frame.ndjson --latest-pidrecords=10147,backend_submitted=480,worker_payload=480,worker_accepted=480,worker_completed=480,hash_mismatch_backend_vs_worker=0.bridgeandcrew.stty rows 30 cols 100 && : > /tmp/rezi-widget-meta-frame.ndjson && : > /tmp/rezi-widget-meta-harness.log && env -u NO_COLOR REZI_FRAME_AUDIT=1 REZI_FRAME_AUDIT_LOG=/tmp/rezi-widget-meta-frame.ndjson REZI_WIDGET_META_HARNESS_LOG=/tmp/rezi-widget-meta-harness.log node /tmp/rezi-widget-meta-pty.mjsTab), modal/focus-trap open and close (Enter,Tab,Esc), command palette path (Ctrl+P,de,Enter), and quit (q).help-trigger-presshelp-closekey ctrl+ppalette-select id=deploypalette-closeshutdown code=0node scripts/frame-audit-report.mjs /tmp/rezi-widget-meta-frame.ndjson --latest-pidrecords=255,backend_submitted=12,worker_payload=12,worker_accepted=12,worker_completed=12,native_summary_records=12,native_header_records=24,hash_mismatch_backend_vs_worker=0.hash32 0x8b5c2cde,byteLen 6956,cmdCount 111hash32 0x3a6b9eb9,byteLen 7152,cmdCount 115hash32 0x234e0f71,byteLen 7212,cmdCount 116hash32 0x1c5d7787,byteLen 7140,cmdCount 115hash32 0x67d8a2e3,byteLen 4212,cmdCount 76Summary by CodeRabbit