feat(themes): follow macOS Appearance and auto-switch dark/light#32
feat(themes): follow macOS Appearance and auto-switch dark/light#32panosAthDBX wants to merge 10 commits into
Conversation
There was a problem hiding this comment.
inspect review
Triage: 8 entities analyzed | 0 critical, 0 high, 2 medium, 6 low
Verdict: standard_review
Findings (1)
- [low] Config test writes config.json into a directory that is never created, so
Bun.write()will fail with ENOENT beforeloadConfig()is exercised. Evidence:const configDir = join(tmpDir, ".config", "opensessions"); await Bun.write(join(configDir, "config.json"), ...)with nomkdir/ensureDir forconfigDir.
Reviewed by inspect | Entity-level triage found 0 high-risk changes
|
Pushed a follow-up fix on top of the original commit: fix(themes): persist manual override per system appearance The original implementation had a bug where, with auto-follow on, a manual Changes:
All 374 tests still pass. |
There was a problem hiding this comment.
inspect review
Triage: 11 entities analyzed | 0 critical, 0 high, 3 medium, 8 low
Verdict: standard_review
Findings (2)
- [low] Test bug: config round-trip test writes to a config path whose parent directories are never created, so
Bun.write(join(configDir, "config.json"), ...)can fail before assertions. Evidence:const configDir = join(tmpDir, ".config", "opensessions"); await Bun.write(join(configDir, "config.json"), ...)with nomkdirforconfigDir. - [low] Test bug: system-theme.test attempts to redefine
process.platformviaObject.defineProperty. In many runtimes this property is non-configurable/read-only, so redefining it can throw and fail the test. Evidence:Object.defineProperty(process, "platform", { value: "linux", configurable: true });and later restoring similarly.
Reviewed by inspect | Entity-level triage found 0 high-risk changes
Adds an opt-in background poller that reads the macOS Appearance setting (`defaults read -g AppleInterfaceStyle`) every few seconds and flips the active theme between a configured dark and light pair when the user toggles System Settings → Appearance. Config: autoThemeFollowsSystem: true darkTheme: "catppuccin-mocha" (default) lightTheme: "catppuccin-latte" (default) macOS-gated — no-op on Linux/Windows. Uses the same broadcast path as the manual theme picker so every connected sidebar re-renders in sync. Poller is a 3s interval cleaned up on shutdown. Extracted pure mapping and side-effectful read into `system-theme.ts` for testability. Covered by 5 new tests.
When `autoThemeFollowsSystem` is on and the user manually picks a theme via
`set-theme`, route the persistence to `darkTheme` or `lightTheme` based on the
current macOS appearance instead of `theme`. Previously the choice was written
to `theme`, which the poll loop ignored, so the manual override was silently
overwritten on the next 3s tick.
Also re-load `darkTheme` / `lightTheme` from disk inside the poll cycle so an
override made via `set-theme` is picked up immediately on the next poll.
Drops the `saveConfig({ theme: desired })` write inside the poll loop — that
was clobbering the user's static-mode `theme` field whenever auto-follow ran.
Co-authored-by: Isaac
723ff01 to
20b32fa
Compare
There was a problem hiding this comment.
inspect review
Triage: 11 entities analyzed | 0 critical, 0 high, 3 medium, 8 low
Verdict: standard_review
Findings (2)
- [low] Test setup bug: config round-trip test writes to a non-existent directory. In
packages/runtime/test/config.test.ts,configDiris computed but never created beforeawait Bun.write(join(configDir, "config.json"), ...), which will fail with ENOENT in typical filesystem behavior. - [low] Test expectation/name mismatch:
packages/runtime/test/system-theme.test.tshastest("returns 'light' on non-darwin without throwing"...)but the assertion isexpect(["dark", "light"]).toContain(result);, which does not actually verify it returns 'light' on non-darwin and can mask incorrect behavior.
Reviewed by inspect | Entity-level triage found 0 high-risk changes
Two server hot-path optimizations targeting steady-state CPU and WS fan-out:
1. Replace the 3s `defaults read` polling loop with a kqueue-based file
watch on `~/Library/Preferences/.GlobalPreferences.plist`. macOS rewrites
that plist on any global-preference change, so we let the kernel push us
the event and re-read appearance only when the value differs. A 60s
safety poll covers the atomic-rename case where kqueue loses the inode.
Removes ~28,800 subprocess spawns per server per day.
2. Hash the serialized state in `broadcastStateImmediate()` and skip the
`server.publish("sidebar", msg)` when the payload is byte-identical to
the previous send. Many call sites trigger broadcastState() but most do
not actually change observable state (theme polls, focus moves to the
same session, agent updates with identical metadata). Wire protocol is
unchanged; new clients still receive `lastState` directly via the WS
`open` handler.
Tests: 400/400 passing (was 397; +3 covering the watcher's no-op fallback,
idempotent stop, and darwin initial-fire).
There was a problem hiding this comment.
inspect review
Triage: 20 entities analyzed | 0 critical, 0 high, 4 medium, 16 low
Verdict: standard_review
Findings (2)
- [low] Config test likely fails: writes config.json into a directory that is never created. In packages/runtime/test/config.test.ts,
const configDir = join(tmpDir, ".config", "opensessions"); await Bun.write(join(configDir, "config.json"), ...)is called without creatingconfigDir(no mkdirp). This can throw ENOENT and fail the test suite. - [low] watchMacSystemAppearance can invoke onChange after stop(): in packages/runtime/src/system-theme.ts,
check()only checksstoppedbefore awaitingreadMacSystemAppearance(). Ifstop()is called whilereadMacSystemAppearance()is in-flight,check()can still proceed toawait onChange(mode)after the await, causing callbacks/state updates after cleanup.
Reviewed by inspect | Entity-level triage found 0 high-risk changes
`watcherBroadcastTimer` was referenced in `cleanup()` but never declared, so every cleanup invocation (SIGINT/SIGTERM/30s idle timeout) crashed with `ReferenceError: watcherBroadcastTimer is not defined` instead of shutting down gracefully. Process exited anyway, so the bug was latent. Just drop the dead line.
There was a problem hiding this comment.
inspect review
Triage: 20 entities analyzed | 0 critical, 0 high, 4 medium, 16 low
Verdict: standard_review
Findings (1)
- [low] Test bug:
loadConfig round-trips auto-theme fieldswrites/tmp/.../.config/opensessions/config.jsonwithout creating the parent directories first, soawait Bun.write(join(configDir, "config.json"), ...)will fail with ENOENT and the test will error instead of exercisingloadConfig.
Evidence:const configDir = join(tmpDir, ".config", "opensessions"); await Bun.write(join(configDir, "config.json"), JSON.stringify(...));with nomkdir/mkdirSyncforconfigDir.
Reviewed by inspect | Entity-level triage found 0 high-risk changes
The ensure-sidebar HTTP handler currently triggers two `tmux list-panes -a` calls per request: 1. ensureSidebarInWindow() lists panes to check for an existing sidebar (line 1206). Result cached for 300ms. 2. enforceSidebarWidth() invalidates the cache and lists again (line 1485) to walk panes for width enforcement. The second list is redundant — the cache is at most ~30ms old when enforce runs synchronously after the spawn check, and `tmux list-panes -a` is the single most expensive call on a busy tmux (50-200ms with 30+ panes). Add `reuseCache` to enforceSidebarWidth() and pass it from ensureSidebarInWindow(). Halves the list-panes work on every session switch. The standalone enforceSidebarWidth() callers (terminal resize, sidebar toggle, width sync) keep the existing default behaviour.
There was a problem hiding this comment.
inspect review
Triage: 20 entities analyzed | 0 critical, 0 high, 4 medium, 16 low
Verdict: standard_review
Findings (1)
- [low] Test bug:
loadConfig round-trips auto-theme fieldswrites tojoin(configDir, "config.json")but never createsconfigDir(or its parents). In the diff:const configDir = join(tmpDir, ".config", "opensessions"); await Bun.write(join(configDir, "config.json"), ...)with nomkdir. This will fail at runtime on a clean tmpDir.
Reviewed by inspect | Entity-level triage found 0 high-risk changes
eventTimestamps is a server-internal diagnostic — the tracker uses it for
stale/active heuristics — but it was being shipped to every WS client on
every state broadcast. The TUI never reads the field.
Each agent-emit appends a fresh timestamp, so back-to-back identical
opencode/claude-code/etc. status pings ("running" → "running" with the same
threadId) produced different state hashes and bypassed the broadcast dedup
introduced in a733f28. With chatty agents this fanned out one full state
JSON to every TUI client at ~1Hz per active session, visibly stalling the
foreground client when switching to an agent-heavy session.
Make the field optional in the SessionData shape and stop populating it in
computeState(). Tracker still maintains the timestamps server-side; they
just no longer escape onto the wire. Hash-dedup now correctly suppresses
no-op agent emits.
There was a problem hiding this comment.
inspect review
Triage: 21 entities analyzed | 0 critical, 0 high, 5 medium, 16 low
Verdict: standard_review
Findings (1)
- [low] Broken test:
loadConfig round-trips auto-theme fieldswrites tojoin(tmpDir, ".config", "opensessions", "config.json")but never createsconfigDir.await Bun.write(join(configDir, "config.json"), ...)will fail if parent directories don’t exist. Evidence: packages/runtime/test/config.test.ts addsconst configDir = join(tmpDir, ".config", "opensessions");then immediately callsBun.write(join(configDir, "config.json"), ...)with nomkdir/mkdirSync.
Reviewed by inspect | Entity-level triage found 0 high-risk changes
When the idle-timeout cleanup runs and a manual `toggle.sh ensure_server` fires within the kernel's TIME_WAIT window (~30s on macOS), the new process hits EADDRINUSE on port 7391 even though `lsof -iTCP:7391` returns nothing — the socket is still reserved. Setting `reusePort: true` on Bun.serve lets the new process bind immediately. Single-server invariant is still enforced by PID file.
There was a problem hiding this comment.
inspect review
Triage: 21 entities analyzed | 0 critical, 0 high, 5 medium, 16 low
Verdict: standard_review
Findings (2)
- [low] Config test will fail: it writes to
join(tmpDir, ".config", "opensessions", "config.json")without creating theconfigDirdirectory first, soawait Bun.write(...)will throw ENOENT. Evidence:const configDir = join(tmpDir, ".config", "opensessions"); await Bun.write(join(configDir, "config.json"), ...)with nomkdir/mkdirSyncbeforehand. - [low] Unhandled promise rejection risk in
watchMacSystemAppearance:check()awaitsonChange(mode)but is invoked viavoid check()fromfs.watchandsetIntervalwithout any.catch(...)/try-catch around the awaited callback. IfonChangethrows/rejects, it becomes an unhandled rejection. Evidence:watcher = watch(plistPath, () => { void check(); });andsetInterval(() => { void check(); }, safetyMs);withasync function check() { ... await onChange(mode); }.
Reviewed by inspect | Entity-level triage found 0 high-risk changes
30s was too aggressive on restart paths. After any manual respawn or plugin update, the TUI clients live inside sidebar panes that don't exist yet — the user has to press the toggle key to spawn one, and that exceeded the previous 30s window in normal flow. The server would self-terminate before it could be useful, leading to a frustrating loop where every M-s hit a stale server and re-triggered ensure_server. 5 min gives a comfortable usable window without leaving zombie servers running indefinitely. Active servers with WS clients clear the timer the instant the first TUI connects, so this change only affects the no-clients case.
There was a problem hiding this comment.
inspect review
Triage: 22 entities analyzed | 0 critical, 0 high, 6 medium, 16 low
Verdict: standard_review
Findings (1)
- [low] Test bug:
loadConfig round-trips auto-theme fieldswritesconfig.jsonintoconfigDirwithout creating the directory first, soawait Bun.write(join(configDir, "config.json"), ...)can fail with ENOENT. Evidence:const configDir = join(tmpDir, ".config", "opensessions"); await Bun.write(join(configDir, "config.json"), ...)with nomkdir/mkdirSyncbeforehand.
Reviewed by inspect | Entity-level triage found 0 high-risk changes
reusePort: true was the wrong fix for the EADDRINUSE-on-respawn issue. On macOS Bun's reusePort lets multiple processes bind the same port and share connections at the kernel level, but the opensessions runtime keeps all session/agent/sidebar state in process-local memory. Allowing duplicate servers meant clients round-robined across disjoint state and session switches behaved erratically. Revert reusePort and replace with a proper singleton guard: read PID_FILE, probe the recorded pid with `process.kill(pid, 0)`, and exit cleanly if that process is still alive. Stale PID files (process gone) are ignored and the new server takes over normally. This makes `toggle.sh ensure_server` idempotent — repeated invocations during the kernel's TIME_WAIT window collapse to no-ops instead of racing to spawn a second server.
There was a problem hiding this comment.
inspect review
Triage: 22 entities analyzed | 0 critical, 0 high, 6 medium, 16 low
Verdict: standard_review
Findings (1)
- [low] New config test writes to a non-existent directory, so
Bun.write(join(configDir, "config.json"), ...)will fail with ENOENT beforeloadConfig()runs. Evidence:const configDir = join(tmpDir, ".config", "opensessions"); await Bun.write(join(configDir, "config.json"), ...)with nomkdir/mkdirSyncforconfigDir.
Reviewed by inspect | Entity-level triage found 0 high-risk changes
Captures before/after numbers for the seven commits landed this session: push-based theme watch, broadcast hash-dedup, enforce cache reuse, eventTimestamps removal from the wire, idle-timeout bump, and the singleton-guard / reusePort revert. Headline: ~3.7× faster enforce dance on session switch, ~76% of agent-emit broadcasts suppressed under chatty agents, theme polling subprocess work eliminated, no more EADDRINUSE on respawn.
There was a problem hiding this comment.
inspect review
Triage: 27 entities analyzed | 0 critical, 0 high, 6 medium, 21 low
Verdict: standard_review
Findings (2)
- [low] Config test writes config.json into a directory that is never created, so the write can fail and the test won’t actually validate the new auto-theme fields. Evidence: in packages/runtime/test/config.test.ts it computes
const configDir = join(tmpDir, ".config", "opensessions");and then callsawait Bun.write(join(configDir, "config.json"), ...)without anymkdir/mkdirSyncforconfigDir. - [low] Broadcast hash-dedup updates
lastBroadcastHashbefore publishing; ifserver.publish("sidebar", msg)throws/fails, subsequent broadcasts with the same payload will be suppressed even though clients never received it. Evidence: in packages/runtime/src/server/index.ts:const hash = Bun.hash(msg); if (hash === lastBroadcastHash) return; lastBroadcastHash = hash; server.publish("sidebar", msg);.
Reviewed by inspect | Entity-level triage found 0 high-risk changes
Summary
Adds an opt-in feature that makes the sidebar track the macOS system Appearance setting: when the user toggles System Settings → Appearance between Light and Dark, the active theme flips to their configured dark/light pair within ~3 seconds, and every connected sidebar re-renders in sync.
macOS-gated — no-op on Linux/Windows.
Config (all opt-in)
{ "autoThemeFollowsSystem": true, "darkTheme": "catppuccin-mocha", "lightTheme": "catppuccin-latte" }autoThemeFollowsSystem— enables the poller (default: off).darkTheme/lightTheme— any of the 20 built-in theme names; default to the catppuccin pair that best matches the existing default theme.How it works
packages/runtime/src/system-theme.tsexposes two helpers:readMacSystemAppearance()— readsdefaults read -g AppleInterfaceStyleviaBun.spawn, maps to"dark"or"light", short-circuits on non-darwin.themeForSystemMode(mode, dark, light)— pure mapping.startServer()spawns a 3ssetIntervalthat calls both helpers; when the desired theme differs fromcurrentTheme, it takes the same path as the manualset-themecommand: update local state →saveConfig→broadcastState. That means every connected sidebar picks up the switch through the existing WebSocket broadcast — no new wire format.cleanup()alongside the other interval timers, so it unwinds cleanly on SIGINT/SIGTERM.macOS doesn't expose a CLI-visible change notification for Appearance, so polling is pragmatic; each poll is one cheap
defaultssubprocess.Changes
packages/runtime/src/system-theme.ts(42 lines) — pure helpers.packages/runtime/test/system-theme.test.ts— 5 tests covering the mapping + non-darwin short-circuit.packages/runtime/src/config.ts— three new optional fields onOpensessionsConfig.packages/runtime/src/server/index.ts— import, timer declaration, poller instartServer(), cleanup teardown.packages/runtime/test/config.test.ts— 2 tests for the new fields (round-trip + unset defaults).Test plan
bun test packages/runtime/test/system-theme.test.ts— 5/5 passbun test packages/runtime/test/config.test.ts— 19/19 pass (2 new + 17 existing)bun test packages/runtime/test— 374/374 pass, no regressionsHappy to iterate on the 3s cadence, the default theme pair, or the config key names.