fix(radio-group): render disabled options consistently#305
Merged
RtlZeroMemory merged 4 commits intomainfrom Mar 19, 2026
Merged
fix(radio-group): render disabled options consistently#305RtlZeroMemory merged 4 commits intomainfrom
RtlZeroMemory merged 4 commits intomainfrom
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughRadioGroup adds per-option Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Widget as RadioGroup Widget
participant Renderer as Renderer
participant Theme as Theme/DesignSystem
User->>Widget: Tab (focus radio group)
User->>Widget: ArrowDown / ArrowUp
Widget->>Renderer: request next option (skip if option.disabled || group.disabled)
Renderer->>Theme: resolve rendering state & colors for optionDisabled
Theme-->>Renderer: disabled styles (rgba(..., 0.5))
Renderer-->>Widget: draw ops with disabled styling for specific options
Widget->>User: update visual focus/selected indicator
Widget->>Widget: invoke onChange if arrow navigation changed value
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 docstrings
🧪 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: 1
🧹 Nitpick comments (1)
packages/core/src/renderer/__tests__/radioGroupRecipeRendering.test.ts (1)
2-44: Expand this rendering assertion across all built-in themes.Right now this only validates
darkTheme; token regressions in other presets won’t be caught. Consider parameterizing this case over all shipped themes.As per coding guidelines, "Validate screens in all built-in themes (Dark, Light, Dimmed, High-contrast, Nord, Dracula) without code changes."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/renderer/__tests__/radioGroupRecipeRendering.test.ts` around lines 2 - 44, The test currently only uses the compiled darkTheme (dsTheme) and must be parameterized to run across all shipped theme presets; update the test to import all preset theme symbols (e.g., darkTheme, lightTheme, dimmedTheme, highContrastTheme, nordTheme, draculaTheme), iterate those presets, compile each with compileTheme, and for each compiled theme call renderOps(ui.radioGroup(...)) and run the same assertions (using findTextOp, ops, indicator checks and style comparisons) against the per-iteration theme instead of the single dsTheme variable so the disabled-option rendering is validated for every built-in theme.
🤖 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`:
- Line 795: The call to renderer.submitFrame(view, undefined, { cols: 40, rows:
10 }, defaultTheme, noRenderHooks()) may fail silently because its result is not
checked; update the test to capture the return value (e.g., const result = await
renderer.submitFrame(...)) and assert that result.ok (or equivalent success
property on the returned object) is true before proceeding to the existing
assertion about selected, ensuring the follow-up re-render actually succeeded;
reference the renderer.submitFrame call in the test and add an assertion on its
returned success flag.
---
Nitpick comments:
In `@packages/core/src/renderer/__tests__/radioGroupRecipeRendering.test.ts`:
- Around line 2-44: The test currently only uses the compiled darkTheme
(dsTheme) and must be parameterized to run across all shipped theme presets;
update the test to import all preset theme symbols (e.g., darkTheme, lightTheme,
dimmedTheme, highContrastTheme, nordTheme, draculaTheme), iterate those presets,
compile each with compileTheme, and for each compiled theme call
renderOps(ui.radioGroup(...)) and run the same assertions (using findTextOp,
ops, indicator checks and style comparisons) against the per-iteration theme
instead of the single dsTheme variable so the disabled-option rendering is
validated for every built-in theme.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ffc30468-1d2d-45a5-8b59-43cd62e42dfa
📒 Files selected for processing (6)
docs/widgets/catalog.jsondocs/widgets/radio-group.mdpackages/core/src/app/__tests__/widgetRenderer.integration.test.tspackages/core/src/renderer/__tests__/radioGroupRecipeRendering.test.tspackages/core/src/renderer/renderToDrawlist/widgets/renderFormWidgets.tspackages/core/src/widgets/__tests__/formWidgets.test.ts
packages/core/src/app/__tests__/widgetRenderer.integration.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/app/__tests__/widgetRenderer.integration.test.ts (1)
805-807: Assert ENTER returns no action to lock the non-confirming contract.At Line 805, capture the ENTER routing result and assert
actionisundefinedso this test directly verifies “Enter does not confirm/change selection.”Proposed patch
- renderer.routeEngineEvent(keyEvent(2 /* ENTER */)); + const enter = renderer.routeEngineEvent(keyEvent(2 /* ENTER */)); + assert.equal(enter.action, undefined); assert.deepEqual(changes, ["enterprise"]); assert.equal(selected, "enterprise");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/app/__tests__/widgetRenderer.integration.test.ts` around lines 805 - 807, The test should capture the return value of renderer.routeEngineEvent(keyEvent(2 /* ENTER */)) and assert that the returned object's action is undefined to verify ENTER doesn't confirm or change selection; modify the lines using renderer.routeEngineEvent and keyEvent to assign the result to a variable (e.g., const result = renderer.routeEngineEvent(...)) and add an assertion like assert.equal(result.action, undefined) while keeping the existing assertions on changes and selected (variables changes and selected).
🤖 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/core/src/app/__tests__/widgetRenderer.integration.test.ts`:
- Around line 805-807: The test should capture the return value of
renderer.routeEngineEvent(keyEvent(2 /* ENTER */)) and assert that the returned
object's action is undefined to verify ENTER doesn't confirm or change
selection; modify the lines using renderer.routeEngineEvent and keyEvent to
assign the result to a variable (e.g., const result =
renderer.routeEngineEvent(...)) and add an assertion like
assert.equal(result.action, undefined) while keeping the existing assertions on
changes and selected (variables changes and selected).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c5fcb46b-a7da-4e13-b51b-80c968f457c5
📒 Files selected for processing (2)
packages/core/src/app/__tests__/widgetRenderer.integration.test.tspackages/core/src/renderer/__tests__/radioGroupRecipeRendering.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/renderer/tests/radioGroupRecipeRendering.test.ts
RtlZeroMemory
force-pushed
the
fix/radio-group-disabled-options
branch
from
05d137c to
0483c4b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/renderer/__tests__/radioGroupRecipeRendering.test.ts (1)
28-60: Test validates disabled styling for both label and selection indicator.The test correctly:
- Renders a radio group with a disabled and selected option (
value: "pro"matches{ value: "pro", ..., disabled: true })- Verifies the label renders with
disabled.fgcolor- Locates the selection indicator
"(o)"by matching y-coordinate- Verifies the indicator also uses
disabled.fgcolorConsider using parameterized tests or
describe.eachfor clearer per-theme failure output.♻️ Optional: parameterized test for clearer failure messages
describe("radioGroup recipe rendering", () => { - test("renders disabled options with disabled recipe colors", () => { - for (const [name, theme] of DS_THEMES) { + for (const [name, theme] of DS_THEMES) { + test(`${name}: renders disabled options with disabled recipe colors`, () => { const ops = renderOps( ui.radioGroup({ // ... }), { viewport: { cols: 40, rows: 4 }, theme }, ); const label = findTextOp(ops, "Pro"); - assert.ok(label && label.kind === "drawText", `${name} theme should render disabled label`); - if (!label || label.kind !== "drawText") continue; + assert.ok(label && label.kind === "drawText", "should render disabled label"); + if (!label || label.kind !== "drawText") return; // ...remaining assertions without name prefix... - } - }); + }); + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/renderer/__tests__/radioGroupRecipeRendering.test.ts` around lines 28 - 60, Refactor the "renders disabled options with disabled recipe colors" test to parameterize per theme (use test.each or describe.each over DS_THEMES) so failures show which theme failed; locate the current loop over DS_THEMES in the test and replace it by a parameterized test that receives (name, theme) and runs the same assertions that use renderOps, ui.radioGroup, findTextOp, label and indicator checks, keeping the exact assertions for indicator.style?.fg and label.style?.fg against theme.colors["disabled.fg"] so behavior is unchanged but failure output is per-theme.
🤖 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/core/src/renderer/__tests__/radioGroupRecipeRendering.test.ts`:
- Around line 28-60: Refactor the "renders disabled options with disabled recipe
colors" test to parameterize per theme (use test.each or describe.each over
DS_THEMES) so failures show which theme failed; locate the current loop over
DS_THEMES in the test and replace it by a parameterized test that receives
(name, theme) and runs the same assertions that use renderOps, ui.radioGroup,
findTextOp, label and indicator checks, keeping the exact assertions for
indicator.style?.fg and label.style?.fg against theme.colors["disabled.fg"] so
behavior is unchanged but failure output is per-theme.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 78e192e8-71e6-4945-b9a7-47c24004a2ba
📒 Files selected for processing (6)
docs/widgets/catalog.jsondocs/widgets/radio-group.mdpackages/core/src/app/__tests__/widgetRenderer.integration.test.tspackages/core/src/renderer/__tests__/radioGroupRecipeRendering.test.tspackages/core/src/renderer/renderToDrawlist/widgets/renderFormWidgets.tspackages/core/src/widgets/__tests__/formWidgets.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/core/src/app/tests/widgetRenderer.integration.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/widgets/tests/formWidgets.test.ts
- packages/core/src/renderer/renderToDrawlist/widgets/renderFormWidgets.ts
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
Render disabled radio options with disabled styling that matches the existing routing contract, and update the docs and catalog to describe the actual arrow-driven interaction model.
Problem
radioGroupkeyboard routing already skipped disabled options, but the renderer showed disabled entries like active ones. The docs also described Enter-based confirmation even though the runtime only changes selection through directional navigation.Root cause
The routing and rendering contracts drifted apart. Per-option disabled state was handled in keyboard routing but not reflected in renderer styling, and the documentation kept an older interaction description that no longer matched the shipped widget.
Fix
Apply disabled styling per radio option in the renderer, add focused regression coverage for disabled-option rendering and arrow-key contract behavior, and update the radio group docs and catalog to reflect the current focus, click, and keyboard semantics.
Tests
npm install --prefer-offline
npm run lint
npm run typecheck
npm run build
node scripts/run-tests.mjs --filter "formWidgets.test.js"
node scripts/run-tests.mjs --filter "radioGroupRecipeRendering.test.js"
node scripts/run-tests.mjs --filter "widgetRenderer.integration.test.js"
node scripts/run-tests.mjs
Notes
This intentionally keeps radio group selection arrow-driven; it does not introduce click-to-select or Enter-to-confirm behavior.
Summary by CodeRabbit
New Features
Behavior Changes
Documentation
Tests