diff --git a/crates/op-editor-ui/src/widgets/ai_chat_transcript_steps.rs b/crates/op-editor-ui/src/widgets/ai_chat_transcript_steps.rs index 8f28437f1..32a2664b4 100644 --- a/crates/op-editor-ui/src/widgets/ai_chat_transcript_steps.rs +++ b/crates/op-editor-ui/src/widgets/ai_chat_transcript_steps.rs @@ -31,6 +31,11 @@ pub(crate) fn activity_step(activity: &ChatActivity) -> ParsedStep { ParsedStep { title: activity.title.clone(), status: Some(status), + // One entry, newlines intact. A row's detail may be an itemized list + // — the quality passes write one line per applied repair — and the + // layout step splits it: `wrap_units` breaks on `\n` before wrapping, + // so each line becomes its own expandable row. Do not "helpfully" + // pre-split here; that would duplicate the split, not enable it. details: activity.detail.iter().cloned().collect(), } } diff --git a/crates/op-editor-ui/src/widgets/ai_chat_transcript_tests/activities.rs b/crates/op-editor-ui/src/widgets/ai_chat_transcript_tests/activities.rs index da0ec9f69..23b3ec6ba 100644 --- a/crates/op-editor-ui/src/widgets/ai_chat_transcript_tests/activities.rs +++ b/crates/op-editor-ui/src/widgets/ai_chat_transcript_tests/activities.rs @@ -288,3 +288,120 @@ render: captured frame "collapsed TS accordions hide details until opened" ); } + +#[test] +fn an_itemized_activity_detail_expands_into_one_row_per_line() { + // The quality passes report their repairs as one line each on the + // "Polishing the layout" row. A single-string detail that rendered as one + // unwrappable blob is exactly the "I cannot see what the check changed" + // complaint this exists to answer. + let detail = [ + "3 auto-repair(s) applied", + "layout · table-gap · Pricing Row [n42] · gap 0 → 16", + "palette · light-mobile-nav-surface · Tab Bar [n7] · fill #F8FAFC → #FFFFFF", + "hierarchy · text-hierarchy · Title [n3] · fontWeight 800 → 400", + ] + .join("\n"); + let mut message = ChatMessage::assistant_streaming(); + message.activities = vec![op_editor_core::ChatActivity { + id: "__polish".into(), + title: "Polishing the layout".into(), + detail: Some(detail), + status: op_editor_core::ChatActivityStatus::Done, + content_offset: None, + }]; + + let items = build_transcript( + std::slice::from_ref(&message), + body(), + op_editor_core::Locale::EnUs, + ); + + let step = &items[0].steps[0]; + assert_eq!(step.label, "Polishing the layout"); + // Rows are word-wrapped to the bubble width, so a long record may occupy + // more than one row — what must not happen is lines being merged or + // dropped. + assert!( + step.details.len() >= 4, + "each reported repair must reach the row list: {:?}", + step.details + ); + // Each reported repair must OWN a row: a row that merely CONTAINS the + // text could be one blob the layout wrapped at an arbitrary column, which + // is the unreadable rendering this test exists to reject. + for record in [ + "3 auto-repair(s) applied", + "layout · table-gap · Pricing Row", + "palette · light-mobile-nav-surface", + "hierarchy · text-hierarchy · Title", + ] { + assert!( + step.details.iter().any(|row| row.starts_with(record)), + "`{record}` must begin its own row: {:?}", + step.details + ); + } + assert!( + step.details.iter().all(|row| !row.contains('\n')), + "no row may carry a raw newline: {:?}", + step.details + ); +} + +#[test] +fn a_long_repair_list_renders_every_line_it_was_given_plus_the_overflow_notice() { + // The host caps the list at 30 and appends a localized "and N more" + // notice; the transcript's job is to render exactly what it was handed — + // silently dropping rows here would hide repairs the host chose to show. + let mut lines = vec!["45 auto-repair(s) applied".to_string()]; + lines.extend( + (0..30).map(|i| format!("layout · container-geometry · Card {i} [n{i}] · gap 24 → 16")), + ); + lines.push("… and 15 more (see log)".to_string()); + let mut message = ChatMessage::assistant_streaming(); + message.activities = vec![op_editor_core::ChatActivity { + id: "__polish".into(), + title: "Polishing the layout".into(), + detail: Some(lines.join("\n")), + status: op_editor_core::ChatActivityStatus::Done, + content_offset: None, + }]; + message.action_step_expanded_overrides = vec![Some(true)]; + + let items = build_transcript( + std::slice::from_ref(&message), + body(), + op_editor_core::Locale::EnUs, + ); + + let step = &items[0].steps[0]; + assert!( + step.details.len() >= 32, + "head line + 30 records + overflow notice must all reach the rows: {}", + step.details.len() + ); + assert!( + step.details + .last() + .is_some_and(|row| row.starts_with("… and 15 more")), + "the truncation notice must own the closing row: {:?}", + step.details.last() + ); + assert_eq!( + step.details + .iter() + .filter(|row| row.starts_with("layout · container-geometry")) + .count(), + 30, + "every one of the 30 shown records must begin its own row" + ); + assert!( + step.expanded, + "an explicit expand override must survive the itemized list" + ); + assert!( + step.rect.size.y > 32.0 * LINE_H, + "an expanded 32-row list must reserve height for its rows" + ); +} diff --git a/crates/op-host-desktop/src/chat_session.rs b/crates/op-host-desktop/src/chat_session.rs index d3b04ed04..7eee377f3 100644 --- a/crates/op-host-desktop/src/chat_session.rs +++ b/crates/op-host-desktop/src/chat_session.rs @@ -348,6 +348,18 @@ fn execute_tool_requests( serde_json::json!({ "check": check.key(), "count": count }) }) .collect::>(), + // The itemized half — one rendered line per applied + // edit, so the loop's credential can list WHAT was + // repaired, not only how much. + "records": quality + .records() + .iter() + .map(op_orchestrator::RepairRecord::line) + .collect::>(), + // Non-edit statements — today, a deliberately skipped + // pass tier. Carried separately from `records` so the + // loop never reports a decision as a repair. + "notes": quality.notes(), }, }) .to_string(), diff --git a/crates/op-host-desktop/src/design_session.rs b/crates/op-host-desktop/src/design_session.rs index 08faf1a34..018c4b911 100644 --- a/crates/op-host-desktop/src/design_session.rs +++ b/crates/op-host-desktop/src/design_session.rs @@ -457,17 +457,35 @@ fn apply_progress(msg: &mut ChatMessage, progress: &[Progress], locale: Locale) // treatment as the two reports above. `remaining` is `None`: // the promise-delivery check has not run yet at this point, and // an assumed "no issues left" would be a claim, not a fact. - Progress::QualityChecked { checks, repairs } => { - match op_host_services::quality_credential::quality_credential_line( - &op_ai::chat_provider::QualitySummary { - checks: checks.clone(), - repairs: repairs.clone(), - }, - None, - ) { - Some(line) => append_narration(msg, line.trim_start()), - None => false, + Progress::QualityChecked { + checks, + repairs, + records, + notes, + } => { + let quality = op_ai::chat_provider::QualitySummary { + checks: checks.clone(), + repairs: repairs.clone(), + records: records.clone(), + notes: notes.clone(), + }; + let mut event_changed = + match op_host_services::quality_credential::quality_credential_line( + &quality, None, + ) { + Some(line) => append_narration(msg, line.trim_start()), + None => false, + }; + // The itemized half rides the "Polishing the layout" row's + // detail rather than the narration: the row is expandable, so + // 41 repairs are one click away instead of 41 lines of prose. + // `_with_records` is deliberately NOT used above — that would + // print the same list twice. + if let Some(detail) = repair_detail_text(&quality, locale) { + event_changed |= + update_activity(msg, "__polish", ChatActivityStatus::Done, Some(detail)); } + event_changed } }; } @@ -548,6 +566,44 @@ fn planned_narration(locale: Locale, count: usize) -> String { op_i18n::translate(locale, key).replace("{{count}}", &count.to_string()) } +/// Build the expandable detail block for the "Polishing the layout" row: +/// a localized "N auto-repair(s) applied" head line, then one line per +/// repair (capped), then a localized notice for whatever the cap left out. +/// +/// Newline-separated because [`op_editor_core::ChatActivity::detail`] is one +/// string that the transcript splits per line into the row's expandable +/// details. `None` when the passes reported no itemized repairs at all — an +/// empty block would add a chevron that reveals nothing. +fn repair_detail_text( + quality: &op_ai::chat_provider::QualitySummary, + locale: Locale, +) -> Option { + let lines = op_host_services::quality_credential::quality_repair_detail_lines(quality); + let notes = op_host_services::quality_credential::quality_note_lines(quality); + if lines.is_empty() && notes.is_empty() { + return None; + } + // Notes first: a run whose intent tier was skipped for authored template + // input reads "layout 0" as "not checked", not "nothing wrong", and a + // reader who never expands past the first line must still get that. + let mut out = notes; + if !lines.is_empty() { + out.push( + op_i18n::translate(locale, "ai.designProgress.detail.repairsApplied") + .replace("{{count}}", &quality.records.len().to_string()), + ); + out.extend(lines); + let overflow = op_host_services::quality_credential::quality_repair_overflow(quality); + if overflow > 0 { + out.push( + op_i18n::translate(locale, "ai.designProgress.detail.repairsMore") + .replace("{{count}}", &overflow.to_string()), + ); + } + } + Some(out.join("\n")) +} + fn append_narration(msg: &mut ChatMessage, text: &str) -> bool { if text.is_empty() || msg.content.contains(text) { return false; @@ -590,6 +646,10 @@ fn count_u32(count: usize) -> u32 { #[path = "design_session_tests.rs"] mod tests; +#[cfg(test)] +#[path = "design_session_quality_tests.rs"] +mod quality_tests; + #[cfg(test)] #[path = "design_session_worker_tests.rs"] mod worker_tests; diff --git a/crates/op-host-desktop/src/design_session_quality_tests.rs b/crates/op-host-desktop/src/design_session_quality_tests.rs new file mode 100644 index 000000000..c2aadca05 --- /dev/null +++ b/crates/op-host-desktop/src/design_session_quality_tests.rs @@ -0,0 +1,210 @@ +//! The quality-ledger half of the design-turn progress adapter: what the +//! deterministic passes repaired, and what they deliberately did not run. +//! +//! Split out of `design_session_tests.rs` at the 800-line cap. These share +//! that file's `super::*` surface and drive the same `apply_progress` entry +//! point; they are grouped here because they all assert on ONE thing — the +//! "Polishing the layout" row's expandable detail, which is the only place a +//! user can see which node a pass touched and why. + +use super::*; + +#[test] +fn quality_records_land_on_the_polish_row_as_expandable_detail() { + // The user's complaint was that the check stage reported "41 auto-repair(s) + // applied" and nothing else. The narration keeps the headline; the itemized + // list rides the "Polishing the layout" row, which the transcript renders + // as expandable detail. + let mut message = op_editor_core::ChatMessage::assistant_streaming(); + let events = vec![ + Progress::CleanupDone, + Progress::QualityChecked { + checks: vec!["layout".into(), "palette".into()], + repairs: vec![("layout".into(), 2), ("palette".into(), 1)], + records: vec![ + "layout · table-gap · Pricing Row [n42] · gap 0 → 16".into(), + "layout · container-geometry · Hero [n7] · padding [32,32] → [16,16]".into(), + "palette · light-mobile-nav-surface · Tab Bar [n9] · fill #F8FAFC → #FFFFFF".into(), + ], + notes: Vec::new(), + }, + ]; + + assert!(super::apply_progress(&mut message, &events, Locale::EnUs)); + + let polish = message + .activities + .iter() + .find(|activity| activity.id == "__polish") + .expect("cleanup must have created the polish row"); + let detail = polish.detail.as_deref().expect("itemized detail"); + assert!( + detail.starts_with("3 auto-repair(s) applied"), + "the head line states the count in the user's locale: {detail}" + ); + for record in [ + "gap 0 → 16", + "padding [32,32] → [16,16]", + "fill #F8FAFC → #FFFFFF", + ] { + assert!( + detail.contains(record), + "`{record}` must be listed on the row: {detail}" + ); + } + assert!( + message.content.contains("41 auto-repair(s)") + || message.content.contains("3 auto-repair(s)"), + "the narration keeps the headline credential: {}", + message.content + ); + assert_eq!( + message.content.matches("gap 0 → 16").count(), + 0, + "the itemized list must not also be dumped into the narration" + ); +} + +#[test] +fn a_long_repair_list_is_capped_on_the_row_with_a_localized_remainder_notice() { + let mut message = op_editor_core::ChatMessage::assistant_streaming(); + let records: Vec = (0..45) + .map(|i| format!("layout · container-geometry · Card {i} [n{i}] · gap 24 → 16")) + .collect(); + let events = vec![ + Progress::CleanupDone, + Progress::QualityChecked { + checks: vec!["layout".into()], + repairs: vec![("layout".into(), 45)], + records, + notes: Vec::new(), + }, + ]; + + assert!(super::apply_progress(&mut message, &events, Locale::ZhCn)); + + let detail = message + .activities + .iter() + .find(|activity| activity.id == "__polish") + .and_then(|activity| activity.detail.clone()) + .expect("itemized detail"); + let lines: Vec<&str> = detail.lines().collect(); + assert_eq!( + lines.len(), + 32, + "head line + 30 records + remainder notice: {lines:?}" + ); + assert!( + lines[0].contains("45"), + "the head line counts every repair, not just the shown ones: {}", + lines[0] + ); + assert!( + lines[31].contains("15"), + "the remainder notice states how many were withheld: {}", + lines[31] + ); + assert!( + !lines[31].is_ascii(), + "the notice must come from the locale table, not a hardcoded English string: {}", + lines[31] + ); +} + +const TIER_SKIP_NOTE: &str = "intent-tier passes skipped (template provenance: slide-deck via \ + namespaced-variables) — authored spacing, surfaces and palette \ + kept as designed; contract-tier checks still ran"; + +#[test] +fn a_skipped_tier_note_heads_the_polish_row_detail() { + let mut message = op_editor_core::ChatMessage::assistant_streaming(); + let events = vec![ + Progress::CleanupDone, + Progress::QualityChecked { + checks: vec!["layout".into()], + repairs: vec![("layout".into(), 1)], + records: vec!["layout · table-gap · Pricing Row [n42] · gap 0 → 16".into()], + notes: vec![TIER_SKIP_NOTE.to_string()], + }, + ]; + + assert!(super::apply_progress(&mut message, &events, Locale::EnUs)); + + let detail = message + .activities + .iter() + .find(|activity| activity.id == "__polish") + .and_then(|activity| activity.detail.clone()) + .expect("itemized detail"); + let lines: Vec<&str> = detail.lines().collect(); + assert!( + lines[0].starts_with("intent-tier passes skipped"), + "what was deliberately NOT run must be the first thing read: {lines:?}" + ); + assert!( + lines[1].contains("1 auto-repair(s) applied"), + "the count head line follows the note: {lines:?}" + ); + assert!( + lines[2].contains("table-gap"), + "then the itemized repairs: {lines:?}" + ); +} + +#[test] +fn a_template_run_that_repaired_nothing_still_shows_why() { + // The shape tiering actually produces: authored input needs no repairs, + // so there is no record list at all. Before notes reached the row, this + // rendered as a bare "Polishing the layout ✓" with nothing behind it — + // indistinguishable from "we checked everything and it was perfect". + let mut message = op_editor_core::ChatMessage::assistant_streaming(); + let events = vec![ + Progress::CleanupDone, + Progress::QualityChecked { + checks: vec!["layout".into(), "structure".into()], + repairs: Vec::new(), + records: Vec::new(), + notes: vec![TIER_SKIP_NOTE.to_string()], + }, + ]; + + assert!(super::apply_progress(&mut message, &events, Locale::EnUs)); + + let detail = message + .activities + .iter() + .find(|activity| activity.id == "__polish") + .and_then(|activity| activity.detail.clone()) + .expect("a note alone must still open the row"); + assert!(detail.starts_with("intent-tier passes skipped"), "{detail}"); + assert!( + !detail.contains("auto-repair(s) applied"), + "no repairs ran, so no count line should claim any: {detail}" + ); +} + +#[test] +fn a_run_with_neither_notes_nor_records_leaves_the_row_bare() { + // The honesty counterpart: nothing to itemize must not manufacture an + // empty expandable row that reveals nothing when clicked. + let mut message = op_editor_core::ChatMessage::assistant_streaming(); + let events = vec![ + Progress::CleanupDone, + Progress::QualityChecked { + checks: vec!["layout".into()], + repairs: Vec::new(), + records: Vec::new(), + notes: Vec::new(), + }, + ]; + + super::apply_progress(&mut message, &events, Locale::EnUs); + + let polish = message + .activities + .iter() + .find(|activity| activity.id == "__polish") + .expect("polish row"); + assert!(polish.detail.is_none(), "{:?}", polish.detail); +} diff --git a/crates/op-host-services/src/quality_credential.rs b/crates/op-host-services/src/quality_credential.rs index 2b1fa4fe8..3332840fd 100644 --- a/crates/op-host-services/src/quality_credential.rs +++ b/crates/op-host-services/src/quality_credential.rs @@ -48,9 +48,54 @@ pub fn quality_summary_from_repairs(summary: &op_orchestrator::RepairSummary) -> .into_iter() .map(|(check, count)| (check.key().to_string(), count)) .collect(), + records: summary + .records() + .iter() + .map(op_orchestrator::RepairRecord::line) + .collect(), + notes: summary.notes().to_vec(), } } +/// How many repair lines a caller shows inline before summarizing the rest. +/// A routine run applies dozens; the ceiling keeps one turn from burying the +/// transcript while the log keeps the complete list. +pub const MAX_INLINE_REPAIR_RECORDS: usize = 30; + +/// The itemized repair lines, capped at [`MAX_INLINE_REPAIR_RECORDS`]. +/// +/// Returns the lines only — no header, no "and N more" notice: the callers +/// that have a locale (the desktop progress panel) localize those around +/// this list, and the ones that do not (the diagnostic transcript lines) +/// render them with their own English framing. Empty when the summary +/// carries no records, so a caller can skip the whole block. +pub fn quality_repair_detail_lines(quality: &QualitySummary) -> Vec { + quality + .records + .iter() + .take(MAX_INLINE_REPAIR_RECORDS) + .cloned() + .collect() +} + +/// The non-edit statements about the run, rendered ahead of the repair list. +/// +/// Uncapped on purpose: notes are decisions, not edits — the orchestrator +/// de-duplicates them, so a run carries 0 or 1 today and would carry a +/// handful at worst. Truncating a "a whole tier of passes did not run" line +/// would hide exactly the fact it exists to surface. +pub fn quality_note_lines(quality: &QualitySummary) -> Vec { + quality.notes.clone() +} + +/// How many records the cap left out — `0` when everything is shown. +pub fn quality_repair_overflow(quality: &QualitySummary) -> usize { + quality + .records + .len() + .saturating_sub(MAX_INLINE_REPAIR_RECORDS) +} + /// Render the credential, or `None` when nothing was checked. /// /// `remaining` is how many issues are still open after the passes ran: @@ -92,6 +137,39 @@ pub fn quality_credential_line( Some(line) } +/// [`quality_credential_line`] plus one `▸` sub-line per applied repair. +/// +/// For the callers whose ONLY channel to the user is this text: the agentic +/// loop's transcript stream and the web host's thinking stream. Both render +/// `▸` sub-lines as the owning step's expandable detail +/// (`split_design_progress`), so the itemized list lands under the credential +/// instead of beside it. Hosts that can attach detail to a structured +/// progress row (the desktop panel) use the plain line and pass +/// [`quality_repair_detail_lines`] to that row instead, so nothing is shown +/// twice. +pub fn quality_credential_line_with_records( + quality: &QualitySummary, + remaining: Option, +) -> Option { + let mut line = quality_credential_line(quality, remaining)?; + // Notes lead: a tier that was deliberately SKIPPED reframes every number + // above it ("layout 0" means "not checked", not "nothing wrong"), so it + // must not sit below a list the reader may never scroll through. + for note in quality_note_lines(quality) { + line.push_str(&format!("\n ▸ {note}")); + } + for record in quality_repair_detail_lines(quality) { + line.push_str(&format!("\n ▸ {record}")); + } + let overflow = quality_repair_overflow(quality); + if overflow > 0 { + line.push_str(&format!( + "\n ▸ … and {overflow} more repair(s) — full list in the log" + )); + } + Some(line) +} + #[cfg(test)] #[path = "quality_credential_tests.rs"] mod tests; diff --git a/crates/op-host-services/src/quality_credential_tests.rs b/crates/op-host-services/src/quality_credential_tests.rs index 53310cb04..a34c67a69 100644 --- a/crates/op-host-services/src/quality_credential_tests.rs +++ b/crates/op-host-services/src/quality_credential_tests.rs @@ -1,4 +1,7 @@ -use super::{quality_credential_line, quality_summary_from_repairs}; +use super::{ + quality_credential_line, quality_credential_line_with_records, quality_note_lines, + quality_summary_from_repairs, MAX_INLINE_REPAIR_RECORDS, +}; use op_ai::chat_provider::QualitySummary; use op_orchestrator::{CheckCategory, RepairSummary}; @@ -6,6 +9,8 @@ fn summary(checks: &[&str], repairs: &[(&str, usize)]) -> QualitySummary { QualitySummary { checks: checks.iter().map(|c| c.to_string()).collect(), repairs: repairs.iter().map(|(c, n)| (c.to_string(), *n)).collect(), + records: Vec::new(), + notes: Vec::new(), } } @@ -95,3 +100,166 @@ fn repair_summary_converts_preserving_checked_and_repaired_split() { ); assert_eq!(wire.total_repairs(), 3); } + +#[test] +fn the_itemized_variant_lists_each_repair_under_the_credential() { + // For the callers whose only channel is this text (the agentic loop, the + // web thinking stream): the `▸` sub-lines are what the transcript turns + // into the credential step's expandable detail. + let quality = QualitySummary { + checks: vec!["layout".into()], + repairs: vec![("layout".into(), 2)], + records: vec![ + "layout · table-gap · Pricing Row [n42] · gap 0 → 16".into(), + "layout · container-geometry · Hero [n7] · padding [32,32] → [16,16]".into(), + ], + notes: Vec::new(), + }; + + let line = quality_credential_line_with_records(&quality, Some(0)).expect("credential"); + + assert!(line.contains("2 auto-repair(s) applied")); + assert!( + line.contains("\n ▸ layout · table-gap · Pricing Row [n42] · gap 0 → 16"), + "each record is its own sub-line: {line}" + ); + assert!( + !line.contains("more repair(s)"), + "nothing was withheld, so no remainder notice: {line}" + ); +} + +#[test] +fn the_itemized_variant_caps_the_list_and_says_how_many_it_withheld() { + let records: Vec = (0..MAX_INLINE_REPAIR_RECORDS + 11) + .map(|i| format!("layout · container-geometry · Card {i} [n{i}] · gap 24 → 16")) + .collect(); + let total = records.len(); + let quality = QualitySummary { + checks: vec!["layout".into()], + repairs: vec![("layout".into(), total)], + records, + notes: Vec::new(), + }; + + let line = quality_credential_line_with_records(&quality, None).expect("credential"); + + assert_eq!( + line.matches("· Card ").count(), + MAX_INLINE_REPAIR_RECORDS, + "the inline list stops at the cap" + ); + assert!( + line.contains("… and 11 more repair(s) — full list in the log"), + "the withheld count must be stated, never silently dropped: {line}" + ); + assert!( + line.contains(&format!("{total} auto-repair(s) applied")), + "the headline still counts every repair: {line}" + ); +} + +#[test] +fn record_lines_ride_the_wire_conversion_alongside_the_counts() { + let mut repairs = RepairSummary::default(); + repairs.record_all( + CheckCategory::Layout, + vec![op_orchestrator::RepairRecord { + pass: "table-gap".into(), + category: CheckCategory::Layout, + node_id: "n42".into(), + node_name: Some("Pricing Row".into()), + detail: "gap 0 → 16".into(), + }], + ); + + let wire = quality_summary_from_repairs(&repairs); + + assert_eq!(wire.repairs, vec![("layout".to_string(), 1)]); + assert_eq!( + wire.records, + vec!["layout · table-gap · Pricing Row [n42] · gap 0 → 16".to_string()], + "the count and the line it stands for must cross the wire together" + ); +} + +const SKIP_NOTE: &str = "intent-tier passes skipped (template provenance: slide-deck via \ + namespaced-variables) — authored spacing, surfaces and palette kept \ + as designed; contract-tier checks still ran"; + +#[test] +fn a_skipped_tier_is_stated_before_the_repairs_that_did_run() { + // Order is the point. "layout 1" under a skipped intent tier means "one + // contract-tier fix", not "the layout was reviewed and needed one fix" — + // a reader who stops after the first detail line must already know that. + let quality = QualitySummary { + checks: vec!["layout".into()], + repairs: vec![("layout".into(), 1)], + records: vec!["layout · table-gap · Pricing Row [n42] · gap 0 → 16".into()], + notes: vec![SKIP_NOTE.to_string()], + }; + + let line = quality_credential_line_with_records(&quality, Some(0)).expect("credential"); + + let note_at = line + .find("intent-tier passes skipped") + .expect("note rendered"); + let record_at = line.find("table-gap").expect("record rendered"); + assert!( + note_at < record_at, + "the skipped-tier note must precede the repair list: {line}" + ); +} + +#[test] +fn a_note_survives_a_run_that_repaired_nothing() { + // The case the tiering exists to produce: an authored template needs no + // repairs, so the record list is empty. Gating the note on repairs would + // hide the decision exactly where it is the ONLY thing worth reporting. + let quality = QualitySummary { + checks: vec!["layout".into(), "structure".into()], + repairs: Vec::new(), + records: Vec::new(), + notes: vec![SKIP_NOTE.to_string()], + }; + + let line = quality_credential_line_with_records(&quality, Some(0)).expect("credential"); + + assert!(line.contains("nothing needed fixing")); + assert!( + line.contains("\n ▸ intent-tier passes skipped"), + "the note must still be reported: {line}" + ); +} + +#[test] +fn a_note_is_never_counted_as_a_repair() { + let quality = QualitySummary { + checks: vec!["layout".into()], + repairs: Vec::new(), + records: Vec::new(), + notes: vec![SKIP_NOTE.to_string()], + }; + + assert_eq!( + quality.total_repairs(), + 0, + "a statement about the run is not an edit to the document" + ); + assert_eq!(quality_note_lines(&quality).len(), 1); +} + +#[test] +fn notes_ride_the_wire_conversion_beside_the_records() { + let mut repairs = RepairSummary::default(); + repairs.record(CheckCategory::Layout, 0); + repairs.note(SKIP_NOTE); + + let wire = quality_summary_from_repairs(&repairs); + + assert_eq!(wire.notes, vec![SKIP_NOTE.to_string()]); + assert!( + wire.records.is_empty(), + "notes must not leak into the itemized repair list" + ); +} diff --git a/crates/op-orchestrator/src/cleanup_repair_summary_tests.rs b/crates/op-orchestrator/src/cleanup_repair_summary_tests.rs index c87387d40..995f55f5a 100644 --- a/crates/op-orchestrator/src/cleanup_repair_summary_tests.rs +++ b/crates/op-orchestrator/src/cleanup_repair_summary_tests.rs @@ -254,3 +254,168 @@ fn the_tally_never_exceeds_the_edits_the_sink_actually_took() { sink.applied.len() ); } + +#[test] +fn the_counts_are_the_itemized_records_grouped_and_nothing_else() { + // The one invariant that keeps the credential honest end to end: the + // number the user is shown and the list they expand are the SAME data. + // Maintaining them separately is how a "41 repairs" headline ends up over + // 39 lines with nobody able to say which two are missing. + let (_sink, summary) = run_with_summary(json!({ + "type": "frame", + "id": "root", + "name": "Mobile Root", + "width": 390, + "height": 844, + "children": [ + { "type": "text", "id": "title", "role": "heading", + "content": "Popular Restaurants", "width": 320, "height": 40, + "fontSize": 30, "fontWeight": 800 }, + { "type": "text", "id": "subtitle", "role": "body-text", + "content": "Fresh Brooklyn favorites, delivered fast.", + "width": 320, "height": 22, "fontSize": 16, "fontWeight": 800 }, + { "type": "text", "id": "placeholder", "name": "Placeholder", + "content": "Search restaurants or dishes", "width": 280, + "height": 24, "fontSize": 17, "fontWeight": 800 }, + { "type": "text", "id": "metadata", "role": "caption", + "content": "20-30 min", "width": 100, "height": 18, + "fontSize": 14, "fontWeight": 800 } + ] + })); + + assert_eq!( + summary.total_repairs(), + summary.records().len(), + "the headline total is the record list's length, not a second tally" + ); + for category in CheckCategory::ALL { + assert_eq!( + summary.repairs_for(category), + summary + .records() + .iter() + .filter(|record| record.category == category) + .count(), + "the per-category count for {category:?} must be its records grouped" + ); + } + let repaired_total: usize = summary.repaired().iter().map(|(_, count)| count).sum(); + assert_eq!( + repaired_total, + summary.records().len(), + "the breakdown must account for every record, with none double-counted" + ); +} + +#[test] +fn every_record_names_the_pass_the_node_and_the_change() { + // "41 auto-repair(s) applied" with no way to see WHAT moved was the whole + // complaint. Each record must carry an attributed pass, a real target, + // and a field-level description — an empty one is a repair nobody can + // audit. + let (_sink, summary) = run_with_summary(json!({ + "type": "frame", + "id": "root", + "name": "Mobile Root", + "width": 390, + "height": 844, + "children": [ + { "type": "text", "id": "title", "role": "heading", + "content": "Popular Restaurants", "width": 320, "height": 40, + "fontSize": 30, "fontWeight": 800 }, + { "type": "text", "id": "subtitle", "role": "body-text", + "content": "Fresh Brooklyn favorites, delivered fast.", + "width": 320, "height": 22, "fontSize": 16, "fontWeight": 800 }, + { "type": "text", "id": "placeholder", "name": "Placeholder", + "content": "Search restaurants or dishes", "width": 280, + "height": 24, "fontSize": 17, "fontWeight": 800 }, + { "type": "text", "id": "metadata", "role": "caption", + "content": "20-30 min", "width": 100, "height": 18, + "fontSize": 14, "fontWeight": 800 } + ] + })); + + assert!( + !summary.records().is_empty(), + "precondition: this fixture must provoke repairs" + ); + for record in summary.records() { + assert!(!record.pass.is_empty(), "unattributed record: {record:?}"); + assert_ne!( + record.pass, "unattributed", + "a real cleanup run must never fall back to the count-only shim: {record:?}" + ); + assert!(!record.detail.is_empty(), "undescribed record: {record:?}"); + assert!( + record.line().contains(&record.detail), + "the rendered line must carry the change it describes: {record:?}" + ); + } + let hierarchy: Vec = summary + .records() + .iter() + .filter(|record| record.category == CheckCategory::Hierarchy) + .map(|record| record.detail.clone()) + .collect(); + assert!( + hierarchy + .iter() + .any(|detail| detail == "fontWeight 800 → 400"), + "the hierarchy demotion must be itemized with before → after: {hierarchy:?}" + ); +} + +#[test] +fn the_double_inset_stripper_reports_which_section_lost_which_padding() { + // The incident this whole itemized-record line exists for: a run reported + // "41 auto-repair(s) applied — layout 36" and the user could see their + // section rhythm had been flattened but not by what. 36 of those came from + // `strip_wrapper_double_inset`. Pinned here as the worked example — the + // record must name the SECTION and the padding it lost, because "padding + // dropped on Hero Section" is the sentence that makes the repair + // disputable instead of mysterious. + let (_sink, summary) = run_with_summary(json!({ + "type": "frame", + "id": "root", + "name": "Home Screen", + "width": 390, + "height": 844, + "layout": "vertical", + "gap": 20, + "padding": [24, 20, 24, 20], + "children": [{ + "type": "frame", + "id": "hero", + "name": "Hero Section", + "layout": "vertical", + "gap": 8, + "padding": [24, 20, 24, 20], + "children": [ + { "type": "text", "id": "t1", "role": "heading", "content": "Good morning", + "width": 300, "height": 32, "fontSize": 24, "fontWeight": 700 }, + { "type": "text", "id": "t2", "role": "body-text", "content": "Three tasks today", + "width": 300, "height": 20, "fontSize": 15, "fontWeight": 400 } + ] + }] + })); + + let padding_records: Vec = summary + .records() + .iter() + .filter(|record| record.detail.contains("padding")) + .map(|record| record.line()) + .collect(); + assert!( + padding_records + .iter() + .any(|line| line.contains("Hero Section") + && line.contains("padding [24,20,24,20] → (unset)")), + "the stripped section and the inset it lost must both be named: {padding_records:?}" + ); + assert!( + padding_records + .iter() + .any(|line| line.starts_with("layout · spacing+footer-sink")), + "the repair must be attributed to the pass group that made it: {padding_records:?}" + ); +} diff --git a/crates/op-orchestrator/src/repair_record.rs b/crates/op-orchestrator/src/repair_record.rs new file mode 100644 index 000000000..68f67065a --- /dev/null +++ b/crates/op-orchestrator/src/repair_record.rs @@ -0,0 +1,599 @@ +//! Field-level record of every document edit the deterministic quality +//! passes applied — the itemized half of `crate::repair_summary`'s tally. +//! +//! **Why this exists.** The credential the user sees used to be a bare +//! count ("41 auto-repair(s) applied"). When a pass repaired something the +//! user did NOT want repaired — a pale showcase band restyled, a section's +//! alignment rewritten — there was no way to find out which pass touched +//! which node, so a mis-repair was indistinguishable from a model mistake. +//! One [`RepairRecord`] per accepted edit makes that answerable. +//! +//! **One source of truth.** Records are produced at exactly the same place +//! the counter counts — `repair_summary::RecordingSink::apply` — so counts +//! are `records.len()` grouped by category, never a parallel tally that can +//! drift from the list. +//! +//! **Granularity, honestly stated.** [`describe_command`] reads the target +//! node BEFORE the edit lands, so single-property commands +//! (`SetNodeLayoutProp` / `SetNodeFillHex` / `PatchNodeData` / …) carry a +//! real `before → after`. Whole-subtree swaps (`ReplaceSubtree`, the shape +//! every `apply_root_transform` pass uses) carry a bounded diff of what +//! changed inside the subtree instead — coarser, but still node-and-field +//! level. Anything else degrades to the command's own name rather than +//! inventing a description. + +use jian_ops_schema::node::PenNode; +use op_editor_core::{EditorCommand, EditorState, LayoutPropValue, NodeId, PenNodeExt}; +use serde_json::Value; + +use crate::repair_summary::CheckCategory; + +/// How many changed nodes a subtree-replacement diff names before it +/// summarizes the rest as a count. Keeps one restructure from producing a +/// transcript-length line. +const SUBTREE_DIFF_NODE_LIMIT: usize = 3; +/// How many changed fields are named per node inside that diff. +const SUBTREE_DIFF_FIELD_LIMIT: usize = 3; +/// How an absent value renders on either side of a `before → after`. A word, +/// not a dash: a repair that DROPS a property (`strip_wrapper_double_inset` +/// removes `padding` outright when every side reaches 0) must read as "this +/// property is now unset", which is the disputed fact — a bare dash reads +/// like a placeholder the renderer failed to fill in. +const UNSET: &str = "(unset)"; +/// Longest rendered value before it is elided. A fill body or a styled-text +/// run serializes to hundreds of characters; the point is recognizability. +const VALUE_MAX_CHARS: usize = 48; + +/// Node fields a subtree diff reports. Deliberately the *visible* ones — +/// the properties a user can see go wrong (surface colour, spacing, +/// alignment, sizing, clipping) — so the record answers "what changed on +/// screen", not "what moved in the JSON". +const DIFFED_FIELDS: [&str; 16] = [ + "fills", + "fill", + "backgroundColor", + "stroke", + "padding", + "gap", + "layoutMode", + "justifyContent", + "alignItems", + "clipContent", + "width", + "height", + "opacity", + "cornerRadius", + "fontSize", + "fontWeight", +]; + +/// One accepted document edit a quality pass applied. +/// +/// `pass` is the pass GROUP the edit belongs to, stamped at the checkpoint +/// that closes the group (see `RepairCounter::checkpoint`). Groups that run +/// a single pass name that pass exactly; multi-pass groups name the family. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct RepairRecord { + /// Pass (or pass group) that applied the edit. + pub pass: String, + /// Check family the edit is credited to in the summary. + pub category: CheckCategory, + /// Target node id — empty for document-level edits (variables). + pub node_id: String, + /// Target node's name at edit time, when it had one. + pub node_name: Option, + /// What changed, field level: `gap 24 → 16`. + pub detail: String, +} + +impl RepairRecord { + /// One-line rendering for the transcript and the log: + /// `layout · table-repair · Pricing Row [n42] · gap 0 → 16`. + pub fn line(&self) -> String { + let mut out = String::new(); + out.push_str(self.category.key()); + out.push_str(" · "); + out.push_str(&self.pass); + let who = self.node_label(); + if !who.is_empty() { + out.push_str(" · "); + out.push_str(&who); + } + out.push_str(" · "); + out.push_str(&self.detail); + out + } + + /// `Name [id]`, `[id]`, or empty — whichever the node actually has. + fn node_label(&self) -> String { + let name = self + .node_name + .as_deref() + .map(str::trim) + .filter(|name| !name.is_empty()); + match (name, self.node_id.as_str()) { + (Some(name), "") => name.to_string(), + (Some(name), id) => format!("{name} [{id}]"), + (None, "") => String::new(), + (None, id) => format!("[{id}]"), + } + } +} + +/// A repair captured at apply time, before a checkpoint attributes it to a +/// category and a pass group. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct PendingRepair { + pub(crate) node_id: String, + pub(crate) node_name: Option, + pub(crate) detail: String, +} + +impl PendingRepair { + pub(crate) fn attribute(self, category: CheckCategory, pass: &str) -> RepairRecord { + RepairRecord { + pass: pass.to_string(), + category, + node_id: self.node_id, + node_name: self.node_name, + detail: self.detail, + } + } +} + +/// Describe `cmd` against the document as it stands BEFORE the command is +/// applied, so `before → after` reflects the real prior value. +/// +/// Never fails: an unrecognized command still yields a record naming the +/// operation, because a silent gap in the list would break the "count == +/// records" contract the summary depends on. +pub(crate) fn describe_command(state: &EditorState, cmd: &EditorCommand) -> PendingRepair { + match cmd { + EditorCommand::PatchNodeData { + node_id, + patch_json, + .. + } => { + let detail = describe_patch(node_value(state, node_id).as_ref(), patch_json); + pending(state, node_id, detail) + } + EditorCommand::SetNodeLayoutProp { + node_id, + property, + value, + } => { + let before = node_field(state, node_id, property); + pending( + state, + node_id, + format!( + "{property} {} → {}", + render_opt(before.as_ref()), + render_layout_value(value) + ), + ) + } + EditorCommand::SetNodeFillHex { node_id, hex } => { + let before = find(state, node_id).and_then(op_editor_core::first_solid_fill_hex); + pending( + state, + node_id, + format!("fill {} → {hex}", before.unwrap_or(UNSET)), + ) + } + EditorCommand::SetNodeStrokeHex { node_id, hex } => { + pending(state, node_id, format!("stroke → {hex}")) + } + EditorCommand::SetNodeStrokeWidth { node_id, width } => { + let before = find(state, node_id).and_then(op_editor_core::fills::node_stroke_width); + pending( + state, + node_id, + format!("strokeWidth {} → {width}", render_num_opt(before)), + ) + } + EditorCommand::SetNodeStrokeSideWidth { + node_id, + side, + width, + } => pending(state, node_id, format!("strokeWidth {side:?} → {width}")), + EditorCommand::SetNodeFontSize { node_id, font_size } => { + let before = node_field(state, node_id, "fontSize"); + pending( + state, + node_id, + format!("fontSize {} → {font_size}", render_opt(before.as_ref())), + ) + } + EditorCommand::SetNodeFontWeight { + node_id, + font_weight, + } => { + let before = node_field(state, node_id, "fontWeight"); + pending( + state, + node_id, + format!("fontWeight {} → {font_weight}", render_opt(before.as_ref())), + ) + } + EditorCommand::SetNodeCornerRadius { node_id, radius } => { + let before = node_field(state, node_id, "cornerRadius"); + pending( + state, + node_id, + format!("cornerRadius {} → {radius}", render_opt(before.as_ref())), + ) + } + EditorCommand::SetNodeRotation { node_id, degrees } => { + pending(state, node_id, format!("rotation → {degrees}")) + } + EditorCommand::SetNodeText { node_id, text } => { + pending(state, node_id, format!("text → {}", elide(text))) + } + EditorCommand::SetNodeName { node_id, name } => { + pending(state, node_id, format!("name → {}", elide(name))) + } + EditorCommand::SetNodeFlag { + node_id, + flag, + value, + } => pending(state, node_id, format!("{flag:?} → {value}")), + EditorCommand::UpdateNode { + node_id, + x, + y, + width, + height, + name, + fill_hex, + .. + } => { + let before = node_value(state, node_id); + let mut parts = Vec::new(); + push_num_change(&mut parts, before.as_ref(), "x", x.map(f64::from)); + push_num_change(&mut parts, before.as_ref(), "y", y.map(f64::from)); + push_num_change(&mut parts, before.as_ref(), "width", width.map(f64::from)); + push_num_change(&mut parts, before.as_ref(), "height", height.map(f64::from)); + if let Some(name) = name { + parts.push(format!("name → {}", elide(name))); + } + if let Some(hex) = fill_hex { + let prior = find(state, node_id).and_then(op_editor_core::first_solid_fill_hex); + parts.push(format!("fill {} → {hex}", prior.unwrap_or(UNSET))); + } + let detail = if parts.is_empty() { + "updated".to_string() + } else { + parts.join(", ") + }; + pending(state, node_id, detail) + } + EditorCommand::DeleteNode { node_id, .. } => { + let kind = find(state, node_id) + .map(node_kind_label) + .unwrap_or_else(|| "node".to_string()); + let descendants = find(state, node_id) + .map(crate::cleanup::count_descendants) + .unwrap_or(0); + pending( + state, + node_id, + format!("removed {kind} (+{descendants} descendant(s))"), + ) + } + EditorCommand::MoveNode { + node_id, + target_parent, + index, + .. + } => { + let parent = node_label_for(state, target_parent); + let slot = index.map(|i| format!(" at index {i}")).unwrap_or_default(); + pending(state, node_id, format!("moved under {parent}{slot}")) + } + EditorCommand::ReplaceSubtree { node_id, node, .. } => { + let detail = match find(state, node_id) { + Some(before) => describe_subtree_replacement(before, node), + None => "restructured".to_string(), + }; + pending(state, node_id, detail) + } + EditorCommand::InsertSubtree { + nodes, parent_id, .. + } + | EditorCommand::InsertAuthoredSubtree { + nodes, parent_id, .. + } + | EditorCommand::InsertAuthoredSubtreePreservingRoots { + nodes, parent_id, .. + } => pending( + state, + parent_id, + format!("inserted {} subtree(s)", nodes.len()), + ), + EditorCommand::SetVariableColor { name, hex } => { + doc_level(format!("variable {name} → {hex}")) + } + EditorCommand::Batch { commands } => { + doc_level(format!("batch of {} edit(s)", commands.len())) + } + other => doc_level(command_label(other).to_string()), + } +} + +/// Fallback label for a command this module does not describe field-by-field. +/// A short static name, never a `Debug` dump — `ReplaceSubtree`'s payload +/// alone would be a whole document. +fn command_label(cmd: &EditorCommand) -> &'static str { + match cmd { + EditorCommand::SetVariableScalar { .. } => "variable value set", + EditorCommand::SetVariables { .. } | EditorCommand::UpsertVariables { .. } => { + "variables merged" + } + EditorCommand::CreateVariable { .. } => "variable created", + EditorCommand::DeleteVariable { .. } => "variable deleted", + EditorCommand::RenameVariable { .. } => "variable renamed", + EditorCommand::SetThemes { .. } | EditorCommand::MergeThemePreset { .. } => "theme merged", + EditorCommand::MergeAppState { .. } => "app state merged", + EditorCommand::ReplaceFontFamily { .. } => "font family replaced", + EditorCommand::ReplaceAllMatchingProperties { .. } => "matching properties replaced", + EditorCommand::SetEllipseArc { .. } => "arc geometry set", + EditorCommand::AddNodeEffect { .. } => "effect added", + EditorCommand::RemoveNodeEffect { .. } => "effect removed", + EditorCommand::SetEffectParam { .. } | EditorCommand::SetEffectColor { .. } => { + "effect updated" + } + _ => "edit applied", + } +} + +/// Compare a `PatchNodeData` payload against the node's current fields. +fn describe_patch(before: Option<&Value>, patch_json: &str) -> String { + let Ok(patch) = serde_json::from_str::(patch_json) else { + return format!("patched {}", elide(patch_json)); + }; + let Some(fields) = patch.as_object() else { + return format!("patched {}", elide(patch_json)); + }; + let mut parts = Vec::new(); + for (key, after) in fields { + let prior = before.and_then(|value| value.get(key)); + if prior == Some(after) { + continue; + } + parts.push(format!( + "{key} {} → {}", + render_opt(prior), + render_value(after) + )); + } + if parts.is_empty() { + return "patched (no field change)".to_string(); + } + parts.join(", ") +} + +/// Bounded node-and-field diff of a whole-subtree swap. +/// +/// Matching is by node id: `ReplaceSubtree` rebuilds the root but keeps the +/// ids of the nodes it carries over, so ids present on both sides are the +/// nodes a pass EDITED (as opposed to added or dropped) — exactly the set a +/// user asking "what did the polish stage do to my section" wants named. +fn describe_subtree_replacement(before: &PenNode, after: &PenNode) -> String { + let old_nodes = flatten(before); + let new_nodes = flatten(after); + let mut changed: Vec = Vec::new(); + let mut changed_total = 0usize; + for (id, new_value) in &new_nodes { + let Some(old_value) = old_nodes.get(id) else { + continue; + }; + let fields = changed_fields(old_value, new_value); + if fields.is_empty() { + continue; + } + changed_total += 1; + if changed.len() < SUBTREE_DIFF_NODE_LIMIT { + let label = new_value + .get("name") + .and_then(Value::as_str) + .map(str::trim) + .filter(|name| !name.is_empty()) + .map(|name| format!("{name} [{id}]")) + .unwrap_or_else(|| format!("[{id}]")); + changed.push(format!("{label}: {}", fields.join(", "))); + } + } + let added = new_nodes + .keys() + .filter(|id| !old_nodes.contains_key(*id)) + .count(); + let removed = old_nodes + .keys() + .filter(|id| !new_nodes.contains_key(*id)) + .count(); + + let mut out = String::from("restructured"); + if !changed.is_empty() { + out.push_str(&format!(" — {}", changed.join("; "))); + if changed_total > changed.len() { + out.push_str(&format!( + " (+{} more node(s))", + changed_total - changed.len() + )); + } + } else if changed_total > 0 { + out.push_str(&format!(" — {changed_total} node(s) changed")); + } + if added > 0 || removed > 0 { + out.push_str(&format!(" [+{added}/-{removed} node(s)]")); + } + out +} + +/// Changed [`DIFFED_FIELDS`] between two serialized nodes, capped. +fn changed_fields(before: &Value, after: &Value) -> Vec { + let mut out = Vec::new(); + for key in DIFFED_FIELDS { + let old = before.get(key); + let new = after.get(key); + if old == new { + continue; + } + out.push(format!("{key} {} → {}", render_opt(old), render_opt(new))); + if out.len() == SUBTREE_DIFF_FIELD_LIMIT { + break; + } + } + out +} + +/// Serialize a subtree into `id -> node object` with `children` stripped, so +/// a field comparison sees only the node's own properties. +fn flatten(node: &PenNode) -> std::collections::BTreeMap { + let mut out = std::collections::BTreeMap::new(); + if let Ok(value) = serde_json::to_value(node) { + flatten_value(&value, &mut out); + } + out +} + +fn flatten_value(value: &Value, out: &mut std::collections::BTreeMap) { + let Some(object) = value.as_object() else { + return; + }; + if let Some(children) = object.get("children").and_then(Value::as_array) { + for child in children { + flatten_value(child, out); + } + } + let Some(id) = object.get("id").and_then(Value::as_str) else { + return; + }; + let mut shallow = object.clone(); + shallow.remove("children"); + out.insert(id.to_string(), Value::Object(shallow)); +} + +fn pending(state: &EditorState, node_id: &NodeId, detail: String) -> PendingRepair { + PendingRepair { + node_id: node_id.as_str().to_string(), + node_name: find(state, node_id).and_then(|node| node.base().name.clone()), + detail, + } +} + +fn doc_level(detail: String) -> PendingRepair { + PendingRepair { + node_id: String::new(), + node_name: None, + detail, + } +} + +fn find<'a>(state: &'a EditorState, node_id: &NodeId) -> Option<&'a PenNode> { + op_editor_core::walkers::find_node(state.active_children(), node_id) +} + +fn node_value(state: &EditorState, node_id: &NodeId) -> Option { + find(state, node_id).and_then(|node| serde_json::to_value(node).ok()) +} + +fn node_field(state: &EditorState, node_id: &NodeId, key: &str) -> Option { + node_value(state, node_id).and_then(|value| value.get(key).cloned()) +} + +fn node_kind_label(node: &PenNode) -> String { + serde_json::to_value(node) + .ok() + .and_then(|value| { + value + .get("type") + .or_else(|| value.get("kind")) + .and_then(Value::as_str) + .map(str::to_string) + }) + .unwrap_or_else(|| "node".to_string()) +} + +fn node_label_for(state: &EditorState, node_id: &NodeId) -> String { + if node_id.as_str().is_empty() { + return "page root".to_string(); + } + match find(state, node_id).and_then(|node| node.base().name.clone()) { + Some(name) if !name.trim().is_empty() => format!("{name} [{}]", node_id.as_str()), + _ => format!("[{}]", node_id.as_str()), + } +} + +fn push_num_change(parts: &mut Vec, before: Option<&Value>, key: &str, after: Option) { + let Some(after) = after else { + return; + }; + let prior = before.and_then(|value| value.get(key)); + parts.push(format!("{key} {} → {}", render_opt(prior), trim_num(after))); +} + +fn render_layout_value(value: &LayoutPropValue) -> String { + match value { + LayoutPropValue::Number(n) => trim_num(*n), + LayoutPropValue::Keyword(k) => k.clone(), + LayoutPropValue::Bool(b) => b.to_string(), + LayoutPropValue::NumberArray(values) => format!( + "[{}]", + values + .iter() + .map(|n| trim_num(*n)) + .collect::>() + .join(",") + ), + } +} + +fn render_opt(value: Option<&Value>) -> String { + value.map(render_value).unwrap_or_else(|| UNSET.to_string()) +} + +fn render_num_opt(value: Option) -> String { + value.map(trim_num).unwrap_or_else(|| UNSET.to_string()) +} + +/// Compact, human-first rendering: strings unquoted, numbers without a +/// trailing `.0`, containers elided once they stop being recognizable. +fn render_value(value: &Value) -> String { + match value { + Value::Null => UNSET.to_string(), + Value::String(s) => elide(s), + Value::Number(n) => n.as_f64().map(trim_num).unwrap_or_else(|| n.to_string()), + Value::Bool(b) => b.to_string(), + // Padding / radii arrive as number arrays; rendering them through + // `to_string` would print `[32.0,32.0,32.0,32.0]` for what the user + // authored as `[32,32,32,32]`. + Value::Array(items) => elide(&format!( + "[{}]", + items.iter().map(render_value).collect::>().join(",") + )), + other => elide(&other.to_string()), + } +} + +fn trim_num(value: f64) -> String { + if value.fract() == 0.0 && value.abs() < 1e15 { + format!("{}", value as i64) + } else { + format!("{value:.2}") + } +} + +fn elide(text: &str) -> String { + let cleaned = text.replace('\n', " "); + if cleaned.chars().count() <= VALUE_MAX_CHARS { + return cleaned; + } + let head: String = cleaned.chars().take(VALUE_MAX_CHARS).collect(); + format!("{head}…") +} + +#[cfg(test)] +#[path = "repair_record_tests.rs"] +mod tests; diff --git a/crates/op-orchestrator/src/repair_record_tests.rs b/crates/op-orchestrator/src/repair_record_tests.rs new file mode 100644 index 000000000..3a25213a7 --- /dev/null +++ b/crates/op-orchestrator/src/repair_record_tests.rs @@ -0,0 +1,230 @@ +//! Coverage for the itemized repair record — specifically the part that is +//! easy to get wrong and impossible to notice: the `before` half of +//! `before → after`. It only exists in the document for the instant before +//! the command is applied, so every assertion here builds a real node, reads +//! the description against the pre-edit state, and checks the OLD value is +//! in the line. A record that only reported the new value would look right +//! in the UI and be useless for the question it exists to answer. + +use super::*; +use crate::test_support::VecDocSink; +use op_editor_core::{EditorCommand, LayoutPropValue, NodeId, PenNodeExt}; +use serde_json::json; + +fn sink_with(tree: serde_json::Value) -> VecDocSink { + let mut sink = VecDocSink::new(); + let node: PenNode = serde_json::from_value(tree).expect("fixture json"); + sink.state.apply(EditorCommand::InsertSubtree { + nodes: vec![node], + parent_id: NodeId::NONE, + page_id: None, + }); + sink +} + +fn showcase_tree() -> serde_json::Value { + json!({ + "type": "frame", + "id": "root", + "name": "Landing Page", + "width": 1200, + "height": 800, + "layout": "vertical", + "gap": 24, + "children": [{ + "type": "frame", + "id": "showcase", + "name": "Showcase Band", + "layout": "horizontal", + "gap": 24, + "padding": [32, 32, 32, 32], + "justifyContent": "center", + "fill": [{ "type": "solid", "color": "#F8FAFC" }], + "children": [] + }] + }) +} + +fn node_id_of(sink: &VecDocSink, name: &str) -> NodeId { + fn walk(nodes: &[PenNode], name: &str) -> Option { + for node in nodes { + if node.base().name.as_deref() == Some(name) { + return Some(node.id_str().to_string()); + } + if let Some(children) = node.children() { + if let Some(hit) = walk(children, name) { + return Some(hit); + } + } + } + None + } + NodeId::new(walk(sink.state.active_children(), name).expect("fixture node")) +} + +#[test] +fn layout_prop_record_carries_the_previous_value() { + let sink = sink_with(showcase_tree()); + let node_id = node_id_of(&sink, "Showcase Band"); + + let described = describe_command( + &sink.state, + &EditorCommand::SetNodeLayoutProp { + node_id: node_id.clone(), + property: "gap".into(), + value: LayoutPropValue::Number(16.0), + }, + ); + + assert_eq!(described.node_name.as_deref(), Some("Showcase Band")); + assert_eq!( + described.detail, "gap 24 → 16", + "the record must name the value the pass overwrote, not only the new one" + ); +} + +#[test] +fn fill_record_carries_the_previous_colour() { + // The user-facing question this exists for: "the polish stage changed my + // pale showcase band — to what, from what?" + let sink = sink_with(showcase_tree()); + let node_id = node_id_of(&sink, "Showcase Band"); + + let described = describe_command( + &sink.state, + &EditorCommand::SetNodeFillHex { + node_id, + hex: "#FFFFFF".into(), + }, + ); + + assert_eq!(described.detail, "fill #F8FAFC → #FFFFFF"); +} + +#[test] +fn patch_record_names_every_changed_key_and_skips_unchanged_ones() { + let sink = sink_with(showcase_tree()); + let node_id = node_id_of(&sink, "Showcase Band"); + + let described = describe_command( + &sink.state, + &EditorCommand::PatchNodeData { + node_id, + // `justifyContent` is already `center`: an unchanged key must not + // be reported as a repair detail, or the list accuses a pass of + // an edit it did not make. + patch_json: r#"{"justifyContent":"center","alignItems":"start"}"#.into(), + page_id: None, + }, + ); + + assert_eq!(described.detail, "alignItems (unset) → start"); +} + +#[test] +fn subtree_replacement_record_diffs_the_nodes_that_changed() { + let sink = sink_with(showcase_tree()); + let root_id = node_id_of(&sink, "Landing Page"); + let before = op_editor_core::walkers::find_node(sink.state.active_children(), &root_id) + .expect("root") + .clone(); + + // What every `apply_root_transform` pass does: hand back a rebuilt root. + let mut after_json = serde_json::to_value(&before).expect("serialize"); + after_json["children"][0]["padding"] = json!([16, 16, 16, 16]); + after_json["children"][0]["justifyContent"] = json!("start"); + let after: PenNode = serde_json::from_value(after_json).expect("rebuilt root"); + + let described = describe_command( + &sink.state, + &EditorCommand::ReplaceSubtree { + node_id: root_id, + node: Box::new(after), + drop_children: true, + page_id: None, + }, + ); + + assert!( + described.detail.contains("Showcase Band"), + "the diff must name the node that changed: {}", + described.detail + ); + assert!( + described + .detail + .contains("padding [32,32,32,32] → [16,16,16,16]"), + "the diff must carry before → after for the changed field: {}", + described.detail + ); + assert!( + described.detail.contains("justifyContent center → start"), + "alignment changes are exactly what a user disputes: {}", + described.detail + ); +} + +#[test] +fn deletion_record_names_what_was_removed_before_it_is_gone() { + let sink = sink_with(showcase_tree()); + let node_id = node_id_of(&sink, "Showcase Band"); + + let described = describe_command( + &sink.state, + &EditorCommand::DeleteNode { + node_id, + page_id: None, + }, + ); + + assert_eq!(described.node_name.as_deref(), Some("Showcase Band")); + assert!( + described.detail.starts_with("removed"), + "unexpected detail: {}", + described.detail + ); +} + +#[test] +fn unknown_commands_still_produce_a_record_rather_than_a_gap() { + // The count is `records.len()`, so a command with no bespoke description + // must still yield one — a silent skip would make the credential's number + // disagree with its own list. + let sink = sink_with(showcase_tree()); + + let described = describe_command(&sink.state, &EditorCommand::PromoteLegacyWidgets); + + assert!(!described.detail.is_empty()); +} + +#[test] +fn record_line_reads_as_pass_node_and_change() { + let record = RepairRecord { + pass: "table-gap".into(), + category: CheckCategory::Layout, + node_id: "n42".into(), + node_name: Some("Pricing Row".into()), + detail: "gap 0 → 16".into(), + }; + + assert_eq!( + record.line(), + "layout · table-gap · Pricing Row [n42] · gap 0 → 16" + ); +} + +#[test] +fn record_line_degrades_without_a_node_name() { + let record = RepairRecord { + pass: "theme-variable-polarity".into(), + category: CheckCategory::Palette, + node_id: String::new(), + node_name: None, + detail: "variable surface → #FFFFFF".into(), + }; + + assert_eq!( + record.line(), + "palette · theme-variable-polarity · variable surface → #FFFFFF" + ); +} diff --git a/crates/op-orchestrator/src/repair_summary.rs b/crates/op-orchestrator/src/repair_summary.rs index 4f1701836..2242dfe31 100644 --- a/crates/op-orchestrator/src/repair_summary.rs +++ b/crates/op-orchestrator/src/repair_summary.rs @@ -12,13 +12,20 @@ //! meaning — see `op_host_services::quality_credential`. //! //! **How the count is taken.** [`RepairCounter::wrap`] returns a -//! [`CountingSink`] that delegates every method to the real sink and bumps a -//! shared atomic on each accepted apply. The cleanup driver shadows its own -//! `sink` binding with that wrapper once, at the top, then calls +//! [`CountingSink`] that delegates every method to the real sink and records +//! each accepted apply. The cleanup driver shadows its own `sink` binding +//! with that wrapper once, at the top, then calls //! [`RepairCounter::checkpoint`] between contiguous groups of passes to -//! attribute the edits since the previous checkpoint to a [`CheckCategory`]. -//! Pass bodies are untouched — this is deliberately a measurement layer, not -//! a refactor of ~40 repair passes. +//! attribute the edits since the previous checkpoint to a [`CheckCategory`] +//! and a pass-group name. Pass bodies are untouched — this is deliberately a +//! measurement layer, not a refactor of ~40 repair passes. +//! +//! **One list, not two accounts.** Every accepted apply produces exactly one +//! [`RepairRecord`] (`crate::repair_record::describe_command` reads the +//! target node BEFORE the edit so the record carries `before → after`), and +//! the per-category counts this summary reports are that list grouped by +//! category. There is no separately-maintained tally that could drift from +//! the itemized detail the user is shown. //! //! **Honesty rules baked in.** A category is recorded as *checked* by the //! checkpoint itself, whether or not it repaired anything, because reaching @@ -34,13 +41,13 @@ //! (`repair_overbold_text_hierarchy` for hierarchy, the geometry loop for //! overflow, and so on). -use std::collections::BTreeMap; -use std::sync::atomic::{AtomicUsize, Ordering}; -use std::sync::Arc; +use std::collections::{BTreeMap, BTreeSet}; +use std::sync::{Arc, Mutex}; use jian_ops_schema::node::PenNode; use op_editor_core::{EditorCommand, EditorState, NodeId}; +use crate::repair_record::{describe_command, PendingRepair, RepairRecord}; use crate::types::DocSink; /// A family of quality checks, as named to the user. Declaration order is @@ -95,68 +102,124 @@ impl CheckCategory { } } -/// What the quality passes checked and how much they repaired, per category. +/// What the quality passes checked and every edit they applied. +/// +/// The itemized [`records`](Self::records) list is the single source: the +/// per-category counts every accessor below reports are derived from it by +/// grouping, so the number in the credential and the lines under it can +/// never disagree. #[derive(Debug, Clone, Default, PartialEq, Eq)] pub struct RepairSummary { - /// Category -> repairs applied. A key present with value 0 means the - /// category was checked and found nothing to fix. - counts: BTreeMap, + /// Categories whose passes reached a checkpoint — including those that + /// found nothing to fix, which is what lets the credential say "checked" + /// truthfully about a clean family. + checked: BTreeSet, + /// Every applied edit, in the order the passes applied it. + records: Vec, + /// Statements about the run that are NOT edits — today, the one line + /// saying a whole tier of passes was deliberately not run (see + /// `crate::repair_tier`). Kept apart from `records` on purpose: a record + /// means "one accepted apply", and a note that counted as one would make + /// the credential claim a repair that never happened. + notes: Vec, } impl RepairSummary { - /// Record that `category`'s passes ran and applied `repairs` edits. - /// Calling with `repairs == 0` still marks the category as checked. + /// Mark `category` as checked and file `records` under it. + pub fn record_all(&mut self, category: CheckCategory, records: Vec) { + self.checked.insert(category); + self.records.extend(records); + } + + /// Count-only ingestion for callers that have a number but no itemized + /// detail — a wire round-trip from an older host, or a test building a + /// summary by hand. Synthesizes placeholder records so counts stay + /// derived from the one list; the placeholders say plainly that no + /// detail was reported rather than pretending to describe an edit. pub fn record(&mut self, category: CheckCategory, repairs: usize) { - *self.counts.entry(category).or_insert(0) += repairs; + let records = (0..repairs) + .map(|_| RepairRecord { + pass: "unattributed".to_string(), + category, + node_id: String::new(), + node_name: None, + detail: "no detail reported".to_string(), + }) + .collect(); + self.record_all(category, records); + } + + /// Add a statement about the run that is not an edit. Repeats are dropped + /// so a driver that reaches the same gate on several roots still reports + /// the decision once. + pub fn note(&mut self, note: impl Into) { + let note = note.into(); + if !self.notes.contains(¬e) { + self.notes.push(note); + } + } + + /// Statements about the run that are not edits, in the order recorded. + pub fn notes(&self) -> &[String] { + &self.notes } /// Fold another summary in — used when one run drives cleanup over /// several root batches (the orchestrator's append path). pub fn merge(&mut self, other: &RepairSummary) { - for (category, count) in &other.counts { - self.record(*category, *count); + self.checked.extend(other.checked.iter().copied()); + self.records.extend(other.records.iter().cloned()); + for note in &other.notes { + self.note(note.clone()); } } /// True when no category was ever checked: render NO credential rather /// than claiming a clean bill of health nobody verified. pub fn is_empty(&self) -> bool { - self.counts.is_empty() + self.checked.is_empty() } /// Categories whose passes actually ran, in display order. pub fn checked(&self) -> Vec { - self.counts.keys().copied().collect() + self.checked.iter().copied().collect() + } + + /// Every applied edit, in application order. + pub fn records(&self) -> &[RepairRecord] { + &self.records } /// Repairs applied under `category` (0 when checked-and-clean or absent). pub fn repairs_for(&self, category: CheckCategory) -> usize { - self.counts.get(&category).copied().unwrap_or(0) + self.records + .iter() + .filter(|record| record.category == category) + .count() } /// Categories that repaired something, paired with their counts, in /// display order. Clean categories are omitted. pub fn repaired(&self) -> Vec<(CheckCategory, usize)> { - self.counts - .iter() - .filter(|(_, count)| **count > 0) - .map(|(category, count)| (*category, *count)) - .collect() + let mut counts: BTreeMap = BTreeMap::new(); + for record in &self.records { + *counts.entry(record.category).or_insert(0) += 1; + } + counts.into_iter().collect() } /// Total edits applied across every category. pub fn total_repairs(&self) -> usize { - self.counts.values().sum() + self.records.len() } } -/// Shared edit counter behind a [`CountingSink`]. Held by the cleanup driver -/// alongside — never inside — the wrapped sink, so a checkpoint can read the -/// tally while the sink is still mutably borrowed. +/// Shared edit buffer behind a [`CountingSink`]. Held by the cleanup driver +/// alongside — never inside — the wrapped sink, so a checkpoint can drain the +/// buffer while the sink is still mutably borrowed. #[derive(Debug)] pub struct RepairCounter { - applied: Arc, - checkpointed: usize, + pending: Arc>>, } impl Default for RepairCounter { @@ -168,8 +231,7 @@ impl Default for RepairCounter { impl RepairCounter { pub fn new() -> Self { Self { - applied: Arc::new(AtomicUsize::new(0)), - checkpointed: 0, + pending: Arc::new(Mutex::new(Vec::new())), } } @@ -177,27 +239,63 @@ impl RepairCounter { pub fn wrap<'a>(&self, inner: &'a mut dyn DocSink) -> CountingSink<'a> { CountingSink { inner, - applied: self.applied.clone(), + pending: self.pending.clone(), } } - /// Attribute every edit since the previous checkpoint to `category`, and - /// mark `category` as checked even when the delta is 0. - pub fn checkpoint(&mut self, summary: &mut RepairSummary, category: CheckCategory) { - let total = self.applied.load(Ordering::SeqCst); - let delta = total.saturating_sub(self.checkpointed); - self.checkpointed = total; - summary.record(category, delta); + /// Attribute every edit since the previous checkpoint to `category` and + /// the pass group named by `pass`, and mark `category` as checked even + /// when nothing was applied. + /// + /// `pass` names the group of passes that ran since the last checkpoint — + /// exact for a single-pass group, a family name for a multi-pass one. + /// Each attributed record is also logged at INFO so a user who pastes + /// their log can be told which pass touched which node. + pub fn checkpoint(&mut self, summary: &mut RepairSummary, category: CheckCategory, pass: &str) { + let drained: Vec = match self.pending.lock() { + Ok(mut pending) => std::mem::take(&mut *pending), + // A poisoned buffer means a pass panicked mid-apply; the tally is + // then unknowable, so record a checked-but-empty category rather + // than a number nobody can stand behind. + Err(_) => Vec::new(), + }; + let records: Vec = drained + .into_iter() + .map(|repair| repair.attribute(category, pass)) + .collect(); + for record in &records { + tracing::info!( + pass = %record.pass, + category = %record.category.key(), + node = %record.node_id, + node_name = %record.node_name.as_deref().unwrap_or(""), + detail = %record.detail, + "quality repair applied" + ); + } + summary.record_all(category, records); } } -/// [`DocSink`] decorator that counts accepted applies. Every method +/// [`DocSink`] decorator that records accepted applies. Every method /// delegates verbatim — including `insert_subtree_returning_root_ids`, which /// MUST forward rather than fall through to the trait default, or an /// immediate-apply sink's real remapped ids would be swallowed. +/// +/// The description is computed BEFORE delegating, against the pre-edit +/// document — that is the only moment the `before` half of `before → after` +/// still exists — and is kept only when the sink accepts the command. pub struct CountingSink<'a> { inner: &'a mut dyn DocSink, - applied: Arc, + pending: Arc>>, +} + +impl CountingSink<'_> { + fn push(&self, repair: PendingRepair) { + if let Ok(mut pending) = self.pending.lock() { + pending.push(repair); + } + } } impl DocSink for CountingSink<'_> { @@ -206,9 +304,10 @@ impl DocSink for CountingSink<'_> { } fn apply(&mut self, cmd: EditorCommand) -> bool { + let described = describe_command(self.inner.state(), &cmd); let accepted = self.inner.apply(cmd); if accepted { - self.applied.fetch_add(1, Ordering::SeqCst); + self.push(described); } accepted } @@ -218,11 +317,23 @@ impl DocSink for CountingSink<'_> { nodes: Vec, parent_id: &NodeId, ) -> Option> { + let described = describe_command( + self.inner.state(), + &EditorCommand::InsertSubtree { + nodes: Vec::new(), + parent_id: parent_id.clone(), + page_id: None, + }, + ); + let count = nodes.len(); let out = self .inner .insert_subtree_returning_root_ids(nodes, parent_id); if out.is_some() { - self.applied.fetch_add(1, Ordering::SeqCst); + self.push(PendingRepair { + detail: format!("inserted {count} subtree(s)"), + ..described + }); } out } diff --git a/crates/op-orchestrator/src/repair_summary_tests.rs b/crates/op-orchestrator/src/repair_summary_tests.rs index 16a190c66..c8b88f4eb 100644 --- a/crates/op-orchestrator/src/repair_summary_tests.rs +++ b/crates/op-orchestrator/src/repair_summary_tests.rs @@ -113,7 +113,7 @@ fn counting_sink_counts_only_accepted_applies() { page_id: None, })); } - counter.checkpoint(&mut summary, CheckCategory::Structure); + counter.checkpoint(&mut summary, CheckCategory::Structure, "test-structure"); assert_eq!( summary.repairs_for(CheckCategory::Structure), @@ -135,7 +135,7 @@ fn checkpoints_attribute_only_the_edits_since_the_previous_one() { page_id: None, }); } - counter.checkpoint(&mut summary, CheckCategory::Structure); + counter.checkpoint(&mut summary, CheckCategory::Structure, "test-structure"); // `InsertSubtree` remaps ids, so target the root by the id it actually // landed under rather than the fixture's authored one. let root_id = base.state.active_children()[0].id_str().to_string(); @@ -147,7 +147,7 @@ fn checkpoints_attribute_only_the_edits_since_the_previous_one() { page_id: None, }); } - counter.checkpoint(&mut summary, CheckCategory::Layout); + counter.checkpoint(&mut summary, CheckCategory::Layout, "test-layout"); assert_eq!(summary.repairs_for(CheckCategory::Structure), 1); assert_eq!(