Skip to content

Fix key repeat rendering lag - #3986

Merged
austinywang merged 9 commits into
mainfrom
issue-1829-key-repeat-lag
May 23, 2026
Merged

austinywang merged 9 commits into
mainfrom
issue-1829-key-repeat-lag

Conversation

@austinywang

@austinywang austinywang commented May 12, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Add a regression test proving printable key repeat reaches Ghostty without forcing a synchronous terminal surface refresh.
  • Remove the per-text-keyDown forceRefresh path so Ghostty wakeups/renderer own steady-state redraws.

Closes #1829

Testing

  • Not run locally per repository policy.

Note

Medium Risk
Touches the core GhosttyNSView.keyDown input pipeline and removes a synchronous refresh trigger, which could affect perceived responsiveness or redraw timing under edge-case text input/IME scenarios.

Overview
Fixes key-repeat rendering lag by removing the per-text-input terminalSurface?.forceRefresh(reason: "keyDown.textInput") path in GhosttyNSView.keyDown, leaving redraws to Ghostty wakeups/renderer (and dropping the associated refreshMs timing breakdown).

Adds a debug-only hook (debugTextInputEventHandler) around interpretKeyEvents and marks debug observers as @MainActor, then introduces regression tests asserting both printable-repeat and IME-committed repeat events are forwarded to Ghostty without incrementing forceRefresh counters; also annotates GhosttyBackquoteRegressionTests with @MainActor for main-thread correctness.

Reviewed by Cursor Bugbot for commit 0d98959. Bugbot is set up for automated code reviews on this repo. Configure here.


Summary by cubic

Fixes key repeat rendering lag by removing synchronous surface refresh during text input and letting Ghostty drive redraws. Adds debug-only tests proving printable and IME repeat events reach Ghostty without forced refresh; fixes #1829.

  • Bug Fixes
    • Removed per-text-input forceRefresh from GhosttyNSView.keyDown; redraws now rely on Ghostty wakeups. Added a debug text-input hook in keyDown and now honor its Boolean result to optionally bypass default interpretation.
    • Added testPrintableKeyRepeatDoesNotForceSurfaceRefresh and testIMECommittedKeyRepeatDoesNotForceSurfaceRefresh; introduced @MainActor debugGhosttySurfaceKeyEventObserver and debugTextInputEventHandler, and marked GhosttyBackquoteRegressionTests as @MainActor.

Written for commit 0d98959. Summary will update on new commits. Review in cubic

Summary by CodeRabbit

  • Tests

    • Added regression tests ensuring repeated printable and IME-committed key repeats are forwarded correctly without triggering synchronous surface refreshes; isolated a test class to the main actor.
  • Refactor

    • Moved debug input hooks and a new debug text-input handler to run on the main thread for more reliable instrumentation.
    • Simplified keyboard/IME forwarding so literal text and fallback key events are handled more consistently.
  • Bug Fixes

    • Removed forced post-input surface refresh to avoid unnecessary redraws and latency.

Review Change Stack

@vercel

vercel Bot commented May 12, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
cmux Ready Ready Preview, Comment May 23, 2026 4:16am
cmux-staging Building Building Preview, Comment May 23, 2026 4:16am

@coderabbitai

coderabbitai Bot commented May 12, 2026 •

Copy link
Copy Markdown

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

GhosttyNSView.keyDown handling removes forced synchronous TerminalSurface refreshes during key repeat and IME input, annotates DEBUG hooks with @MainActor, adds a DEBUG text-input handler, and updates IME forwarding logic. Two DEBUG tests assert repeats forward without forcing refreshes.

Changes

Key Repeat Refresh Removal

Layer / File(s) Summary
Debug observer and logging instrumentation
Sources/GhosttyTerminalView.swift
debugGhosttySurfaceKeyEventObserver is annotated @MainActor; a new @MainActor debugTextInputEventHandler is added. DEBUG timing removes the refreshMs metric and its phase from keyDown profiling/logging.
DEBUG interpretation fallback
Sources/GhosttyTerminalView.swift
In keyDown(with:) DEBUG path, debugTextInputEventHandler(self, textInputEvent) is consulted first; if it returns false, interpretKeyEvents([textInputEvent]) is invoked as a fallback.
IME text handling and forced refresh removal
Sources/GhosttyTerminalView.swift
The IME accumulated-text branch is refactored to clear composing and iterate committed segments, forwarding per-segment either literal text or a key-only fallback without refresh-flag bookkeeping; the post-text-input terminalSurface?.forceRefresh(...) call and its timing measurement are removed.
Key repeat regression tests and test annotation
cmuxTests/TerminalAndGhosttyTests.swift, cmuxTests/CJKIMEInputTests.swift
Two DEBUG-only tests assert printable and IME-committed key repeats forward to Ghostty without synchronous forced surface refreshes (debugForceRefreshCount() remains 0). A test class is annotated @MainActor.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • manaflow-ai/cmux#4382: Modifies GhosttyNSView.keyDown(with:) IME/text-input interpretation and overlaps input-path logic.

