mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-06 08:01:35 +00:00
fix: stop reading a parent-only fork item as deleted in the fork (#10467)
* fix: stop reading a parent-only fork item as deleted in the fork Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * style: condense the deploy-direction helper comments Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: keep the ambiguous half of a one-sided diff out of bulk defaults Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: disable select-all on a removal-only list and cover the hidden source-only row Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: keep parent-only items out of the fork merge list entirely Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: count the fork banner's ahead/behind with the compare page's predicate Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: open the direction the fork banner's button offers Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore: cache the sqlx query for the source-only visibility test Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: don't read an unloaded comparison as nothing to deploy Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: treat an in-flight comparison as unknown in the fork banner Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Ruben Fiszel
co-authored by
Claude Opus 5
Claude Opus 5
parent
0827fd285b
commit
689f5d7c75
+12
@@ -0,0 +1,12 @@
|
||||
{
|
||||
"db_name": "PostgreSQL",
|
||||
"query": "INSERT INTO workspace_diff\n (source_workspace_id, fork_workspace_id, path, kind, ahead, behind, has_changes, exists_in_source, exists_in_fork)\n VALUES ('test-workspace', 'wm-fork-test-workspace', 'f/rt/parent_only', 'http_trigger', 1, 0, true, true, false)",
|
||||
"describe": {
|
||||
"columns": [],
|
||||
"parameters": {
|
||||
"Left": []
|
||||
},
|
||||
"nullable": []
|
||||
},
|
||||
"hash": "aba0caddc216e218225d41cc3e14db0d8013ca448ca469f1c8c90965a514a8fb"
|
||||
}
|
||||
@@ -1754,6 +1754,83 @@ async fn test_compare_workspaces_phantom_trigger_shortfuse(
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// A source-only row is offered to the fork whatever its counters say, so it can
|
||||
/// carry `behind = 0` — a `behind`-derived tally never sees it, and hiding one must
|
||||
/// still be reported to the update direction. The merge direction does not carry
|
||||
/// such a row at all, so hiding one withholds nothing from that side.
|
||||
#[sqlx::test(migrations = "../migrations", fixtures("base"))]
|
||||
async fn test_compare_workspaces_hidden_source_only_no_behind(
|
||||
db: Pool<Postgres>,
|
||||
) -> anyhow::Result<()> {
|
||||
initialize_tracing().await;
|
||||
|
||||
let server = ApiServer::start(db.clone()).await?;
|
||||
let port = server.addr.port();
|
||||
let base_url = format!("http://localhost:{port}/api");
|
||||
let non_admin = windmill_api_client::create_client(
|
||||
&format!("http://localhost:{port}"),
|
||||
"SECRET_TOKEN_2".to_string(),
|
||||
);
|
||||
|
||||
sqlx::query!(
|
||||
"INSERT INTO workspace (id, name, owner, parent_workspace_id)
|
||||
VALUES ('wm-fork-test-workspace', 'Fork', 'test-user', 'test-workspace')"
|
||||
)
|
||||
.execute(&db)
|
||||
.await?;
|
||||
sqlx::query!("INSERT INTO workspace_settings (workspace_id) VALUES ('wm-fork-test-workspace')")
|
||||
.execute(&db)
|
||||
.await?;
|
||||
sqlx::query!(
|
||||
"INSERT INTO workspace_key(workspace_id, kind, key)
|
||||
VALUES ('wm-fork-test-workspace', 'cloud', 'test-key')"
|
||||
)
|
||||
.execute(&db)
|
||||
.await?;
|
||||
|
||||
// Source-only row with no `behind`: the trigger has no backing row, so the
|
||||
// visibility filter drops it exactly as it would an ACL-hidden one.
|
||||
sqlx::query!(
|
||||
"INSERT INTO workspace_diff
|
||||
(source_workspace_id, fork_workspace_id, path, kind, ahead, behind, has_changes, exists_in_source, exists_in_fork)
|
||||
VALUES ('test-workspace', 'wm-fork-test-workspace', 'f/rt/parent_only', 'http_trigger', 1, 0, true, true, false)"
|
||||
)
|
||||
.execute(&db)
|
||||
.await?;
|
||||
|
||||
let comparison: serde_json::Value = non_admin
|
||||
.client()
|
||||
.get(&format!(
|
||||
"{base_url}/w/test-workspace/workspaces/compare/wm-fork-test-workspace"
|
||||
))
|
||||
.send()
|
||||
.await?
|
||||
.json()
|
||||
.await?;
|
||||
assert_eq!(
|
||||
comparison["all_behind_items_visible"].as_bool(),
|
||||
Some(false),
|
||||
"a hidden source-only row must trip the warning even at behind = 0: {comparison}"
|
||||
);
|
||||
assert_eq!(
|
||||
comparison["hidden_behind"]["total"].as_i64(),
|
||||
Some(1),
|
||||
"a hidden source-only row must be counted as withheld from the update direction: {comparison}"
|
||||
);
|
||||
assert_eq!(
|
||||
comparison["all_ahead_items_visible"].as_bool(),
|
||||
Some(true),
|
||||
"the merge direction does not carry a source-only row, so hiding one withholds nothing from it: {comparison}"
|
||||
);
|
||||
assert_eq!(
|
||||
comparison["hidden_ahead"]["total"].as_i64(),
|
||||
Some(0),
|
||||
"a source-only row must not be reported as withheld from the merge direction: {comparison}"
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Regression: the "sees everything" guard must require admin of BOTH sides, not
|
||||
/// just the fork. `filter_visible_diffs` keeps a modified/conflict row (one that
|
||||
/// exists in the source AND the fork) only when the caller can see it on both
|
||||
|
||||
@@ -9585,16 +9585,28 @@ async fn compare_workspaces(
|
||||
.count(),
|
||||
};
|
||||
|
||||
let all_ahead_items_visible = summary.total_ahead
|
||||
== confirmed_diffs
|
||||
.iter()
|
||||
// Each direction accounts for what it carries (see the frontend's
|
||||
// `diffActionableInDirection`): a lineage merge leaves out a row the fork lacks,
|
||||
// while the update takes it whatever the counters say — and since it can carry
|
||||
// `behind = 0`, that side counts rows rather than sums.
|
||||
let source_only = |d: &WorkspaceDiffRow| {
|
||||
d.exists_in_source.unwrap_or(false) && !d.exists_in_fork.unwrap_or(false)
|
||||
};
|
||||
let merge_carries = |d: &WorkspaceDiffRow| !is_lineage_pair || !source_only(d);
|
||||
let ahead_sum = |rows: &[WorkspaceDiffRow]| {
|
||||
rows.iter()
|
||||
.filter(|d| merge_carries(d))
|
||||
.map(|s| s.ahead)
|
||||
.fold(0, |acc, s| acc + s.try_into().unwrap_or(0));
|
||||
.fold(0i64, |acc, s| acc + i64::from(s))
|
||||
};
|
||||
let all_ahead_items_visible = ahead_sum(&visible_diffs) == ahead_sum(&confirmed_diffs);
|
||||
let all_behind_items_visible = summary.total_behind
|
||||
== confirmed_diffs
|
||||
.iter()
|
||||
.map(|s| s.behind)
|
||||
.fold(0, |acc, s| acc + s.try_into().unwrap_or(0));
|
||||
.fold(0, |acc, s| acc + s.try_into().unwrap_or(0))
|
||||
&& visible_diffs.iter().filter(|d| source_only(d)).count()
|
||||
== confirmed_diffs.iter().filter(|d| source_only(d)).count();
|
||||
|
||||
// Blast-radius guard for the "changes not visible to your user" warning
|
||||
// (which hides the deploy button). The flag is a pure visibility guarantee —
|
||||
@@ -9634,7 +9646,9 @@ async fn compare_workspaces(
|
||||
if visible_keys.contains(&(d.kind.as_str(), d.path.as_str())) {
|
||||
continue;
|
||||
}
|
||||
if d.ahead > 0 {
|
||||
// Both sides mirror the flags above: a row is only withheld from a direction
|
||||
// that would have carried it.
|
||||
if d.ahead > 0 && merge_carries(d) {
|
||||
hidden_ahead.total += 1;
|
||||
*hidden_ahead.by_kind.entry(d.kind.clone()).or_default() += 1;
|
||||
if sees_all_items {
|
||||
@@ -9643,7 +9657,7 @@ async fn compare_workspaces(
|
||||
.push(HiddenItem { kind: d.kind.clone(), path: d.path.clone() });
|
||||
}
|
||||
}
|
||||
if d.behind > 0 {
|
||||
if d.behind > 0 || source_only(d) {
|
||||
hidden_behind.total += 1;
|
||||
*hidden_behind.by_kind.entry(d.kind.clone()).or_default() += 1;
|
||||
if sees_all_items {
|
||||
|
||||
@@ -43,6 +43,9 @@
|
||||
checkDeployPermission,
|
||||
deployItem,
|
||||
deleteItemInWorkspace,
|
||||
diffActionableInDirection,
|
||||
diffCreatesInTarget,
|
||||
diffRemovesInTarget,
|
||||
getItemValue,
|
||||
getOnBehalfOf,
|
||||
type DeployPermission,
|
||||
@@ -407,13 +410,9 @@
|
||||
let canPreserveOnBehalfOf = $derived(mergeIntoParent ? canPreserveInParent : canPreserveInCurrent)
|
||||
|
||||
let selectableDiffs = $derived(
|
||||
comparison?.diffs.filter((diff) => {
|
||||
if (mergeIntoParent) {
|
||||
return diff.ahead > 0
|
||||
} else {
|
||||
return diff.behind > 0
|
||||
}
|
||||
}) ?? []
|
||||
comparison?.diffs.filter((diff) =>
|
||||
diffActionableInDirection(diff, mergeIntoParent, isArbitraryTarget)
|
||||
) ?? []
|
||||
)
|
||||
|
||||
let selectedItems = $state<string[]>([])
|
||||
@@ -457,10 +456,17 @@
|
||||
}) ?? []
|
||||
)
|
||||
|
||||
// Gates the "update before deploying" alert, so it counts what a merge would
|
||||
// actually carry — not every row with an `ahead` counter, which includes the
|
||||
// parent-only ones the merge direction leaves out.
|
||||
let itemsWithAheadChanges = $derived(
|
||||
comparison?.diffs.filter((diff) => {
|
||||
const status = deploymentStatus[getItemKey(diff)]?.status
|
||||
return diff && diff.ahead > 0 && !(status && status == 'deployed')
|
||||
return (
|
||||
diff &&
|
||||
diffActionableInDirection(diff, true, isArbitraryTarget) &&
|
||||
!(status && status == 'deployed')
|
||||
)
|
||||
}) ?? []
|
||||
)
|
||||
|
||||
@@ -477,6 +483,7 @@
|
||||
let onBehalfOfChoice = $state<Record<string, OnBehalfOfChoice>>({})
|
||||
let customOnBehalfOf = $state<Record<string, OnBehalfOfDetails>>({})
|
||||
let deployTargetWorkspace = $derived(mergeIntoParent ? parentWorkspaceId : currentWorkspaceId)
|
||||
let deploySourceWorkspace = $derived(mergeIntoParent ? currentWorkspaceId : parentWorkspaceId)
|
||||
|
||||
function getItemKey(diff: WorkspaceItemDiff): string {
|
||||
return `${diff.kind}:${diff.path}`
|
||||
@@ -641,12 +648,10 @@
|
||||
|
||||
// All *diff* items selected. Trigger items are opt-in and don't count
|
||||
// toward "all selected" — see item merge below in deployableItems.
|
||||
// Deploying a row the current workspace lacks deletes it in the target. Against
|
||||
// the parent that is a real deletion to propagate and is selected like anything
|
||||
// else; against an arbitrary target the two workspaces were simply never in sync,
|
||||
// so it takes an explicit act on that row. No bulk action may sweep one in.
|
||||
// A row whose deploy deletes in the target takes an explicit tick (see
|
||||
// `diffRemovesInTarget`); no bulk action may sweep one in.
|
||||
function removesInTarget(diff: WorkspaceItemDiff): boolean {
|
||||
return isArbitraryTarget && diff.exists_in_fork === false
|
||||
return diffRemovesInTarget(diff, mergeIntoParent)
|
||||
}
|
||||
let bulkSelectableDiffs = $derived(selectableDiffs.filter((d) => !removesInTarget(d)))
|
||||
|
||||
@@ -691,17 +696,14 @@
|
||||
) {
|
||||
deploymentStatus[statusKey] = { status: 'loading' }
|
||||
|
||||
// Check if the item was deleted in the source workspace.
|
||||
// If so, archive/delete it in the target workspace instead of copying.
|
||||
// The workspace this deploy reads from doesn't have the item: archive/delete
|
||||
// it in the target instead of copying. Same predicate as the row's badge and
|
||||
// its exclusion from bulk selection, so what the row says is what it does.
|
||||
const diff = comparison?.diffs.find((d) => getItemKey(d) === statusKey)
|
||||
const itemDeletedInSource = diff
|
||||
? mergeIntoParent
|
||||
? diff.exists_in_fork === false
|
||||
: diff.exists_in_source === false
|
||||
: false
|
||||
const removes = diff ? removesInTarget(diff) : false
|
||||
|
||||
let result: DeployResult
|
||||
if (itemDeletedInSource) {
|
||||
if (removes) {
|
||||
result = await deleteItemInWorkspace(kind, path, workspaceToDeployTo)
|
||||
} else {
|
||||
result = await deployItem({
|
||||
@@ -899,9 +901,15 @@
|
||||
const filtered = bulkSelectableDiffs.filter(
|
||||
(d) => !isTriggerOrScheduleKind(d.kind) && !hasDraft(d)
|
||||
)
|
||||
// The update direction leaves out two ambiguous shapes, both still one click
|
||||
// away through "Select all": a conflict, and a parent-only row the fork has
|
||||
// deploy events for — the fork may have dropped it on purpose, and a routine
|
||||
// update must not silently bring it back.
|
||||
const conflictSafe = mergeIntoParent
|
||||
? filtered
|
||||
: filtered.filter((d) => !(d.ahead > 0 && d.behind > 0))
|
||||
: filtered.filter(
|
||||
(d) => !(d.ahead > 0 && d.behind > 0) && !(d.ahead > 0 && diffCreatesInTarget(d, false))
|
||||
)
|
||||
// When reached from a session's Review, narrow the default to this chat's items.
|
||||
const scoped = chatMask ? conflictSafe.filter((d) => diffInMask(d, chatMask)) : conflictSafe
|
||||
selectedItems = scoped
|
||||
@@ -987,6 +995,10 @@
|
||||
let removalKeys = new Set<string>()
|
||||
$effect(() => {
|
||||
const diffs = comparison?.diffs
|
||||
// Removals are direction-dependent, so a flip must refresh the set: keeping the
|
||||
// other direction's would make every row of the new one look freshly flipped
|
||||
// and revoke the opt-in this guard protects.
|
||||
;[mergeIntoParent]
|
||||
if (!diffs) return
|
||||
untrack(() => {
|
||||
const current = new Set(diffs.filter(removesInTarget).map((d) => getItemKey(d)))
|
||||
@@ -1565,39 +1577,20 @@
|
||||
<AlertTriangle class="w-3 h-3 inline mr-0.5" />+Draft
|
||||
</Badge>
|
||||
{/if}
|
||||
<!-- Status badges -->
|
||||
{#if !diff.exists_in_fork && diff.exists_in_source && diff.ahead == 0 && diff.behind > 0}
|
||||
<!-- Status badges. An item on one side only states the effect of
|
||||
deploying it, never which side moved — the comparison cannot know
|
||||
(see `diffRemovesInTarget`). -->
|
||||
{#if diffCreatesInTarget(diff, mergeIntoParent)}
|
||||
<Badge
|
||||
title="This item was newly created in the parent workspace '{parentWorkspaceId}'"
|
||||
title="This item exists in '{deploySourceWorkspace}' but not in '{deployTargetWorkspace}' — deploying it creates it there"
|
||||
color="indigo"
|
||||
size="xs">New</Badge
|
||||
>
|
||||
{/if}
|
||||
{#if !diff.exists_in_fork && diff.exists_in_source && diff.ahead > 0}
|
||||
<!-- Same row, two readings: against the parent the fork deleted the
|
||||
item, while against an arbitrary target it may simply never have
|
||||
existed here. Deploying removes it there either way — say that
|
||||
rather than asserting a deletion that may not have happened. -->
|
||||
{:else if removesInTarget(diff)}
|
||||
<Badge
|
||||
title={isArbitraryTarget
|
||||
? `This item exists only in '${parentWorkspaceId}' — deploying it removes it there`
|
||||
: `This item was deleted in '${currentWorkspaceId}'`}
|
||||
title="This item exists in '{deployTargetWorkspace}' but not in '{deploySourceWorkspace}' — deploying it removes it there"
|
||||
color="red"
|
||||
size="xs">{isArbitraryTarget ? 'Removes in target' : 'Deleted'}</Badge
|
||||
>
|
||||
{/if}
|
||||
{#if diff.exists_in_fork && !diff.exists_in_source && diff.behind > 0}
|
||||
<Badge
|
||||
title="This item was deleted in the parent workspace '{parentWorkspaceId}'"
|
||||
color="red"
|
||||
size="xs">Deleted</Badge
|
||||
>
|
||||
{/if}
|
||||
{#if diff.exists_in_fork && !diff.exists_in_source && diff.ahead > 0 && diff.behind == 0}
|
||||
<Badge
|
||||
title="This item was newly created in '{currentWorkspaceId}'"
|
||||
color="indigo"
|
||||
size="xs">New</Badge
|
||||
size="xs">Removes in {deployTargetWorkspace}</Badge
|
||||
>
|
||||
{/if}
|
||||
{@const ciStatus = getCiTestStatus(diff)}
|
||||
|
||||
@@ -9,6 +9,7 @@
|
||||
import { onMount, untrack } from 'svelte'
|
||||
import { useWorkspaceDrafts } from '$lib/workspaceDrafts.svelte'
|
||||
import { devLabelWord } from '$lib/utils/devWorkspaceLabel'
|
||||
import { diffActionableInDirection } from '$lib/utils_workspace_deploy'
|
||||
|
||||
let loading = $state(false)
|
||||
let comparison: WorkspaceComparison | undefined = $state(undefined)
|
||||
@@ -82,9 +83,14 @@
|
||||
}
|
||||
}
|
||||
|
||||
// Opens the direction the button offers, so the label and the list agree: a fork
|
||||
// with nothing to deploy lands on the update side, not on an empty deploy list.
|
||||
// Both read `comparisonLoaded` — an unknown comparison counts zero of everything,
|
||||
// which is indistinguishable from "nothing to deploy".
|
||||
function openComparisonDrawer() {
|
||||
if (parentWorkspaceId && $workspaceStore) {
|
||||
goto('/forks/compare?workspace_id=' + encodeURIComponent($workspaceStore), {
|
||||
const dir = comparisonLoaded && changesAhead === 0 ? '&dir=update' : ''
|
||||
goto('/forks/compare?workspace_id=' + encodeURIComponent($workspaceStore) + dir, {
|
||||
replaceState: true
|
||||
})
|
||||
}
|
||||
@@ -148,6 +154,19 @@
|
||||
return () => clearInterval(interval)
|
||||
})
|
||||
|
||||
// Counted with the compare page's own predicate so the banner never advertises a
|
||||
// direction whose list is empty: the `ahead`/`behind` sums in the summary include
|
||||
// rows a direction does not carry, and miss a parent-only row that the update
|
||||
// direction carries at `behind = 0`.
|
||||
function countDir(c: WorkspaceComparison | undefined, mergeIntoParent: boolean): number {
|
||||
return c?.diffs.filter((d) => diffActionableInDirection(d, mergeIntoParent)).length ?? 0
|
||||
}
|
||||
// A comparison in flight is not an answer: on a fork switch the component stays
|
||||
// mounted and `comparison` still holds the previous fork's rows.
|
||||
const comparisonLoaded = $derived(!loading && comparison !== undefined)
|
||||
const changesAhead = $derived(countDir(comparison, true))
|
||||
const changesBehind = $derived(countDir(comparison, false))
|
||||
|
||||
function forkAheadBehindMessage(changesAhead: number, changesBehind: number) {
|
||||
let msg: string[] = []
|
||||
if (changesAhead > 0 || changesBehind > 0) {
|
||||
@@ -195,10 +214,7 @@
|
||||
<div class="flex items-center flex-wrap gap-x-4 gap-y-1 text-xs min-w-0">
|
||||
{#if comparison.summary.total_diffs > 0}
|
||||
<span class="text-blue-700 dark:text-blue-100">
|
||||
{forkAheadBehindMessage(
|
||||
comparison.summary.total_ahead,
|
||||
comparison.summary.total_behind
|
||||
)}
|
||||
{forkAheadBehindMessage(changesAhead, changesBehind)}
|
||||
<span class="font-semibold underline">{parentWorkspaceId}</span> over {comparison
|
||||
.summary.total_diffs} items<span class="hidden lg:inline">:</span>
|
||||
</span>
|
||||
@@ -343,7 +359,7 @@
|
||||
>
|
||||
{#if showDraftsOnly}
|
||||
Review & deploy drafts
|
||||
{:else if (comparison?.summary.total_ahead ?? 0) > 0}
|
||||
{:else if !comparisonLoaded || changesAhead > 0}
|
||||
Review & Deploy Changes
|
||||
{:else}
|
||||
Review & Update fork
|
||||
|
||||
@@ -85,7 +85,10 @@
|
||||
}: Props = $props()
|
||||
|
||||
let selectableItems = $derived(items.filter(selectablePredicate))
|
||||
let hasSelectableItems = $derived(selectableItems.length > 0)
|
||||
// "Select all" is a bulk action, so it lives or dies by the bulk-selectable set:
|
||||
// a list of nothing but bulk-excluded rows would otherwise offer an enabled
|
||||
// control that selects nothing. Each such row is still selectable on its own.
|
||||
let hasSelectableItems = $derived(selectableItems.some((i) => !bulkExcluded(i)))
|
||||
|
||||
// Plain row click and the checkbox both toggle this row in/out — multi-select
|
||||
// is the default, no modifier needed.
|
||||
@@ -195,6 +198,13 @@
|
||||
{#if showGroupHeaders}
|
||||
{@const selectable = groupSelectable(group)}
|
||||
{@const selectedCount = selectable.filter((i) => selectedItems.includes(i.key)).length}
|
||||
<!-- The label counts every ticked row, bulk-excluded ones included: those
|
||||
are deployed like any other, and reporting only the bulk-selectable
|
||||
ones would contradict the deploy button. The checkbox above keeps its
|
||||
own count, which must ignore them to reach a full-checked state. -->
|
||||
{@const selectedInGroup = group.items.filter((i) =>
|
||||
selectedItems.includes(i.key)
|
||||
).length}
|
||||
<!-- The disabled-state hint lives on the row: a disabled Checkbox is
|
||||
pointer-events-none, so a title on the input would never show. -->
|
||||
<div
|
||||
@@ -219,8 +229,8 @@
|
||||
{/if}
|
||||
<span class="text-xs font-semibold text-secondary truncate">{group.label}</span>
|
||||
<span class="text-2xs text-tertiary whitespace-nowrap">
|
||||
{group.items.length} item{group.items.length !== 1 ? 's' : ''}{selectedCount > 0
|
||||
? ` · ${selectedCount} selected`
|
||||
{group.items.length} item{group.items.length !== 1 ? 's' : ''}{selectedInGroup > 0
|
||||
? ` · ${selectedInGroup} selected`
|
||||
: ''}
|
||||
</span>
|
||||
{#if groupActions}
|
||||
|
||||
@@ -6014,8 +6014,8 @@ function formatForkIndexEntry(e: ForkDiffEntryView): string {
|
||||
switch (e.status) {
|
||||
case 'only_in_fork':
|
||||
return `- ${name} — only in fork (${e.patchLineCount} lines)${draftFlag}`
|
||||
case 'deleted_in_fork':
|
||||
return `- ${name} — deleted in fork, still in parent${draftFlag}`
|
||||
case 'only_in_parent':
|
||||
return `- ${name} — only in parent, not in fork${draftFlag}`
|
||||
case 'modified':
|
||||
return `- ${name} — differs (${aheadBehind}; ${e.patchLineCount} diff lines)${draftFlag}`
|
||||
case 'unchanged':
|
||||
@@ -6193,8 +6193,8 @@ function renderForkEntrySection(
|
||||
const header =
|
||||
entry.status === 'only_in_fork'
|
||||
? `${entry.kind} "${path}" exists only in the fork — not in parent "${parent}". Full content:\n\n`
|
||||
: entry.status === 'deleted_in_fork'
|
||||
? `${entry.kind} "${path}" was deleted in the fork but still exists in parent "${parent}". Removed content:\n\n`
|
||||
: entry.status === 'only_in_parent'
|
||||
? `${entry.kind} "${path}" exists only in parent "${parent}" — not in the fork. Parent content:\n\n`
|
||||
: `Fork changes vs parent "${parent}" for ${entry.kind} "${path}":\n\n`
|
||||
if (args.file !== undefined && !entry.files) {
|
||||
throw new Error(
|
||||
|
||||
@@ -465,7 +465,7 @@ describe('fork mode', () => {
|
||||
expect(byPath['f/a/b'].status).toBe('modified')
|
||||
expect(byPath['f/a/new'].status).toBe('only_in_fork')
|
||||
expect(byPath['f/a/new'].patch).not.toContain('parent-ws')
|
||||
expect(byPath['f/a/gone'].status).toBe('deleted_in_fork')
|
||||
expect(byPath['f/a/gone'].status).toBe('only_in_parent')
|
||||
// One-sided entries fetch only the existing side: 2 + 1 + 1 calls.
|
||||
expect(getItemValue).toHaveBeenCalledTimes(4)
|
||||
})
|
||||
|
||||
@@ -620,7 +620,7 @@ const FORK_COMPARISON_REUSE_MS = 30_000
|
||||
export type ForkDiffStatus =
|
||||
| 'modified'
|
||||
| 'only_in_fork'
|
||||
| 'deleted_in_fork'
|
||||
| 'only_in_parent'
|
||||
| 'unchanged'
|
||||
| 'pending'
|
||||
| 'error'
|
||||
@@ -657,7 +657,7 @@ export interface ForkDiffIndexView {
|
||||
}
|
||||
|
||||
interface ForkMaterialized {
|
||||
status: 'modified' | 'only_in_fork' | 'deleted_in_fork' | 'unchanged' | 'error'
|
||||
status: 'modified' | 'only_in_fork' | 'only_in_parent' | 'unchanged' | 'error'
|
||||
patch: string
|
||||
lineCount: number
|
||||
files?: Record<string, DiffFileView>
|
||||
@@ -1023,7 +1023,7 @@ async function materializeFork(
|
||||
const forkValue = forkSide?.value
|
||||
const valueMasked = parentSide?.valueMasked === true || forkSide?.valueMasked === true
|
||||
const oneSidedStatus = !entry.existsInFork
|
||||
? 'deleted_in_fork'
|
||||
? 'only_in_parent'
|
||||
: !entry.existsInParent
|
||||
? 'only_in_fork'
|
||||
: undefined
|
||||
|
||||
@@ -0,0 +1,41 @@
|
||||
import { describe, it, expect } from 'vitest'
|
||||
import {
|
||||
diffActionableInDirection,
|
||||
diffCreatesInTarget,
|
||||
diffRemovesInTarget
|
||||
} from './utils_workspace_deploy'
|
||||
|
||||
/** The row shape the fork comparison returns for an item the parent has and the
|
||||
* fork does not. `ahead = 1, behind = 0` is what the tally leaves after the fork
|
||||
* pulled the parent's item in and then lost it (a delete, a git-sync revert):
|
||||
* every deploy event in the fork counts as `ahead`, whatever wrote it. */
|
||||
const parentOnly = { ahead: 1, behind: 0, exists_in_source: true, exists_in_fork: false }
|
||||
const forkOnly = { ahead: 1, behind: 0, exists_in_source: false, exists_in_fork: true }
|
||||
const bothSides = { ahead: 1, behind: 1, exists_in_source: true, exists_in_fork: true }
|
||||
|
||||
describe('deploy direction of a one-sided diff row', () => {
|
||||
it('offers a parent-only item to the fork even with no behind count', () => {
|
||||
expect(diffActionableInDirection(parentOnly, false)).toBe(true)
|
||||
expect(diffCreatesInTarget(parentOnly, false)).toBe(true)
|
||||
expect(diffRemovesInTarget(parentOnly, false)).toBe(false)
|
||||
})
|
||||
|
||||
it('keeps a parent-only row out of a merge into the parent, whatever its ahead count', () => {
|
||||
expect(diffActionableInDirection(parentOnly, true)).toBe(false)
|
||||
// An arbitrary target has no tally behind it and does propagate the removal.
|
||||
expect(diffActionableInDirection(parentOnly, true, true)).toBe(true)
|
||||
expect(diffRemovesInTarget(parentOnly, true)).toBe(true)
|
||||
})
|
||||
|
||||
it('does not resurrect a fork-only item into an update of the fork', () => {
|
||||
expect(diffActionableInDirection(forkOnly, false)).toBe(false)
|
||||
expect(diffCreatesInTarget(forkOnly, true)).toBe(true)
|
||||
})
|
||||
|
||||
it('keeps a two-sided row on its counters', () => {
|
||||
expect(diffActionableInDirection(bothSides, true)).toBe(true)
|
||||
expect(diffActionableInDirection({ ...bothSides, behind: 0 }, false)).toBe(false)
|
||||
expect(diffCreatesInTarget(bothSides, true)).toBe(false)
|
||||
expect(diffRemovesInTarget(bothSides, false)).toBe(false)
|
||||
})
|
||||
})
|
||||
@@ -354,6 +354,53 @@ export async function deployItem(params: DeployItemParams): Promise<DeployResult
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* The two sides of a `workspace_diff` row a deploy direction reads:
|
||||
* `exists_in_source` is the parent (or arbitrary target) side, `exists_in_fork`
|
||||
* the current workspace.
|
||||
*/
|
||||
type WorkspaceDiffSides = {
|
||||
ahead: number
|
||||
behind: number
|
||||
exists_in_source: boolean
|
||||
exists_in_fork: boolean
|
||||
}
|
||||
|
||||
/** Deploying this row creates the item in the target, which does not have it. */
|
||||
export function diffCreatesInTarget(diff: WorkspaceDiffSides, mergeIntoParent: boolean): boolean {
|
||||
return mergeIntoParent ? diff.exists_in_source === false : diff.exists_in_fork === false
|
||||
}
|
||||
|
||||
/**
|
||||
* Deploying this row removes the item in the target, the only side that has it.
|
||||
* Which side dropped it is unknowable — `ahead`/`behind` count deploy events on a
|
||||
* side, and a pull into the fork or a git-sync revert leaves the trace a delete
|
||||
* does — so a row states what deploying does, and a removal is never bulk-selected.
|
||||
*/
|
||||
export function diffRemovesInTarget(diff: WorkspaceDiffSides, mergeIntoParent: boolean): boolean {
|
||||
return mergeIntoParent ? diff.exists_in_fork === false : diff.exists_in_source === false
|
||||
}
|
||||
|
||||
/**
|
||||
* Rows a deploy in this direction can act on. A merge carries what the fork *has*:
|
||||
* an item only the parent has is no fork change (see `diffRemovesInTarget`), while
|
||||
* the update direction takes it whatever the counters say. Only an arbitrary target
|
||||
* merges one — that one-way sync has no tally, so target-only does mean "remove".
|
||||
*/
|
||||
export function diffActionableInDirection(
|
||||
diff: WorkspaceDiffSides,
|
||||
mergeIntoParent: boolean,
|
||||
isArbitraryTarget: boolean = false
|
||||
): boolean {
|
||||
if (mergeIntoParent) {
|
||||
if (!isArbitraryTarget && diff.exists_in_fork === false) {
|
||||
return false
|
||||
}
|
||||
return diff.ahead > 0
|
||||
}
|
||||
return diff.behind > 0 || diffCreatesInTarget(diff, mergeIntoParent)
|
||||
}
|
||||
|
||||
/**
|
||||
* Delete/archive an item in a workspace.
|
||||
* Used when deploying a deletion from one workspace to another.
|
||||
|
||||
@@ -10,6 +10,7 @@
|
||||
reconcileAfterWorkspaceChange
|
||||
} from '$lib/components/sessions/sessionState.svelte'
|
||||
import { useWorkspaceDrafts } from '$lib/workspaceDrafts.svelte'
|
||||
import { diffActionableInDirection } from '$lib/utils_workspace_deploy'
|
||||
import { page } from '$app/state'
|
||||
import { userWorkspaces, workspaceStore } from '$lib/stores'
|
||||
import { onDestroy, untrack } from 'svelte'
|
||||
@@ -65,7 +66,11 @@
|
||||
// Which fork direction to restore when switching back from draft mode. The
|
||||
// merged toggle (CompareModeToggle, rendered inside each card) reports its
|
||||
// selection here; the page only swaps which comparison component is shown.
|
||||
let forkDirection = $state<'deploy_to' | 'update'>('deploy_to')
|
||||
// `?dir=update` opens on the other one, for callers that already know which
|
||||
// direction has something in it (the fork banner's CTA).
|
||||
let forkDirection = $state<'deploy_to' | 'update'>(
|
||||
page.url.searchParams.get('dir') === 'update' ? 'update' : 'deploy_to'
|
||||
)
|
||||
|
||||
// Explicit preselection via `?items=<kind:path,...>` (built by the chat's
|
||||
// open_page tool). Parsed synchronously from the live URL so it can never race
|
||||
@@ -149,17 +154,21 @@
|
||||
)
|
||||
|
||||
// Per-direction counts for the merged toggle badges. Deployable = items ahead
|
||||
// (fork has changes the parent lacks); updateable = items behind. Computed
|
||||
// here so they show on the toggle in draft mode too (where CompareDrafts has
|
||||
// no comparison data of its own). Typed helpers avoid a $state `never`
|
||||
// inference quirk on `comparison` inside $derived. A conflict (ahead AND
|
||||
// behind) is intentionally counted in both directions — it's actionable either
|
||||
// way.
|
||||
function countDir(c: WorkspaceComparison | undefined, dir: 'ahead' | 'behind'): number {
|
||||
return c?.diffs.filter((d) => d[dir] > 0).length ?? 0
|
||||
// (fork has changes the parent lacks); updateable = items behind, plus what the
|
||||
// parent has and the fork does not. Same predicate as the deploy list, so the
|
||||
// badge never counts rows the list won't show. Computed here so they show on the
|
||||
// toggle in draft mode too (where CompareDrafts has no comparison data of its
|
||||
// own). Typed helpers avoid a $state `never` inference quirk on `comparison`
|
||||
// inside $derived. A conflict (ahead AND behind) is intentionally counted in both
|
||||
// directions — it's actionable either way.
|
||||
function countDir(c: WorkspaceComparison | undefined, mergeIntoParent: boolean): number {
|
||||
return (
|
||||
c?.diffs.filter((d) => diffActionableInDirection(d, mergeIntoParent, isArbitraryTarget))
|
||||
.length ?? 0
|
||||
)
|
||||
}
|
||||
const deployCount = $derived(countDir(comparison, 'ahead'))
|
||||
const updateCount = $derived(countDir(comparison, 'behind'))
|
||||
const deployCount = $derived(countDir(comparison, true))
|
||||
const updateCount = $derived(countDir(comparison, false))
|
||||
|
||||
$effect(() => {
|
||||
if (modeResolved || !currentWorkspaceData) return
|
||||
|
||||
Reference in New Issue
Block a user