diff --git a/.pi/skills/yellowjacket-dev/SKILL.md b/.pi/skills/yellowjacket-dev/SKILL.md index 16686c7..b252f1b 100644 --- a/.pi/skills/yellowjacket-dev/SKILL.md +++ b/.pi/skills/yellowjacket-dev/SKILL.md @@ -130,7 +130,13 @@ reference, because you need them *before* the failure, not after. what you otherwise get is `Property 'scroll' does not exist on type 'CSSResult'` pointing at a line of prose, or every test in the suite failing to import. It went in after the trap cost a fourth session in - which its own warning had been read twice. + which its own warning had been read twice. **The same command carries + a second CSS check**: a nested rule whose selector starts with an + element name (`audio-player { … }` rather than `& audio-player { … }`) + is silently dropped by the device's Chrome 113 and by nothing else, so + every tier you can run renders it correctly. Run it after touching + `index.css` or any `css` literal; a rule directly inside a top-level + `@media` is not nested and is not flagged. - **A failing CI job's log is reachable even when `gitea_ci job_logs` says it is not.** That endpoint 404s on this Gitea build. The REST API answers, with the `GITEA_TOKEN` already in the environment: diff --git a/CLAUDE.md b/CLAUDE.md index d5475c0..46cf9fc 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -3496,6 +3496,27 @@ android-inspect` forwards the WebView's devtools socket and `make android-eval` asks the real page — raw CDP, because `connectOverCDP` calls `Browser.setDownloadBehavior` and a WebView refuses it. +**One of those gaps is checked rather than remembered.** A nested rule +whose selector starts with an element name is not a parse error anyone +would notice on 113 — the rule simply does not exist, there and nowhere +else, which is how the bottom bar's `text-overflow: ellipsis` came to +have never truncated on the device. `make css-check` +(`frontend/scripts/check-css-nesting.mjs`, a pre-commit hook and a CI +step) fails on one, over every `frontend/*.css` and the `css` literals +alike — a glob rather than `index.css` by name, because the hook fires +on `frontend/**/*.{ts,css}` and a sweep that names one file goes green +over a stylesheet it never opened — and +says the fix is a leading `&` — valid in both syntaxes, so no nested +rule here has a reason to omit it. Two things it has to get right, and +both follow from asking whether a *style* rule is anywhere above rather +than what the immediate parent is: `@media (…) { bottom-nav { … } }` at +the top level is an ordinary rule and is the majority of what a regex +over the file would report, while the same rule one level inside +`.bar { @media (…) { … } }` is nested and is flagged. The check is the +cheap version of the answer; a build-time downlevel (Lightning CSS +targeting 113) would fix the class permanently and is a dependency and +a build step rather than twenty lines. + `build/config.yml`'s `version` is the *metadata* version and is not what the app reports — `main.version` is stamped at link time from the packaging recipe's git-derived version. diff --git a/Makefile b/Makefile index 83008a4..fb3e26c 100644 --- a/Makefile +++ b/Makefile @@ -172,12 +172,17 @@ ui-setup: ## Install the Vitest browser provider's own Chromium (once) bindings-check: ## Fail if the generated bindings are stale @./scripts/bindings-check.sh +# Two CSS traps that report a long way from their cause, or not at all. # A backtick inside a comment in a css`` literal ends the literal, and # what you get back is a type error about CSSResult, or every test in -# the suite failing to import. Four sessions, three plans. Instant. +# the suite failing to import. Four sessions, three plans. And a nested +# rule starting with an element name is dropped by the device's +# Chrome 113 in silence -- no tier here runs an engine that can see it. +# Instant. .PHONY: css-check -css-check: ## Fail if a css`` literal was ended early by a backtick in a comment +css-check: ## Fail on a css`` literal ended early by a backtick, or a nested rule needing an & @cd frontend && node scripts/check-css-literals.mjs + @cd frontend && node scripts/check-css-nesting.mjs # .pi/ and CLAUDE.md document commands, and a doc that documents a # command wrongly is worse than no doc: an agent runs it confidently. diff --git a/frontend/scripts/check-css-literals.mjs b/frontend/scripts/check-css-literals.mjs index c8738e3..54ec676 100644 --- a/frontend/scripts/check-css-literals.mjs +++ b/frontend/scripts/check-css-literals.mjs @@ -23,97 +23,23 @@ * as the literal contains an unterminated `/*`. Nothing else produces * that, and a legitimate literal cannot contain one. */ -import { readFileSync } from 'node:fs'; -import { globSync } from 'node:fs'; +import { globSync, readFileSync } from 'node:fs'; + +import { taggedLiterals } from './css-literals.mjs'; const TAGS = ['css', 'html', 'svg']; -/** - * Find the end of a template literal that starts at `start` (the index - * of its opening backtick), respecting escapes and `${}` substitutions. - * Returns the index of the closing backtick, or -1. - */ -function endOfTemplate(src, start) { - let depth = 0; - - for (let i = start + 1; i < src.length; i++) { - const c = src[i]; - - if (c === '\\') { - i++; - continue; - } - - if (c === '$' && src[i + 1] === '{') { - depth++; - i++; - continue; - } - - if (c === '}' && depth > 0) { - depth--; - continue; - } - - if (c === '`' && depth === 0) return i; - } - - return -1; -} - -/** Strip `${...}` substitutions, which may legitimately contain anything. */ -function stripSubstitutions(text) { - let out = ''; - let depth = 0; - - for (let i = 0; i < text.length; i++) { - if (text[i] === '$' && text[i + 1] === '{') { - depth++; - i++; - continue; - } - - if (text[i] === '}' && depth > 0) { - depth--; - continue; - } - - if (depth === 0) out += text[i]; - } - - return out; -} - -function lineOf(src, index) { - return src.slice(0, index).split('\n').length; -} - const files = globSync('src/**/*.ts', { cwd: process.cwd() }); const problems = []; for (const file of files) { const src = readFileSync(file, 'utf8'); - const tagPattern = new RegExp(`(^|[^\\w$.])(${TAGS.join('|')})\``, 'g'); - let match; - - while ((match = tagPattern.exec(src)) !== null) { - const open = match.index + match[0].length - 1; - const close = endOfTemplate(src, open); - - if (close === -1) continue; - - const body = stripSubstitutions(src.slice(open + 1, close)); + for (const { tag, body, line } of taggedLiterals(src, TAGS)) { const opens = (body.match(/\/\*/g) ?? []).length; const closes = (body.match(/\*\//g) ?? []).length; - if (opens > closes) { - problems.push({ - file, - line: lineOf(src, open), - tag: match[2], - }); - } + if (opens > closes) problems.push({ file, line, tag }); } } diff --git a/frontend/scripts/check-css-nesting.mjs b/frontend/scripts/check-css-nesting.mjs new file mode 100644 index 0000000..6338793 --- /dev/null +++ b/frontend/scripts/check-css-nesting.mjs @@ -0,0 +1,79 @@ +#!/usr/bin/env node +/** + * Fail on a nested rule whose selector starts with an element name. + * + * See `css-nesting.mjs` for what the phone does with one. No tier here + * can see it: the component tier, the e2e tier and `make ui-visual` all + * run a current Chromium, where the rule applies normally, so the only + * report is a screenshot of the device — which is how the bottom bar's + * title came to have never truncated there. + * + * It covers `index.css` and the `css` literals in the components alike, + * because a shadow-root stylesheet is parsed by the same engine. + */ +import { globSync, readFileSync } from 'node:fs'; + +import { taggedLiterals } from './css-literals.mjs'; +import { findBareNestedRules } from './css-nesting.mjs'; + +const problems = []; + +// Every stylesheet, not `index.css` by name: the hook that runs this +// fires on `frontend/**/*.{ts,css}`, so naming one file promises a +// coverage the sweep does not deliver -- a second stylesheet would be +// silently unswept while the hook still went green over it. There is +// only `index.css` today, which is exactly when this is free to fix. +const stylesheets = globSync('*.css', { cwd: process.cwd() }); + +if (stylesheets.length === 0) { + console.error('css-nesting-check: no stylesheet matched *.css'); + process.exit(1); +} + +for (const file of stylesheets) { + for (const { line, selector } of findBareNestedRules( + readFileSync(file, 'utf8'), + )) { + problems.push({ file, line, selector }); + } +} + +const sources = globSync('src/**/*.ts', { cwd: process.cwd() }); + +// A sweep over an empty glob passes, and this one is expected to find +// nothing, so "it found nothing" has to mean it looked. +if (sources.length === 0) { + console.error('css-nesting-check: no sources matched src/**/*.ts'); + process.exit(1); +} + +for (const file of sources) { + const src = readFileSync(file, 'utf8'); + + for (const literal of taggedLiterals(src, ['css'])) { + for (const { line, selector } of findBareNestedRules(literal.body)) { + problems.push({ file, line: literal.line + line - 1, selector }); + } + } +} + +if (problems.length > 0) { + for (const p of problems) { + console.error( + `${p.file}:${p.line}: nested rule "${p.selector.split('\n')[0]}" starts ` + + 'with an element name — write it as "& ' + + `${p.selector.split('\n')[0]}"`, + ); + } + + console.error( + `\ncss-nesting-check: ${problems.length} problem(s). ` + + 'Chrome 113 (the device) drops a nested rule that does not start ' + + 'with a symbol; the leading & is valid in both syntaxes.', + ); + process.exit(1); +} + +console.log( + `css-nesting-check: ${stylesheets.length} stylesheet(s) + ${sources.length} files, no bare nested rules`, +); diff --git a/frontend/scripts/css-literals.mjs b/frontend/scripts/css-literals.mjs new file mode 100644 index 0000000..702bc51 --- /dev/null +++ b/frontend/scripts/css-literals.mjs @@ -0,0 +1,103 @@ +/** + * Finding the `css` tagged templates in a TypeScript source. + * + * Two checks read them — the unterminated-comment one and the nesting + * one — and a second scanner would be a second thing to keep in step + * with how a template literal actually ends. + */ + +/** + * Find the end of a template literal that starts at `start` (the index + * of its opening backtick), respecting escapes and `${}` substitutions. + * Returns the index of the closing backtick, or -1. + */ +export function endOfTemplate(src, start) { + let depth = 0; + + for (let i = start + 1; i < src.length; i++) { + const c = src[i]; + + if (c === '\\') { + i++; + continue; + } + + if (c === '$' && src[i + 1] === '{') { + depth++; + i++; + continue; + } + + if (c === '}' && depth > 0) { + depth--; + continue; + } + + if (c === '`' && depth === 0) return i; + } + + return -1; +} + +/** + * Strip `${...}` substitutions, which may legitimately contain anything. + * + * Newlines inside them are kept, so a line number taken from the + * stripped text still names the right line of the file it came from. + */ +export function stripSubstitutions(text) { + let out = ''; + let depth = 0; + + for (let i = 0; i < text.length; i++) { + if (text[i] === '$' && text[i + 1] === '{') { + depth++; + i++; + continue; + } + + if (text[i] === '}' && depth > 0) { + depth--; + continue; + } + + if (depth === 0) out += text[i]; + else if (text[i] === '\n') out += '\n'; + } + + return out; +} + +/** The 1-based line number of `index` in `src`. */ +export function lineOf(src, index) { + return src.slice(0, index).split('\n').length; +} + +/** + * Every tagged template literal in `src` whose tag is in `tags`. + * + * `body` has its substitutions stripped and `line` is the line its + * opening backtick sits on, so `line + (n - 1)` is the file line of the + * body's own line `n`. + */ +export function taggedLiterals(src, tags) { + const pattern = new RegExp(`(^|[^\\w$.])(${tags.join('|')})\``, 'g'); + const found = []; + + let match; + + while ((match = pattern.exec(src)) !== null) { + const open = match.index + match[0].length - 1; + const close = endOfTemplate(src, open); + + if (close === -1) continue; + + found.push({ + tag: match[2], + body: stripSubstitutions(src.slice(open + 1, close)), + line: lineOf(src, open), + }); + } + + return found; +} diff --git a/frontend/scripts/css-nesting.mjs b/frontend/scripts/css-nesting.mjs new file mode 100644 index 0000000..006fb5d --- /dev/null +++ b/frontend/scripts/css-nesting.mjs @@ -0,0 +1,119 @@ +/** + * A nested rule whose selector starts with an element name is silently + * dropped on the phone. + * + * The device renders in Chrome 113, which predates relaxed CSS nesting + * (Chrome 120): before that a nested selector had to start with + * something that could not be read as the beginning of a declaration, + * so `.bottom-bar { audio-player { … } }` is not a parse error anyone + * would notice — the inner rule simply does not exist, on the phone and + * only on the phone. Three were live in `index.css`, one of them the + * `text-overflow: ellipsis` on the bottom bar's title, which had + * therefore never truncated on the device. + * + * `& audio-player` is valid in both syntaxes, so no nested rule here + * has any reason to omit it. + * + * Two things the detection has to get right: + * + * - **A rule directly inside an at-rule is not nested.** + * `@media (…) { bottom-nav { … } }` at the top level is an ordinary + * rule and is fine — and it is the majority of the matches a regex + * over the file would produce. What decides it is whether a *style* + * rule is somewhere above, not what the immediate parent is: inside + * `.bar { @media (…) { audio-player { … } } }` the inner rule is + * nested, at-rule in between or not. + * - **A declaration is not a rule.** `background: url(…)` and any + * string or comment can hold a brace, so this tracks them rather than + * matching lines. + */ + +/** Does this selector start with an identifier, rather than a symbol? */ +function startsWithIdent(selector) { + return /^[A-Za-z_\u00A0-\uFFFF]/.test(selector); +} + +/** + * Every nested style rule in `css` whose selector starts with an + * element name, as `{ line, selector }` with a 1-based line. + */ +export function findBareNestedRules(css) { + const found = []; + /** The blocks we are inside, innermost last: 'style' or 'at'. */ + const stack = []; + /** The text since the last `{`, `}` or `;` — a prelude, if a `{` follows. */ + let prelude = ''; + let preludeLine = 1; + let line = 1; + + const startPrelude = () => { + prelude = ''; + preludeLine = line; + }; + + for (let i = 0; i < css.length; i++) { + const c = css[i]; + + if (c === '\n') { + line++; + if (prelude.trim() === '') preludeLine = line; + prelude += c; + continue; + } + + if (c === '/' && css[i + 1] === '*') { + const end = css.indexOf('*/', i + 2); + const comment = css.slice(i, end === -1 ? css.length : end + 2); + + line += (comment.match(/\n/g) ?? []).length; + i += comment.length - 1; + if (prelude.trim() === '') preludeLine = line; + continue; + } + + if (c === '"' || c === "'") { + let j = i + 1; + + while (j < css.length && css[j] !== c) { + if (css[j] === '\\') j++; + j++; + } + + prelude += css.slice(i, j + 1); + i = j; + continue; + } + + if (c === '{') { + const selector = prelude.trim(); + const kind = selector.startsWith('@') ? 'at' : 'style'; + + if ( + kind === 'style' && + stack.includes('style') && + startsWithIdent(selector) + ) { + found.push({ line: preludeLine, selector }); + } + + stack.push(kind); + startPrelude(); + continue; + } + + if (c === '}') { + stack.pop(); + startPrelude(); + continue; + } + + if (c === ';') { + startPrelude(); + continue; + } + + prelude += c; + } + + return found; +} diff --git a/frontend/test/utils/css-nesting.test.ts b/frontend/test/utils/css-nesting.test.ts new file mode 100644 index 0000000..bcfdec6 --- /dev/null +++ b/frontend/test/utils/css-nesting.test.ts @@ -0,0 +1,79 @@ +/** + * The nesting check's own semantics. + * + * `make css-check` runs it over a tree that currently has no violation, + * so the check passing says nothing about whether it can still find + * one. What it has to get right is two distinctions, and both are the + * kind a regex over the file gets wrong: a rule directly inside an + * at-rule is not nested, and a brace inside a string or a comment is + * not a block. + * + * The rule it enforces is the device's: Chrome 113 predates relaxed CSS + * nesting, so a nested selector starting with an element name is + * dropped in silence. See `scripts/css-nesting.mjs`. + */ +import { describe, expect, it } from 'vitest'; + +import { findBareNestedRules } from '../../scripts/css-nesting.mjs'; + +describe('the nested-rule check', () => { + it('flags a nested rule that starts with an element name', () => { + const found = findBareNestedRules( + '.bottom-bar {\n color: red;\n\n audio-player { margin: 0 }\n}', + ); + + expect(found).toEqual([{ line: 4, selector: 'audio-player' }]); + }); + + it('accepts the same rule written with a leading &', () => { + expect( + findBareNestedRules('.bottom-bar {\n & audio-player { margin: 0 }\n}'), + ).toEqual([]); + }); + + it('accepts a nested selector that starts with any other symbol', () => { + expect( + findBareNestedRules('.bar {\n #track-info { color: red }\n}'), + ).toEqual([]); + expect(findBareNestedRules('.bar {\n :host { color: red }\n}')).toEqual( + [], + ); + }); + + it('leaves a top-level rule alone, element name or not', () => { + expect(findBareNestedRules('p {\n margin: 0;\n}')).toEqual([]); + }); + + /** + * The majority of what a naive sweep would report: a media query at + * the top level holds ordinary rules, not nested ones. + */ + it('leaves a rule directly inside an at-rule alone', () => { + expect( + findBareNestedRules( + '@media (max-width: 599px) {\n bottom-nav { display: flex }\n}', + ), + ).toEqual([]); + }); + + /** + * And the other half of that: what decides it is whether a style rule + * is anywhere above, not what the immediate parent is. + */ + it('flags one inside an at-rule that is itself inside a rule', () => { + expect( + findBareNestedRules( + '.bar {\n @media (min-width: 900px) {\n audio-player { margin: 0 }\n }\n}', + ), + ).toEqual([{ line: 3, selector: 'audio-player' }]); + }); + + it('reads through a brace in a string or a comment', () => { + expect( + findBareNestedRules('.a {\n background: url("x{y}");\n}'), + ).toEqual([]); + expect( + findBareNestedRules('.a {\n /* audio-player { x: y } */\n}'), + ).toEqual([]); + }); +}); diff --git a/lefthook.yml b/lefthook.yml index 70a03ba..bbc42ce 100644 --- a/lefthook.yml +++ b/lefthook.yml @@ -58,6 +58,15 @@ pre-commit: root: "frontend/" run: node scripts/check-css-literals.mjs + # A nested rule whose selector starts with an element name is + # silently dropped by the device's Chrome 113, and by nothing else -- + # so every tier here renders it correctly and only a screenshot of + # the phone disagrees. Instant. + css-nesting: + glob: "frontend/**/*.{ts,css}" + root: "frontend/" + run: node scripts/check-css-nesting.mjs + # Deliberately sequential, unlike pre-commit. `go test -race` # saturates every core for the better part of a minute and the UI tier # is a real browser with wall-clock timeouts, so run together the