Skip to content

Commit ffa5f0b

Browse files
committed
fix(chat): bound the media block by braces, and stop the comment copying the cap
Both review comments on #75 were right. 1. THE GATE LOOKUP WAS STILL ORDER-DEPENDENT My earlier fix found the rhythm block by content instead of position, but still terminated the slice with `indexOf('\n }')`. The --shell-x gate is written on one line, so it has no `\n }` of its own — the search ran straight past it into the next multi-line block. Measured: for a rule whose body is 27 characters, the old search returned 238, i.e. it reached about nine times past the rule it was supposed to bound. It happened to resolve correctly, because the swallowed span did not contain `#log {`. That is luck of ordering, not correctness. Demonstrated by moving the one-liner gate to sit directly before the rhythm gate: under the old mechanism the FIRST gate's slice then contains `#log {`, so it selects the wrong gate and asserts against a span covering both — silently. Under balanced braces the same reorder still resolves to the right gate and all 30 tests pass. `blockAt()` matches braces, over a comment-stripped copy so a brace inside prose cannot throw off the count. 2. THE STYLESHEET COMMENT KEPT ITS OWN COPY OF THE CAP It still said 680px and "~116 characters" after the cap became 820. That is the third place this number has drifted and the only one nothing was watching — the existing pin covers CHAT-TYPOGRAPHY.md, not the stylesheet's own prose. Not synced: the comment must not carry the number at all, so it points at `--prose-max` instead. The two font measurements (8.13px per `ch`, 5.86px per average character) stay, because those are facts about the typeface rather than a decision that can be revised. Guards, each bypass-verified by reverting the fix: - the rhythm gate losing its padding, proving the block is located non-vacuously - the comment restating the current cap, and restating the OLD one (the reviewer's bug) - the comment no longer pointing at --prose-max - plus the reorder above, which the previous mechanism failed and this one survives 30 tests in webviewCss, 32 suites green.
1 parent a9adf3a commit ffa5f0b

2 files changed

Lines changed: 64 additions & 11 deletions

File tree

extensions/levelcode-ai/media/chat.html

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -72,12 +72,15 @@
7272
in this file are all cards, dialogs and the empty state. In a 380px sidebar the container bounded
7373
it, so it never looked wrong — but the chat can now open as an editor tab (AI: Open Chat in
7474
Editor), and at 900px the same CSS produced ~154-character lines.
75-
Measured in the shipped font at 13px: the average prose character is 5.86px, so 680px is ~116
76-
characters — a real improvement while staying wide enough that a fenced code block does not wrap
77-
constantly. NOT expressed in `ch`: `0` is 8.13px here, 39% wider than average text, so a `ch` cap
78-
silently overshoots by about a third.
75+
The cap itself is `--prose-max`, declared on `body`, and is deliberately NOT restated here. This
76+
comment carried its own copy of the number and said 680 for exactly as long as it took the cap to
77+
become 820 — which is what a second copy of a value is always for. docs/CHAT-TYPOGRAPHY.md D1 has
78+
how it was chosen; webviewCss.test.js fails if this comment starts quoting a length again.
79+
NOT expressed in `ch`: `0` measures 8.13px against an average prose character of 5.86px, so a
80+
`ch` cap silently overshoots by about a third. Those two figures stay because they are facts about
81+
the typeface, not a decision that can be revised.
7982
Applied to EVERY direct child, not just .msg, so messages, cards and the activity timeline share
80-
one column instead of drifting apart at width. Below 680px this is inert — which is why the
83+
one column instead of drifting apart at width. Below the cap this is inert — which is why the
8184
sidebar is untouched. */
8285
#log > * { width: 100%; max-width: var(--prose-max); margin-inline: auto; box-sizing: border-box; }
8386
/* ---- per-response copy button (hover-reveal under a completed assistant message) ---- */

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

Lines changed: 56 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,30 @@ const path = require('path');
2121
const html = fs.readFileSync(path.join(__dirname, '..', 'media', 'chat.html'), 'utf8');
2222
const css = html.slice(html.indexOf('<style'), html.indexOf('</style>'));
2323

24+
// Comment-stripped CSS, for anything that reasons about BLOCK STRUCTURE rather than text. A `{` inside
25+
// a comment would throw off brace matching, and the comments in this stylesheet are prose-heavy enough
26+
// that one will eventually contain a brace.
27+
const cssBlocks = css.replace(/\/\*[\s\S]*?\*\//g, '');
28+
29+
/**
30+
* The body of the `{ … }` block that opens at or after `from`, matched by BALANCED braces.
31+
*
32+
* Written because the obvious shortcut is wrong: searching for a literal closing brace (`\n }`) only
33+
* terminates rules that happen to be formatted across multiple lines. A one-line block — like
34+
* `@media (…) { body { --shell-x: 24px; } }` — has no such terminator, so the search runs on into the
35+
* NEXT block and quietly returns a slice spanning both. A test built on that asserts against whatever
36+
* the file's rule order happens to put in reach.
37+
*/
38+
function blockAt(src, from) {
39+
const open = src.indexOf('{', from);
40+
if (open < 0) { return ''; }
41+
let depth = 0;
42+
for (let i = open; i < src.length; i++) {
43+
if (src[i] === '{') { depth++; } else if (src[i] === '}') { depth--; if (!depth) { return src.slice(open + 1, i); } }
44+
}
45+
return ''; // unbalanced — the caller's assertion fails on the empty body, which is the honest result
46+
}
47+
2448
let n = 0;
2549
function test(name, fn) { fn(); n++; console.log(' ok - ' + name); }
2650

@@ -273,6 +297,26 @@ test('TRANSCRIPT: the prose column is bounded, and every child shares the one me
273297
'the measure should be an absolute length, not `ch` — see CHAT-TYPOGRAPHY.md D1');
274298
});
275299

