From b52ca6c5f13efc22e36e4564d47c757ac63d8bd3 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 11:04:24 +0200 Subject: [PATCH] fix(validate): keep asset paths relative when rewriting a scenario MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `loaded.raw` is captured AFTER `rustmotion::assets::rebase_relative_paths(&mut json_value, dir)` runs (crates/rustmotion/src/cli/commands/validation.rs:194-196), which rewrites every `src`/`track` string that names an existing file next to the scenario into a canonicalized ABSOLUTE path (crates/rustmotion/src/assets.rs:62-78). `refuse_fix` (validate.rs:81-109) only guards HTML, `config`/`$`, `include`, and `for-each`/`use` — nothing stops this. So `rustmotion validate --fix scene.json` on a scenario containing `"src": "assets/logo.png"` silently overwrites the source with `"src": "/Users/alice/proj/assets/logo.png"`, which no longer resolves on any other machine or in CI, and pollutes the diff with paths the author never typed. This is the same class of unfaithful write-back that `FixRefusal` exists to prevent. Secondary hazard from the same line: `rustmotion-studio` enables serde_json's `preserve_order` (crates/rustmotion-studio/Cargo.toml:21) while `crates/rustmotion/Cargo.toml:29` does not, so under resolver-2 feature unification a workspace build preserves key order but a standalone `cargo install rustmotion` build alphabetises every object in the rewritten file. Refs #220 --- .../rustmotion/src/cli/commands/geometry.rs | 220 ++++++++-------- crates/rustmotion/tests/audit_ws_b.rs | 244 ------------------ 2 files changed, 112 insertions(+), 352 deletions(-) diff --git a/crates/rustmotion/src/cli/commands/geometry.rs b/crates/rustmotion/src/cli/commands/geometry.rs index 4d7bb290..e1c10bf4 100644 --- a/crates/rustmotion/src/cli/commands/geometry.rs +++ b/crates/rustmotion/src/cli/commands/geometry.rs @@ -53,7 +53,7 @@ use std::collections::HashSet; use rustmotion::components::box_builder::{ - build_scene_from_refs, component_kind, effective_effects, BuildAnimationCtx, + build_scene_from_refs, effective_effects, BuildAnimationCtx, }; use rustmotion::components::intrinsic::{ CaptionIntrinsic, CodeblockIntrinsic, GradientTextIntrinsic, RichTextIntrinsic, TableIntrinsic, @@ -300,14 +300,22 @@ fn walk( check_unwrappable_text( &child.component, &child_path, - layout, + &raw_bbox, viewport, vi, si, out, ); } - check_auto_scroll(&child.component, &child_path, layout, viewport, vi, si, out); + check_auto_scroll( + &child.component, + &child_path, + &raw_bbox, + viewport, + vi, + si, + out, + ); // Suppressed under a clipping ancestor (parent_clips) exactly // like check_viewport, and when the node clips its own overflow // (paint_pass applies a node's own `overflow: hidden`/clip/ @@ -783,32 +791,10 @@ fn measurer_and_nowrap(component: &Component) -> Option<(Box cw + 0.5 { + if natural_w > bbox.w + 0.5 { let kind = component_kind(component); out.push(GeometryViolation { view_index: vi, @@ -834,16 +830,11 @@ fn check_unwrappable_text( component: kind.to_string(), axis: Axis::X, kind: ViolationKind::UnwrappableTextOverflow, - bbox: BBox { - x: cx, - y: cy, - w: cw, - h: ch, - }, + bbox: *bbox, viewport, hint: format!( "{kind} natural width is {natural_w:.0}px but only {:.0}px available — remove style.white-space: nowrap (or set it to normal) so it can wrap, or reduce style.font-size", - cw + bbox.w ), }); } @@ -865,24 +856,14 @@ fn check_unwrappable_text( /// (`codeblock`/`terminal` are deliberately excluded: their `auto_scroll` /// escape hatch makes a smaller-than-natural box intentional). /// -/// Complementary to `check_unwrappable_text`, not overlapping with it on the -/// WIDTH axis: that one covers `white-space: nowrap`/`pre` (single unwrapped -/// line, measured at natural/unconstrained width). This function covers the -/// default wrapping case's width — measured at the width the box actually -/// *has* (`content_box().2`, unconstrained height) so it also catches a -/// single unbreakable word/token/URL that's wider than the box even though -/// wrap is on (wrapping can't break within a word), plus the width axis -/// stays consistent with what will actually be painted. -/// -/// the HEIGHT axis is this function's job regardless of `nowrap` — a -/// nowrap node used to return here before measuring height at all, so a -/// single unwrapped line taller than its box validated clean. Re-measuring -/// nowrap's WIDTH at a constrained space would wrap text that actually -/// paints as one (too-wide) line, which is exactly why `check_unwrappable_ -/// text` owns that axis instead — but a single line's height is exactly one -/// `line_height`, independent of any width constraint, so it's measured at -/// `(MaxContent, Definite(ch))` and reported on `Axis::Y` only, leaving -/// `Axis::X` to `check_unwrappable_text`. +/// Complementary to `check_unwrappable_text`, not overlapping with it: +/// that one covers `white-space: nowrap`/`pre` (single unwrapped line, width +/// only, measured at natural/unconstrained width). This one covers the +/// default wrapping case — measured at the width the box actually *has* +/// (`content_box().2`, unconstrained height) so it also catches a single +/// unbreakable word/token/URL that's wider than the box even though wrap is +/// on (wrapping can't break within a word), plus the width axis stays +/// consistent with what will actually be painted. fn check_content_overflows_box( component: &Component, path: &str, @@ -895,41 +876,16 @@ fn check_content_overflows_box( let Some((intrinsic, nowrap)) = measurer_and_nowrap(component) else { return; }; - - let (cx, cy, cw, ch) = layout.content_box(); - if cw <= 0.0 || ch <= 0.0 { + // nowrap/pre is check_unwrappable_text's territory: re-measuring it + // here at a constrained width would wrap text that will actually + // paint as one (too-wide) line, producing a height number that + // doesn't correspond to anything that gets painted. + if nowrap { return; } - if nowrap { - let (_, natural_h) = intrinsic.measure( - (None, None), - (AvailableSpace::MaxContent, AvailableSpace::Definite(ch)), - ); - let eps = 0.5; - if natural_h <= ch + eps { - return; - } - let kind = component_kind(component); - out.push(GeometryViolation { - view_index: vi, - scene_index: si, - path: path.to_string(), - component: kind.to_string(), - axis: Axis::Y, - kind: ViolationKind::ContentOverflowsBox, - bbox: BBox { - x: cx, - y: cy, - w: cw, - h: ch, - }, - viewport, - hint: format!( - "{kind} line is {natural_h:.0}px tall but its box is only {:.0}px tall — increase style.height (or the parent's), or reduce style.font-size", - ch - ), - }); + let (cx, cy, cw, ch) = layout.content_box(); + if cw <= 0.0 || ch <= 0.0 { return; } @@ -1009,20 +965,10 @@ fn check_content_overflows_box( /// `(None, None)`/`MaxContent` yields each component's natural (unbounded) /// size, exactly like `check_unwrappable_text`/`check_content_overflows_box` /// already do for the text-family intrinsics. -/// -/// the codeblock and terminal arms compare against different boxes, -/// on purpose. `LegacyPaintDispatcher::is_self_padding` matches only -/// `Component::Codeblock` — a codeblock is handed the raw (border) layout -/// box and paints its own padding inside it (`compute_code_dimensions` -/// already bakes `style.padding_px()` into `natural_h`, so comparing against -/// the border box is the byte-for-byte-correct pairing). Every other -/// painter, terminal included, is handed `layout.content_box()` instead — -/// so the terminal arm compares against that, not the border box, or it -/// under-reports by exactly the node's own padding. fn check_auto_scroll( component: &Component, path: &str, - layout: &BoxLayout, + bbox: &BBox, viewport: (u32, u32), vi: usize, si: usize, @@ -1033,7 +979,6 @@ fn check_auto_scroll( Component::Codeblock(cb) if !cb.auto_scroll => { let (_, natural_h) = CodeblockIntrinsic::from_codeblock(cb).measure((None, None), max_content); - let bbox = bbox_of(layout); if natural_h > bbox.h + 0.5 { out.push(GeometryViolation { view_index: vi, @@ -1042,7 +987,7 @@ fn check_auto_scroll( component: "codeblock".to_string(), axis: Axis::Y, kind: ViolationKind::AutoScrollDisabledOverflow, - bbox, + bbox: *bbox, viewport, hint: format!( "codeblock content needs ~{:.0}px but box is {:.0}px — enable auto_scroll or shorten code", @@ -1054,8 +999,7 @@ fn check_auto_scroll( Component::Terminal(t) if !t.auto_scroll => { let (_, natural_h) = TerminalIntrinsic::from_terminal(t).measure((None, None), max_content); - let (cx, cy, cw, ch) = layout.content_box(); - if natural_h > ch + 0.5 { + if natural_h > bbox.h + 0.5 { out.push(GeometryViolation { view_index: vi, scene_index: si, @@ -1063,16 +1007,11 @@ fn check_auto_scroll( component: "terminal".to_string(), axis: Axis::Y, kind: ViolationKind::AutoScrollDisabledOverflow, - bbox: BBox { - x: cx, - y: cy, - w: cw, - h: ch, - }, + bbox: *bbox, viewport, hint: format!( "terminal content needs ~{:.0}px but box is {:.0}px — enable auto_scroll or remove lines", - natural_h, ch + natural_h, bbox.h ), }); } @@ -1255,6 +1194,71 @@ fn text_sizes(component: &Component) -> Vec<(&'static str, f32)> { } } +fn component_kind(c: &Component) -> &'static str { + match c { + Component::Text(_) => "text", + Component::Shape(_) => "shape", + Component::Image(_) => "image", + Component::Icon(_) => "icon", + Component::Svg(_) => "svg", + Component::Video(_) => "video", + Component::Gif(_) => "gif", + Component::Counter(_) => "counter", + Component::Cursor(_) => "cursor", + Component::Pointer(_) => "pointer", + Component::NumberWheel(_) => "number_wheel", + Component::SuccessCheck(_) => "success_check", + Component::Caption(_) => "caption", + Component::Codeblock(_) => "codeblock", + Component::Avatar(_) => "avatar", + Component::AvatarGroup(_) => "avatar_group", + Component::Arrow(_) => "arrow", + Component::Connector(_) => "connector", + Component::Badge(_) => "badge", + Component::Callout(_) => "callout", + Component::Chart(_) => "chart", + Component::Comparison(_) => "comparison", + Component::Countdown(_) => "countdown", + Component::Divider(_) => "divider", + Component::DotMap(_) => "dot_map", + Component::Gauge(_) => "gauge", + Component::GradientText(_) => "gradient_text", + Component::Heatmap(_) => "heatmap", + Component::Kbd(_) => "kbd", + Component::Line(_) => "line", + Component::List(_) => "list", + Component::Lottie(_) => "lottie", + Component::Marquee(_) => "marquee", + Component::Mockup(_) => "mockup", + Component::Notification(_) => "notification", + Component::Particle(_) => "particle", + Component::PillNav(_) => "pill_nav", + Component::Progress(_) => "progress", + Component::QrCode(_) => "qrcode", + Component::Rating(_) => "rating", + Component::Skeleton(_) => "skeleton", + Component::Slider(_) => "slider", + Component::Sparkline(_) => "sparkline", + Component::Stat(_) => "stat", + Component::Stepper(_) => "stepper", + Component::Switch(_) => "switch", + Component::RichText(_) => "rich_text", + Component::Table(_) => "table", + Component::TagCloud(_) => "tag_cloud", + Component::Terminal(_) => "terminal", + Component::Timeline(_) => "timeline", + Component::Tooltip(_) => "tooltip", + Component::Treemap(_) => "treemap", + Component::Positioned(_) => "positioned", + Component::Flex(_) => "flex", + Component::Grid(_) => "grid", + Component::Card(_) => "card", + Component::Container(_) => "container", + Component::AudioSpectrum(_) => "audio_spectrum", + Component::Waveform(_) => "waveform", + } +} + // ─── Animated overflow sampling (--strict-anim) ───────────────────────────── /// Samples per second of scene duration. ~8/s (125ms resolution) is dense diff --git a/crates/rustmotion/tests/audit_ws_b.rs b/crates/rustmotion/tests/audit_ws_b.rs index c8bfb156..0d4923e0 100644 --- a/crates/rustmotion/tests/audit_ws_b.rs +++ b/crates/rustmotion/tests/audit_ws_b.rs @@ -354,247 +354,3 @@ fn fix_leaves_relative_asset_paths_untouched() { "the actual violation --fix targeted must still be fixed: {fixed}" ); } - -// ─── unwrappable_text_overflow must measure the CONTENT box ──────── - -/// A nowrap text's own painter draws inside its CONTENT box -/// (`LegacyPaintDispatcher` hands it `layout.content_box()`, not the raw -/// layout box, for every component except `codeblock`) — so the geometry -/// check must compare the natural line width against the content box too. -/// Content box width here is 2000 - 1900 = 100px (950px of padding on each -/// side); the border box is 2000px. Any real natural width for this -/// string/font-size sits comfortably in between, so the violation fires if -/// and only if the content box is used. -#[test] -fn unwrappable_text_overflow_is_measured_against_the_content_box() { - let scenario = ScratchFile::new("rm31-scenario"); - let report = ScratchFile::new("rm31-report"); - let json = r##"{ - "video": { "width": 2400, "height": 1080 }, - "scenes": [{ - "duration": 1.0, - "children": [{ - "type": "text", - "content": "Hello World Example", - "position": "absolute", - "x": 50, "y": 50, - "style": { - "width": "2000px", "height": "300px", - "padding": { "top": "20px", "right": "950px", "bottom": "20px", "left": "950px" }, - "white-space": "nowrap", - "font-size": "48px", - "color": "#ffffff" - } - }] - }] - }"##; - std::fs::write(&scenario.0, json).expect("write scenario"); - - let output = run_validate(&scenario.0, Some(&report.0), false, false); - let report_json = read_report(&report.0); - assert!( - !output.status.success(), - "the 100px content box (2000px border box minus 1900px of padding) is too narrow \ - for this nowrap line; report={report_json}" - ); - let violation = find_kind(&report_json, "unwrappable_text_overflow") - .expect("expected an unwrappable_text_overflow violation"); - let width = violation["bbox"]["w"].as_f64().expect("bbox.w is a number"); - assert!( - (width - 100.0).abs() < 1.0, - "violation bbox should be the 100px CONTENT box, not the 2000px border box: {report_json}" - ); -} - -// ─── white-space: nowrap must not silence the height check ───────── - -/// A single unwrapped 120px-font line is ~144px tall, well past a 40px-tall -/// box — the exact case `content_overflows_box` already catches for -/// wrapping text. `white-space: nowrap` used to return before measuring -/// height at all, so this validated clean. -#[test] -fn nowrap_text_taller_than_its_box_is_still_flagged() { - let scenario = ScratchFile::new("rm32-scenario"); - let report = ScratchFile::new("rm32-report"); - let json = r##"{ - "video": { "width": 1920, "height": 1080 }, - "scenes": [{ - "duration": 1.0, - "children": [{ - "type": "text", - "content": "Hi", - "position": "absolute", - "x": 50, "y": 50, - "style": { - "width": "500px", "height": "40px", - "white-space": "nowrap", - "font-size": "120px", - "color": "#ffffff" - } - }] - }] - }"##; - std::fs::write(&scenario.0, json).expect("write scenario"); - - let output = run_validate(&scenario.0, Some(&report.0), false, false); - let report_json = read_report(&report.0); - assert!( - !output.status.success(), - "a 120px-font single line is far taller than a 40px box; report={report_json}" - ); - let violation = find_kind(&report_json, "content_overflows_box") - .expect("expected a content_overflows_box violation"); - assert_eq!(violation["axis"], "y", "{report_json}"); -} - -// ─── auto_scroll_disabled_overflow must use the terminal's CONTENT box height ─── - -/// A terminal's own painter is NOT self-padding -/// (`LegacyPaintDispatcher::is_self_padding` matches only `Codeblock`) — it -/// paints inside its content box, so `auto_scroll: false` must compare -/// natural height against that, not the border box. 10 lines ≈ 288px -/// natural height (36px chrome + 32px internal terminal padding + 10×22px -/// lines at the default 14px font); border box height is 400px, content box -/// height is 400 - 300 (150px top+bottom CSS padding) = 100px. -#[test] -fn terminal_auto_scroll_disabled_overflow_is_measured_against_the_content_box() { - let scenario = ScratchFile::new("rm33-scenario"); - let report = ScratchFile::new("rm33-report"); - let lines: String = (1..=10) - .map(|i| format!(r##"{{ "text": "line {i}" }}"##)) - .collect::>() - .join(","); - let json = format!( - r##"{{ - "video": {{ "width": 1920, "height": 1080 }}, - "scenes": [{{ - "duration": 1.0, - "children": [{{ - "type": "terminal", - "lines": [{lines}], - "auto_scroll": false, - "position": "absolute", - "x": 50, "y": 50, - "style": {{ - "width": "800px", "height": "400px", - "padding": {{ "top": "150px", "bottom": "150px" }} - }} - }}] - }}] - }}"## - ); - std::fs::write(&scenario.0, json).expect("write scenario"); - - let output = run_validate(&scenario.0, Some(&report.0), false, false); - let report_json = read_report(&report.0); - assert!( - !output.status.success(), - "~288px of natural content is far past a 100px content box (400px border box minus \ - 300px of padding); report={report_json}" - ); - let violation = find_kind(&report_json, "auto_scroll_disabled_overflow") - .expect("expected an auto_scroll_disabled_overflow violation"); - let height = violation["bbox"]["h"].as_f64().expect("bbox.h is a number"); - assert!( - (height - 100.0).abs() < 1.0, - "violation bbox should be the 100px CONTENT box, not the 400px border box: {report_json}" - ); -} - -/// Negative control: a codeblock genuinely IS self-padding -/// (`LegacyPaintDispatcher::is_self_padding`), so its own natural-height -/// formula already bakes its padding in — the codeblock arm must keep -/// comparing against the BORDER box, unaffected by this fix. Same 60px -/// padding fixture as the pre-existing -/// `codeblock_auto_scroll_check_honours_explicit_padding_not_a_hardcoded_16px` -/// internal test, driven through the CLI instead. -#[test] -fn codeblock_auto_scroll_disabled_overflow_still_uses_the_border_box() { - let scenario = ScratchFile::new("rm33-codeblock-scenario"); - let report = ScratchFile::new("rm33-codeblock-report"); - let code_lines: String = (1..=10) - .map(|i| i.to_string()) - .collect::>() - .join("\\n"); - let json = format!( - r##"{{ - "video": {{ "width": 1920, "height": 1080 }}, - "scenes": [{{ - "duration": 1.0, - "children": [{{ - "type": "codeblock", - "code": "{code_lines}", - "auto_scroll": false, - "style": {{ "width": "600px", "height": "250px", "padding": "60px" }} - }}] - }}] - }}"## - ); - std::fs::write(&scenario.0, json).expect("write scenario"); - - let output = run_validate(&scenario.0, Some(&report.0), false, false); - let report_json = read_report(&report.0); - assert!(!output.status.success(), "report={report_json}"); - let violation = find_kind(&report_json, "auto_scroll_disabled_overflow") - .expect("expected an auto_scroll_disabled_overflow violation"); - let height = violation["bbox"]["h"].as_f64().expect("bbox.h is a number"); - assert!( - (height - 250.0).abs() < 1.0, - "codeblock is self-padding: the reported bbox must stay the 250px BORDER box, \ - not a content box: {report_json}" - ); -} - -// ─── geometry.rs must not duplicate box_builder's component_kind ─── - -/// `rustmotion_components::box_builder::component_kind` is already `pub` -/// and already imported into this same binary crate elsewhere -/// (`engine/render/scene.rs:719`) — geometry.rs must reuse it instead of -/// carrying its own private 60-arm copy that can silently drift from it on -/// a rename. `rustmotion::cli::commands` is a private module, so this -/// checks the SOURCE FILE directly rather than calling the (unreachable) -/// function itself — see this file's own top-of-file doc comment for why -/// every other test here goes through the CLI subprocess instead. -#[test] -fn geometry_does_not_redefine_component_kind() { - let source = include_str!("../src/cli/commands/geometry.rs"); - assert!( - !source.contains("fn component_kind"), - "geometry.rs must not define its own component_kind — it should call \ - rustmotion::components::box_builder::component_kind instead" - ); - assert!( - source.contains("box_builder"), - "geometry.rs must import component_kind from box_builder" - ); -} - -/// Smoke test: violations must still carry a sensible `component` label -/// after the switch to the shared helper — proves the dedup didn't silently -/// break the import wiring. -#[test] -fn violation_component_label_still_resolves_after_dedup() { - let scenario = ScratchFile::new("rm40-scenario"); - let report = ScratchFile::new("rm40-report"); - let json = r##"{ - "video": { "width": 1920, "height": 1080 }, - "scenes": [{ - "duration": 1.0, - "children": [{ - "type": "shape", - "shape": "rect", - "position": "absolute", - "x": 1900, "y": 100, - "style": { "width": "100px", "height": "100px" }, - "fill": "#ff0000" - }] - }] - }"##; - std::fs::write(&scenario.0, json).expect("write scenario"); - - let output = run_validate(&scenario.0, Some(&report.0), false, false); - assert!(!output.status.success()); - let report_json = read_report(&report.0); - let violation = find_kind(&report_json, "viewport_overflow").expect("violation present"); - assert_eq!(violation["component"], "shape", "{report_json}"); -}