Repository navigation
fix(theme): refresh terminal colors via direct OSC probe - #1130
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Live terminal-color refresh (?997 notifications, SIGWINCH, and the mount probe) now writes Hunk's startup OSC 10/11/4 query plus a DA1 fence through the renderer and collects replies with OpenTUI's subscribeOsc, instead of clearPaletteCache() + getPalette(). A probe ends when the terminal has answered rather than after OpenTUI's 300 ms idle timeout, still picks up terminals answering after 300 ms, and a notification during an in-flight probe discards it and re-probes, so rapid switches no longer settle on a mix of old and new colors. OpenTUI's theme_mode event is an extra trigger. Probes skip the DA1 fence during OpenTUI's startup capability window, keep an owed fence after a timeout, and fall back to getPalette() if the renderer's private writeOut disappears.
elucid
force-pushed
the
fix/osc-terminal-color-refresh
branch
from
October 6, 2026 19:57
7f193a9 to
90fc0e0
Compare
elucid
marked this pull request as ready for review
October 7, 2026 13:36
Contributor
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+239
to
+243
| // During OpenTUI's startup capability window a DA1 reply may be OpenTUI's own, so probes there | ||
| // skip the fence and finish once every color is answered or on the timeout. | ||
| const fenced = fencesActive; | ||
| try { | ||
| write(fenced ? `${TERMINAL_COLOR_QUERY}${DEVICE_ATTRIBUTES_QUERY}` : TERMINAL_COLOR_QUERY); |
Contributor
There was a problem hiding this comment.
During the first five seconds, probes have no DA1 fence. If a probe times out while another refresh is queued, finish() immediately starts the next probe. Late colors from the old probe then reach the new subscriber. A complete old reply set closes that probe before its own replies arrive, leaving Hunk on the old palette.
Keep timed-out startup probes from feeding later probes, and add a test with a queued refresh and late startup replies.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/hunk/src/ui/theme/terminalColorWatcher.ts
Line: 239-243
Comment:
**Late replies restore old colors**
During the first five seconds, probes have no DA1 fence. If a probe times out while another refresh is queued, `finish()` immediately starts the next probe. Late colors from the old probe then reach the new subscriber. A complete old reply set closes that probe before its own replies arrive, leaving Hunk on the old palette.
Keep timed-out startup probes from feeding later probes, and add a test with a queued refresh and late startup replies.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
264
to
+268
| const schedule = () => { | ||
| if (disposed) return; | ||
| if (probe) probe.stale = true; | ||
| if (timer) clearTimeout(timer); | ||
| timer = setTimeout(() => void refresh(), REFRESH_DEBOUNCE_MS); | ||
| timer = setTimeout(requestProbe, debounceMs); |
Contributor
There was a problem hiding this comment.
schedule() restarts the debounce but leaves an earlier probeRequested set. If a queued refresh is waiting for DA1 and another notification arrives, that DA1 can call drain() before the new delay ends. Hunk sends one query immediately and another when the timer fires. This adds extra queries and can briefly apply colors before the intended settling delay ends.
Clear the pending request when restarting the debounce, or make drain() wait while timer is set.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/hunk/src/ui/theme/terminalColorWatcher.ts
Line: 264-268
Comment:
**Queued refresh skips the delay**
`schedule()` restarts the debounce but leaves an earlier `probeRequested` set. If a queued refresh is waiting for DA1 and another notification arrives, that DA1 can call `drain()` before the new delay ends. Hunk sends one query immediately and another when the timer fires. This adds extra queries and can briefly apply colors before the intended settling delay ends.
Clear the pending request when restarting the debounce, or make `drain()` wait while `timer` is set.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+173
to
+176
| await session.waitForText("export const answer = 42;"); | ||
| await waitForForeground(session, PALETTE_A.ansi[5]!, "export"); | ||
| await waitForForeground(session, PALETTE_A.foreground, "answer"); | ||
| return session; |
Contributor
There was a problem hiding this comment.
launchTestDiffSession() now waits for text and colors before returning the session to the caller's try/finally. If any wait fails, the caller never receives the session and cannot close it. Previously these checks were inside the test's try, so cleanup still ran. The new path can leave the Hunk process and terminal session running after a failed check.
Close the session in this helper's failure path before rethrowing.
Prompt To Fix With AI
This is a comment left during a code review.
Path: test/pty/terminal-theme-integration.test.ts
Line: 173-176
Comment:
**Failed checks leave sessions open**
`launchTestDiffSession()` now waits for text and colors before returning the session to the caller's `try/finally`. If any wait fails, the caller never receives the session and cannot close it. Previously these checks were inside the test's `try`, so cleanup still ran. The new path can leave the Hunk process and terminal session running after a failed check.
Close the session in this helper's failure path before rethrowing.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
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
1 participant
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.
Problem
#1128 made the
terminaltheme the default and refreshes it live. When the terminal sends a color-scheme notification (CSI ?997) or Hunk receivesSIGWINCH, it calls OpenTUI'sclearPaletteCache()+getPalette(). That covers @benvinegar's follow-up on #1128 ("detect the theme switch vs having to restart") and theSIGWINCHhook dhh added in omacom/omarchy#13518. Measured against a scripted PTY terminal that answers OSC 10/11/12 and OSC 4 but not OSC 13–19 (like most terminals and multiplexers), thegetPalette()path has three problems:Slow: every refresh costs about 300 ms. OpenTUI also queries OSC 12–19 and waits out its idle timeout for the ones nobody answers. Time from
?997to repaint was 419–437 ms.Misses slow terminals: when replies take 300 ms or more (SSH, busy multiplexers),
getPalette()returns all nulls and Hunk keeps the old colors. This happened in 18/18 runs at 300, 500, and 1000 ms.Mixed palette after rapid switches: Ghostty often sends a stale
?997first and answers it with the old colors, then switches and sends the real one 0–150 ms later. Hunk ended on the new palette in 0/108 of these races. Two things cause it:clearPaletteCache()does not cancel an in-flight detection, so the follow-up probe joins it.?997.The screen is left with the new foreground/background and the old 16-color palette.
What changed
watchTerminalColors(packages/hunk/src/ui/theme/terminalColorWatcher.ts) now probes the terminal directly:ESC[c) through the renderer's frame-ordered output.renderer.subscribeOsc()and parses them withparseTerminalColorReplies.?997,SIGWINCH, and one mount probe when startup found no colors. The debounce stays at 100 ms.getPalette()refresh path is gone.SIGWINCH.setupTerminal(), plus a DCS-wrapped retry inside tmux once XTVERSION answers. It stops listening for capability replies 5 s later. So for 5 s after mount the watcher can't tell whether a DA1 reply is OpenTUI's. During that window, probes skip DA1 and finish once every color is answered or on the timeout. OpenTUI's DA1 replies are never swallowed.theme_modetrigger. OpenTUI'stheme_modeevent is now also a trigger, for when Hunk's startup probe came up empty and OpenTUI later learns the colors. It only requests a probe and never discards one in flight.writeOut, with a fallback. OpenTUI 0.5.6 declareswriteOutprivate, and there is no public raw-write API.BufferedBackend.writeOutinrenderer-output.zig), which a directstdout.writedoes not.rendererRawWriter(). If a future OpenTUI removeswriteOut, the watcher falls back toclearPaletteCache()+getPalette(), so an upgrade makes refresh slower instead of breaking it.core/theme/detection.tsnow exports the color query (TERMINAL_COLOR_QUERY),DEVICE_ATTRIBUTES_QUERY,isDeviceAttributesReply, andhasTerminalColors. The startup probe and the watcher share them (ui → core only;deps:checkis clean).docs/themes.mdand the website themes page, and added a patch changeset.This makes the existing
SIGWINCH/?997refresh faster and more reliable, in both regular and piped (patch -,pager) modes. It adds no new way for a terminal to signal a switch.Results
The scripted fake PTY terminal is
/tmp/theme-exp-osc(OpenTUI 0.5.6). It answers OSC 10/11/12 and OSC 4 but not OSC 13–19. Modes:diffwith TTY stdin,patch -piped,pagerpiped. Branch runs were taken both inside OpenTUI's 5 s startup window (probes without DA1) and after it (probes with DA1).getPalette)?997→ repaint, instant replies (12 switches)?997race (d1 50–150 ms × d2 0–150 ms × 3 transitions × 3 modes)?997?997while a probe is in flight (250 ms replies)themeMode/theme_modeReal Ghostty (macOS, light/dark
GitHub Light/Dark Default, system appearance toggled via System Events, two Dark→Light→Dark cycles per mode):diff(TTY stdin)patch -pipedgetPalette().?997pattern appeared (for example997;1then997;2), and the watcher still ended on the right colors every time.Tests
Unit (
terminalColorWatcher.test.ts):getPalette()fallback whenwriteOutis missingtheme_modetriggerrendererRawWriterPTY (
test/pty/terminal-theme-integration.test.ts; the harness'slaunchShellCommandcan now answer terminal queries):diffmodepatch -mode?997race ends on the new paletteOn
origin/mainthe race and slow-terminal tests fail; they pass on this branch.Verification
bun run typecheckbun run lintbun run deps:checkbun run check:docsbun run format:checkbun run testJujutsu source reading > logs unexpected source failures…— fails the same way onorigin/main(6c95708) in this environmentbun run test:integrationPTY extensionstrust prompt ×2 andPTY lifecycleSIGHUP/SIGQUIT/SIGPIPE — all fail the same way onorigin/mainhere (bun test test/pty/extensions-integration.test.ts test/pty/lifecycle.test.ts); all theme PTY tests passbun run test:tty-smokescript, which isn't available hereNot tested
?997only after refreshing its colors.Follow-ups (out of scope)
?997norSIGWINCH.--theme auto, which fixes its theme id at startup.getPalette(), would remove the privatewriteOutcast;themeModestays stale when replies take more than 250 ms (fix(core): honor late OSC replies + bound re-query on CSI ?997 (runtime theme switching on non-2031 terminals) anomalyco/opentui#1213 is related).