Reviewed cursor visibility and IME positioning in PR

Summary

In Progress

Understand
Reviewed PR details and related issues.
Understand
Understand
Confirmed cursor visibility behavior on shutdown.
Understand
Verify
Noted push failure due to unstaged changes.
Verify

You are given one or more GitHub PR URLs: https://github.com/badlogic/pi-mono/pull/800 why does the current code break ime positioning? cant we hide the cursor while still set the cursor position for ime?

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.

Analyzing IME cursor breakage

Current code disables hardware cursor movement when hardware cursor use is off, causing the IME to rely on an outdated cursor position, which breaks IME positioning since the cursor isn't moved though it might be hidden.

Considering cursor hide-with-move and docs check

Hiding the cursor while still moving it may require toggling hide after moving since the code doesn't support moving without showing; IME candidates likely depend on cursor visibility or position. I'll also check the coding-agent README and docs for any needed updates due to the new hardware cursor setting.

Reviewing hardwareCursor setting and IME handling

I'm noting the README likely needs updates to include the new hardwareCursor setting and related env var. The current code hides the cursor unconditionally when hardwareCursor is false, which may conflict with IME use cases that require moving the cursor while it remains hidden. This suggests a need to allow hiding the cursor without disabling cursor positioning for better IME compatibility.

Reviewing hardwareCursor addition and IME cursor behavior

The PR adds a hardwareCursor setting with runtime toggle and updates changelogs in coding-agent and tui, but both lack proper PR attribution and author metadata. The README settings table omits hardwareCursor and its env var PI_HARDWARE_CURSOR, so it needs updating to clarify their relationship. The current TUI implementation disables cursor positioning when hardwareCursor is false by hiding the cursor and returning early, which breaks IME candidate window tracking since IME relies on cursor position updates. While it is technically possible to move a hidden cursor, the existing code always shows the cursor after moving, preventing hidden positioning. This suggests a need to separate cursor visibility from positioning to support IME scenarios. The PR adds UI toggle support aligned with the new setting. No tests were added or run, and no related issues exist. Questions remain about terminal compatibility with positioning hidden cursors and whether a "position-only" mode should be introduced.

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

packages/coding-agent/CHANGELOG.md: Entry added under Added, but it is missing the required PR link and author attribution.
packages/tui/CHANGELOG.md: Entry added under Added, but it is missing the required PR link and author attribution.
A changelog entry is present for both packages, but needs formatting fixes before merge. Good:
Adds per-instance hardware cursor configuration and a runtime toggle, which makes the IME fix usable without restarting.
Wires the setting through /settings so the toggle is discoverable and immediate. Bad:
IME positioning breaks when hardware cursor is disabled because positionHardwareCursor short-circuits and never updates the terminal cursor position. The IME candidate window will then anchor to a stale position. Hiding the cursor alone does not keep positioning updated because the cursor movement logic is skipped.
The README settings table and settings.json example in packages/coding-agent/README.md do not mention hardwareCursor. That needs documentation if this is a supported setting.
Changelog entries are missing the required PR attribution format. Ugly:
TUI always calls showCursor when hardware cursor is enabled. There is no option to keep the cursor hidden while still updating the position. If some terminals require the cursor to remain hidden to avoid artifacts, this change forces visibility and may regress those terminals. It is the same reason the hardware cursor was disabled by default. Questions or Assumptions:
Do you want a third mode where the cursor position is updated but the cursor remains hidden, or should visibility remain tied to hardware cursor positioning?
Are there terminals where IME placement works when the cursor is hidden, or do we need visibility to keep IME working? Change summary:
Adds hardwareCursor setting in coding-agent, wires it to /settings, and passes the flag into TUI.
Adds per-instance hardware cursor configuration and a runtime toggle in pi-tui. Tests:
Not run. Not requested.

i think we don't need a setting. we need to differntiate between cursor positioning (always on!) and hardware cursor visibility (toggle via ENV var). can you please check how we can always position the cursor, but make visibility toggleable?

