From d7172c669e22d5c40e73b55d622630ac69667f15 Mon Sep 17 00:00:00 2001 From: Orual Date: Sat, 8 Aug 2026 01:33:23 -0400 Subject: [PATCH] PM-83: complete J5 preview lifecycle corrections Epic: PM-86 Task: PM-83 --- src/appview/drafts.rs | 117 +++++++++++++++ src/publish.rs | 340 ++++++++++++++++++++++++++++++------------ src/publish/draft.rs | 2 + 3 files changed, 364 insertions(+), 95 deletions(-) diff --git a/src/appview/drafts.rs b/src/appview/drafts.rs index 4a75531..34d8f5c 100644 --- a/src/appview/drafts.rs +++ b/src/appview/drafts.rs @@ -341,8 +341,11 @@ pub(super) async fn upload_image( mod tests { use std::str::FromStr; + use axum::response::IntoResponse; + use axum_extra::extract::PrivateCookieJar; use jacquard_common::types::string::Did; use sqlx::sqlite::{SqliteConnectOptions, SqlitePoolOptions}; + use tower::ServiceExt; use super::*; use crate::publish::draft::{DraftModelInput, DraftPartInput}; @@ -471,6 +474,120 @@ mod tests { assert!(!delete_draft(&state, &a, "d1").await.unwrap()); } + #[tokio::test] + async fn authenticated_draft_lifecycle_round_trips_and_rejects_cross_owner_access() { + let mut state = crate::appview::test_support::state().await; + crate::appview::test_support::seed_identity(&state.pool, DID_A, "alice.com").await; + crate::appview::test_support::seed_identity(&state.pool, DID_B, "bob.com").await; + crate::appview::test_support::seed_profile(&state.pool, DID_A, "Alice").await; + crate::appview::test_support::seed_profile(&state.pool, DID_B, "Bob").await; + let (pds, server) = crate::appview::test_support::loopback_pds(false, None).await; + state.resolver = crate::appview::test_support::loopback_resolver(&pds); + let key_a = crate::appview::test_support::seed_oauth_session_at( + &state, + DID_A, + "draft-a", + Some(&pds), + ) + .await; + let key_b = crate::appview::test_support::seed_oauth_session_at( + &state, + DID_B, + "draft-b", + Some(&pds), + ) + .await; + let cookie = |key| { + jacquard_axum::oauth::set_session_cookie( + PrivateCookieJar::new(state.cookie_key.clone()), + &state.oauth_config, + &key, + ) + .unwrap() + .into_response() + .headers() + .get(axum::http::header::SET_COOKIE) + .unwrap() + .to_str() + .unwrap() + .split(';') + .next() + .unwrap() + .to_owned() + }; + let app = crate::appview::router().with_state(state.clone()); + let created = app + .clone() + .oneshot( + axum::http::Request::builder() + .method("POST") + .uri("/app/drafts") + .header(axum::http::header::COOKIE, cookie(key_a.clone())) + .header("content-type", "application/json") + .body(axum::body::Body::from( + serde_json::to_vec(&draft_named("Before")).unwrap(), + )) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(created.status(), axum::http::StatusCode::OK); + let created: DraftIdResponse = serde_json::from_slice( + &axum::body::to_bytes(created.into_body(), 1 << 20) + .await + .unwrap(), + ) + .unwrap(); + let path = format!("/app/drafts/{}", created.draft_id); + let updated = app + .clone() + .oneshot( + axum::http::Request::builder() + .method("PUT") + .uri(&path) + .header(axum::http::header::COOKIE, cookie(key_a.clone())) + .header("content-type", "application/json") + .body(axum::body::Body::from( + serde_json::to_vec(&draft_named("After")).unwrap(), + )) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(updated.status(), axum::http::StatusCode::OK); + let reloaded = app + .clone() + .oneshot( + axum::http::Request::builder() + .uri(&path) + .header(axum::http::header::COOKIE, cookie(key_a.clone())) + .body(axum::body::Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(reloaded.status(), axum::http::StatusCode::OK); + let reloaded: DraftThingInput = serde_json::from_slice( + &axum::body::to_bytes(reloaded.into_body(), 1 << 20) + .await + .unwrap(), + ) + .unwrap(); + assert_eq!(reloaded.name.as_deref(), Some("After")); + let cross_owner = app + .oneshot( + axum::http::Request::builder() + .uri(&path) + .header(axum::http::header::COOKIE, cookie(key_b)) + .body(axum::body::Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(cross_owner.status(), axum::http::StatusCode::NOT_FOUND); + server.abort(); + } + #[tokio::test] async fn promote_failure_keeps_draft() { let state = state().await; diff --git a/src/publish.rs b/src/publish.rs index da4e4d8..de8e6fa 100644 --- a/src/publish.rs +++ b/src/publish.rs @@ -181,9 +181,11 @@ struct Flow { save_in_flight: Signal, /// The mounted name input element, focused once after the first upload. name_element: Signal>>, - /// Current preview identity per stable model. Independent models do not - /// cancel each other; stale completions must match the complete identity. + /// Current preview operation token per stable model. Independent models do + /// not cancel each other; stale completions must match their own run token. preview_generations: Signal>, + /// Monotonic token source for preview operations in this flow instance. + preview_operation_counter: Signal, } #[component] @@ -228,6 +230,7 @@ fn PublishFlow(resume: String) -> Element { // The mounted name input element, focused once after the first upload. let name_element: Signal>> = use_signal(|| None); let preview_generations = use_signal(HashMap::new); + let preview_operation_counter = use_signal(|| 0_u64); let flow = Flow { client, @@ -250,6 +253,7 @@ fn PublishFlow(resume: String) -> Element { save_in_flight, name_element, preview_generations, + preview_operation_counter, }; // Resume a draft once on mount when `?draft=` is present; always load @@ -815,6 +819,9 @@ impl Flow { onclick: move |_| { let mut c = premove_w.composition; let mut guard = c.write(); + let Some(model_id) = guard.models.first().and_then(|model| model.model_id.clone()) else { + return; + }; let removed_primary = { let model = &mut guard.models[0]; let removed = model.parts.remove(pi); @@ -827,14 +834,10 @@ impl Flow { removed_primary }; if removed_primary { - clear_generated_preview(&mut guard, 0); - } - if let Some(model_id) = guard.models[0].model_id.clone() { + clear_generated_preview(&mut guard, &model_id, None); premove_w.preview_generations.write().remove(&model_id); } drop(guard); - let flow = premove_w.clone(); - spawn(async move { flow.recompute_model_preview(0).await; }); premove_w.clone().mark_dirty_and_save(); }, "Remove" @@ -931,7 +934,7 @@ impl Flow { let model_name = model.name.clone().unwrap_or_default(); let parts = model.parts.clone(); let name_w = self.clone(); - let remove_w = self.clone(); + let mut remove_w = self.clone(); let addfile_w = self.clone(); rsx! { section { key: "model-{mi}", class: "wizard-model", @@ -954,7 +957,14 @@ impl Flow { class: "button button-ghost wizard-remove", onclick: move |_| { let mut c = remove_w.composition; - c.write().models.remove(mi); + let mut guard = c.write(); + let Some(model_id) = guard.models.get(mi).and_then(|model| model.model_id.clone()) else { + return; + }; + clear_generated_preview(&mut guard, &model_id, None); + guard.models.remove(mi); + remove_w.preview_generations.write().remove(&model_id); + drop(guard); remove_w.clone().mark_dirty_and_save(); }, "Remove model" @@ -1033,6 +1043,9 @@ impl Flow { onclick: move |_| { let mut c = premove_w.composition; let mut guard = c.write(); + let Some(model_id) = guard.models.get(mi).and_then(|model| model.model_id.clone()) else { + return; + }; let removed_primary = { let model = &mut guard.models[mi]; let removed = model.parts.remove(pi); @@ -1045,14 +1058,12 @@ impl Flow { removed_primary }; if removed_primary { - clear_generated_preview(&mut guard, mi); - } - if let Some(model_id) = guard.models[mi].model_id.clone() { + clear_generated_preview(&mut guard, &model_id, None); premove_w.preview_generations.write().remove(&model_id); } drop(guard); let flow = premove_w.clone(); - spawn(async move { flow.recompute_model_preview(mi).await; }); + spawn(async move { flow.recompute_model_preview(model_id).await; }); premove_w.clone().mark_dirty_and_save(); }, "Remove" @@ -2090,8 +2101,12 @@ impl Flow { ensure_model_identity(model, &resource_id); model.parts.push(part); } + let model_id = + g.models.first().and_then(|model| model.model_id.clone()); drop(g); - self.clone().recompute_model_preview(0).await; + if let Some(model_id) = model_id { + self.clone().recompute_model_preview(model_id).await; + } // Focus the name field once, on the first staged // file, so the user lands on the next likely edit. if was_first && let Some(element) = self.name_element.read().clone() { @@ -2171,16 +2186,20 @@ impl Flow { format: format.clone(), ..Default::default() }; - { + let model_id = { let mut composition = self.composition; let mut guard = composition.write(); - if let Some(model) = guard.models.get_mut(model_index) { - install_staged_resource(model, resource); - ensure_model_identity(model, &resource_id); - model.parts.push(part); - } + let Some(model) = guard.models.get_mut(model_index) else { + return; + }; + install_staged_resource(model, resource); + ensure_model_identity(model, &resource_id); + model.parts.push(part); + model.model_id.clone() + }; + if let Some(model_id) = model_id { + self.clone().recompute_model_preview(model_id).await; } - self.clone().recompute_model_preview(model_index).await; upload.set(UploadStatus::Idle); let flow = self.clone(); flow.save_draft().await; @@ -2191,20 +2210,22 @@ impl Flow { } } - async fn recompute_model_preview(mut self, model_index: usize) { + async fn recompute_model_preview(mut self, model_id: String) { let draft_scope = self .draft_id .read() .clone() .unwrap_or_else(|| "unsaved".into()); - let (model_id, primary_id, primary_name, staged) = { + let (primary_id, primary_name, staged) = { let composition = self.composition.read(); - let Some(model) = composition.models.get(model_index) else { + let Some(model) = composition + .models + .iter() + .find(|model| model.model_id.as_deref() == Some(model_id.as_str())) + else { return; }; - let (Some(model_id), Some(primary_id)) = - (model.model_id.clone(), model.primary_resource_id.clone()) - else { + let Some(primary_id) = model.primary_resource_id.clone() else { return; }; let Some(primary) = model @@ -2215,28 +2236,35 @@ impl Flow { return; }; ( - model_id, primary_id, primary.source_name.clone(), model.staged_resources.clone(), ) }; - let Some(identity) = preview_identity(&draft_scope, &model_id, &primary_id, &staged) else { - self.clear_model_preview(model_index, &model_id); + let Some(composition_identity) = + preview_identity(&draft_scope, &model_id, &primary_id, &staged) + else { + self.clear_model_preview(&model_id, None); return; }; + let operation_id = { + let mut counter = self.preview_operation_counter; + let next = counter.read().saturating_add(1); + counter.set(next); + format!("{model_id}:{next}") + }; self.preview_generations .write() - .insert(model_id.clone(), identity.clone()); + .insert(model_id.clone(), operation_id.clone()); let acquired = match acquire_preview_resources(&self.client, &staged).await { Ok(resources) => resources, Err(error) => { tracing::debug!(%error, %model_id, "generated preview acquisition incomplete"); - self.clear_model_preview(model_index, &model_id); + self.clear_model_preview(&model_id, Some(&operation_id)); return; } }; - if !self.preview_is_current(&model_id, &identity) { + if !self.preview_is_current(&model_id, &operation_id) { return; } let png = if format_registry::format_for_extension(&primary_name) @@ -2244,7 +2272,7 @@ impl Flow { { let DraftBundlePlan::Complete(bundle) = plan_draft_ldraw_bundle(&primary_id, &acquired) else { - self.clear_model_preview(model_index, &model_id); + self.clear_model_preview(&model_id, Some(&operation_id)); return; }; render_preview_bundle(bundle).await @@ -2253,15 +2281,16 @@ impl Flow { .iter() .find(|resource| resource.metadata.resource_id == primary_id) else { + self.clear_model_preview(&model_id, Some(&operation_id)); return; }; render_preview(&primary_name, primary.bytes.clone()).await }; let Ok(png_bytes) = png else { - self.clear_model_preview(model_index, &model_id); + self.clear_model_preview(&model_id, Some(&operation_id)); return; }; - if !self.preview_is_current(&model_id, &identity) { + if !self.preview_is_current(&model_id, &operation_id) { return; } let Some(image) = upload_generated_preview( @@ -2275,12 +2304,33 @@ impl Flow { else { return; }; - if !self.preview_is_current(&model_id, &identity) { + if !self.preview_is_current(&model_id, &operation_id) { return; } let mut composition = self.composition; let mut guard = composition.write(); - attach_generated_preview(&mut guard, model_index, &primary_id, image); + let Some(model) = guard + .models + .iter() + .find(|model| model.model_id.as_deref() == Some(model_id.as_str())) + else { + return; + }; + let Some(current_primary) = model.primary_resource_id.as_deref() else { + return; + }; + let current_identity = preview_identity( + &draft_scope, + &model_id, + current_primary, + &model.staged_resources, + ); + if current_primary != primary_id + || current_identity.as_deref() != Some(composition_identity.as_str()) + { + return; + } + attach_generated_preview(&mut guard, &model_id, &primary_id, &operation_id, image); } fn preview_is_current(&self, model_id: &str, identity: &str) -> bool { @@ -2290,10 +2340,26 @@ impl Flow { .is_some_and(|current| current == identity) } - fn clear_model_preview(&mut self, model_index: usize, model_id: &str) { - self.preview_generations.write().remove(model_id); - let mut composition = self.composition; - clear_generated_preview(&mut composition.write(), model_index); + fn clear_model_preview(&mut self, model_id: &str, operation_id: Option<&str>) { + let should_clear = { + let mut generations = self.preview_generations.write(); + match operation_id { + Some(operation_id) + if generations + .get(model_id) + .is_some_and(|current| current == operation_id) => + { + generations.remove(model_id); + true + } + Some(_) => false, + None => generations.remove(model_id).is_some(), + } + }; + if should_clear || operation_id.is_none() { + let mut composition = self.composition; + clear_generated_preview(&mut composition.write(), model_id, operation_id); + } } /// Upload an image and attach it to the target's cover/previews, then save @@ -2656,61 +2722,72 @@ async fn acquire_preview_resources( fn remove_image(list: &mut Option>, image: &Image) { if let Some(images) = list { - images.retain(|candidate| candidate != image); + if let Some(index) = images.iter().position(|candidate| candidate == image) { + images.remove(index); + } if images.is_empty() { *list = None; } } } -fn clear_generated_preview(composition: &mut DraftThingInput, model_index: usize) { - let generated = composition +fn clear_generated_preview( + composition: &mut DraftThingInput, + model_id: &str, + operation_id: Option<&str>, +) { + let Some(model_index) = composition .models - .get(model_index) - .and_then(|model| model.generated_preview.as_ref()) - .map(|preview| preview.image.clone()); - let Some(image) = generated else { return }; - remove_image(&mut composition.previews, &image); - if composition - .cover + .iter() + .position(|model| model.model_id.as_deref() == Some(model_id)) + else { + return; + }; + let generated = composition.models[model_index] + .generated_preview .as_ref() - .is_some_and(|cover| cover == &vec![image.clone()]) - { - composition.cover = None; - } - if let Some(model) = composition.models.get_mut(model_index) { - remove_image(&mut model.previews, &image); - if model - .cover - .as_ref() - .is_some_and(|cover| cover == &vec![image.clone()]) - { - model.cover = None; - } - model.generated_preview = None; - for part in &mut model.parts { - if part - .generated_preview - .as_ref() - .is_some_and(|preview| preview.image == image) - { - remove_image(&mut part.previews, &image); - part.generated_preview = None; - } + .filter(|preview| { + operation_id.is_none_or(|operation| preview.operation_id.as_deref() == Some(operation)) + }) + .cloned(); + let Some(generated) = generated else { return }; + let image = generated.image; + remove_image(&mut composition.previews, &image); + remove_image(&mut composition.cover, &image); + let model = &mut composition.models[model_index]; + remove_image(&mut model.previews, &image); + remove_image(&mut model.cover, &image); + model.generated_preview = None; + for part in &mut model.parts { + if part.generated_preview.as_ref().is_some_and(|preview| { + preview.operation_id == generated.operation_id + && preview.source_resource_id == generated.source_resource_id + }) { + remove_image(&mut part.previews, &image); + part.generated_preview = None; } } } fn attach_generated_preview( composition: &mut DraftThingInput, - model_index: usize, + model_id: &str, source_resource_id: &str, + operation_id: &str, image: Image, ) { - clear_generated_preview(composition, model_index); + clear_generated_preview(composition, model_id, None); + let Some(model_index) = composition + .models + .iter() + .position(|model| model.model_id.as_deref() == Some(model_id)) + else { + return; + }; let seed_cover = model_index == 0 && composition.cover.as_ref().is_none_or(Vec::is_empty); let provenance = GeneratedPreview { source_resource_id: source_resource_id.to_owned(), + operation_id: Some(operation_id.to_owned()), image: image.clone(), }; composition @@ -2720,20 +2797,19 @@ fn attach_generated_preview( if seed_cover { composition.cover = Some(vec![image.clone()]); } - if let Some(model) = composition.models.get_mut(model_index) { - model - .previews - .get_or_insert_with(Vec::new) - .push(image.clone()); - model.generated_preview = Some(provenance.clone()); - if let Some(part) = model - .parts - .iter_mut() - .find(|part| part.resource_id.as_deref() == Some(source_resource_id)) - { - part.previews.get_or_insert_with(Vec::new).push(image); - part.generated_preview = Some(provenance); - } + let model = &mut composition.models[model_index]; + model + .previews + .get_or_insert_with(Vec::new) + .push(image.clone()); + model.generated_preview = Some(provenance.clone()); + if let Some(part) = model + .parts + .iter_mut() + .find(|part| part.resource_id.as_deref() == Some(source_resource_id)) + { + part.previews.get_or_insert_with(Vec::new).push(image); + part.generated_preview = Some(provenance); } } @@ -3297,8 +3373,15 @@ mod tests { }], ..Default::default() }; - attach_generated_preview(&mut composition, 0, "resource-1", first); - attach_generated_preview(&mut composition, 0, "resource-1", second.clone()); + composition.models[0].model_id = Some("model-1".into()); + attach_generated_preview(&mut composition, "model-1", "resource-1", "op-1", first); + attach_generated_preview( + &mut composition, + "model-1", + "resource-1", + "op-2", + second.clone(), + ); for images in [ composition.previews.as_ref().unwrap(), composition.models[0].previews.as_ref().unwrap(), @@ -3311,12 +3394,78 @@ mod tests { assert_eq!(composition.cover, Some(vec![user])); } + #[test] + fn stale_preview_cleanup_cannot_clear_newer_operation() { + let old = test_image("old generated"); + let current = test_image("current generated"); + let mut composition = DraftThingInput { + models: vec![DraftModelInput { + model_id: Some("model-1".into()), + parts: vec![DraftPartInput { + resource_id: Some("resource-1".into()), + ..Default::default() + }], + ..Default::default() + }], + ..Default::default() + }; + attach_generated_preview(&mut composition, "model-1", "resource-1", "old-op", old); + attach_generated_preview( + &mut composition, + "model-1", + "resource-1", + "current-op", + current.clone(), + ); + clear_generated_preview(&mut composition, "model-1", Some("old-op")); + assert_eq!( + composition.models[0] + .generated_preview + .as_ref() + .unwrap() + .operation_id + .as_deref(), + Some("current-op") + ); + assert!( + composition.models[0] + .previews + .as_ref() + .unwrap() + .contains(¤t) + ); + } + + #[test] + fn generated_preview_cleanup_preserves_user_cover_and_normalizes_empty_lists() { + let generated = test_image("generated"); + let user = test_image("user"); + let mut composition = DraftThingInput { + cover: Some(vec![user.clone()]), + models: vec![DraftModelInput { + model_id: Some("model-1".into()), + cover: Some(vec![user.clone()]), + parts: vec![DraftPartInput { + resource_id: Some("resource-1".into()), + ..Default::default() + }], + ..Default::default() + }], + ..Default::default() + }; + attach_generated_preview(&mut composition, "model-1", "resource-1", "op-1", generated); + clear_generated_preview(&mut composition, "model-1", Some("op-1")); + assert_eq!(composition.cover, Some(vec![user.clone()])); + assert_eq!(composition.models[0].cover, Some(vec![user])); + } + #[test] fn clearing_generated_preview_removes_only_generated_cover_and_refs() { let generated = test_image("generated"); let user = test_image("user"); let mut composition = DraftThingInput { models: vec![DraftModelInput { + model_id: Some("model-1".into()), parts: vec![DraftPartInput { resource_id: Some("resource-1".into()), previews: Some(vec![user.clone()]), @@ -3326,8 +3475,9 @@ mod tests { }], ..Default::default() }; - attach_generated_preview(&mut composition, 0, "resource-1", generated); - clear_generated_preview(&mut composition, 0); + composition.models[0].model_id = Some("model-1".into()); + attach_generated_preview(&mut composition, "model-1", "resource-1", "op-1", generated); + clear_generated_preview(&mut composition, "model-1", Some("op-1")); assert!(composition.cover.is_none()); assert!(composition.previews.is_none()); assert_eq!(composition.models[0].parts[0].previews, Some(vec![user])); diff --git a/src/publish/draft.rs b/src/publish/draft.rs index 84bae37..f54fcef 100644 --- a/src/publish/draft.rs +++ b/src/publish/draft.rs @@ -129,6 +129,8 @@ pub struct AcquiredDraftResource { #[serde(rename_all = "camelCase")] pub struct GeneratedPreview { pub source_resource_id: String, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub operation_id: Option, pub image: Image, } -- knotmirror 0.0.1