From 19f6c10c3ff8a21b10d19559ff59efcfcc1df6cf Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 29 Oct 2026 14:31:45 -0700 Subject: [PATCH] perf(renderer): drop group-has-[:focus-visible] rules that restyle whole subtrees (#36098) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #34312. Takes over naufaldi/orca#1 by @naufaldi, with the main.css part dropped because #34932 already covers it. - SshTargetCard and the reasoning chevron now use the scoped `[[class~='group/x']:has(:focus-visible)_&]` shape. Tailwind's `group-has-*` compiles to a non-subject `:has()` whose subject is `*`, and that rule made Blink restyle whole subtrees on every mutation. This change is the entire measured win: 19.4 → 1.6 ms, 12.5 → 3.6 ms and 8.0 → 2.4 ms per mutation. - rich-markdown-editor.css drops a redundant `:has()` on the TOC header, as defence in depth. - Adds a guard, check-has-restyle-scope.mjs. It compiles the renderer's Tailwind CSS, adds plain stylesheets and the listed vendor stylesheets, and fails on any non-subject `:has()` whose subject has no class, id or attribute. Three pdf.js rules are baselined after being measured as harmless. It runs in lint, check:changed and PR CI in about 1.5 s. Co-authored-by: Naufaldi --- .github/workflows/pr.yml | 4 + config/scripts/check-changed-selection.mjs | 8 + config/scripts/check-has-restyle-scope.mjs | 258 ++++++++++++++++++ .../scripts/check-has-restyle-scope.test.mjs | 68 +++++ config/scripts/run-lint.mjs | 1 + package.json | 1 + .../src/assets/rich-markdown-editor.css | 2 +- .../NativeChatReasoningDisclosure.tsx | 2 +- .../src/components/settings/SshTargetCard.tsx | 2 +- 9 files changed, 343 insertions(+), 3 deletions(-) create mode 100644 config/scripts/check-has-restyle-scope.mjs create mode 100644 config/scripts/check-has-restyle-scope.test.mjs diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 83312489af1..233bcd1efe8 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -376,6 +376,10 @@ jobs: if: needs.code_paths.outputs.static_analysis == 'true' run: node config/scripts/check-reliability-gates.mjs + - name: Check compiled CSS :has() restyle scope + if: needs.code_paths.outputs.static_analysis == 'true' + run: node config/scripts/check-has-restyle-scope.mjs + - name: Enforce dead design-system classes if: needs.code_paths.outputs.static_analysis == 'true' run: node node_modules/oxlint/bin/oxlint --config config/oxlint-dead-classes.json src/renderer diff --git a/config/scripts/check-changed-selection.mjs b/config/scripts/check-changed-selection.mjs index 960dcd3737f..67771e122dd 100644 --- a/config/scripts/check-changed-selection.mjs +++ b/config/scripts/check-changed-selection.mjs @@ -144,6 +144,14 @@ export const VERIFY_CHECKS = [ args: ['config/scripts/check-readme-local-links.mjs'], inputs: ['README.md', 'docs/readme/'] }, + { + // Compiles Tailwind (~1.5s), so it runs only for inputs that can change the emitted CSS. + name: 'compiled :has() restyle scope', + args: ['config/scripts/check-has-restyle-scope.mjs'], + inputs: ['electron.vite.config.ts'], + matches: (files) => + files.some((file) => /^src\/(?:renderer\/.*\.(?:css|tsx?)|shared\/.*\.css)$/.test(file)) + }, { name: 'import boundaries', runtime: 'bun', diff --git a/config/scripts/check-has-restyle-scope.mjs b/config/scripts/check-has-restyle-scope.mjs new file mode 100644 index 00000000000..ff2ff5e61ab --- /dev/null +++ b/config/scripts/check-has-restyle-scope.mjs @@ -0,0 +1,258 @@ +// Rejects `:has()` rules that make Blink restyle the anchor's whole subtree on any mutation +// (#34312). Checks Tailwind's compiled output plus plain and listed package stylesheets. +// Not covered: `?inline` imports, template-literal imports, `.js` files, relative node_modules paths. +import { execFileSync } from 'node:child_process' +import { existsSync, readFileSync } from 'node:fs' +import { createRequire } from 'node:module' +import path from 'node:path' +import process from 'node:process' +import { pathToFileURL } from 'node:url' + +const TAILWIND_ENTRY = 'src/renderer/src/assets/main.css' +// Plain pathspec `*` also matches `/`; files main.css imports are rescanned harmlessly. +const PLAIN_CSS = ['src/renderer/*.css', 'src/shared/*.css'] +// Package stylesheets imported from renderer JS (xterm is also compiled in through main.css). +const VENDOR_CSS = [ + '@xterm/xterm/css/xterm.css', + 'monaco-editor/min/vs/editor/editor.main.css', + 'pdfjs-dist/web/pdf_viewer.css', + 'react-grab/styles.css' +] +// Accepted vendor findings. pdf.js's sheet stays loaded once the lazy ImageViewer first opens, but +// these anchor on viewer-only widgets: the #34312 harness showed no subtree restyle and the same +// cost as with every pdf.js :has() rule removed. A vendor bump that adds a finding fails here. +const VENDOR_BASELINE = new Set([ + 'pdfjs-dist/web/pdf_viewer.css: .annotationLayer .buttonWidgetAnnotation:is(.checkBox, .radioButton):has([data-canvas-name="checked"]) input:checked', + 'pdfjs-dist/web/pdf_viewer.css: .annotationLayer .buttonWidgetAnnotation:is(.checkBox, .radioButton):has([data-canvas-name="unchecked"]) input:not(:checked)', + 'pdfjs-dist/web/pdf_viewer.css: #addSignatureDialog .mainContainer #addSignatureActionContainer #addSignatureControls #horizontalContainer #addSignatureDescriptionContainer:has(input:disabled) > label' +]) +const CSS_IMPORT = + /(?:\bimport\s*(?:[\w*{}\s,]+?\bfrom\s*)?\(?|@import\s+(?:url\()?)\s*(?:['"]([^'"]+\.css)['"]|([^'"()\s]+\.css)\s*\))/g +const IMPORT_SCAN = ['src/renderer/*.ts', 'src/renderer/*.tsx', ...PLAIN_CSS] +const LS_FILES = ['ls-files', '-z', '--cached', '--others', '--exclude-standard', '--'] +const GREP_CSS = ['grep', '-lz', '--untracked', '-F', '.css', '--'] +// Whole-line comments only: stripping mid-line `//` would eat imports after a URL string. +const COMMENT = /^\s*\/\*[\s\S]*?\*\/|^\s*\/\/.*$/gm +const SPECIFIC = new Set(['class', 'id', 'attribute']) +const SIBLING = new Set(['next-sibling', 'later-sibling']) + +// Tailwind's own packages, resolved through the direct @tailwindcss/vite dependency. +function loadToolchain(root) { + const fromVite = createRequire( + createRequire(path.join(root, 'package.json')).resolve('@tailwindcss/vite') + ) + const nodePath = fromVite.resolve('@tailwindcss/node') + return { + tailwind: fromVite(nodePath), + oxide: fromVite('@tailwindcss/oxide'), + lightningcss: createRequire(nodePath)('lightningcss') + } +} + +function compounds(selector) { + const result = [[]] + for (const component of selector) { + if (component.type === 'combinator') { + result.push([]) + } else { + result.at(-1).push(component) + } + } + return result.filter((compound) => compound.length > 0) +} + +// Blink's descendant invalidation set keys on the subject's class/id/attribute; without one it is the whole subtree. +function isBroad(compound) { + return !compound.some( + (component) => + SPECIFIC.has(component.type) || + (['is', 'where'].includes(component.kind) && + component.selectors.every((branch) => !isBroad(compounds(branch).at(-1) ?? []))) + ) +} + +// Only `:has(+ x)` / `:has(~ x)` (a leading :scope) stay among siblings; `:has(.a + .b)` starts with a descendant. +function isSiblingOnly(relativeSelectors) { + return relativeSelectors.every( + ([first, ...rest]) => + first?.type === 'pseudo-class' && + first.kind === 'scope' && + rest.every((component) => component.type !== 'combinator' || SIBLING.has(component.value)) + ) +} + +// `outerSubject` is the rule's final subject when this selector sits inside a non-subject compound, else null. +function visitSelector(selector, outerSubject, report) { + const parts = compounds(selector) + parts.forEach((compound, index) => { + const subject = index === parts.length - 1 ? outerSubject : (outerSubject ?? parts.at(-1)) + for (const component of compound) { + // `:host()` holds one selector, not a list, and is outside the light DOM. + if (component.type !== 'pseudo-class' || component.kind === 'host') { + continue + } + if (component.kind === 'has') { + if (subject && isBroad(subject) && !isSiblingOnly(component.selectors)) { + report() + } + component.selectors.forEach((nested) => visitSelector(nested, null, report)) + continue + } + // :is()/:where()/:not()/:nth-child(of) arguments match this same element, so they keep its context. + for (const nested of [...(component.selectors ?? []), ...(component.of ?? [])]) { + visitSelector(nested, subject, report) + } + } + }) +} + +// Top-level commas only; escapes and brackets in Tailwind class names stay intact. +function splitSelectorList(text) { + const parts = [''] + let depth = 0 + for (let index = 0; index < text.length; index++) { + const char = text[index] + if (char === '\\') { + parts[parts.length - 1] += text.slice(index, index + 2) + index++ + continue + } + depth += '(['.includes(char) ? 1 : ')]'.includes(char) ? -1 : 0 + if (char === ',' && depth === 0) { + parts.push('') + } else { + parts[parts.length - 1] += char + } + } + return parts.map((part) => part.trim().replace(/\s+/g, ' ')) +} + +/** Selectors whose non-subject `:has()` invalidates a whole subtree; nesting is lowered first. */ +export function findHasRestyleProblems(css, { lightningcss } = loadToolchain(process.cwd())) { + const lowered = lightningcss.transform({ + filename: 'input.css', + code: Buffer.from(css), + include: lightningcss.Features.Nesting, + errorRecovery: true + }) + const problems = new Set() + const flat = lowered.code.toString() + const lines = flat.split('\n') + lightningcss.transform({ + filename: 'flat.css', + code: Buffer.from(flat), + errorRecovery: true, + visitor: { + Rule: { + style(rule) { + rule.value.selectors.forEach((selector, index) => + visitSelector(selector, null, () => { + // Selector text only for findings: slicing the sheet per rule dominated the runtime. + const { line, column } = rule.value.loc + const tail = lines.slice(line).join('\n') + const texts = splitSelectorList(tail.slice(column - 1, tail.indexOf('{', column - 1))) + problems.add(texts[index] ?? texts.join(', ')) + }) + ) + } + } + } + }) + return [...problems] +} + +/** Tailwind's build of `css` as Vite would emit it, scanning `scanBase` for candidates. */ +export async function compileTailwind( + css, + { base, scanBase, toolchain = loadToolchain(process.cwd()) } +) { + const { tailwind, oxide } = toolchain + const compiler = await tailwind.compile(css, { base, onDependency() {} }) + // A null root means auto-detection from `scanBase`; `source(none)` turns scanning off. + const autoSource = { base: scanBase, pattern: '**/*', ...compiler.root, negated: false } + const detected = compiler.root === 'none' ? [] : [autoSource] + const scanner = new oxide.Scanner({ sources: [...detected, ...compiler.sources] }) + return compiler.build(scanner.scan()) +} + +// Tracked plus untracked-but-not-ignored paths, so local work is checked before it is staged. +function gitPaths(root, args) { + return execFileSync('git', ['-c', 'submodule.recurse=false', ...args], { + cwd: root, + encoding: 'utf8' + }) + .split('\0') + .filter((file) => file && file !== TAILWIND_ENTRY && existsSync(path.join(root, file))) +} + +// Package CSS that renderer TS/TSX imports or a plain stylesheet @imports; main.css compiles its own. +function unlistedVendorImports(root) { + const specifiers = new Set() + // git grep prefilters ~13k renderer files; reading each one dominated the runtime. + for (const file of gitPaths(root, [...GREP_CSS, ...IMPORT_SCAN])) { + const source = readFileSync(path.join(root, file), 'utf8').replace(COMMENT, '') + for (const [, quoted, unquoted] of source.matchAll(CSS_IMPORT)) { + specifiers.add(quoted ?? unquoted) + } + } + return [...specifiers] + .filter((specifier) => !/^(?:\.|\/|@\/)/.test(specifier) && !VENDOR_CSS.includes(specifier)) + .sort() +} + +export async function main(root = process.cwd()) { + const toolchain = loadToolchain(root) + const unlisted = unlistedVendorImports(root) + if (unlisted.length > 0) { + console.error(`Add renderer-imported stylesheets to VENDOR_CSS: ${unlisted.join(', ')}`) + return 1 + } + const resolveVendor = createRequire(path.join(root, 'src/renderer/src/main.tsx')).resolve + const entry = path.join(root, TAILWIND_ENTRY) + const sources = [ + { + name: `${TAILWIND_ENTRY} (compiled)`, + css: await compileTailwind(readFileSync(entry, 'utf8'), { + base: path.dirname(entry), + scanBase: path.join(root, 'src/renderer'), + toolchain + }) + }, + ...gitPaths(root, [...LS_FILES, ...PLAIN_CSS]).map((file) => ({ + name: file, + css: readFileSync(path.join(root, file), 'utf8') + })), + ...VENDOR_CSS.map((specifier) => ({ + name: specifier, + css: readFileSync(resolveVendor(specifier), 'utf8') + })) + ] + const all = sources.flatMap(({ name, css }) => + findHasRestyleProblems(css, toolchain).map((selector) => `${name}: ${selector}`) + ) + const stale = [...VENDOR_BASELINE].filter((entry) => !all.includes(entry)) + if (stale.length > 0) { + console.error(`Remove stale VENDOR_BASELINE entries:\n- ${stale.join('\n- ')}`) + return 1 + } + const findings = all.filter((finding) => !VENDOR_BASELINE.has(finding)) + if (findings.length > 0) { + console.error(`:has() restyle-scope check failed with ${findings.length} finding(s):`) + for (const finding of findings) { + console.error(`- ${finding}`) + } + console.error( + 'A :has() followed by a combinator into a subject with no class, id or attribute makes ' + + 'Blink restyle the whole subtree on any mutation (#34312). Give the subject a class, e.g. ' + + "[[class~='group/x']:has(…)_&]:… instead of group-has-*." + ) + return 1 + } + console.log(`:has() restyle-scope check passed (${sources.length} stylesheets).`) + return 0 +} + +if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) { + // An uninstalled VENDOR_CSS package surfaces as Node's "Cannot find module" message. + process.exit(await main().catch((error) => (console.error(error.message), 1))) +} diff --git a/config/scripts/check-has-restyle-scope.test.mjs b/config/scripts/check-has-restyle-scope.test.mjs new file mode 100644 index 00000000000..3836094c1fb --- /dev/null +++ b/config/scripts/check-has-restyle-scope.test.mjs @@ -0,0 +1,68 @@ +import path from 'node:path' +import { describe, expect, it } from 'vitest' +import { compileTailwind, findHasRestyleProblems } from './check-has-restyle-scope.mjs' + +// Compiled shapes (the Tailwind spelling that emits each is noted); each restyles the whole anchor subtree. +// Rules need a declaration: lightningcss drops empty ones before the check sees them. +const subtreeWide = [ + '.a:is(:where(.group\\/row):has(:is(:focus-visible)) *)', // group-has-[…]/row + '.a:is(:where(.group\\/x):is(:has(:focus-visible)) *)', // group-[:has(…)]/x + '.a:is(:where(.peer):is(:has(.x)) ~ *)', // peer-[:has(…)] + '.a:is(:where(.peer):has(:checked) ~ *)', // peer-has-checked + '.a:not(:is(:where(.group):has(:is(.x)) *))', // not-group-has-[…] + '.a:is(:where(.group):not(:has(:is(.x))) *)', // group-not-has-[…] + ':is(.a:has(.x) > *)', // [&:has(x)]:*: + ':is(.a:has(.x) *)', // [&:has(x)]:**: + ':is(.a:has(:is(.x)) *)', // has-[x]:**: + '.row:has(.x) .a *', // [.row:has(x)_&_*] + '.a:has(:is(.x)) svg', // has-[x]:[&_svg]: + '.a:has(.x) svg', // [&:has(x)_svg] + ':is(.anchor:has(.trigger) .scoped) *', // [&:is(.anchor:has(.trigger)_.scoped)_*] + '.a:nth-child(2n of .g:has(.x) *)', + '.zz:has(.a + .b) span', + '.foo:has(.a ~ .b) *', + '.zz:has(:focus-visible) span', + '.zz:has(.x) ::before', + ':is(body:has(.modal)) span' +] + +const scoped = [ + "[class~='group/row']:has(:focus-visible) .a", + '.card:has([data-slot="card-action"])', + '.x .comment-md-p > strong:first-child:has(+ br) + br', + '.zz:has(~ .y) span', + 'body:has(.qq) .yy', + ':is(body:has(.modal)) .x', + ':is(.anchor:has(.trigger) .scoped) .ok', + '.a:is(:where(.group\\/row):hover *)', + '.anchor:has(.x) :where(.scoped), .anchor:has(.x) :is(.scoped, #target)', + ':host(.dark) .card:has(.x)', + "body > :is([data-slot='dialog-content']):has(~ [data-slot='dialog-overlay'])" +] + +describe('findHasRestyleProblems', () => { + it.each(subtreeWide)('flags %s', (selector) => { + expect(findHasRestyleProblems(`${selector} { color: red }`)).toHaveLength(1) + }) + + it.each(scoped)('allows %s', (selector) => { + expect(findHasRestyleProblems(`${selector} { color: red }`)).toEqual([]) + }) + + it('sees rules through nesting, @layer and @media', () => { + expect(findHasRestyleProblems('.zz:has(.x) { & * { color: red } }')).toHaveLength(1) + expect( + findHasRestyleProblems('@layer u { @media (hover: hover) { .zz:has(.x) i { color: red } } }') + ).toHaveLength(1) + }) + + it('flags a group-has-* utility through the real Tailwind compile', async () => { + const css = await compileTailwind( + "@import 'tailwindcss/utilities' source(none);\n@source inline('group-has-[:focus-visible]/row:opacity-100');", + { base: path.resolve(import.meta.dirname, '../../src/renderer/src/assets') } + ) + expect(findHasRestyleProblems(css)).toEqual([ + '.group-has-\\[\\:focus-visible\\]\\/row\\:opacity-100:is(:where(.group\\/row):has(:focus-visible) *)' + ]) + }) +}) diff --git a/config/scripts/run-lint.mjs b/config/scripts/run-lint.mjs index de3123fcedc..b2b7e775943 100644 --- a/config/scripts/run-lint.mjs +++ b/config/scripts/run-lint.mjs @@ -21,6 +21,7 @@ export const LINT_STEPS = [ 'verify:mobile-localization-coverage', 'lint:ast-grep-gates', 'check:main-sync-spawn-ratchet', + 'check:has-restyle-scope', 'lint:js-plugins', 'oxlint', 'check:dead-classes', diff --git a/package.json b/package.json index 7cf074f2ec5..943aa1e5657 100644 --- a/package.json +++ b/package.json @@ -65,6 +65,7 @@ "check:focus-read-ratchet": "node config/scripts/check-focus-read-ratchet.mjs", "check:layout-writer-ratchets": "node config/scripts/check-layout-writer-ratchets.mjs", "check:readme-local-links": "node config/scripts/check-readme-local-links.mjs", + "check:has-restyle-scope": "node config/scripts/check-has-restyle-scope.mjs", "check:daemon-protocol-crossing": "node config/scripts/check-daemon-protocol-crossing.mjs", "check:runtime-launcher-protocol-ratchet": "node config/scripts/check-runtime-launcher-protocol-ratchet.mjs", "check:node-runtime-pin": "node config/scripts/check-node-runtime-pin.mjs", diff --git a/src/renderer/src/assets/rich-markdown-editor.css b/src/renderer/src/assets/rich-markdown-editor.css index 1422b6d1438..61df48dcc5f 100644 --- a/src/renderer/src/assets/rich-markdown-editor.css +++ b/src/renderer/src/assets/rich-markdown-editor.css @@ -32,7 +32,7 @@ } /* Why: the TOC header uses the same 40px chrome row; swap its border for an inset shadow so both dividers align. */ -.rich-markdown-editor-layout:has(.markdown-toc-panel) .markdown-toc-header { +.rich-markdown-editor-layout .markdown-toc-header { border-bottom: none; box-shadow: inset 0 -1px 0 color-mix(in srgb, var(--border) 72%, transparent); } diff --git a/src/renderer/src/components/native-chat/NativeChatReasoningDisclosure.tsx b/src/renderer/src/components/native-chat/NativeChatReasoningDisclosure.tsx index 2093d55715c..9db5b682d68 100644 --- a/src/renderer/src/components/native-chat/NativeChatReasoningDisclosure.tsx +++ b/src/renderer/src/components/native-chat/NativeChatReasoningDisclosure.tsx @@ -43,7 +43,7 @@ export function NativeChatReasoningChevron(): React.JSX.Element { return ( ) } diff --git a/src/renderer/src/components/settings/SshTargetCard.tsx b/src/renderer/src/components/settings/SshTargetCard.tsx index 0184b274ade..23a8e84e699 100644 --- a/src/renderer/src/components/settings/SshTargetCard.tsx +++ b/src/renderer/src/components/settings/SshTargetCard.tsx @@ -189,7 +189,7 @@ export function SshTargetCard({ } // Reveal repetitive Connect labels on hover/focus while retaining width to avoid shifts. return ( - + {button} )