Proposing cursor positioning refactor

I'm planning to separate hardware cursor positioning from visibility by removing the early return in positioning, always updating cursor position if available, and controlling show/hide via a dedicated flag—potentially an env var—to improve semantics and flexibility. I'll suggest updating README to document the new environment variable controlling cursor visibility.

Detailing cursor positioning mechanism

I'm outlining how to always position the hardware cursor in the rendering flow by modifying the flag logic in the main TUI file to separate movement from visibility. The plan clarifies handling cursor presence, overlay interactions, and naming updates for cursor visibility control while preserving current overlay hide/show calls.

Yes. The current behavior in packages/tui/src/tui.ts short-circuits cursor positioning when useHardwareCursor is false. That means the cursor is never moved, so IME candidate windows anchor to a stale position.

To always position the cursor while making visibility toggleable, split the concerns:

Always compute and move the cursor if a cursor marker was found.
Only show or hide the cursor based on the env var.

Concrete approach in TUI:

Keep a flag for visibility, for example showHardwareCursor = process.env.PI_HARDWARE_CURSOR === "1" or rename the existing useHardwareCursor to showHardwareCursor.
In positionHardwareCursor, remove the early return that skips movement when the flag is false.
Move the cursor if cursorPos is available and totalLines > 0, regardless of visibility.
After moving, call showCursor() only if visibility is enabled. Otherwise call hideCursor().
Still hide the cursor if there is no cursorPos or totalLines is 0, but do not return before movement is attempted when cursorPos exists.

Pseudo-change inside positionHardwareCursor:

if (!cursorPos || totalLines <= 0) {  this.terminal.hideCursor();  return;}
move cursor to target row and colthis.hardwareCursorRow = targetRow
if (this.showHardwareCursor) this.terminal.showCursor()else this.terminal.hideCursor()

if (!cursorPos || totalLines <= 0) {  this.terminal.hideCursor();  return;}
move cursor to target row and colthis.hardwareCursorRow = targetRow
if (this.showHardwareCursor) this.terminal.showCursor()else this.terminal.hideCursor()

That keeps IME positioning stable while allowing visibility control.

If we remove the new coding-agent setting, we should also remove:

SettingsManager additions and /settings UI toggle
TUI constructor arg and setUseHardwareCursor method
The changelog entries referencing the setting

We would keep PI_HARDWARE_CURSOR as visibility only. If you want me to implement these changes, say so.

well, we haven't merged the pr yet, so i think we can just make this change in main. we can still. so

rename to showHardwareCursor, exposes as getter/setter
adjust positionHardwareCursor to always position, but conditionally show/hide cursor
add showHardwareCursor to SettingsManager and expose in Settiongs UI in interactive mode, like it's done in the PR

agree?

Planning cursor rename and behavior changes

I'm renaming useHardwareCursor to showHardwareCursor with getter/setter, adjusting constructor and visibility accordingly. The hardware cursor will always position if a cursorPos exists but hide if showHardwareCursor is false, still moving the cursor even when hidden. I'll update changelog entries in packages/tui and coding-agent under Unreleased.

Detailing hardware cursor setting integration

I'm updating TUI to rename and expose showHardwareCursor with getter/setter, adjusting constructor and cursor positioning logic to always move but conditionally show or hide the cursor. I'm adding showHardwareCursor to settings-manager and selector with UI integration and updating interactive-mode to apply it. I’ll also update the coding-agent README to document the new setting and add changelog entries reflecting these changes.

Editedtui.ts

Editedsettings-manager.ts

Editedsettings-selector.ts

Investigating indentation inconsistencies

I'm checking if tabs or spaces indent the target code block, considering potential CRLF issues, and exploring precise edits by matching existing indentation via exact line strings from reading, aiming to replace the case block without mismatches.

Preparing precise edit with tabs

