Reviewed and summarized GitHub PR #719

Summary

Closed, Completed

Understand
Reviewed linked issue and relevant source files.
Understand
Understand
Checked changelog and documentation for updates.
Understand
Verify
Summarized and commented on PR and issue.
Verify

You are given one or more GitHub PR URLs: https://github.com/badlogic/pi-mono/pull/719

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 linked issue #525 and examine the relevant source files:

Now let me check the changelog and look for any documentation that might need updates:

Let me verify the current state of the changelog on main and check if there are any other related issues:

Now let me check the TUI docs to see if documentation updates might be needed:

PR: https://github.com/badlogic/pi-mono/pull/719

Changelog:

Changelog entries are present in the diff under ## [Unreleased] with ### Added and ### Fixed sections
Missing attribution: PR is from external contributor @can1357. Entries need to include PR link and author per AGENTS.md format: ([#719](https://github.com/badlogic/pi-mono/pull/719) by [@can1357](https://github.com/can1357))
Note: The current main branch's ## [Unreleased] section is empty, so these entries will be new

Good:

Addresses a real user pain point for CJK/IME users (issue #525, #491) where the hardware cursor position didn't follow the fake cursor, causing IME candidate windows to appear in wrong locations
Clean API design: adds optional getCursorPosition() method to Component interface, allowing components to opt-in to hardware cursor positioning without breaking existing components
Correctly uses visibleWidth() for Unicode-aware cursor column calculation in both Input and Editor components
Input queuing during cell size detection (inputQueue) prevents input loss during terminal initialization
Fixes race condition by clearing cellSizeQueryPending before calling setCellDimensions() / invalidate() / requestRender()
Properly hides hardware cursor when autocomplete is active in Editor
Added try/catch around crash log writes (best-effort, non-blocking)
Separates logical cursor tracking (cursorRow for content position) from hardware cursor tracking (hardwareCursorRow for escape sequence positioning)

Bad:

No tests added for the new hardware cursor functionality. TUI cursor tracking is noted as "brittle" in the issue comments - tests would help ensure this doesn't regress
The previousCursor equality check in areCursorsEqual() could use a single comparison if cursors were stored as a string key or similar (minor)
Missing trailing newline in CHANGELOG.md at end of file (removed by this PR)

Ugly:

Container's getCursorPosition() uses cached lastRenderLineCounts but falls back to re-calling child.render(width) if width changed. This coupling between render state and cursor position could lead to subtle bugs if a child component's render output changes between calls without invalidate() being called. The fallback also has performance implications for complex component trees.

Questions or Assumptions:

Does this work correctly with overlays? The getCursorPosition() call happens on the base Container, but overlays are composited on top. If an overlay component (like autocomplete) returns a cursor position, will it be correct relative to the composited output?
The commit message mentions "Fixed cursor visibility when autocomplete is active in editor component" but the Editor's getCursorPosition() returns null when autocompleting. Is this the intended behavior, or should the autocomplete list's cursor position be used?

Change summary:

Adds optional getCursorPosition(width): { row: number; col: number } | null method to Component interface
Implements getCursorPosition() in Input, Editor, and Container components
Editor returns null when autocomplete is active (hides hardware cursor)
TUI tracks hardware cursor position separately from logical cursor position
TUI calls updateHardwareCursor() after each render to position hardware cursor
Adds input queuing during cell size detection to prevent input loss
Fixes race condition by setting cellSizeQueryPending = false before triggering re-renders
Refactors handleInput() to separate input buffering from processing via new processInput()

Tests:

No tests added. Manual testing required:
- IME input in editor and single-line input fields
- Multi-byte Unicode cursor positioning (CJK characters, emoji)
- Cursor position with word-wrapped lines in Editor
- Autocomplete active/inactive cursor visibility transitions
- Input during terminal cell size detection (race condition)
- Suspend/resume cursor state
Existing TUI tests should be run to verify no regressions

Documentation:

packages/coding-agent/docs/tui.md should be updated to document the new optional getCursorPosition?(width: number): { row: number; col: number } | null method in the Component Interface section. This is relevant for extension authors creating custom focusable components.

read all of @packages/tui/src/tui.ts and @packages/tui/src/components/editor.ts (and why do we have @packages/tui/src/editor-component.ts?) we need a deep analysis of how we can fix this issue correctly. i feel this one is a hack. components reporting the cursor position: in what coordinate system? relative to the components first line? could we do this based on what component has focus?

Now let me look at how Editor constructor gets tui and how focus is managed:

Deep Analysis: Hardware Cursor Positioning

Current Architecture Summary

TUI extends Container, tracks:
- focusedComponent - which component receives input
- cursorRow - where the terminal cursor is for differential rendering (NOT the user's text cursor)
- Overlays with their own focus stack
Component interface is minimal:
- render(width): string[]
- handleInput?(data)
- invalidate()
Editor and Input render a "fake cursor" using reverse video (\x1b[7m). The hardware cursor is hidden. This causes IME candidate windows to appear at the wrong location.
EditorComponent in editor-component.ts is an interface for custom editor implementations (vim mode, etc.). The Editor class in editor.ts is the default implementation.

Problems with the PR's Approach

1. Coordinate System Confusion

The PR's getCursorPosition(width) returns { row, col } where:

row is relative to the component's first rendered line (0-indexed)
col is the visual column (using visibleWidth())

But Container's implementation walks ALL children, accumulates line counts, and returns absolute coordinates. This mixing of relative/absolute is confusing and error-prone.

2. Every Component Must Participate

Container has to implement getCursorPosition() by iterating children:

typescript

getCursorPosition(width: number): { row: number; col: number } | null {    let rowOffset = 0;    for (const child of this.children) {        const childCursor = child.getCursorPosition?.(width);        if (childCursor) {            return { row: rowOffset + childCursor.row, col: childCursor.col };        }        rowOffset += child.render(width).length; // Re-renders child!    }    return null;}

getCursorPosition(width: number): { row: number; col: number } | null {    let rowOffset = 0;    for (const child of this.children) {        const childCursor = child.getCursorPosition?.(width);        if (childCursor) {            return { row: rowOffset + childCursor.row, col: childCursor.col };        }        rowOffset += child.render(width).length; // Re-renders child!    }    return null;}

This re-renders children to count lines (the PR tries to cache this, but it's fragile).

3. Ignores Focus

The TUI already knows which component has focus via focusedComponent. But the PR's approach walks all children looking for any cursor. This is wasteful and semantically wrong: only the focused component should report a cursor.

4. Doesn't Handle Overlays

Overlays are composited at specific (row, col) positions. If a focused overlay component reports cursor position (2, 5), that's relative to the overlay's content, not the final composited output. The PR doesn't account for overlay positioning.

5. Cache Staleness

The Container caches lastRenderLineCounts but this can get out of sync if a child's render output changes between render() and getCursorPosition() calls.

Better Approach: Focus-Based Position Tracking

The key insight: TUI already tracks focus. It just needs to also track where the focused component appears in the rendered output.

Option A: Track Focused Component Position During Render

typescript

class TUI {    private focusedComponentStartRow: number = -1;        private doRender(): void {        // During render, track where focused component starts        let currentRow = 0;        const focusedLines: string[] = [];                for (const child of this.children) {            if (child === this.focusedComponent) {                this.focusedComponentStartRow = currentRow;            }            const lines = child.render(width);            currentRow += lines.length;        }                // After render, position hardware cursor        if (this.focusedComponent?.getCursorPosition) {            const pos = this.focusedComponent.getCursorPosition(width);            if (pos) {                const absoluteRow = this.focusedComponentStartRow + pos.row;                this.positionHardwareCursor(absoluteRow, pos.col);            }        }    }}

class TUI {    private focusedComponentStartRow: number = -1;        private doRender(): void {        // During render, track where focused component starts        let currentRow = 0;        const focusedLines: string[] = [];                for (const child of this.children) {            if (child === this.focusedComponent) {                this.focusedComponentStartRow = currentRow;            }            const lines = child.render(width);            currentRow += lines.length;        }                // After render, position hardware cursor        if (this.focusedComponent?.getCursorPosition) {            const pos = this.focusedComponent.getCursorPosition(width);            if (pos) {                const absoluteRow = this.focusedComponentStartRow + pos.row;                this.positionHardwareCursor(absoluteRow, pos.col);            }        }    }}

For overlays: the overlay's (row, col) position is already known from resolveOverlayLayout(). Add that offset.

Pros:

Only focused component needs getCursorPosition()
No Container implementation needed
Position is tracked naturally during render
Clear coordinate system: component returns relative coords, TUI adds offset

Cons:

Need to track focused component's position through the component tree
Nested containers require recursive tracking

Option B: Cursor Marker in Rendered Output

Components emit a special marker where the cursor should be:

typescript

// Special marker that won't render visuallyconst CURSOR_MARKER = "\x1b[?cursor;r;c\x1b[0m"; // r=row, c=col within component
// In Editor.render():const cursorMarker = `\x1b[?cursor;${layoutLineIndex};${visualCol}\x1b[0m`;

// Special marker that won't render visuallyconst CURSOR_MARKER = "\x1b[?cursor;r;c\x1b[0m"; // r=row, c=col within component
// In Editor.render():const cursorMarker = `\x1b[?cursor;${layoutLineIndex};${visualCol}\x1b[0m`;

TUI scans rendered output for marker, extracts position, strips marker, positions hardware cursor.

Pros:

Works automatically with any nesting
No interface changes to Component
Coordinates are naturally in final rendered space
Works with overlays automatically (marker appears in composited output)

Cons:

Scanning overhead (though minimal)
Marker must be stripped before actual terminal output
More "magical" behavior

Option C: Separate Cursor Reporting from Component Interface

Instead of adding to Component interface, create a separate CursorReporter interface:

typescript

interface CursorReporter {    getCursorPosition(width: number): { row: number; col: number } | null;}
// Editor implements both Component and CursorReporterclass Editor implements Component, CursorReporter { ... }

interface CursorReporter {    getCursorPosition(width: number): { row: number; col: number } | null;}
// Editor implements both Component and CursorReporterclass Editor implements Component, CursorReporter { ... }

TUI checks if focusedComponent is also a CursorReporter:

typescript

if (isCursorReporter(this.focusedComponent)) {    const pos = this.focusedComponent.getCursorPosition(width);    // ...}

if (isCursorReporter(this.focusedComponent)) {    const pos = this.focusedComponent.getCursorPosition(width);    // ...}

Pros:

Explicit opt-in
Clean separation of concerns
Container doesn't need to implement anything

Cons:

Still need to track focused component's position in output

Recommendation

Option A (Focus-Based Tracking) is the cleanest:

Only the focused component implements getCursorPosition() returning relative coordinates (row relative to its first line, col as visual width)
TUI tracks where the focused component starts during render
For overlays, add the overlay's position offset
Container does NOT implement getCursorPosition() - it's not needed because TUI tracks the focused component directly
The coordinate system is explicit:
- Components return { row, col } relative to their own output
- TUI adds the component's start row to get absolute position

This avoids the PR's issues:

No walking all children
No cache staleness
Clear coordinate system
Works with overlays
Only focused component participates

TUI tracks where the focused component starts during render how?

You're right to push on this - it's the crux of the problem. Let me think through the options:

The Core Challenge

TUI calls Container.render() which just concatenates child outputs:

typescript

class Container {    render(width: number): string[] {        const lines: string[] = [];        for (const child of this.children) {            lines.push(...child.render(width));  // No tracking of which child produced which lines        }        return lines;    }}

class Container {    render(width: number): string[] {        const lines: string[] = [];        for (const child of this.children) {            lines.push(...child.render(width));  // No tracking of which child produced which lines        }        return lines;    }}

The focused component could be:

A direct child of TUI
Nested inside a Container child
Inside an overlay (which has explicit positioning)

Option A Problems

For direct children, TUI could override render() to track positions. But for nested components, we'd need to either:

Walk the tree to find the path to focused component, then track line counts along that path
Pass down a "row offset" parameter through all renders (invasive interface change)
Have every Container maintain a childPositions map (overhead for all containers)

All of these are complex and fragile.

Why Option B (Marker) Is Actually Cleaner

The marker approach sidesteps the tracking problem entirely:

typescript

// Only the focused component emits this markerconst CURSOR_MARKER = "\x1b]8;;cursor\x07"; // Using OSC 8 (hyperlink) with special ID                                            // Or any zero-width escape sequence
class Editor {    render(width: number): string[] {        // ... layout text ...                // Emit marker at cursor position (only if we have focus)        if (this.hasFocus) {            // Insert marker right before the fake cursor            line = before + CURSOR_MARKER + fakeCursor + after;        }                return lines;    }}

// Only the focused component emits this markerconst CURSOR_MARKER = "\x1b]8;;cursor\x07"; // Using OSC 8 (hyperlink) with special ID                                            // Or any zero-width escape sequence
class Editor {    render(width: number): string[] {        // ... layout text ...                // Emit marker at cursor position (only if we have focus)        if (this.hasFocus) {            // Insert marker right before the fake cursor            line = before + CURSOR_MARKER + fakeCursor + after;        }                return lines;    }}

Then in TUI:

typescript

class TUI {    doRender(): void {        let newLines = this.render(width);        newLines = this.compositeOverlays(newLines, width, height);                // Find cursor marker in final output        const cursorPos = this.findAndStripCursorMarker(newLines);                newLines = this.applyLineResets(newLines);                // ... differential rendering ...                // Position hardware cursor at marker location        if (cursorPos) {            this.positionHardwareCursor(cursorPos.row, cursorPos.col);        }    }        private findAndStripCursorMarker(lines: string[]): { row: number; col: number } | null {        for (let row = 0; row < lines.length; row++) {            const col = this.findMarkerColumn(lines[row]);            if (col !== -1) {                lines[row] = lines[row].replace(CURSOR_MARKER, '');                return { row, col };            }        }        return null;    }}

class TUI {    doRender(): void {        let newLines = this.render(width);        newLines = this.compositeOverlays(newLines, width, height);                // Find cursor marker in final output        const cursorPos = this.findAndStripCursorMarker(newLines);                newLines = this.applyLineResets(newLines);                // ... differential rendering ...                // Position hardware cursor at marker location        if (cursorPos) {            this.positionHardwareCursor(cursorPos.row, cursorPos.col);        }    }        private findAndStripCursorMarker(lines: string[]): { row: number; col: number } | null {        for (let row = 0; row < lines.length; row++) {            const col = this.findMarkerColumn(lines[row]);            if (col !== -1) {                lines[row] = lines[row].replace(CURSOR_MARKER, '');                return { row, col };            }        }        return null;    }}

Why this works:

The marker flows through all the Container.render() calls naturally
After overlay compositing, it appears at the correct absolute position
Only one marker exists (from focused component)
TUI doesn't need to know the component tree structure
No interface changes to Container

The focused component needs to know it has focus. Currently Editor already has access to this.tui, so it could check this.tui.focusedComponent === this or TUI could set a flag.

Remaining Question

How does the focused component know it has focus?

Options:

Component queries TUI: this.tui.hasFocus(this) (requires reference to TUI)
TUI sets a flag on focus change: component.focused = true
Focus is passed to render: render(width, focused) (interface change)
Separate render call for focused component with flag

Editor already has this.tui reference in its constructor, so option 1 works for Editor. For other components (Input, etc.), they'd need the same pattern or option 2.

What do you think - is the marker approach worth exploring, or do you see issues with it?

i think the marker approach is good, if we can find a marker that's guaranteed unique, does not mess with rendering, etc.

i think we can introduce a Focusable interface that components that need focused flag to check if they need to emit the marker should implement.

makes sense?

Yes, makes sense. Let me draft the design:

Marker Choice

Looking for something that:

Is zero-width visually
Terminals will ignore (not render anything)
Won't conflict with existing escape sequences
Easy to detect and strip

APC (Application Program Command) is ideal: \x1b_...\x1b\\

It's explicitly defined for application-specific use. Terminals should ignore unknown APC sequences. The TUI already handles \x1b_G for Kitty graphics, so we just need a distinct payload.

typescript

// Cursor position marker - APC with unique payloadconst CURSOR_MARKER = "\x1b_pi:cursor\x1b\\";

// Cursor position marker - APC with unique payloadconst CURSOR_MARKER = "\x1b_pi:cursor\x1b\\";

Or even shorter with BEL terminator (also valid):

Focusable Interface

typescript

/** * Interface for components that can receive focus and display a cursor. * When focused, component should emit CURSOR_MARKER at the cursor position. */export interface Focusable {    /** Set by TUI when focus changes */    focused: boolean;}
/** Type guard */export function isFocusable(component: Component): component is Component & Focusable {    return 'focused' in component;}

/** * Interface for components that can receive focus and display a cursor. * When focused, component should emit CURSOR_MARKER at the cursor position. */export interface Focusable {    /** Set by TUI when focus changes */    focused: boolean;}
/** Type guard */export function isFocusable(component: Component): component is Component & Focusable {    return 'focused' in component;}

TUI Changes

typescript

class TUI extends Container {    setFocus(component: Component | null): void {        // Clear old focus        if (this.focusedComponent && isFocusable(this.focusedComponent)) {            this.focusedComponent.focused = false;        }                this.focusedComponent = component;                // Set new focus        if (component && isFocusable(component)) {            component.focused = true;        }    }        private doRender(): void {        let newLines = this.render(width);        newLines = this.compositeOverlays(newLines, width, height);                // Find and strip cursor marker, get position        const cursorPos = this.extractCursorPosition(newLines);                newLines = this.applyLineResets(newLines);                // ... existing differential rendering ...                // Position hardware cursor        if (cursorPos) {            this.positionHardwareCursor(cursorPos.row, cursorPos.col);            this.terminal.showCursor();        } else {            this.terminal.hideCursor();        }    }        private extractCursorPosition(lines: string[]): { row: number; col: number } | null {        const marker = "\x1b_pi:c\x07";        for (let row = 0; row < lines.length; row++) {            const markerIndex = lines[row].indexOf(marker);            if (markerIndex !== -1) {                // Calculate visual column (width of text before marker)                const beforeMarker = lines[row].slice(0, markerIndex);                const col = visibleWidth(beforeMarker);                                // Strip marker                lines[row] = lines[row].replace(marker, '');                                return { row, col };            }        }        return null;    }}

class TUI extends Container {    setFocus(component: Component | null): void {        // Clear old focus        if (this.focusedComponent && isFocusable(this.focusedComponent)) {            this.focusedComponent.focused = false;        }                this.focusedComponent = component;                // Set new focus        if (component && isFocusable(component)) {            component.focused = true;        }    }        private doRender(): void {        let newLines = this.render(width);        newLines = this.compositeOverlays(newLines, width, height);                // Find and strip cursor marker, get position        const cursorPos = this.extractCursorPosition(newLines);                newLines = this.applyLineResets(newLines);                // ... existing differential rendering ...                // Position hardware cursor        if (cursorPos) {            this.positionHardwareCursor(cursorPos.row, cursorPos.col);            this.terminal.showCursor();        } else {            this.terminal.hideCursor();        }    }        private extractCursorPosition(lines: string[]): { row: number; col: number } | null {        const marker = "\x1b_pi:c\x07";        for (let row = 0; row < lines.length; row++) {            const markerIndex = lines[row].indexOf(marker);            if (markerIndex !== -1) {                // Calculate visual column (width of text before marker)                const beforeMarker = lines[row].slice(0, markerIndex);                const col = visibleWidth(beforeMarker);                                // Strip marker                lines[row] = lines[row].replace(marker, '');                                return { row, col };            }        }        return null;    }}

Editor/Input Changes

typescript

class Editor implements Component, Focusable {    focused: boolean = false;        render(width: number): string[] {        // ... existing layout code ...                // When building the line with cursor:        if (layoutLine.hasCursor && this.focused) {            // Emit marker right before the fake cursor character            const marker = "\x1b_pi:c\x07";            displayText = before + marker + fakeCursor + after;        } else {            displayText = before + fakeCursor + after;        }                // ...    }}

class Editor implements Component, Focusable {    focused: boolean = false;        render(width: number): string[] {        // ... existing layout code ...                // When building the line with cursor:        if (layoutLine.hasCursor && this.focused) {            // Emit marker right before the fake cursor character            const marker = "\x1b_pi:c\x07";            displayText = before + marker + fakeCursor + after;        } else {            displayText = before + fakeCursor + after;        }                // ...    }}

Summary

Marker: \x1b_pi:c\x07 (APC sequence, zero-width, ignored by terminals)
Focusable interface: Components opt-in, TUI manages focused flag
TUI scans output: After render + compositing, find marker, extract position, strip marker
Position hardware cursor: At the absolute position where marker was found

This keeps the Component interface unchanged, only components that need cursor tracking implement Focusable, and the marker naturally flows through any nesting/compositing.

Want me to look at what it would take to implement this properly as an alternative to the PR?

next question: for differential rendering, we set the cursor to the first line that changed within the visible terminal viewport, does this cursor positioning mess with that somehow?

Great question. Yes, there's a potential conflict. Let me trace through:

Current Differential Rendering

typescript

private doRender(): void {    // ... find firstChanged, lastChanged ...        // Move cursor to first changed line (relative to current position)    const lineDiff = firstChanged - this.cursorRow;    if (lineDiff > 0) buffer += `\x1b[${lineDiff}B`;    else if (lineDiff < 0) buffer += `\x1b[${-lineDiff}A`;    buffer += "\r"; // Column 0        // Write changed lines...        // Track where cursor ended up    this.cursorRow = finalCursorRow;}

private doRender(): void {    // ... find firstChanged, lastChanged ...        // Move cursor to first changed line (relative to current position)    const lineDiff = firstChanged - this.cursorRow;    if (lineDiff > 0) buffer += `\x1b[${lineDiff}B`;    else if (lineDiff < 0) buffer += `\x1b[${-lineDiff}A`;    buffer += "\r"; // Column 0        // Write changed lines...        // Track where cursor ended up    this.cursorRow = finalCursorRow;}

cursorRow tracks where the terminal cursor is so the next render can calculate relative movement.

The Problem

If after differential rendering we move the cursor to the IME position:

typescript

// After differential rendering leaves cursor at row 50this.cursorRow = 50;
// Then we move cursor to IME position at row 10this.positionHardwareCursor(10, 15);

// After differential rendering leaves cursor at row 50this.cursorRow = 50;
// Then we move cursor to IME position at row 10this.positionHardwareCursor(10, 15);

Next render, we think cursor is at row 50, but it's actually at row 10. Movement calculation is wrong.

The Solution: Track Both

We need two row trackers:

typescript

class TUI {    private cursorRow = 0;         // Logical: where content rendering ends    private hardwareCursorRow = 0; // Actual: where terminal cursor physically is}

class TUI {    private cursorRow = 0;         // Logical: where content rendering ends    private hardwareCursorRow = 0; // Actual: where terminal cursor physically is}

Movement calculations use hardwareCursorRow:

typescript

private doRender(): void {    // Movement is relative to actual cursor position    const lineDiff = firstChanged - this.hardwareCursorRow;        // ... write lines ...        // After writing, cursor is at end of last written line    this.cursorRow = finalCursorRow;    this.hardwareCursorRow = finalCursorRow;        // Now position for IME (updates hardwareCursorRow)    if (cursorPos) {        this.positionHardwareCursor(cursorPos.row, cursorPos.col);        this.hardwareCursorRow = cursorPos.row;        this.terminal.showCursor();    }}

private doRender(): void {    // Movement is relative to actual cursor position    const lineDiff = firstChanged - this.hardwareCursorRow;        // ... write lines ...        // After writing, cursor is at end of last written line    this.cursorRow = finalCursorRow;    this.hardwareCursorRow = finalCursorRow;        // Now position for IME (updates hardwareCursorRow)    if (cursorPos) {        this.positionHardwareCursor(cursorPos.row, cursorPos.col);        this.hardwareCursorRow = cursorPos.row;        this.terminal.showCursor();    }}

The PR actually handles this correctly with separate cursorRow and hardwareCursorRow tracking. So the marker approach would need the same:

typescript

private extractCursorPosition(lines: string[]): { row: number; col: number } | null {    // ... find marker, calculate position ...}
private doRender(): void {    // ... existing differential rendering using hardwareCursorRow for movement ...        this.cursorRow = finalCursorRow;    this.hardwareCursorRow = finalCursorRow;        // Position for IME    const cursorPos = /* extracted earlier */;    if (cursorPos) {        const rowDelta = cursorPos.row - this.hardwareCursorRow;        if (rowDelta > 0) this.terminal.write(`\x1b[${rowDelta}B`);        else if (rowDelta < 0) this.terminal.write(`\x1b[${-rowDelta}A`);        this.terminal.write(`\x1b[${cursorPos.col + 1}G`); // Absolute column        this.hardwareCursorRow = cursorPos.row;        this.terminal.showCursor();    } else {        this.terminal.hideCursor();    }}

private extractCursorPosition(lines: string[]): { row: number; col: number } | null {    // ... find marker, calculate position ...}
private doRender(): void {    // ... existing differential rendering using hardwareCursorRow for movement ...        this.cursorRow = finalCursorRow;    this.hardwareCursorRow = finalCursorRow;        // Position for IME    const cursorPos = /* extracted earlier */;    if (cursorPos) {        const rowDelta = cursorPos.row - this.hardwareCursorRow;        if (rowDelta > 0) this.terminal.write(`\x1b[${rowDelta}B`);        else if (rowDelta < 0) this.terminal.write(`\x1b[${-rowDelta}A`);        this.terminal.write(`\x1b[${cursorPos.col + 1}G`); // Absolute column        this.hardwareCursorRow = cursorPos.row;        this.terminal.showCursor();    } else {        this.terminal.hideCursor();    }}

So yes, there's interaction, but it's manageable with dual tracking. The key insight: column doesn't matter for differential rendering (it always does \r to go to column 0), only row matters.

ok, create a feature branch and implement this. i beleive @packages/tui/src/components/input.ts will also need this treatment then

Now let me implement the changes. First, I'll update tui.ts:

Now I need to update the rest of doRender to use hardwareCursorRow for movement calculations and position the hardware cursor at the end. Let me read the current state and make the updates:

Now I need to replace the doRender method to use hardwareCursorRow for movement and position the hardware cursor. This is a larger change:

Now let me update the Editor class to implement Focusable and emit the cursor marker:

I need to update the cursor rendering section to emit CURSOR_MARKER when focused and not autocompleting:

Now let me update the Input component similarly:

Now let's run the type checker to make sure everything compiles:

Let me check what's exported from the tui package and add the new exports:

Now let me add the changelog entry:

Now let me update the TUI documentation to document the Focusable interface:

Let me run check again to make sure everything is still good:

All checks pass. Let me provide a summary of the implementation:

Summary of Changes

packages/tui/src/tui.ts

Added Focusable interface with focused: boolean property
Added isFocusable() type guard
Added CURSOR_MARKER constant (APC escape sequence \x1b_pi:c\x07)
Added hardwareCursorRow tracking separate from cursorRow
Updated setFocus() to manage the focused flag on components
Added extractCursorPosition() to find and strip cursor marker from rendered output
Added positionHardwareCursor() to move hardware cursor for IME
Updated doRender() to extract cursor position and position hardware cursor after rendering
Updated stop() and requestRender() to use hardwareCursorRow

packages/tui/src/components/editor.ts

Implemented Focusable interface
Added focused: boolean = false property
Updated render to emit CURSOR_MARKER when focused and not showing autocomplete

packages/tui/src/components/input.ts

Implemented Focusable interface
Added focused: boolean = false property
Updated render to emit CURSOR_MARKER when focused

packages/tui/src/index.ts

Exported CURSOR_MARKER, Focusable, and isFocusable

packages/tui/CHANGELOG.md

Added changelog entries for the new feature

packages/coding-agent/docs/tui.md

Added documentation for the Focusable interface

The implementation uses a marker-based approach where:

Focused components emit CURSOR_MARKER at the cursor position in their render output
TUI scans for this marker after rendering (including overlay compositing)
TUI strips the marker and positions the hardware cursor at that absolute position
This enables proper IME candidate window positioning for CJK input

i'm on a german language keyboard, how can i test ime on macos?

To test IME on macOS:

Add a CJK input source:
- System Settings → Keyboard → Input Sources → Edit...
- Click + at bottom left
- Add one of these (easiest to test):
  - Japanese - Romaji (type "nihongo" → shows candidates like 日本語)
  - Chinese, Simplified - Pinyin (type "nihao" → shows candidates like 你好)
  - Korean - 2-Set Korean
Switch input sources:
- Press Ctrl+Space or Fn+E to cycle through input sources
- Or click the input menu in the menu bar (flag icon)
Test IME:
- Switch to Japanese Romaji
- Start typing in the editor, e.g., type "nihongo"
- A candidate window should appear near the cursor showing options like 日本語, にほんご, etc.
- Press Space to cycle candidates, Enter to select

What to look for:

Before fix: The IME candidate window appears at the bottom-right corner of the terminal (where the hidden hardware cursor was)
After fix: The IME candidate window should appear directly at/near the text cursor position in the Editor/Input component

Quick test:

bash

cd /Users/badlogic/workspaces/pi-mono/packages/coding-agentnpm run dev# Switch to Japanese input (Ctrl+Space)# Type in the editor

cd /Users/badlogic/workspaces/pi-mono/packages/coding-agentnpm run dev# Switch to Japanese input (Ctrl+Space)# Type in the editor

Japanese Romaji is the easiest to test since you just type Latin characters and it shows Japanese candidates.

ok, looking good, works great in editor, but not so great in @packages/coding-agent/examples/extensions/overlay-test.ts any idea?

The issue is that OverlayTestComponent doesn't implement Focusable and doesn't emit CURSOR_MARKER. Let me update the example:

isn't it using Input?

You're right, it's not using the Input component - it has its own inline cursor rendering. Let me update the render method to emit the marker:

Now it should work. The example had its own inline cursor rendering, so it needed to:

Implement Focusable interface
Emit CURSOR_MARKER before the fake cursor when this.focused is true

This is a good example to show in the documentation - any custom component with inline text input needs to do this for IME support. Try it now with:

╭────────────────────────────────────────────────────────────────────╮ Loaded context: │ 🧪 Overlay Test │ /Users/badlogic/│ │ /Users/badlogic/│ ─── Edge Cases (borders should align) ─── │ │ Wide: 中文日本語한글テスト漢字繁體简体ひらがなカタカナ가나다라마바 │ Loaded skills: │ Styled: RED GREEN YELLOW ACCENT DIM more colors │ /Users/badlogic/│ Emoji: 👨‍👩‍👧‍👦 🇯🇵 🚀 💻 🎉 🔥 😀 🎯 🌟 💡 🎨 🔧 📦 🏆 🌈 🎪 🎭 🎬 🎮 🎲 │ /Users/badlogic/│ │ /Users/badlogic/│ ─── Actions ─── │ /Users/badlogic/│ ▶ Search: ehy │ /Users/badlogic/│ Run: │ /Users/badlogic/│ Settings │ /Users/badlogic/│ Cancel │ /Users/badlogic/│ │ /Users/badlogic/│ ↑↓ navigate • type to input • Enter select • Esc cancel │ ╰────────────────────────────────────────────────────────────────────╯

hm, the border is now broken, see border marker position to the left of ehy

The issue is that visibleWidth() probably doesn't strip APC sequences, so the marker is affecting the width calculation. Let me check:

Found it! visibleWidth() doesn't strip APC sequences. I need to add that:

The issue was visibleWidth() wasn't stripping APC sequences. Now it handles \x1b_...\x07 and \x1b_...\x1b\\ patterns. Try it again - the borders should align now.

hm, now that right border is completely gone

The compositing functions (sliceByColumn, sliceWithWidth) might also need to handle APC sequences. Let me check:

Found it! extractAnsiCode doesn't handle APC sequences (ESC _). Let me add that:

Now both visibleWidth() and extractAnsiCode() handle APC sequences. The slice/extract functions use extractAnsiCode internally, so they should now properly preserve the cursor marker as a zero-width sequence. Try again!

looking good now!

Excellent! Let me update the changelog to mention the visibleWidth/extractAnsiCode fix and then summarize the complete implementation:

All checks pass. Here's a summary of the complete implementation:

Summary

Core Implementation (packages/tui/src/tui.ts)

Focusable interface - Components implement focused: boolean to receive focus state
CURSOR_MARKER - APC escape sequence \x1b_pi:c\x07 (zero-width)
isFocusable() type guard - Check if component implements Focusable
Dual cursor tracking - cursorRow (logical) vs hardwareCursorRow (actual terminal position)
setFocus() - Manages focused flag on Focusable components
extractCursorPosition() - Finds marker in rendered output, returns position, strips marker
positionHardwareCursor() - Moves terminal cursor after differential rendering

Components

Editor - Implements Focusable, emits marker when focused and not autocompleting
Input - Implements Focusable, emits marker when focused
overlay-test.ts - Updated example showing how custom components use the interface

Utilities (packages/tui/src/utils.ts)

visibleWidth() - Now strips APC sequences from width calculation
extractAnsiCode() - Now recognizes APC sequences, so sliceWithWidth() and extractSegments() preserve them correctly

Exports (packages/tui/src/index.ts)

Added CURSOR_MARKER, Focusable, isFocusable

Documentation

docs/tui.md - Added "Focusable Interface (IME Support)" section

How it works

Focused component emits CURSOR_MARKER at cursor position in render output
Marker flows through Container rendering and overlay compositing naturally
TUI scans final output for marker, extracts absolute position, strips marker
After differential rendering, TUI positions hardware cursor at marker location
IME candidate windows appear at correct position

Would you like me to commit these changes?

ok commit merge into main. changelog.md and all docs are up to date?

Loading more messages…