Skip to content

Commit b610b2c

Browse files
committed
fix(workflows): scope the fork preview to one workflow and close review gaps
- Fork "View changes" now loads only the previewed workflow: its deployed state, its identity mapping, its target row and its block pairs, instead of every deployed state in the source workspace plus the full promote plan. The plan item comes from the same buildForkPromotePlanItems decision the promote uses. The route gets a per-user rate limit. - Word marks give up past 64 edits per line pair, so many long rewritten lines cannot stall the tab. - Agent tool params that their block marks as password fields are masked whatever their name. - A list item whose label changed but whose body did not is no longer flagged as a masked value change. - Hoist double casts under their annotations, make sourceWorkflowId optional on the wire for rollout, lazy-load both diff modals, and re-record the settings module baseline: the block registry the diff canvas needs is reached only through the lazy chunk opened on click.
1 parent 90b0089 commit b610b2c

23 files changed

Lines changed: 558 additions & 220 deletions

File tree

‎apps/sim/app/api/workspaces/[id]/fork/workflow-diff/route.test.ts‎

Lines changed: 58 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -17,25 +17,25 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'
1717
import { OrchestrationError } from '@/lib/core/orchestration/types'
1818

1919
const mocks = vi.hoisted(() => ({
20-
loadSourceDeployedStates: vi.fn(),
20+
loadSourceDeployedWorkflow: vi.fn(),
2121
loadTargetDraftState: vi.fn(),
2222
loadForkBlockMap: vi.fn(),
23-
computeForkPromotePlan: vi.fn(),
23+
resolveForkPlanItem: vi.fn(),
2424
}))
2525

2626
vi.mock('@/lib/core/application/workspace-authorization', () => workspaceAuthorizationMock)
2727
vi.mock('@/lib/workspaces/permissions/utils', () => permissionsMock)
2828
vi.mock('@/ee/workspace-forking/lib/lineage/authz', () => workspaceForkingAuthzMock)
2929
vi.mock('@/ee/workspace-forking/lib/lineage/lineage', () => workspaceForkingLineageMock)
3030
vi.mock('@/ee/workspace-forking/lib/copy/deploy-bridge', () => ({
31-
loadSourceDeployedStates: mocks.loadSourceDeployedStates,
31+
loadSourceDeployedWorkflow: mocks.loadSourceDeployedWorkflow,
3232
loadTargetDraftState: mocks.loadTargetDraftState,
3333
}))
3434
vi.mock('@/ee/workspace-forking/lib/mapping/block-map-store', () => ({
3535
loadForkBlockMap: mocks.loadForkBlockMap,
3636
}))
3737
vi.mock('@/ee/workspace-forking/lib/promote/promote-plan', () => ({
38-
computeForkPromotePlan: mocks.computeForkPromotePlan,
38+
resolveForkPlanItem: mocks.resolveForkPlanItem,
3939
}))
4040

4141
import { GET } from '@/app/api/workspaces/[id]/fork/workflow-diff/route'
@@ -70,20 +70,15 @@ describe('fork workflow-diff route', () => {
7070
childWorkspaceId: 'child',
7171
})
7272
mocks.loadForkBlockMap.mockResolvedValue({ parentToChild: new Map(), childToParent: new Map() })
73-
mocks.loadSourceDeployedStates.mockResolvedValue({
74-
deployedWorkflows: [{ id: 'wf-src' }],
75-
sourceStates: new Map([['wf-src', emptyState]]),
76-
})
77-
mocks.computeForkPromotePlan.mockResolvedValue({
78-
items: [
79-
{
80-
sourceWorkflowId: 'wf-src',
81-
targetWorkflowId: 'wf-tgt',
82-
mode: 'create',
83-
sourceMeta: { name: 'Ask Biz' },
84-
},
85-
],
86-
archivedTargets: [],
73+
mocks.loadSourceDeployedWorkflow.mockImplementation(async (_ws: string, id: string) =>
74+
id === 'wf-src' ? { summary: { id, name: 'Ask Biz' }, state: emptyState } : null
75+
)
76+
mocks.resolveForkPlanItem.mockResolvedValue({
77+
sourceWorkflowId: 'wf-src',
78+
targetWorkflowId: 'wf-tgt',
79+
targetName: null,
80+
mode: 'create',
81+
sourceMeta: { name: 'Ask Biz' },
8782
})
8883
})
8984

@@ -98,7 +93,7 @@ describe('fork workflow-diff route', () => {
9893
)
9994

10095
expect(response.status).toBe(403)
101-
expect(mocks.loadSourceDeployedStates).not.toHaveBeenCalled()
96+
expect(mocks.loadSourceDeployedWorkflow).not.toHaveBeenCalled()
10297
})
10398

