Settings UI: theme, font sizes, and worktrees directory - #5
Merged
Conversation
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two approaches in the first draft were wrong in ways that look right: - px->em conversion of app.css: JavaFX resolves em against the inherited font, not .root, so nested rules compound (.code-area/.lineno would land at 10.15px, not 11px). Replaced with a runtime-generated scaled stylesheet, which also reaches popup scene graphs that an inline root style never would. - per-surface ghostty font_size: the theme-toggle path already calls ghostty_surface_update_config, which would drop the override and reset the terminal mid-session. Moved into the generated ghostty .conf, which makes the size apply live to running surfaces and shrinks the diff to one class. Also: codec no longer clamps (matching sidebarWidth), names RepositoryManager as the state writer, adds UserConfig.flushPendingSaves, makes cmd+, inert while a modal is showing, and records the libghostty font-size assumption as a verification step. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Eleven tasks, TDD throughout. Task 1 is a gate: it verifies libghostty honours font-size in a config file, and cuts terminal font size from the round if it does not, rather than falling back to the per-surface write the spec rejected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A one-line pass-through to applyTerminalTheme would be flagged as a redundant wrapper; the shared path is documented on the real method instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A comment between the -fx-font-size colon and its px value (e.g. "-fx-font-size: /* TODO bump */ 12px;") was matched by neither the declaration alternative (whitespace-only between colon and digit) nor recognised as part of a declaration by the standalone-comment alternative, so the value was silently left unscaled. Allow comments plus whitespace between the colon and the value so the whole thing is still recognised as one declaration.
saveAsync previously spawned an independent virtual thread per call and tracked only the latest future via a plain AtomicReference.set(), so concurrent saves (the settings modal commits on both field-blur and Browse) could race with no ordering guarantee, and flushPendingSaves could return while an earlier save was still writing -- silently losing updates and defeating the flush's purpose. Route saves through a single-threaded executor with newest-wins coalescing and a no-op-flush, mirroring AnnotationStore's proven pattern. Also: resolve configFile to an absolute path so save() never lands its temp file on a different filesystem, and strengthen the "preserves unknown members" test to check parsed structure instead of substrings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…yling - UiFontScale and TerminalThemes gain async entry points (stylesheetForAsync, configFileForAsync): a cache hit/default resolves synchronously with no I/O, a miss generates on a virtual thread and lands via Platform.runLater, and a generation failure is logged instead of reaching the uncaught handler. ThemeManager and MainWorkspace route slider-driven live-apply through these; the startup path in ThemeManager's constructor stays synchronous as required. - SettingsModal.sizeRow debounces persistence via the slider's valueChangingProperty: live apply fires every tick, the commit callback (Settings.commitUiFontSize/commitTerminalFontSize) fires once, on drag release. The terminal slider is now integer-only, matching what TerminalThemes actually renders. - DrydockApplication gates cmd+shift+L on modalLayer().isShowingModal(), matching cmd+comma, so the theme cannot change underneath an open Settings modal and desync its radio. - SettingsModal's RadioButton/Slider get style classes and matching app.css rules (following the SearchRail .result-check precedent), fixing near-unreadable radio captions and slider tracks in both themes. - Minor fixes: stale comment naming a nonexistent method, UserConfig's loadAsync/runPendingSave now catch(Throwable) so a future can never hang forever, MainWorkspace's terminal font provider is a DoubleSupplier, and UserConfigTest's user.home-mutating tests get @ResourceLock. - docs/manual-terminal-checklist.md gains sections for opening/closing Settings, default-size parity, dual-theme legibility, and the worktrees-directory round trip. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
MainWorkspace.applyTerminalConfig had no staleness guard: a fast slider drag spawns one virtual thread per cache-miss size, and thread start order doesn't determine callback order, so an older size's callback could land after a newer one and leave terminals showing a stale size. Guard it the same way ThemeManager.setUiFontSize already does: record the latest requested (theme, fontSize) and drop a callback that no longer matches it. ThemeManager.setUiFontSize assigned its uiFontSize field before async generation completed, so a generation failure left the field naming a size with no cached stylesheet -- apply()'s "cache hit guaranteed" Javadoc was false, a later setTheme could throw UncheckedIOException out of the toggle handler, and the bad size could reach persistence and fail the *next* launch from the constructor. uiFontSize is now only assigned once its stylesheet is confirmed generated, and DrydockApplication.commitUiFontSize persists that confirmed value instead of the raw slider position. Also collapse redundant full-scene CSS reapplies during a drag by comparing the resolved stylesheet URL instead of the raw font size. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… state An arrow key or a track click never sets Slider.valueChanging, so the commit ran in the same FX event as the apply. On a first visit to a size the stylesheet generation is a cache miss whose success callback is still queued in Platform.runLater, so ThemeManager.uiFontSize() -- which commitUiFontSize read back -- still held the PREVIOUS size. Every keyboard adjustment persisted one step behind, invisibly until restart. The read-back was guarding a real hazard: ThemeManager's constructor generates the scaled sheet synchronously before the stage is shown, and a generation failure threw, so a persisted bad size was a hard launch failure. Fix that at the source instead -- UiFontScale.stylesheetFor now logs and falls back to the bundled unscaled sheet, caching the fallback so no later FX-thread re-application retries the failed I/O inline. With resolution total, stylesheetForAsync always invokes its callback and no persisted size can prevent launch. That removes the reason for the read-back, so the apply/persist sequencing moves into a pure SizeSetting record whose persist step is handed the value the user chose. SettingsModal.Settings loses six methods for two, and DrydockApplication wires RepositoryManager's update methods directly -- there is no lambda body left for a read-back to hide in. SizeSettingTest drives that sequencing with a deferred-apply double, no toolkit needed: reverting the one line to the read-back fails two of its five tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… improve identifiers Fixes three documentation/naming issues found in code review: 1. MainWorkspace.applyTerminalTheme: Stale Javadoc claimed the settings modal calls this method to apply terminal font size changes. Actually, the modal calls previewTerminalFontSize. Updated to clarify that applyTerminalTheme is called only by the theme toggle, and explain the distinction between the two callers (apply vs preview). 2. UiFontScale.stylesheetFor and stylesheetForAsync: Javadoc promised "never throws" and "onReady always runs exactly once", but the fallback path calls baseStylesheetUrl() which can NPE if the bundled stylesheet resource is missing—unhandled by the virtual thread. Softened both claims to what is actually guaranteed: generation failures degrade to the bundled stylesheet, so no persisted size prevents app startup. The unhandled case (missing bundled resource) is a broken build, which fails at startup anyway. 3. SizeSetting.appliedSize: Renamed to currentSize to clarify that the record component covers two different data sources: for interface size it reads the theme manager's applied size, but for terminal size it reads persisted state. Updated Javadoc to explicitly document this dual role. Rename touches SizeSetting.java, its usages, and SizeSettingTest.java (which still passes all 5 test cases). All 454 tests pass. No logic changes. Documentation and naming only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- SettingsModal: flush a pending worktrees-directory edit on every modal close path (Done/x/Esc/backdrop), not just focus-loss, by wiring ModalLayer's onClosed callback to a new flushPendingEdit(). Made the commit idempotent and re-entrancy-guarded so Esc never loses a typed path and a single Enter keystroke never fires two overlapping saves. - TerminalThemes/MainWorkspace/DrydockApplication: pre-warm the ghostty config cache at startup (off the FX thread) instead of restructuring the synchronous FX-thread config lookup at session-open time, and make configFileFor's Javadoc describe the real (warmed-by-construction, not guaranteed) contract. - UserConfig: route loadAsync's read through the same single-threaded SAVE_EXECUTOR saves use, so a load is FIFO-ordered against every previously queued save instead of racing it on an independent thread. - SettingsModal: correct a comment that described a completion path (an "early return inside saveWorktreesDirectory") that does not exist. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…sModal FIX 1: Assign lastCommitted[0] only on successful save, not before. This ensures failed saves remain retryable—pressing Enter again will retry, not silently no-op. Idempotency (no redundant save for unchanged value) is preserved by the early return check. On close-flush, unchanged values still compare and return early. FIX 2: Compute Optional<Path> directory before setting committing[0]=true. If Path.of() threw InvalidPathException (practically unreachable but worst-case possible), the flag would latch and all future commits would no-op. Computing first ensures the exception escapes cleanly without latching the guard. All disable/re-enable paths (success and failure) remain intact and unconditional in the whenComplete callback. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Running the app revealed what no review could: the worktrees Directory field and its Browse button were the modal's only controls without a style class, so they fell back to modena's light defaults -- a white field and a grey 3D button inside the dark modal. They now reuse the New-worktree modal's .worktree-field and .worktree-cancel-button, the same controls doing the same job. Adds app.drydock.diag.settingsScript alongside the file's existing diag hooks, with verbs to open the modal, set the theme and both sizes, type into the directory field, close via the Esc path, snapshot, and dump the persisted state. The Settings modal is the one part of this feature no unit test can reach, so its risky paths get a driver rather than an unverified claim. Verified with it: - Esc no longer discards a typed directory: the config file goes from absent to holding the typed path across the Esc close. - The modal is legible in both themes. - Interface size scales the whole UI, popups included, across 11-16. - ghostty honours font-size in a config file for EMBEDDED surfaces -- the gate this design rested on. The child pty's winsize tracks it: 13px -> 30x98, 18px -> 21x71, 10px -> 39x131, 14px -> 27x92. - A size change reaches an ALREADY-RUNNING session, and survives a theme toggle (21x71 before and after) -- the regression the rejected per-surface approach would have caused. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Adds a Settings modal — theme, interface font size, terminal font size, worktrees directory — reachable from a new gear button in the title bar and from
⌘,.Spec:
docs/superpowers/specs/2026-07-25-settings-ui-design.mdWhere each setting lives
WorkspaceUiState.themeWorkspaceUiState.uiFontSize(new)WorkspaceUiState.terminalFontSize(new)~/.drydock/config.jsonCosmetic state rides the existing single-writer
ApplicationStateStore; the schema change is additive and decodes leniently, so older documents load with defaults.UserConfigwas read-only and gains atomic writes (temp file + atomic move), serialized through one executor with newest-wins coalescing and a bounded flush at shutdown.Two design decisions worth reviewing
Interface font size does not use CSS
em. JavaFX resolves font-relative sizes against the inherited font, not.root, so a mechanical px→em rewrite compounds:.code-area(12px) containing.lineno(11px) lands at 10.15px instead of 13.54px at factor 16/13. InsteadUiFontScalegenerates a scaled copy ofapp.cssat runtime. An inline style on the scene root was also rejected — it never reaches combo popups, context menus, or tooltips, which are separate scene graphs, asapp.css:1304already documents.UiFontScaleTestlocks the nested-ratio behaviour in.Terminal font size lives in the generated ghostty config, not
ghostty_surface_config_s.font_size. A theme toggle callsghostty_surface_update_config, which re-derives a running surface's config and would silently discard a per-surface override mid-session. Going through the config file also makes the size apply live to running terminals over the path a theme toggle already takes.Verification
455 tests pass. Beyond that, the modal is the one component with no unit-test reach (no JavaFX toolkit harness here), so it was driven in a running app via a new
app.drydock.diag.settingsScripthook, alongside the file's existing diag hooks:font-sizefor embedded surfaces — the assumption this design rested on. Measured externally from the child pty's winsize, since the surface is a native view above the FX scene: 13px → 30×98, 18px → 21×71, 10px → 39×131, 14px → 27×92.That pass also caught the one defect no review could see: the Directory field and Browse button were the modal's only unstyled controls, falling back to modena's light defaults inside the dark modal.
Known limitations
ThemeManager's constructor generates the scaled stylesheet synchronously. This is a deliberate deviation from AGENTS.md's no-FX-thread-I/O rule: the sheet must be in place before the stage is shown, or every launch flashes a re-layout. Documented at the call site.SizeSettingTest).🤖 Generated with Claude Code