Skip to content

fix: prevent cursor jumps during synchronized redraws - #4310

Closed
ogulcancelik wants to merge 2 commits into
masterfrom
issue/4303-synchronized-cursor
Closed

ogulcancelik wants to merge 2 commits into
masterfrom
issue/4303-synchronized-cursor

Conversation

@ogulcancelik

@ogulcancelik ogulcancelik commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator

summary

  • prevent the Windows drawn cursor from jumping above the Codex composer while typing, including with animations disabled
  • exclude intermediate synchronized-frame positions from cursor settling
  • preserve the previous settled candidate across idle gaps and give each changed candidate its own quiet window, while keeping the maximum hold for continuous movement

refs #4303

remaining failure reproduced

The initial fix was insufficient. With Windows system ConPTY, a synchronized frame can close at Codex's repair cell, with the real input caret restored in a separate write about 8–11 ms later. Ignoring only positions inside synchronized output does not cover that ordering.

The old settling timer also survived idle gaps. On the next redraw it could immediately publish the temporary repair position instead of preserving the previous quiet candidate. This followup corrects that timing without adding a cursor-intent escape-sequence parser.

live Windows verification

Tested in native Windows Terminal on Windows 11 10.0.26200.9457 using the reporter's exact JJLiebig/codex 0.154.0-fork.5, Astra low, and Herdr's drawn cursor. Each completed recording typed 150 characters over a 25-second recording; no model prompt was submitted.

Build ConPTY Whimsy/animations Observed result
original PR fix system on repeated off-composer cursor jumps
original PR fix system off repeated off-composer cursor jumps
followup system off no off-composer cursor detected
followup system on no off-composer cursor detected
followup bundled on no off-composer cursor detected

Recordings and raw trace are retained locally. The corrected recordings still show the moving input caret, not a permanently hidden cursor. These are bounded VM observations, not verification on every reporter's machine.

automated validation

  • regression tests failed on the original settling implementation and pass with the correction
  • native Windows cursor tests: 14 passed
  • native Windows pane tests: 343 passed, 1 ignored
  • just check passed after rebasing onto current master: 3,629 tests plus formatting, clippy, Windows cross-target lint, and maintenance/integration/docs checks
  • fresh-context GPT reviewer and simplifier completed with no remaining findings

The live VM runs preceded the rebase; both cursor/terminal files are byte-for-byte unchanged by the rebase. Native macOS was not tested. The behavior remains Windows-gated. Temporary named sessions/tasks were removed and the VM checkout restored clean.

@coderabbitai

coderabbitai Bot commented Sep 17, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 04f4a03b-7af6-41de-b35a-8ca9fff4474c

📥 Commits

Reviewing files that changed from the base of the PR and between 32161ce and cbb3a40.

📒 Files selected for processing (2)
  • src/pane/cursor.rs
  • src/pane/terminal.rs

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

CursorPositionSettleState now tracks candidate timing separately from the maximum hold timer. It preserves eligible candidates, resets the quiet window only when the candidate changes, and centralizes settle resets. process_pty_bytes skips settle observation during synchronized output. Windows-only tests cover complete and split synchronized sequences, including final cursor position, visibility, and shape.

Suggested reviewers: jjliebig

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to cbb3a

The change prevents synchronized redraw positions from being adopted as cursor settle candidates while retaining final cursor state handling. No concrete merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 64.71% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly and concisely describes the primary change: preventing cursor jumps during synchronized redraws.
Description check ✅ Passed The description directly explains the cursor-jump fix, ConPTY behavior, implementation details, tests, and validation results.
  • Fix all pre-merge checks with AI

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@kangal-bot

Copy link
Copy Markdown
Collaborator

@coderabbitai review
@greptileai

@coderabbitai

coderabbitai Bot commented Sep 17, 2026

Copy link
Copy Markdown

@kangal-bot I will review the changes.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@greptile-apps

greptile-apps Bot commented Sep 17, 2026

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

The PR appears safe to merge; no actionable correctness, security, or repository-rule violations remain.

Summary