10499
it('rejects a request without the source workflow id', async () => {
@@ -108,7 +103,7 @@ describe('fork workflow-diff route', () => {
108103
)
109104

110105
expect(response.status).toBe(400)
111-
expect(mocks.loadSourceDeployedStates).not.toHaveBeenCalled()
106+
expect(mocks.loadSourceDeployedWorkflow).not.toHaveBeenCalled()
112107
})
113108

114109
it('rejects workspaces that are not a direct fork edge without reading state', async () => {
@@ -120,7 +115,7 @@ describe('fork workflow-diff route', () => {
120115
)
121116

122117
expect(response.status).toBe(400)
123-
expect(mocks.loadSourceDeployedStates).not.toHaveBeenCalled()
118+
expect(mocks.loadSourceDeployedWorkflow).not.toHaveBeenCalled()
124119
})
125120

126121
it('maps a workflow outside the sync plan to 404', async () => {
@@ -147,4 +142,46 @@ describe('fork workflow-diff route', () => {
147142
afterLabel: 'Ask Biz (deployed)',
148143
})
149144
})
145+
it('maps a workflow whose target is excluded from sync to 404', async () => {
146+
mocks.resolveForkPlanItem.mockResolvedValue(null)
147+
148+
const response = await GET(
149+
request({ otherWorkspaceId: 'parent', direction: 'push', sourceWorkflowId: 'wf-src' }),
150+
routeContext
151+
)
152+
153+
expect(response.status).toBe(404)
154+
})
155+
156+
it('reads the target draft and only its block pairs when the sync replaces a workflow', async () => {
157+
const targetState = {
158+
...emptyState,
159+
blocks: {},
160+
}
161+
mocks.resolveForkPlanItem.mockResolvedValue({
162+
sourceWorkflowId: 'wf-src',
163+
targetWorkflowId: 'wf-tgt',
164+
targetName: 'Ask Biz prod',
165+
mode: 'replace',
166+
sourceMeta: { name: 'Ask Biz' },
167+
})
168+
mocks.loadTargetDraftState.mockResolvedValue(targetState)
169+
170+
const response = await GET(
171+
request({ otherWorkspaceId: 'parent', direction: 'push', sourceWorkflowId: 'wf-src' }),
172+
routeContext
173+
)
174+
175+
expect(response.status).toBe(200)
176+
expect(mocks.loadTargetDraftState).toHaveBeenCalledWith('wf-tgt', 'parent')
177+
expect(mocks.loadForkBlockMap).toHaveBeenCalledWith(expect.anything(), 'child', {
178+
side: 'parent',
179+
workflowId: 'wf-tgt',
180+
})
181+
await expect(response.json()).resolves.toMatchObject({
182+
targetWorkflowId: 'wf-tgt',
183+
before: targetState,
184+
beforeLabel: 'Ask Biz prod (current)',
185+
})
186+
})
150187
})

