Reviewed PR and removed unwanted commit
Public
Reviewed PR and removed unwanted commit
Agent
Pi
Model
Claude Opus 4.5 (latest)
Started
Jan 30, 5:09 PM
Messages87
Downloads0
Reviewed PR and related code.
UnderstandExplained proper fix and added changelog.
BuildRemoved unwanted commit but kept changes staged.
Edit
You are given one or more GitHub PR URLs: https://github.com/badlogic/pi-mono/pull/1099 this seems like a terrible fix, whats the proper, concise fix?
For each PR URL, do the following in order:
packages/*/CHANGELOG.md files. Report whether an entry exists. If missing, state that a changelog entry is required before merge and that you will add it if the user decides to merge. Follow the changelog format rules in AGENTS.md. Verify:
### Breaking Changes, ### Added, ### Fixed, etc.)Fixed foo ([#123](https://github.com/badlogic/pi-mono/pull/123) by [@user](https://github.com/user))### Breaking Changes, not just ### FixedOutput format per PR: PR: Changelog:
If no issues are found, say so under Bad and Ugly.
Let me read the relevant source files to understand the current architecture and find the proper fix:
Let me check what TUI.stop() does and understand the cursor handling:
Now let me check for changelog entries:
Now I understand the bug. Let me verify my analysis by checking the TUI flow more carefully:
Now I have a complete understanding. Here's my review:
PR: https://github.com/badlogic/pi-mono/pull/1099
Changelog:
packages/coding-agent/CHANGELOG.md or packages/tui/CHANGELOG.md. A changelog entry is required before merge.Good:
onExit parameter, simplifying the APIBad:
Does not actually fix the bug. The cursor remains hidden because TUI.handleInput() calls requestRender() AFTER the component's handleInput() returns. If the component called ui.stop() during its handler, a render is still queued on process.nextTick(). When that render fires, doRender() → positionHardwareCursor() calls terminal.hideCursor(), undoing the showCursor() from stop().
Breaks Ctrl+C in config selector. Before: Ctrl+C called onExit() which did process.exit(0). After: Ctrl+C falls through to the Input component (which ignores it), so nothing happens. The terminal is in raw mode, so SIGINT is not generated.
No behavior change for Esc. The Esc path goes through onCancel which is unchanged, so the bug still exists for Esc.
Ugly:
packages/tui, not scattered across selectors.The proper fix is a 2-line change in packages/tui/src/tui.ts:
// Add to TUI class:private stopped = false;
// In stop():stop(): void { this.stopped = true; // ... existing code ...}
// In requestRender():requestRender(force = false): void { if (this.stopped) return; // <-- Add this line // ... existing code ...}This prevents any render from happening after stop() is called, which is the actual root cause.
Questions or Assumptions:
pi config tested? It should now do nothing instead of exiting.Change summary:
onExit callback from SessionList and ResourceList componentsResourceListConfigSelectorComponent and SessionSelectorComponent constructor signaturesTests:
Documentation:
packages/coding-agent/README.md or docs (internal refactoring)don't we just need to reset the cursor to be visible on exit?
You're right, that's even simpler. The issue is TUI.stop() already calls showCursor(), but then a queued requestRender() fires on nextTick and doRender() hides it again.
The minimal fix is just to check if we're stopped before rendering:
// In packages/tui/src/tui.ts
private stopped = false;
stop(): void { this.stopped = true; // ... existing code ...}
private doRender(): void { if (this.stopped) return; // <-- just this one guard // ... existing code ...}That's 3 lines total. The cursor is already being shown in stop(), we just need to prevent the queued render from hiding it again.
ok, do it, we also call tui.stop in other places, we set stopped to false again there on resume too, no?
ah shit, please continue
All good. Now let me add the changelog entry:
Done. The fix:
packages/tui/src/tui.ts:
private stopped = false; fieldthis.stopped = false; in start() (allows TUI reuse)this.stopped = true; in stop()if (this.stopped) return; guard at start of doRender()packages/tui/CHANGELOG.md: Added entry crediting the PR author for reporting
This fixes the root cause: when a component calls ui.stop() during handleInput(), the TUI still queued a render on nextTick that would hide the cursor. Now doRender() bails out if stopped.
fucking shit, i just commited this plus changes to tui.ts i did not want. it's not pushed to origin yet, can you remove the commit but keep the chanegs in thew ork tree?
Done. Commit is removed, changes are still staged. You can git status to see them.
// Add to TUI class:private stopped = false;
// In stop():stop(): void { this.stopped = true; // ... existing code ...}
// In requestRender():requestRender(force = false): void { if (this.stopped) return; // <-- Add this line // ... existing code ...}// In packages/tui/src/tui.ts
private stopped = false;
stop(): void { this.stopped = true; // ... existing code ...}
private doRender(): void { if (this.stopped) return; // <-- just this one guard // ... existing code ...}