The PR refines Windows cursor-position settling to prevent temporary synchronized-redraw positions from displacing the displayed caret.

  • Tracks a separate quiet-window start for each changed cursor candidate.
  • Promotes an eligible candidate before processing a later redraw while retaining a maximum hold for continuous movement.
  • Excludes intermediate synchronized-output positions from settlement.
  • Adds regression coverage for delayed ConPTY caret restoration, moving candidates, maximum holds, and split synchronized sequences.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[PTY bytes processed] --> B{Synchronized output active?}
  B -- Yes --> C[Skip cursor observation]
  C --> D[Suppress render and delay]
  B -- No --> E[Observe final cursor state]
  E --> F{Existing candidate quiet long enough?}
  F -- Yes --> G[Promote previous candidate]
  F -- No --> H[Keep settled cursor]
  G --> I{Incoming position changed?}
  H --> I
  I -- Yes --> J[Start candidate quiet window]
  I -- No --> K[Settle or update current cursor]
  J --> L{Quiet window or maximum hold elapsed?}
  L -- No --> M[Render previous settled position]
  L -- Yes --> N[Report latest candidate]
  K --> N
Loading

Reviews (2) · Last reviewed commit: "fix: settle cursor positions across dela..."

@ogulcancelik

Copy link
Copy Markdown
Collaborator Author

@JJLiebig could you test this one?

@axelalbertkarlsson

Copy link
Copy Markdown
Contributor

@ogulcancelik Tested it from this branch, has not solved the issue (at least for me) can try to figure out a way to solve it and post it here / in the issue if it is a small edit.

Screen recording

Inspelning.2026-09-17.223743.mp4

@JJLiebig

JJLiebig commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator

@axelalbertkarlsson not saying we shouldn't fix this but the astra "dots" will be removed in the next codex-cli update, it's already removed in alpha cause it created a ton of issues.

openai/codex#45137 & gets replaced with openai/codex#46096

@axelalbertkarlsson

Copy link
Copy Markdown
Contributor

@axelalbertkarlsson not saying we shouldn't fix this but the astra "dots" will be removed in the next codex-cli update, it's already removed in alpha cause it created a ton of issues.

@JJLiebig Totally fair! With the Astra dots removed the cursor moving is a lot less annoying, but the cursor still moves. Understand if this is not a priority atm

@JJLiebig

JJLiebig commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator

@axelalbertkarlsson no absolutely, I think this is great actually as we can fix the cursor-move-issue and test it with the astra dots - so excellent time to fix it I suppose haha

I must say I have the same core issue but because my personal config has the flat | cursor and not the unix-style block one, I never noticed I think.

@ogulcancelik can confirm @axelalbertkarlsson's findings, still jumping around. Getting some logging on ConPTY bytes now.

@axelalbertkarlsson

Copy link
Copy Markdown
Contributor

@JJLiebig Ahh nice! Don't know the root cause of this issue but I'll try to see if i can find it, the issue has not been unique to Herdr, have noticed in alot of different repos.

@JJLiebig

JJLiebig commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator

Some extra capture evidence on native Windows 11, Windows Terminal:

Environment

  • Windows 11 Pro 10.0.26200, native Windows Terminal (not WSL)
  • herdr 0.9.1 debug build of this branch (head 5ed63c78, includes the 32161cec guard), [ui] host_cursor = "drawn"
  • codex-cli 0.154.0-fork.5, model gpt-6-astra low

Result: the drawn cursor still flashes/jumps while typing in the composer, so the current guard alone is not enough. I captured the raw writes reaching process_pty_bytes together with the cursor cursor_settle_state.observe would see (temporary env-gated logging; no behaviour change, reverted afterwards). The gap is in how the guard reads the mode.

What the capture shows

Every synchronized frame arrives as two writes:

  • A: the opening ?2026h. The mode is ON at the end of this write, so !synchronized_output is false and the guard correctly skips it (observe=0).
  • B: the frame body and/or the closing ?2026l. The mode is OFF at the end of this write, so the guard treats it as unsynchronized and calls observe(...) with the frame's end-of-frame cursor — an intermediate cell, not the composer caret.

One ~30s typing capture:

  • 323 writes contained ?2026h; all 323 skipped (observe=0)
  • 323 writes contained ?2026l; all 323 observed (observe=1)
  • observed cursor rows on those closing writes: row 23 ×232, row 27 ×162, row 28 ×11, row 26 ×10 — i.e. mostly not the composer row
  • render_prof per second: pty.synchronized_output_suppressed 7–11, but pty.cursor_state_update still 13–14