Poem

🐰 I hopped into the keydown stream,
No forced refresh to break the dream,
Repeats now flow, smooth and bright,
MainActor keeps the timing right,
Tests whisper: the surface stays light.

🚥 Pre-merge checks | ✅ 16 | ❌ 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 (16 passed)
Check name Status Explanation
Title check ✅ Passed The title "Fix key repeat rendering lag" directly matches the main objective of removing synchronous refresh during text input to address the key-repeat lag issue from #1829.
Linked Issues check ✅ Passed The PR successfully addresses the core requirements from #1829: removes per-text-input synchronous refresh to eliminate batched rendering, adds regression tests verifying no forced refreshes on key repeat, and marks debug utilities as @MainActor.
Out of Scope Changes check ✅ Passed All changes are narrowly scoped to fixing the key-repeat lag issue: removed the forceRefresh call, added debug infrastructure, and introduced regression tests. The @MainActor annotations on GhosttyBackquoteRegressionTests are a necessary alignment with the new debug hook design.
Cmux Swift Actor Isolation ✅ Passed PR explicitly marks debug static properties @MainActor in #if DEBUG blocks; no implicit isolation mistakes, Sendable violations, or background context access.
Cmux Swift Blocking Runtime ✅ Passed Removes blocking forceRefresh("keyDown.textInput") call from text input path. No new blocking/timing primitives in production. Test-only blocking code is properly isolated.
Cmux No Hacky Sleeps ✅ Passed PR changes are primarily in Swift files. New TypeScript/JavaScript files are in Prototypes/ and Resources/ (non-production), with no "hacky sleeps" violating the rule in production code.
Cmux Swift Concurrency ✅ Passed PR adds @MainActor annotations and DEBUG-only tests. No legacy async patterns (DispatchQueue, Combine, completion handlers, fire-and-forget Tasks) introduced.
Cmux Swift @Concurrent ✅ Passed No violations of swift-concurrent-annotation rules. Changes use @MainActor on synchronous closure storage properties, not @concurrent functions. No async patterns involved.
Cmux Swift File And Package Boundaries ✅ Passed PR adds -3 net lines to GhosttyTerminalView via @MainActor debug hooks and bug fix. Test additions allowed. File is UI/AppKit/Ghostty glue, inherently app-target code. No 250+ line threshold exceeded.
Cmux Swift Logging ✅ Passed PR complies with swift-logging.md: No print/NSLog added to production code; removes refreshMs logging; debug hooks are DEBUG-guarded; tests are DEBUG-only with XCTSkip fallback; no secrets.
Cmux User-Facing Error Privacy ✅ Passed PR contains no user-facing error messages, alerts, or exposed sensitive details. All changes are DEBUG-only code, tests, or internal infrastructure—compliant with user-facing error privacy rules.
Cmux Full Internationalization ✅ Passed All changes are DEBUG-only instrumentation, test methods, or @MainActor annotations—exempted per the rule. No user-facing strings, dialogs, or string catalogs modified.
Cmux Swiftui State Layout ✅ Passed PR introduces only DEBUG-only static variables and test additions; no new @Published/@observable state or SwiftUI state violations.
Cmux Architecture Rethink ✅ Passed Removes per-keystroke forceRefresh, adds @MainActor debug hooks, includes debug-only tests. Fits allowed cases: small fix, required bridges, test-only synchronization.
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed PR adds no new standalone windows, NSWindow, NSPanel, NSWindowController code. Changes are debug hooks (static vars), DEBUG-only test fixtures, and @MainActor annotations—all allowed by policy.
Description check ✅ Passed PR description covers summary and testing sections, but omits video demo, review trigger commands, and incomplete checklist.

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

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch issue-1829-key-repeat-lag

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 May 12, 2026 •

Copy link
Copy Markdown
Contributor

Greptile Summary

