Reviewed cursor visibility and IME positioning in PRPublic

Agent

Model

GPT-5.2 Codex

Started

Jan 17, 10:24 AM

Messages203

Downloads0

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.

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?

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

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?

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

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

ok, do we need to enable visibility when the tui is shut down?

Yes, on shutdown we should always show the cursor to restore the terminal. TUI.stop already calls terminal.showCursor, so visibility is restored regardless of showHardwareCursor.

Also, push failed because main is ahead and there are unstaged changes in the worktree from other files. I cannot rebase without addressing those. How do you want to proceed with the rebase and push?