Repository navigation
Fix restored scrollback keeping original theme's colors (white-on-white after theme change) - #5175
Conversation
Session scrollback is captured via Ghostty's `write_screen_file:copy,vt` export, which prepends terminal-color OSC sequences (OSC 10/11) that bake the capture-time theme's default foreground/background. Replaying those on restore reconfigures the freshly launched terminal's dynamic colors, so restored default-colored cells keep the old theme after a theme change — rendering as unreadable white-on-white scrollback (#5165). This regression test asserts that the scrollback replay path strips terminal-color OSC sequences while preserving explicit SGR colors and non-color escape sequences (e.g. OSC 8 hyperlinks). It fails until the fix lands. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
When cmux restores the previous session's scrollback, the captured VT export bakes the capture-time theme by prepending OSC 10/11 (and the other dynamic terminal-color OSC sequences). Replaying that into a freshly launched terminal overrode the active theme's default foreground/background, so restored default-colored cells kept the original theme. After a theme change this rendered as unreadable white-on-white scrollback. Strip terminal-color OSC sequences (palette + dynamic fg/bg/cursor/ highlight colors and their resets: OSC 4/5/10-19/104/105/110-119) from the captured scrollback before replay, so the active theme owns the default colors and restored default-colored cells track it. Explicit per-cell SGR colors and every non-color escape sequence (titles, hyperlinks, prompt marks) are preserved verbatim. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughStrips OSC 10/11/12 terminal-color sequences from captured scrollback during normalization and adds a regression test verifying replay output removes those sequences while preserving SGR colors, OSC 8 hyperlinks, and plain text. ChangesScrollback Replay Theme Color Stripping
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 17 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (17 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 SummaryFixes the white-on-white scrollback regression (#5165) by stripping terminal-color OSC sequences (OSC 4/5/10–19/104/105/110–119) from captured scrollback before replay, so restored default-colored cells track the active theme rather than the capture-time theme. The stripping is applied at replay time, so existing on-disk snapshots are corrected without re-capture.
Confidence Score: 5/5Safe to merge — the change is a pure additive text filter in the replay pipeline with no side effects on session state, storage, or concurrency. The OSC stripping logic is linear, handles both BEL and ST terminators, correctly falls through on non-color sequences, and drops only well-defined color-reconfiguring OSC command numbers. The regression test exercises the full round-trip through replayEnvironment, confirming the filter is wired correctly. No actor isolation, blocking primitives, or state-representation issues were introduced. Sources/SessionPersistence.swift is now over 2100 lines; the new OSC parsing helpers are self-contained and could be extracted, but this does not affect correctness. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[normalizedScrollback] --> B{non-nil and non-whitespace?}
B -- No --> Z[return nil]
B -- Yes --> C[strippingTerminalColorOSCSequences]
C --> D{byte is ESC and next is OSC introducer?}
D -- No --> E[append byte, advance index]
E --> D
D -- Yes --> F[parse numeric OSC code]
F --> G{isTerminalColorOSCCode?}
G -- No --> H[emit ESC advance by 1 - preserve sequence verbatim]
H --> D
G -- Yes --> I[scan for BEL or ST terminator]
I --> J{terminated?}
J -- Yes --> K[drop entire OSC sequence]
J -- No --> L[drop to end of buffer]
K --> D
L --> M[return stripped String]
M --> N[truncatedScrollback]
N --> O[ansiSafeReplayText]
O --> P[write replay file]
Reviews (2): Last reviewed commit: "Broaden restored-scrollback OSC test to ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| func testScrollbackReplayStripsThemeBakedDefaultColorOSCSequences() { | ||
| let tempDir = FileManager.default.temporaryDirectory | ||
| .appendingPathComponent("cmux-scrollback-replay-\(UUID().uuidString)", isDirectory: true) | ||
| try? FileManager.default.createDirectory(at: tempDir, withIntermediateDirectories: true) | ||
| defer { try? FileManager.default.removeItem(at: tempDir) } | ||
|
|
||
| let esc = "\u{001B}" | ||
| // Captured under a dark theme: default fg baked white, default bg baked dark. | ||
| let setForeground = "\(esc)]10;rgb:ff/ff/ff\(esc)\\" | ||
| let setBackground = "\(esc)]11;rgb:28/2c/34\(esc)\\" | ||
| // A BEL-terminated cursor-color OSC, the other dynamic-color terminator form. | ||
| let setCursor = "\(esc)]12;rgb:c0/c1/b5\u{0007}" | ||
| let red = "\(esc)[31m" | ||
| let reset = "\(esc)[0m" | ||
| // OSC 8 hyperlinks are scrollback content, not terminal color config; keep them. | ||
| let hyperlink = "\(esc)]8;;https://example.com\(esc)\\link\(esc)]8;;\(esc)\\" | ||
| let source = "\(setForeground)\(setBackground)\(setCursor)plain default text\n" | ||
| + "\(red)RED\(reset) \(hyperlink)\n" | ||
|
|
||
| let environment = SessionScrollbackReplayStore.replayEnvironment( | ||
| for: source, | ||
| tempDirectory: tempDir | ||
| ) | ||
|
|
||
| guard let path = environment[SessionScrollbackReplayStore.environmentKey] else { | ||
| XCTFail("Expected replay file path") | ||
| return | ||
| } | ||
| guard let contents = try? String(contentsOfFile: path, encoding: .utf8) else { | ||
| XCTFail("Expected replay file contents") | ||
| return | ||
| } | ||
|
|
||
| // Terminal-color OSC sequences must be stripped so the active theme owns | ||
| // default fg/bg/cursor and restored default cells track it. | ||
| XCTAssertFalse(contents.contains("\(esc)]10;"), "OSC 10 (set foreground) must be stripped") | ||
| XCTAssertFalse(contents.contains("\(esc)]11;"), "OSC 11 (set background) must be stripped") | ||
| XCTAssertFalse(contents.contains("\(esc)]12;"), "OSC 12 (set cursor color) must be stripped") | ||
| XCTAssertFalse(contents.contains("rgb:ff/ff/ff"), "baked default-color payload must be gone") | ||
|
|
||
| // Explicit SGR colors, plain text, and hyperlinks are preserved verbatim. | ||
| XCTAssertTrue(contents.contains("plain default text")) | ||
| XCTAssertTrue(contents.contains("\(red)RED\(reset)")) | ||
| XCTAssertTrue(contents.contains(hyperlink), "non-color OSC sequences must be preserved") | ||
| } |
There was a problem hiding this comment.
CLAUDE.md requires both commits in the PR: "Commit 1: Add the failing test only … Commit 2: Add the fix." This PR contains only commit 1. SessionScrollbackReplayStore.ansiSafeReplayText in Sources/SessionPersistence.swift is unchanged; it only prepends/appends ESC[0m and never strips OSC sequences. As written, the test will fail on every CI run and the white-on-white regression remains unfixed for users who merge this branch.
| XCTAssertFalse(contents.contains("\(esc)]10;"), "OSC 10 (set foreground) must be stripped") | ||
| XCTAssertFalse(contents.contains("\(esc)]11;"), "OSC 11 (set background) must be stripped") | ||
| XCTAssertFalse(contents.contains("\(esc)]12;"), "OSC 12 (set cursor color) must be stripped") | ||
| XCTAssertFalse(contents.contains("rgb:ff/ff/ff"), "baked default-color payload must be gone") | ||
|
|
||
| // Explicit SGR colors, plain text, and hyperlinks are preserved verbatim. | ||
| XCTAssertTrue(contents.contains("plain default text")) | ||
| XCTAssertTrue(contents.contains("\(red)RED\(reset)")) | ||
| XCTAssertTrue(contents.contains(hyperlink), "non-color OSC sequences must be preserved") |
There was a problem hiding this comment.
The PR description explicitly commits to stripping OSC 4, 5, 13–19, 104, 105, and 110–119 in addition to 10–12, but the test only asserts on 10, 11, and 12. When the fix lands, none of those additional sequences will be regression-covered. At minimum, representative assertions for the palette group (OSC 4), its reset (OSC 104), and one from the 110–119 reset block should be added so the test actually guards the full claimed scope.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![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 `@cmuxTests/SessionPersistenceTests.swift`:
- Around line 505-542: The test only verifies OSC 10/11/12 are stripped; update
the test in SessionPersistenceTests.swift (around the
SessionScrollbackReplayStore.replayEnvironment usage) to include additional OSC
sequences for palette set/reset (OSC 4/5 and their resets 104/105) and
dynamic-color resets (OSC 110–119) in the source string, then add XCTAssertFalse
checks that the replay contents do not contain those OSC prefixes or their
payloads (e.g. "\(esc)]4;", "\(esc)]5;", "\(esc)]104;", "\(esc)]105;", and
"\(esc)]11"‑style 110–119 prefixes and any sample color payloads), while keeping
the existing assertions that SGR colors, plain text, and non-color OSC
hyperlinks are preserved.
🪄 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: d62ac3ef-4417-46ca-8835-81c65cf192f3
📒 Files selected for processing (1)
cmuxTests/SessionPersistenceTests.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmuxTests/SessionPersistenceTests.swift">
<violation number="1" location="cmuxTests/SessionPersistenceTests.swift:518">
P0: This test calls `SessionScrollbackReplayStore.replayEnvironment(for:tempDirectory:)` and asserts that OSC color sequences are stripped, but the implementation in `SessionScrollbackReplayStore` does not appear to have been modified in this PR to actually perform the stripping. As-is, this test will fail on CI and the white-on-white regression remains unfixed for users.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| let source = "\(setForeground)\(setBackground)\(setCursor)plain default text\n" | ||
| + "\(red)RED\(reset) \(hyperlink)\n" | ||
|
|
||
| let environment = SessionScrollbackReplayStore.replayEnvironment( |
There was a problem hiding this comment.
P0: This test calls SessionScrollbackReplayStore.replayEnvironment(for:tempDirectory:) and asserts that OSC color sequences are stripped, but the implementation in SessionScrollbackReplayStore does not appear to have been modified in this PR to actually perform the stripping. As-is, this test will fail on CI and the white-on-white regression remains unfixed for users.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmuxTests/SessionPersistenceTests.swift, line 518:
<comment>This test calls `SessionScrollbackReplayStore.replayEnvironment(for:tempDirectory:)` and asserts that OSC color sequences are stripped, but the implementation in `SessionScrollbackReplayStore` does not appear to have been modified in this PR to actually perform the stripping. As-is, this test will fail on CI and the white-on-white regression remains unfixed for users.</comment>
<file context>
@@ -486,6 +486,62 @@ final class SessionPersistenceTests: XCTestCase {
+ let source = "\(setForeground)\(setBackground)\(setCursor)plain default text\n"
+ + "\(red)RED\(reset) \(hyperlink)\n"
+
+ let environment = SessionScrollbackReplayStore.replayEnvironment(
+ for: source,
+ tempDirectory: tempDir
</file context>
Regression test verification (local)The CI
So the test genuinely catches the bug. |
Addresses review feedback: the regression test only asserted OSC 10/11/12 were stripped, but the replay-time contract also covers palette set/reset (OSC 4/5/104/105) and the dynamic-color resets (OSC 110-119). Add representative coverage — OSC 4 (palette set), OSC 104 (palette reset), and OSC 110 (foreground reset) — so an implementation that missed those would fail the test. SGR colors, plain text, and OSC 8 hyperlinks remain asserted as preserved. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks for the reviews. Addressed:
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cmuxTests/SessionPersistenceTests.swift (1)
511-547:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winExpand this regression to the remaining OSC families.
This now covers
OSC 4/104/110, but the replay contract for this PR also includesOSC 5/105and the rest of the10...19/110...119ranges. An implementation that strips only the currently-sampled opcodes would still pass here.🤖 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 `@cmuxTests/SessionPersistenceTests.swift` around lines 511 - 547, The test currently asserts stripping for OSC families 4/104/110 only; update the assertions in SessionPersistenceTests (around the SessionScrollbackReplayStore.replayEnvironment usage and the local variable contents) to also assert that OSC 5 and 105 and the full 10...19 and 110...119 ranges are stripped—e.g., add XCTAssertFalse checks for sequences like "\(esc)]5;", "\(esc)]105;", and "\(esc)]1", "\(esc)]19;", "\(esc)]110;", "\(esc)]119;" (and any baked payloads such as color/rgb tokens associated with those ranges) so the replay contract covers all related OSC families rather than just the sampled opcodes.
🤖 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.
Duplicate comments:
In `@cmuxTests/SessionPersistenceTests.swift`:
- Around line 511-547: The test currently asserts stripping for OSC families
4/104/110 only; update the assertions in SessionPersistenceTests (around the
SessionScrollbackReplayStore.replayEnvironment usage and the local variable
contents) to also assert that OSC 5 and 105 and the full 10...19 and 110...119
ranges are stripped—e.g., add XCTAssertFalse checks for sequences like
"\(esc)]5;", "\(esc)]105;", and "\(esc)]1", "\(esc)]19;", "\(esc)]110;",
"\(esc)]119;" (and any baked payloads such as color/rgb tokens associated with
those ranges) so the replay contract covers all related OSC families rather than
just the sampled opcodes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d984a266-ef24-4eac-857f-1d5e744be8ab
📒 Files selected for processing (1)
cmuxTests/SessionPersistenceTests.swift
This branch was successfully deployed
1 participant
Summary
Fixes #5165. When cmux restores the previous session's scrollback on launch, restored history kept the default foreground/background baked from the theme it was created under instead of tracking the currently active theme. After a theme change this rendered restored default-colored cells as unreadable white-on-white.
Root cause
Session scrollback is captured via Ghostty's
write_screen_file:copy,vtexport (TerminalController.readTerminalTextFromVTExportForSnapshot). That export is told to bake the live theme —Surface.writeScreenFilepasses.foreground,.background, and.paletteto the VT formatter, which prependsOSC 10/OSC 11dynamic-color sequences carrying the capture-time default fg/bg (and resolves palette entries to literal RGB).On restore, cmux replays the captured scrollback by
cat-ing it into the freshly launched shell (SessionScrollbackReplayStore→CMUX_RESTORE_SCROLLBACK_FILE→ shell integration). ReplayingOSC 10/OSC 11reconfigures the live terminal's dynamic colors, overriding the active theme — so default-colored cells in restored history render with the old theme's default fg/bg (white-on-white when the new theme has a contrasting background).Empirically confirmed (captured under dark Dracula, restored under a light theme); the snapshot scrollback begins with:
…and the default-colored restored lines (shell prompt, command output) render invisible (white-on-white) under the light theme.
Fix
Strip terminal-color OSC sequences from the captured scrollback before replay (
SessionScrollbackReplayStore). The active theme owns the terminal's default foreground/background/cursor/palette, so restored history must not carry sequences that reconfigure them. Stripped: OSC4/5/104/105(palette/special set+reset),10–19(dynamic fg/bg/cursor/highlight/…),110–119(their resets). Explicit per-cell SGR colors and every non-color escape (titles, OSC 8 hyperlinks, OSC 133 prompt marks, …) are preserved verbatim.This is done at replay time, so it also repairs scrollback captured by older builds already on disk.
After the fix, restored default-colored cells track the active theme (dark-on-white under a light theme); the white-on-white is gone.
Reproduction / verification
OSC 10/OSC 11.Tests
Two-commit structure per
CLAUDE.md:SessionPersistenceTests.testScrollbackReplayStripsThemeBakedDefaultColorOSCSequencesasserts the replay path strips terminal-color OSC sequences while preserving SGR colors and non-color escapes. Fails without the fix.Added to the already-wired
cmuxTests/SessionPersistenceTests.swift(XCTest, matching its sibling replay tests). Verified locally withxcodebuild -scheme cmux-unit -only-testing:cmuxTests/SessionPersistenceTests/testScrollbackReplayStripsThemeBakedDefaultColorOSCSequences→** TEST SUCCEEDED **. E2E/UI left to CI per repo policy.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Note
Low Risk
Localized replay-path text sanitization with a focused regression test; no auth, persistence schema, or network changes.
Overview
Fixes #5165 by sanitizing captured session scrollback before replay so restored default-colored text follows the active theme instead of the theme baked in at capture time.
SessionScrollbackReplayStorenow runs scrollback throughstrippingTerminalColorOSCSequencesahead of truncation and ANSI-safe wrapping. That removes OSC sequences that set or reset terminal colors (palette4/5/104/105, dynamic fg/bg/cursor10–19, resets110–119), including BEL- and ST-terminated forms, while leaving SGR colors and other escapes (e.g. OSC 8 hyperlinks) intact. Because this runs at replay time, existing on-disk snapshots are corrected without re-capture.Adds regression test
testScrollbackReplayStripsThemeBakedDefaultColorOSCSequencesinSessionPersistenceTests.Reviewed by Cursor Bugbot for commit 00a0390. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes #5165 by stripping terminal-color OSC sequences during restored scrollback replay so default-colored text uses the active theme, avoiding white-on-white after theme changes. Done in the replay path so existing snapshots on disk are fixed too.
ESC \) terminators; drop truncated sequences.testScrollbackReplayStripsThemeBakedDefaultColorOSCSequencesto cover palette set/reset and dynamic‑color resets, ensuring only theme-reconfiguring OSC are removed while text, SGR, and hyperlinks remain.Written for commit 00a0390. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests