feat(ui): add interactive, draggable, and resizable Window and WindowManager components - #3728
feat(ui): add interactive, draggable, and resizable Window and WindowManager components#3728Unnati1007 wants to merge 2 commits into
Conversation
github-actions
Bot
added
type:feature
|
@Karanjot786 please review the PR and merge it under GSSoC'26 |
📝 WalkthroughWalkthroughThe store now isolates asynchronous batch state with ChangesStore batch isolation
Terminal window UI
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Input as Terminal input
participant Manager as WindowManager
participant Window as Window
participant Layout as Layout system
Input->>Manager: Send mouse action
Manager->>Window: Resolve click target
Manager->>Window: Update focus or window state
Manager->>Layout: Synchronize window and content bounds
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: 13
🧹 Nitpick comments (3)
packages/store/src/store.ts (1)
46-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unexplained
anytypes.
globalBatchStoresintroducesSet<any>andBatchEntry<any>without an inline justification. Use an internal erased type based onunknown, or document why a safe typed representation is not possible.As per coding guidelines, “No
anywithout an inline comment explaining why.”🤖 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 46, Update the globalBatchStores declaration to remove both any usages, replacing them with an internal erased representation based on unknown that remains compatible with its Map and BatchEntry usage; do not add any unless an inline justification is required.Source: Coding guidelines
packages/ui/src/WindowManager.ts (1)
199-208: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the empty children list in
getLayoutNode.The override returns a node with
childrenset to[]. This bypasses the flex layout for windows, andsyncLayout()positions each window directly. The intent is clear from the inline comment, but a caller that expectsgetLayoutNode()to describe the subtree receives an incomplete tree.Extend the comment to state that window layout nodes are computed separately in
syncLayout().🤖 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/WindowManager.ts` around lines 199 - 208, Extend the inline comment in getLayoutNode() to explicitly state that the empty children list intentionally bypasses flex layout because window layout nodes are computed and positioned separately by syncLayout().packages/ui/src/Window.ts (1)
29-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider setting
focusable = trueonWindow.
Windowrenders a focus ring and readsthis.isFocusedat Line 194.WindowManager._focusWindowassignsisFocuseddirectly. The widget focus system is bypassed, so keyboard focus traversal does not reach windows.As per coding guidelines: "Set
focusable = trueon components that accept keyboard focus".🤖 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/Window.ts` around lines 29 - 79, Set the Window component’s focusable property to true during construction so it participates in the widget focus system and keyboard traversal. Update the Window constructor near its existing state initialization, while preserving the current isFocused and WindowManager focus behavior.Source: Coding guidelines
🤖 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/store/src/store.ts`:
- Around line 45-47: Move the pending-entry map and flush-generation state from
the module-level globalBatchStores and globalBatchEpoch variables into
BatchContext so independent root batches remain isolated. Update batch creation
and the flush scheduling/processing flow, including flushBatch and the logic
around lines 147–159, to pass and use the active context rather than shared
globals. Add coverage for overlapping root batches where one completes
synchronously and the other is suspended, including same-store updates and
independent notifications/commit or rollback behavior.
- Around line 147-152: Update flushBatch and the rollback closures registered in
the deferred batch paths to discard only the rejected batch entry rather than
restoring a stale prevState snapshot. Ensure final listener notifications use
the state immediately before the deferred commit, preserving unrelated immediate
updates made while the batch was suspended. Add a regression test covering an
independent update followed by rejection of the suspended batch.
In `@packages/ui/src/Window.test.ts`:
- Line 88: Remove the invalid caps.unicode getter spy from the Window test,
since unicode is a writable data property rather than an accessor. Remove the
caps import as well if it is no longer used after deleting the spy.
- Around line 13-36: Update the Window tests, including the cases around
“initializes with default options”, “can minimize and maximize”, and the later
tests, to create a real Screen for each behavior, attach the Window, call
updateRect() and render(), and assert the resulting rendered or observable
Screen state instead of only inspecting widget internals.
In `@packages/ui/src/Window.ts`:
- Around line 155-178: Update Window.getClickTarget so control hit testing never
evaluates positions below column 1: before checking each enabled control, ensure
rightOffset is at least 1, and stop further control checks once that lower bound
is reached. Preserve the existing title-region behavior while preventing clicks
on columns where _renderSelf did not draw a control.
- Around line 221-233: Update Window._renderSelf at
packages/ui/src/Window.ts:221-233 to use the existing useUnicode/caps.unicode
decision when rendering controls, selecting ASCII x, =, and o replacements for
×, ⧉, and ▢ on non-Unicode terminals; also update
packages/ui/src/Window.ts:207-219 to select . instead of … for title truncation
when Unicode is unavailable.
- Around line 129-135: Update Window.close() to call this.markDirty() after
mutating the window state and style, ensuring the closed window schedules a
repaint even when it has no parent.
- Around line 207-219: Update the title truncation logic in the Window title
rendering block to compute available space from the title start column through
the first control button, clamping it to a nonnegative width. Ensure the
rendered title never exceeds that space, including narrow windows, and replace
the non-ASCII ellipsis with the supported ASCII fallback.
In `@packages/ui/src/WindowManager.ts`:
- Around line 68-81: Remove the locally constructed InputParser and its
start/stop lifecycle from WindowManager.mount() and unmount(); do not attach
another parser to process.stdin. Receive global mouse events through the widget
tree or an injected/shared input parser, preserving the existing
_handleGlobalMouse handling and unsubscribe cleanup.
- Around line 162-178: Update the drag and resize logic in the `drag.mode`
branches to keep the entire window inside the manager rectangle: derive the drag
position maxima from `managerW`/`managerH` minus the current window
width/height, and constrain resized dimensions between `minWidth`/`minHeight`
and the corresponding manager dimensions minus the window’s current position.
Remove the `managerW - 3` and `managerH - 1` magic-number bounds while
preserving minimum-size enforcement.
- Around line 246-252: Replace the any parameter type in the shiftCoordinates
helper with the imported LayoutNode type, preserving the existing recursive
updates to computed coordinates and children.
- Line 16: Add packages/ui/src/WindowManager.test.ts for the WindowManager
component, following the existing Window.test.ts testing conventions and
covering its basic instantiation/rendering behavior. Keep the implementation
focused on satisfying the UI component test guideline.
- Around line 243-256: Prevent cumulative coordinate drift in the syncLayout
flow around computeLayout and shiftCoordinates. Apply the contentRect offset
only to layout coordinates produced by the current computation, or otherwise
ensure previously shifted child coordinates are reset before each shift;
preserve correct absolute positioning when computeLayout returns early for
clean, unchanged nodes.
---
Nitpick comments:
In `@packages/store/src/store.ts`:
- Line 46: Update the globalBatchStores declaration to remove both any usages,
replacing them with an internal erased representation based on unknown that
remains compatible with its Map and BatchEntry usage; do not add any unless an
inline justification is required.
In `@packages/ui/src/Window.ts`:
- Around line 29-79: Set the Window component’s focusable property to true
during construction so it participates in the widget focus system and keyboard
traversal. Update the Window constructor near its existing state initialization,
while preserving the current isFocused and WindowManager focus behavior.
In `@packages/ui/src/WindowManager.ts`:
- Around line 199-208: Extend the inline comment in getLayoutNode() to
explicitly state that the empty children list intentionally bypasses flex layout
because window layout nodes are computed and positioned separately by
syncLayout().
🪄 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: 7f804649-bb79-4b2a-96d6-9954f68c8aff
📒 Files selected for processing (6)
packages/store/src/store.test.tspackages/store/src/store.tspackages/ui/src/Window.test.tspackages/ui/src/Window.tspackages/ui/src/WindowManager.tspackages/ui/src/index.ts
| const batchLocalStorage = new AsyncLocalStorage<BatchContext>(); | ||
| const globalBatchStores = new Map<Set<any>, BatchEntry<any>>(); | ||
| let globalBatchEpoch = 0; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Scope pending entries and flush state to each BatchContext.
AsyncLocalStorage isolates only depth. globalBatchStores and globalBatchEpoch still join independent root batches.
If batch A queues its synchronous flush, then batch B starts before the microtask runs, Line 157 suppresses A's notification. If batch B remains suspended, A's update stays pending. If both batches update the same store, batch B can also commit or roll back batch A's entry.
Store the pending-entry map and flush generation in BatchContext. Pass that context to flushBatch. Add coverage for overlapping root batches where one batch is synchronous and the other is suspended.
Also applies to: 107-112, 147-159
🤖 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` around lines 45 - 47, Move the pending-entry map
and flush-generation state from the module-level globalBatchStores and
globalBatchEpoch variables into BatchContext so independent root batches remain
isolated. Update batch creation and the flush scheduling/processing flow,
including flushBatch and the logic around lines 147–159, to pass and use the
active context rather than shared globals. Add coverage for overlapping root
batches where one completes synchronously and the other is suspended, including
same-store updates and independent notifications/commit or rollback behavior.
| function flushBatch(threw: boolean, immediate = false) { | ||
| if (threw) { | ||
| for (const [, { rollback }] of _batchStores) { | ||
| for (const [, { rollback }] of globalBatchStores) { | ||
| rollback(); | ||
| } | ||
| _batchStores.clear(); | ||
| globalBatchStores.clear(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not restore a stale pre-batch snapshot on rollback.
A suspended batch captures prevState before an unrelated update can commit. If the batch later rejects, Line 150 calls rollback, and the closure at Line 461 or Line 574 restores that stale snapshot.
For example, a deferred { x: 1 } update followed by an immediate { y: 10 } update will revert y to its old value when the deferred batch rejects. Deferred batch changes have not been committed, so rollback must discard only the deferred entry. Final listener notifications must also use the state immediately before the deferred commit.
Add a rejection regression test with an independent update while the batch is suspended.
Also applies to: 449-467, 558-580
🤖 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` around lines 147 - 152, Update flushBatch and
the rollback closures registered in the deferred batch paths to discard only the
rejected batch entry rather than restoring a stale prevState snapshot. Ensure
final listener notifications use the state immediately before the deferred
commit, preserving unrelated immediate updates made while the batch was
suspended. Add a regression test covering an independent update followed by
rejection of the suspended batch.
| it('initializes with default options', () => { | ||
| const win = new Window({ title: 'Test Window' }); | ||
| expect(win.title).toBe('Test Window'); | ||
| expect(win.windowX).toBe(0); | ||
| expect(win.windowY).toBe(0); | ||
| expect(win.windowWidth).toBe(30); | ||
| expect(win.windowHeight).toBe(10); | ||
| expect(win.isMinimized).toBe(false); | ||
| expect(win.isMaximized).toBe(false); | ||
| expect(win.isClosed).toBe(false); | ||
| }); | ||
|
|
||
| it('can minimize and maximize', () => { | ||
| const win = new Window({ x: 2, y: 3, width: 20, height: 10 }); | ||
| win.minimize(); | ||
| expect(win.isMinimized).toBe(true); | ||
|
|
||
| win.maximize(); | ||
| expect(win.isMaximized).toBe(true); | ||
| win.maximize(); | ||
| expect(win.isMaximized).toBe(false); | ||
| expect(win.windowX).toBe(2); | ||
| expect(win.windowY).toBe(3); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect existing UI test patterns that use Screen rendering.
rg -n -C 3 \
--glob '*.test.ts' \
--glob '*.test.tsx' \
'new Screen|\.updateRect\(|\.render\(' \
packagesRepository: Karanjot786/TermUI
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the failing test file and the Widget contract used by UI tests.
printf '--- packages/ui/src/Window.test.ts outline ---\n'
ast-grep outline packages/ui/src/Window.test.ts --view compact || true
printf '\n--- packages/ui/src/Window.test.ts first 130 lines ---\n'
sed -n '1,130p' packages/ui/src/Window.test.ts | nl -ba
printf '\n--- packages/ui/src/Window.ts imports ---\n'
sed -n '1,80p' packages/ui/src/Window.ts | nl -ba
printf '\n--- Widget render/updateRect availability ---\n'
fd -e ts -i 'widget\.ts$|widget\.test\.ts$' packages | head -20
rg -n "class Widget|abstract .*render|render\\(\\(" packages/widgets/src packages/ui/src/Window.tsRepository: Karanjot786/TermUI
Length of output: 247
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the test file and Widget contract without printf option issues.
printf '%s\n' '--- packages/ui/src/Window.test.ts outline ---'
ast-grep outline packages/ui/src/Window.test.ts --view compact || true
printf '%s\n' ''
printf '%s\n' '--- packages/ui/src/Window.test.ts first 130 lines ---'
sed -n '1,130p' packages/ui/src/Window.test.ts | nl -ba
printf '%s\n' ''
printf '%s\n' '--- packages/ui/src/Window.ts imports ---'
sed -n '1,80p' packages/ui/src/Window.ts | nl -ba
printf '%s\n' ''
printf '%s\n' '--- Widget render/updateRect availability ---'
fd -e ts -i 'widget\.ts$|widget\.test\.ts$' packages | head -20
rg -n "class Widget|abstract .*render|render\\(\\(" packages/widgets/src packages/ui/src/Window.tsRepository: Karanjot786/TermUI
Length of output: 451
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- packages/ui/src/Window.test.ts first 130 lines ---'
sed -n '1,130p' packages/ui/src/Window.test.ts
echo
echo '--- packages/ui/src/Window.ts first 80 lines ---'
sed -n '1,80p' packages/ui/src/Window.ts
echo
echo '--- Widget class references ---'
rg -n "class Widget|render\\(|updateRect\\(" packages/widgets/src packages/ui/src/Window.tsRepository: Karanjot786/TermUI
Length of output: 50377
Render these widget tests through Screen.
The current cases call widget methods and inspect internal state only. They do not create a real Screen, call updateRect(), or call render(), so they miss the required UI test path. Create a real Screen for each behavior and assert rendered or observable screen state. Also applies to lines 40-57 and 61-103.
🤖 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/Window.test.ts` around lines 13 - 36, Update the Window
tests, including the cases around “initializes with default options”, “can
minimize and maximize”, and the later tests, to create a real Screen for each
behavior, attach the Window, call updateRect() and render(), and assert the
resulting rendered or observable Screen state instead of only inspecting widget
internals.
Source: Coding guidelines
| }); | ||
|
|
||
| it('syncs window content positions', () => { | ||
| vi.spyOn(caps, 'unicode', 'get').mockReturnValue(true); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Confirm that `unicode` is a data property rather than a getter accessor.
rg -n -C 3 'unicode:' packages/core/src/terminal/env-caps.ts
rg -n -C 3 "spyOn\\(caps, 'unicode', 'get'\\)" packages/ui/src/Window.test.tsRepository: Karanjot786/TermUI
Length of output: 928
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
const caps = {
unicode: !process.env.NO_UNICODE && process.env.TERM !== 'dumb',
motion: !process.env.NO_MOTION && !process.env.CI,
};
const symbol = Object.getOwnPropertyDescriptors(Object.getPrototypeOf(caps));
console.log(JSON.stringify({
typeofDescriptor: typeof symbol.get,
ownDescriptor: JSON.stringify(Object.getOwnPropertyDescriptor(caps, 'unicode')),
prototypeDescriptor: symbol.get && JSON.stringify({ get: 'function' }) ?? null,
}, null, 2));
function getDescriptor(obj, prop) {
let current = obj;
while (current != null) {
const desc = Object.getOwnPropertyDescriptor(current, prop);
if (desc != null) return desc;
current = Object.getPrototypeOf(current);
}
return undefined;
}
const descriptor = getDescriptor(caps, 'unicode');
console.log(JSON.stringify({
descriptorGet: descriptor?.get != null,
descriptorEnumerable: descriptor?.enumerable,
descriptorConfigurable: descriptor?.configurable,
descriptorWritable: descriptor?.writable ?? null,
}, null, 2));
JS
sed -n '1,95p' packages/ui/src/Window.test.tsRepository: Karanjot786/TermUI
Length of output: 933
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
const caps = {
unicode: envProp('NO_UNICODE') && process.env.TERM !== 'dumb',
motion: envProp('NO_MOTION') && !process.env.CI,
};
function envProp(name) { return !process.env[name]; }
const descriptor = Object.getOwnPropertyDescriptor(caps, 'unicode');
console.log(JSON.stringify({
ownDescriptorGet: descriptor && descriptor.get !== null,
ownDescriptorEnumerable: descriptor && descriptor.enumerable,
ownDescriptorConfigurable: descriptor && descriptor.configurable,
ownDescriptorWritable: descriptor && descriptor.writable,
}, null, 2));
JS
sed -n '1,95p' packages/ui/src/Window.test.tsRepository: Karanjot786/TermUI
Length of output: 4156
Remove the invalid caps.unicode getter spy.
caps.unicode is defined as a writable data property in packages/core/src/terminal/env-caps.ts. vi.spyOn(caps, 'unicode', 'get') does not attach to this property and can fail in test setup. Remove the spy and the unused caps import if no Unicode behavior is covered.
🤖 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/Window.test.ts` at line 88, Remove the invalid caps.unicode
getter spy from the Window test, since unicode is a writable data property
rather than an accessor. Remove the caps import as well if it is no longer used
after deleting the spy.
Source: Learnings
| close(): void { | ||
| this.isClosed = true; | ||
| this.setStyle({ visible: false }); | ||
| if (this.parent) { | ||
| this.parent.removeChild(this); | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Call this.markDirty() in close().
close() mutates isClosed and the style. It does not mark the widget dirty. If the window has no parent, the removal path does not run and no repaint is scheduled.
As per coding guidelines: "Every state-mutating method on a widget calls this.markDirty()."
🔧 Proposed fix
close(): void {
this.isClosed = true;
this.setStyle({ visible: false });
+ this.markDirty();
if (this.parent) {
this.parent.removeChild(this);
}
}📝 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.
| close(): void { | |
| this.isClosed = true; | |
| this.setStyle({ visible: false }); | |
| if (this.parent) { | |
| this.parent.removeChild(this); | |
| } | |
| } | |
| close(): void { | |
| this.isClosed = true; | |
| this.setStyle({ visible: false }); | |
| this.markDirty(); | |
| if (this.parent) { | |
| this.parent.removeChild(this); | |
| } | |
| } |
🤖 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/Window.ts` around lines 129 - 135, Update Window.close() to
call this.markDirty() after mutating the window state and style, ensuring the
closed window schedules a repaint even when it has no parent.
Source: Coding guidelines
| } from '@termuijs/core'; | ||
| import { Window } from './Window.js'; | ||
|
|
||
| export class WindowManager extends Widget { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for the WindowManager test file and the index exports.
fd -t f 'Window' packages/ui/src
rg -nP --type=ts "export .*\b(Window|WindowManager)\b" packages/ui/src/index.tsRepository: Karanjot786/TermUI
Length of output: 398
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Window-related component/test files:"
fd -t f 'Window' packages/ui/src | sort
echo
echo "Relevant index exports:"
sed -n '240,256p' packages/ui/src/index.ts | cat -n
echo
echo "Window.test.ts outline:"
ast-grep outline packages/ui/src/Window.test.ts --view expanded || true
echo
echo "WindowManager.test.ts outline:"
ast-grep outline packages/ui/src/WindowManager.test.ts --view expanded || true
echo
echo "References to WindowManager in UI source/tests:"
rg -n --type=ts --type=tsx "WindowManager|windowManager" packages/ui/src || trueRepository: Karanjot786/TermUI
Length of output: 1294
Add WindowManager.test.ts.
packages/ui/src/index.ts exports WindowManager, but the component only has packages/ui/src/Window.test.ts. Create packages/ui/src/WindowManager.test.ts to satisfy the UI component guideline.
🤖 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/WindowManager.ts` at line 16, Add
packages/ui/src/WindowManager.test.ts for the WindowManager component, following
the existing Window.test.ts testing conventions and covering its basic
instantiation/rendering behavior. Keep the implementation focused on satisfying
the UI component test guideline.
Source: Coding guidelines
| override mount(): void { | ||
| super.mount(); | ||
| this._globalInput = new InputParser(process.stdin); | ||
| this._globalInput.start(); | ||
| this._unsubGlobalMouse = this._globalInput.onMouse((event: TermMouseEvent) => this._handleGlobalMouse(event)); | ||
| } | ||
|
|
||
| override unmount(): void { | ||
| this._unsubGlobalMouse?.(); | ||
| this._unsubGlobalMouse = null; | ||
| this._globalInput?.stop(); | ||
| this._globalInput = null; | ||
| super.unmount(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Do not create a second InputParser on process.stdin.
mount() constructs its own InputParser and calls start(). The snippet from packages/core/src/input/InputParser.ts:63-222 shows that start() attaches a data handler to the supplied stdin stream and that the parser owns a StringDecoder. The application already runs an input parser for the same stream.
Two parsers decode the same bytes independently. Escape sequences split across chunks can be parsed inconsistently, and stop() calls this._decoder.end() on a stream the other parser still uses.
Receive mouse events through the widget tree or through an injected parser instead of constructing one here.
🤖 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/WindowManager.ts` around lines 68 - 81, Remove the locally
constructed InputParser and its start/stop lifecycle from WindowManager.mount()
and unmount(); do not attach another parser to process.stdin. Receive global
mouse events through the widget tree or an injected/shared input parser,
preserving the existing _handleGlobalMouse handling and unsubscribe cleanup.
| if (drag.mode === 'drag') { | ||
| // Update relative position and clamp title bar within manager boundary | ||
| const nextX = drag.winStartX + dx; | ||
| const nextY = drag.winStartY + dy; | ||
|
|
||
| drag.win.windowX = Math.max(0, Math.min(managerW - 3, nextX)); | ||
| drag.win.windowY = Math.max(0, Math.min(managerH - 1, nextY)); | ||
| } else if (drag.mode === 'resize') { | ||
| const direction = drag.resizeDirection; | ||
| if (direction === 'both' || direction === 'horizontal') { | ||
| const nextW = drag.winStartWidth + dx; | ||
| drag.win.windowWidth = Math.max(drag.win.minWidth, nextW); | ||
| } | ||
| if (direction === 'both' || direction === 'vertical') { | ||
| const nextH = drag.winStartHeight + dy; | ||
| drag.win.windowHeight = Math.max(drag.win.minHeight, nextH); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Clamp the window bounds to the manager rectangle.
Two problems exist in this block.
The drag clamp uses managerW - 3 and managerH - 1. These magic numbers do not depend on the window size, so a wide window can extend far past the right edge of the manager.
The resize branch clamps only to minWidth and minHeight. It has no upper bound, so a drag to the right can grow a window past the manager rectangle. Window._renderSelf then writes cells outside the manager area.
Derive both clamps from managerW, managerH, and the current window size.
🤖 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/WindowManager.ts` around lines 162 - 178, Update the drag and
resize logic in the `drag.mode` branches to keep the entire window inside the
manager rectangle: derive the drag position maxima from `managerW`/`managerH`
minus the current window width/height, and constrain resized dimensions between
`minWidth`/`minHeight` and the corresponding manager dimensions minus the
window’s current position. Remove the `managerW - 3` and `managerH - 1`
magic-number bounds while preserving minimum-size enforcement.
| computeLayout(winLayoutNode, contentRect.width, contentRect.height); | ||
|
|
||
| // Shift child coordinates from relative content space to absolute screen space | ||
| const shiftCoordinates = (node: any, dx: number, dy: number) => { | ||
| node.computed.x += dx; | ||
| node.computed.y += dy; | ||
| for (const c of node.children) { | ||
| shiftCoordinates(c, dx, dy); | ||
| } | ||
| }; | ||
|
|
||
| for (const c of winLayoutNode.children) { | ||
| shiftCoordinates(c, contentRect.x, contentRect.y); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Fix cumulative coordinate drift in the child shift.
computeLayout returns early when the node is not dirty and the container size did not change. The snippet from packages/core/src/layout/LayoutEngine.ts:72-84 confirms this bail-out. shiftCoordinates runs on every syncLayout() call and adds contentRect.x and contentRect.y to node.computed each time.
When computeLayout bails out, the previous shift is not reset. The children then move by one content offset on every frame and leave the window.
Shift into a derived value, or force a recompute before shifting.
🔧 Proposed fix
- computeLayout(winLayoutNode, contentRect.width, contentRect.height);
+ // Force a fresh relative layout so the absolute shift below is never applied twice.
+ winLayoutNode._dirty = true;
+ winLayoutNode._lastContainerWidth = -1;
+ winLayoutNode._lastContainerHeight = -1;
+ computeLayout(winLayoutNode, contentRect.width, contentRect.height);🤖 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/WindowManager.ts` around lines 243 - 256, Prevent cumulative
coordinate drift in the syncLayout flow around computeLayout and
shiftCoordinates. Apply the contentRect offset only to layout coordinates
produced by the current computation, or otherwise ensure previously shifted
child coordinates are reset before each shift; preserve correct absolute
positioning when computeLayout returns early for clean, unchanged nodes.
| const shiftCoordinates = (node: any, dx: number, dy: number) => { | ||
| node.computed.x += dx; | ||
| node.computed.y += dy; | ||
| for (const c of node.children) { | ||
| shiftCoordinates(c, dx, dy); | ||
| } | ||
| }; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace any with LayoutNode.
LayoutNode is already imported at Line 5 and describes computed and children.
As per coding guidelines: "No any without an inline comment explaining why."
🔧 Proposed fix
- const shiftCoordinates = (node: any, dx: number, dy: number) => {
+ const shiftCoordinates = (node: LayoutNode, dx: number, dy: number): void => {
node.computed.x += dx;
node.computed.y += dy;
for (const c of node.children) {
shiftCoordinates(c, dx, dy);
}
};📝 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 shiftCoordinates = (node: any, dx: number, dy: number) => { | |
| node.computed.x += dx; | |
| node.computed.y += dy; | |
| for (const c of node.children) { | |
| shiftCoordinates(c, dx, dy); | |
| } | |
| }; | |
| const shiftCoordinates = (node: LayoutNode, dx: number, dy: number): void => { | |
| node.computed.x += dx; | |
| node.computed.y += dy; | |
| for (const c of node.children) { | |
| shiftCoordinates(c, dx, dy); | |
| } | |
| }; |
🤖 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/WindowManager.ts` around lines 246 - 252, Replace the any
parameter type in the shiftCoordinates helper with the imported LayoutNode type,
preserving the existing recursive updates to computed coordinates and children.
Source: Coding guidelines
|
Window/WindowManager compiles, drag/resize wired+tested. Two blockers: (1) Window.ts:632-637 truncates the title with raw .length/.slice — use stringWidth/truncate. (2) it silently bundles an undisclosed AsyncLocalStorage rewrite of store.ts batch()/flushBatch (+101/-58), unrelated + colliding with #3245/#3730. Drop the store.ts changes; fix title truncation. |
2 participants
Description
This PR implements and exports the new
WindowandWindowManagercomponents under the@termuijs/uipackage. These components allow developers to build multi-window terminal dashboard environments where individual panels can be dynamically dragged, resized, minimized, maximized, focused, and stacked using mouse interactions.Related Issue
Closes #3726
Which package(s)?
@termuijs/ui
Type of Change
type:bug)type:feature)type:docs)type:testing)type:refactor)type:design)type:accessibility)type:performance)type:devops)type:security)Checklist
needs-starcheck blocks your merge otherwise.bun vitest runbun run buildbun run typecheckCONTRIBUTING.md.type: short description.markDirty()(if your change affects rendering).anytypes without an inline comment explaining why.GSSoC 2026 Participation
https://gssoc.girlscript.org/profile/Unnati1007Screenshots / Recordings (UI changes)
Notes for the Reviewer
markDirty()inside state changes to trigger proper window layout redraws.Window.test.tsfor all core features (dragging, bounds clamping, maximizing, minimizing, focus swapping, and event propagation).Summary by CodeRabbit
New Features
Bug Fixes
Tests