I'm planning to insert the new case before the existing one by matching the exact line with four tabs indentation, ensuring the edit uses the correct tab characters for accurate replacement.

Editedsettings-selector.ts

Editedinteractive-mode.ts

Updating cursor setting integration

I'm adding the showHardwareCursor property to the settings JSON and interface, updating its usage in the tui code, and documenting it in the settings table and JSON example in the README while referencing the existing env var note.

Locating JSON snippet in README

I'm searching for the editorPaddingX setting example in the README to add the showHardwareCursor entry nearby in the JSON settings example.

EditedREADME.md

Verifying full changelog read

I realized the changelog read was likely truncated at 50KB, which risks missing existing entries; since editing requires full context, I need to reread the entire changelog file without limits before updating it.

Planning segmented changelog reads

Since the changelog exceeds 50KB and the read tool truncates, I'll read it in multiple segments using offsets and limits to ensure full coverage, balancing the requirement to read entire files with the tool's constraints.

EditedCHANGELOG.md

Finalizing cursor setting updates

The cursor positioning now moves even when showing the hardware cursor is disabled, with initial hiding handled accordingly; relevant settings selectors and interface comments are updated without introducing em dashes, and no further code or documentation changes appear necessary.

Completing cursor feature integration

The new showHardwareCursor setting is integrated into the interactive settings UI and TUI actions, with cursor hiding and positioning behavior adjusted accordingly; no type errors or docs updates needed, and no tests have been run.

Summary:

Split hardware cursor visibility from positioning by adding showHardwareCursor state in TUI and always positioning the cursor for IME.
Added showHardwareCursor setting in coding-agent SettingsManager and exposed it in /settings, plus wired it into TUI construction and runtime updates.
Documented showHardwareCursor in packages/coding-agent/README.md and added changelog entries for coding-agent and tui.

Tests not run.

oki, looks great, changelog needs citation of pr and contributor, then commit and push JUST the files you modified

Clarifying changelog attribution

I'm assessing whether to include a PR link or contributor in the changelog entry but see the change is a direct internal commit without a PR or issue; I'll likely omit the PR link since none applies and follow internal changelog rules accordingly.

Including PR link and author in changelog

I'll update both changelog entries to cite PR 800 and contributor @ghoulr per user request, then commit and push only the modified files with these edits.

EditedCHANGELOG.md

Flagging check failure due to unrelated unused import

The check failed because of an unused import in an untracked file not created by us, preventing commit despite instructions to fix all errors. We can't safely modify that file ourselves and should inform the user that the error must be resolved before proceeding or confirm how to handle this external warning.

Requesting user guidance on unrelated lint failure

The lint check failed due to an unused import in an untracked file from other work, blocking commit despite user request. I'll inform the user of this issue and ask whether to fix and include that file in the commit or proceed otherwise, while noting which files are ready to commit.

npm run check failed due to an unrelated untracked file: packages/coding-agent/examples/extensions/trigger-compact.ts has an unused import. That file is not part of our changes and is currently untracked.

How do you want to proceed?

Options:

I fix that unused import and include the file in the commit.
You or another agent handle it, then I rerun checks and commit only our files.

Once confirmed, I will commit and push only these files:

packages/tui/src/tui.ts
packages/tui/CHANGELOG.md
packages/coding-agent/src/core/settings-manager.ts
packages/coding-agent/src/modes/interactive/components/settings-selector.ts
packages/coding-agent/src/modes/interactive/interactive-mode.ts
packages/coding-agent/README.md
packages/coding-agent/CHANGELOG.md

try again npm run check

Isolating commits to only agent files

There are unexpected unrelated file changes present that must not be staged or committed. I'll restrict git add to only the files modified by the agent and skip all others to avoid touching other agents' files.

Confirming full file reads before commit

Although edits are done, I'll read the full contents of modified files now to align with instructions before committing only the agent-modified files, ensuring unrelated changes aren't included.