Skip to content

Commit 2ac2110

Browse files
committed
fix(chat): prose tracks the workbench, and settings are clamped before CSS
Review on #72 found `0` documented as "follows the editor's UI font size" while the code cleared the inline property and let a flat `--prose-size: 14px` win. Making the description true was the better fix than making it match, because the flat value hid a real defect: D2 argues prose should read a step ABOVE workbench chrome, and a flat 14px only satisfies that at the default 13px. Raise the editor's UI font for accessibility and the relationship inverts — 14px prose inside 18px buttons, the divergence pointing the wrong way, for the users who most need it not to. `--prose-size` is now `calc(var(--vscode-font-size, 13px) + 1px)`, so the decision holds at every workbench size. At 13px it still resolves to 14px: nothing moves for anyone who has not changed it, and T1's ~108-character figure is unaffected. Both settings are now clamped at the host boundary. `minimum`/`maximum` in the contribution schema only drive the settings editor — it squiggles and saves anyway, and a hand-edited settings.json, a synced profile or a bad merge never passes through that UI. The values land directly in CSS, where `proseWidth: 1` is a one-pixel transcript: a panel with nothing left on screen to open settings with, whose only way out is finding the JSON file again. Out-of-range is pulled into range rather than rejected — someone asking for a 200px measure wants it narrow. `0` is now stated the same way in all four places it appears: it means "leave the stylesheet alone", not "unconstrained" (the width's default is a 680px measure) and not "follow the workbench" (the size's default tracks it, but by the stylesheet's offset, not by the sentinel). Guards, each bypass-verified by reverting the fix: - the size must be workbench-relative, with a px fallback, offset 1-3px, and must still resolve to 14px at the default — 4 separate failures - the clamp's ceiling must equal the schema's `maximum`, its floor must exceed the schema `minimum` (0, the sentinel), and the description must name both - 0/undefined/NaN/negative/Infinity fall back; below-floor snaps up; absurd snaps down 32 suites green.
1 parent 8f7e883 commit 2ac2110

5 files changed

Lines changed: 131 additions & 16 deletions

File tree

docs/CHAT-TYPOGRAPHY.md

Lines changed: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -78,9 +78,15 @@ This is the real trade-off in the whole document, so it gets stated rather than
7878
paragraphs of explanation at that size, at `line-height: 1.5`, is why the panel feels cramped next to
7979
the reference.
8080

81-
**Decision:** message bodies get their own size (~14px) and leading (~1.65), expressed relative to a
82-
single custom property. Everything else — the composer, buttons, session cards, the status row,
83-
approval chips — keeps inheriting the workbench size, so the panel still belongs to the editor.
81+
**Decision:** message bodies get their own size and leading (~1.65), expressed through a single custom
82+
property. Everything else — the composer, buttons, session cards, the status row, approval chips —
83+
keeps inheriting the workbench size, so the panel still belongs to the editor.
84+
85+
The size is an **offset**, `calc(var(--vscode-font-size) + 1px)`, not a flat 14px. Review caught the
86+
reason: a flat value silently inverts the decision for anyone who has raised the editor's UI font for
87+
accessibility — an 18px workbench would read 14px prose inside 18px chrome, which is the divergence
88+
this decision argues for, pointing the wrong way. At the default 13px it resolves to the same 14px, so
89+
the change is invisible to everyone who has not touched it.
8490

8591
**The cost, honestly:** the chat will no longer match workbench chrome exactly. That is a real
8692
inconsistency, and it is the deliberate price of the panel being a place you *read* rather than a
@@ -117,10 +123,22 @@ for exactly this case).
117123

118124
### D7 — It stays hackable: two settings, no hard-coded values.
119125

120-
`levelcode.ai.chat.proseWidth` (px, `0` = unconstrained) and `levelcode.ai.chat.fontSize`
121-
(`0` = follow the workbench). Both flow through CSS custom properties set on the container, so the
122-
defaults are a starting point rather than a verdict — consistent with the editor's whole posture, and
123-
the honest answer to anyone who preferred the old density.
126+
`levelcode.ai.chat.proseWidth` (px) and `levelcode.ai.chat.fontSize` (px). Both flow through the CSS
127+
custom properties above, set on the container, so the defaults are a starting point rather than a
128+
verdict — consistent with the editor's whole posture, and the honest answer to anyone who preferred
129+
the old density.
130+
131+
**`0` means "leave the stylesheet alone" for both**, and nothing more. The first draft of this line
132+
claimed `0` = *unconstrained* for the width and `0` = *follow the workbench* for the size; neither was
133+
what the code did, and review caught both. The width's default is a 680px measure, not the absence of
134+
one — the way to widen it is a large number. The size's default now does track the workbench, but by
135+
the D2 offset, which is a property of the stylesheet rather than of the sentinel.
136+
137+
Both are **clamped at the host boundary** (`clampSetting`, 8–24 and 320–2000). `minimum`/`maximum` in
138+
the contribution schema only drive the settings *editor*; a hand-edited `settings.json` reaches
139+
`getConfiguration()` unchecked, and these values land directly in CSS. `proseWidth: 1` is a one-pixel
140+
transcript — a panel with nothing left on screen to open settings with, whose only exit is finding the
141+
JSON file again. `webviewCss.test.js` pins the clamp to the schema so the two cannot drift.
124142

