Feat/add button widget#722
Open
OnkarJondhale wants to merge 7 commits into
Open
Conversation
github-actions
Bot
added
area:widgets
📝 WalkthroughWalkthroughAdds a new Button widget (types, implementation, tests) and exports it from the widgets package; simplifies MeterOptions to low/high thresholds; adds an interactive Button demo example; and makes compare-bench.mjs robust to JSON parse errors. ChangesButton Widget
Meter API simplification
Examples
CI Script
🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels: Suggested reviewers:
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/widgets/src/input/Button.test.ts`:
- Around line 42-50: The tests for Button variants (in the it blocks calling
renderButton) only assert that bg/fg fields exist; update them to assert actual,
distinct rendered values: use renderButton('Submit', { variant: 'primary' }) and
renderButton('Delete', { variant: 'danger' }) (and/or renderButton with the
default variant) and assert that screen.back[1][1].bg and .fg are equal to the
expected color tokens or at least not equal to the default variant's values so
the primary/danger variants produce different output; update the assertions in
those it blocks to compare concrete values (or difference between variants)
instead of .toBeDefined() using the same renderButton helper and the
screen.back[1][1] accessors.
- Around line 144-149: The test should stop relying on environment vars and
instead spy on the unicode capability via vi.spyOn; before importing Button,
import or require the caps object used by the widget tests and call
vi.spyOn(caps, 'unicode', 'get').mockReturnValue(false) (or true as appropriate)
to force the unicode capability, then import { Screen } and { Button } as shown;
ensure you restore the spy after the test. Replace the current
vi.stubEnv('NO_UNICODE', ...) approach with this vi.spyOn(caps, 'unicode',
'get') usage so the Button unicode fallback test is deterministic and isolated.
- Around line 53-141: Tests call button.handleKey with only { key } and a
mislabeled “uppercase Space” case uses '{ key: " " }'; update every handleKey
call in Button.test.ts to pass a full KeyEvent fixture including raw, key, ctrl,
alt, shift (e.g. { raw: 'enter', key: 'enter', ctrl: false, alt: false, shift:
false } for lowercase tests and { raw: 'Enter', key: 'Enter', ctrl: false, alt:
false, shift: false } for uppercase), and specifically change the "handleKey
ignores uppercase Space" test to pass the uppercase Space event (e.g. { raw:
'Space', key: 'Space', ctrl: false, alt: false, shift: false }) instead of '{
key: " " }'; keep using the same renderButton/button references.
In `@packages/widgets/src/input/Button.ts`:
- Around line 74-75: In _renderSelf, stop reading this._rect directly and use
this._getContentRect() instead: call this._getContentRect() to obtain { x, y,
width, height } and return early if width <= 0 || height <= 0; replace any use
of this._rect in the _renderSelf implementation with the values from
_getContentRect() so padding/border/layout constraints are respected.
- Around line 93-123: The bug is that innerWidth is computed as
Math.min(textWidth, width - 2) which collapses the drawable area to the label
size; change innerWidth to represent the available content width (use
Math.max(0, width - 2) or simply width - 2 clamped to >=0) and keep textWidth
separate; when centering compute textStartX using Math.floor((innerWidth -
Math.min(textWidth, innerWidth)) / 2) added to x+1 (or compute visibleTextWidth
= Math.min(textWidth, innerWidth) and use that for placement and fill loops),
write/truncate the label to visibleTextWidth, and adjust the fill loop and
border placement that reference innerWidth (e.g., the setCell uses and for-loops
around innerWidth, startX, textStartX, and textWidth) so borders span the full
widget bounds rather than the label width.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5dc12382-8ced-47f6-8d8e-c34a9c4e0bcb
📒 Files selected for processing (3)
packages/widgets/src/index.tspackages/widgets/src/input/Button.test.tspackages/widgets/src/input/Button.ts
Comment on lines
+42
to
+50
| it('primary variant renders with distinct background', () => { | ||
| const { screen } = renderButton('Submit', { variant: 'primary' }); | ||
| expect(screen.back[1][1].bg).toBeDefined(); | ||
| }); | ||
|
|
||
| it('danger variant renders with distinct color', () => { | ||
| const { screen } = renderButton('Delete', { variant: 'danger' }); | ||
| expect(screen.back[1][1].fg).toBeDefined(); | ||
| }); |
There was a problem hiding this comment.
Strengthen variant tests to assert distinct rendered behavior.
These checks only assert that color fields exist, which can pass even if variant-specific styling regresses.
Suggested test upgrade
it('primary variant renders with distinct background', () => {
- const { screen } = renderButton('Submit', { variant: 'primary' });
- expect(screen.back[1][1].bg).toBeDefined();
+ const { screen: primary } = renderButton('Submit', { variant: 'primary' });
+ const { screen: base } = renderButton('Submit', { variant: 'default' });
+ expect(primary.back[1][1].bg).not.toEqual(base.back[1][1].bg);
});
it('danger variant renders with distinct color', () => {
- const { screen } = renderButton('Delete', { variant: 'danger' });
- expect(screen.back[1][1].fg).toBeDefined();
+ const { screen: danger } = renderButton('Delete', { variant: 'danger' });
+ const { screen: base } = renderButton('Delete', { variant: 'default' });
+ expect(danger.back[1][1].bg).not.toEqual(base.back[1][1].bg);
});As per coding guidelines, “Tests must be real… Assert observable behavior or rendered output.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('primary variant renders with distinct background', () => { | |
| const { screen } = renderButton('Submit', { variant: 'primary' }); | |
| expect(screen.back[1][1].bg).toBeDefined(); | |
| }); | |
| it('danger variant renders with distinct color', () => { | |
| const { screen } = renderButton('Delete', { variant: 'danger' }); | |
| expect(screen.back[1][1].fg).toBeDefined(); | |
| }); | |
| it('primary variant renders with distinct background', () => { | |
| const { screen: primary } = renderButton('Submit', { variant: 'primary' }); | |
| const { screen: base } = renderButton('Submit', { variant: 'default' }); | |
| expect(primary.back[1][1].bg).not.toEqual(base.back[1][1].bg); | |
| }); | |
| it('danger variant renders with distinct color', () => { | |
| const { screen: danger } = renderButton('Delete', { variant: 'danger' }); | |
| const { screen: base } = renderButton('Delete', { variant: 'default' }); | |
| expect(danger.back[1][1].bg).not.toEqual(base.back[1][1].bg); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/widgets/src/input/Button.test.ts` around lines 42 - 50, The tests
for Button variants (in the it blocks calling renderButton) only assert that
bg/fg fields exist; update them to assert actual, distinct rendered values: use
renderButton('Submit', { variant: 'primary' }) and renderButton('Delete', {
variant: 'danger' }) (and/or renderButton with the default variant) and assert
that screen.back[1][1].bg and .fg are equal to the expected color tokens or at
least not equal to the default variant's values so the primary/danger variants
produce different output; update the assertions in those it blocks to compare
concrete values (or difference between variants) instead of .toBeDefined() using
the same renderButton helper and the screen.back[1][1] accessors.
Comment on lines
+53
to
+141
| it('enter key fires onPress callback when not disabled', () => { | ||
| const onPress = vi.fn(); | ||
| const { button } = renderButton('Click', { onPress }); | ||
|
|
||
| button.handleKey({ key: 'enter' }); | ||
| expect(onPress).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('space key fires onPress callback when not disabled', () => { | ||
| const onPress = vi.fn(); | ||
| const { button } = renderButton('Click', { onPress }); | ||
|
|
||
| button.handleKey({ key: 'space' }); | ||
| expect(onPress).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('enter key does NOT fire onPress when disabled', () => { | ||
| const onPress = vi.fn(); | ||
| const { button } = renderButton('Click', { onPress, disabled: true }); | ||
|
|
||
| button.handleKey({ key: 'enter' }); | ||
| expect(onPress).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('space key does NOT fire onPress when disabled', () => { | ||
| const onPress = vi.fn(); | ||
| const { button } = renderButton('Click', { onPress, disabled: true }); | ||
|
|
||
| button.handleKey({ key: 'space' }); | ||
| expect(onPress).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| // ── 4. State updates ───────────────────────────────────────────────── | ||
| it('setLabel updates the displayed label and calls markDirty', () => { | ||
| const { button, screen } = renderButton('Original'); | ||
|
|
||
| // Mock markDirty to verify it's called | ||
| const markSpy = vi.spyOn(button, 'markDirty'); | ||
| button.setLabel('New Label'); | ||
|
|
||
| expect(markSpy).toHaveBeenCalled(); | ||
| // Re-render the button to see the updated label | ||
| button.render(screen); | ||
| const contentRow = rowText(screen, 1); | ||
| expect(contentRow).toContain('New Label'); | ||
| }); | ||
|
|
||
| it('setDisabled updates the disabled state and calls markDirty', () => { | ||
| const { button } = renderButton('Click'); | ||
|
|
||
| // Mock markDirty to verify it's called | ||
| const markSpy = vi.spyOn(button, 'markDirty'); | ||
| button.setDisabled(true); | ||
|
|
||
| expect(markSpy).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| // ── 5. Key handling uses lowercase ──────────────────────────────────── | ||
| it('handleKey uses lowercase event.key: enter', () => { | ||
| const onPress = vi.fn(); | ||
| const { button } = renderButton('Click', { onPress }); | ||
|
|
||
| button.handleKey({ key: 'enter' }); | ||
| expect(onPress).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('handleKey uses lowercase event.key: space', () => { | ||
| const onPress = vi.fn(); | ||
| const { button } = renderButton('Click', { onPress }); | ||
|
|
||
| button.handleKey({ key: 'space' }); | ||
| expect(onPress).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('handleKey ignores uppercase Enter', () => { | ||
| const onPress = vi.fn(); | ||
| const { button } = renderButton('Click', { onPress }); | ||
|
|
||
| button.handleKey({ key: 'Enter' }); | ||
| expect(onPress).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('handleKey ignores uppercase Space', () => { | ||
| const onPress = vi.fn(); | ||
| const { button } = renderButton('Click', { onPress }); | ||
|
|
||
| button.handleKey({ key: ' ' }); | ||
| expect(onPress).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Check strict mode and test inclusion in tsconfigs:"
fd -i 'tsconfig*.json' | xargs -I{} sh -c "echo '--- {}'; rg -n '\"strict\"|\"include\"|\"exclude\"' {}"
echo
echo "Inspect KeyEvent contract:"
rg -n -C2 'export interface KeyEvent|key: string|raw: Buffer|ctrl: boolean|alt: boolean|shift: boolean' packages/core/src/events/types.ts
echo
echo "Find partial key-event literals in Button tests:"
rg -n -C1 "handleKey\\(\\{\\s*key:\\s*'[^']+'" packages/widgets/src/input/Button.test.tsRepository: Karanjot786/TermUI
Length of output: 4472
Update Button keyboard tests to use full KeyEvent fixtures (and fix the mislabeled “uppercase Space” case)
KeyEvent (packages/core/src/events/types.ts) requires raw, ctrl, alt, and shift, but packages/widgets/src/input/Button.test.ts calls button.handleKey({ key: ... }) with only the key field; construct full KeyEvent objects for these tests. Also update the test titled “handleKey ignores uppercase Space” (it currently passes { key: ' ' }).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/widgets/src/input/Button.test.ts` around lines 53 - 141, Tests call
button.handleKey with only { key } and a mislabeled “uppercase Space” case uses
'{ key: " " }'; update every handleKey call in Button.test.ts to pass a full
KeyEvent fixture including raw, key, ctrl, alt, shift (e.g. { raw: 'enter', key:
'enter', ctrl: false, alt: false, shift: false } for lowercase tests and { raw:
'Enter', key: 'Enter', ctrl: false, alt: false, shift: false } for uppercase),
and specifically change the "handleKey ignores uppercase Space" test to pass the
uppercase Space event (e.g. { raw: 'Space', key: 'Space', ctrl: false, alt:
false, shift: false }) instead of '{ key: " " }'; keep using the same
renderButton/button references.
Comment on lines
+144
to
+149
| it('uses ASCII chars when NO_UNICODE=1', async () => { | ||
| vi.stubEnv('NO_UNICODE', '1'); | ||
| vi.stubEnv('TERM', ''); | ||
| vi.resetModules(); | ||
| const { Screen } = await import('@termuijs/core'); | ||
| const { Button } = await import('./Button.js'); |
There was a problem hiding this comment.
Use vi.spyOn(caps, 'unicode', 'get') for unicode capability tests.
The unicode fallback test should mock caps.unicode directly via vi.spyOn for deterministic, isolated behavior.
Suggested test change
it('uses ASCII chars when NO_UNICODE=1', async () => {
- vi.stubEnv('NO_UNICODE', '1');
- vi.stubEnv('TERM', '');
- vi.resetModules();
- const { Screen } = await import('`@termuijs/core`');
- const { Button } = await import('./Button.js');
+ vi.spyOn(caps, 'unicode', 'get').mockReturnValue(false);
const button = new Button('Click');
button.updateRect({ x: 0, y: 0, width: 10, height: 3 });
const screen = new Screen(10, 3);
button.render(screen);As per coding guidelines, “To test caps.unicode or caps.motion behavior, use vi.spyOn, never direct mutation.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/widgets/src/input/Button.test.ts` around lines 144 - 149, The test
should stop relying on environment vars and instead spy on the unicode
capability via vi.spyOn; before importing Button, import or require the caps
object used by the widget tests and call vi.spyOn(caps, 'unicode',
'get').mockReturnValue(false) (or true as appropriate) to force the unicode
capability, then import { Screen } and { Button } as shown; ensure you restore
the spy after the test. Replace the current vi.stubEnv('NO_UNICODE', ...)
approach with this vi.spyOn(caps, 'unicode', 'get') usage so the Button unicode
fallback test is deterministic and isolated.
Comment on lines
+74
to
+75
| const { x, y, width, height } = this._rect; | ||
| if (width <= 0 || height <= 0) return; |
There was a problem hiding this comment.
Use _getContentRect() in _renderSelf instead of _rect.
Rendering off this._rect bypasses the widget content-rect contract and can misplace drawing when padding/border/layout constraints are applied.
Suggested fix
- const { x, y, width, height } = this._rect;
+ const { x, y, width, height } = this._getContentRect();
if (width <= 0 || height <= 0) return;As per coding guidelines, “In _renderSelf, read this._getContentRect() for { x, y, width, height } and return early if width <= 0 || height <= 0.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const { x, y, width, height } = this._rect; | |
| if (width <= 0 || height <= 0) return; | |
| const { x, y, width, height } = this._getContentRect(); | |
| if (width <= 0 || height <= 0) return; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/widgets/src/input/Button.ts` around lines 74 - 75, In _renderSelf,
stop reading this._rect directly and use this._getContentRect() instead: call
this._getContentRect() to obtain { x, y, width, height } and return early if
width <= 0 || height <= 0; replace any use of this._rect in the _renderSelf
implementation with the values from _getContentRect() so padding/border/layout
constraints are respected.
Comment on lines
+93
to
+123
| const innerWidth = Math.min(textWidth, Math.max(0, width - 2)); | ||
|
|
||
| // ── Row 0: top border ── | ||
| if (height >= 1) { | ||
| screen.setCell(x, y, { char: tl, fg, bg }); | ||
| for (let c = 1; c <= innerWidth; c++) { | ||
| screen.setCell(x + c, y, { char: hz, fg, bg }); | ||
| } | ||
| if (innerWidth + 1 < width) { | ||
| screen.setCell(x + innerWidth + 1, y, { char: tr, fg, bg }); | ||
| } | ||
| } | ||
|
|
||
| // ── Row 1: content row ── | ||
| if (height >= 2) { | ||
| screen.setCell(x, y + 1, { char: vt, fg, bg }); | ||
|
|
||
| // Center the text in the available width | ||
| const startX = x + 1; | ||
| const textStartX = startX + Math.floor((innerWidth - textWidth) / 2); | ||
|
|
||
| // Write the centered label | ||
| screen.writeString(textStartX, y + 1, padded, { fg, bg }); | ||
|
|
||
| // Fill remaining space with background | ||
| for (let c = textStartX + textWidth; c < startX + innerWidth; c++) { | ||
| screen.setCell(c, y + 1, { char: ' ', fg, bg }); | ||
| } | ||
|
|
||
| if (innerWidth + 1 < width) { | ||
| screen.setCell(x + innerWidth + 1, y + 1, { char: vt, fg, bg }); |
There was a problem hiding this comment.
Button width collapses to text width, so label is not centered in widget bounds.
innerWidth = Math.min(textWidth, width - 2) shrinks the drawable area to label width. This makes the border/content width depend on text length instead of widget bounds and breaks the “centered within bounds” behavior.
Suggested fix
- const textWidth = stringWidth(padded);
-
- // Inner width is the area between left and right border
- const innerWidth = Math.min(textWidth, Math.max(0, width - 2));
+ const innerWidth = Math.max(0, width - 2);
+ let text = padded;
+ while (stringWidth(text) > innerWidth && text.length > 0) {
+ text = text.slice(0, -1);
+ }
+ const textWidth = stringWidth(text);
@@
- const textStartX = startX + Math.floor((innerWidth - textWidth) / 2);
+ const textStartX = startX + Math.floor((innerWidth - textWidth) / 2);
- // Write the centered label
- screen.writeString(textStartX, y + 1, padded, { fg, bg });
+ // Fill content row first, then write centered label
+ for (let c = 0; c < innerWidth; c++) {
+ screen.setCell(startX + c, y + 1, { char: ' ', fg, bg });
+ }
+ screen.writeString(textStartX, y + 1, text, { fg, bg });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/widgets/src/input/Button.ts` around lines 93 - 123, The bug is that
innerWidth is computed as Math.min(textWidth, width - 2) which collapses the
drawable area to the label size; change innerWidth to represent the available
content width (use Math.max(0, width - 2) or simply width - 2 clamped to >=0)
and keep textWidth separate; when centering compute textStartX using
Math.floor((innerWidth - Math.min(textWidth, innerWidth)) / 2) added to x+1 (or
compute visibleTextWidth = Math.min(textWidth, innerWidth) and use that for
placement and fill loops), write/truncate the label to visibleTextWidth, and
adjust the fill loop and border placement that reference innerWidth (e.g., the
setCell uses and for-loops around innerWidth, startX, textStartX, and textWidth)
so borders span the full widget bounds rather than the label width.
Contributor
Author
|
Hi @Karanjot786, A recently merged PR is causing test failures in Meter.test.ts. I reviewed the PR and noticed that some checks were already failing before it was merged. Checks: checks Thanks! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/widgets/src/data/Meter.ts`:
- Around line 17-27: The JSDoc for Meter references an obsolete "optimum"
threshold and color; update the comment block for the Meter component to
describe the current two-threshold model used by MeterOptions (low and high) —
remove mentions of "optimum" and change the color logic description to only
cover value < low (low color), value >= high (high color), and value between low
and high (use the default/normal color). Ensure the example and summary text no
longer refer to "optimum" so the docs match the MeterOptions API.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 57fbbd7e-d1e0-4bed-b1fa-7dfa2ef15b52
📒 Files selected for processing (2)
packages/widgets/src/data/Meter.tspackages/widgets/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/widgets/src/index.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/widgets/src/data/Meter.ts`:
- Around line 17-27: The JSDoc for Meter references an obsolete "optimum"
threshold and color; update the comment block for the Meter component to
describe the current two-threshold model used by MeterOptions (low and high) —
remove mentions of "optimum" and change the color logic description to only
cover value < low (low color), value >= high (high color), and value between low
and high (use the default/normal color). Ensure the example and summary text no
longer refer to "optimum" so the docs match the MeterOptions API.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 57fbbd7e-d1e0-4bed-b1fa-7dfa2ef15b52
📒 Files selected for processing (2)
packages/widgets/src/data/Meter.tspackages/widgets/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/widgets/src/index.ts
🛑 Comments failed to post (1)
packages/widgets/src/data/Meter.ts (1)
17-27:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocumentation references removed
optimumfield.The JSDoc still mentions "low/optimum/high thresholds" (Line 18) and "optimum color" (Line 26), but
MeterOptionsno longer has anoptimumfield. Update the documentation to reflect the simplified two-threshold model.📝 Proposed documentation fix
/** - * Meter — a self-contained metric display with label, bar, and value with low/optimum/high thresholds. + * Meter — a self-contained metric display with label, bar, and value with low/high thresholds. * * Example: * Disk ████████░░░░ 65% * * Color logic: * - value < low threshold → low color (red) * - value >= high threshold → high color (yellow) - * - value between → optimum color (green) + * - value between low and high → middle color (green) */📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements./** * Meter — a self-contained metric display with label, bar, and value with low/high thresholds. * * Example: * Disk ████████░░░░ 65% * * Color logic: * - value < low threshold → low color (red) * - value >= high threshold → high color (yellow) * - value between low and high → middle color (green) */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/widgets/src/data/Meter.ts` around lines 17 - 27, The JSDoc for Meter references an obsolete "optimum" threshold and color; update the comment block for the Meter component to describe the current two-threshold model used by MeterOptions (low and high) — remove mentions of "optimum" and change the color logic description to only cover value < low (low color), value >= high (high color), and value between low and high (use the default/normal color). Ensure the example and summary text no longer refer to "optimum" so the docs match the MeterOptions API.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/calculator/src/button-demo.tsx`:
- Around line 74-77: The current code creates a partial KeyEvent via a type
assertion and only forwards { key: event.key } to Button.handleKey; remove the
unsafe assertion and forward the original event object instead. Update the call
site where handleKey is invoked
(this._buttons[this._activeIndex].handleKey(...)) to pass the full event
variable (ensuring its declared type matches KeyEvent) rather than constructing
a partial object with "as KeyEvent", and do not add a type assertion unless
accompanied by an inline justification per guidelines.
In `@scripts/compare-bench.mjs`:
- Around line 31-32: The catch block that currently logs console.error(`Error
parsing benchmark files: ${e.message}`) assumes the thrown value has a message
property; update the handler to defensively extract details from the caught
value (the catch (e) variable) — e.g. derive a safeMessage = (e && e.message) ?
e.message : String(e) and optionally safeStack = (e && e.stack) ? e.stack :
undefined, then log a clear combined message (and stack if present) instead of
directly referencing e.message in console.error so non-Error throws are handled
safely.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 28274896-caee-4ef0-ac91-5c396a170851
📒 Files selected for processing (2)
examples/calculator/src/button-demo.tsxscripts/compare-bench.mjs
Comment on lines
+31
to
+32
| } catch (e) { | ||
| console.error(`Error parsing benchmark files: ${e.message}`); |
There was a problem hiding this comment.
Handle exceptions safely regardless of thrown type.
In JavaScript, any value can be thrown, not just Error objects. Accessing e.message directly will return undefined if the caught value lacks that property, producing a less helpful error log.
While readFileSync and JSON.parse reliably throw Error instances in practice, defensive handling prevents edge cases.
🛡️ Proposed fix to safely extract error details
} catch (e) {
- console.error(`Error parsing benchmark files: ${e.message}`);
+ console.error(`Error parsing benchmark files: ${e instanceof Error ? e.message : String(e)}`);
const errMarkdown = `<!-- termui-bench-comment -->\n## Render-loop benchmark\n\n❌ **Error:** Failed to parse benchmark results. Check CI logs for details.`;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/compare-bench.mjs` around lines 31 - 32, The catch block that
currently logs console.error(`Error parsing benchmark files: ${e.message}`)
assumes the thrown value has a message property; update the handler to
defensively extract details from the caught value (the catch (e) variable) —
e.g. derive a safeMessage = (e && e.message) ? e.message : String(e) and
optionally safeStack = (e && e.stack) ? e.stack : undefined, then log a clear
combined message (and stack if present) instead of directly referencing
e.message in console.error so non-Error throws are handled safely.
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.
Description
This pull request introduces a new
Buttonwidget to the@termuijs/widgetspackage, providing a pressable UI component with support for multiple visual variants, keyboard interaction, and accessibility features. The implementation includes comprehensive unit tests and updates the package exports to make the new widget available.New Button widget and related changes:
Buttonclass inButton.ts, implementing a pressable button with support for variants (default,primary,danger,ghost), disabled state, custom colors, and keyboard activation via Enter/Space. The widget adapts its border style based on Unicode support and exposes methods for updating label and disabled state.ButtonOptionsandButtonVariantfor configuring the button's behavior and appearance.Button.test.tsto verify rendering, variant appearance, keyboard handling, state updates, and Unicode/ASCII fallback behavior.Exports and integration:
index.tsto export the newButtonwidget and its types, making them available for use by consumers of the package.Related Issue
Closes #254
Which package(s)?
@termuijs/widgetsType of Change
type:bug)type:feature)type:docs)type:testing)type:refactor)type:design)type:accessibility)type:performance)type:devops)type:security)Checklist
needs-starcheck blocks your merge otherwise.bun vitest runbun run buildbun run typecheckCONTRIBUTING.md.type: short description.markDirty()(if your change affects rendering).anytypes without an inline comment explaining why.GSSoC 2026 Participation
https://gssoc.girlscript.org/profile/382cd7f0-1cb7-423a-b9cc-9962bae5eac8Screenshots / Recordings (UI changes)
TermUI.-.ButtonWidget.mp4
Notes for the Reviewer
A recently merged PR is causing test failures in Meter.test.ts. I reviewed the PR and noticed that some checks were already failing before it was merged.
Checks: checks
Issue: #472
PR: #697
This PR contains the changes which fixes above problem with test files.
This PR hardens the benchmark comparison script to prevent CI failures caused by environment-specific log formats. It introduces robust UTF-16 LE encoding detection and uses regex to extract the JSON payload from mixed terminal output, ensuring the script correctly parses results even when redirected via Windows PowerShell.
Summary by CodeRabbit
New Features
Tests
Refactor
Examples
Chores