From 213458b60cd92e5a917953bdfafcb3451d24b04b Mon Sep 17 00:00:00 2001 From: Gregory Schier Date: Tue, 8 Sep 2026 08:56:44 -0700 Subject: [PATCH] feat(import): warn that a document already has a workspace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Renaming the second workspace was the mechanism, not the news: what matters is that this document already produced a workspace, so importing it here copies it instead of updating it. The plan says that outright, names the workspace, and does it on key overlap rather than on the name — so it holds after either side has been renamed, and stays quiet when the import is merging into that very workspace. Plan notes carry a level so the ones worth a second thought read as cautions, and the destination row gets its "new" chip back. Co-Authored-By: Claude Opus 5 --- .../components/ImportDataDialog.tsx | 50 +++++-- .../yaak-rpc-schema/bindings/gen_util.ts | 7 +- crates/yaak-models/bindings/gen_util.ts | 7 +- crates/yaak-models/src/util.rs | 30 +++++ crates/yaak/src/import.rs | 125 +++++++++++++----- 5 files changed, 173 insertions(+), 46 deletions(-) diff --git a/apps/yaak-client/components/ImportDataDialog.tsx b/apps/yaak-client/components/ImportDataDialog.tsx index facb8047..9e9f5680 100644 --- a/apps/yaak-client/components/ImportDataDialog.tsx +++ b/apps/yaak-client/components/ImportDataDialog.tsx @@ -335,6 +335,7 @@ function LoadedImportDataDialog({ data: { kind: "destination", label: existing?.name ?? planned?.name ?? "New workspace", + isNew: planDestination.type === "new_workspace", }, children: itemTree, }; @@ -371,7 +372,12 @@ function LoadedImportDataDialog({ key={`${warning.title}:${warning.detail}`} className="flex items-start gap-2.5 px-3 py-2.5" > - +
{warning.title}
{warning.detail}
@@ -555,11 +561,15 @@ function ImportTreeRow({ <>
{row.label}
+ {row.kind === "destination" && row.isNew && ( + + )} ); } const { item } = row; + const label = actionLabel(item); return ( <> {item.model === "folder" || item.model === "environment" ? ( @@ -585,27 +595,47 @@ function ImportTreeRow({ />
) : ( - actionLabel(item) && ( - - {actionLabel(item)} - - + /> ) )} ); } +function ActionChip({ + label, + help, + className, +}: { + label: string; + help: string | null; + className?: string; +}) { + return ( + + {label} + {help != null && } + + ); +} + function actionLabel(item: ImportPlanItem): string | null { switch (item.action) { case "create": @@ -673,7 +703,7 @@ function ancestorsOf(item: ImportPlanItem, byId: Map): I * themselves. */ type TreeRow = - | { kind: "destination"; label: string } + | { kind: "destination"; label: string; isNew: boolean } | { kind: "group"; label: string; icon: IconProps["icon"] } | { kind: "item"; item: ImportPlanItem }; diff --git a/crates/common/yaak-rpc-schema/bindings/gen_util.ts b/crates/common/yaak-rpc-schema/bindings/gen_util.ts index f706cb08..3217af23 100644 --- a/crates/common/yaak-rpc-schema/bindings/gen_util.ts +++ b/crates/common/yaak-rpc-schema/bindings/gen_util.ts @@ -88,7 +88,12 @@ export type ImportPlanItem = { */ export type ImportPlanReason = "moved_into_ignored_folder"; -export type ImportPlanWarning = { title: string; detail: string }; +export type ImportPlanWarning = { title: string; detail: string; level: ImportPlanWarningLevel }; + +/** + * Whether a plan's note is something to know or something to think twice about. + */ +export type ImportPlanWarningLevel = "info" | "warning"; /** * The model types an import plan can contain. diff --git a/crates/yaak-models/bindings/gen_util.ts b/crates/yaak-models/bindings/gen_util.ts index f706cb08..3217af23 100644 --- a/crates/yaak-models/bindings/gen_util.ts +++ b/crates/yaak-models/bindings/gen_util.ts @@ -88,7 +88,12 @@ export type ImportPlanItem = { */ export type ImportPlanReason = "moved_into_ignored_folder"; -export type ImportPlanWarning = { title: string; detail: string }; +export type ImportPlanWarning = { title: string; detail: string; level: ImportPlanWarningLevel }; + +/** + * Whether a plan's note is something to know or something to think twice about. + */ +export type ImportPlanWarningLevel = "info" | "warning"; /** * The model types an import plan can contain. diff --git a/crates/yaak-models/src/util.rs b/crates/yaak-models/src/util.rs index deb89485..47a2dc07 100644 --- a/crates/yaak-models/src/util.rs +++ b/crates/yaak-models/src/util.rs @@ -109,6 +109,36 @@ pub enum ImportDestination { pub struct ImportPlanWarning { pub title: String, pub detail: String, + #[serde(default)] + pub level: ImportPlanWarningLevel, +} + +/// Whether a plan's note is something to know or something to think twice about. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, Deserialize, Serialize, TS)] +#[serde(rename_all = "snake_case")] +#[ts(export, export_to = "gen_util.ts")] +pub enum ImportPlanWarningLevel { + #[default] + Info, + Warning, +} + +impl ImportPlanWarning { + pub fn info(title: impl Into, detail: impl Into) -> Self { + Self { + title: title.into(), + detail: detail.into(), + level: ImportPlanWarningLevel::Info, + } + } + + pub fn warning(title: impl Into, detail: impl Into) -> Self { + Self { + title: title.into(), + detail: detail.into(), + level: ImportPlanWarningLevel::Warning, + } + } } /// Where an import's contents came from, used to link the committed workspace back to it. diff --git a/crates/yaak/src/import.rs b/crates/yaak/src/import.rs index d704fd67..a619d1e0 100644 --- a/crates/yaak/src/import.rs +++ b/crates/yaak/src/import.rs @@ -104,13 +104,7 @@ pub fn plan_import_resources( .collect::>(); for workspace in &mut workspaces { let unique = unique_name(&workspace.name, &taken); - if unique != workspace.name { - warnings.push(ImportPlanWarning { - title: "Workspace renamed".to_string(), - detail: format!("{} → {unique} · that name is taken", workspace.name), - }); - workspace.name = unique.clone(); - } + workspace.name = unique.clone(); taken.push(unique); } @@ -135,10 +129,10 @@ pub fn plan_import_resources( } else { format!("{} imported workspaces", resources.workspaces.len()) }; - warnings.push(ImportPlanWarning { - title: "Workspace settings skipped".to_string(), - detail: format!("{source} · {}", display_list(&skipped_fields)), - }); + warnings.push(ImportPlanWarning::info( + "Workspace settings skipped", + format!("{source} · {}", display_list(&skipped_fields)), + )); } } (workspace_id.clone(), folder_id.clone()) @@ -265,22 +259,22 @@ pub fn plan_import_resources( for (source_name, imported_name, variable_count) in separated_base_environments { let variables = if variable_count == 1 { "variable" } else { "variables" }; - warnings.push(ImportPlanWarning { - title: "Base environment kept separate".to_string(), - detail: format!("{source_name} → {imported_name} · {variable_count} {variables}"), - }); + warnings.push(ImportPlanWarning::info( + "Base environment kept separate", + format!("{source_name} → {imported_name} · {variable_count} {variables}"), + )); } if converted_duplicate_base_environment { - warnings.push(ImportPlanWarning { - title: "Base environments separated".to_string(), - detail: "Only the first remains the base environment".to_string(), - }); + warnings.push(ImportPlanWarning::info( + "Base environments separated", + "Only the first remains the base environment", + )); } if converted_duplicate_folder_environment { - warnings.push(ImportPlanWarning { - title: "Folder environments separated".to_string(), - detail: "Only the first remains attached to each folder".to_string(), - }); + warnings.push(ImportPlanWarning::info( + "Folder environments separated", + "Only the first remains attached to each folder", + )); } let resources = BatchUpsertResult { @@ -302,9 +296,47 @@ pub fn plan_import_resources( origin, }; merge_with_linked_source(query_manager, &mut plan, &original)?; + warn_if_imported_elsewhere(&query_manager.connect(), &mut plan)?; Ok(plan) } +/// A document that already produced a workspace can be merged back into it. Importing it anywhere +/// else copies it instead, which is worth saying before the user finds two of everything. +fn warn_if_imported_elsewhere(db: &ClientDb, plan: &mut ImportPlan) -> Result<()> { + let destination_id = match &plan.destination { + ImportDestination::ExistingWorkspace { workspace_id, .. } => Some(workspace_id.as_str()), + ImportDestination::NewWorkspace => None, + }; + let incoming = plan.source_keys.values().collect::>(); + + let mut names = BTreeSet::new(); + for workspace in db.list_workspaces()? { + if Some(workspace.id.as_str()) == destination_id { + continue; + } + for source in db.list_import_sources(&workspace.id)? { + let overlaps = db + .list_import_source_resources(&source.id)? + .iter() + .any(|row| incoming.contains(&row.source_key)); + if overlaps { + names.insert(workspace.name.clone()); + break; + } + } + } + + if names.is_empty() { + return Ok(()); + } + let names = names.iter().map(String::as_str).collect::>(); + plan.warnings.push(ImportPlanWarning::warning( + "Already imported", + format!("{} · importing here makes a second copy", display_list(&names)), + )); + Ok(()) +} + /// Commit a previously prepared plan in one transaction, applying only its selected items. pub fn commit_import_plan( query_manager: &QueryManager, @@ -654,10 +686,10 @@ fn resolve_linked_source( fn ambiguous_source_warning(sources: &[ImportSource]) -> ImportPlanWarning { let labels = sources.iter().map(|s| s.origin_label.as_str()).collect::>(); - ImportPlanWarning { - title: "Imported as new".to_string(), - detail: format!("{} already contain these resources", display_list(&labels)), - } + ImportPlanWarning::warning( + "Imported as new", + format!("{} already contain these resources", display_list(&labels)), + ) } /// Rewrite a plan against the destination's linked import source, if it has one: resources whose @@ -1671,7 +1703,6 @@ mod tests { ) .expect("plan first import"); assert_eq!(first.resources.workspaces[0].name, "Imported"); - assert!(first.warnings.iter().all(|w| w.title != "Workspace renamed")); commit_import_plan(&query_manager, first).expect("commit first import"); let second = plan_import_resources( @@ -1684,12 +1715,6 @@ mod tests { ) .expect("plan second import"); assert_eq!(second.resources.workspaces[0].name, "Imported (2)"); - let warning = second - .warnings - .iter() - .find(|w| w.title == "Workspace renamed") - .expect("the rename is explained"); - assert_eq!(warning.detail, "Imported → Imported (2) · that name is taken"); commit_import_plan(&query_manager, second).expect("commit second import"); let third = plan_import_resources( @@ -1714,6 +1739,38 @@ mod tests { assert_eq!(names, BTreeSet::from(["Imported".to_string(), "Imported (2)".to_string()])); } + #[test] + fn importing_a_document_that_already_has_a_workspace_warns() { + let (query_manager, _blob_manager, _rx) = + yaak_models::init_in_memory().expect("initialize database"); + let committed = first_import(&query_manager); + let workspace_id = committed.workspaces[0].id.clone(); + + let elsewhere = plan_import_resources( + &query_manager, + "OpenAPI".to_string(), + ImportDestination::NewWorkspace, + imported_resources(), + Some(importer_keys()), + Some(linked_origin()), + ) + .expect("plan a second copy"); + let warning = elsewhere + .warnings + .iter() + .find(|w| w.title == "Already imported") + .expect("copying a document that already landed somewhere is called out"); + assert_eq!(warning.detail, "Imported · importing here makes a second copy"); + + // Merging back into the workspace it created is the whole point, so it says nothing. + let merging = replan(&query_manager, &workspace_id, imported_resources()); + assert!( + merging.warnings.iter().all(|w| w.title != "Already imported"), + "{:?}", + merging.warnings + ); + } + #[test] fn environment_collisions_are_explicit_and_do_not_overwrite() { let (query_manager, _blob_manager, _rx) =