Removes the per-keystroke terminalSurface?.forceRefresh(reason: "keyDown.textInput") call from GhosttyNSView.keyDown, letting Ghostty's own wakeup/renderer drive redraws and eliminating the key-repeat rendering lag reported in #1829. Adds two #if DEBUG regression tests that verify both the direct-text and IME-accumulated-text paths still forward events to Ghostty without triggering a forced surface refresh.

  • GhosttyTerminalView.swift: Both debug static hooks (debugGhosttySurfaceKeyEventObserver, new debugTextInputEventHandler) are now correctly marked @MainActor; the #if DEBUG intercept path routes text-input events through the handler before falling back to interpretKeyEvents, keeping production paths unchanged.
  • CJKIMEInputTests.swift: GhosttyBackquoteRegressionTests gains @MainActor to satisfy Swift 6 actor-isolation requirements.
  • TerminalAndGhosttyTests.swift: Two new regression tests are added to TerminalNotificationDirectInteractionTests, which is missing @MainActor even though both new tests read and write @MainActor-isolated static vars — this is the same fix applied to GhosttyBackquoteRegressionTests in the same PR.

Confidence Score: 4/5

Safe to merge after adding @MainActor to TerminalNotificationDirectInteractionTests; the production change itself is correct.

The production change removing forceRefresh is clean and well-motivated. The one outstanding concern is that TerminalNotificationDirectInteractionTests is not annotated with @MainActor even though the two new test methods directly access @MainActor-isolated static vars — the same annotation the PR already applied to GhosttyBackquoteRegressionTests. Under Swift 6 strict concurrency this is a compile-time actor-isolation violation, meaning the new regression tests would not build or run.

cmuxTests/TerminalAndGhosttyTests.swift — the TerminalNotificationDirectInteractionTests class declaration needs @MainActor to match the access pattern of the two new test methods.

Important Files Changed

Filename Overview
Sources/GhosttyTerminalView.swift Removes per-keystroke forceRefresh from keyDown, adds @MainActor to both debug static vars, and wires a #if DEBUG-only debugTextInputEventHandler intercept. Production change is clean and minimal.
cmuxTests/CJKIMEInputTests.swift Adds @MainActor to GhosttyBackquoteRegressionTests to satisfy Swift 6 actor-isolation requirements for the class's test methods.
cmuxTests/TerminalAndGhosttyTests.swift Adds two DEBUG-only regression tests for key-repeat without forced refresh; TerminalNotificationDirectInteractionTests is missing the @MainActor annotation needed because both new tests access @MainActor-isolated static vars, likely causing a Swift 6 compilation error.

Sequence Diagram

sequenceDiagram
    participant App as NSEvent (keyDown)
    participant GNV as GhosttyNSView
    participant IKE as interpretKeyEvents / debugTextInputEventHandler
    participant GH as ghostty_surface_key
    participant TS as TerminalSurface (renderer)

    App->>GNV: keyDown(event)
    GNV->>IKE: [DEBUG] debugTextInputEventHandler? or interpretKeyEvents
    IKE-->>GNV: accumulatedText / directText
    GNV->>GH: ghostty_surface_key (repeat/text)
    Note over GNV,TS: forceRefresh REMOVED — redraws driven by Ghostty wakeups
    GH-->>TS: Ghostty wakeup → renderer redraws
Loading

Reviews (7): Last reviewed commit: "fix: respect text input debug hook resul..." | Re-trigger Greptile

Comment thread cmuxTests/TerminalAndGhosttyTests.swift

@jesstelford jesstelford 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.

LGTM! 🚢

@cursor cursor 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.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit b0257cb. Configure here.

Comment thread Sources/GhosttyTerminalView.swift

@coderabbitai coderabbitai 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.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 8429-8434: The debug block currently calls
Self.debugTextInputEventHandler(self, textInputEvent) but ignores its Bool
return, preventing a handler from opting into the normal path; change the logic
so you call interpretKeyEvents([textInputEvent]) only when either
Self.debugTextInputEventHandler is nil or when it returns false — i.e., invoke
the handler, capture its Bool result, and if that result is false fallback to
interpretKeyEvents(_:), keeping the same parameters (textInputEvent) and
preserving the existing DEBUG-only conditional around
debugTextInputEventHandler.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: e18337f9-6d82-44da-8b19-5d3531b18b3b

📥 Commits

Reviewing files that changed from the base of the PR and between b0257cb and 9e5ca2e.

📒 Files selected for processing (2)
  • Sources/GhosttyTerminalView.swift
  • cmuxTests/TerminalAndGhosttyTests.swift

Comment thread Sources/GhosttyTerminalView.swift
@austinywang

Copy link
Copy Markdown
Contributor Author

Verified the Greptile TerminalNotificationDirectInteractionTests actor-isolation note against current HEAD: the class is already annotated @MainActor in cmuxTests/TerminalAndGhosttyTests.swift, so no code change is needed for that item.

This branch was successfully deployed

1 active deployment
Preview – cmux — 0d98959e Deployed May 23, 2026 by vercel[bot]
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.

Key repeat rendering lag: display freezes then catches up all at once

2 participants