diff --git a/apps/sim/app/api/workspaces/[id]/fork/diff/route.ts b/apps/sim/app/api/workspaces/[id]/fork/diff/route.ts index 72b0d7dd435..4bb41317799 100644 --- a/apps/sim/app/api/workspaces/[id]/fork/diff/route.ts +++ b/apps/sim/app/api/workspaces/[id]/fork/diff/route.ts @@ -28,10 +28,7 @@ import { collectForkClearedRefCandidates, } from '@/ee/workspace-forking/lib/promote/cleared-refs' import { computeForkPromotePlan } from '@/ee/workspace-forking/lib/promote/promote-plan' -import { - buildForkTriggerPlan, - resolveForkTriggerPaths, -} from '@/ee/workspace-forking/lib/promote/trigger-urls' +import { buildForkTriggerPlan } from '@/ee/workspace-forking/lib/promote/trigger-urls' import { buildForkBlockIdResolver } from '@/ee/workspace-forking/lib/remap/block-identity' import { readTargetDraftDependentValue } from '@/ee/workspace-forking/lib/remap/remap-references' @@ -188,7 +185,13 @@ export const GET = withRouteHandler( resolveBlockId, targetWebhooks: await loadTargetWebhookPathsByBlock(db, allTargetIds), }) - const { changes: triggerUrlChanges } = resolveForkTriggerPaths(triggerPlan) + // The RAW retiring set, not the default resolution: the client derives which of these actually + // stop being served from the picks the user is making right now, so the heads-up and the + // overwrite confirm can never disagree with the Trigger URLs rows. + const retiringTriggerUrls = triggerPlan.retiring.map((row) => ({ + workflowName: row.workflowName, + path: row.path, + })) // Every trigger that HAS a public URL, plus every one whose URL is up for decision - not just // the decisions, so the section reads as a standing statement of each URL rather than an alert. // @@ -259,7 +262,7 @@ export const GET = withRouteHandler( resourceUsages: collectForkResourceUsages(plan.items, sourceStates), copyableUnmapped: plan.copyableUnmapped, clearedRefs, - triggerUrlChanges, + retiringTriggerUrls, triggerMappings, }) } diff --git a/apps/sim/ee/workspace-forking/components/fork-sync/fork-sync-view.tsx b/apps/sim/ee/workspace-forking/components/fork-sync/fork-sync-view.tsx index a3ab75a55d2..266a1a993be 100644 --- a/apps/sim/ee/workspace-forking/components/fork-sync/fork-sync-view.tsx +++ b/apps/sim/ee/workspace-forking/components/fork-sync/fork-sync-view.tsx @@ -598,10 +598,10 @@ function TriggerMappingRow({ controller, mapping }: TriggerMappingRowProps) { // A trigger that already serves a URL keeps it, so the row states the URL and offers no // control. Only a trigger the sync would give a NEW URL has something to decide. const decidable = mapping.ownPath === null && mapping.adoptablePaths.length > 0 - const chosen = - mapping.sourceBlockId in controller.triggerAdoptions - ? controller.triggerAdoptions[mapping.sourceBlockId] - : (mapping.defaultAdoptPath ?? '') + const pathOwners = controller.triggerPathOwnersFor(mapping.sourceBlockId) + // The RESOLVED choice, not the raw pick: a path another row claimed first is awarded once, so + // displaying the raw pick would promise a URL this row is not going to get. + const chosen = controller.triggerChoiceFor(mapping.sourceBlockId) const resultingPath = mapping.ownPath ?? (chosen === '' ? null : chosen) return ( @@ -625,13 +625,23 @@ function TriggerMappingRow({ controller, mapping }: TriggerMappingRowProps) { // The full URL lives under the row and follows the selection, so an option only // has to name the CHOICE. Several retiring URLs is the one case that needs a // disambiguator, and the path tail is what distinguishes them. - ...mapping.adoptablePaths.map((path) => ({ - label: + // + // A URL another trigger already took is disabled and says who took it: two blocks + // cannot serve one path, and the resolver awards it to the first slot - so + // allowing the pick would leave this row reading "Keeps this URL" while the sync + // silently minted it a new one. + ...mapping.adoptablePaths.map((path) => { + const owner = pathOwners.get(path) + const base = mapping.adoptablePaths.length === 1 ? 'Keep existing URL' - : `Keep …${path.slice(-12)}`, - value: path, - })), + : `Keep …${path.slice(-12)}` + return { + label: owner ? `${base} · taken by ${owner}` : base, + value: path, + disabled: owner !== undefined, + } + }), { label: 'Generate new URL', value: NEW_TRIGGER_URL_VALUE }, ]} value={chosen === '' ? NEW_TRIGGER_URL_VALUE : chosen} diff --git a/apps/sim/ee/workspace-forking/components/fork-sync/trigger-choices.test.ts b/apps/sim/ee/workspace-forking/components/fork-sync/trigger-choices.test.ts new file mode 100644 index 00000000000..1fdd3717ede --- /dev/null +++ b/apps/sim/ee/workspace-forking/components/fork-sync/trigger-choices.test.ts @@ -0,0 +1,114 @@ +/** + * @vitest-environment node + */ +import { describe, expect, it } from 'vitest' +import type { ForkTriggerMapping } from '@/lib/api/contracts/workspace-fork' +import { + forkDyingTriggerUrls, + forkTriggerChoices, + forkTriggerPathOwners, +} from '@/ee/workspace-forking/components/fork-sync/trigger-choices' + +function mapping(overrides: Partial = {}): ForkTriggerMapping { + return { + sourceBlockId: 'blk', + blockName: 'Slack messages', + workflowName: 'ITSM intake', + ownPath: null, + adoptablePaths: ['p1'], + defaultAdoptPath: 'p1', + ...overrides, + } +} + +describe('forkTriggerChoices', () => { + it('takes the default when the user has not chosen', () => { + expect(forkTriggerChoices([mapping()], {}).get('blk')).toBe('p1') + }) + + it('honours an explicit pick over the default', () => { + const mappings = [mapping({ adoptablePaths: ['p1', 'p2'], defaultAdoptPath: null })] + expect(forkTriggerChoices(mappings, { blk: 'p2' }).get('blk')).toBe('p2') + }) + + it("treats an explicit '' as minting a new URL, overriding the default", () => { + expect(forkTriggerChoices([mapping()], { blk: '' }).get('blk')).toBe('') + }) + + it('ignores a pick the slot never offered', () => { + expect(forkTriggerChoices([mapping()], { blk: 'not-offered' }).get('blk')).toBe('') + }) + + /** + * Two blocks cannot serve one path (`path_deployment_unique`) and the server awards it to the + * first slot, so the second row's real outcome is a NEW URL - not the path it asked for. + */ + it('awards a contested path to the first row only', () => { + const mappings = [ + mapping({ sourceBlockId: 'a', blockName: 'Slack A', defaultAdoptPath: null }), + mapping({ sourceBlockId: 'b', blockName: 'Slack B', defaultAdoptPath: null }), + ] + const chosen = forkTriggerChoices(mappings, { a: 'p1', b: 'p1' }) + expect(chosen.get('a')).toBe('p1') + expect(chosen.get('b')).toBe('') + }) +}) + +describe('forkDyingTriggerUrls', () => { + const retiring = [ + { workflowName: 'ITSM intake', path: 'p1' }, + { workflowName: 'ITSM intake', path: 'p2' }, + ] + + it('excludes a URL some row adopts', () => { + const chosen = forkTriggerChoices([mapping()], {}) + expect(forkDyingTriggerUrls(retiring, chosen).map((r) => r.path)).toEqual(['p2']) + }) + + /** + * The bug this exists for: the server computes its warning from the DEFAULT resolution, so + * choosing "Generate new URL" used to kill a URL the confirm never mentioned. + */ + it('re-lists a URL once the user opts into a new one instead', () => { + const chosen = forkTriggerChoices([mapping()], { blk: '' }) + expect(forkDyingTriggerUrls(retiring, chosen).map((r) => r.path)).toEqual(['p1', 'p2']) + }) + + it('drops a URL the user adopts where the default adopted nothing', () => { + const mappings = [mapping({ adoptablePaths: ['p1', 'p2'], defaultAdoptPath: null })] + const chosen = forkTriggerChoices(mappings, { blk: 'p2' }) + expect(forkDyingTriggerUrls(retiring, chosen).map((r) => r.path)).toEqual(['p1']) + }) + + /** A contested path is still served by its winner, so it is not dying. */ + it('counts a contested path as adopted exactly once', () => { + const mappings = [ + mapping({ sourceBlockId: 'a', defaultAdoptPath: null }), + mapping({ sourceBlockId: 'b', defaultAdoptPath: null }), + ] + const chosen = forkTriggerChoices(mappings, { a: 'p1', b: 'p1' }) + expect(forkDyingTriggerUrls(retiring, chosen).map((r) => r.path)).toEqual(['p2']) + }) +}) + +describe('forkTriggerPathOwners', () => { + const mappings = [ + mapping({ sourceBlockId: 'a', blockName: 'Slack A', defaultAdoptPath: null }), + mapping({ sourceBlockId: 'b', blockName: 'Slack B', defaultAdoptPath: null }), + ] + + it('names the row that claimed a path, from another row’s perspective', () => { + const chosen = forkTriggerChoices(mappings, { a: 'p1' }) + expect(forkTriggerPathOwners(mappings, chosen, 'b').get('p1')).toBe('Slack A') + }) + + it('never reports a row as the owner of its own claim', () => { + const chosen = forkTriggerChoices(mappings, { a: 'p1' }) + expect(forkTriggerPathOwners(mappings, chosen, 'a').has('p1')).toBe(false) + }) + + it('reports nothing while no row has claimed anything', () => { + const chosen = forkTriggerChoices(mappings, {}) + expect(forkTriggerPathOwners(mappings, chosen, 'b').size).toBe(0) + }) +}) diff --git a/apps/sim/ee/workspace-forking/components/fork-sync/trigger-choices.ts b/apps/sim/ee/workspace-forking/components/fork-sync/trigger-choices.ts new file mode 100644 index 00000000000..914f12a533a --- /dev/null +++ b/apps/sim/ee/workspace-forking/components/fork-sync/trigger-choices.ts @@ -0,0 +1,63 @@ +import type { ForkTriggerMapping, ForkTriggerUrlChange } from '@/lib/api/contracts/workspace-fork' + +/** + * Which retiring URL each arriving trigger currently takes, keyed by source block id. `''` means + * "mint a new URL". + * + * Mirrors `resolveForkTriggerPaths` on the server, which is what makes the preview trustworthy: + * an override counts only for a path the slot actually offered, and a path is awarded to the + * FIRST row that claims it - two blocks cannot serve one path (`path_deployment_unique`), so a + * later row claiming the same URL silently receives a new one instead. + */ +export function forkTriggerChoices( + mappings: readonly ForkTriggerMapping[], + adoptions: Readonly> +): Map { + const chosen = new Map() + const claimed = new Set() + for (const mapping of mappings) { + const picked = + mapping.sourceBlockId in adoptions + ? adoptions[mapping.sourceBlockId] + : (mapping.defaultAdoptPath ?? '') + const honoured = + picked !== '' && mapping.adoptablePaths.includes(picked) && !claimed.has(picked) ? picked : '' + if (honoured !== '') claimed.add(honoured) + chosen.set(mapping.sourceBlockId, honoured) + } + return chosen +} + +/** + * The retiring URLs the CURRENT choices leave unserved. + * + * Derived from the raw retiring set rather than read off the diff: the server computes its own + * default before the user picks anything, so a preview built from it would omit a URL the user + * has just chosen to abandon - in the one modal that exists to state irreversible consequences. + */ +export function forkDyingTriggerUrls( + retiring: readonly ForkTriggerUrlChange[], + chosen: ReadonlyMap +): ForkTriggerUrlChange[] { + const adopted = new Set(Array.from(chosen.values()).filter((path) => path !== '')) + return retiring.filter((row) => !adopted.has(row.path)) +} + +/** + * The block name already claiming each path, from the perspective of one row - so its picker can + * disable a URL another trigger took rather than letting the user select a choice the sync will + * silently overrule. + */ +export function forkTriggerPathOwners( + mappings: readonly ForkTriggerMapping[], + chosen: ReadonlyMap, + forSourceBlockId: string +): Map { + const owners = new Map() + for (const mapping of mappings) { + if (mapping.sourceBlockId === forSourceBlockId) continue + const pick = chosen.get(mapping.sourceBlockId) + if (pick) owners.set(pick, mapping.blockName) + } + return owners +} diff --git a/apps/sim/ee/workspace-forking/components/fork-sync/use-fork-sync.ts b/apps/sim/ee/workspace-forking/components/fork-sync/use-fork-sync.ts index 54b3669a249..c28d29c371b 100644 --- a/apps/sim/ee/workspace-forking/components/fork-sync/use-fork-sync.ts +++ b/apps/sim/ee/workspace-forking/components/fork-sync/use-fork-sync.ts @@ -35,6 +35,11 @@ import { effectiveCopyDependentValue, effectiveDependentValue, } from '@/ee/workspace-forking/components/fork-sync/dependent-value' +import { + forkDyingTriggerUrls, + forkTriggerChoices, + forkTriggerPathOwners, +} from '@/ee/workspace-forking/components/fork-sync/trigger-choices' import { type ForkDirection, useForkDiff, @@ -180,7 +185,10 @@ export interface ForkSyncController { workflowChanges: ForkWorkflowChange[] /** Names of target workflows this sync archives, for the confirm modal. */ archivedWorkflowNames: string[] - /** Public trigger URLs this sync would stop serving in the target (warn before overwriting). */ + /** + * Public trigger URLs the CURRENT picks leave unserved. Derived from the retiring set and the + * live adoption choices, so the heads-up, the overwrite confirm and the rows always agree. + */ triggerUrlChanges: ForkTriggerUrlChange[] /** Arriving triggers whose URL is a choice: keep a retiring one, or mint a new one. */ triggerMappings: ForkTriggerMapping[] @@ -190,6 +198,13 @@ export interface ForkSyncController { */ triggerAdoptions: Readonly> setTriggerAdoption: (sourceBlockId: string, path: string) => void + /** Paths another trigger row has already claimed, so this row can disable them. */ + triggerPathOwnersFor: (sourceBlockId: string) => ReadonlyMap + /** + * The path a row will actually serve, resolved the same way the server resolves it. Never + * reports a path another row claimed first, so the row's displayed URL is its real outcome. + */ + triggerChoiceFor: (sourceBlockId: string) => string /** Names of deployed SOURCE workflows marked "Exclude from sync" - never sent. */ excludedSourceWorkflows: string[] /** Names of mapped TARGET workflows marked "Exclude from sync" - never replaced or archived. */ @@ -320,6 +335,10 @@ export function useForkSync(params: { () => diff.data?.triggerMappings ?? [], [diff.data?.triggerMappings] ) + const retiringTriggerUrls = useMemo( + () => diff.data?.retiringTriggerUrls ?? [], + [diff.data?.retiringTriggerUrls] + ) // Keys the backend offers as copy candidates, so the entry rows show a "Copy instead" // affordance only for those - clearing a name-match suggestion returns the ref to the copy @@ -782,6 +801,24 @@ export function useForkSync(params: { setTriggerAdoptions((prev) => ({ ...prev, [sourceBlockId]: path })) } + /** Live choices, resolved exactly as the server will resolve them (first claim wins a path). */ + const chosenTriggerPaths = useMemo( + () => forkTriggerChoices(triggerMappings, triggerAdoptions), + [triggerMappings, triggerAdoptions] + ) + + const triggerUrlChanges = useMemo( + () => forkDyingTriggerUrls(retiringTriggerUrls, chosenTriggerPaths), + [retiringTriggerUrls, chosenTriggerPaths] + ) + + const triggerPathOwnersFor = (sourceBlockId: string): ReadonlyMap => + forkTriggerPathOwners(triggerMappings, chosenTriggerPaths, sourceBlockId) + + /** The path a row will actually serve, or '' for a new URL - never a claim another row won. */ + const triggerChoiceFor = (sourceBlockId: string): string => + chosenTriggerPaths.get(sourceBlockId) ?? '' + const discard = () => { setTargets({}) setReconfig({}) @@ -965,10 +1002,12 @@ export function useForkSync(params: { dependentClears, workflowChanges, archivedWorkflowNames, - triggerUrlChanges: diff.data?.triggerUrlChanges ?? [], + triggerUrlChanges, triggerMappings, triggerAdoptions, setTriggerAdoption, + triggerPathOwnersFor, + triggerChoiceFor, excludedSourceWorkflows: diff.data?.excludedSourceWorkflows ?? [], excludedTargetWorkflows: diff.data?.excludedTargetWorkflows ?? [], mcpReauthCount: diff.data?.mcpReauthServerIds.length ?? 0, diff --git a/apps/sim/lib/api/contracts/workspace-fork.test.ts b/apps/sim/lib/api/contracts/workspace-fork.test.ts index 03ebc5d2326..b8defe69939 100644 --- a/apps/sim/lib/api/contracts/workspace-fork.test.ts +++ b/apps/sim/lib/api/contracts/workspace-fork.test.ts @@ -188,7 +188,7 @@ describe('getForkDiffContract response excluded-workflow lists', () => { const parsed = getForkDiffContract.response.schema.parse(baseDiffResponse) expect(parsed.excludedSourceWorkflows).toEqual([]) expect(parsed.excludedTargetWorkflows).toEqual([]) - expect(parsed.triggerUrlChanges).toEqual([]) + expect(parsed.retiringTriggerUrls).toEqual([]) expect(parsed.triggerMappings).toEqual([]) }) @@ -215,12 +215,12 @@ describe('getForkDiffContract response excluded-workflow lists', () => { defaultAdoptPath: 'live-slack-path', }, ], - triggerUrlChanges: [{ workflowName: 'ITSM intake', path: 'dead-path' }], + retiringTriggerUrls: [{ workflowName: 'ITSM intake', path: 'dead-path' }], }) expect(parsed.triggerMappings[0].ownPath).toBe('prod-live-path') expect(parsed.triggerMappings[0].adoptablePaths).toEqual([]) expect(parsed.triggerMappings[1].defaultAdoptPath).toBe('live-slack-path') - expect(parsed.triggerUrlChanges[0].path).toBe('dead-path') + expect(parsed.retiringTriggerUrls[0].path).toBe('dead-path') }) it('accepts a trigger mapping choice on the promote body, including "new URL"', () => { diff --git a/apps/sim/lib/api/contracts/workspace-fork.ts b/apps/sim/lib/api/contracts/workspace-fork.ts index 5546100fdcf..c861a0995d6 100644 --- a/apps/sim/lib/api/contracts/workspace-fork.ts +++ b/apps/sim/lib/api/contracts/workspace-fork.ts @@ -604,10 +604,15 @@ export const getForkDiffContract = defineRouteContract({ */ clearedRefs: z.array(forkClearedRefSchema), /** - * Public trigger URLs this sync would stop serving in the target. Defaulted so a new client - * tolerates an old server's response during rollout. + * Every public trigger URL this sync retires in the target, BEFORE any adoption is applied. + * + * Deliberately pre-adoption: which of these actually stop being served depends on the + * caller's live picks in `triggerMappings`, which only exist client-side until the promote + * call. Returning the post-default set instead would freeze the preview at the server's + * guess, so choosing "Generate new URL" would kill a URL the confirm never warned about. + * Defaulted so a new client tolerates an old server's response during rollout. */ - triggerUrlChanges: z.array(forkTriggerUrlChangeSchema).default([]), + retiringTriggerUrls: z.array(forkTriggerUrlChangeSchema).default([]), /** Arriving trigger blocks whose URL this sync decides, with their adoptable alternatives. */ triggerMappings: z.array(forkTriggerMappingSchema).default([]), }),