Repository navigation
Cap embedded terminal scrollback - #114
Conversation
# Conflicts: # src/renderer/Thread.zig
…y-pr # Conflicts: # src/renderer/Thread.zig
📝 WalkthroughWalkthroughChangesRenderer and embedded surface controls
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AppMailbox
participant App
participant Surface
participant RendererThread
AppMailbox->>App: Reject redraw_surface when full
App->>App: Store redraw retry request
App->>App: Drain mailbox
App->>Surface: Notify mailbox drained
Surface->>RendererThread: Signal retained visibility retry
RendererThread->>AppMailbox: Retry draw submission
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Greptile SummaryThis PR adds two independently useful features to the cmux fork of Ghostty: (1) a new
Confidence Score: 4/5Safe to merge with awareness that on iOS the draw_frame_begin/end instrumentation events do not bracket actual GPU work; consumers must treat them as draw-queued signals rather than draw-completed signals. The scrollback-cap feature is straightforward and well-tested. The visibility-regain coalescing is complex but has exhaustive inline tests covering full-mailbox retries, backend failures, vsync deferral, generation staleness, and stop-callback teardown. The only observable gap is that on the app-thread draw path (iOS), draw_frame_begin/end events fire on the renderer thread immediately after the message is queued, not when the GPU frame actually renders. src/renderer/Thread.zig (drawFrame must_draw_from_app_thread instrumentation branch); src/apprt/embedded.zig (rendererInstrumentation wiring) Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant RT as Renderer Thread
participant AppMB as App Mailbox
participant AT as App Thread
participant VR as visibility_retry async
Note over RT: Surface becomes visible (.visible msg)
RT->>RT: drainMailbox coalesces hide/show via VisibilityDrainState
RT->>RT: "applyRendererVisibilityTransition -> updateVisibilityRegainFrame()"
RT->>AppMB: push(redraw_surface)
alt Mailbox full
AppMB-->>RT: push returns 0 (app_mailbox_full)
RT->>RT: "VisibilityRegainState.pending = true, generation = G1"
RT->>RT: renderAfterMailboxDrain - one immediate retry (also full)
Note over RT: Frame retained, awaiting capacity
AT->>AT: "drainMailbox -> takeRedrawRetryRequest"
AT->>RT: appMailboxDrained() stores G1, notifies visibility_retry
VR-->>RT: visibilityRetryCallback fires
RT->>RT: "retrySubmission(G1) -> drawFrame(forced)"
RT->>AppMB: push(redraw_surface) second attempt
AppMB-->>AT: "processes redraw_surface -> GPU draw"
RT->>RT: cancel pending, syncDrawTimer
else Mailbox accepted
AppMB-->>AT: "processes redraw_surface -> GPU draw"
RT->>RT: cancel pending, normal draw cycle resumes
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant RT as Renderer Thread
participant AppMB as App Mailbox
participant AT as App Thread
participant VR as visibility_retry async
Note over RT: Surface becomes visible (.visible msg)
RT->>RT: drainMailbox coalesces hide/show via VisibilityDrainState
RT->>RT: "applyRendererVisibilityTransition -> updateVisibilityRegainFrame()"
RT->>AppMB: push(redraw_surface)
alt Mailbox full
AppMB-->>RT: push returns 0 (app_mailbox_full)
RT->>RT: "VisibilityRegainState.pending = true, generation = G1"
RT->>RT: renderAfterMailboxDrain - one immediate retry (also full)
Note over RT: Frame retained, awaiting capacity
AT->>AT: "drainMailbox -> takeRedrawRetryRequest"
AT->>RT: appMailboxDrained() stores G1, notifies visibility_retry
VR-->>RT: visibilityRetryCallback fires
RT->>RT: "retrySubmission(G1) -> drawFrame(forced)"
RT->>AppMB: push(redraw_surface) second attempt
AppMB-->>AT: "processes redraw_surface -> GPU draw"
RT->>RT: cancel pending, syncDrawTimer
else Mailbox accepted
AppMB-->>AT: "processes redraw_surface -> GPU draw"
RT->>RT: cancel pending, normal draw cycle resumes
end
Reviews (1): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -652,38 +908,54 @@ fn drawFrame(self: *Thread, now: bool) void { | |||
| // `render_now` no-op, permanently blanking the surface. macOS keeps the | |||
There was a problem hiding this comment.
draw_frame_begin/end events fire before the actual draw on iOS
On the must_draw_from_app_thread path (iOS), both draw_frame_begin and draw_frame_end are emitted synchronously on the renderer thread immediately after a successful app_mailbox.push, while the actual GPU draw occurs asynchronously on the app thread—potentially many milliseconds later. On the direct-draw path (else branch), the same events bracket the real renderer.drawFrame call. A cmux consumer using these events for frame-latency analysis or draw-phase gating will see near-zero draw durations on iOS even during heavy rendering, because the "end" fires before the GPU work starts.
| if (regain_was_pending and !t.visibility_regain.isPending()) { | ||
| t.syncDrawTimer(); | ||
| } | ||
|
|
||
| // PageList mutations maintain their own compression dirty state. Checking |
There was a problem hiding this comment.
When visibility_regain.isPending() is true, drawNowCallback (the iOS display-link path) returns .rearm without drawing. If the app mailbox remains full across multiple display-link ticks—for example, under sustained back-pressure where the app thread is slow to drain—the surface will stay blank for those frames. Recovery only happens via visibilityRetryCallback after appMailboxDrained fires. This is intentional, but the gap is unbounded by the renderer itself and depends entirely on app-thread mailbox drain latency.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@src/terminal/Terminal.zig`:
- Around line 2350-2351: Update the test around t.screens.active.reset() to
re-select a live selection immediately before resetting, rather than relying on
switchScreen(.primary), which clears it. Assert that reset changes the selection
activity token as required by Screen.reset(), while preserving the existing
previous-token comparison.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f52b8e86-2e76-4b8b-bd87-bbdc8d073616
📒 Files selected for processing (10)
include/ghostty.hsrc/App.zigsrc/Surface.zigsrc/apprt/embedded.zigsrc/renderer.zigsrc/renderer/Thread.zigsrc/renderer/generic.zigsrc/renderer/instrumentation.zigsrc/terminal/Terminal.zigsrc/terminal/render.zig
| t.screens.active.reset(); | ||
| try testing.expectEqual(@as(SelectionActivity, 4), t.selectionActivity()); | ||
| try testing.expectEqual(previous, t.selectionActivity()); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant ranges in Terminal.zig
sed -n '2280,2370p' src/terminal/Terminal.zig
printf '\n---- switchScreen ----\n'
rg -n "switchScreen|selectionActivity|reset\(\)" src/terminal/Terminal.zig
printf '\n---- Screen.reset implementation context ----\n'
sed -n '3600,3705p' src/terminal/Terminal.zigRepository: manaflow-ai/ghostty
Length of output: 9483
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the Screen.reset definition and nearby selection handling
rg -n "fn reset|selection|selectionActivity|active.reset|switchScreen" src/terminal/Terminal.zigRepository: manaflow-ai/ghostty
Length of output: 2437
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the test and surrounding helper calls with line numbers
nl -ba src/terminal/Terminal.zig | sed -n '2320,2365p'
printf '\n---- selection-related definitions ----\n'
nl -ba src/terminal/Terminal.zig | sed -n '3600,3695p'Repository: manaflow-ai/ghostty
Length of output: 197
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Minimal proof for the disputed contract:
# - whether `switchScreen` clears selection
# - whether `reset()` touches selectionActivity / selection state
python3 - <<'PY'
from pathlib import Path
text = Path('src/terminal/Terminal.zig').read_text()
for needle in [
'switchScreen',
'selectionActivity',
'active.reset()',
'fn reset(',
]:
print(f'== {needle} ==')
for i, line in enumerate(text.splitlines(), 1):
if needle in line:
start = max(1, i-6)
end = min(len(text.splitlines()), i+12)
for j in range(start, end+1):
print(f'{j:5d}: {text.splitlines()[j-1]}')
print()
break
PYRepository: manaflow-ai/ghostty
Length of output: 2534
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect selection activity updates and reset/clearSelection behavior.
rg -n "selection_activity|clearSelection\(|selection_scroll|selection changes|advance.*selection|fetchAdd|store\(.release\)" src/terminal/Terminal.zig
printf '\n---- nearby around clearSelection ----\n'
python3 - <<'PY'
from pathlib import Path
text = Path('src/terminal/Terminal.zig').read_text().splitlines()
for needle in ['pub fn clearSelection', 'selection_activity', 'selection_scroll']:
for i, line in enumerate(text, 1):
if needle in line:
start=max(1, i-12); end=min(len(text), i+40)
print(f'\n== {needle} @ line {i} ==')
for j in range(start, end+1):
print(f'{j:5d}: {text[j-1]}')
break
PYRepository: manaflow-ai/ghostty
Length of output: 5104
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the screen reset implementation and inspect its body.
rg -n "pub fn reset\(|fn reset\(" src/terminal src | head -n 50
printf '\n---- around candidate reset definitions ----\n'
python3 - <<'PY'
from pathlib import Path
cands = []
for path in Path('src').rglob('*.zig'):
text = path.read_text(errors='ignore').splitlines()
for i, line in enumerate(text, 1):
if 'fn reset(' in line or 'pub fn reset(' in line:
cands.append((str(path), i))
for path, i in cands[:20]:
print(f'## {path}:{i}')
text = Path(path).read_text(errors='ignore').splitlines()
start=max(1, i-12); end=min(len(text), i+45)
for j in range(start, end+1):
print(f'{j:5d}: {text[j-1]}')
print()
PYRepository: manaflow-ai/ghostty
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Also inspect the terminal reset path where active.reset() is called.
nl -ba src/terminal/Terminal.zig | sed -n '3810,3845p'Repository: manaflow-ai/ghostty
Length of output: 197
🏁 Script executed:
#!/bin/bash
set -euo pipefail
nl -ba src/terminal/Terminal.zig | sed -n '3810,3845p'Repository: manaflow-ai/ghostty
Length of output: 197
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '540,700p' src/terminal/Screen.zig
printf '\n---- token updates ----\n'
rg -n "selection_activity|fetchAdd|store\\(|clearSelection\\(" src/terminal/Screen.zigRepository: manaflow-ai/ghostty
Length of output: 7069
Exercise reset with a live selection. switchScreen(.primary) clears the selection first, so t.screens.active.reset() only checks the already-clear path. Re-select immediately before reset and assert the token change required by Screen.reset().
🤖 Prompt for 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.
In `@src/terminal/Terminal.zig` around lines 2350 - 2351, Update the test around
t.screens.active.reset() to re-select a live selection immediately before
resetting, rather than relying on switchScreen(.primary), which clears it.
Assert that reset changes the selection activity token as required by
Screen.reset(), while preserving the existing previous-token comparison.
1 participant
Caps embedded terminal scrollback at 8 MiB per surface while retaining the current absolute-scroll-row restoration changes. This is the Ghostty dependency for manaflow-ai/cmux#7863.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Cap embedded surfaces’ scrollback via an embedder-owned limit (set to 8 MiB in
cmux) and harden the renderer path to avoid missed frames and reduce typing lag. Adds an optional renderer activity callback for lightweight update/draw instrumentation.New Features
ghostty_surface_new_with_scrollback_limit(...); the cap only lowers the configuredscrollback-limit. Getter:ghostty_surface_scrollback_limit_bytes(...).ghostty_renderer_event_cb) with events: UPDATE_FRAME_BEGIN/END and DRAW_FRAME_BEGIN/END; exposed asrenderer_event_cbonghostty_surface_config_s.Bug Fixes
Written for commit 14d4b0e. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes