Skip to content

Commit 9918e65

Browse files
committed
fix(components): stop five painters aborting on schema-valid input (#144)
Each of these renders a scenario that `rustmotion validate` accepts, and each one killed the encode mid-frame. A painter that panics takes the whole render with it, so the guard belongs in the painter rather than upstream. - shape: skia asserts `pos.len() == colors.len()` inside the gradient shader, so a `stops` list of a different length than `colors` aborted the process. Drop stops we cannot honour and let skia space the colours. - table: `"row_colors": []` deserializes to Some(vec![]), not None, so the default palette was never substituted and the modulo guard still indexed an empty slice. - tag_cloud: same shape — `palette()` handed back the caller's empty vec and the painter took `index % len()` on it. - dot_map: `dot_spacing: 0` makes the division +inf, and `inf as u32` saturates to u32::MAX, scheduling ~1.8e19 iterations. The geometry pass rejects 0.01 but not 0, so the floor has to live in the painter. Also caps the grid per axis so a large box cannot schedule unbounded work. - codeblock diff: `col`, `delete` and `insert` all counted bytes while the reveal interpolates a fraction of that total, so mid-animation offsets landed inside a multi-byte glyph and `replace_range` aborted. Switch the accounting to characters and convert to byte offsets only when slicing. This also fixes the animation itself: a CJK glyph used to take three reveal steps instead of one. The new integration test drives all five through the real pipeline — serde, box_builder, run_layout, paint_tree — so a fix that guarded the painter but left the component undeserialisable would still fail. dot_map paints on a worker with a deadline, because a runaway loop would otherwise wedge CI instead of reporting.
1 parent 7136239 commit 9918e65

6 files changed

Lines changed: 255 additions & 25 deletions

File tree

‎crates/rustmotion-components/src/codeblock/diff.rs‎

Lines changed: 34 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -444,7 +444,7 @@ pub(super) fn compute_word_diff(old_line: &str, new_line: &str) -> Vec<FragmentE
444444
});
445445
pending_delete.clear();
446446
}
447-
col += change.value().len();
447+
col += change.value().chars().count();
448448
}
449449
ChangeTag::Delete => {
450450
pending_delete.push_str(change.value());
@@ -456,7 +456,7 @@ pub(super) fn compute_word_diff(old_line: &str, new_line: &str) -> Vec<FragmentE
456456
insert: change.value().to_string(),
457457
});
458458
pending_delete.clear();
459-
col += change.value().len();
459+
col += change.value().chars().count();
460460
}
461461
}
462462
}
@@ -474,6 +474,19 @@ pub(super) fn compute_word_diff(old_line: &str, new_line: &str) -> Vec<FragmentE
474474

475475
// ─── Cursor-animated line editing ────────────────────────────────────────────
476476

