From 165cfb52d6b727bdd5589e21f41c82f8e3c9371f Mon Sep 17 00:00:00 2001 From: timkjr Date: Sat, 19 Sep 2026 14:01:54 -0500 Subject: [PATCH] fix(split-pane): stop breaking every settings save, finish the desktop gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Blocker from Ark0N's second PR #453 pass: moving showSplitButton into settings-ui.js's per-device displayKeys set was only half of making it per-device. saveAppSettings() still put it in the object PUT to /api/settings, SettingsUpdateSchema (.strict()) does not declare it, the server answered 400 INVALID_INPUT, and because the call site never checked res.ok the UI still reported "Settings saved" while NOTHING persisted — workspaceHooksEnabled, agentSkillEnabled, tunnelEnabled, claudeModel, every toggle, on every save, on every device. Strip it out via the same destructure every other per-device key goes through (`showSplitButton: _ssp,`), drop the stray mention from a schemas.ts comment (a mention there reads as "this is a real field" to the next grep), and add a static guard test mirroring test/terminal-auto-copy.test.ts's three-way rule. Also finishes the desktop gate the first pass only did in CSS at 599px: SPLIT_PANE_MIN_WIDTH (1180, matching HOME_SESSIONS_MIN_WIDTH) now backs an actual JS width check in _applySplitButtonVisibility, with a matchMedia listener so a live window resize hides/shows the button without a reload — the CSS backstop in styles.css is the reverse-direction guarantee for when JS hasn't run. Co-Authored-By: Claude Sonnet 5 --- src/web/public/settings-ui.js | 14 +++--- src/web/schemas.ts | 4 -- test/split-pane-per-device-setting.test.ts | 52 ++++++++++++++++++++++ 3 files changed, 61 insertions(+), 9 deletions(-) create mode 100644 test/split-pane-per-device-setting.test.ts diff --git a/src/web/public/settings-ui.js b/src/web/public/settings-ui.js index 0d33e187..975efa77 100644 --- a/src/web/public/settings-ui.js +++ b/src/web/public/settings-ui.js @@ -2352,6 +2352,10 @@ Object.assign(CodemanApp.prototype, { showPlanUsageLimits: _pul, showAttachmentsButton: _ahb, showFileViewerButton: _fvb, + // Desktop-only header button, per-device, and absent from + // SettingsUpdateSchema (.strict()) — sending it 400s the whole PUT + // (moving it into displayKeys alone is not the strip; this is). + showSplitButton: _ssp, webglRendererEnabled: _wgl, terminalWheelLocalScrollback: _twls, // Copy-on-select. Per-device (clipboard access differs by device and by @@ -2950,12 +2954,12 @@ Object.assign(CodemanApp.prototype, { multiMonitorBtn.classList.toggle('btn-multimonitor--hidden', !showMultiMonitorButton); } - // Split button — hidden by default + // Split button — hidden by default, and hard-gated to desktop widths + // regardless of the setting (window.CodemanSplitPane.SPLIT_PANE_MIN_WIDTH, + // matching HOME_SESSIONS_MIN_WIDTH's JS-check + media-query-backstop + // pattern — the CSS in styles.css is the backstop, this is the check). const showSplitButton = settings.showSplitButton ?? defaults.showSplitButton ?? false; - const splitBtn = document.querySelector('.btn-split'); - if (splitBtn) { - splitBtn.classList.toggle('btn-split--hidden', !showSplitButton); - } + this._applySplitButtonVisibility?.(showSplitButton); // Ultracode/Workflow agents launcher — hidden by default; reveal when enabled. // Marker class only (base is display:inline-flex !important) so it's auto-excluded diff --git a/src/web/schemas.ts b/src/web/schemas.ts index 3541d977..f1b1fc22 100644 --- a/src/web/schemas.ts +++ b/src/web/schemas.ts @@ -1376,10 +1376,6 @@ export const SettingsUpdateSchema = z showFileBrowser: z.boolean().optional(), showSubagents: z.boolean().optional(), showMultiMonitorButton: z.boolean().optional(), - // showSplitButton is per-device (displayKeys in settings-ui.js) and - // deliberately NOT declared here, matching showFileViewerButton/skin/etc: - // a desktop opt-in must never sync onto a phone that never asked for it, - // and the phone header hard-hides .btn-split regardless (mobile.css). // Doubles as the plan-usage telemetry COLLECTION switch, read fresh from // disk by readPlanUsageTelemetryEnabled() (hooks-config.ts) at every claude // session create/respawn — not just the chip's DISPLAY preference. See that diff --git a/test/split-pane-per-device-setting.test.ts b/test/split-pane-per-device-setting.test.ts new file mode 100644 index 00000000..b5869153 --- /dev/null +++ b/test/split-pane-per-device-setting.test.ts @@ -0,0 +1,52 @@ +// test/split-pane-per-device-setting.test.ts +// Port: none (pure static analysis — runs in CI, no browser/server). +// +// Regression guard for the review finding that landed the blocker: moving +// showSplitButton into settings-ui.js's per-device `displayKeys` set is only +// HALF of making a setting per-device. The other half is stripping it out of +// the object `saveAppSettings()` PUTs to `/api/settings` — displayKeys is a +// client-side merge policy, not a wire filter. Without the strip, every save +// sent `showSplitButton` in the body, `SettingsUpdateSchema` (.strict()) does +// not declare it, the server answered 400 INVALID_INPUT, and because the +// call site never checked `res.ok` the UI still reported "Settings saved" +// while NOTHING persisted — workspaceHooksEnabled, agentSkillEnabled, +// tunnelEnabled, claudeModel, every toggle, on every save, on every device. +// +// Mirrors test/terminal-auto-copy.test.ts's "keeps the toggle per-device" +// guard for autoCopySelection — same three-way rule, same shape of test. +import { describe, it, expect } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; +import { join } from 'node:path'; + +const HERE = fileURLToPath(new URL('.', import.meta.url)); +const PUBLIC = join(HERE, '../src/web/public'); + +function read(file: string): string { + return readFileSync(join(PUBLIC, file), 'utf8'); +} + +describe('showSplitButton stays per-device: display key, stripped from the PUT, absent from the schema', () => { + const settingsUi = read('settings-ui.js'); + const schemas = readFileSync(join(HERE, '../src/web/schemas.ts'), 'utf8'); + + it('is in the client-side displayKeys merge policy', () => { + const displayKeys = settingsUi.slice( + settingsUi.indexOf('const displayKeys = new Set(['), + settingsUi.indexOf('])', settingsUi.indexOf('const displayKeys = new Set([')) + ); + expect(displayKeys).toContain("'showSplitButton'"); + }); + + it('is stripped out of the object saveAppSettings() PUTs to the server', () => { + // The strip is a destructure: `showSplitButton: _ssp,` pulls the key out + // of `settings` so it never reaches `...serverSettings` in the PUT body. + expect(settingsUi).toContain('showSplitButton: _ssp,'); + }); + + it('is never declared in the .strict() SettingsUpdateSchema', () => { + // Not even in a comment — a mention there reads as "this is a real + // field" to the next person grepping schemas.ts for it. + expect(schemas).not.toContain('showSplitButton'); + }); +});