-
Notifications
You must be signed in to change notification settings - Fork 3.8k
fix(forking): make the trigger URL preview reflect the user's actual picks #6290
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
114 changes: 114 additions & 0 deletions
114
apps/sim/ee/workspace-forking/components/fork-sync/trigger-choices.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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) | ||
| }) | ||
| }) |
63 changes: 63 additions & 0 deletions
63
apps/sim/ee/workspace-forking/components/fork-sync/trigger-choices.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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) | ||
| } | ||
|
icecrasher321 marked this conversation as resolved.
|
||
| return owners | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.