300+
test('TRANSCRIPT: the stylesheet comment keeps no copy of the cap', () => {
301+
// Review found the comment beside the rule still saying "680px" and "~116 characters" after the cap
302+
// became 820 — the third place this number has drifted, and the only one no test was watching. The
303+
// doc pin above covers CHAT-TYPOGRAPHY.md; nothing covered the stylesheet's own prose.
304+
//
305+
// The fix is not to sync it. A comment that restates a value will drift again the next time the
306+
// value changes, so it must not carry the number at all — it points at `--prose-max` instead.
307+
const block = /THE MEASURE[\s\S]*?\*\//.exec(css);
308+
assert.ok(block, 'the THE MEASURE comment is gone — this guard covers nothing');
309+
assert.match(block[0], /--prose-max/, 'the comment must point at the property rather than restate its value');
310+
311+
// 380px and 900px are VIEWPORT widths — facts about where the problem showed up, which cannot go
312+
// stale. Any other length in here is a copy of a decision that can.
313+
const VIEWPORTS = ['380px', '900px'];
314+
const restated = (block[0].match(/\b\d{3,4}px\b/g) || []).filter((v) => !VIEWPORTS.includes(v));
315+
assert.deepStrictEqual(restated, [],
316+
'the measure comment quotes a length again — reference --prose-max instead, or it drifts the next '
317+
+ 'time the cap moves: ' + restated.join(', '));
318+
});
319+
276320
test('SHELL: the composer shares the transcript column instead of spanning the panel', () => {
277321
// docs/CHAT-TYPOGRAPHY.md D9. T1 bounded the TRANSCRIPT and nothing else, so at editor width the
278322
// input was a ~1580px box under an 820px conversation — the single thing that most made the panel
@@ -332,14 +376,20 @@ test('TRANSCRIPT: the looser rhythm is gated to reading width, so the sidebar is
332376
// T1's exit criterion is that a narrow panel renders exactly as before — a user who upgrades and
333377
// never opens the editor tab should see nothing move. Verified against develop's computed styles
334378
// at 520px: padding, gap, paragraph and heading margins, line-height and font-size all identical.
335-
// Located by CONTENT, not by position: there is now more than one `min-width: 760px` gate (D9 gates
336-
// --shell-x on the same breakpoint), and an indexOf would silently grab whichever comes first in the
337-
// file — passing or failing on rule ORDER rather than on the thing being asserted.
338-
const gates = [...css.matchAll(/@media \(min-width: 760px\)/g)].map((m) => m.index);
379+
// Located by CONTENT, not by position: there is more than one `min-width: 760px` gate (D9 gates
380+
// --shell-x on the same breakpoint), so an indexOf would grab whichever comes first in the file and
381+
// assert on rule ORDER rather than on the thing being checked.
382+
//
383+
// And located with BALANCED BRACES, not by searching for a literal `\n }`. Review caught that: the
384+
// --shell-x gate is written on one line, so it has no `\n }` of its own and the search ran straight
385+
// past it into the next multi-line block. It happened to resolve correctly here — the swallowed span
386+
// did not contain `#log {` — but only because of where the rules currently sit. Move one rule and
387+
// the slice silently spans two gates, which is the same order-dependence in a new disguise.
388+
const gates = [...cssBlocks.matchAll(/@media \(min-width: 760px\)/g)].map((m) => m.index);
339389
assert.ok(gates.length, 'the width gate is gone — the rhythm change would now hit the sidebar too');
340-
const at = gates.find((i) => css.slice(i, css.indexOf('\n }', i)).includes('#log {'));
390+
const at = gates.find((i) => blockAt(cssBlocks, i).includes('#log {'));
341391
assert.ok(at !== undefined, 'no min-width:760px gate contains the #log rhythm rules');
342-
const block = css.slice(at, css.indexOf('\n }', at));
392+
const block = blockAt(cssBlocks, at);
343393
assert.match(block, /#log \{[^}]*padding:/, 'the wider page margin belongs inside the gate');
344394
assert.match(block, /margin-bottom:\s*1em/, 'prose spacing must be em-based so T2 scales it');
345395
assert.match(block, /h1[\s\S]*margin:\s*1\.6em 0 \.55em/,

0 commit comments

Comments
 (0)