From 1d56385c5881937e1159c40607e3a4942dcfb615 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 10:59:13 +0200 Subject: [PATCH] refactor(geometry): reuse the canonical component_kind helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `rustmotion_components::box_builder::component_kind` (box_builder.rs:2090-2154) is public and already imported elsewhere in the same crate (`crates/rustmotion/src/engine/render/scene.rs:744`). geometry.rs re-implements it as a private 60-arm copy. Both must be edited for every new component, and both already carry the same two label bugs: `QrCode(_) => "qrcode"` while the serde tag is `qr_code`, and `Container(_) => "container"` while the serde tag is `div`. Those strings are user-facing — they are the `component:` field of every `GeometryViolation` and drive the hint branches at geometry.rs:686 and :1591 — so a violation on a `qr_code` reports a type name that does not exist in the schema. Exhaustiveness protects against a *missing* arm but not against the two copies drifting on a rename, which is exactly how the labels got out of sync with serde in the first place. Refs #220 --- .../rustmotion/src/cli/commands/geometry.rs | 67 +------------------ crates/rustmotion/tests/audit_ws_b.rs | 54 +++++++++++++++ 2 files changed, 55 insertions(+), 66 deletions(-) diff --git a/crates/rustmotion/src/cli/commands/geometry.rs b/crates/rustmotion/src/cli/commands/geometry.rs index 22796887..4d7bb290 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, effective_effects, BuildAnimationCtx, + build_scene_from_refs, component_kind, effective_effects, BuildAnimationCtx, }; use rustmotion::components::intrinsic::{ CaptionIntrinsic, CodeblockIntrinsic, GradientTextIntrinsic, RichTextIntrinsic, TableIntrinsic, @@ -1255,71 +1255,6 @@ 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 0b13a3e5..c8bfb156 100644 --- a/crates/rustmotion/tests/audit_ws_b.rs +++ b/crates/rustmotion/tests/audit_ws_b.rs @@ -544,3 +544,57 @@ fn codeblock_auto_scroll_disabled_overflow_still_uses_the_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}"); +}