Repository navigation
Conversation
forceRefresh already reconciles display ID (to restart stuck CVDisplayLinks) and Metal layer size before forcing a redraw. Adding occlusion reconciliation makes every forceRefresh call a recovery point for any occlusion desync, regardless of cause. forceRefresh is called on: focus changes, geometry reconciliation, surface creation, user input, and the cmux refresh-surfaces CLI. ghostty_surface_set_occlusion is cheap (mailbox push) and idempotent (renderer-side dedup), so redundant calls have negligible cost.
|
@peteraxelblom is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change modifies Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Greptile SummaryThis PR adds a single line —
Confidence Score: 5/5Safe to merge — single idempotent C FFI call added to an already-guarded path, consistent with existing patterns. No P0 or P1 findings. The change is one line, well-commented, and follows the exact same defensive pattern already used for display ID reassertion. It does not introduce allocations, file I/O, or formatting (as prohibited by CLAUDE.md for this path), and the lock-free, idempotent nature of ghostty_surface_set_occlusion is clearly stated in both the PR description and the surrounding code comments. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller as Caller
participant FR as forceRefresh()
participant SO as setOcclusion()
participant DID as ghostty_surface_set_display_id
participant GFR as view.forceRefreshSurface()
participant GR as ghostty_surface_refresh
Caller->>FR: forceRefresh(reason:)
FR->>FR: guard attachedView, surface, window, bounds
FR->>SO: setOcclusion(view.isVisibleInUI) NEW
Note over SO: lock-free mailbox push, renderer deduplicates
SO->>SO: guard let surface = self.surface
FR->>DID: ghostty_surface_set_display_id(currentSurface, displayID)
FR->>GFR: view.forceRefreshSurface()
FR->>FR: guard let surface = self.surface re-read
FR->>GR: ghostty_surface_refresh(surface)
Reviews (2): Last reviewed commit: "fix: reconcile occlusion state in forceR..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // the renderer may think this surface is occluded. Re-asserting here | ||
| // provides a recovery path on focus changes, geometry updates, and | ||
| // manual cmux refresh-surfaces invocations. | ||
| setOcclusion(view.isVisibleInUI) |
There was a problem hiding this comment.
CLAUDE.md explicitly calls out TerminalSurface.forceRefresh() as a typing-latency-sensitive function: "called on every keystroke. Do not add allocations, file I/O, or formatting here."
The new setOcclusion call doesn't add any of those three prohibited categories — it's a C FFI call that does a lock-free mailbox push — but it does add a call site that fires on every keystroke (the keyDown.textInput path). On that path isVisibleInUI is virtually always true, so every keystroke results in a redundant ghostty_surface_set_occlusion(surface, true).
The PR description states the call is idempotent and the renderer deduplicates, which is reassuring. Worth briefly confirming whether Ghostty's mailbox dedup happens entirely before acquiring any lock (even a spinlock) or whether there's any coordination cost, since even a compare-and-swap on the hot path can affect P99 keystroke latency in profiling.
Context Used: CLAUDE.md (source)
|
Closing — pushed prematurely without testing. Will reopen after manual verification. |
|
Manually tested on a debug build with all three changes applied. No jitter regression from the additional setOcclusion calls. Reopening. |
SummarySolid defense-in-depth addition. Making Rebase needed (blocking)Line numbers and surrounding context differ from current Suggestions (non-blocking)Trim the comment. // Recovery: re-sync occlusion in case a prior call was dropped
// (e.g. terminalSurface was nil during a visibility transition).
setOcclusion(view.isVisibleInUI)Verify comment syntax. Consider consolidating with #2484 and #2485. Great addition to the recovery toolkit. |
teamleaderleo
added
area: terminal
This branch has not been deployed
3 participants
Summary
Adds
setOcclusion(view.isVisibleInUI)toTerminalSurface.forceRefresh(), following the existing pattern of reconciling display ID and Metal layer size before forcing a surface redraw.Why
forceRefreshis the established recovery method for rendering issues — it is called on focus changes, geometry reconciliation, surface creation, user input, and via thecmux refresh-surfacesCLI command. It already re-asserts the display ID (to restart stuck CVDisplayLinks) and re-syncs the Metal layer size. Adding occlusion reconciliation makes it a recovery point for any occlusion state desync.This is defense-in-depth: if any code path causes Ghostty's renderer to think a surface is occluded when it should be visible, the next
forceRefreshcall will correct it.ghostty_surface_set_occlusionis cheap (lock-free mailbox push) and idempotent (renderer thread deduplicates), so the redundant calls on already-synced surfaces have negligible cost.Change
One line added after the guard clauses in
forceRefresh(). No other changes.Test plan
cmux refresh-surfacesstill worksRelates to #1156, #2224, #914, #2279
Summary by cubic
Add occlusion reconciliation to
TerminalSurface.forceRefresh()by callingsetOcclusion(view.isVisibleInUI)before forcing a redraw. This keeps renderer visibility in sync and makes each refresh a recovery point for occlusion desyncs (on focus changes, geometry updates, input, surface creation, andcmux refresh-surfaces) with negligible overhead.Written for commit f4b4786. Summary will update on new commits.
Summary by CodeRabbit