fix: resolve 4 bugs in termui - #3453
Closed
saurabhhhcodes wants to merge 1 commit into
Closed
Conversation
github-actions
Bot
added
area:dev-server
📝 WalkthroughWalkthroughThe PR adjusts FPS rounding in ChangesFPS rounding
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🧪 Generate unit tests (beta)
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: 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 `@packages/dev-server/src/devtools.ts`:
- Line 81: Update the FPS rounding expression in the devtools FPS calculation to
apply the magnitude-aware epsilon correction to the scaled value being rounded,
preserving one-decimal output. Add a regression test in the devtools test suite
covering a half-tenth FPS boundary and asserting the corrected rounded result.
In `@scripts/build-registry.ts`:
- Line 47: Update collectDeps to sort dependency names as strings using the
default lexicographic sort or localeCompare instead of numeric subtraction. Add
a reverse-order dependency fixture in build-registry tests so the sorting
behavior is verified with unsorted input.
🪄 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: b31f8796-ee39-4176-afab-ee6e079f744a
📒 Files selected for processing (2)
packages/dev-server/src/devtools.tsscripts/build-registry.ts
| widgetCount, | ||
| lastRenderAt: now, | ||
| fps: Math.round(fps * 10) / 10, | ||
| fps: Math.round(fps * 10 + Number.EPSILON) / 10, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate devtools files =="
fd -a 'devtools\.(ts|tsx)$' . | sed 's#^\./##'
echo
echo "== outline devtools.ts =="
ast-grep outline packages/dev-server/src/devtools.ts --view expanded || true
echo
echo "== relevant source around FPS calculation =="
sed -n '1,140p' packages/dev-server/src/devtools.ts | cat -n
echo
echo "== test file if present =="
if [ -f packages/dev-server/src/devtools.test.ts ]; then
sed -n '1,220p' packages/dev-server/src/devtools.test.ts | cat -n
else
echo "packages/dev-server/src/devtools.test.ts not found"
fi
echo
echo "== precise JS numeric behavior probe for Number.EPSILON and half-tenth boundaries =="
node - <<'JS'
const values = [
1.4499990000000002,
1.4500000000000002,
1.4500000000000003,
1.4500000000000005,
1.4500000000000008,
1.450000000000001,
1.450000000000002,
30.450000000000003,
30.450000000000007,
30.550000000000004,
];
for (const fps of values) {
const x = fps * 10;
const y = fps * 10 + Number.EPSILON;
console.log(JSON.stringify({
fps,
eps: Number.EPSILON,
x,
xHex: x.toString(16),
y,
yHex: y.toString(16),
sameRep: x === y,
unchangedRound: Math.round(x) === Math.round(y),
correctedRound: Math.round(fps * 10 + Number.EPSILON) / 10,
}));
}
JSRepository: Karanjot786/TermUI
Length of output: 21936
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== inspect current diff/stat =="
git diff --stat -- packages/dev-server/src/devtools.ts packages/dev-server/src/devtools.test.ts || true
git diff -- packages/dev-server/src/devtools.ts packages/dev-server/src/devtools.test.ts | sed -n '1,220p' || true
echo
echo "== static occurrences of Number.EPSILON in devtools files =="
rg -n "Number\.EPSILON|Math\.round\(fps \* 10" packages/dev-server/src/devtools.ts packages/dev-server/src/devtools.test.ts || true
echo
echo "== search fps rounding tests in packages/dev-server =="
rg -n -i "frames per second|fps|frame rate|perftime|recordRender" packages/dev-server packages -g '*.ts' -g '*.tsx' | head -n 200 || trueRepository: Karanjot786/TermUI
Length of output: 11416
Use a magnitude-aware rounding correction.
Number.EPSILON applies to the result of fps / 32, while Math.round(fps * 10 + Number.EPSILON) / 10 depends on fps * 10. For normal FPS values, adding Number.EPSILON after scaling can change nothing, so a half-tenth FPS boundary can still round incorrectly. Apply the epsilon to fps * 10 or round from the unscaled value, and add a regression test for the half-tenth case in packages/dev-server/src/devtools.test.ts.
🤖 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/devtools.ts` at line 81, Update the FPS rounding
expression in the devtools FPS calculation to apply the magnitude-aware epsilon
correction to the scaled value being rounded, preserving one-decimal output. Add
a regression test in the devtools test suite covering a half-tenth FPS boundary
and asserting the corrected rounded result.
| 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
rg -n -C 3 'collectDeps|sort\(' scripts/build-registry.ts scripts/build-registry.test.ts
rg -n --glob 'tsconfig*.json' '"strict"[[:space:]]*:[[:space:]]*true'Repository: Karanjot786/TermUI
Length of output: 2917
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '--- build-registry.ts snippet ---\n'
sed -n '36,50p' scripts/build-registry.ts
printf '\n--- package scripts/config references ---\n'
node - <<'JS'
const fs = require('node:fs');
const pkg = JSON.parse(fs.readFileSync('package.json', 'utf8'));
console.log(JSON.stringify({
scripts: pkg.scripts,
devDependencies: Object.fromEntries(Object.entries(pkg.devDependencies || {}).filter(([k]) => k.includes('type') || k.includes('vitest'))),
dependencies: Object.fromEntries(Object.entries(pkg.dependencies || {}).filter(([k]) => k.includes('type') || k.includes('vitest'))),
}, null, 2));
JS
printf '\n--- package manager config files ---\n'
git ls-files | grep -E '(^|/)(package.json|pnpm-workspace.yaml|yarn.lock|package-lock.json|bun.lockb|bun.lock|tsconfig.*\.json)$' || true
printf '\n--- TypeScript strict settings evidence ---\n'
git ls-files 'tsconfig*.json' | while read -r f; do
echo "### $f"
rg -n '"(strict|noImplicitAny|noImplicitReturns)"' "$f" || true
done
printf '\n--- node behavior probe ---\n'
node - <<'JS'
const inputs = ['`@termuijs/widgets`', '`@termuijs/core`', '`@termuijs/motion`'];
console.log('localeCompare sorted:', [...inputs].sort((a, b) => a.localeCompare(b)));
console.log('subtraction order:', [...inputs].sort((a, b) => a - b).join(','));
console.log('sample comparator result:', '`@termuijs/widgets`'.localeCompare('`@termuijs/core`'), '`@termuijs/widgets`' - '`@termuijs/core`');
JSRepository: Karanjot786/TermUI
Length of output: 247
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- build-registry.ts snippet ---'
sed -n '36,50p' scripts/build-registry.ts
printf '%s\n' '--- package scripts/config references ---'
node - <<'JS'
const fs = require('node:fs');
const pkg = JSON.parse(fs.readFileSync('package.json', 'utf8'));
console.log(JSON.stringify({
scripts: pkg.scripts,
devDependencies: Object.fromEntries(Object.entries(pkg.devDependencies || {}).filter(([k]) => k.includes('type') || k.includes('vitest'))),
dependencies: Object.fromEntries(Object.entries(pkg.dependencies || {}).filter(([k]) => k.includes('type') || k.includes('vitest'))),
}, null, 2));
JS
printf '%s\n' '--- tracked config/package files ---'
git ls-files | grep -E '(^|/)(package.json|pnpm-workspace.yaml|yarn.lock|package-lock.json|bun.lockb|bun.lock|tsconfig.*\.json)$' || true
printf '%s\n' '--- TypeScript strict settings evidence ---'
while IFS= read -r f; do
printf '%s\n' "### $f"
rg -n '"(strict|noImplicitAny|noImplicitReturns)"' "$f" || true
done < <(git ls-files 'tsconfig*.json')
printf '%s\n' '--- node behavior probe ---'
node - <<'JS'
const inputs = ['`@termuijs/widgets`', '`@termuijs/core`', '`@termuijs/motion`'];
const localeSorted = [...inputs].sort((a, b) => a.localeCompare(b));
const subtractionOrder = [...inputs].sort((a, b) => a - b).join(',');
console.log('localeCompare sorted:', localeSorted);
console.log('subtraction order:', subtractionOrder);
console.log('sample localeCompare:', '`@termuijs/widgets`'.localeCompare('`@termuijs/core`'));
console.log('sample subtraction:', Number('`@termuijs/widgets`'), Number('`@termuijs/core`'), Number('`@termuijs/widgets`') - Number('`@termuijs/core`'));
JSRepository: Karanjot786/TermUI
Length of output: 5257
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- root tsconfig strict section ---'
node - <<'JS'
const fs = require('node:fs');
for (const [file, selector] of [['tsconfig.base.json', 'strict'], ['vite.config.ts', 'test/typescript'], ['vitest.config.ts', 'test/typescript']]) {
try {
const raw = fs.readFileSync(file, 'utf8');
console.log(`### ${file}`);
if (selector === 'strict') {
parsed = JSON.parse(raw);
console.log(JSON.stringify(parsed.compilerOptions || parsed.extends || parsed.includes || parsed.exclude || parsed.exclude, null, 2));
} else {
console.log(raw);
}
} catch (e) {
console.log(e && e.message || String(e));
}
}
JS
printf '%s\n' '--- node behavior probe ---'
node - <<'JS'
const inputs = ['`@termuijs/widgets`', '`@termuijs/core`', '`@termuijs/motion`'];
console.log(JSON.stringify({
localeCompareSorted: [...inputs].sort((a, b) => a.localeCompare(b)),
numericSubtractionOrder: [...inputs].sort((a, b) => a - b),
sampleLocale: '`@termuijs/widgets`'.localeCompare('`@termuijs/core`'),
sampleNumber: Number('`@termuijs/widgets`'),
sampleNan: '`@termuijs/widgets`' - '`@termuijs/core`'
}, null, 2));
JSRepository: Karanjot786/TermUI
Length of output: 1480
Restore string sorting for dependency names.
collectDeps returns strings from Set<string>, so a - b is invalid under TypeScript strict mode. NaN comparators also keep insertion order at runtime, so the registry dependencies are not sorted. Use .sort() or a.localeCompare(b), and add a reverse-order collectDeps fixture in scripts/build-registry.test.ts since the current fixture is already sorted.
🤖 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 collectDeps to sort dependency
names as strings using the default lexicographic sort or localeCompare instead
of numeric subtraction. Add a reverse-order dependency fixture in build-registry
tests so the sorting behavior is verified with unsorted input.
Source: Coding guidelines
4 tasks
4 tasks
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 PR fixes real bugs found in the codebase:
trim()to''misses whitespace-only input;.trim().length === 0is explicit.Number.EPSILONtoMath.round: prevents floating-point drift (e.g.1.005 * 100rounding to 100 instead of 101)..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: #3452
Summary by CodeRabbit