Skip to content

fix: render OSC 4 palette overrides instead of forwarding the index - #2162

Open
hamidi-dev wants to merge 1 commit into
herdrdev:masterfrom
hamidi-dev:fix/osc4-palette-rendering
Open

fix: render OSC 4 palette overrides instead of forwarding the index#2162
hamidi-dev wants to merge 1 commit into
herdrdev:masterfrom
hamidi-dev:fix/osc4-palette-rendering

Conversation

@hamidi-dev

Copy link
Copy Markdown

What's broken

A cell carrying a palette index is emitted to the host terminal as that index, so the host resolves it against its own palette. When a program in a pane redefines an entry with OSC 4, libghostty parses and stores it — palette_color_query_response even answers OSC 4 queries with the new value — but the redefinition never reaches the screen.

Repro

printf '\033]4;18;rgb:a9/b1/d6\033\\\033[38;5;18mshould be lavender\033[0m\n'

In a herdr pane this renders in the host's colour for index 18; outside herdr it's lavender.

Found via hamidi-dev/opentab#12, reported by SimonEisenhauer — a curses TUI that themes itself with init_color. Every theme rendered identically inside herdr while other TUIs looked fine, because those emit truecolor SGR and take the CellColor::Rgb path. I've shipped a workaround on my side, but since this is the root cause I thought I'd submit a PR to fix it here too.

Fix

Compare the render state's active palette against COLOR_PALETTE_DEFAULT to find exactly the entries a program overrode, and emit those as RGB. Every other index is still forwarded, so panes that never use OSC 4 render as before and keep following the host terminal's theme.

Resolving every index would be simpler, but it would replace the host's palette for users whose terminal doesn't answer herdr's colour queries — their theme would silently become libghostty's default. Only resolving real overrides can't regress anyone.

@coderabbitai

coderabbitai Bot commented Aug 1, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The change adds Ghostty default palette retrieval and applies only OSC 4-modified palette entries during full-frame and dirty-patch rendering. Changed indexed colors become RGB colors. Unchanged indexed and direct RGB colors retain their existing representation.

Changes

Palette-aware terminal rendering

Layer / File(s) Summary
Default palette retrieval
src/ghostty/mod.rs
Terminal::default_palette retrieves Ghostty’s 256-entry default palette and propagates errors.
Palette override conversion
src/pane/terminal.rs
PaletteOverrides records active entries that differ from defaults. Indexed foreground, background, and underline colors use RGB values only for those entries.
Rendering integration and validation
src/pane/terminal.rs
Full-frame and dirty-patch rendering pass palette overrides into cell-style conversion. Tests cover changed entries, unchanged entries, absent overrides, and direct RGB colors.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Renderer
  participant Terminal
  participant Ghostty
  participant PaletteOverrides
  Renderer->>Terminal: request default_palette()
  Terminal->>Ghostty: query default 256-entry palette
  Ghostty-->>Terminal: return palette or error
  Renderer->>PaletteOverrides: compare active and default palettes
  PaletteOverrides-->>Renderer: return changed palette entries
  Renderer->>Renderer: convert changed indexed colors to RGB
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the fix for rendering OSC 4 palette overrides instead of forwarding palette indices.
Description check ✅ Passed The description explains the OSC 4 palette rendering bug, reproduction case, root cause, and selective RGB conversion fix.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@kangal-bot kangal-bot added the ai-review Trigger automated AI reviews for pull requests admitted by the PR gate label Aug 1, 2026
@greptile-apps

greptile-apps Bot commented Aug 1, 2026

Copy link
Copy Markdown

Greptile Summary

This PR renders palette indices changed through OSC 4 as explicit RGB values while continuing to forward untouched indices to the host terminal.

  • Adds a safe wrapper for retrieving libghostty's default 256-color palette.
  • Compares active and default palettes during full and incremental rendering.
  • Resolves overridden foreground, background, and underline palette colors to RGB.
  • Adds focused tests for overridden, untouched, and direct-RGB colors.

Confidence Score: 5/5

The PR appears safe to merge, with palette overrides correctly resolved and palette-only changes forcing a redraw.

The new FFI getter matches libghostty's documented array contract, active/default comparison isolates runtime overrides, and all relevant cell-style and redraw paths apply the new resolution.

Important Files Changed

Filename Overview
src/ghostty/mod.rs Adds an ABI-compatible getter for the terminal's default 256-entry RGB palette.
src/pane/terminal.rs Detects runtime palette overrides, resolves affected indexed cell colors to RGB across full and dirty rendering, and adds focused unit tests.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Cell palette index] --> B{Active entry differs<br/>from default palette?}
  B -- Yes: OSC 4 override --> C[Emit explicit RGB]
  B -- No --> D[Emit indexed color]
  D --> E[Host terminal resolves theme color]
Loading

Reviews (1): Last reviewed commit: "fix: render OSC 4 palette overrides inst..." | Re-trigger Greptile

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/pane/terminal.rs (1)

3088-3135: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add integration coverage for both rendering paths.

The tests validate PaletteOverrides and ghostty_cell_color in isolation. They do not exercise Terminal::default_palette(), render(), or collect_dirty_patch(). Add one full-frame and one dirty-patch test for an OSC 4 override. Assert that the changed index renders as Color::Rgb and an unchanged index remains Color::Indexed.


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 0f5002ed-7923-4e50-b266-f18df9c006a3

📥 Commits

Reviewing files that changed from the base of the PR and between 26a7bc8 and ceb6994.

📒 Files selected for processing (2)
  • src/ghostty/mod.rs
  • src/pane/terminal.rs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Trigger automated AI reviews for pull requests admitted by the PR gate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants