Repository navigation
Bound read_text scrollback readback: decouple lines from scrollback - #6842
Closed
austinywang wants to merge 11 commits into
Closed
austinywang wants to merge 11 commits into
austinywang wants to merge 11 commits into
Conversation
surface.read_text with `lines` but no `scrollback` should stay viewport-bounded. The coordinator currently sets includeScrollback=true whenever `lines` is present, so a small bounded poll (e.g. the tmux-compat 4-line HUD probe) pulls and formats the entire terminal history on the main actor before trimming — the CPU/memory spike in #6500. This test fails on current code (records includeScrollback=true) and goes green with the decoupling fix in the next commit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…back surface.read_text forced includeScrollback=true whenever `lines` was present, so a bounded `--lines N` request (notably the tmux-compat 4-line HUD probe, which sends no `scrollback`) pulled and formatted the full terminal history on the main actor before trimming. Under large scrollback this spiked CPU and memory (PageFormatter.formatWithState on the main actor, overlapping the Ghostty renderer) — issue #6500. Treat `lines` and `scrollback` as orthogonal: `scrollback` selects history vs. viewport, `lines` bounds the tail. A read with `lines` but no `scrollback` now stays viewport-bounded (no full-history materialization). Every CLI path that wants a scrollback tail already sends `scrollback:true` alongside `--lines`, so `cmux read-screen` behavior is unchanged. Turns the previous commit's regression test green and adds a payload test proving the no-scrollback path reads only the viewport. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe coordinator now treats ChangesSurface read_text parameter handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (23 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 |
Contributor
Greptile SummaryThis PR fixes a CPU/memory spike (issue #6500) caused by
Confidence Score: 5/5Safe to merge — the fix is narrow, well-tested, and fully backward-compatible at the CLI level. The change removes a single incorrect coupling ( No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[surface.read_text RPC] --> B{context resolves\nTabManager?}
B -- No --> C[err: unavailable]
B -- Yes --> D[read scrollback param\ndefault: false]
D --> E[read lines param]
E --> F{lineLimit present\nand <= 0?}
F -- Yes --> G[err: invalid_params\nlines must be > 0]
F -- No --> H{includeScrollback?}
H -- true --> I[Read viewport +\nscreen + history + active]
H -- false --> J[Read viewport only]
I --> K{lineLimit set?}
J --> K
K -- Yes --> L[tailTerminalLines\nbefore encoding]
K -- No --> M[Return full text]
L --> M
M --> N[Return read result]
%%{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"}}}%%
flowchart TD
A[surface.read_text RPC] --> B{context resolves\nTabManager?}
B -- No --> C[err: unavailable]
B -- Yes --> D[read scrollback param\ndefault: false]
D --> E[read lines param]
E --> F{lineLimit present\nand <= 0?}
F -- Yes --> G[err: invalid_params\nlines must be > 0]
F -- No --> H{includeScrollback?}
H -- true --> I[Read viewport +\nscreen + history + active]
H -- false --> J[Read viewport only]
I --> K{lineLimit set?}
J --> K
K -- Yes --> L[tailTerminalLines\nbefore encoding]
K -- No --> M[Return full text]
L --> M
M --> N[Return read result]
Reviews (9): Last reviewed commit: "Localize read text lines validation" | Re-trigger Greptile |
Aziz test-framework policy: new and touched non-UI tests use Swift Testing (XCTest is reserved for cmuxUITests). The bounded-viewport regression added for #6500 landed in this XCTest file, so convert the whole (tiny) suite to `import Testing` / `@Test` / `#expect` rather than splitting one payload-behavior suite across two frameworks. Behavior and assertions are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Re: the CodeRabbit pre-merge Cmux Full Internationalization check flagging This is a false positive — the string is not added by this PR and cannot be localized at this layer:
So CodeRabbit's suggested resolution (add a (The Docstring-coverage warning is a repo-wide threshold artifact on the touched tiny test file and is non-blocking per CodeRabbit's own note.) |
This comment has been minimized.
This comment has been minimized.
Contributor
Author
CI analysis — the red
|
…ease-cpu-spike-from-surface
…fake Merging origin/main brought in a `controlSurfaceRoutingResolvesTabManager` override on FakeSurfaceControlCommandContext (returns `true`) alongside the one this branch added for the read_text tests, producing an invalid redeclaration that broke the CmuxControlSocketTests target. Keep main's method and the `readTextInvocation` recorder; drop this branch's duplicate method and the now-unused `readTextResolvesTabManager` flag (no test toggled it). Verified: `swift build --build-tests` compiles and all 11 ControlCommandCoordinatorSurfaceTests pass (read_text + dock + report_pwd). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ease-cpu-spike-from-surface
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+124202
to
+124218
| "rpc.v2.surface.read_text.linesMustBeGreaterThanZero": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "lines must be greater than 0" | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "lines は 0 より大きい値にしてください" | ||
| } | ||
| } | ||
| } | ||
| }, |
Contributor
There was a problem hiding this comment.
The touched Localizable.xcstrings catalog supports 21 locales (ar, bs, da, de, en, es, fr, it, ja, km, ko, nb, ok, pl, pt, pt-BR, px, ru, th, tr, uk), but the new rpc.v2.surface.read_text.linesMustBeGreaterThanZero key only provides en and ja entries. Users in any other locale will see the untranslated English fallback from defaultValue: when this error fires through String(localized:). Per the full-internationalization rule, every new catalog entry must cover all locales already present in the touched file.
Rule Used: Flag production user-facing text that is not fully... (source)
…ease-cpu-spike-from-surface
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #6500
Problem
Stable cmux intermittently spiked CPU (~96%) and memory (3.8 GB footprint, peak 5.6 GB) while servicing
surface.read_textreadback. The hot path wasPageFormatter.formatWithStaterunning on the main actor over the full terminal scrollback, overlapping the Ghostty renderer threads.Root cause is in the control coordinator's
surface.read_texthandler:This forced a full scrollback read whenever
lineswas present. A bounded--lines Nrequest would pull and format the entire terminal history first (theincludeScrollbackpath reads SCREEN + history + active — three full passes), then trim to N lines afterward.The most damaging caller is the internal tmux-compat HUD probe (
CMUXCLI+TmuxCompatSupport.swift), which pollssurface.read_textwith{"lines": 4}and noscrollback. Each 4-line poll materialized and formatted the whole history on the main actor — repeated polling against a large scrollback is exactly the reported spike.Fix
Treat
linesandscrollbackas orthogonal (issue direction #1):scrollbackselects history vs. visible viewport.linesbounds the returned tail.A read with
linesbut noscrollbacknow stays viewport-bounded — no full-history materialization. Callers that want a bounded scrollback tail pass both (thecmux read-screen --lines N/capture-paneCLIs already sendscrollback: truealongside--lines), so all CLI behavior is unchanged. The only behavioral change is at the v2 API boundary:lines-without-scrollbackbecomes a cheap viewport tail instead of a forced full-history read.Tests (two-commit red/green)
ControlCommandCoordinatorSurfaceTestsassertssurface.read_textwithlinesbut noscrollbackforwardsincludeScrollback: false(fails on current code, which forcestrue), plus coverage for thescrollback/lines/neither combinations and thelines <= 0rejection.terminalTextPayloadtest proving the no-scrollback bounded path reads only the viewport and never touches the (intentionally huge) screen/history snapshot fields.Scope / follow-up
This fully bounds the unintended full-history reads (the repeated-poll spike). An explicit
read-screen --scrollback --lines Nover very large scrollback still formats the requested history once on the Ghostty side; truly bounding that needs a Ghostty C-API change (a "tail N rows" selection — the CEXACTcoord currently clampsyto the viewport height, so the cmux side can't express it today). That's issue direction #2 and is a separate, heavier change.Localization
No user-facing strings added or changed. CLI help ("
--linesimplies--scrollback") stays accurate since the CLI still sendsscrollback: true. The socket error"lines must be greater than 0"is unchanged (machine-facing API error, consistent with sibling unlocalized socket errors).🤖 Generated with Claude Code
Summary by cubic
Decouples
linesfromscrollbackinsurface.read_textso--lines Nwithoutscrollbackreads only the viewport. Fixes #6500, removes CPU/memory spikes from bounded polls, localizes thelinesvalidation error via the app bundle, and keeps CLI behavior by makingread-screen --linesimply scrollback even if--scrollback falseis set.Bug Fixes
scrollback(history vs. viewport) andlines(tail bound) independently; stop forcing scrollback when onlylinesis set.lines-only reads; bounded scrollback tails still requirescrollback: true.read-screen --linesimpliesscrollback: trueand overrides--scrollback falseto match legacy behavior.lineswithinvalid_paramsand a localized message.Refactors
ControlSurfaceReadTextStringsandcontrolSurfaceReadTextStrings()to resolve validation strings in the app bundle.TerminalControllerTerminalTextTeststo SwiftTesting; added regression tests for param handling and the viewport-only path.Written for commit 672cb2d. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
read_textnow treatsscrollbackandlinesindependently, so requesting a line limit no longer forces scrollback to be included.linesvalues are now rejected with an invalid-parameters error.Tests
read_textparameter handling and scrollback behavior.