From f3a79ce6092355f6c250ef87c3f53a64ecb738fb Mon Sep 17 00:00:00 2001 From: Fini Date: Mon, 20 Jul 2026 21:35:43 +0800 Subject: [PATCH] fix(agent): preserve absolute avatar media during finalize --- crates/op-orchestrator/src/avatar_repair.rs | 61 +++++++++---- .../src/avatar_repair_tests.rs | 74 +++++++++++++++ .../src/loop_finalize_tests.rs | 89 +++++++++++++++++++ 3 files changed, 207 insertions(+), 17 deletions(-) diff --git a/crates/op-orchestrator/src/avatar_repair.rs b/crates/op-orchestrator/src/avatar_repair.rs index 5cf3c24be..73f959df4 100644 --- a/crates/op-orchestrator/src/avatar_repair.rs +++ b/crates/op-orchestrator/src/avatar_repair.rs @@ -2,8 +2,10 @@ //! //! An explicitly named/role-tagged avatar is a fixed SQUARE circle: //! `width == height`, `cornerRadius` ≥ half the size, `clipContent: true`, -//! with its image child filling both axes. Models routinely violate that -//! contract (measured: +//! with its image child covering both axes. An auto-layout slot uses +//! `fill_container`; an explicit `layout: none` slot pins the image to the +//! slot's numeric size because fill sizing has no flex parent. Models routinely +//! violate that contract (measured: //! GLM-5.2 test0711-2.op built an 88×44 pill holding a 44×44 image — on //! canvas it reads as an empty grey circle NEXT TO a square photo). The //! shape is a CONTRACT (a round avatar slot is round), so it is repaired @@ -18,7 +20,7 @@ //! NUMERIC thumb stays the designer's call. use crate::types::DocSink; -use jian_ops_schema::node::container::CornerRadius; +use jian_ops_schema::node::container::{CornerRadius, LayoutMode}; use jian_ops_schema::node::PenNode; use op_editor_core::{EditorCommand, NodeId, PenNodeExt}; @@ -121,6 +123,7 @@ fn avatar_slot_repair( Some(CornerRadius::PerCorner(corners)) => corners.iter().copied().fold(f64::MAX, f64::min), None => 0.0, }; + let absolute_slot = matches!(frame.container.layout.as_ref(), Some(LayoutMode::None)); // Explicit-avatar branch: the slot ITSELF may carry no numeric // height at all (measured: an "AvatarImg" slot authored fill×fill // holding a fill×300 headshot resolved as a 42×300 strip down the @@ -132,21 +135,20 @@ fn avatar_slot_repair( Some(height) if height > 0.0 && height <= MAX_AVATAR_SIDE => height, Some(_) => return None, None => 44.0, - }; + } + .round(); let clips = frame.container.clip_content == Some(true); - let image_fills = image_fills_both_axes(only_child); + let image_covers = image_covers_slot(only_child, absolute_slot, side); let slot_not_square = node.width_px() != Some(side) || node.height_px() != Some(side); let slot_not_round = radius + f64::EPSILON < side / 2.0; - if !clips || !image_fills || slot_not_square || slot_not_round { + if !clips || !image_covers || slot_not_square || slot_not_round { let slot_patch = format!( r#"{{"width":{side},"height":{side},"clipContent":true,"cornerRadius":{radius}}}"#, - side = side.round(), + side = side, radius = (side / 2.0).round() ); - let image_patch = Some(( - only_child.id_str().to_string(), - r#"{"width":"fill_container","height":"fill_container"}"#.to_string(), - )); + let image_patch_json = image_cover_patch(absolute_slot, side); + let image_patch = Some((only_child.id_str().to_string(), image_patch_json)); return Some((slot_patch, image_patch)); } } @@ -154,6 +156,7 @@ fn avatar_slot_repair( if height <= 0.0 || height > MAX_AVATAR_SIDE { return None; } + let side = height.round(); let width = node.width_px(); // Row-thumb branch: only the unbounded flex-steal shape (width // fill_container) qualifies — a deliberately wide numeric thumb is a @@ -166,20 +169,20 @@ fn avatar_slot_repair( if !row_thumb { return None; } - let square = width == Some(height); + let square = width == Some(side); let clips = frame.container.clip_content == Some(true); - let image_fills = image_fills_both_axes(only_child); - if square && clips && image_fills { + let image_covers = image_covers_slot(only_child, absolute_slot, side); + if square && clips && image_covers { return None; } let slot_patch = format!( r#"{{"width":{h},"height":{h},"clipContent":true}}"#, - h = height.round() + h = side ); - let image_patch = (!image_fills).then(|| { + let image_patch = (!image_covers).then(|| { ( only_child.id_str().to_string(), - r#"{"width":"fill_container","height":"fill_container"}"#.to_string(), + image_cover_patch(absolute_slot, side), ) }); Some((slot_patch, image_patch)) @@ -201,6 +204,30 @@ fn image_fills_both_axes(node: &PenNode) -> bool { if sizing_is_fill_container(&image.width) && sizing_is_fill_container(&image.height)) } +fn image_is_pinned_to_square(node: &PenNode, side: f64) -> bool { + matches!(node, PenNode::Image(_)) + && node.width_px() == Some(side) + && node.height_px() == Some(side) + && node.base().x.unwrap_or(0.0).abs() <= f64::EPSILON + && node.base().y.unwrap_or(0.0).abs() <= f64::EPSILON +} + +fn image_covers_slot(node: &PenNode, absolute_slot: bool, side: f64) -> bool { + if absolute_slot { + image_is_pinned_to_square(node, side) + } else { + image_fills_both_axes(node) + } +} + +fn image_cover_patch(absolute_slot: bool, side: f64) -> String { + if absolute_slot { + format!(r#"{{"x":0,"y":0,"width":{side},"height":{side}}}"#) + } else { + r#"{"width":"fill_container","height":"fill_container"}"#.to_string() + } +} + fn sizing_is_fill_container(width: &Option) -> bool { use jian_ops_schema::sizing::{SizingBehavior, SizingKeyword}; matches!( diff --git a/crates/op-orchestrator/src/avatar_repair_tests.rs b/crates/op-orchestrator/src/avatar_repair_tests.rs index c9e7afcd2..abc2a310a 100644 --- a/crates/op-orchestrator/src/avatar_repair_tests.rs +++ b/crates/op-orchestrator/src/avatar_repair_tests.rs @@ -111,6 +111,44 @@ fn square_clipped_avatar_with_zero_radius_becomes_round() { ); } +#[test] +fn absolute_avatar_image_is_pinned_to_numeric_slot_bounds() { + let doc: jian_ops_schema::PenDocument = serde_json::from_str( + r##"{ "version": "1.0", "children": [{ + "type":"frame", "id":"root", "name":"Screen", "width":390, "height":844, + "layout":"vertical", "children":[{ + "type":"frame", "id":"slot", "name":"Avatar", "role":"avatar", + "width":40, "height":40, "layout":"none", "cornerRadius":20, + "clipContent":true, "children":[{ + "type":"image", "id":"img", "name":"face headshot", "src":"", + "x":3, "y":5, "width":"fill_container", "height":"fill_container" + }] + }] + }] }"##, + ) + .expect("doc"); + let mut state = op_editor_core::EditorState::from_document(doc); + let mut sink = crate::loop_finalize::StateDocSink { state: &mut state }; + + crate::avatar_repair::repair_avatar_slots_for_all_roots(&mut sink); + + let root = &state.active_children()[0]; + let image = find_by_id(root, "img").expect("image"); + assert_eq!(image.width_px(), Some(40.0)); + assert_eq!(image.height_px(), Some(40.0)); + assert_eq!(image.base().x, Some(0.0)); + assert_eq!(image.base().y, Some(0.0)); + + let once = serde_json::to_string(state.active_children()).expect("snapshot"); + let mut sink = crate::loop_finalize::StateDocSink { state: &mut state }; + crate::avatar_repair::repair_avatar_slots_for_all_roots(&mut sink); + assert_eq!( + serde_json::to_string(state.active_children()).expect("snapshot"), + once, + "absolute avatar repair is idempotent" + ); +} + /// The EXACT shape measured in test0711-22.op (23:07 run): slot clipped and /// image filling, but `width: fill_container` stretched the avatar into a /// page-wide pill. The repair squares it to its height. @@ -213,6 +251,42 @@ fn fill_container_row_thumbnail_is_squared_but_numeric_wide_thumb_is_kept() { ); } +#[test] +fn absolute_row_thumbnail_keeps_numeric_image_bounds_when_squared() { + let doc: jian_ops_schema::PenDocument = serde_json::from_str( + r##"{ "version": "1.0", "children": [{ + "type":"frame", "id":"root", "name":"Music Home", "width":390, "height":844, + "layout":"vertical", "children":[{ + "type":"frame", "id":"player", "name":"MiniPlayer", + "width":"fill_container", "height":64, "layout":"horizontal", + "children":[{ + "type":"frame", "id":"art", "name":"MPArt", + "width":"fill_container", "height":40, "layout":"none", + "cornerRadius":8, "clipContent":true, "children":[{ + "type":"image", "id":"cover", "name":"album cover", "src":"", + "x":0, "y":0, "width":40, "height":40 + }] + },{ + "type":"text", "id":"title", "name":"Title", "content":"Midnight Static", + "width":"fit_content", "height":"fit_content" + }] + }] + }] }"##, + ) + .expect("doc"); + let mut state = op_editor_core::EditorState::from_document(doc); + let mut sink = crate::loop_finalize::StateDocSink { state: &mut state }; + + crate::avatar_repair::repair_avatar_slots_for_all_roots(&mut sink); + + let root = &state.active_children()[0]; + let slot = find_by_id(root, "art").expect("slot"); + let image = find_by_id(root, "cover").expect("image"); + assert_eq!(slot.width_px(), Some(40.0)); + assert_eq!(image.width_px(), Some(40.0)); + assert_eq!(image.height_px(), Some(40.0)); +} + /// test0711-22 00:25 shape: "AvatarImg" slot authored fill×fill holding a /// fill×300 headshot — resolved as a 42×300 strip down the screen. The /// explicit AvatarImg SLOT name is the contract signal; the slot becomes a diff --git a/crates/op-orchestrator/src/loop_finalize_tests.rs b/crates/op-orchestrator/src/loop_finalize_tests.rs index 69daec7c5..c2df109f3 100644 --- a/crates/op-orchestrator/src/loop_finalize_tests.rs +++ b/crates/op-orchestrator/src/loop_finalize_tests.rs @@ -50,6 +50,95 @@ fn fill_of(state: &EditorState, name: &str) -> Option { Some(v.get("fill").cloned().unwrap_or(json!(null))) } +/// A fixed-size avatar using absolute layout must leave finalize with a +/// fixed-size image. `fill_container` has no usable cross-axis size in a +/// `layout: none` parent and resolves the image to zero height. +#[test] +fn loop_finalize_keeps_absolute_avatar_image_sized_to_slot() { + let mut state = state_with_forest(json!([{ + "type": "frame", + "id": "root", + "name": "Music Home", + "width": 390, + "height": 844, + "layout": "vertical", + "children": [{ + "type": "frame", + "id": "header", + "name": "Header", + "width": "fill_container", + "height": 64, + "layout": "horizontal", + "children": [{ + "type": "frame", + "id": "avatar", + "name": "Avatar", + "role": "avatar", + "width": 40, + "height": 40, + "layout": "none", + "cornerRadius": 20, + "clipContent": true, + "children": [{ + "type": "image", + "id": "portrait", + "name": "Portrait", + "src": "https://example.com/avatar.jpg", + "x": 0, + "y": 0, + "width": 40, + "height": 40 + }] + }] + }] + }])); + + apply_loop_finalize(&mut state); + + let image = find_by_name(state.active_children(), "Portrait").expect("avatar image survives"); + assert_eq!(image.width_px(), Some(40.0)); + assert_eq!( + image.height_px(), + Some(40.0), + "absolute avatar image must not be rewritten to fill_container" + ); +} + +#[test] +fn loop_finalize_keeps_absolute_row_thumbnail_image_sized_to_slot() { + let mut state = state_with_forest(json!([{ + "type":"frame", "id":"root", "name":"Music Home", "width":390, "height":844, + "layout":"vertical", "children":[{ + "type":"frame", "id":"player", "name":"MiniPlayer", + "width":"fill_container", "height":64, "layout":"horizontal", + "children":[{ + "type":"frame", "id":"art", "name":"MPArt", + "width":"fill_container", "height":40, "layout":"none", + "cornerRadius":8, "clipContent":true, "children":[{ + "type":"image", "id":"cover", "name":"Album Cover", + "src":"https://example.com/cover.jpg", "x":0, "y":0, + "width":40, "height":40 + }] + },{ + "type":"text", "id":"title", "name":"Track Title", + "content":"Midnight Static", "width":"fit_content", "height":"fit_content" + }] + }] + }])); + + apply_loop_finalize(&mut state); + + let slot = find_by_name(state.active_children(), "MPArt").expect("thumbnail survives"); + let image = find_by_name(state.active_children(), "Album Cover").expect("image survives"); + assert_eq!(slot.width_px(), Some(40.0)); + assert_eq!(image.width_px(), Some(40.0)); + assert_eq!( + image.height_px(), + Some(40.0), + "absolute row thumbnail must not be rewritten to fill_container" + ); +} + #[test] fn loop_finalize_preserves_explicit_mobile_viewport_with_tall_content() { let mut state = state_with_forest(json!([{