refactor(lint): drop seven rules that fire on correct compositions - #3366
Conversation
Each rule below either reports a hazard the compiler or runtime already
prevents, duplicates another rule's invariant with a weaker detector, or
cannot be cleared by its own fixHint. Measured over the 643 shipped
registry HTML files, this cuts lint output from 1740 findings to 507
(-70.9%) and removes 40 errors, with no new codes introduced.
- scene_layer_missing_visibility_kill: regex heuristic keyed on `#sceneN`
ids. It only accepts the literal string `visibility: "hidden"`, so the
canonical GSAP hard kill (`tl.set(el, { autoAlpha: 0 })`, which sets
visibility hidden at runtime) never clears it — an unfixable error. It
also matched the `0` inside `opacity: 0.5` and treated `.from({opacity:
0})` entrances as exits. gsap_exit_missing_hard_kill owns this invariant
using parsed tween timing and real clip boundaries, and accepts every
hidden encoding.
- unscoped_gsap_selector: wrapScopedCompositionScript already rewrites
string GSAP targets to the composition root for every sub-composition
script (pinned by compositionScoping.test.ts "executes document and GSAP
selectors inside the composition root"). The rule also never fired on a
standalone sub-composition file or a <template> sub-comp.
- caption_transcript_parse_error: required the inline TRANSCRIPT array to
be strict JSON so Studio could read it, but Studio's parseTranscriptArray
already normalizes unquoted keys, single quotes, and trailing commas. It
errored on ten shipped caption components whose transcripts Studio parses.
- composition_self_attribute_selector: warned that
`[data-composition-id="x"] .y` leaks across instances, but
scopeCssToComposition rewrites that selector to each instance's runtime
scope. It was also the pattern the rest of the toolchain prescribes.
- timed_element_missing_visibility_hidden: strict subset of
timed_element_missing_clip_class, which reports the same condition as an
error, so it only ever added a second line saying the same thing.
- pointer_events_none: Studio selection ergonomics only, no render impact,
on 124 of 211 shipped blocks.
- google_fonts_import: the producer resolves Google Fonts during
compile/render, as the message itself said.
system_font_will_alias is narrowed to distributed/Lambda renders, where
system-font capture is off and the fallback is a real defect. Under a local
render the substitution is the renderer working as designed, so the info
tier is gone.
The three tests that used composition_self_attribute_selector as a probe
for "this style source was collected" now use scoped_css_missing_wrapper,
which still fires once per source.
terencecho
left a comment
terencecho
left a comment
There was a problem hiding this comment.
Approve. Walked each of the seven dropped rules against the surviving coverage and the runtime that supposedly makes them false positives; PR-body claims hold.
Per rule
scene_layer_missing_visibility_kill— kill-regex was literalvisibility\s*:\s*["']hidden["'], sotl.set(el, { autoAlpha: 0 })never cleared it, and the exit-detect regexopacity\s*:\s*0also matches insideopacity: 0.5and on.from({ opacity: 0 })entrances. Coverage moves togsap_exit_missing_hard_kill, which usesisHardKillSet→isHiddenGsapState; that acceptsopacity: 0,autoAlpha: 0,visibility: hidden, anddisplay: none(gsap.ts:207–216, 283–290). Also keyed to real clip-start boundaries + parsed tween timing, not an ad-hoc#sceneNid convention. Strict upgrade.unscoped_gsap_selector— guarded byif (!localTimelineCompId || localTimelineCompId === rootCompositionId) continue;, so it only fires when a script's registered timeline id differs from the root — i.e. the multi-root single-file shape thatmultiple_root_compositionsalready errors on (project.ts:433). AndwrapScopedCompositionScriptrewrites string GSAP targets to the composition root at compile time (compositionScoping.ts:273+,__hfNormalizeSelector), so even if the shape slipped through, the described data-loss failure mode doesn't happen. Genuinely obsolete.caption_transcript_parse_error— used strictJSON.parseon the extracted array literal. Studio'sparseTranscriptArray(studio/src/captions/parser.ts:274–289) explicitly normalizes single quotes → double, unquoted keys → quoted, and strips trailing commas before parsing. Any transcript Studio accepts that isn't strict JSON was a guaranteed false positive with no runtime consequence.composition_self_attribute_selector—scopeCssToComposition(compositionScoping.ts:221–251) rewrites[data-composition-id="x"] …selectors to each instance's runtime scope, so the leak the rule warned about doesn't occur. The three tests that used it as a probe for "was this style source collected" now target a nonexistent composition id and check forscoped_css_missing_wrapperinstead — one finding per source, coverage preserved.timed_element_missing_visibility_hidden— info severity, strict subset. Both rules skip whenclass="clip"is present; when it isn't,timed_element_missing_clip_class(composition.ts:544–575) fires as an error regardless of hidden-style fallback. The info-level finding only ever restated (a subset of) the error.pointer_events_none— pure Studio-selectability ergonomics, no render effect.grep -rn "pointer-events\s*:\s*none" registry/blocks/shows ~200 hits across the ~211 blocks, so the 124/211 figure in the description reads right.google_fonts_import— the message itself said "the producer resolves Google Fonts during compile/render". Confirmed in producer/src/services/deterministicFonts.ts (lines 587–1146: fetch → cache → serve). Self-cancelling warning.system_font_will_aliasnarrowing — kept the correct half (distributed / Lambda: capture is off, fallback is a real defect) and dropped the info-severity local half where the substitution is the renderer working as designed. One tiny thing to note (not a blocker for this PR):lintProjectdoesn't currently thread adistributedflag through tolintHyperframeHtml, so the narrowed rule is dormant in every CLI call today — grep fordistributed:\s*trueoutside tests returns nothing. Whoever picks up the Lambda-side lint entry should wiredistributed: truethere; the rule is correctly shaped to catch the real defect once that lands.
Meta
- CI: all completed checks SUCCESS/SKIPPED at head; no in-progress;
mergeStateStatus: BLOCKEDis justREVIEW_REQUIRED. - Diff scope is well-contained: rule bodies + dead helpers (
countClassUsage,isSuspiciousGlobalSelector,getSingleClassSelector,extractArrayLiteral,selectorTargetsCompositionId,escapeRegExp), plus three test rewrites that swap the retired probe rule forscoped_css_missing_wrapper. Numbers match: -518 mostly test bodies + rule bodies + helpers.
— Review by tai (pr-review)
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at abfea745. No blockers — the deletion is clean and I could not find a dropped defect class. tai already walked all seven rules against their surviving coverage at this same head, so I'm not restating that; below is only the delta.
The "strict subset" claim is not literally true — but it fails in the safe direction
The body (and tai's review) grade timed_element_missing_visibility_hidden as a strict subset of timed_element_missing_clip_class. I extracted both predicates and ran them against synthetic tags rather than comparing them by eye, and the skip sets differ:
- deleted rule skips
audio,script,style - surviving rule skips
audio,video,script,style,template
So there are exactly two element types where the deleted rule fired and nothing survives:
| case | deleted rule | surviving rule |
|---|---|---|
<div data-start> no clip |
1 | 1 |
<img data-start> no clip |
1 | 1 |
<video data-start> no clip |
1 | 0 |
<template data-start> no clip |
1 | 0 |
<div data-start style="visibility:hidden"> |
0 | 1 |
<div data-duration> only |
0 | 1 |
The conclusion still holds, for a better reason than the one stated. <video id data-start data-duration data-track-index src> with no class="clip" is the documented canonical pattern (packages/core/docs/core.md:117-118, :207-209), and <video class="clip"> is authored nowhere outside packages/engine/src/services/audioMixer.test.ts. video_nested_in_timed_element reinforces it — its fixHint tells authors to make the <video> a direct child of the stage. And <template data-start> has zero occurrences in the tree.
So that extra reach was pure false-positive, and removing it is better than "subset" implies, not worse. Worth correcting in the body anyway: the next person who touches timed_element_missing_clip_class's skipTags would otherwise inherit "an info-level rule covered the same ground" as a verified premise, when for video and template nothing does.
Deletion hygiene — confirmations, no action needed
- All seven removed codes have zero references anywhere in the tree at this head, not just inside the diff. Nothing in docs, skills, registry, fixtures or Studio still names them, which matches the "none of these codes appeared in docs or skills" claim.
- One decoy worth recording so nobody re-files it:
escapeRegExpis listed as a removed dead helper, andpackages/lint/src/rules/gsap.ts:745still calls it. That is not a dangling reference —core.ts,gsap.tsandmedia.tseach carried their own local copy, and onlycore.ts's (whose sole consumer was the deletedselectorTargetsCompositionId) went away.gsap.tsandmedia.tskeep theirs and still use them.
One cross-PR consequence, detail on #3367
This PR renumbers the rule arrays that #3367's slowest_rule property uses as its identity (<group>#<index>): 34 of the 81 surviving slots change meaning. The two PRs have disjoint file sets and both branch off the same commit, so they merge clean and nothing goes red. Written up on #3367 — nothing to change here.
— Review by Rames (pr-review), James's assistant
…parable Review catch on #3367: `slowest_rule` is the one positional key in either event. It is `<group>#<index>`, so adding or removing a rule renumbers every later slot in that group and the same string means different rules in two builds. #3366 does exactly that to 34 of 81 surviving slots, and `rule_count` alone says only THAT the ruleset moved, not which groups. `rule_group_counts` carries the per-group sizes alongside it, so a consumer comparing two builds can tell which groups' indices still mean the same thing without anyone having to remember which release dropped rules. `codes`, `code_counts` and `rule_group_ms` are keyed by name and were never affected. Also corrects the rule count in the RULE_GROUPS comment: 86, not ~60, as LINT_RULE_COUNT in the same file computes.
|
Thanks both. One correction taken, one decoy recorded. "strict subset" was wrong — PR body corrected. You're right that the skip sets differ, and I'd stated the claim more strongly than the code supports. Verified independently: the deleted rule skipped Agreed that losing it is an improvement rather than a gap, and I confirmed the reason against the source rather than taking it on faith —
Cross-PR numbering — fixed on #3367 rather than here. Good catch that One thing I found while verifying, filed separately rather than folded in here. Linting the exact primitive-clip block from So the documented canonical example does not pass its own linter. That's outside this PR's scope and I'm not expanding it, but it's the same class of defect and worth its own change. |
…verge (#3367) * feat(telemetry): measure which lint rules fire, cost, and fail to converge Lint rule changes are currently argued from anecdote. This adds the three measurements needed to argue them from data. `lint_report`, once per `hyperframes lint` or `hyperframes check`: - `code_counts` / `codes` — which rules actually fire, and how often - `rule_group_ms` — milliseconds per rule-source module (core, gsap, media, ...) - `slowest_rule` / `slowest_rule_ms` — slowest single rule as `<group>#<index>` - `rule_count` — how many rules this build ran `lint_rule_streak`, once per finding that survives an edit to its file: - `edits` — how many edits the finding survived - `cleared` — whether it eventually went away The streak event is the one that matters. A lint pass costs about 5ms, so per-rule CPU is not what makes the authoring loop slow; a rule an agent cannot satisfy is, because every failed attempt costs a full edit-and-relint cycle. A single run cannot see that, so `lint_rule_streak` reconstructs it across runs: high `edits` with `cleared: false` is a rule nobody can fix, and the `cleared: true` distribution is the baseline to judge it against. An iteration is counted only when the file's content digest CHANGED and the finding is still there. Re-linting an untouched project is not an attempt, which is what stops `check` (which lints on every invocation) from inflating the numbers. Rule identity is the source module plus an index within it. Naming all 86 rules would make the timings prettier but it is a refactor this measurement does not need: the group locates the file, and the index locates the rule. Version, agent runtime, CI flag, and invocation id are already attached to every event by `trackEvent`, so lint pain can be split by CLI version and by which agent produced it without adding anything here. Privacy: only rule codes, counts, and timings are sent. Streak state lives in ~/.hyperframes/lint-streaks.json alongside config.json (so `rm -rf ~/.hyperframes` is still a full reset) and stores digests only — no file paths, no project names, no composition source. Nothing is written and nothing is emitted when telemetry is off. Entries expire after 14 days and are capped at 500 files. `EventProperties` gains string arrays and numeric maps. `codes` and `code_counts` are inherently a set and a histogram; flattening them into dynamic top-level keys would make them unqueryable. PostHog stores both natively. `trackLintRun` is the single call site shared by `lint` and `check`, and it swallows every error — telemetry must never turn a green lint red. * feat(telemetry): emit per-group rule counts so slowest_rule stays comparable Review catch on #3367: `slowest_rule` is the one positional key in either event. It is `<group>#<index>`, so adding or removing a rule renumbers every later slot in that group and the same string means different rules in two builds. #3366 does exactly that to 34 of 81 surviving slots, and `rule_count` alone says only THAT the ruleset moved, not which groups. `rule_group_counts` carries the per-group sizes alongside it, so a consumer comparing two builds can tell which groups' indices still mean the same thing without anyone having to remember which release dropped rules. `codes`, `code_counts` and `rule_group_ms` are keyed by name and were never affected. Also corrects the rule count in the RULE_GROUPS comment: 86, not ~60, as LINT_RULE_COUNT in the same file computes.
3 participants
What
Removes seven lint rules and narrows an eighth.
Removed:
scene_layer_missing_visibility_kill,unscoped_gsap_selector,caption_transcript_parse_error,composition_self_attribute_selector,timed_element_missing_visibility_hidden,pointer_events_none,google_fonts_import.Narrowed:
system_font_will_aliasnow only fires for distributed / Lambda renders.Why
Each of these either reports a hazard the compiler or runtime already prevents, duplicates another rule's invariant with a weaker detector, or cannot be cleared by its own
fixHint. That last category is the expensive one: an agent iterating against an unfixable error burns a full edit-and-relint cycle every time and never converges.Measured over the 643 shipped registry HTML files, lint output drops from 1740 findings to 507 (-70.9%), including 40 fewer errors. No new codes are introduced.
Per rule:
scene_layer_missing_visibility_killwas a regex heuristic keyed on#sceneNids. Its kill check only accepted the literal stringvisibility: "hidden", so the canonical GSAP hard killtl.set(el, { autoAlpha: 0 })(which setsvisibility: hiddenat runtime) never cleared it. It also matched the0insideopacity: 0.5and treated.from({ opacity: 0 })entrances as exits.gsap_exit_missing_hard_killalready owns this invariant using parsed tween timing and real clip-start boundaries, and accepts every hidden encoding.unscoped_gsap_selectorwarned that a bare.titleselector would leak across compositions when bundled.wrapScopedCompositionScriptalready rewrites string GSAP targets to the composition root for every sub-composition script; the existing testcompositionScoping.test.ts > "executes document and GSAP selectors inside the composition root"pins exactly that scenario. The rule also never fired on a standalone sub-composition file or a<template>sub-comp, only on the multi-root single-file shape thatmultiple_root_compositionsalready rejects.caption_transcript_parse_errorrequired the inlineTRANSCRIPTarray to be strict JSON so Studio could read it, but Studio'sparseTranscriptArrayalready normalizes unquoted keys, single quotes, and trailing commas before parsing. The rule errored on ten shipped caption components whose transcripts Studio parses without complaint.composition_self_attribute_selectorwarned that[data-composition-id="x"] .yleaks to sibling instances.scopeCssToCompositionrewrites that selector to each instance's runtime scope, so it does not. It was also the exact scoping pattern the rest of the toolchain prescribes, and it fired 522 times across the registry.timed_element_missing_visibility_hiddenoverlappedtimed_element_missing_clip_class, which reports the same condition as an error, so on<div>/<img>it only ever added a second line restating the error. Correcting the original wording here: it was not a strict subset. The two rules skip different tags — the deleted one skippedaudio/script/style, the surviving one also skipsvideoandtemplate. So the deleted rule reached two cases nothing now covers:<video data-start>and<template data-start>withoutclass="clip". Both of those were pure false positives.<video id data-start data-duration data-track-index src>with noclass="clip"is the documented canonical clip (packages/core/docs/core.md:117-120,:205-209), and<template data-start>occurs nowhere in the tree. Losing that reach is an improvement, not a gap. (Caught in review by Rames; verified against the docs.)pointer_events_noneflagged a standard decorative-overlay pattern for Studio selection ergonomics. It has no effect on the rendered video and fired on 124 of 211 blocks.google_fonts_importadvised against a path its own message described as working ("the producer resolves Google Fonts during compile/render").system_font_will_aliasis kept but narrowed. On a distributed / Lambda render, system-font capture is off and the fallback is a real defect worth a warning. On a local render the substitution is the renderer working as designed, so there was nothing for an author to act on.How
Rule bodies deleted along with the helpers that became dead (
countClassUsage,isSuspiciousGlobalSelector,getSingleClassSelector,extractArrayLiteral,selectorTargetsCompositionId,escapeRegExp).Three tests used
composition_self_attribute_selectoras a probe for "this style source was collected" (linked CSS,<style>block, nested-template style). They now usescoped_css_missing_wrapperagainst a composition id with no wrapper, which still produces one finding per source, so the coverage is unchanged.The
system_font_will_aliastests now run withdistributed: true, plus a new test pinning that it stays silent on a local render.Test plan
Unit tests added/updated
Manual testing performed
Documentation updated (if applicable) — none of these codes appeared in docs or skills
packages/lint: 514 tests pass.packages/clilintProject.test.ts: 76 tests pass.Before/after run of both linter versions over the same 643 registry HTML files, with
filePathandisSubCompositionset: 1740 -> 507 findings, errors 426 -> 386, warnings 685 -> 121, info 629 -> 0, no new codes.Verified
gsap_exit_missing_hard_killstill errors on a genuine clip-boundary exit with no hard kill, and clears on bothtl.set(..., { autoAlpha: 0 })andtl.set(..., { visibility: "hidden" }).Verified Studio's
extractTranscriptreturns 28 / 46 / 28 words for three registry files thatcaption_transcript_parse_errorrejected.Verified
scopeCssToCompositionrewrites an authored[data-composition-id="scene-a"] .titleto a per-instance runtime scope.