Reviewed PR and removed unwanted commit

Summary

Closed, Completed

Understand
Reviewed PR and related code.
Understand
Build
Explained proper fix and added changelog.
Build
Edit
Removed 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:

Read the PR page in full. Include description, all comments, all commits, and all changed files.
Identify any linked issues referenced in the PR body, comments, commit messages, or cross links. Read each issue in full, including all comments.
Analyze the PR diff. Read all relevant code files in full with no truncation from the current main branch and compare against the diff. Do not fetch PR file blobs unless a file is missing on main or the diff context is insufficient. Include related code paths that are not in the diff but are required to validate behavior.
Check for a changelog entry in the relevant 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:
- Entry uses correct section (### Breaking Changes, ### Added, ### Fixed, etc.)
- External contributions include PR link and author: Fixed foo ([#123](https://github.com/badlogic/pi-mono/pull/123) by [@user](https://github.com/user))
- Breaking changes are in ### Breaking Changes, not just ### Fixed
Check if packages/coding-agent/README.md, packages/coding-agent/docs/.md, packages/coding-agent/examples/**/.md require modification. This is usually the case when existing features have been changed, or new features have been added.
Provide a structured review with these sections:
- Good: solid choices or improvements
- Bad: concrete issues, regressions, missing tests, or risks
- Ugly: subtle or high impact problems
Add Questions or Assumptions if anything is unclear.
Add Change summary and Tests.

Output format per PR: PR: Changelog:

... Good:
... Bad:
... Ugly:
... Questions or Assumptions:
... Change summary:
... Tests:
...

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:

Readsession-selector.ts

Readconfig-selector.ts

Readsession-picker.ts

Readconfig-selector.ts

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:

SessionList and ResourceList always invoke onCancel() callback on exit
The TUI continues running and re-renders after that

The issue seems to be that in the standalone selector contexts (not interactive mode), when Ctrl+C is pressed:

In ResourceList.handleInput, Ctrl+C calls onExit?.()
In SessionList, there's no Ctrl+C handler visible in the handleInput, so it relies on onExit

But I don't see Ctrl+C handling in SessionList... Let me check again.

Looking at SessionList.handleInput:

There's no explicit Ctrl+C handler in the session list's handleInput

Wait, looking at the diff, the PR:

Removes onExit callback from both components
Removes the Ctrl+C handler from ResourceList that called onExit

The 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:

Readtui.ts

Now I understand the architecture. Looking at TUI.stop():

typescript

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();}

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:

"The terminal cursor remains hidden after exiting the sessions selector (pi --resume) and the config selector (pi config)."
"This occurs because 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:

User presses Esc -> onCancel() is called -> ui.stop() + resolve(null)
User presses Ctrl+C -> 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:

ReadCHANGELOG.md50 lines

I see there's no changelog entry for this PR. Let me now understand the proper fix. The issue is:

TUI.handleInput() calls requestRender() AFTER the component's handleInput returns
If the component called ui.stop() in its handleInput, a render is still queued
The queued render runs on nextTick and calls hideCursor()

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:

Yes, 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 ...}


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:

No entry exists in packages/coding-agent/CHANGELOG.md or packages/tui/CHANGELOG.md. A changelog entry is required before merge.

Good:

Correctly identifies the symptom (cursor hidden after exiting selectors)
Removes unused onExit parameter, simplifying the API

Bad:

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:

This is a TUI framework bug being patched at the application layer. The fix should be in packages/tui, not scattered across selectors.

The proper fix is a 2-line change in packages/tui/src/tui.ts:

typescript

// 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 ...}

// 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:

Was this tested with Esc? The bug should still occur with Esc after this PR.
Was Ctrl+C in pi config tested? It should now do nothing instead of exiting.

Change summary:

Removes onExit callback from SessionList and ResourceList components
Removes Ctrl+C handler from ResourceList
Simplifies ConfigSelectorComponent and SessionSelectorComponent constructor signatures
Updates test files to match new signatures

Tests:

Test files updated to remove the extra callback argument
No new tests for the cursor visibility fix (because the fix doesn't work)

Documentation:

No changes needed to 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:

Add stopped flag, check in requestRender() - prevents the render from being queued
Add stopped flag, check in doRender() - prevents the render from executing
Process exit handler that shows cursor - but this is a hack and doesn't help if the process continues

Option 2 is the simplest - just add one line at the start of doRender():

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:

typescript

// 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 ...}

// 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.

Readtui.ts100 lines

Readtui.ts30 lines

Readtui.ts40 lines

Readtui.ts20 lines

Editedtui.ts

ah shit, please continue

Editedtui.ts

All good. Now let me add the changelog entry:

EditedCHANGELOG.md

Done. The fix:

packages/tui/src/tui.ts:
- Added private stopped = false; field
- Set this.stopped = false; in start() (allows TUI reuse)
- Set this.stopped = true; in stop()
- Added 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.