fix: address 5 upstream issues (batch rollback, throttle, CheckboxGroup, Tree, Color) - #3239
fix: address 5 upstream issues (batch rollback, throttle, CheckboxGroup, Tree, Color)#3239tmdeveloper007 wants to merge 1 commit into
Conversation
…eckboxGroup, Tree, Color) - fix(store): always call flushBatch(threw) on error regardless of batch depth (Karanjot786#3233) - fix(core): throttle trailing call fires only when new calls arrived during wait (Karanjot786#3229) - Added hasNewCalls flag to track if subsequent calls came in after leading call - Trailing fires when hasNewCalls || !leading - fix(ui): CheckboxGroup: subscribe handleKey to events emitter (Karanjot786#3232) - fix(ui): Tree: subscribe handleKey to events emitter + add handleKey method (Karanjot786#3231) - fix(core): Color.ts: use g_c/b_c for green/blue channels in grayscale distance (Karanjot786#3230) Refs: upstream Karanjot786#3230, Karanjot786#3229, Karanjot786#3232, Karanjot786#3231, Karanjot786#3233
github-actions
Bot
added
type:bug
📝 WalkthroughWalkthroughThis PR corrects grayscale ANSI color matching, tracks trailing throttle calls explicitly, changes nested batch rollback handling, and adds keyboard event wiring and key actions for CheckboxGroup and Tree widgets. ChangesCore utility corrections
Batch rollback behavior
Widget keyboard handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels: 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
🧹 Nitpick comments (1)
packages/store/src/store.ts (1)
76-76: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd regression coverage for caught nested batch errors.
At Line 76,
flushBatch(true)now runs while an outer batch may still be active. Because_batchStoresis shared across nesting levels, this can roll back outer updates when an inner error is caught. Existing tests cover outer rejection, but not this changed path; add a test that defines the expected state and subscriber behavior for a caught inner error.🤖 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/store/src/store.ts` at line 76, Add regression coverage around flushBatch in the store batching tests for a caught error inside a nested batch. Define an outer batch that applies updates, trigger and catch an inner batch error, then verify the expected final store state and subscriber notifications, ensuring outer updates are preserved and the inner failure does not incorrectly roll them back.
🤖 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/core/src/utils/throttle.ts`:
- Around line 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.
In `@packages/ui/src/Tree.ts`:
- Around line 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.
---
Nitpick comments:
In `@packages/store/src/store.ts`:
- Line 76: Add regression coverage around flushBatch in the store batching tests
for a caught error inside a nested batch. Define an outer batch that applies
updates, trigger and catch an inner batch error, then verify the expected final
store state and subscriber notifications, ensuring outer updates are preserved
and the inner failure does not incorrectly roll them back.
🪄 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: ce7ba88d-d0e2-4e3c-893b-58e28373bcdd
📒 Files selected for processing (5)
packages/core/src/style/Color.tspackages/core/src/utils/throttle.tspackages/store/src/store.tspackages/ui/src/CheckboxGroup.tspackages/ui/src/Tree.ts
| 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; |
There was a problem hiding this comment.
🎯 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.
| 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.
| private handleKey(event: { key: string }): void { | ||
| switch (event.key) { | ||
| case 'up': | ||
| this.selectPrev(); | ||
| break; | ||
| case 'down': | ||
| this.selectNext(); | ||
| break; | ||
| case 'enter': | ||
| case ' ': |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 "KeyEvent|case 'space'|emit\\('key'" packages/core/src packages/ui/srcRepository: 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.
| 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
1 participant
Summary
Addresses 5 pending upstream issues with targeted bug fixes.
Changes
packages/store/src/store.tsflushBatch(threw)on error regardless of batch nesting depthpackages/core/src/utils/throttle.tshasNewCallsflag; trailing fires only when new calls arrived during wait intervalpackages/ui/src/CheckboxGroup.tshandleKeytoeventsemitterpackages/ui/src/Tree.tshandleKeytoeventsemitter + addhandleKeymethodpackages/core/src/style/Color.tsg_c/b_cfor green/blue channels in grayscale distance calculationTests
All 7 throttle tests pass; 49 CheckboxGroup tests pass; 1 batch test pass; build and typecheck pass.
Summary by CodeRabbit
New Features
Bug Fixes