Repository navigation
Fix restored scrollback keeping original theme's colors (white-on-white after theme change) #5175
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -486,6 +486,72 @@ final class SessionPersistenceTests: XCTestCase { | |
| XCTAssertTrue(contents.hasSuffix(reset)) | ||
| } | ||
|
|
||
| // Regression for https://github.com/manaflow-ai/cmux/issues/5165. | ||
| // | ||
| // Ghostty's `write_screen_file:copy,vt` export (used to capture session | ||
| // scrollback) prepends OSC 10 / OSC 11 sequences that bake the capture-time | ||
| // theme's default foreground/background. Replaying those into a freshly | ||
| // launched terminal reconfigures the live terminal's dynamic colors, so | ||
| // restored default-colored cells keep the OLD theme instead of tracking the | ||
| // active one — producing white-on-white scrollback after a theme change. | ||
| // The active theme owns default fg/bg, so the restored history must not carry | ||
| // these terminal-color OSC sequences. | ||
| 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}" | ||
| // Palette set/reset and a dynamic-color reset are equally theme state that | ||
| // restored history must not re-impose, so they are stripped too. | ||
| let setPalette = "\(esc)]4;1;rgb:aa/00/00\(esc)\\" | ||
| let resetPalette = "\(esc)]104;1\(esc)\\" | ||
| let resetForeground = "\(esc)]110;\(esc)\\" | ||
| 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)" | ||
| + "\(setPalette)\(resetPalette)\(resetForeground)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("\(esc)]4;"), "OSC 4 (set palette entry) must be stripped") | ||
| XCTAssertFalse(contents.contains("\(esc)]104;"), "OSC 104 (reset palette entry) must be stripped") | ||
| XCTAssertFalse(contents.contains("\(esc)]110;"), "OSC 110 (reset foreground) must be stripped") | ||
| XCTAssertFalse(contents.contains("rgb:ff/ff/ff"), "baked default-color payload must be gone") | ||
| XCTAssertFalse(contents.contains("rgb:aa/00/00"), "baked palette 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") | ||
|
Comment on lines
+540
to
+552
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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] marked this conversation as resolved.
|
||
| } | ||
|
Comment on lines
+499
to
+553
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| func testSessionScrollbackPersistenceHonorsReportedShellState() { | ||
| XCTAssertTrue( | ||
| Workspace.shouldPersistSessionScrollback( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P0: This test calls
SessionScrollbackReplayStore.replayEnvironment(for:tempDirectory:)and asserts that OSC color sequences are stripped, but the implementation inSessionScrollbackReplayStoredoes 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