SemianiakaVY/openpencil
1
0
Fork 0
Code Issues Pull requests Actions 1 Packages Projects Releases Wiki Activity

fix(editor): embed relinked images and warn on machine-local links

Browse source 
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.
This commit is contained in:
Fini 2026-07-18 16:34:25 +08:00
parent 5eeaa701dd
commit 212e228f26
6 changed files with 136 additions and 71 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

9
crates/op-editor-core/src/image_panel_state.rs
View file

@ -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,

12
crates/op-editor-ui/src/widgets/property_panel_image_assets.rs
View file

@ -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<ImagePane
let message = match state.editor_ui.image_panel.status_for(&node_id, s) {
Some(ImageAssetStatus::Unresolved) => "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 {

21
crates/op-editor-ui/src/widgets/property_panel_image_assets_tests.rs
View file

@ -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"));

17
crates/op-host-desktop/src/image_panel_host.rs
View file

@ -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())),

2
crates/op-host-desktop/src/persistence.rs
View file

@ -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
}
}

146
crates/op-host-desktop/src/persistence_image.rs
View file

@ -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: <data-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<String> = 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"
);
}
}