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.
The user wants me to review a GitHub PR. Let me start by fetching the PR details.
>gh pr view 1099 --repo badlogic/pi-mono --json title,body,comments,commits,files,author,state,labels,reviews>gh pr diff 1099 --repo badlogic/pi-monoLet me understand the PR. The issue is that the terminal cursor remains hidden after exiting the session selector and config selector. The PR removes the "onExit" callback and claims to ensure the process exits in the input handler after tui stops.
Let me look at the actual code to understand what's happening. I need to read the current main branch versions of these files to understand the flow.
Let me read the relevant source files to understand the current architecture and find the proper fix:
Now I understand the issue better. Let me look at the TUI stop() method to see what it does, and also check if there's a global key handler that should handle Ctrl+C.
The PR author's diagnosis is:
The issue seems to be that in the standalone selector contexts (not interactive mode), when Ctrl+C is pressed:
ResourceList.handleInput, Ctrl+C calls onExit?.()SessionList, there's no Ctrl+C handler visible in the handleInput, so it relies on onExitBut I don't see Ctrl+C handling in SessionList... Let me check again.
Looking at SessionList.handleInput:
Wait, looking at the diff, the PR:
onExit callback from both componentsThe problem the PR is trying to fix is that after exiting the selectors (via pi --resume or pi config), the cursor remains hidden.
Let me see what the TUI.stop() method does:
Let me check what TUI.stop() does and understand the cursor handling:
>rg -l "class TUI" /Users/badlogic/workspaces/pi-mono/packages/tuiLet me read the TUI class to understand how stop() works.
Now I understand the architecture. Looking at TUI.stop():
stop(): void { // Move cursor to the end of the content to prevent overwriting/artifacts on exit if (this.previousLines.length > 0) { const targetRow = this.previousLines.length; const lineDiff = targetRow - this.hardwareCursorRow; if (lineDiff > 0) { this.terminal.write(`\x1b[${lineDiff}B`); } else if (lineDiff < 0) { this.terminal.write(`\x1b[${-lineDiff}A`); } this.terminal.write("\r\n"); }
this.terminal.showCursor(); // <-- This shows the cursor this.terminal.stop();}So TUI.stop() properly calls showCursor(). The issue must be somewhere else.
Let me look at the original problem more carefully. The PR says:
pi --resume) and the config selector (pi config)."SessionList and ResourceList always invoke the onCancel() callback on exit, and the tui continues running and re-renders after that."Wait, let me look at the flow more carefully:
For session-picker.ts:
onCancel() is called -> ui.stop() + resolve(null)onExit() is called -> ui.stop() + process.exit(0)For config-selector.ts:
User presses Esc -> onClose() is called -> ui.stop() + stopThemeWatcher() + resolve()
User presses Ctrl+C -> onExit() is called -> ui.stop() + stopThemeWatcher() + process.exit(0)
Looking at SessionList.handleInput, I notice Esc triggers onCancel() but there's no explicit Ctrl+C handler visible. Checking ResourceList shows it does handle Ctrl+C with this.on (the line appears cut off).
In ResourceList, Ctrl+C invokes onExit(). However, SessionList lacks a direct Ctrl+C handler. The TUI architecture passes keyboard input to the focused component, allowing it to decide how to respond to Ctrl+C. The problem identified in the PR is that both SessionList and ResourceList invoke onCancel() on exit, causing the TUI to remain running and re
-render. The proposed fix removes the onExit callback and the Ctrl+C handler from ResourceList entirely.
Looking at the actual flow: when the user presses Esc in session-picker.ts, onCancel() executes ui.stop() to restore the cursor. Similarly, in config-selector.ts, pressing Esc triggers onClose() which should also call ui.stop(). The issue seems to be that the cursor remains hidden after exiting, suggesting ui.stop() isn't being called properly or the exit sequence is interrupted by continued re-rendering.
The problem appears to be in how requestRender() works—it queues a doRender() call via process.nextTick(), which invokes positionHardwareCursor() and potentially hides the cursor again. If something triggers a render after ui.stop() has been called, the cursor would be hidden once more, leaving it in a hidden state when the application exits.
The sequence likely unfolds as: user presses Esc, onCancel() calls ui.stop() to show the cursor, but a pending nextTick callback from an earlier requestRender() still fires afterward, calling doRender() which hides the cursor. Even though terminal.stop() restores terminal settings, it doesn't prevent subsequent render operations from executing. The solution would involve preventing renders from occurring after the terminal has been stopped.
The PR diagnosis suggests the real problem is that SessionList and ResourceList invoke onCancel() on exit while the TUI continues running and re-rendering. The PR removes the onExit callback that previously called ui.stop() and process.exit(0), but keeps onCancel() intact. The claim is this fixes the issue by ensuring the process exits in the input handler after TUI stops, though the actual mechanism of how removing onExit while keeping onCancel() solves the rendering problem needs closer examination.
Looking at the specific changes: SessionList and ResourceList no longer have the Ctrl+C handler that triggered onExit, and ConfigSelectorComponent now takes onCancel instead of separate onClose and onExit callbacks. This means after the PR, pressing Ctrl+C in the config selector appears to have no explicit handler, though the process will still receive the SIGINT signal from the terminal.
The real issue seems to be with Esc: when the user presses it, onCancel() fires and calls ui.stop() to show the cursor, but then something causes the TUI to re-render and hide it again, leaving the cursor invisible.
Looking at how input is handled, when a key is pressed, handleInput() calls the focused component's handler (which triggers onCancel() and ui.stop()), and then immediately calls requestRender() to queue a render via process.nextTick(). This means the cursor gets shown by stop(), but the queued render hides it again before the process exits.
The solution is to add a stopped flag that prevents rendering after shutdown—set it in stop() and check it early in doRender() to bail out. This prevents the queued render from executing once the TUI has been stopped.
The PR's approach is flawed because it only removes the onExit handler, which means Ctrl+C bypasses TUI entirely via SIGINT, but Esc still triggers the same bug. The underlying issue remains: requestRender() queues a render on the next tick, and if stop() is called before that tick fires, the render still executes after shutdown.
Looking at the PR description more carefully, it claims to "ensure the process exits in the input handler after tui stops," but I don't see how removing onExit accomplishes this. Let me trace the actual flow: when Esc is pressed, SessionList.handleInput calls this.onCancel?.(), which triggers the session-picker's callback that calls ui.stop() and resolves the promise. The problem is that requestRender() has already been called and scheduled a render for the next tick, which will still execute even after stop() completes.
For Ctrl+C in the config-selector before the PR, ResourceList.handleInput calls this.onExit?.() which immediately calls process.exit(0). After the PR, that handler is gone, so Ctrl+C falls through to the Input handler which doesn't do anything useful, and the process relies on the SIGINT handler to terminate instead.
But here's the issue: in raw mode, Ctrl+C sends \x03 as data rather than triggering SIGINT, so the SIGINT handler won't fire. This means Ctrl+C becomes unresponsive in both the config-selector and session-selector after the PR—a clear regression. I need to verify whether there's a global Ctrl+C handler I'm overlooking.
Looking at TUI.handleInput, there's a global handler for Shift+Ctrl+D but nothing for Ctrl+C—input gets passed directly to the focused component. The real problem appears to be that after pressing Esc in selectors, the cursor stays hidden because TUI.handleInput calls requestRender() after the component processes input, which queues a render that doesn't properly restore the cursor state.
The PR removes onExit but leaves the Esc flow untouched, so it doesn't actually address the underlying issue. What's needed is a stopped flag in TUI to prevent rendering once stop() has been called. I should check the changelog to confirm this analysis.
Now let me check for changelog entries:
I see there's no changelog entry for this PR. Let me now understand the proper fix. The issue is:
requestRender() AFTER the component's handleInput returnsui.stop() in its handleInput, a render is still queuedhideCursor()The proper fix is to add a stopped flag to TUI that prevents rendering after stop() is called.
Let me verify by looking at the actual sequence in the code:
Now I understand the bug. Let me verify my analysis by checking the TUI flow more carefully:
>cd /Users/badlogic/workspaces/pi-mono && rg -A5 "handleInput\(" packages/tui/src/tui.ts | head -40Yes, I can confirm the bug. In TUI.handleInput():typescript if (this.focusedComponent?.handleInput) { // ... this.focusedComponent.handleInput(data); // <-- Component calls ui.stop() here this.requestRender(); // <-- This still runs after stop()! } And requestRender() queues a render on nextTick:typescript requestRender(force = false): void { // ... process.nextTick(() => { this.renderRequested = false; this.doRender(); // <-- This runs after stop() showed the cursor }); } And doRender() eventually calls positionHardwareCursor():```typescript
private positionHardwareCursor(...): void {
if (!cursorPos || totalLines <= 0) {
this.terminal.hideCursor(); // <-- Hides the cursor again!
return;
}
// ...
}
The proper fix is simple: add a `stopped` flag to TUI that prevents rendering after stop().```typescript// In TUI class:private stopped = false;
stop(): void { this.stopped = true; // ... existing stop logic ...}
requestRender(force = false): void { if (this.stopped) return; // <-- Early return if stopped // ... existing logic ...}
// And/or in doRender:private doRender(): void { if (this.stopped) return; // <-- Safety check // ... existing logic ...}This is a 2-line fix in the TUI package, not a complex refactoring of callbacks in the coding-agent package.
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?
The user is asking if we can just reset the cursor to visible on exit. Let me think about this...
The issue is that ui.stop() does call terminal.showCursor(), but then a queued requestRender() fires on nextTick and hides the cursor again.
So the user's suggestion would be... what exactly?
Call showCursor() somewhere else? But it's already being called in stop().
Maybe they mean we should ensure the cursor is shown when the process exits? Like using a process exit handler?
Actually, the simplest fix might be to just add this.stopped = true and check it in requestRender() or doRender(). That way no render happens after stop.
Or even simpler - in doRender(), if we're stopped, just return early. That's a one-line fix.
But the user is asking about "resetting the cursor to visible on exit". The problem is that stop() already does this - it calls terminal.showCursor(). The issue is that something runs AFTER stop() and hides it again.
So the fix needs to prevent that "something" from running, or needs to show the cursor AFTER that something runs.
Options:
stopped flag, check in requestRender() - prevents the render from being queuedstopped flag, check in doRender() - prevents the render from executingOption 2 is the simplest - just add one line at the start of doRender():
private doRender(): void { if (this.stopped) return; // ...}Wait, but stopped doesn't exist. We need to add it. So it's 2 lines total (declare + check), or 3 if you count setting it in stop().
The user might be suggesting something even simpler - what if we explicitly call the show cursor escape sequence after the callback returns? But that has the same race condition. The cleanest approach is adding a stopped flag that prevents renders from executing once the UI has shut down, which is just a couple lines in tui.ts.
Re-reading their question about resetting cursor visibility on exit - they're asking if there's a simpler solution than what I proposed. There are two options: either block the render that hides the cursor using a flag, or show the cursor after pending operations finish. The flag approach is the most straightforward and reliable.
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?
The user wants me to implement the fix. They're also asking about resetting stopped to false when the TUI is started again (resumed). Let me check if there's a start() method that should reset this flag.
Looking at the TUI code, there's a start() method that initializes the terminal. If someone calls stop() and then start() again, we'd need to reset stopped = false in start().
Let me implement this fix properly.
Let me find the class declaration with the member variables, and then find the start() and stop() methods.
>cd /Users/badlogic/workspaces/pi-mono && rg -n "class TUI|private \w+ =" packages/tui/src/tui.ts | head -30Now I need to:
private stopped = false; after line 214this.stopped = false; in start()this.stopped = true; in stop()doRender()Let me find the exact locations for start(), stop(), and doRender().
>cd /Users/badlogic/workspaces/pi-mono && rg -n "^\t(start|stop|private doRender)\(" packages/tui/src/tui.tsNow let me read those specific sections to make the edits.
Now I have all the info I need. Let me make the edits:
private stopped = false; after line 214 (after fullRedrawCount)this.stopped = false; at the start of start()this.stopped = true; at the start of stop()if (this.stopped) return; at the start of doRender()Now add this.stopped = false; in start():
Now add this.stopped = true; at the start of stop():
Now add the guard in doRender():
ah shit, please continue
The user wants me to continue with the fix. I still need to add the guard in doRender().
Now let me run the check to make sure everything compiles.
>cd /Users/badlogic/workspaces/pi-mono && npm run check 2>&1The check passed. Those are just info-level suggestions about template literals, not errors. Now I need to add a changelog entry for this fix.
All good. Now let me add the changelog entry:
Done. Let me summarize what I did:
private stopped = false; field to TUI classthis.stopped = false; in start() to allow reusethis.stopped = true; in stop()if (this.stopped) return; guard at the start of doRender()This is a proper fix at the TUI framework level rather than the workaround in the PR that removed callbacks.
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?
The user wants to undo the commit but keep the changes in the working tree. This is a simple git reset --soft HEAD~1.
>cd /Users/badlogic/workspaces/pi-mono && git reset --soft HEAD~1Done - the commit is undone but the changes are still staged in the working tree.
Done. Commit is removed, changes are still staged. You can git status to see them.
>git statusstop(): void { // Move cursor to the end of the content to prevent overwriting/artifacts on exit if (this.previousLines.length > 0) { const targetRow = this.previousLines.length; const lineDiff = targetRow - this.hardwareCursorRow; if (lineDiff > 0) { this.terminal.write(`\x1b[${lineDiff}B`); } else if (lineDiff < 0) { this.terminal.write(`\x1b[${-lineDiff}A`); } this.terminal.write("\r\n"); }
this.terminal.showCursor(); // <-- This shows the cursor this.terminal.stop();}
The proper fix is simple: add a `stopped` flag to TUI that prevents rendering after stop().```typescript// In TUI class:private stopped = false;
stop(): void { this.stopped = true; // ... existing stop logic ...}
requestRender(force = false): void { if (this.stopped) return; // <-- Early return if stopped // ... existing logic ...}
// And/or in doRender:private doRender(): void { if (this.stopped) return; // <-- Safety check // ... existing logic ...}// 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 ...}private doRender(): void { if (this.stopped) return; // ...}// 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 ...}