fix IME position when using claude code on MacOS - #12957
Conversation
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR keeps the terminal cursor position cache updated when block-grid cursors are hidden via SHOW_CURSOR, so macOS IME candidate windows can anchor to the active cursor location for Claude Code-style TUIs. It also refactors editor cursor caching to reuse the same computed rect.
Concerns
- No blocking correctness, security, or spec-alignment concerns found in the changed lines.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR factors terminal cursor geometry so hidden cursors can keep terminal_view:cursor_* position cache entries current without painting, and uses that cache path for active command/output grids when TermMode::SHOW_CURSOR is false. It also simplifies editor cursor rect reuse.
Concerns
- No blocking correctness or security concerns found in the changed lines.
- The PR includes visual evidence for the user-facing IME positioning change.
- No approved or repository spec context was provided for implementation drift review.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
Linked Issue
Fixes #9145
Testing
To repro the bug. Open
claudeCLI on MacOS and then open the IME. The IME will be in the top-left corner of the grid, i.e. position (0, 0).On this branch, the position is correct:
Description
The bug is due to the cached cursor position (in
PaintContext::position_cachebeing incorrect). The IME uses the cached cursor position to decide where to render. The bug was occurring because the position was stale at (0, 0).When you run
claude, it tries to prevent visual flickering using the synchronized update protocol. It emits escape codes like this:This is from inspecting PTY recordings BTW
It wraps each "frame" in synchronized output as well as hiding and then showing the cursor. I presume this is to ensure the cursor doesn't jump around at all during the update cycle. They want to maintain the user's idea of the cursor position even though updating the screen means the actual cursor needs to fully traverse the grid to update each cell. Hiding it during the update ensures that the user won't notice that.
So why does this go wrong?
We do implement this sync output protocol. However, we do not actually paint the scene (Warp's idea of a frame) synchronously. We send a wakeup event, but wakeup events are buffered and coalesced, i.e. throttled. When
\e[?2026l(end sync) is received, we correctly send a wakeup event and the cursor is SHOWN. But soon the\e[?25l(hide cursor) for the next update comes in and gets applied to the terminal model and coalesces with the last wakeup event. Now we get a deferred render where cursor is HIDDEN.Finally rendering the scene happens and
BlockListElement::draw_blockis called. It checksTermMode::SHOW_CURSOR. If that is on, it callsfn render_cursorwhich is the function responsible for setting the cached cursor position which the IME depends on. If not, no position is cached and the IME will not have an up-to-date position.I think in an ideal world,