Skip to content

fix: address 5 upstream issues (batch rollback, throttle, CheckboxGroup, Tree, Color) #3239

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
tmdeveloper007 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: address 5 upstream issues (batch rollback, throttle, CheckboxGroup, Tree, Color) #3239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion packages/core/src/style/Color.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,9 @@ function rgbToAnsi256(r: number, g: number, b: number): number {
if (grayIdx > 23) grayIdx = 23;
const grayColor = 232 + grayIdx;
r_c = ANSI256_RGB[grayColor * 3];
const distGray = (r - r_c)**2 + (g - r_c)**2 + (b - r_c)**2;
g_c = ANSI256_RGB[grayColor * 3 + 1];
b_c = ANSI256_RGB[grayColor * 3 + 2];
const distGray = (r - r_c)**2 + (g - g_c)**2 + (b - b_c)**2;
if (distGray < bestDist) {
bestDist = distGray;
bestColor = grayColor;
Expand Down
5 changes: 4 additions & 1 deletion packages/core/src/utils/throttle.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,17 @@ export function throttle<T extends (...args: unknown[]) => void>(
const leading = options?.leading ?? true;
let timer: ReturnType<typeof setTimeout> | undefined;
let lastArgs: Parameters<T> | undefined;
let hasNewCalls = false;

const throttled = function (...args: Parameters<T>) {
lastArgs = args;
if (timer) hasNewCalls = true;
if (!timer) {
if (leading) func(...args);
timer = setTimeout(() => {
timer = undefined;
if (lastArgs !== args || !leading) func(...lastArgs!);
if (hasNewCalls || !leading) func(...lastArgs!);
hasNewCalls = false;
Comment on lines +30 to +40

@coderabbitai coderabbitai Bot Jul 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Reset hasNewCalls when cancelling.

throttled.cancel() clears the timer and arguments at Lines 46-50 but leaves this new flag set. After a cancelled window that received a second call, the next independent leading call can incorrectly trigger a trailing invocation. Reset hasNewCalls = false in cancel() and cover this sequence with a regression test.

Proposed fix 
 throttled.cancel = () => {
     clearTimeout(timer);
     timer = undefined;
     lastArgs = undefined;
+    hasNewCalls = false;
 };
📝 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
let hasNewCalls = false;
const throttled = function (...args: Parameters<T>) {
lastArgs = args;
if (timer) hasNewCalls = true;
if (!timer) {
if (leading) func(...args);
timer = setTimeout(() => {
timer = undefined;
if (lastArgs !== args || !leading) func(...lastArgs!);
if (hasNewCalls || !leading) func(...lastArgs!);
hasNewCalls = false;
throttled.cancel = () => {
clearTimeout(timer);
timer = undefined;
lastArgs = undefined;
hasNewCalls = false;
};
🤖 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/core/src/utils/throttle.ts` around lines 30 - 40, Reset the
hasNewCalls flag inside throttled.cancel() alongside the timer and argument
cleanup, so cancellation fully clears the throttle state. Add a regression test
covering a second call followed by cancel and then a new leading call, ensuring
no stale trailing invocation occurs.

lastArgs = undefined;
}, wait);
}
Expand Down
4 changes: 1 addition & 3 deletions packages/store/src/store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,7 @@ export function batch<T>(fn: () => T): T {
} catch (err) {
threw = true;
_batchDepth--;
if (_batchDepth === 0) {
flushBatch(threw);
}
flushBatch(threw);
throw err;
}

Expand Down
1 change: 1 addition & 0 deletions packages/ui/src/CheckboxGroup.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ export class CheckboxGroup extends Widget {
(options.defaultValues ?? []).filter(v => knownValues.has(v)),
);
this.onChange = options.onChange;
this.events.on('key', this.handleKey.bind(this));
}

get selectedValues(): string[] {
Expand Down
17 changes: 17 additions & 0 deletions packages/ui/src/Tree.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,22 @@ export class Tree extends Widget {
this._roots = roots;
this._activeColor = options.activeColor ?? { type: 'named', name: 'cyan' };
this._onSelect = options.onSelect;
this.events.on('key', this.handleKey.bind(this));
}

private handleKey(event: { key: string }): void {
switch (event.key) {
case 'up':
this.selectPrev();
break;
case 'down':
this.selectNext();
break;
case 'enter':
case ' ':
Comment on lines +23 to +32

@coderabbitai coderabbitai Bot Jul 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
rg -n -C2 "KeyEvent|case 'space'|emit\\('key'" packages/core/src packages/ui/src

Repository: Karanjot786/TermUI

Length of output: 50375

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- packages/ui/src/Tree.ts outline ---'
ast-grep outline packages/ui/src/Tree.ts || true

printf '%s\n' '--- packages/ui/src/Tree.ts lines 1-220 ---'
cat -n packages/ui/src/Tree.ts

printf '%s\n' '--- relevant imports in ui package files ---'
for f in packages/ui/src/Tree.ts packages/ui/src/Tree.test.ts; do
  if [ -f "$f" ]; then
    echo "### $f"
    sed -n '1,80p' "$f"
  else
    echo "missing: $f"
  fi
done

printf '%s\n' '--- KeyEvent key maps for space / printable chars ---'
cat -n packages/core/src/input/KeyMap.ts | sed -n '1,130p'

Repository: Karanjot786/TermUI

Length of output: 14550

Use KeyEvent and the canonical 'space' key.

Tree is already subscribing to the core 'key' event, whose contract is KeyEvent, and the input parser emits Space as 'space', not ' '. Keep the component contract aligned and normalize the handler case.

Proposed fix 
-    private handleKey(event: { key: string }): void {
+    private handleKey(event: KeyEvent): void {
...
-            case ' ':
+            case 'space':
📝 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
private handleKey(event: { key: string }): void {
switch (event.key) {
case 'up':
this.selectPrev();
break;
case 'down':
this.selectNext();
break;
case 'enter':
case ' ':
private handleKey(event: KeyEvent): void {
switch (event.key) {
case 'up':
this.selectPrev();
break;
case 'down':
this.selectNext();
break;
case 'enter':
case 'space':
🤖 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/Tree.ts` around lines 23 - 32, The handleKey method should
accept the core KeyEvent type instead of an inline object, and its switch should
use the canonical 'space' key value rather than ' '. Update the import and
handler case while preserving the existing enter/space behavior.

Source: Coding guidelines

this.confirm();
break;
}
}

private _flatten(): { node: TreeNode; depth: number; path: number[]; hasChildren: boolean }[] {
Expand All @@ -36,6 +52,7 @@ export class Tree extends Widget {
selectNext(): void { const f = this._flatten(); if (this._cursorIndex < f.length - 1) { this._cursorIndex++; this.markDirty(); } }
selectPrev(): void { if (this._cursorIndex > 0) { this._cursorIndex--; this.markDirty(); } }
toggleExpand(): void { const f = this._flatten(); const it = f[this._cursorIndex]; if (it?.hasChildren) { it.node.expanded = !it.node.expanded; this.markDirty(); } }
toggleExpandKey(): void { this.toggleExpand(); }
confirm(): void {
const f = this._flatten(); const it = f[this._cursorIndex];
if (it) { it.hasChildren ? this.toggleExpand() : this._onSelect?.(it.node, it.path); }
Expand Down
Loading