From 212e228f26e857d048da12739b51283be0b83e98 Mon Sep 17 00:00:00 2001 From: Fini Date: Sat, 18 Jul 2026 16:34:25 +0800 Subject: [PATCH] fix(editor): embed relinked images and warn on machine-local links MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Relink wrote the picked file's path into src, which renders on this machine and silently breaks for anyone the file is shared with — the only remaining save path that produced a non-portable reference. It now embeds the picked file as a data URL like the fill picker, with no path fallback on read failure. Path-form sources that do resolve locally get a new LinkedLocal status whose warning row says the image won't survive sharing, instead of passing as Ok. --- .../op-editor-core/src/image_panel_state.rs | 9 +- .../widgets/property_panel_image_assets.rs | 12 +- .../property_panel_image_assets_tests.rs | 21 +++ .../op-host-desktop/src/image_panel_host.rs | 17 +- crates/op-host-desktop/src/persistence.rs | 2 +- .../op-host-desktop/src/persistence_image.rs | 146 ++++++++++-------- 6 files changed, 136 insertions(+), 71 deletions(-) diff --git a/crates/op-editor-core/src/image_panel_state.rs b/crates/op-editor-core/src/image_panel_state.rs index 0580b7ae3..905e8ec27 100644 --- a/crates/op-editor-core/src/image_panel_state.rs +++ b/crates/op-editor-core/src/image_panel_state.rs @@ -46,8 +46,15 @@ pub enum ImageGeneratePhase { /// Host-checked status of the selected image node's `src` asset. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum ImageAssetStatus { - /// Remote / data URL, or a local file that exists. + /// Remote / data URL — fully portable, nothing to warn about. Ok, + /// A local filesystem path that DOES resolve on this machine + /// (Divergence from TS, which folds this into `Ok`: a path that + /// happens to exist here is still only a pointer, not the image + /// content — sharing the document reproduces the exact "file is + /// missing" report for anyone without that same path. See the + /// shared-.op portability audit, 2026-07-18.) + LinkedLocal, /// Relative path with no document path to resolve against /// (TS "Relative image path cannot be resolved yet"). Unresolved, diff --git a/crates/op-editor-ui/src/widgets/property_panel_image_assets.rs b/crates/op-editor-ui/src/widgets/property_panel_image_assets.rs index 8bafad798..d39c76ec8 100644 --- a/crates/op-editor-ui/src/widgets/property_panel_image_assets.rs +++ b/crates/op-editor-ui/src/widgets/property_panel_image_assets.rs @@ -52,7 +52,9 @@ const EMPTY_STATE_H: f32 = 90.0; // TS py-8 icon + label block /// Warning surfaced under the thumbnail row (TS `LocalImageWarning`). #[derive(Debug, Clone, PartialEq, Eq)] pub struct ImageAssetWarning { - /// Literal TS message string. + /// Literal TS message string for `Unresolved` / `Missing`; the + /// `LinkedLocal` message is a Rust-side addition (no TS + /// equivalent — see `ImageAssetStatus::LinkedLocal`'s doc). pub message: &'static str, pub asset_path: String, } @@ -89,6 +91,14 @@ pub fn image_panel_view(state: &EditorState, node: &PenNode) -> Option "Relative image path cannot be resolved yet", Some(ImageAssetStatus::Missing) => "Image file is missing", + // Resolves fine here, but it's a pointer, not embedded + // content — sharing this document won't carry the + // image with it. A one-time nudge, not an error state: + // the button below (Relink) re-runs through the + // embed-not-link path and clears this. + Some(ImageAssetStatus::LinkedLocal) => { + "Linked to a local file — won't be included when this file is shared" + } Some(ImageAssetStatus::Ok) | None => return None, }; Some(ImageAssetWarning { diff --git a/crates/op-editor-ui/src/widgets/property_panel_image_assets_tests.rs b/crates/op-editor-ui/src/widgets/property_panel_image_assets_tests.rs index 3886b365e..095613ff7 100644 --- a/crates/op-editor-ui/src/widgets/property_panel_image_assets_tests.rs +++ b/crates/op-editor-ui/src/widgets/property_panel_image_assets_tests.rs @@ -57,6 +57,27 @@ fn warning_requires_a_host_asset_check() { assert!(selected_image_view(&state).warning.is_none()); } +/// A local path that DOES resolve on this machine still surfaces a +/// (softer, informational) warning — resolving here isn't the same as +/// being portable when the document is shared. `Ok` is the ONLY status +/// that suppresses the row. +#[test] +fn linked_local_status_still_warns_that_sharing_will_lose_the_image() { + let mut state = image_state(); + let id = state.selection.anchor.as_str().to_string(); + state.editor_ui.image_panel.asset_check = Some(ImageAssetCheck { + node_id: id, + src: "./assets/hero.png".into(), + status: ImageAssetStatus::LinkedLocal, + }); + let warning = selected_image_view(&state).warning.expect("warning"); + assert_eq!( + warning.message, + "Linked to a local file — won't be included when this file is shared" + ); + assert_eq!(warning.asset_path, "./assets/hero.png"); +} + #[test] fn local_asset_path_matches_ts_predicate() { assert!(is_local_asset_path("./a.png")); diff --git a/crates/op-host-desktop/src/image_panel_host.rs b/crates/op-host-desktop/src/image_panel_host.rs index b2381fdd7..41008cd90 100644 --- a/crates/op-host-desktop/src/image_panel_host.rs +++ b/crates/op-host-desktop/src/image_panel_host.rs @@ -235,6 +235,8 @@ pub(crate) fn active_image_gen_profile( } // --- Asset status (TS resolveRuntimeAssetSource + warning gating) ---- +// Divergence from TS: a local path that resolves on THIS machine is +// `LinkedLocal`, not folded into `Ok` — see `ImageAssetStatus::LinkedLocal`. pub(crate) fn asset_status(src: &str, document_path: Option<&Path>) -> ImageAssetStatus { let trimmed = src.trim(); @@ -262,7 +264,7 @@ pub(crate) fn asset_status(src: &str, document_path: Option<&Path>) -> ImageAsse .unwrap_or(normalized); if is_absolute_path(&decoded) { return if Path::new(&decoded).exists() { - ImageAssetStatus::Ok + ImageAssetStatus::LinkedLocal } else { ImageAssetStatus::Missing }; @@ -272,7 +274,7 @@ pub(crate) fn asset_status(src: &str, document_path: Option<&Path>) -> ImageAsse }; let base = doc.parent().unwrap_or_else(|| Path::new(".")); if base.join(&decoded).exists() { - ImageAssetStatus::Ok + ImageAssetStatus::LinkedLocal } else { ImageAssetStatus::Missing } @@ -522,22 +524,25 @@ mod tests { asset_status("/definitely/not/here/x.png", None), ImageAssetStatus::Missing ); - // Absolute path that exists → ok. + // Absolute path that exists → LinkedLocal, not Ok — it resolves + // HERE, but it's still a pointer, not portable content (see + // `ImageAssetStatus::LinkedLocal`'s doc). let dir = std::env::temp_dir(); let file = dir.join(format!("op-image-panel-test-{}.png", std::process::id())); std::fs::write(&file, b"x").unwrap(); assert_eq!( asset_status(&file.display().to_string(), None), - ImageAssetStatus::Ok + ImageAssetStatus::LinkedLocal ); - // Relative path resolved against the document dir. + // Relative path resolved against the document dir — same + // LinkedLocal treatment. let doc = dir.join("doc.op"); assert_eq!( asset_status( file.file_name().unwrap().to_str().unwrap(), Some(doc.as_path()) ), - ImageAssetStatus::Ok + ImageAssetStatus::LinkedLocal ); assert_eq!( asset_status("nope-not-here.png", Some(doc.as_path())), diff --git a/crates/op-host-desktop/src/persistence.rs b/crates/op-host-desktop/src/persistence.rs index 51cf174aa..4dac994af 100644 --- a/crates/op-host-desktop/src/persistence.rs +++ b/crates/op-host-desktop/src/persistence.rs @@ -367,7 +367,7 @@ pub fn run_action( ActionOutcome::Noop } FileAction::RelinkImage => { - crate::persistence_image::handle_relink_image(host, current_path.as_deref()); + crate::persistence_image::handle_relink_image(host); ActionOutcome::Noop } } diff --git a/crates/op-host-desktop/src/persistence_image.rs b/crates/op-host-desktop/src/persistence_image.rs index 3f97f6891..e54ad79f1 100644 --- a/crates/op-host-desktop/src/persistence_image.rs +++ b/crates/op-host-desktop/src/persistence_image.rs @@ -1,7 +1,8 @@ //! Image / SVG import handlers for [`super::persistence::run_action`]. //! //! Split out of `persistence.rs` so that file stays under the 800-line -//! cap. Two entry points: +//! cap. Three entry points, all embed-not-link (every one ends in a +//! `data:` URL — see the shared-.op portability audit, 2026-07-18): //! //! - [`handle_import_image_or_svg`] — toolbar shape-picker action: //! pops `rfd::FileDialog`, decodes the file, and inserts a new @@ -11,6 +12,9 @@ //! pops the same dialog and writes the chosen image into the //! selected node's primary fill as //! `PenFill::Image { url: }`. +//! - [`handle_relink_image`] — image-section warning row's Relink +//! button: pops the same dialog and rewrites the selected +//! `ImageNode.src` with the picked file's `data:` URL. use base64::engine::general_purpose::STANDARD as B64; use base64::Engine as _; @@ -154,14 +158,37 @@ pub fn handle_pick_fill_image(host: &mut WidgetHostNative) { } /// Handle the image-section warning row's Relink button: pop the -/// image file dialog and rewrite the selected `ImageNode.src` — -/// stored relative to the document when both share a root (TS -/// `onUpdate({ src: toStoredAssetPath(result.filePath, documentPath) })`). -pub fn handle_relink_image(host: &mut WidgetHostNative, current_path: Option<&Path>) { +/// image file dialog, read the picked file, and rewrite the selected +/// `ImageNode.src` as a `data:` URL — the SAME embed-not-link contract +/// every other image-entry path already uses +/// (`handle_import_image_or_svg`, `handle_pick_fill_image`). +/// +/// This used to write a document-relative or absolute filesystem PATH +/// (TS `onUpdate({ src: toStoredAssetPath(result.filePath, documentPath) })`). +/// That "fixed" the warning locally — the relinked file exists on the +/// relinking machine — but produced a document that is STILL not +/// portable: share it, and the recipient hits the exact same missing- +/// image report the Relink button was supposed to resolve, because +/// their machine doesn't have that path either. Embedding closes that +/// gap (shared-.op portability audit, 2026-07-18): the fixed reference +/// is real content, not another local pointer. +pub fn handle_relink_image(host: &mut WidgetHostNative) { let Some(path) = pick_image_path(host) else { return; }; - let stored = to_stored_asset_path(&path, current_path); + apply_relink(host, &path); +} + +/// Core of [`handle_relink_image`], factored out so the embed-then-write +/// behavior is testable without a real file-picker dialog. +fn apply_relink(host: &mut WidgetHostNative, path: &Path) { + let url = match read_as_data_url(path) { + Ok(u) => u, + Err(e) => { + eprintln!("[relink-image] {}: {e}", path.display()); + return; + } + }; let state = host.editor_state_mut(); let id = state.selection.anchor.clone(); if !id.is_real() { @@ -171,7 +198,7 @@ pub fn handle_relink_image(host: &mut WidgetHostNative, current_path: Option<&Pa if let Some(jian_ops_schema::node::PenNode::Image(image)) = op_editor_core::walkers::find_node_mut(state.active_children_mut(), &id) { - image.src = stored.into(); + image.src = url.into(); } // Drop the stale asset check so the warning row clears on the // next pump (it re-probes the new src). @@ -179,67 +206,62 @@ pub fn handle_relink_image(host: &mut WidgetHostNative, current_path: Option<&Pa host.mark_editor_state_dirty(); } -/// Port of TS `toStoredAssetPath` (document-assets.ts:87-104): an -/// absolute picked path becomes document-relative when the document -/// path shares its prefix (drive / root), else stays absolute. -/// Separators normalize to `/` either way. -fn to_stored_asset_path(asset: &Path, document: Option<&Path>) -> String { - let normalized = asset.display().to_string().replace('\\', "/"); - let Some(doc_dir) = document.and_then(Path::parent) else { - return normalized; - }; - let base = doc_dir.display().to_string().replace('\\', "/"); - let base_parts: Vec<&str> = base.split('/').filter(|s| !s.is_empty()).collect(); - let target_parts: Vec<&str> = normalized.split('/').filter(|s| !s.is_empty()).collect(); - // Windows drive prefixes must match case-insensitively (TS - // extractPathPrefix comparison); on POSIX both prefixes are `/`. - let drive = |parts: &[&str]| -> String { - parts - .first() - .filter(|p| p.len() == 2 && p.as_bytes()[1] == b':') - .map(|p| p.to_lowercase()) - .unwrap_or_default() - }; - if drive(&base_parts) != drive(&target_parts) { - return normalized; - } - let mut shared = 0; - while shared < base_parts.len() - && shared < target_parts.len() - && base_parts[shared].eq_ignore_ascii_case(target_parts[shared]) - { - shared += 1; - } - let ups = base_parts.len() - shared; - let mut segments: Vec = std::iter::repeat_n("..".to_string(), ups).collect(); - segments.extend(target_parts[shared..].iter().map(|s| s.to_string())); - if segments.is_empty() { - ".".to_string() - } else { - segments.join("/") - } -} - #[cfg(test)] mod relink_tests { - use super::to_stored_asset_path; - use std::path::Path; + use super::apply_relink; + use op_host_native::widget_host::WidgetHostNative; + /// The Relink button must embed the picked file, not point at its + /// path — a path is only ever valid on the machine that picked it, + /// so a "fixed" reference that's still a path reproduces the exact + /// missing-image report for anyone the document is shared with. #[test] - fn stored_path_relativizes_against_the_document_dir() { - let doc = Path::new("/projects/site/design.op"); - assert_eq!( - to_stored_asset_path(Path::new("/projects/site/assets/a.png"), Some(doc)), - "assets/a.png" + fn relink_embeds_the_picked_file_as_a_data_url_instead_of_a_path() { + let mut host = WidgetHostNative::new(); + let id = host + .editor_state_mut() + .insert_image_node_at_viewport("Photo", "assets/broken.png") + .expect("image node inserted and selected"); + + let tmp_path = + std::env::temp_dir().join(format!("op-relink-test-{}.png", std::process::id())); + std::fs::write(&tmp_path, b"not a real png, just bytes to embed").expect("write temp file"); + apply_relink(&mut host, &tmp_path); + let _ = std::fs::remove_file(&tmp_path); + + let node = op_editor_core::walkers::find_node(host.editor_state().active_children(), &id) + .expect("image node still present"); + let jian_ops_schema::node::PenNode::Image(image) = node else { + panic!("expected an image node, got {node:?}"); + }; + assert!( + image.src.starts_with("data:image/png;base64,"), + "relink must embed as a data: URL, not store the picked path: {}", + image.src ); + } + + /// A read failure (file vanished between pick and read, unreadable + /// permissions, …) must leave the node's `src` untouched — no + /// fallback to writing the unreadable path either. + #[test] + fn relink_leaves_src_untouched_when_the_file_cannot_be_read() { + let mut host = WidgetHostNative::new(); + host.editor_state_mut() + .insert_image_node_at_viewport("Photo", "assets/broken.png") + .expect("image node inserted and selected"); + + let missing_path = std::env::temp_dir().join("op-relink-test-does-not-exist.png"); + apply_relink(&mut host, &missing_path); + + let node = host.editor_state().selected_node().expect("still selected"); + let jian_ops_schema::node::PenNode::Image(image) = node else { + panic!("expected an image node, got {node:?}"); + }; assert_eq!( - to_stored_asset_path(Path::new("/projects/shared/b.png"), Some(doc)), - "../shared/b.png" - ); - // No document → absolute, normalized. - assert_eq!( - to_stored_asset_path(Path::new("/projects/site/assets/a.png"), None), - "/projects/site/assets/a.png" + image.src.as_str(), + "assets/broken.png", + "a read failure must not overwrite the existing src" ); } }