Skip to content

Fix terminal rendering lag during key repeat - #3131

Closed
iammmiru wants to merge 1 commit into
manaflow-ai:mainfrom
iammmiru:fix-key-repeat-render-lag
Closed

iammmiru wants to merge 1 commit into
manaflow-ai:mainfrom
iammmiru:fix-key-repeat-render-lag

Conversation

@iammmiru

@iammmiru iammmiru commented Apr 23, 2026 •

Copy link
Copy Markdown

Summary

Hi! I'm really enjoying using cmux — thanks for building it.
I ran into the key-repeat lag described in #1829 and dug into it a bit.

  • What changed?
    • Removed the synchronous terminalSurface?.forceRefresh(reason: "keyDown.textInput") call from GhosttyNSView.keyDown(with:).
  • Why?
    • Holding down keys could make terminal rendering freeze briefly and then catch up in bursts.
    • My understanding is that this extra force-refresh was adding synchronous main-thread work in the text-input hot path and interfering with Ghostty’s normal wakeup/tick-driven rendering.

Testing

  • How did you test this change?
    • Built a tagged debug app locally and tested terminal input behavior manually.
  • What did you verify manually?
    • Held printable keys in terminal panes
    • Held backspace
    • Checked Neovim hjkl
    • Checked arrow keys
    • Verified that rendering stayed smooth instead of freezing and catching up in bursts

Demo Video

For UI or behavior changes, include a short demo video (GitHub upload, Loom, or other direct link).

  • Video URL or attachment:

Review Trigger (Copy/Paste as PR comment)

@codex review
@coderabbitai review
@greptile-apps review
@cubic-dev-ai review

Checklist

  • I tested the change locally
  • I added or updated tests for behavior changes
  • I updated docs/changelog if needed
  • I requested bot reviews after my latest commit (copy/paste block above or equivalent)
  • All code review bot comments are resolved
  • All human review comments are resolved

Summary by CodeRabbit

  • Refactor
    • Improved keyboard input responsiveness by streamlining the rendering pipeline and removing unnecessary refresh operations during text input.

Summary by cubic

Fixes render lag during key repeat by removing a synchronous terminalSurface?.forceRefresh in the text-input path. Rendering now stays smooth under sustained input and relies on Ghostty’s normal wakeup/tick cycle.

  • Bug Fixes
    • Removed forceRefresh(reason: "keyDown.textInput") from GhosttyNSView.keyDown(with:) to avoid main-thread stalls during key repeat.
    • Dropped refreshMs from debug timing logs.
    • Added comment clarifying why synchronous refresh is avoided; explicit refresh remains in geometry/focus paths.

Written for commit 7b9b748. Summary will update on new commits.

@vercel

vercel Bot commented Apr 23, 2026

Copy link
Copy Markdown

@iammmiru is attempting to deploy a commit to the Manaflow Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitai

coderabbitai Bot commented Apr 23, 2026 •

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 37a6c948-d70d-4325-bc72-f82193c07c6d

📥 Commits

Reviewing files that changed from the base of the PR and between 7ebf223 and 7b9b748.

📒 Files selected for processing (1)
  • Sources/GhosttyTerminalView.swift

📝 Walkthrough

Walkthrough

The keyDown hot path in GhosttyTerminalView.swift is optimized by removing a synchronous terminalSurface.forceRefresh() call after text input, eliminating associated debug timing code and control flags. Rendering now relies on Ghostty's existing wakeup/renderer mechanism instead.

Changes

Cohort / File(s) Summary
Key input rendering optimization
Sources/GhosttyTerminalView.swift
Removed synchronous forceRefresh(reason: "keyDown.textInput") from the keyDown hot path after text insertion (IME/computed keys), eliminating debug timing (refreshMs breakdown) and the shouldRefreshAfterTextInput control flag. Rendering deferred to async wakeup mechanism with explanatory comment on key repeat behavior.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Poem

🐰 A rabbit's tale of key-press grace,
No sync refresh to slow the race—
Let wakeup dance and render flow,
Faster keystrokes, faster go! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: removing a synchronous refresh call that was causing rendering lag during key repeat.
Description check ✅ Passed The description follows the template with complete Summary and Testing sections. The author clearly explains the problem, solution, and manual verification performed.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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 and usage tips.

@greptile-apps

greptile-apps Bot commented Apr 23, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR removes the synchronous terminalSurface?.forceRefresh(reason: "keyDown.textInput") call from the keyDown text-input hot path in GhosttyNSView, along with its associated shouldRefreshAfterTextInput flag and refreshMs timing variable. The fix aligns with the project's own CLAUDE.md guidance, which explicitly warns against adding work to keystroke-sensitive paths and instructs developers to rely on Ghostty's wakeup/renderer instead of manual refresh loops.

Confidence Score: 5/5

Safe to merge — small, targeted removal of a synchronous call that was causing key-repeat lag, consistent with documented project policy.

All changes are deletions of code that was explicitly discouraged by the project's own CLAUDE.md pitfalls section. No new logic is introduced, no correctness regression is present, and the rendering path continues to be driven by Ghostty's existing wakeup/tick mechanism. No P0 or P1 findings.

No files require special attention.

Important Files Changed

Filename Overview
Sources/GhosttyTerminalView.swift Removes synchronous forceRefresh call and associated tracking variable from the keyDown text-input hot path; adds an explanatory comment. Change is minimal, focused, and consistent with the project's documented rendering policy.

Sequence Diagram

sequenceDiagram
    participant User
    participant AppKit
    participant GhosttyNSView
    participant GhosttyCore
    participant Renderer

    User->>AppKit: Key press / key repeat
    AppKit->>GhosttyNSView: keyDown(with:)
    GhosttyNSView->>GhosttyCore: ghostty_surface_key()
    Note over GhosttyNSView: BEFORE: forceRefresh() called synchronously here on every text input
    GhosttyCore-->>Renderer: schedules wakeup/tick
    Renderer-->>User: frame rendered

    Note over GhosttyNSView: AFTER: returns immediately, renderer drives display via Ghostty wakeups
Loading

Reviews (1): Last reviewed commit: "fix: avoid sync refresh on repeated text..." | Re-trigger Greptile

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 1 file

You’re at about 90% of the monthly review limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.

@sampsn

sampsn commented Apr 23, 2026

Copy link
Copy Markdown

do your changes fix this issue for you?

@iammmiru

Copy link
Copy Markdown
Author

Yes cmux feels very responsive and snappy now

@sampsn

sampsn commented May 7, 2026

Copy link
Copy Markdown

dang this needs to get merged in

@jesstelford

Copy link
Copy Markdown

It looks like #3986 duplicates this + plus adds a test to confirm no rendering regressions are added; should we combine the two branches?

@teamleaderleo

Copy link
Copy Markdown
Collaborator

Thanks for this! The key-repeat lag fix landed on main in #3986. You opened this first, so you got there first. Closing since main covers it now.

@github-project-automation github-project-automation Bot moved this from Todo to Done in cmux backlog Sep 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants