mirror of
https://github.com/mountain-loop/yaak.git
synced 2026-09-09 19:31:57 +02:00
feat(import): warn that a document already has a workspace
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 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
cdc034b25a
commit
213458b60c
@@ -335,6 +335,7 @@ function LoadedImportDataDialog({
|
|||||||
data: {
|
data: {
|
||||||
kind: "destination",
|
kind: "destination",
|
||||||
label: existing?.name ?? planned?.name ?? "New workspace",
|
label: existing?.name ?? planned?.name ?? "New workspace",
|
||||||
|
isNew: planDestination.type === "new_workspace",
|
||||||
},
|
},
|
||||||
children: itemTree,
|
children: itemTree,
|
||||||
};
|
};
|
||||||
@@ -371,7 +372,12 @@ function LoadedImportDataDialog({
|
|||||||
key={`${warning.title}:${warning.detail}`}
|
key={`${warning.title}:${warning.detail}`}
|
||||||
className="flex items-start gap-2.5 px-3 py-2.5"
|
className="flex items-start gap-2.5 px-3 py-2.5"
|
||||||
>
|
>
|
||||||
<Icon icon="info" color="info" size="sm" className="mt-0.5" />
|
<Icon
|
||||||
|
icon={warning.level === "warning" ? "alert_triangle" : "info"}
|
||||||
|
color={warning.level === "warning" ? "warning" : "info"}
|
||||||
|
size="sm"
|
||||||
|
className="mt-0.5"
|
||||||
|
/>
|
||||||
<div className="min-w-0">
|
<div className="min-w-0">
|
||||||
<div className="text-sm font-medium">{warning.title}</div>
|
<div className="text-sm font-medium">{warning.title}</div>
|
||||||
<div className="text-xs text-text-subtle mt-0.5">{warning.detail}</div>
|
<div className="text-xs text-text-subtle mt-0.5">{warning.detail}</div>
|
||||||
@@ -555,11 +561,15 @@ function ImportTreeRow({
|
|||||||
<>
|
<>
|
||||||
<Icon color="secondary" icon={row.kind === "destination" ? "house" : row.icon} />
|
<Icon color="secondary" icon={row.kind === "destination" ? "house" : row.icon} />
|
||||||
<div className="truncate flex-1">{row.label}</div>
|
<div className="truncate flex-1">{row.label}</div>
|
||||||
|
{row.kind === "destination" && row.isNew && (
|
||||||
|
<ActionChip label="new" help="Created by this import" className="text-success" />
|
||||||
|
)}
|
||||||
</>
|
</>
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
const { item } = row;
|
const { item } = row;
|
||||||
|
const label = actionLabel(item);
|
||||||
return (
|
return (
|
||||||
<>
|
<>
|
||||||
{item.model === "folder" || item.model === "environment" ? (
|
{item.model === "folder" || item.model === "environment" ? (
|
||||||
@@ -585,27 +595,47 @@ function ImportTreeRow({
|
|||||||
/>
|
/>
|
||||||
</div>
|
</div>
|
||||||
) : (
|
) : (
|
||||||
actionLabel(item) && (
|
label != null && (
|
||||||
<InlineCode
|
<ActionChip
|
||||||
|
label={label}
|
||||||
|
help={actionHelp(item)}
|
||||||
className={classNames(
|
className={classNames(
|
||||||
"py-0 bg-transparent w-32 shrink-0 whitespace-nowrap text-xs",
|
|
||||||
"inline-flex items-center justify-center gap-1.5",
|
|
||||||
item.action === "create" && "text-success",
|
item.action === "create" && "text-success",
|
||||||
item.action === "update" && "text-info",
|
item.action === "update" && "text-info",
|
||||||
item.action === "delete" && "text-danger",
|
item.action === "delete" && "text-danger",
|
||||||
item.action === "keep_local" && item.selected && "text-warning",
|
item.action === "keep_local" && item.selected && "text-warning",
|
||||||
item.action === "ignored" && "text-text-subtlest",
|
item.action === "ignored" && "text-text-subtlest",
|
||||||
)}
|
)}
|
||||||
>
|
/>
|
||||||
{actionLabel(item)}
|
|
||||||
<IconTooltip content={actionHelp(item)} iconSize="xs" />
|
|
||||||
</InlineCode>
|
|
||||||
)
|
)
|
||||||
)}
|
)}
|
||||||
</>
|
</>
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
function ActionChip({
|
||||||
|
label,
|
||||||
|
help,
|
||||||
|
className,
|
||||||
|
}: {
|
||||||
|
label: string;
|
||||||
|
help: string | null;
|
||||||
|
className?: string;
|
||||||
|
}) {
|
||||||
|
return (
|
||||||
|
<InlineCode
|
||||||
|
className={classNames(
|
||||||
|
"py-0 bg-transparent w-32 shrink-0 whitespace-nowrap text-xs",
|
||||||
|
"inline-flex items-center justify-center gap-1.5",
|
||||||
|
className,
|
||||||
|
)}
|
||||||
|
>
|
||||||
|
{label}
|
||||||
|
{help != null && <IconTooltip content={help} iconSize="xs" />}
|
||||||
|
</InlineCode>
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
function actionLabel(item: ImportPlanItem): string | null {
|
function actionLabel(item: ImportPlanItem): string | null {
|
||||||
switch (item.action) {
|
switch (item.action) {
|
||||||
case "create":
|
case "create":
|
||||||
@@ -673,7 +703,7 @@ function ancestorsOf(item: ImportPlanItem, byId: Map<string, ImportPlanItem>): I
|
|||||||
* themselves.
|
* themselves.
|
||||||
*/
|
*/
|
||||||
type TreeRow =
|
type TreeRow =
|
||||||
| { kind: "destination"; label: string }
|
| { kind: "destination"; label: string; isNew: boolean }
|
||||||
| { kind: "group"; label: string; icon: IconProps["icon"] }
|
| { kind: "group"; label: string; icon: IconProps["icon"] }
|
||||||
| { kind: "item"; item: ImportPlanItem };
|
| { kind: "item"; item: ImportPlanItem };
|
||||||
|
|
||||||
|
|||||||
+6
-1
@@ -88,7 +88,12 @@ export type ImportPlanItem = {
|
|||||||
*/
|
*/
|
||||||
export type ImportPlanReason = "moved_into_ignored_folder";
|
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.
|
* The model types an import plan can contain.
|
||||||
|
|||||||
Generated
+6
-1
@@ -88,7 +88,12 @@ export type ImportPlanItem = {
|
|||||||
*/
|
*/
|
||||||
export type ImportPlanReason = "moved_into_ignored_folder";
|
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.
|
* The model types an import plan can contain.
|
||||||
|
|||||||
@@ -109,6 +109,36 @@ pub enum ImportDestination {
|
|||||||
pub struct ImportPlanWarning {
|
pub struct ImportPlanWarning {
|
||||||
pub title: String,
|
pub title: String,
|
||||||
pub detail: 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<String>, detail: impl Into<String>) -> Self {
|
||||||
|
Self {
|
||||||
|
title: title.into(),
|
||||||
|
detail: detail.into(),
|
||||||
|
level: ImportPlanWarningLevel::Info,
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
pub fn warning(title: impl Into<String>, detail: impl Into<String>) -> 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.
|
/// Where an import's contents came from, used to link the committed workspace back to it.
|
||||||
|
|||||||
+91
-34
@@ -104,13 +104,7 @@ pub fn plan_import_resources(
|
|||||||
.collect::<Vec<_>>();
|
.collect::<Vec<_>>();
|
||||||
for workspace in &mut workspaces {
|
for workspace in &mut workspaces {
|
||||||
let unique = unique_name(&workspace.name, &taken);
|
let unique = unique_name(&workspace.name, &taken);
|
||||||
if unique != workspace.name {
|
workspace.name = unique.clone();
|
||||||
warnings.push(ImportPlanWarning {
|
|
||||||
title: "Workspace renamed".to_string(),
|
|
||||||
detail: format!("{} → {unique} · that name is taken", workspace.name),
|
|
||||||
});
|
|
||||||
workspace.name = unique.clone();
|
|
||||||
}
|
|
||||||
taken.push(unique);
|
taken.push(unique);
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -135,10 +129,10 @@ pub fn plan_import_resources(
|
|||||||
} else {
|
} else {
|
||||||
format!("{} imported workspaces", resources.workspaces.len())
|
format!("{} imported workspaces", resources.workspaces.len())
|
||||||
};
|
};
|
||||||
warnings.push(ImportPlanWarning {
|
warnings.push(ImportPlanWarning::info(
|
||||||
title: "Workspace settings skipped".to_string(),
|
"Workspace settings skipped",
|
||||||
detail: format!("{source} · {}", display_list(&skipped_fields)),
|
format!("{source} · {}", display_list(&skipped_fields)),
|
||||||
});
|
));
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
(workspace_id.clone(), folder_id.clone())
|
(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 {
|
for (source_name, imported_name, variable_count) in separated_base_environments {
|
||||||
let variables = if variable_count == 1 { "variable" } else { "variables" };
|
let variables = if variable_count == 1 { "variable" } else { "variables" };
|
||||||
warnings.push(ImportPlanWarning {
|
warnings.push(ImportPlanWarning::info(
|
||||||
title: "Base environment kept separate".to_string(),
|
"Base environment kept separate",
|
||||||
detail: format!("{source_name} → {imported_name} · {variable_count} {variables}"),
|
format!("{source_name} → {imported_name} · {variable_count} {variables}"),
|
||||||
});
|
));
|
||||||
}
|
}
|
||||||
if converted_duplicate_base_environment {
|
if converted_duplicate_base_environment {
|
||||||
warnings.push(ImportPlanWarning {
|
warnings.push(ImportPlanWarning::info(
|
||||||
title: "Base environments separated".to_string(),
|
"Base environments separated",
|
||||||
detail: "Only the first remains the base environment".to_string(),
|
"Only the first remains the base environment",
|
||||||
});
|
));
|
||||||
}
|
}
|
||||||
if converted_duplicate_folder_environment {
|
if converted_duplicate_folder_environment {
|
||||||
warnings.push(ImportPlanWarning {
|
warnings.push(ImportPlanWarning::info(
|
||||||
title: "Folder environments separated".to_string(),
|
"Folder environments separated",
|
||||||
detail: "Only the first remains attached to each folder".to_string(),
|
"Only the first remains attached to each folder",
|
||||||
});
|
));
|
||||||
}
|
}
|
||||||
|
|
||||||
let resources = BatchUpsertResult {
|
let resources = BatchUpsertResult {
|
||||||
@@ -302,9 +296,47 @@ pub fn plan_import_resources(
|
|||||||
origin,
|
origin,
|
||||||
};
|
};
|
||||||
merge_with_linked_source(query_manager, &mut plan, &original)?;
|
merge_with_linked_source(query_manager, &mut plan, &original)?;
|
||||||
|
warn_if_imported_elsewhere(&query_manager.connect(), &mut plan)?;
|
||||||
Ok(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::<BTreeSet<_>>();
|
||||||
|
|
||||||
|
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::<BTreeSet<_>>();
|
||||||
|
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.
|
/// Commit a previously prepared plan in one transaction, applying only its selected items.
|
||||||
pub fn commit_import_plan(
|
pub fn commit_import_plan(
|
||||||
query_manager: &QueryManager,
|
query_manager: &QueryManager,
|
||||||
@@ -654,10 +686,10 @@ fn resolve_linked_source(
|
|||||||
|
|
||||||
fn ambiguous_source_warning(sources: &[ImportSource]) -> ImportPlanWarning {
|
fn ambiguous_source_warning(sources: &[ImportSource]) -> ImportPlanWarning {
|
||||||
let labels = sources.iter().map(|s| s.origin_label.as_str()).collect::<BTreeSet<_>>();
|
let labels = sources.iter().map(|s| s.origin_label.as_str()).collect::<BTreeSet<_>>();
|
||||||
ImportPlanWarning {
|
ImportPlanWarning::warning(
|
||||||
title: "Imported as new".to_string(),
|
"Imported as new",
|
||||||
detail: format!("{} already contain these resources", display_list(&labels)),
|
format!("{} already contain these resources", display_list(&labels)),
|
||||||
}
|
)
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Rewrite a plan against the destination's linked import source, if it has one: resources whose
|
/// 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");
|
.expect("plan first import");
|
||||||
assert_eq!(first.resources.workspaces[0].name, "Imported");
|
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");
|
commit_import_plan(&query_manager, first).expect("commit first import");
|
||||||
|
|
||||||
let second = plan_import_resources(
|
let second = plan_import_resources(
|
||||||
@@ -1684,12 +1715,6 @@ mod tests {
|
|||||||
)
|
)
|
||||||
.expect("plan second import");
|
.expect("plan second import");
|
||||||
assert_eq!(second.resources.workspaces[0].name, "Imported (2)");
|
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");
|
commit_import_plan(&query_manager, second).expect("commit second import");
|
||||||
|
|
||||||
let third = plan_import_resources(
|
let third = plan_import_resources(
|
||||||
@@ -1714,6 +1739,38 @@ mod tests {
|
|||||||
assert_eq!(names, BTreeSet::from(["Imported".to_string(), "Imported (2)".to_string()]));
|
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]
|
#[test]
|
||||||
fn environment_collisions_are_explicit_and_do_not_overwrite() {
|
fn environment_collisions_are_explicit_and_do_not_overwrite() {
|
||||||
let (query_manager, _blob_manager, _rx) =
|
let (query_manager, _blob_manager, _rx) =
|
||||||
|
|||||||
Reference in New Issue
Block a user