fix(forking): make the trigger URL preview reflect the user's actual picks

icecrasher321 · claude · icecrasher321 · commit a9b167ced64e · 2026-08-05T10:28:14.000-07:00
Two Cursor findings on #6272, both in the preview layer - the sync's write path was correct in each case, but the UI stated an outcome that did not match it. - The heads-up and overwrite confirm read `triggerUrlChanges` straight off the diff, which the server computes with its DEFAULT resolution before the user chooses anything. Selecting "Generate new URL" for a trigger that would have adopted a URL therefore killed that URL with no warning, in the one modal whose job is to state irreversible consequences (it also over-warned in the reverse case). The diff now returns the RAW retiring set and the client subtracts the live choices, so the rows, the heads-up and the confirm cannot disagree. - The picker let two triggers select the same retiring URL and showed both as keeping it. Two blocks cannot serve one path (`path_deployment_unique`) and the resolver awards it to the first slot, so the loser silently got a new URL. A path another row claimed is now disabled and named, and each row displays its RESOLVED outcome rather than its raw pick. The choice resolution is a pure module mirroring `resolveForkTriggerPaths` (offered-paths guard, first-claim-wins), so the preview and the server agree by construction rather than by two hand-kept implementations. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
diff --git 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
@@ -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
@@ -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> = {}): 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
@@ -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<Record<string, string>>
+): Map<string, string> {
+  const chosen = new Map<string, string>()
+  const claimed = new Set<string>()
+  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<string, string>
+): 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<string, string>,
+  forSourceBlockId: string
+): Map<string, string> {
+  const owners = new Map<string, string>()
+  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
@@ -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<Record<string, string>>
   setTriggerAdoption: (sourceBlockId: string, path: string) => void
+  /** Paths another trigger row has already claimed, so this row can disable them. */
+  triggerPathOwnersFor: (sourceBlockId: string) => ReadonlyMap<string, string>
+  /**
+   * 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<string, string> =>
+    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
@@ -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
@@ -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([]),
     }),