‎apps/sim/app/api/workspaces/[id]/fork/workflow-diff/route.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ export const GET = defineInternalJsonRoute({
1212
contract: getForkWorkflowDiffContract,
1313
auth: internalSessionAuth,
1414
operation: forkOperations.syncPreview,
15-
rateLimit: internalRateLimits.none({ reason: 'Preserve existing internal fork request policy' }),
15+
rateLimit: internalRateLimits.user({ bucketName: 'workspace-fork-workflow-diff' }),
1616
errorPolicy: internalForkErrorPolicy,
1717
mapInput: ({ params, query }) => ({ workspaceId: params.id, ...query }),
1818
useCase: getWorkspaceSyncWorkflowDiff,
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,2 @@
1-
export { type CompareSide, CompareVersionsModal } from './compare-versions-modal'
1+
export type { CompareSide } from './compare-versions-modal'
22
export { Versions } from './versions'

‎apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/components/deploy-modal/components/general/general.tsx‎

Lines changed: 26 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
'use client'
22

3-
import { useId, useState } from 'react'
3+
import { lazy, Suspense, useId, useState } from 'react'
44
import {
55
Button,
66
ChipButtonGroup,
@@ -17,17 +17,27 @@ import {
1717
} from '@sim/emcn'
1818
import { createLogger } from '@sim/logger'
1919
import type { WorkflowDeploymentVersionResponse } from '@/lib/workflows/persistence/utils'
20+
import {
21+
type ComparePair,
22+
resolveComparePair,
23+
} from '@/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/components/deploy-modal/components/general/compare-pair'
2024
import type { DeployReadiness } from '@/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-deploy-readiness'
2125
import { Preview, PreviewWorkflow } from '@/app/workspace/[workspaceId]/w/components/preview'
2226
import { useDeploymentVersionState, useRevertToVersion } from '@/hooks/queries/workflows'
2327
import { useWorkflowRegistry } from '@/stores/workflows/registry/store'
2428
import type { WorkflowState } from '@/stores/workflows/workflow/types'
25-
import { type ComparePair, resolveComparePair } from './compare-pair'
26-
import { CompareVersionsModal, Versions } from './components'
29+
import { Versions } from './components'
2730
import { formatVersionLabel } from './format-version-label'
2831

2932
const logger = createLogger('GeneralDeploy')
3033

34+
/** The comparison canvas is heavy and rarely opened, so it stays out of the editor's initial bundle. */
35+
const CompareVersionsModal = lazy(() =>
36+
import(
37+
'@/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/components/deploy-modal/components/general/components/compare-versions-modal'
38+
).then((module) => ({ default: module.CompareVersionsModal }))
39+
)
40+
3141
interface GeneralDeployProps {
3242
workflowId: string | null
3343
deployedState?: WorkflowState | null
@@ -350,17 +360,19 @@ export function GeneralDeploy({
350360
/>
351361

352362
{workflowId && comparePair && (
353-
<CompareVersionsModal
354-
key={JSON.stringify(comparePair)}
355-
open
356-
onOpenChange={(open) => {
357-
if (!open) setComparePair(null)
358-
}}
359-
workflowId={workflowId}
360-
versions={versions}
361-
initialBase={comparePair.base}
362-
initialTarget={comparePair.target}
363-
/>
363+
<Suspense fallback={null}>
364+
<CompareVersionsModal
365+
key={JSON.stringify(comparePair)}
366+
open
367+
onOpenChange={(open) => {
368+
if (!open) setComparePair(null)
369+
}}
370+
workflowId={workflowId}
371+
versions={versions}
372+
initialBase={comparePair.base}
373+
initialTarget={comparePair.target}
374+
/>
375+
</Suspense>
364376
)}
365377

366378
{workflowToShow && (

‎apps/sim/app/workspace/[workspaceId]/w/components/preview/components/preview-workflow/components/diff-label/diff-label.tsx‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,8 @@
33
import { cn } from '@sim/emcn'
44
import type { BlockDiffStatus } from '@/lib/workflows/comparison'
55

6-
const DIFF_LABEL: Record<BlockDiffStatus, string> = {
6+
/** The word for each comparison status, shared by the canvas label and the change list badge. */
7+
export const DIFF_LABEL: Record<BlockDiffStatus, string> = {
78
added: 'Added',
89
modified: 'Modified',
910
removed: 'Removed',

‎apps/sim/app/workspace/[workspaceId]/w/components/workflow-diff/components/change-list/binding-change-row.tsx‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -34,13 +34,15 @@ export function BindingChangeRow({ blockType, field, oldValue, newValue }: Bindi
3434
<span className='text-[var(--text-muted)]'>Differs between workspaces</span>
3535
) : (
3636
<>
37-
<span className='min-w-0 truncate text-[var(--text-muted)]'>
38-
{formatScalar(blockType, field, oldValue)}
39-
</span>
37+
<OverflowText
38+
label={formatScalar(blockType, field, oldValue)}
39+
className='min-w-0 flex-1 text-[var(--text-muted)]'
40+
/>
4041
<ArrowRight className='size-[12px] shrink-0 text-[var(--text-icon)]' />
41-
<span className='min-w-0 truncate text-[var(--text-tertiary)]'>
42-
{formatScalar(blockType, field, newValue)}
43-
</span>
42+
<OverflowText
43+
label={formatScalar(blockType, field, newValue)}
44+
className='min-w-0 flex-1 text-[var(--text-tertiary)]'
45+
/>
4446
</>
4547
)}
4648
</div>

‎apps/sim/app/workspace/[workspaceId]/w/components/workflow-diff/components/change-list/change-list.tsx‎

Lines changed: 12 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import { Badge, cn, OverflowText } from '@sim/emcn'
1414
import { ChevronDown } from '@sim/emcn/icons'
1515
import { humanizeBlockName } from '@sim/workflow-renderer'
1616
import type { BlockDiffStatus, WorkflowDiffSummary } from '@/lib/workflows/comparison'
17+
import { DIFF_LABEL } from '@/app/workspace/[workspaceId]/w/components/preview/components/preview-workflow/components/diff-label/diff-label'
1718
import { BindingChangeRow } from '@/app/workspace/[workspaceId]/w/components/workflow-diff/components/change-list/binding-change-row'
1819
import {
1920
DIFF_SIGN,
@@ -36,12 +37,6 @@ const STATUS_BADGE_VARIANT: Record<BlockDiffStatus, 'green' | 'amber' | 'red'> =
3637
removed: 'red',
3738
}
3839

39-
const STATUS_LABEL: Record<BlockDiffStatus, string> = {
40-
added: 'Added',
41-
modified: 'Modified',
42-
removed: 'Removed',
43-
}
44-
4540
interface ChangeListProps {
4641
summary: WorkflowDiffSummary
4742
/** The two sides, so cards can show an added or removed block's fields and detect moves */
@@ -166,14 +161,14 @@ export function ChangeList({
166161
}
167162
>
168163
<div className='flex flex-col gap-1.5 rounded-md border border-[var(--border)] bg-[var(--surface-2)] p-3 text-small'>
169-
{summary.variableChanges.addedNames.map((name) => (
170-
<NamedRow key={`a-${name}`} kind='added' name={name} />
164+
{summary.variableChanges.addedNames.map((name, index) => (
165+
<NamedRow key={`a-${index}-${name}`} kind='added' name={name} />
171166
))}
172-
{summary.variableChanges.modifiedNames.map((name) => (
173-
<NamedRow key={`m-${name}`} kind='changed' name={name} />
167+
{summary.variableChanges.modifiedNames.map((name, index) => (
168+
<NamedRow key={`m-${index}-${name}`} kind='changed' name={name} />
174169
))}
175-
{summary.variableChanges.removedNames.map((name) => (
176-
<NamedRow key={`r-${name}`} kind='removed' name={name} />
170+
{summary.variableChanges.removedNames.map((name, index) => (
171+
<NamedRow key={`r-${index}-${name}`} kind='removed' name={name} />
177172
))}
178173
</div>
179174
</Section>
@@ -293,7 +288,7 @@ const BlockCard = memo(function BlockCard({
293288
className='flex-1 font-medium text-[var(--text-primary)] text-small'
294289
/>
295290
<Badge variant={bindingsOnly ? 'gray' : STATUS_BADGE_VARIANT[entry.status]} size='sm'>
296-
{bindingsOnly ? 'Bindings only' : STATUS_LABEL[entry.status]}
291+
{bindingsOnly ? 'Bindings only' : DIFF_LABEL[entry.status]}
297292
</Badge>
298293
{hasBody && (
299294
<ChevronDown
@@ -371,16 +366,16 @@ const BlockCard = memo(function BlockCard({
371366
nested
372367
/>
373368
))}
374-
{entry.membership?.added.map((row) => (
369+
{entry.membership?.added.map((row, index) => (
375370
<NamedRow
376-
key={`a-${row.name}`}
371+
key={`a-${index}-${row.name}`}
377372
kind='added'
378373
name={`${humanizeBlockName(row.name)}${row.moved ? ' (moved in)' : ''}`}
379374
/>
380375
))}
381-
{entry.membership?.removed.map((row) => (
376+
{entry.membership?.removed.map((row, index) => (
382377
<NamedRow
383-
key={`r-${row.name}`}
378+
key={`r-${index}-${row.name}`}
384379
kind='removed'
385380
name={`${humanizeBlockName(row.name)}${row.moved ? ' (moved out)' : ''}`}
386381
/>
Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1 @@
1-
export { BindingChangeRow } from './binding-change-row'
21
export { ChangeList } from './change-list'
3-
export { FieldChangeRow } from './field-change-row'
4-
export { KeyedListDiff } from './keyed-list-diff'
5-
export { InlineDiff, TextDiff } from './text-diff'

‎apps/sim/app/workspace/[workspaceId]/w/components/workflow-diff/components/change-list/keyed-list-diff.tsx‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,9 @@ export function KeyedListDiff({ blockType, field, oldValue, newValue }: KeyedLis
5858
{row.oldLabel && (
5959
<span className='text-[var(--text-muted)]'> (was {row.oldLabel})</span>
6060
)}
61+
{row.secretChanged && (
62+
<span className='text-[var(--text-muted)]'> (a masked value changed)</span>
63+
)}
6164
</span>
6265
{row.kind === 'changed' && row.oldText !== row.newText ? (
6366
<TextDiff oldText={row.oldText} newText={row.newText} />

‎apps/sim/app/workspace/[workspaceId]/w/components/workflow-diff/components/change-list/text-diff-lines.test.ts‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -152,4 +152,12 @@ describe('buildDiffRows', () => {
152152
{ type: 'oversized', oldLines: MAX_DIFF_LINES, newLines: MAX_DIFF_LINES + 1 },
153153
])
154154
})
155+
it('gives up on word marks for a heavily rewritten pair instead of diffing it word by word', () => {
156+
const before = Array.from({ length: 150 }, (_, i) => `alpha${i}`).join(' ')
157+
const after = Array.from({ length: 150 }, (_, i) => `beta${i}`).join(' ')
158+
const lines = markWordChanges(toLines(before, after))
159+
160+
expect(lines.map((line) => line.kind)).toEqual(['removed', 'added'])
161+
expect(lines.every((line) => line.parts === undefined)).toBe(true)
162+
})
155163
})

0 commit comments

Comments
 (0)