mirror of
https://github.com/simstudioai/sim.git
synced 2026-09-24 15:45:35 +08:00
fix(forking): make the trigger URL preview reflect the user's actual picks (#6290)
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>
This commit is contained in:
co-authored by
Claude Opus 5
parent
53c3beace1
commit
402f862210
@@ -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,
|
||||
})
|
||||
}
|
||||
|
||||
@@ -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}
|
||||
|
||||
@@ -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)
|
||||
})
|
||||
})
|
||||
@@ -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
|
||||
}
|
||||
@@ -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,
|
||||
|
||||
@@ -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"', () => {
|
||||
|
||||
@@ -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([]),
|
||||
}),
|
||||
|
||||
Reference in New Issue
Block a user