Representative lines (t = ms since server start; cursor=x,y,vis,shape is the position that becomes a settle candidate):

t=20530ms sync=0 observe=1 h=0 l=1 cursor=47,27,1,0 bytes=\e[?2026l
t=20870ms sync=0 observe=1 h=0 l=1 cursor=0,23,1,0  bytes=\e[0 q\e[?2026l
t=14177ms sync=0 observe=1 h=0 l=1 cursor=19,28,0,0 bytes=\e[1m\x085\e[?2026l

The first is ?2026l on its own: the write starts inside the frame opened by the previous write and ends outside it, so the end-of-write check reports "not synchronized" and the mid-frame cursor 47,27 is observed.

Why the new tests pass

They split ?2026h / ?2026l so the body write leaves the mode ON, which is exactly the path the guard protects. Real ConPTY delivery ends the mode inside the write carrying the frame tail / ?2026l, so the guarded branch is never taken for the position that leaks.

Possible direction

Gate on whether the write participated in a synchronized frame at any point, rather than on the mode at the end of the write — e.g. only observe when the write both starts and ends outside synchronized output (or when it contains no ?2026h/?2026l boundary). That keeps the frame body and its trailing close write out of the settle state while still observing the normal post-frame cursor restore.

Happy to re-run this capture against a new revision.

@axelalbertkarlsson

Copy link
Copy Markdown
Contributor

@JJLiebig & @ogulcancelik This fixed it! https://github.com/axelalbertkarlsson/herdr/tree/fix/codex-cursor-settle Have not tested it on another computer but I guess you could take some inspo from there

Fixherdr.1.mp4

@JJLiebig

JJLiebig commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator

@axelalbertkarlsson tried your commit:
image
Welp, more fixing to do.

It sometimes flickers, sometimes it's fine. Even right now, with a master built exe on laptop, no flicker, but on my desktop it does. Odd.


Edit, some notes on Axel's build:

Tested axelalbertkarlsson/fix/codex-cursor-settle (tip c01c510c) natively on Windows 11 + Windows Terminal, with codex-cli 0.154.0-fork.5 on gpt-6-astra and host_cursor = "drawn". It still flashes: the drawn cursor lands at column 0 on the row above the composer while the caret sits at (2,26).

From an instrumented build (log-only, reverted after):

  • 642 PTY writes captured; 316 ended non-synchronized and were classified positioned.
  • 327 of those observed the cursor at column 0 (left edge), rows 23/24. Representative:
    t=21756ms sync=0 placement=pos cursor=0,24,1 bytes=\e[?2026l
    t=21615ms sync=0 placement=pos cursor=0,23,1 bytes=\e[0 q\e[?2026l\e[?2026h\e[0 q\e[?2026l
    
  • The composer caret is at (2,26), so those are mid-frame positions being adopted.

Why it still leaks: CursorPlacementTracker adopts the frame-close cursor when the frame closed on a positioning sequence. Agents position the cursor to paint or clear a row and then close the frame, placing the caret in a later write. A bare CUP right before ?2026l is not a caret placement, but it is classified positioned, so that branch adopts it. The printed (hold) branch looks right; the positioned branch is the leak.

Also worth noting: the cursor-rendering files are byte-identical between the current release (5c61d8ec) and the PR base (101ccc20) — git diff 5c61d8ec 101ccc20 -- src/pane/cursor.rs src/pane/terminal.rs src/protocol/render_ansi.rs src/client/state.rs is empty. So an older build is not inherently better here; the symptom is intermittent and depends on how the agent's frames happen to split.

Possible direction: treat the frame close as never authoritative on its own — hold the previous settled cursor across ?2026l and only adopt a positioning that arrives after the block closes.

@axelalbertkarlsson

axelalbertkarlsson commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

@JJLiebig fuck that's confusing! Classic works on my machine issue then. But I'll try figure out the root cause, and then test on both work/personal laptops

@ogulcancelik
ogulcancelik force-pushed the issue/4303-synchronized-cursor branch from 0882b9d to bebe833 Compare September 18, 2026 14:40
@ogulcancelik
ogulcancelik force-pushed the issue/4303-synchronized-cursor branch from bebe833 to cbb3a40 Compare September 18, 2026 14:54
@ogulcancelik ogulcancelik added ai-review Trigger automated AI reviews for pull requests admitted by the PR gate greptile-review Trigger Greptile review for contributor-approved pull requests coderabbit-review labels Sep 18, 2026
@ogulcancelik

Copy link
Copy Markdown
Collaborator Author

@greptileai

@ogulcancelik

Copy link
Copy Markdown
Collaborator Author

i reviewed the docstring-coverage warning. these are internal cursor helpers and descriptive regression tests; the non-obvious timing rule already has a comment. i am keeping the code consistent with the repo guidance against redundant comments rather than adding docstrings solely for the coverage percentage.

@ogulcancelik

Copy link
Copy Markdown
Collaborator Author

@axelalbertkarlsson @JJLiebig my gpt is pretty confident on latest patch, tested on a windows wm. could you give it a shot?

@JJLiebig

JJLiebig commented Sep 18, 2026

Copy link
Copy Markdown
Collaborator

lgtm @ogulcancelik

4310.mp4

@axelalbertkarlsson can you check the head as well rq?

Oh one thing I realized, this was still 0.154, gonna briefly check 0.155 as well

Edit: Yep still jumpy, a lot better, but not entirely fixed.

0155_boot.mp4

@axelalbertkarlsson

Copy link
Copy Markdown
Contributor

@JJLiebig & @ogulcancelik Looks alot better!! The recording did not catch is entirely but when holding down backspace did trigger the flickering a tiny bit but this looks good to me.

Inspelning.2026-09-18.172848.mp4

@axelalbertkarlsson

Copy link
Copy Markdown
Contributor

@JJLiebig I gotta go now, so if you’ve got other higher-priority PRs to get to, I can help you test this more tomorrow.

@JJLiebig

Copy link
Copy Markdown
Collaborator

@axelalbertkarlsson thanks for all the help, got a script to properly tackle this locally and I think there's a few things we could do.

@JJLiebig

JJLiebig commented Sep 18, 2026

Copy link
Copy Markdown
Collaborator

Follow-up for anyone cherry-picking: I pulled the settle logic apart from a raw capture and put a candidate fix on a side branch.

Branch: fix/cursor-settle-confirm (commit dd7a78af, based on cbb3a402)
issue/4303-synchronized-cursor...fix/cursor-settle-confirm

What the capture showed: during Codex start/exit the terminal parks the cursor at column 0 on a row above the composer (clear-and-rehome, ?25h after a title set, the & codex shell echo), and the real caret arrives 30–70 ms later. Those parks are single observations, so they aged past the 20 ms settle window and got drawn.

The branch holds a candidate that jumps (different row, or >2 column move) for the max window (100 ms) instead of the settle window (20 ms). Ordinary same-row caret steps still settle in 20 ms, so typing is unaffected. It also restarts the churn window after a quiet gap so a finished burst can't adopt a lone sample. Cursor tests pass unchanged, plus a regression test from the trace; just check is green.

Caveat: the repaint is rare and my detector wasn't precise enough for a clean A/B (an early version counted the shell's own prompt cursor; the Codex-gated version skips the transition frames where it actually happens). Treat this as the captured mechanism plus a targeted mitigation, not proof of elimination.

Next thing to try: today cursor_settle_state.observe() samples the cursor on every PTY write, so a redraw's intermediate park cells are recorded and then have to be smoothed away. The alternative is to sample the cursor once at the end of a write burst — debounce the writes and only read the cursor after the pane has been quiet for a few ms, when the child has finished drawing and its real caret is the last thing it set. Then mid-redraw parks never enter the settle state in the first place and the max-hold heuristic isn't needed. Trade-off: cursor updates wait for the burst to end. Can has no Windows box, so I'll prototype that here and report back.

@JJLiebig

Copy link
Copy Markdown
Collaborator

Closed in favour of #4389

@JJLiebig JJLiebig closed this Sep 19, 2026
@JJLiebig
JJLiebig deleted the issue/4303-synchronized-cursor branch September 19, 2026 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Trigger automated AI reviews for pull requests admitted by the PR gate coderabbit-review greptile-review Trigger Greptile review for contributor-approved pull requests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants