fix: resolve 4 bugs in termui - #3705
Conversation
📝 WalkthroughWalkthroughThe pull request applies four isolated fixes: it prevents duplicate streaming timers, improves percentage rounding, bounds hexadecimal entity parsing, and uses numeric ordering when comparing ChangesAI streaming timer cleanup
Pomodoro percentage rounding
RSS hexadecimal entity parsing
TreeSelect numeric comparison
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)
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: 4
🤖 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/ai-streaming/src/index.tsx`:
- Line 43: Replace the window.__interval access in the interval setup around
StreamingText.tick() with a runtime-neutral module-scoped timer handle or safely
checked globalThis property. Use that handle consistently for clearInterval and
reassignment so the Bun TermUI app starts without requiring the browser window
global.
In `@examples/pomodoro-timer/src/index.tsx`:
- Line 185: Update the percentage calculation in the label expression of the
relevant timer component so the epsilon is applied before multiplying the value
by 100 (or equivalently scale the epsilon), ensuring boundary values such as
0.285 round to 29%. Add a regression test covering the 0.285 case.
In `@examples/rss-reader/src/index.tsx`:
- Line 30: Update the hexadecimal entity parsing around codePoint so captured
digits are never silently truncated: either reject entities with more than eight
hexadecimal digits, or constrain the regex to capture at most eight digits
before parsing. Preserve correct conversion for valid entities such as
&`#x000000041`;.
In `@packages/ui/src/TreeSelect.ts`:
- Around line 185-186: Update the array sorting logic around sortedA and sortedB
to use the same explicit numeric comparator for both string arrays, avoiding
arithmetic subtraction that violates strict TypeScript and preventing
lexicographic ordering differences. Preserve the existing comparison behavior
after both arrays are numerically sorted.
🪄 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: 4357a33e-9e3f-4286-a032-c4142dce2c37
📒 Files selected for processing (4)
examples/ai-streaming/src/index.tsxexamples/pomodoro-timer/src/index.tsxexamples/rss-reader/src/index.tsxpackages/ui/src/TreeSelect.ts
| this.addChild(this._streamingText); | ||
|
|
||
| setInterval(() => { | ||
| clearInterval(window.__interval); window.__interval = setInterval(() => { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify the declared runtime and timer/global declarations.
while IFS= read -r file; do
echo "=== $file ==="
sed -n '1,220p' "$file"
done < <(
fd -a -t f '^(package\.json|tsconfig.*\.json|bunfig\.toml|\.nvmrc|\.tool-versions)$' .
)
rg -n -C 3 'window|globalThis|declare global|interface Window|setInterval' examples/ai-streamingRepository: Karanjot786/TermUI
Length of output: 209
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== relevant files ==="
git ls-files | rg '(^examples/ai-streaming/|package\.json$|tsconfig.*\.json$|bunfig\.toml$|^\.nvmrc$|^\.tool-versions$)' || true
echo
echo "=== examples/ai-streaming package and config snippets ==="
fd -a -t f '^(package\.json|tsconfig.*\.json|bunfig\.toml|\.nvmrc|\.tool-versions)$' examples/ai-streaming -x sh -c 'echo "### $(pwd)/$1"; sed -n "1,220p" "$1"' sh {}
echo
echo "=== line 43 context ==="
wc -l examples/ai-streaming/src/index.tsx
sed -n '1,90p' examples/ai-streaming/src/index.tsx
echo
echo "=== window/globalThis/setInterval declarations/usages ==="
rg -n -C 3 'window|globalThis|declare global|interface Window|setInterval|clearInterval' examples/ai-streaming || trueRepository: Karanjot786/TermUI
Length of output: 8346
🌐 Web query:
Bun runtime globalThis window type window object Node.js Bun documentation
💡 Result:
In the Bun runtime, the window object is not defined by default [1][2]. Because Bun is a server-first JavaScript runtime, it does not implement browser-specific DOM APIs such as window [1][3]. Key details regarding globals in Bun: Global Objects: Bun supports globalThis, which serves as the cross-platform standard for accessing the global scope [4][5]. In Bun, globalThis acts as an alias for the global object, maintaining compatibility with the Node.js runtime [4][6]. Design Philosophy: The Bun team has explicitly stated that making window a global by default is avoided to prevent libraries from incorrectly identifying the environment as a browser and triggering inappropriate code paths [7]. Handling window References: If you have code that relies on the existence of a window object (e.g., for server-side rendering or cross-platform compatibility), you can handle it in a few ways: - Feature Detection: Use typeof window !== 'undefined' checks in your code to safely detect the environment [1]. - Runtime Replacement: You can use Bun's --define flag during execution or build time to replace references to window with undefined or another object [8]. For example: bun --define window="undefined" src/index.ts [8] - External Libraries: If you require DOM-like functionality on the server, you may use external libraries (such as JSDOM or LinkeDOM) rather than expecting native support for window [7].
Citations:
- 1: Can't find variable: window oven-sh/bun#4837
- 2: bun's not defined window behaviour different to node's behaviour oven-sh/bun#7769
- 3: https://bun.sh/docs/runtime/web-apis
- 4: https://bun.com/docs/runtime/globals
- 5: https://bun.sh/docs/runtime/globals
- 6: https://bun.com/docs/runtime/nodejs-compat
- 7: Support DOM APIs on the Server oven-sh/bun#8621
- 8: https://bun.com/docs/guides/runtime/define-constant
Use a runtime-neutral timer handle.
This example is a Bun TermUI app (bun src/index.tsx), and Bun does not provide the browser window global. The clearInterval(...) / assignment on line 43 throws before StreamingText.tick() starts. Keep the interval handle in module scope or use a checked globalThis property.
🤖 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/ai-streaming/src/index.tsx` at line 43, Replace the
window.__interval access in the interval setup around StreamingText.tick() with
a runtime-neutral module-scoped timer handle or safely checked globalThis
property. Use that handle consistently for clearInterval and reassignment so the
Bun TermUI app starts without requiring the browser window global.
| 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
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
fd -a 'index.tsx' . | sed 's#^\./##' | rg 'examples/pomodoro-timer/src/index.tsx|pomodoro-timer/src/index.tsx' || true
echo "== relevant lines =="
if [ -f examples/pomodoro-timer/src/index.tsx ]; then
nl -ba examples/pomodoro-timer/src/index.tsx | sed -n '160,210p'
fi
echo "== javascript rounding behavior probe =="
node - <<'JS'
const cases = [0.285, 0.9995, 0.195, 0.125, 0.5];
for (const value of cases) {
const scaled = value * 100;
const current = Math.round(scaled + Number.EPSILON);
const proposed = Math.round((value + Number.EPSILON) * 100);
const scaledEpsilon = Math.round(scaled + Number.EPSILON * 100);
console.log(JSON.stringify({
value,
scaled,
current,
proposed,
scaledEpsilon
}));
}
JSRepository: Karanjot786/TermUI
Length of output: 292
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant lines =="
awk '{ printf "%7d %s\n", NR, $0 }' examples/pomodoro-timer/src/index.tsx | sed -n '160,210p'
echo "== javascript rounding behavior probe =="
node - <<'JS'
const cases = [0.285, 0.9995, 0.195, 0.125, 0.5];
for (const value of cases) {
const scaled = value * 100;
const current = Math.round(scaled + Number.EPSILON);
const proposed = Math.round((value + Number.EPSILON) * 100);
const scaledEpsilon = Math.round(scaled + Number.EPSILON * 100);
console.log(JSON.stringify({
value,
scaled,
current,
proposed,
scaledEpsilon
}));
}
JSRepository: Karanjot786/TermUI
Length of output: 3135
Scale the epsilon before rounding.
Line 185 adds Number.EPSILON after scaling by 100. This leaves boundary values like 0.285 rounding to 28% instead of 29%.
Add the epsilon before scaling, or scale the epsilon, and add a regression test for 0.285.
Suggested 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 percentage
calculation in the label expression of the relevant timer component so the
epsilon is applied before multiplying the value by 100 (or equivalently scale
the epsilon), ensuring boundary values such as 0.285 round to 29%. Add a
regression test covering the 0.285 case.
| return value.replace(/&(#x?[0-9a-fA-F]+|[a-zA-Z]+);/g, (match, entity: string) => { | ||
| if (entity.startsWith('#x')) { | ||
| const codePoint = Number.parseInt(entity.slice(2), 16); | ||
| const codePoint = Number.parseInt(entity.slice(2, 10), 16); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not truncate hexadecimal entities before parsing.
The regular expression still captures all hexadecimal digits, but entity.slice(2, 10) discards digits after the first eight. For example, A is converted to code point 0x04 instead of 0x41.
Reject entities longer than eight digits, or update the regular expression to capture at most eight digits.
Proposed fix
- const codePoint = Number.parseInt(entity.slice(2, 10), 16);
+ const hexDigits = entity.slice(2);
+ if (hexDigits.length > 8) return match;
+ const codePoint = Number.parseInt(hexDigits, 16);📝 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 codePoint = Number.parseInt(entity.slice(2, 10), 16); | |
| const hexDigits = entity.slice(2); | |
| if (hexDigits.length > 8) return match; | |
| const codePoint = Number.parseInt(hexDigits, 16); |
🤖 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/rss-reader/src/index.tsx` at line 30, Update the hexadecimal entity
parsing around codePoint so captured digits are never silently truncated: either
reject entities with more than eight hexadecimal digits, or constrain the regex
to capture at most eight digits before parsing. Preserve correct conversion for
valid entities such as &`#x000000041`;.
| const sortedA = [...a].sort((a, b) => a - b); | ||
| const sortedB = [...b].sort(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="packages/ui/src/TreeSelect.ts"
sed -n '183,190p' "$file"
if rg -n 'sort\(\(a, b\) => a - b\)' "$file"; then
echo "Direct subtraction of string operands remains."
exit 1
fi
echo "Verify that both arrays use the same explicit numeric comparator."Repository: Karanjot786/TermUI
Length of output: 569
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="packages/ui/src/TreeSelect.ts"
tsconfig="tsconfig.json"
# Locate and print the method around _valuesEqual; keep small context.
sed -n '160,200p' "$file"
echo '--- TypeScript numeric comparison semantics probe ---'
node - <<'JS'
const a = ["10", "2"].sort((x, y) => x - y);
const b = ["10", "2"].sort();
console.log(JSON.stringify({ a, b }));
JSRepository: Karanjot786/TermUI
Length of output: 1356
Use one type-safe numeric comparator for both arrays.
a and b are string values, so a - b is not allowed by strict TypeScript. sortedB also uses lexicographic sorting, so ["10", "2"] can sort differently and compare unequal. Use an explicit numeric comparator for both sorts.
🤖 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/TreeSelect.ts` around lines 185 - 186, Update the array
sorting logic around sortedA and sortedB to use the same explicit numeric
comparator for both string arrays, avoiding arithmetic subtraction that violates
strict TypeScript and preventing lexicographic ordering differences. Preserve
the existing comparison behavior after both arrays are numerically sorted.
Source: Coding guidelines
1 participant
Description
This PR fixes real bugs found in the codebase:
parseInt: without10, strings like'0x1F'or'08'parse in unintended bases..sort()coerces elements to strings, so[10, 9, 2]sorts as[10, 2, 9]; numeric comparator sorts correctly.Number.EPSILONtoMath.round: prevents floating-point drift (e.g.1.005 * 100rounding to 100 instead of 101).Type of Change
How Has This Been Tested?
Checklist
Related Issue
Ref: #3704
Summary by CodeRabbit