125143
---
126144

extensions/levelcode-ai/extension.js

Lines changed: 26 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2083,6 +2083,24 @@ async function browseProviderModels(cfg, providerId) {
20832083
sendConfigToWebview();
20842084
}
20852085

2086+
// A settings number on its way into a CSS custom property.
2087+
//
2088+
// `minimum`/`maximum` in the contribution schema only drive the settings *editor*: it draws a squiggle
2089+
// and saves the value anyway, and a hand-edited settings.json, a synced profile or a bad merge never
2090+
// passes through that UI at all. Whatever is in the file is what `getConfiguration()` returns, and here
2091+
// it lands directly in CSS — where `chat.proseWidth: 1` is a one-pixel transcript and
2092+
// `chat.fontSize: 0.5` is a blank panel. Neither leaves anything on screen to open settings with, so
2093+
// the way out is to find the JSON file again; clamping at the boundary is cheaper than that.
2094+
//
2095+
// Out-of-range is pulled INTO the range rather than rejected — someone who asks for a 200px measure
2096+
// wants it narrow, so give them the narrowest readable one instead of silently ignoring them. Only 0,
2097+
// a negative, or a non-number means "leave the stylesheet's own value alone".
2098+
function clampSetting(raw, lo, hi) {
2099+
const n = Number(raw);
2100+
if (!Number.isFinite(n) || n <= 0) { return 0; }
2101+
return Math.min(Math.max(n, lo), hi);
2102+
}
2103+
20862104
function sendConfigToWebview() {
20872105
const cfg = aiConfig();
20882106
// Gateway mode (signed in — the gateway only routes when authenticated): the footer reflects the
@@ -2092,10 +2110,14 @@ function sendConfigToWebview() {
20922110
const groupActivity = cfg.get('chat.groupActivity', true) !== false;
20932111
// T2/T5 (docs/CHAT-TYPOGRAPHY.md D2, D7). Prose gets its own size because `--vscode-font-size` is the
20942112
// size of menu labels and tree rows — right for chrome, wrong for reading three paragraphs. Both are
2095-
// escape hatches by design: 0 means "follow the workbench" / "do not constrain", so anyone who
2096-
// preferred the old density has a one-setting way back rather than an argument.
2097-
const proseSize = Number(cfg.get('chat.fontSize', 0)) || 0;
2098-
const proseWidth = Number(cfg.get('chat.proseWidth', 0)) || 0;
2113+
// escape hatches by design, so anyone who preferred the old density has a one-setting way back
2114+
// rather than an argument.
2115+
//
2116+
// 0 means "leave the stylesheet alone" for both — the size then tracks the workbench
2117+
// (`--vscode-font-size` + 1px) and the measure stays at its 680px default. It does NOT mean
2118+
// "unconstrained": the way to widen the measure is a large number, not 0.
2119+
const proseSize = clampSetting(cfg.get('chat.fontSize', 0), 8, 24);
2120+
const proseWidth = clampSetting(cfg.get('chat.proseWidth', 0), 320, 2000);
20992121
if (providerMode() === 'gateway' && cloudSignedIn) {
21002122
const model = gatewayModel();
21012123
post({

extensions/levelcode-ai/media/chat.html

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,8 +32,13 @@
3232
/* T2 (D2) — the READING type. `--vscode-font-size` is the size of menu labels and tree rows: right
3333
for chrome, wrong for three paragraphs of explanation. Message bodies alone get their own size
3434
and leading; every control around them keeps inheriting the workbench, so the panel still
35-
belongs to the editor. Both are overridable at runtime by the settings (D7). */
36-
--prose-size: 14px;
35+
belongs to the editor. Both are overridable at runtime by the settings (D7).
36+
An OFFSET, not a flat 14px, so prose TRACKS the workbench instead of pinning against it. A flat
37+
value quietly inverts for anyone who raises the editor's UI font for accessibility: at a 18px
38+
workbench they would read 14px prose inside 18px chrome — the divergence D2 argues for, pointing
39+
the wrong way. At the default 13px this still resolves to 14px, so nothing moves for anyone who
40+
has not changed it. */
41+
--prose-size: calc(var(--vscode-font-size, 13px) + 1px);
3742
--prose-leading: 1.65; }
3843
/* Log is a flex column → children default to flex-shrink:1. Any child with overflow:hidden (e.g. the
3944
approval/edit cards) then gets an auto min-size of 0 and the flex algorithm crushes it to an invisible

extensions/levelcode-ai/package.json

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -369,14 +369,14 @@
369369
"default": 0,
370370
"minimum": 0,
371371
"maximum": 24,
372-
"markdownDescription": "Font size for chat **prose** (message bodies), in pixels. `0` follows the editor's UI font size.\n\nThe workbench size is tuned for menu labels and tree rows; message bodies default to a slightly larger size so a long answer reads comfortably. Controls, cards and the composer always follow the workbench."
372+
"markdownDescription": "Font size for chat **prose** (message bodies), in pixels. `0` tracks the editor's UI font size, one step larger for reading — a 13px workbench gives 14px prose.\n\nThe workbench size is tuned for menu labels and tree rows; message bodies read a step above it so a long answer is comfortable. Controls, cards and the composer always match the workbench exactly. Values are clamped to 8–24."
373373
},
374374
"levelcode.ai.chat.proseWidth": {
375375
"type": "number",
376376
"default": 0,
377377
"minimum": 0,
378378
"maximum": 2000,
379-
"markdownDescription": "Maximum width of the chat transcript, in pixels. `0` uses the default reading measure (680px).\n\nOnly has an effect when the panel is wider than the measurein a narrow sidebar the container already bounds the line length. Set a large value to let the transcript fill the full width."
379+
"markdownDescription": "Maximum width of the chat transcript, in pixels. `0` uses the default reading measure (680px) — it does **not** mean unconstrained.\n\nOnly has an effect when the panel is wider than the measure: in a narrow sidebar the container already bounds the line length. Raise it to let the transcript spread across a wide editor tab. Values are clamped to 320–2000."
380380
},
381381
"levelcode.ai.chat.groupActivity": {
382382
"type": "boolean",

extensions/levelcode-ai/test/webviewCss.test.js

Lines changed: 71 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -302,7 +302,8 @@ test('TRANSCRIPT: prose has its own type, and chrome does not follow it', () =>
302302
// size is tuned for menu labels; message bodies get their own. The scoping is the entire safety
303303
// property: applied to `.msg` instead of `.msg .body` it would drag the role label, the copy
304304
// button and the checkpoint control up with it, and the panel would stop matching the editor.
305-
assert.match(css, /#log\s*\{[^}]*--prose-size:\s*\d+px/, 'the prose size is no longer a custom property');
305+
assert.match(css, /#log\s*\{[^}]*--prose-size:\s*calc\(var\(--vscode-font-size[^)]*\)\s*\+\s*\d+px\)/,
306+
'the prose size must be an OFFSET from the workbench, not a flat value — see below');
306307
assert.match(css, /#log\s*\{[^}]*--prose-leading:\s*[\d.]+/, 'the prose leading is no longer a custom property');
307308
assert.match(css, /\.msg \.body \{[^}]*font-size:\s*var\(--prose-size\)[^}]*line-height:\s*var\(--prose-leading\)/,
308309
'prose type must be set on .msg .body');
@@ -314,6 +315,75 @@ test('TRANSCRIPT: prose has its own type, and chrome does not follow it', () =>
314315
'the turn label must keep an absolute size, or it grows with the prose it is labelling');
315316
});
316317

318+
test('TRANSCRIPT: the prose size TRACKS the workbench rather than pinning against it', () => {
319+
// The bug a flat `--prose-size: 14px` hides, and the reason review's "0 doesn't do what the
320+
// description says" comment was worth more than a wording fix.
321+
//
322+
// D2 argues prose should read a step ABOVE workbench chrome. A flat value only satisfies that at
323+
// the default 13px: raise the editor's UI font for accessibility and the relationship inverts —
324+
// 14px prose inside 18px buttons, the divergence pointing the wrong way, for exactly the users who
325+
// most need it not to. An offset holds the decision at every workbench size.
326+
const decl = /--prose-size:\s*([^;]+);/.exec(css);
327+
assert.ok(decl, 'the stylesheet no longer declares --prose-size');
328+
const m = /calc\(\s*var\(--vscode-font-size(?:\s*,\s*(\d+)px)?\)\s*\+\s*(\d+)px\s*\)/.exec(decl[1]);
329+
assert.ok(m, '--prose-size must be workbench-relative, got: ' + decl[1].trim());
330+
331+
// The offset stays small: this is "a step above for reading", not a second font scale. Past ~3px
332+
// the panel stops looking like part of the editor and D2's cost is no longer the one we accepted.
333+
assert.ok(Number(m[2]) >= 1 && Number(m[2]) <= 3,
334+
'the reading offset is ' + m[2] + 'px; beyond ~3px the chat stops belonging to the workbench');
335+
// A fallback is required: `--vscode-font-size` is injected by the host, and the file opens in a
336+
// plain browser during development, where an unresolved var() would void the whole declaration.
337+
assert.ok(m[1], '--vscode-font-size needs a px fallback, or the rule is void outside the webview');
338+
339+
// And the default must still land on the 14px the doc and T1's character count were measured at.
340+
assert.strictEqual(Number(m[1]) + Number(m[2]), 14,
341+
'at the default 13px workbench this must still resolve to 14px, or T1\'s ~108-character figure moves');
342+
});
343+
344+
test('TRANSCRIPT: settings are clamped at the host boundary, to the schema\'s own bounds', () => {
345+
// `minimum`/`maximum` in the contribution schema are advice for the settings EDITOR — it squiggles
346+
// and saves anyway, and a hand-edited settings.json, a synced profile or a bad merge never passes
347+
// through that UI at all. These two land straight in CSS, where `proseWidth: 1` is a one-pixel
348+
// transcript: a panel with nothing left on screen to open settings with, whose only way out is
349+
// finding the JSON file again.
350+
const ext = fs.readFileSync(path.join(__dirname, '..', 'extension.js'), 'utf8');
351+
const src = /function clampSetting\(raw, lo, hi\) \{[\s\S]*?\n\}/.exec(ext);
352+
assert.ok(src, 'clampSetting is gone — the settings reach CSS unchecked again');
353+
const clampSetting = new Function(src[0] + '\nreturn clampSetting;')(); // eslint-disable-line no-new-func
354+
355+
const props = require('../package.json').contributes.configuration.properties;
356+
for (const key of ['fontSize', 'proseWidth']) {
357+
const call = new RegExp('clampSetting\\(cfg\\.get\\(\'chat\\.' + key + '\', 0\\),\\s*(\\d+),\\s*(\\d+)\\)').exec(ext);
358+
assert.ok(call, 'chat.' + key + ' is read without clampSetting — the bound is decorative again');
359+
const lo = Number(call[1]), hi = Number(call[2]);
360+
const schema = props['levelcode.ai.chat.' + key];
361+
362+
// The pin: the number the host enforces IS the number the settings UI advertises. Raising one
363+
// without the other is the drift this test exists to catch.
364+
assert.strictEqual(hi, schema.maximum,
365+
'chat.' + key + ' clamps at ' + hi + ' but the schema advertises ' + schema.maximum);
366+
assert.ok(lo > schema.minimum,
367+
'the floor must be a real readability bound, not the schema minimum (which is 0, the sentinel)');
368+
assert.ok(new RegExp('clamped to ' + lo + '.' + hi).test(schema.markdownDescription),
369+
'chat.' + key + ' does not tell the user it is clamped to ' + lo + '-' + hi);
370+
371+
// 0 is the sentinel, and it must survive the clamp — clamping it up to `lo` would make the
372+
// default un-expressible and permanently override the stylesheet.
373+
assert.strictEqual(clampSetting(0, lo, hi), 0, '0 must stay 0 — it means "leave the stylesheet alone"');
374+
assert.strictEqual(clampSetting(undefined, lo, hi), 0, 'an unset value falls back to the stylesheet');
375+
assert.strictEqual(clampSetting('nonsense', lo, hi), 0, 'a non-number falls back rather than emitting NaNpx');
376+
assert.strictEqual(clampSetting(-5, lo, hi), 0, 'a negative falls back — CSS would drop it and behave unpredictably');
377+
assert.strictEqual(clampSetting(Infinity, lo, hi), 0, 'Infinity falls back rather than emitting "Infinitypx"');
378+
379+
// In range, untouched. Out of range, pulled in — someone asking for a 200px measure wants it
380+
// narrow, so give them the narrowest readable one instead of ignoring them.
381+
assert.strictEqual(clampSetting(lo + 1, lo, hi), lo + 1, 'an in-range value must pass through unchanged');
382+
assert.strictEqual(clampSetting(1, lo, hi), lo, 'a below-floor value snaps to the floor');
383+
assert.strictEqual(clampSetting(1e9, lo, hi), hi, 'an absurd value snaps to the ceiling');
384+
}
385+
});
386+
317387
test('TRANSCRIPT: the heading scale has steps you can actually see', () => {
318388
// The old 1.3/1.18/1.07 put 0.11em between h2 and h3 — 1.4px at 13px, i.e. three levels of
319389
// hierarchy that were indistinguishable without selecting the text.

0 commit comments

Comments
 (0)