fix: resolve 4 bugs in termui - #3496
Conversation
github-actions
Bot
added
area:examples
📝 WalkthroughWalkthroughThe changes update progress percentage rounding, add Form promise rejection logging, and replace lexicographic sorting with comparator-based sorting in MultiSelect and registry dependency collection. ChangesProgress percentage rounding
Form rejection logging
MultiSelect ordering
Registry dependency ordering
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.5)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
🤖 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/todo-app/src/index.ts`:
- Line 107: Update the percentage calculation at pct to add Number.EPSILON to
value before multiplying by 100, then round the scaled result. Add a regression
test covering value 0.575 and assert it produces 58.
In `@packages/ui/src/Form.ts`:
- Line 142: Fix the validation flow in the Form submit logic so the rejection
handler is syntactically attached to the Promise.all expression or handled with
try/catch. Move _isValidating = false and markDirty() into a finally block so
cleanup runs for both resolved and rejected validators, while keeping submit
callbacks only on successful validation; add a Form.test.ts case covering a
rejected validator.
In `@scripts/build-registry.ts`:
- Line 47: Update the sorting logic in collectDeps to compare package specifiers
lexicographically as strings instead of subtracting them numerically. Preserve
the existing deduplication and return behavior while ensuring the ordering
matches the registry tests when the bun script runs.
🪄 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: 1604854d-a84d-43e4-a467-e8146b38e445
📒 Files selected for processing (4)
examples/todo-app/src/index.tspackages/ui/src/Form.tspackages/ui/src/MultiSelect.tsscripts/build-registry.ts
| const filled = Math.round(barWidth * value); | ||
|
|
||
| const pct = Math.round(value * 100); | ||
| const pct = Math.round(value * 100 + Number.EPSILON); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
node <<'EOF'
const value = 0.575;
const current = Math.round(value * 100 + Number.EPSILON);
const proposed = Math.round((value + Number.EPSILON) * 100);
console.log({ current, proposed });
if (current !== 57 || proposed !== 58) process.exit(1);
EOFRepository: Karanjot786/TermUI
Length of output: 186
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg '(^|/)examples/todo-app/src/index\.ts$' || true
echo "== relevant context =="
if [ -f examples/todo-app/src/index.ts ]; then
nl -ba examples/todo-app/src/index.ts | sed -n '80,120p'
fi
echo "== runtime boundary probe =="
node - <<'JS'
const values = [0.575, 0.015, 0.025, 0.035, 0.045, 0.055, 0.065, 0.075];
const expected = [58, 2, 2, 4, 4, 6, 6, 8];
const currentErrors = [];
const proposedErrors = [];
for (let i=0;i<values.length;i++) {
const value = values[i];
const current = Math.round(value * 100 + Number.EPSILON);
const proposed = Math.round((value + Number.EPSILON) * 100);
if (current !== expected[i]) currentErrors.push(`${value}: ${current}`);
if (proposed !== expected[i]) proposedErrors.push(`${value}: ${proposed}`);
console.log(`${value}: current=${current} proposed=${proposed} expected=${expected[i]}`);
}
console.log(`current_errors=${currentErrors.length} proposed_errors=${proposedErrors.length}`);
JSRepository: Karanjot786/TermUI
Length of output: 268
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant context =="
awk '{printf "%6d\t%s\n", NR, $0}' examples/todo-app/src/index.ts | sed -n '80,120p'
echo "== runtime boundary probe =="
node - <<'JS'
const values = [0.575, 0.015, 0.025, 0.035, 0.045, 0.055, 0.065, 0.075];
const expected = [58, 2, 2, 4, 4, 6, 6, 8];
const currentErrors = [];
const proposedErrors = [];
for (let i=0;i<values.length;i++) {
const value = values[i];
const current = Math.round(value * 100 + Number.EPSILON);
const proposed = Math.round((value + Number.EPSILON) * 100);
if (current !== expected[i]) currentErrors.push(`${value}: ${current}`);
if (proposed !== expected[i]) proposedErrors.push(`${value}: ${proposed}`);
console.log(`${value}: current=${current} proposed=${proposed} expected=${expected[i]}`);
}
console.log(`current_errors=${currentErrors.length} proposed_errors=${proposedErrors.length}`);
JSRepository: Karanjot786/TermUI
Length of output: 2624
Scale the epsilon before rounding.
Adding Number.EPSILON after value * 100 does not correct some floating-point rounding boundaries. For example, 0.575 * 100 can be 57.49999999999999, which still rounds to 57 instead of 58.
Add the epsilon before scaling and add a regression test for this boundary value.
Proposed fix
- const pct = Math.round(value * 100 + Number.EPSILON);
+ const pct = Math.round((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/todo-app/src/index.ts` at line 107, Update the percentage
calculation at pct to add Number.EPSILON to value before multiplying by 100,
then round the scaled result. Add a regression test covering value 0.575 and
assert it produces 58.
| } | ||
| } | ||
|
|
||
| .catch(err => console.error("Promise.all failed:", err)); No newline at end of file |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Attach the rejection handler correctly and preserve submit cleanup.
Line 142 is a standalone .catch(...) expression, so TypeScript cannot parse this file. Attach the handler to the Promise.all(...) expression or wrap the await in try/catch.
When validation rejects, execution also bypasses Lines 92 and 97. Move _isValidating = false and markDirty() into finally; keep submit callbacks on the successful validation path. Add a test in packages/ui/src/Form.test.ts for a rejected validator.
🧰 Tools
🪛 Biome (2.5.5)
[error] 142-142: Expected a statement but instead found '.catch(err => console.error("Promise.all failed:", err))'.
(parse)
🪛 GitHub Actions: CI / 0_build-and-test.txt
[error] 142-142: The @termuijs/ui build (tsup/esbuild) failed due to an unexpected '.' at the start of '.catch(err => console.error("Promise.all failed:", err));'. TypeScript also reported declaration and syntax errors on this line.
🪛 GitHub Actions: CI / build-and-test
[error] 142-142: The @termuijs/ui build command (tsup) failed because of an unexpected '.' in .catch(err => console.error("Promise.all failed:", err));. TypeScript also reported syntax errors including missing try, ), and ;.
🤖 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` at line 142, Fix the validation flow in the Form
submit logic so the rejection handler is syntactically attached to the
Promise.all expression or handled with try/catch. Move _isValidating = false and
markDirty() into a finally block so cleanup runs for both resolved and rejected
validators, while keeping submit callbacks only on successful validation; add a
Form.test.ts case covering a rejected validator.
Source: Linters/SAST tools
| let m: RegExpExecArray | null; | ||
| while ((m = re.exec(content)) !== null) deps.add(m[1]!); | ||
| return [...deps].sort(); | ||
| return [...deps].sort((a, b) => a - b); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
npx tsc --noEmit --pretty false
npx vitest run scripts/build-registry.test.tsRepository: Karanjot786/TermUI
Length of output: 5708
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '== files ==\n'
git ls-files | rg '(^|/)(build-registry|build-registry\.test)\.(ts|tsx)$|tsconfig.*\.json$|package\.json$' || true
printf '\n== build-registry outline ==\n'
ast-grep outline scripts/build-registry.ts --view expanded || true
printf '\n== relevant build-registry lines ==\n'
sed -n '1,100p' scripts/build-registry.ts
printf '\n== relevant tests around dependency order ==\n'
sed -n '100,160p' scripts/build-registry.test.ts
printf '\n== tsconfig files ==\n'
for f in $(git ls-files 'tsconfig*.json'); do
echo "--- $f"
cat -n "$f"
done
printf '\n== runtime comparator behavior probe ==\n'
node - <<'JS'
const deps = ['foo', 'bar', 'baz'];
console.log([...deps].sort((a, b) => a - b));
console.log('comparator returns NaN:', Number.isNaN((a, b) => a - b) ? true : ((a,b)=>a-b)('foo','bar'));
JSRepository: Karanjot786/TermUI
Length of output: 13309
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '== package scripts/typecheck fields ==\n'
node - <<'JS'
const { readFileSync } = require('node:fs');
const pkg = JSON.parse(readFileSync('package.json', 'utf-8'));
console.log(JSON.stringify({ scripts: pkg.scripts, dependencies: pkg.dependencies }, null, 2));
JS
printf '\n== lockfile snippets around bun/tsc packages if present ==\n'
git ls-files | rg '(^|/)(bun\.lockb|bun\.lock|pnpm-lock\.yaml|package-lock\.json|yarn\.lock)$' || trueRepository: Karanjot786/TermUI
Length of output: 816
Use a string comparator for package specifiers.
collectDeps handles string specifiers, so a - b makes the compiled script compare by NaN instead of lexicographically when the bun script is run. Use a string comparator to match the registry collectDeps test expectations.
🤖 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/build-registry.ts` at line 47, Update the sorting logic in
collectDeps to compare package specifiers lexicographically as strings instead
of subtracting them numerically. Preserve the existing deduplication and return
behavior while ensuring the ordering matches the registry tests when the bun
script runs.
Source: Coding guidelines
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).Promise.all: an unhandled rejection in any input promise previously crashed silently..sort()coerces elements to strings, so[10, 9, 2]sorts as[10, 2, 9]; numeric comparator sorts correctly..sort()coerces elements to strings, so[10, 9, 2]sorts as[10, 2, 9]; numeric comparator sorts correctly.Type of Change
How Has This Been Tested?
Checklist
Related Issue
Ref: #3495
Summary by CodeRabbit