477+
/// Byte offset of the `n`-th character, saturating at the end of the string.
478+
///
479+
/// Every column in a `FragmentEdit` is a character count, because the reveal
480+
/// interpolates a *fraction* of the total edit length: counting bytes makes the
481+
/// animation land part-way through a multi-byte glyph, which both slices at an
482+
/// invalid boundary and spends three frames revealing one CJK character.
483+
fn byte_at(s: &str, char_idx: usize) -> usize {
484+
s.char_indices()
485+
.nth(char_idx)
486+
.map(|(i, _)| i)
487+
.unwrap_or(s.len())
488+
}
489+
477490
pub(super) fn draw_cursor_edited_line(
478491
canvas: &Canvas,
479492
font: &Font,
@@ -498,7 +511,10 @@ pub(super) fn draw_cursor_edited_line(
498511
return;
499512
}
500513

501-
let total_work: usize = edits.iter().map(|e| e.delete.len() + e.insert.len()).sum();
514+
let total_work: usize = edits
515+
.iter()
516+
.map(|e| e.delete.chars().count() + e.insert.chars().count())
517+
.sum();
502518
if total_work == 0 {
503519
let highlighted = highlight_code(new_line, language, theme);
504520
if let Some(line) = highlighted.first() {
@@ -515,32 +531,33 @@ pub(super) fn draw_cursor_edited_line(
515531

516532
for edit in edits {
517533
let adjusted_col = (edit.col as i64 + offset_adjust).max(0) as usize;
518-
let delete_len = edit.delete.len();
519-
let insert_len = edit.insert.len();
534+
let delete_len = edit.delete.chars().count();
535+
let insert_len = edit.insert.chars().count();
520536
let edit_work = delete_len + insert_len;
521537

522538
if work_done + edit_work <= chars_progress {
523-
let end = (adjusted_col + delete_len).min(current_line.len());
524-
let start = adjusted_col.min(current_line.len());
525-
current_line.replace_range(start..end, &edit.insert);
539+
let start = byte_at(&current_line, adjusted_col);
540+
let end = byte_at(&current_line, adjusted_col + delete_len);
541+
current_line.replace_range(start..end.max(start), &edit.insert);
526542
offset_adjust += insert_len as i64 - delete_len as i64;
527543
work_done += edit_work;
528544
} else {
529545
let remaining_progress = chars_progress - work_done;
530546
if remaining_progress < delete_len {
531547
let chars_deleted = remaining_progress;
532-
let del_start = (adjusted_col + delete_len - chars_deleted).min(current_line.len());
533-
let del_end = (adjusted_col + delete_len).min(current_line.len());
548+
let first_deleted = adjusted_col + delete_len - chars_deleted;
549+
let del_start = byte_at(&current_line, first_deleted);
550+
let del_end = byte_at(&current_line, adjusted_col + delete_len);
534551
if del_start < del_end {
535552
current_line.replace_range(del_start..del_end, "");
536553
}
537-
cursor_col = Some(del_start.min(current_line.len()));
554+
cursor_col = Some(first_deleted);
538555
} else {
539556
let chars_inserted = remaining_progress - delete_len;
540-
let start = adjusted_col.min(current_line.len());
541-
let end = (adjusted_col + delete_len).min(current_line.len());
542-
let partial_insert = &edit.insert[..chars_inserted.min(edit.insert.len())];
543-
current_line.replace_range(start..end, partial_insert);
557+
let start = byte_at(&current_line, adjusted_col);
558+
let end = byte_at(&current_line, adjusted_col + delete_len);
559+
let partial_insert = &edit.insert[..byte_at(&edit.insert, chars_inserted)];
560+
current_line.replace_range(start..end.max(start), partial_insert);
544561
cursor_col = Some(adjusted_col + chars_inserted);
545562
}
546563
break;
@@ -562,7 +579,8 @@ pub(super) fn draw_cursor_edited_line(
562579
};
563580

564581
if should_show {
565-
let prefix = &current_line[..col.min(current_line.len())];
582+
// `col` counts characters, like every other column here.
583+
let prefix = &current_line[..byte_at(&current_line, col)];
566584
let (prefix_width, _) = font.measure_str(prefix, None);
567585
let cursor_x = x + prefix_width;
568586
let mut cursor_paint = paint_from_hex(cursor_color);

‎crates/rustmotion-components/src/dot_map.rs‎

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -154,12 +154,23 @@ impl DotMap {
154154
world_paint.set_style(PaintStyle::Fill);
155155
world_paint.set_anti_alias(true);
156156

157-
let spacing = self.dot_spacing;
157+
// A spacing at or below zero makes the division +inf, and `inf as u32`
158+
// saturates to u32::MAX in Rust — the nested loop below would then be
159+
// scheduled for ~1.8e19 iterations and never return. The geometry pass
160+
// rejects 0.01 but not 0, so the floor has to live here.
161+
let spacing = if self.dot_spacing.is_finite() && self.dot_spacing >= 1.0 {
162+
self.dot_spacing
163+
} else {
164+
1.0
165+
};
158166
let radius = self.dot_radius;
159167
let margin = spacing;
160168

161-
let cols = ((w - margin * 2.0) / spacing) as u32;
162-
let rows = ((h - margin * 2.0) / spacing) as u32;
169+
// Belt and braces: even a legal spacing on a very large box should not
170+
// be able to schedule an unbounded amount of work.
171+
const MAX_DOTS_PER_AXIS: u32 = 4096;
172+
let cols = (((w - margin * 2.0) / spacing) as u32).min(MAX_DOTS_PER_AXIS);
173+
let rows = (((h - margin * 2.0) / spacing) as u32).min(MAX_DOTS_PER_AXIS);
163174

164175
for row in 0..rows {
165176
for col in 0..cols {

‎crates/rustmotion-components/src/shape.rs‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,15 @@ impl Painter for Shape {
6161
.iter()
6262
.map(|c| color4f_from_hex(c))
6363
.collect();
64-
let stops: Option<Vec<f32>> = gradient.stops.clone();
64+
// skia asserts `pos.len() == colors.len()` inside the gradient
65+
// shader — a mismatch aborts the process instead of erroring.
66+
// Nothing upstream enforces the pairing, so drop stops we
67+
// cannot honour and let skia distribute the colours evenly.
68+
let stops: Option<Vec<f32>> = gradient
69+
.stops
70+
.as_ref()
71+
.filter(|s| s.len() == colors.len())
72+
.cloned();
6573
let mut paint = Paint::default();
6674
paint.set_anti_alias(true);
6775

‎crates/rustmotion-components/src/table.rs‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -146,7 +146,14 @@ impl Table {
146146
let text_color = self.style.color_str_or("#FFFFFF");
147147
let header_text_color = self.header_text_color.as_deref().unwrap_or("#FFFFFF");
148148
let default_row_colors = vec!["#1F2937".to_string(), "#111827".to_string()];
149-
let row_colors = self.row_colors.as_ref().unwrap_or(&default_row_colors);
149+
// `"row_colors": []` deserializes to Some(vec![]), not None — a generator
150+
// writes it to mean "no striping". Row painting indexes this slice, so an
151+
// empty one has to fall back rather than reach the painter.
152+
let row_colors = self
153+
.row_colors
154+
.as_ref()
155+
.filter(|c| !c.is_empty())
156+
.unwrap_or(&default_row_colors);
150157

151158
// Resolve fonts before the optional clip below so an early return on
152159
// font failure keeps canvas save/restore balanced.

‎crates/rustmotion-components/src/tag_cloud.rs‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -79,11 +79,12 @@ impl TagCloud {
7979
1.0 - (1.0 - p).powi(3)
8080
}
8181

82+
/// Never empty: callers index into it with `%`, and `"colors": []` — which a
83+
/// generator emits to mean "no custom palette" — otherwise divides by zero.
8284
fn palette(&self) -> Vec<&str> {
83-
if let Some(colors) = &self.colors {
84-
colors.iter().map(|s| s.as_str()).collect()
85-
} else {
86-
DEFAULT_PALETTE.to_vec()
85+
match &self.colors {
86+
Some(colors) if !colors.is_empty() => colors.iter().map(|s| s.as_str()).collect(),
87+
_ => DEFAULT_PALETTE.to_vec(),
8788
}
8889
}
8990

Lines changed: 185 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,185 @@
1+
//! A painter must never panic — nor hang — on input the schema accepts.
2+
//!
3+
//! `rustmotion validate` is the documented gate before delivery, and it answers
4+
//! "Valid scenario" for every component below. Each one was observed aborting or
5+
//! wedging the renderer mid-frame, which kills the whole encode: the scenarios
6+
//! here are the reproductions, verbatim, kept as a regression floor.
7+
//!
8+
//! Every case routes through the real pipeline — serde, box_builder, run_layout,
9+
//! paint_tree — so a fix that only guards the painter while leaving the component
10+
//! undeserialisable would still fail.
11+
12+
use std::sync::mpsc;
13+
use std::time::Duration;
14+
15+
use rustmotion_components::box_builder::{build_scene_with_anim, BuildAnimationCtx};
16+
use rustmotion_components::legacy_dispatch::LegacyPaintDispatcher;
17+
use rustmotion_components::{ChildComponent, Component, PositionMode};
18+
use rustmotion_core::css::taffy_bridge::ConversionContext;
19+
use rustmotion_core::engine::layout_pass::run_layout;
20+
use rustmotion_core::engine::paint_pass::{paint_tree, PaintFrame};
21+
22+
const W: u32 = 400;
23+
const H: u32 = 300;
24+
25+
/// Deserialize one component and paint it at `time`. Panics propagate — that is
26+
/// the point of the test.
27+
fn paint(json: serde_json::Value, time: f64) {
28+
let component: Component = serde_json::from_value(json).expect("component is schema-valid");
29+
let children = vec![ChildComponent {
30+
component,
31+
position: Some(PositionMode::Absolute { x: 0.0, y: 0.0 }),
32+
x: None,
33+
y: None,
34+
z_index: None,
35+
bleed: false,
36+
}];
37+
38+
let mut surface =
39+
skia_safe::surfaces::raster_n32_premul((W as i32, H as i32)).expect("raster surface");
40+
let canvas = surface.canvas();
41+
canvas.clear(skia_safe::Color4f::new(0.0, 0.0, 0.0, 0.0));
42+
43+
let built = build_scene_with_anim(
44+
&children,
45+
(W as f32, H as f32),
46+
BuildAnimationCtx {
47+
time,
48+
scene_duration: 2.0,
49+
fps: 30,
50+
},
51+
);
52+
let layout = run_layout(
53+
&built.root,
54+
(W as f32, H as f32),
55+
&ConversionContext::default(),
56+
);
57+
let dispatcher = LegacyPaintDispatcher::for_scene(&built);
58+
let frame = PaintFrame {
59+
time,
60+
frame_index: (time * 30.0) as u32,
61+
fps: 30,
62+
video_width: W,
63+
video_height: H,
64+
scene_duration: 2.0,
65+
camera: None,
66+
};
67+
paint_tree(canvas, &built.root, &layout, &frame, &dispatcher);
68+
}
69+
70+
/// Paint on a worker so a runaway loop fails the test instead of wedging the
71+
/// suite. `dot_spacing: 0` used to spin for billions of iterations; a plain
72+
/// `paint()` call would hang CI rather than report.
73+
fn paint_within(json: serde_json::Value, time: f64, budget: Duration, what: &str) {
74+
let (tx, rx) = mpsc::channel();
75+
let worker = std::thread::spawn(move || {
76+
paint(json, time);
77+
let _ = tx.send(());
78+
});
79+
match rx.recv_timeout(budget) {
80+
Ok(()) => worker.join().expect(what),
81+
Err(_) => panic!("{what}: still painting after {budget:?} — runaway loop"),
82+
}
83+
}
84+
85+
#[test]
86+
fn shape_survives_a_gradient_whose_stops_do_not_match_its_colors() {
87+
// skia asserts pos.len() == colors.len() inside the gradient shader, so a
88+
// mismatched `stops` aborted the process rather than returning an error.
89+
for gradient_type in ["linear", "radial"] {
90+
paint(
91+
serde_json::json!({
92+
"type": "shape",
93+
"shape": "rect",
94+
"style": { "width": 200, "height": 100 },
95+
"fill": {
96+
"type": gradient_type,
97+
"colors": ["#FF0000", "#0000FF"],
98+
"stops": [0.0, 0.5, 1.0]
99+
}
100+
}),
101+
0.5,
102+
);
103+
}
104+
}
105+
106+
#[test]
107+
fn table_survives_an_empty_row_colors_list() {
108+
// `row_colors: []` deserializes to Some(vec![]), so the default palette was
109+
// never substituted and the modulo guard still indexed an empty slice.
110+
paint(
111+
serde_json::json!({
112+
"type": "table",
113+
"headers": ["A", "B"],
114+
"rows": [["1", "2"], ["3", "4"]],
115+
"row_colors": [],
116+
"style": { "width": 300, "height": 200 }
117+
}),
118+
0.5,
119+
);
120+
}
121+
122+
#[test]
123+
fn tag_cloud_survives_an_empty_colors_list() {
124+
// palette() returned the caller's empty vec, and the painter took
125+
// `index % palette.len()` on it.
126+
paint(
127+
serde_json::json!({
128+
"type": "tag_cloud",
129+
"tags": [{ "text": "rust", "weight": 3 }, { "text": "skia", "weight": 1 }],
130+
"colors": [],
131+
"style": { "width": 300, "height": 200 }
132+
}),
133+
0.5,
134+
);
135+
}
136+
137+
#[test]
138+
fn dot_map_terminates_on_a_zero_dot_spacing() {
139+
// (w - 0) / 0 is +inf, and `inf as u32` saturates to u32::MAX in Rust, so the
140+
// nested loop was scheduled for ~1.8e19 iterations. The geometry pass catches
141+
// dot_spacing: 0.01 but not 0.
142+
paint_within(
143+
serde_json::json!({
144+
"type": "dot_map",
145+
"points": [{ "lat": 48.8, "lng": 2.3 }],
146+
"dot_spacing": 0,
147+
"style": { "width": 300, "height": 150 }
148+
}),
149+
0.5,
150+
Duration::from_secs(10),
151+
"dot_map with dot_spacing: 0",
152+
);
153+
}
154+
155+
#[test]
156+
fn codeblock_diff_survives_multibyte_text() {
157+
// The edit script counts bytes while the reveal interpolates a fraction of
158+
// that count, so mid-animation offsets landed inside a multi-byte character
159+
// and `replace_range` aborted with "not a char boundary".
160+
for (from, to) in [
161+
("let a = 1;", "let café = «héllo→»;"),
162+
("let x = 1;", "let y = \"éàü\";"),
163+
("a", "日本語のテキスト"),
164+
("ok", "🎬 clap"),
165+
] {
166+
// Sweep the reveal: the panic only fires on the frames where progress
167+
// lands part-way through a glyph.
168+
for step in 0..=20 {
169+
paint(
170+
serde_json::json!({
171+
"type": "codeblock",
172+
"code": from,
173+
"language": "rust",
174+
"diff": true,
175+
"states": [
176+
{ "code": from, "at": 0.0 },
177+
{ "code": to, "at": 0.4, "cursor": { "enabled": true } }
178+
],
179+
"style": { "width": 380, "height": 120 }
180+
}),
181+
f64::from(step) * 0.1,
182+
);
183+
}
184+
}
185+
}

0 commit comments

Comments
 (0)