fix: resolve 4 bugs in termui - #3571
Conversation
github-actions
Bot
added
area:examples
📝 WalkthroughWalkthroughThe changes improve floating-point rounding in progress displays and add console error logging for rejected promises in the form and development server flows. ChangesBehavior and diagnostics
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.6)packages/dev-server/src/server.tsFile contains syntax errors that prevent linting: Line 383: Expected a parenthesis '(' but instead found '=>'. packages/ui/src/Form.tsFile contains syntax errors that prevent linting: Line 142: Expected a statement but instead found '.catch(err => console.error("Promise.all failed:", err))'. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 3
🧹 Nitpick comments (1)
packages/ui/src/Switch.ts (1)
117-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for knob-position rounding.
packages/ui/src/Switch.test.tschecks rendering bounds, but it does not assert which track cell receives the knob. Add tests around the rounding thresholds for_animProgressto protect this precision fix.🤖 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/ui/src/Switch.ts` at line 117, Add regression coverage in Switch.test.ts for the knob-position calculation used by Switch, targeting the _animProgress rounding thresholds around the Math.round(... + Number.EPSILON) behavior. Assert that each boundary value places the knob in the expected track cell while preserving existing rendering-bound tests.
🤖 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/pomodoro-timer/src/index.tsx`:
- Line 185: Update the label calculation in the progress-rendering method to add
Number.EPSILON to this._value before multiplying by 100, ensuring values
affected by floating-point precision round to the expected percentage.
In `@packages/dev-server/src/server.ts`:
- Line 383: Fix the invalid rejection callback on exitedPromise by providing a
valid parameter and logging the received error in the console.error call.
Preserve awaiting exitedPromise while including clear child-process exit failure
context and diagnostic details.
In `@packages/ui/src/Form.ts`:
- Around line 141-142: Remove the detached .catch after submit() in Form, which
is invalid because submit() already awaits Promise.all. Attach rejection
handling directly to the Promise.all call or move it into submit()’s existing
try/catch/finally flow, preserving validator error handling.
---
Nitpick comments:
In `@packages/ui/src/Switch.ts`:
- Line 117: Add regression coverage in Switch.test.ts for the knob-position
calculation used by Switch, targeting the _animProgress rounding thresholds
around the Math.round(... + Number.EPSILON) behavior. Assert that each boundary
value places the knob in the expected track cell while preserving existing
rendering-bound tests.
🪄 Autofix
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: 53d5693a-3e8c-4fe1-a37e-7275fdb442d2
📒 Files selected for processing (4)
examples/pomodoro-timer/src/index.tsxpackages/dev-server/src/server.tspackages/ui/src/Form.tspackages/ui/src/Switch.ts
| const attrs = styleToCellAttrs(this._style); | ||
|
|
||
| const label = this._showLabel ? ` ${Math.round(this._value * 100)}%` : ''; | ||
| const label = this._showLabel ? ` ${Math.round(this._value * 100 + Number.EPSILON)}%` : ''; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Apply Number.EPSILON before scaling the progress value.
Number.EPSILON is added after this._value * 100, where the adjustment can be lost. For example, 0.145 * 100 can evaluate to 14.499999999999998, and the current expression still rounds to 14 instead of 15.
Proposed fix
- const label = this._showLabel ? ` ${Math.round(this._value * 100 + Number.EPSILON)}%` : '';
+ const label = this._showLabel ? ` ${Math.round((this._value + Number.EPSILON) * 100)}%` : '';📝 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.
| const label = this._showLabel ? ` ${Math.round(this._value * 100 + Number.EPSILON)}%` : ''; | |
| const label = this._showLabel ? ` ${Math.round((this._value + Number.EPSILON) * 100)}%` : ''; |
🤖 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 `@examples/pomodoro-timer/src/index.tsx` at line 185, Update the label
calculation in the progress-rendering method to add Number.EPSILON to
this._value before multiplying by 100, ensuring values affected by
floating-point precision round to the expected percentage.
| this._killChild(); | ||
|
|
||
| await exitedPromise.catch(() => {}); | ||
| await exitedPromise.catch( => console.error()); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
npx biome check packages/dev-server/src/server.tsRepository: Karanjot786/TermUI
Length of output: 2385
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Relevant server.ts line context:\n'
sed -n '370,390p' packages/dev-server/src/server.ts | nl -ba -v370
printf '\nParse probe for the exact callback form with Node TypeScript parser if available:\n'
node --version
node --input-type=module - <<'JS'
for (const src of [
'await exitedPromise.catch( => console.error());',
'await Promise.resolve().catch((err) => console.error("Child process exit failed:", err));',
]) {
try {
new Function(src);
console.log('runtime syntax ok:', src);
} catch (error) {
console.log(`${error.name}: ${error.message.split('\n')[0]}`);
console.log(src);
}
}
JS
printf '\nTypeScript syntax check if local typescript is available:\n'
if command -v npx >/dev/null 2>&1; then
npx -y typescript@latest -v
cat > /tmp/check_syntax.ts <<'TS'
const exitedPromise = Promise.reject(new Error("x"));
await exitedPromise.catch( => console.error());
await exitedPromise.catch((err) => {
console.error('Child process exit failed:', err);
});
TS
npx -y typescript@latest --noEmit --strict /tmp/check_syntax.ts || true
else
echo "npx unavailable"
fiRepository: Karanjot786/TermUI
Length of output: 229
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Relevant server.ts line context:\n'
sed -n '370,390p' packages/dev-server/src/server.ts | awk '{printf "%04d\t%s\n", NR+369, $0}'
printf '\nExact current line:\n'
sed -n '383p' packages/dev-server/src/server.ts | awk '{printf "\t%s\n", $0}'
printf '\nNode syntax probe for the callback form:\n'
node --version || true
node --input-type=module - <<'JS'
for (const label of ['bad', 'good']) {
const src = label === 'bad'
? 'await exitedPromise.catch( => console.error());'
: 'await Promise.resolve().catch((err) => console.error("Child process exit failed:", err));';
try {
new Function(src);
console.log(label, 'passes JavaScript syntax check:', src);
} catch (error) {
console.log(label + '-syntax-error');
console.log(error.name + ': ' + error.message.split('\n')[0]);
console.log(src);
}
}
JSRepository: Karanjot786/TermUI
Length of output: 1544
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Server.ts top-level await context:\n'
sed -n '350,390p' packages/dev-server/src/server.ts | awk '{printf "%04d\t%s\n", NR+349, $0}'
printf '\nNode syntax probe wrapped in an async function to isolate callback syntax:\n'
node --input-type=module - <<'JS'
function probe(label, body) {
return { label, body, passes: (() => {
try {
new Function(body);
return true;
} catch (error) {
return false;
}
})() };
}
for (const { label, body } of [
probe('empty-arg-callback', 'return async function() { const exitedPromise = Promise.resolve(0); await exitedPromise.catch( => console.error()); };'),
probe('typed-arg-callback', 'return async function() { const exitedPromise = Promise.resolve(0); await exitedPromise.catch((err) => console.error("Child process exit failed:", err)); };'),
]).results) {
console.log(label + ': ' + (passes ? 'passes' : 'fails JavaScript syntax')) + body;
}
JSRepository: Karanjot786/TermUI
Length of output: 2556
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Node syntax probe for callback parameter syntax in async Promise.catch context:\n'
node --input-type=module - <<'JS'
async function run() {
const entries = [
{ label: bad-argless-callback, body:
'const exitedPromise = Promise.resolve(0);
await exitedPromise.catch( => console.error());'
},
{ label: typed-err-callback, body:
'const exitedPromise = Promise.resolve(0);
await exitedPromise.catch((err) => console.error("Child process exit failed:", err));'
},
];
for (const entry of entries) {
try {
new Function(entry.body);
console.log(entry.label + ": JavaScript syntax ok");
} catch (error) {
console.log(entry.label + ": JavaScript syntax fail");
console.log(error.name + ": " + error.message.split("\n")[0]);
console.log(entry.body);
}
}
}
run().catch(console.error);
JSRepository: Karanjot786/TermUI
Length of output: 1057
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
cat > "$tmpdir/probe.js" <<'JS'
async function run() {
const entries = [
{ label: "argless-arrow-catch", body: "const exitedPromise = Promise.resolve(0);\nawait exitedPromise.catch( => console.error());" },
{ label: "typed-err-arrow-catch", body: "const exitedPromise = Promise.resolve(0);\nawait exitedPromise.catch((err) => console.error(\"Child process exit failed:\", err));" },
];
for (const entry of entries) {
try {
new Function(entry.body);
console.log(entry.label + ": JavaScript syntax ok");
} catch (error) {
console.log(entry.label + ": JavaScript syntax fail");
console.log(error.name + ": " + error.message.split("\n")[0]);
console.log(entry.body);
}
}
}
run().catch(console.error);
JS
node "$tmpdir/probe.js"
printf '\nExact current line byte-level check for shorthand arrow token:\n'
sed -n '383p' packages/dev-server/src/server.ts | od -c | head -n 20Repository: Karanjot786/TermUI
Length of output: 745
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
cat > "$tmpdir/probe.js" <<'JS'
function run() {
const entries = [
{ label: "argless-arrow-catch", body: "const exitedPromise = Promise.resolve(0);\nawait exitedPromise.catch( => console.error());" },
{ label: "typed-err-arrow-catch", body: "const exitedPromise = Promise.resolve(0);\nawait exitedPromise.catch((err) => console.error(\"Child process exit failed:\", err));" },
];
for (const entry of entries) {
try {
new Function(entry.body);
console.log(entry.label + ": JavaScript syntax ok");
} catch (error) {
console.log(entry.label + ": JavaScript syntax fail");
console.log(error.name + ": " + error.message.split("\n")[0]);
console.log(entry.body);
}
}
}
run();
JS
node "$tmpdir/probe.js"
printf '\nLine 383 character stream:\n'
sed -n '383p' packages/dev-server/src/server.ts | tr '\n' '\0' | sed 's/./[&]/g' | tr '?' ' 'Repository: Karanjot786/TermUI
Length of output: 848
Replace the invalid .catch callback.
await exitedPromise.catch( => console.error()) has an invalid arrow-function parameter list. The callback also ignores the rejection, so diagnostics are lost if the child-exit promise rejects.
Use a valid rejection handler:
await exitedPromise.catch((err) => {
console.error('Child process exit failed:', err);
});🧰 Tools
🪛 Biome (2.5.6)
[error] 383-383: Expected a parenthesis '(' but instead found '=>'.
(parse)
🤖 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/dev-server/src/server.ts` at line 383, Fix the invalid rejection
callback on exitedPromise by providing a valid parameter and logging the
received error in the console.error call. Preserve awaiting exitedPromise while
including clear child-process exit failure context and diagnostic details.
Source: Linters/SAST tools
|
|
||
| .catch(err => console.error("Promise.all failed:", err)); No newline at end of file |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
npx biome check packages/ui/src/Form.tsRepository: Karanjot786/TermUI
Length of output: 631
Remove the detached .catch(...) after submit().
packages/ui/src/Form.ts(142,1) is invalid TypeScript: Promise.all is awaited inside submit(), and a standalone .catch(...) after the class is only parsed as a leading statement of the next class item. Attach it to Promise.all if a rejected validator should be caught, or move validation/error handling into try/catch/finally.
🧰 Tools
🪛 Biome (2.5.6)
[error] 142-142: Expected a statement but instead found '.catch(err => console.error("Promise.all failed:", err))'.
(parse)
🤖 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/ui/src/Form.ts` around lines 141 - 142, Remove the detached .catch
after submit() in Form, which is invalid because submit() already awaits
Promise.all. Attach rejection handling directly to the Promise.all call or move
it into submit()’s existing try/catch/finally flow, preserving validator error
handling.
Source: Linters/SAST tools
1 participant
Description
This PR fixes real bugs found in the codebase:
Number.EPSILONtoMath.round: prevents floating-point drift (e.g.1.005 * 100rounding to 100 instead of 101).Number.EPSILONtoMath.round: prevents floating-point drift (e.g.1.005 * 100rounding to 100 instead of 101).Promise.all: an unhandled rejection in any input promise previously crashed silently.Type of Change
How Has This Been Tested?
Checklist
Related Issue
Ref: #3570
Summary by CodeRabbit