Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 9 additions & 6 deletions apps/sim/app/api/workspaces/[id]/fork/diff/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'

Expand Down Expand Up @@ -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.
Comment thread
icecrasher321 marked this conversation as resolved.
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.
//
Expand Down Expand Up @@ -259,7 +262,7 @@ export const GET = withRouteHandler(
resourceUsages: collectForkResourceUsages(plan.items, sourceStates),
copyableUnmapped: plan.copyableUnmapped,
clearedRefs,
triggerUrlChanges,
retiringTriggerUrls,
triggerMappings,
})
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand All @@ -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}
Expand Down
Original file line number Diff line number Diff line change
@@ -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)
})
})
Original file line number Diff line number Diff line change
@@ -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)
}
Comment thread
icecrasher321 marked this conversation as resolved.
return owners
}
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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[]
Expand All @@ -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. */
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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({})
Expand Down Expand Up @@ -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,
Expand Down
6 changes: 3 additions & 3 deletions apps/sim/lib/api/contracts/workspace-fork.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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([])
})

Expand All @@ -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"', () => {
Expand Down
11 changes: 8 additions & 3 deletions apps/sim/lib/api/contracts/workspace-fork.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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([]),
Comment thread
icecrasher321 marked this conversation as resolved.
/** Arriving trigger blocks whose URL this sync decides, with their adoptable alternatives. */
triggerMappings: z.array(forkTriggerMappingSchema).default([]),
}),
Expand Down
Loading