fix(websem): close the load-active SMIL hole; arm the locks; widen the refusal register - #62
Merged
Conversation
…red state SMIL defaults begin to offset 0s, so an animation element is active the moment Chromium loads the document: it overrides its target's authored state before any sample is requested. The compiler treated every one as a sampling-only blocker — strict compiled Base and refused only at sample time, best-effort declared SamplesAsBase, and the host filtered that declaration out of Base renders. A Base render of <set attributeName="fill" to="red"/> painted the authored blue with exit 0 and zero declarations in both admissions while Chromium paints red — the silent wrong pixel recorded open in the D-N register at the paths rung. The inventory now classifies its findings by what they distort. Dynamic surfaces that leave Base honest (event handlers, CSS animation carriers, <style>) stay sampling blockers: Base renders, sampling refuses under strict and resolves to Base, declared, under best-effort. A beyond-inventory animation element is an authored-state override against its SMIL default target: strict refuses at construction like any beyond-slice construct, and best-effort recompiles with the target left out, declared Skipped at the target's stable path in document order — a declared hole in every view, never a wrong pixel in any. One that cannot be attributed to a skippable element — an href retarget, a root-<svg> target — refuses in both admissions, exactly as <script> does. The host's Base-time SamplesAsBase filter is now correct as it stands: everything it hides really is sampling-only. The three laws that pinned the defective behavior move with it, and the groups/shapes contract laws gain the nested and wrong-shape cases as override laws (the circle case is a named over-refusal: x does not apply to a circle, but the inventory owns no applicability model and a declared hole is never a wrong pixel).
The 20-row corpus named path-grammar, viewport, and stroke refusals and not one SMIL construct, so the enumeration gate exerted no pressure on the exact class the recorded hole lived in. Three rows close that: - svg-smil-set-load-active — a <set> on a consumed attribute of an admitted rect; declared by best-effort at the target's stable path, refused at construction under strict. - svg-smil-animate-transform — an <animateTransform> on a <g>; the override targets the container, so the subtree is the declared hole. - svg-smil-retarget-href — a <set href> retarget; not attributable to one skippable element without id resolution, so document-level: both admissions refuse, exactly as <script> does.
The register's one recorded silent wrong pixel — a SMIL animation on a consumed attribute, active at load in Chromium while a Base render painted the authored state undeclared — is closed by the websem classification fix. The addendum records what the measurement moved: the hole was wider than recorded (the host's Base-time filter also swallowed the library's declaration, so --strict exited 0 too), the fix departs from the sketched keep-authored-and-annotate remedy because a declared wrong pixel is not a category the first law recognizes, and the admitted rect-x animation keeps its corpus-pinned static-projection Base semantics — that divergence is documented and gated for the one admitted element, no longer silent for every other. The statement of record and the compiler README carry the same split: dynamic surfaces that leave Base honest still sample as Base; load-active animation elements skip their targets, declared, or refuse document-level where unattributable.
The seam job armed the legacy and donor locks (grida, htmlcss) and the host's (n0_cli), but the three locks guarding the engine-of-record path itself — websem may not parse, paint, or do I/O; rframe may not name a backend; n0 may not depend on the semantic front-end — reached CI only through test-crates' broad workspace run, on a different trigger and without --locked. A PR scoped to engine paths now runs all three under the same gate that the rest of the seam graph answers to.
'Taffy is the layout engine' read as a claim about the whole tree; it is true of htmlcss, grida, and the n0-model authored tier, and false of the Web-first render path, which runs no layout at all — websem, rframe, n0, n0_cli, and csscascade carry no taffy dependency, and n0-model's resolve tier is never called on that path. A zero-context session orienting from this file should not infer a layout capability the engine of record does not have.
The charter defers position to #43 by design, but two of its route sections read as open work that has since landed or moved. The first-three-PRs list is discharged (#46, #59, #60) with its one carried obligation — required-check registration on the main ruleset — named instead of implied. The phase table gains the reading note the amendment already mandates: D-N routed SVG through the Web semantic path, so Phase 3's packer-route reading is superseded while its import-to-document lane stays deferred, not dead.
The corpus documented 23 refusals and the slice refuses far more; every construct the next rungs graduate — and every high-frequency construct real exports emit — now has a row the enumeration gate holds. Twenty-two additions: the points shapes, text, use/defs, image, nested svg, the gradient and pattern paint servers (declared at both doors: the element and the url() reference), the clip/mask/filter resource scopes, the anchor/switch/foreignObject containers, the translucency ladder (element opacity, fill-opacity, translucent sRGB), display and visibility, rounded rects, shape-geometry percentages, and the CSS transform property. Each departs by name in both admissions today, so the rung that admits one must move its row — a silent regression in either direction now fails the gate.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
13 tasks
WalkthroughThe SVG animation inventory now distinguishes sampling blockers from load-active authored-state overrides. Strict admission refuses unsupported overrides at construction, while best-effort admission skips attributable targets consistently across Base and sampled frames, with updated tests, corpus entries, documentation, and architecture gates. ChangesLoad-active SVG animation handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SVGSource
participant AnimationInventory
participant Compiler
participant Frame
SVGSource->>AnimationInventory: inspect load-active animation elements
AnimationInventory-->>SVGSource: return blockers and authored overrides
SVGSource->>Compiler: construct with override skip map
Compiler->>Frame: omit attributable override targets
SVGSource->>Compiler: recompile requested sample with same skips
Compiler-->>Frame: return frame sharing the declared skips
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/websem/src/svg_animation.rs (1)
428-458: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider identifying the root by node id rather than path string.
targets_root = parent_path == "svg"makes a document-level refusal decision from a formatted path. It is correct given the walk always roots at"svg"and nested paths contain/, but comparingparent.node_id()against the inspected root would make the classification independent of path formatting.Also note that two animation elements under the same parent yield two
AuthoredOverrides with the sametarget;svg.rsfolds them intoHashMap<NodeId, String>, so only one reason reaches the declaration (the target is still skipped, so no wrong pixel).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/websem/src/svg_animation.rs` around lines 428 - 458, Replace the path-string comparison in the targets_root calculation with a node-id comparison against the inspected document root, using the root identifier already available in the surrounding traversal. Keep the existing carries_href handling and document-level override behavior unchanged, including target-based folding of multiple AuthoredOverride entries.crates/websem/src/svg.rs (1)
568-635: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueOptional: the override path compiles the subtree twice.
Best-effort with overrides discards the first compilation entirely and recompiles with the skip map. Correct and construction-only, but if source size ever matters, the walk could take the skip map lazily (or the inventory could run before the compile using a cheaper materialized-shape probe) to avoid the second pass. Fine to defer.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/websem/src/svg.rs` around lines 568 - 635, The best-effort override path in the SVG compilation flow performs and discards an initial compile before recompiling with override_skips. Refactor the flow around compile_svg_element and AnimationInventory::inspect so the override inventory is determined before the full compilation, or pass the skip map lazily during the existing walk, while preserving strict errors, document-level rejection, degradation collection, and the final compilation results.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/websem/README.md`:
- Around line 47-52: Trim the added degradation-semantics paragraph in the
websem README so it no longer restates the beyond-inventory animation-element
behavior described in crates/n0_cli/README.md. Replace those details with a
concise pointer to that README as the single statement of record, while
preserving only the compiler-map context and frame-identical law as appropriate.
In `@crates/websem/tests/best_effort.rs`:
- Around line 257-260: Update the animation-degradation assertions in the
strict/best-effort test flow around LOAD_ACTIVE_SET to match the actual
best-effort skip reason, which does not include “document load.” Keep the strict
assertion requiring the construct and “document load,” but adjust the
best-effort assertion to validate the authored-state override wording used by
the implementation.
---
Nitpick comments:
In `@crates/websem/src/svg_animation.rs`:
- Around line 428-458: Replace the path-string comparison in the targets_root
calculation with a node-id comparison against the inspected document root, using
the root identifier already available in the surrounding traversal. Keep the
existing carries_href handling and document-level override behavior unchanged,
including target-based folding of multiple AuthoredOverride entries.
In `@crates/websem/src/svg.rs`:
- Around line 568-635: The best-effort override path in the SVG compilation flow
performs and discards an initial compile before recompiling with override_skips.
Refactor the flow around compile_svg_element and AnimationInventory::inspect so
the override inventory is determined before the full compilation, or pass the
skip map lazily during the existing walk, while preserving strict errors,
document-level rejection, degradation collection, and the final compilation
results.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 84a90675-ee09-4aa6-91ef-2ebbd88cc430
⛔ Files ignored due to path filters (25)
fixtures/web-first/unsupported/svg-anchor.svgis excluded by!**/*.svgfixtures/web-first/unsupported/svg-clip-path.svgis excluded by!**/*.svgfixtures/web-first/unsupported/svg-css-transform-property.svgis excluded by!**/*.svgfixtures/web-first/unsupported/svg-display-none.svgis excluded by!**/*.svgfixtures/web-first/unsupported/svg-element-opacity.svgis excluded by!**/*.svgfixtures/web-first/unsupported/svg-fill-opacity.svgis excluded by!**/*.svgfixtures/web-first/unsupported/svg-filter.svgis excluded by!**/*.svgfixtures/web-first/unsupported/svg-foreign-object.svgis excluded by!**/*.svgfixtures/web-first/unsupported/svg-gradient-paint-server.svgis excluded by!**/*.svgfixtures/web-first/unsupported/svg-image.svgis excluded by!**/*.svgfixtures/web-first/unsupported/svg-mask.svgis excluded by!**/*.svgfixtures/web-first/unsupported/svg-nested-svg.svgis excluded by!**/*.svgfixtures/web-first/unsupported/svg-pattern-paint-server.svgis excluded by!**/*.svgfixtures/web-first/unsupported/svg-polygon.svgis excluded by!**/*.svgfixtures/web-first/unsupported/svg-polyline.svgis excluded by!**/*.svgfixtures/web-first/unsupported/svg-rect-percentage-geometry.svgis excluded by!**/*.svgfixtures/web-first/unsupported/svg-rect-rounded.svgis excluded by!**/*.svgfixtures/web-first/unsupported/svg-smil-animate-transform.svgis excluded by!**/*.svgfixtures/web-first/unsupported/svg-smil-retarget-href.svgis excluded by!**/*.svgfixtures/web-first/unsupported/svg-smil-set-load-active.svgis excluded by!**/*.svgfixtures/web-first/unsupported/svg-switch.svgis excluded by!**/*.svgfixtures/web-first/unsupported/svg-text.svgis excluded by!**/*.svgfixtures/web-first/unsupported/svg-translucent-fill.svgis excluded by!**/*.svgfixtures/web-first/unsupported/svg-use.svgis excluded by!**/*.svgfixtures/web-first/unsupported/svg-visibility-hidden.svgis excluded by!**/*.svg
📒 Files selected for processing (14)
.github/workflows/consolidation-gates.ymlAGENTS.mdcrates/n0_cli/README.mdcrates/websem/README.mdcrates/websem/src/svg.rscrates/websem/src/svg_animation.rscrates/websem/tests/best_effort.rscrates/websem/tests/groups_contract.rscrates/websem/tests/shapes_contract.rscrates/websem/tests/svg_animation_x.rscrates/websem/tests/unsupported_corpus.rsdocs/wg/consolidation/charter.mddocs/wg/consolidation/svg-engine-of-record.mdfixtures/web-first/unsupported/README.md
Comment on lines
+47
to
+52
| skipped by name at a stable path, a blocked dynamic surface that leaves Base | ||
| honest resolves every sample to Base, and a beyond-inventory animation element | ||
| — active at document load, so its target's authored state never honestly | ||
| renders — skips its target in every view, declared at the target's path. | ||
| Where nothing degrades the two are frame-identical, and a law checks that | ||
| over the whole corpus. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Restates n0_cli/README.md's degradation semantics instead of linking to it.
This paragraph now nearly duplicates crates/n0_cli/README.md's wording on beyond-inventory animation-element skip/declare behavior (target's-authored-state-never-honestly-renders / skips-its-target-in-every-view / declared-at-the-target's-path). This file's own earlier text (unchanged, lines 39-40) already commits to crates/n0_cli/README.md being "the single statement of record for that slice and what it refuses" and this table being "the compiler's map, not a second copy of it" — the new detail goes past a map into a restatement that now needs to be kept in sync across two files.
✏️ Proposed trim back to a pointer
**Strict** refuses on the first construct outside the slice — the harness that
names the edge. **Best-effort**, the product default, compiles what it admits
and declares everything else as a named degradation: a subtree construct is
-skipped by name at a stable path, a blocked dynamic surface that leaves Base
-honest resolves every sample to Base, and a beyond-inventory animation element
-— active at document load, so its target's authored state never honestly
-renders — skips its target in every view, declared at the target's path.
+skipped by name at a stable path; see `crates/n0_cli/README.md` for the exact
+degradation shapes (dynamic-surface `SamplesAsBase`, beyond-inventory
+animation-element target skips, and the unattributable-retarget refusal).
Where nothing degrades the two are frame-identical, and a law checks that
over the whole corpus.Based on coding guidelines: "The admitted n0 slice must be documented only in crates/n0_cli/README.md; do not restate it elsewhere, and link to that statement of record."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| skipped by name at a stable path, a blocked dynamic surface that leaves Base | |
| honest resolves every sample to Base, and a beyond-inventory animation element | |
| — active at document load, so its target's authored state never honestly | |
| renders — skips its target in every view, declared at the target's path. | |
| Where nothing degrades the two are frame-identical, and a law checks that | |
| over the whole corpus. | |
| skipped by name at a stable path; see `crates/n0_cli/README.md` for the exact | |
| degradation shapes (dynamic-surface `SamplesAsBase`, beyond-inventory | |
| animation-element target skips, and the unattributable-retarget refusal). | |
| Where nothing degrades the two are frame-identical, and a law checks that | |
| over the whole corpus. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/websem/README.md` around lines 47 - 52, Trim the added
degradation-semantics paragraph in the websem README so it no longer restates
the beyond-inventory animation-element behavior described in
crates/n0_cli/README.md. Replace those details with a concise pointer to that
README as the single statement of record, while preserving only the compiler-map
context and frame-identical law as appropriate.
Source: Coding guidelines
Comment on lines
+257
to
+260
| assert!( | ||
| strict.to_string().contains(named) && strict.to_string().contains("document load"), | ||
| "{label}: the refusal names the construct and the load-time law; got {strict}" | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the literal "document load" substring appears in the AnimationError/Degradation formatting.
rg -n "document load" crates/websem/src/svg_animation.rs crates/websem/src/svg.rsRepository: gridaco/nothing
Length of output: 387
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files 'crates/websem/src/svg.rs' 'crates/websem/src/svg_animation.rs' 'crates/websem/tests/best_effort.rs'
echo
echo "== relevant svg.rs matches with context =="
sed -n '560,835p' crates/websem/src/svg.rs | cat -n | sed -n '1,280p'
echo
echo "== degradation/animation error text =="
rg -n "CompileError::UnsupportedAnimation|AnimationError|degradations|reason\\(" crates/websem/src/svg.rs crates/websem/src/svg_animation.rs crates/websem/tests/best_effort.rsRepository: gridaco/nothing
Length of output: 25937
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== best_effort.rs animation skip test context =="
sed -n '230,290p' crates/websem/tests/best_effort.rs | cat -n
echo
echo "== svg_animation.rs error text context =="
sed -n '1,65p' crates/websem/src/svg_animation.rs | cat -n
sed -n '115,180p' crates/websem/src/svg_animation.rs | cat -n
sed -n '210,320p' crates/websem/src/svg_animation.rs | cat -n
sed -n '408,455p' crates/websem/src/svg_animation.rs | cat -nRepository: gridaco/nothing
Length of output: 16501
Fix the animation-degradation assertions around LOAD_ACTIVE_SET
Strict errors include "document load", but best-effort skips for <set> do not: the declared reason is "its authored state is overridden at document load by ..." on line 612, while "document load" only appears earlier in comments/doc text. This makes the tests fail on cases where LOG_ACTIVE_SET is used.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/websem/tests/best_effort.rs` around lines 257 - 260, Update the
animation-degradation assertions in the strict/best-effort test flow around
LOAD_ACTIVE_SET to match the actual best-effort skip reason, which does not
include “document load.” Keep the strict assertion requiring the construct and
“document load,” but adjust the best-effort assertion to validate the
authored-state override wording used by the implementation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this is
The minimal-valid-setup pass before the per-attribute evolution loop: the one live law violation closed, the refusal register widened to every class the loop will graduate, the n0-path locks armed in CI, and the orientation docs corrected to match the tree.
The law repair
A beyond-inventory SMIL animation element (
<set>,animateTransform, a second or beyond-slice<animate>) is active at document load in Chromium — SMIL defaultsbeginto offset0s— so a Base render of its target's authored state is a wrong pixel, not a sampling gap. Before this fix,<set attributeName="fill" to="red"/>rendered the authored fill at exit 0 with zero declarations in both admissions (--strictincluded: the compile had no strict arm for blockers, and the host's Base-time filter swallowed the library's declaration). The D-N register recorded the hole open at the paths rung; the refusal corpus had no SMIL row to notice.The fix is a classification, not a patch over the filter:
<style>, the inline-HTML entry) — unchanged: Base honest, samples-as-Base declared, strict refuses the sample request. Everything the host's Base filter hides is now genuinely Base-inert.hrefretargets and root-<svg>targets are unattributable and refuse in both admissions, like<script>.The four laws that pinned the defective behavior moved with it; a dated addendum closes the register entry, including the deliberate departure from the sketched keep-authored-and-annotate remedy (a declared wrong pixel is not a category the first law recognizes; attributable constructs skip, per the attribute-patrol precedent).
The register
The refusal corpus grows 20 → 45 rows: three SMIL rows gating the closure, then every construct class the next rungs graduate and every high-frequency construct real exports emit — points shapes, text, use/defs, image, nested svg, gradient/pattern paint servers (declared at both doors), clip/mask/filter, anchor/switch/foreignObject, the translucency ladder, display/visibility, rounded rects, shape-geometry percentages, the CSS transform property. Each departs by name in both admissions today, so a rung that admits one must move its row, and a silent regression in either direction fails the enumeration gate.
Arming and orientation
websem/rframe/n0architecture locks now run in the consolidation seam job under--locked(previously reachable only via the broad workspace run on a different trigger).AGENTS.md: the Taffy claim is scoped to the engines that have layout; the Web-first render path runs none.charter.md: first-three-PRs marked discharged (ci(consolidation): arm permanent migration gates #46, refactor(cg): extract shared vocabulary crate #59, feat(consolidation): add sealed scoreboard v0 #60) with the carried required-check obligation named; the phase table gains the amendment reading note (D-N superseded Phase 3's render route; its import-to-document lane is deferred, not dead).Gates
rframeis untouched.