Skip to content

Commit c141830

Browse files
committed
fix(forking): stop a parent re-pick blanking a dependent's stored target value
A dependent selector (a sheet under a spreadsheet, a label under a mailbox) is invalidated when its parent is re-picked, because the stored child no longer exists under the new parent. That invalidation was recorded by writing an empty string into the in-session override map — the same value the user's own "clear this field" produces. The two are not the same thing, and the map is submitted verbatim and written into the target workflow's configuration, so an invalidated field cleared the target's real stored value. The sharpest case is an undo. Re-pick a parent away from its original target, then back. The parent nets out unchanged, so nothing is remapped and the remap's own clearing pass never runs — but the child is still blank, and that blank lands on a value the user never touched, with nothing in the UI showing it happened. Record the invalidation with a distinct marker instead. It reads as blank in the selector, the in-block chain context, and the Sync gate, so a required invalidated field still blocks Sync and still renders; but it is omitted from the submitted payload rather than sent as empty, so no override is written and the target keeps what it had. A blank the user picked themselves is still submitted and still clears the target. Also: skip the cascade entirely when a re-pick selects the value the field already had, since the selector fires its change handler either way. Fork file copy: a file whose name is already taken in a reused target folder is de-duplicated with the same allocator the ordinary upload path uses, rather than colliding with the folder-name unique index and being dropped from the fork with its blob deleted. Adds hook-level coverage for the submitted payload, which had none.
1 parent d152fad commit c141830

7 files changed

Lines changed: 1051 additions & 27 deletions

File tree

apps/sim/ee/workspace-forking/components/fork-sync/dependent-value.test.ts

Lines changed: 160 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,12 +5,15 @@ import { describe, expect, it } from 'vitest'
55
import type { ForkDependentReconfig } from '@/lib/api/contracts/workspace-fork'
66
import {
77
applyDependentRepick,
8+
DEPENDENT_CLEARED_BY_PARENT,
89
dependentKey,
910
effectiveCopyDependentValue,
1011
effectiveDependentValue,
1112
getActionableDependentFields,
1213
getDisplayedDependentFields,
14+
isDependentClearedByParent,
1315
isDependentConfigurationActionable,
16+
submittedDependentValue,
1417
} from '@/ee/workspace-forking/components/fork-sync/dependent-value'
1518

1619
const field = (overrides: Partial<ForkDependentReconfig> = {}): ForkDependentReconfig => ({
@@ -143,15 +146,39 @@ describe('applyDependentRepick', () => {
143146

144147
expect(next).toEqual({
145148
[dependentKey(site)]: 'site-new',
146-
[dependentKey(drive)]: '',
147-
[dependentKey(spreadsheet)]: '',
148-
[dependentKey(sheet)]: '',
149+
[dependentKey(drive)]: DEPENDENT_CLEARED_BY_PARENT,
150+
[dependentKey(spreadsheet)]: DEPENDENT_CLEARED_BY_PARENT,
151+
[dependentKey(sheet)]: DEPENDENT_CLEARED_BY_PARENT,
149152
[dependentKey(unrelated)]: 'still-keep-me',
150153
})
151154
expect(effectiveDependentValue(drive, next, false)).toBe('')
152155
expect(effectiveCopyDependentValue(sheet, next)).toBe('')
153156
})
154157

158+
it('re-picking the value the field already had leaves its descendants alone', () => {
159+
const spreadsheet = field({
160+
subBlockKey: 'spreadsheetId',
161+
currentValue: 'sheet-doc',
162+
providesContextKey: 'spreadsheetId',
163+
})
164+
const range = field({
165+
subBlockKey: 'range',
166+
currentValue: 'Sheet1!A1:D',
167+
consumesContextKeys: ['spreadsheetId'],
168+
})
169+
170+
const next = applyDependentRepick(
171+
{},
172+
spreadsheet,
173+
[spreadsheet, range],
174+
'sheet-doc',
175+
effectiveDependentValue(spreadsheet, {}, false)
176+
)
177+
178+
expect(next).toEqual({ [dependentKey(spreadsheet)]: 'sheet-doc' })
179+
expect(effectiveDependentValue(range, next, false)).toBe('Sheet1!A1:D')
180+
})
181+
155182
it('only changes the selected field when it provides no selector context', () => {
156183
const leaf = field({ subBlockKey: 'issueKey', currentValue: 'ISSUE-1' })
157184
const unrelated = field({ subBlockKey: 'label', currentValue: 'keep-me' })
@@ -204,12 +231,96 @@ describe('applyDependentRepick', () => {
204231
)
205232
).toEqual({
206233
[dependentKey(projectOne)]: 'P1-NEW',
207-
[dependentKey(issueOne)]: '',
234+
[dependentKey(issueOne)]: DEPENDENT_CLEARED_BY_PARENT,
208235
[dependentKey(issueTwo)]: 'P2-1',
209236
})
210237
})
211238
})
212239

240+
describe('submittedDependentValue', () => {
241+
const mappedParent = { copying: false, parentChanged: false }
242+
243+
it('omits an optional descendant a parent re-pick blanked, so the target keeps its value', () => {
244+
const spreadsheet = field({
245+
subBlockKey: 'spreadsheetId',
246+
currentValue: 'doc-old',
247+
providesContextKey: 'spreadsheetId',
248+
})
249+
const sheet = field({
250+
subBlockKey: 'sheetName',
251+
currentValue: 'Sheet1',
252+
required: true,
253+
consumesContextKeys: ['spreadsheetId'],
254+
})
255+
const range = field({
256+
subBlockKey: 'range',
257+
currentValue: 'A1:D50',
258+
required: false,
259+
consumesContextKeys: ['spreadsheetId'],
260+
})
261+
262+
const next = applyDependentRepick(
263+
{},
264+
spreadsheet,
265+
[spreadsheet, sheet, range],
266+
'doc-new',
267+
effectiveDependentValue(spreadsheet, {}, false)
268+
)
269+
270+
expect(isDependentClearedByParent(range, next)).toBe(true)
271+
expect(submittedDependentValue(range, next, mappedParent)).toBeUndefined()
272+
expect(submittedDependentValue(sheet, next, mappedParent)).toBeUndefined()
273+
expect(submittedDependentValue(spreadsheet, next, mappedParent)).toBe('doc-new')
274+
})
275+
276+
it('submits an empty value the user picked themselves, so an explicit clear still clears', () => {
277+
const label = field({ subBlockKey: 'label', currentValue: 'INBOX' })
278+
279+
const next = applyDependentRepick({}, label, [label], '', 'INBOX')
280+
281+
expect(isDependentClearedByParent(label, next)).toBe(false)
282+
expect(submittedDependentValue(label, next, mappedParent)).toBe('')
283+
})
284+
285+
it('submits a re-picked descendant once the user chooses a replacement', () => {
286+
const spreadsheet = field({
287+
subBlockKey: 'spreadsheetId',
288+
currentValue: 'doc-old',
289+
providesContextKey: 'spreadsheetId',
290+
})
291+
const range = field({
292+
subBlockKey: 'range',
293+
currentValue: 'A1:D50',
294+
consumesContextKeys: ['spreadsheetId'],
295+
})
296+
297+
const cleared = applyDependentRepick(
298+
{},
299+
spreadsheet,
300+
[spreadsheet, range],
301+
'doc-new',
302+
effectiveDependentValue(spreadsheet, {}, false)
303+
)
304+
const repicked = applyDependentRepick(
305+
cleared,
306+
range,
307+
[spreadsheet, range],
308+
'A1:Z',
309+
effectiveDependentValue(range, cleared, false)
310+
)
311+
312+
expect(submittedDependentValue(range, repicked, mappedParent)).toBe('A1:Z')
313+
})
314+
315+
it('falls back to the stored value under an unchanged parent and to the source when copying', () => {
316+
const untouched = field({ subBlockKey: 'label', currentValue: 'INBOX' })
317+
const copied = field({ subBlockKey: 'documentSelector', currentValue: '', sourceValue: 'doc' })
318+
319+
expect(submittedDependentValue(untouched, {}, mappedParent)).toBe('INBOX')
320+
expect(submittedDependentValue(copied, {}, { copying: true, parentChanged: false })).toBe('doc')
321+
})
322+
})
323+
213324
describe('isDependentConfigurationActionable', () => {
214325
it('hides stored values when the mapped parent is unchanged', () => {
215326
expect(
@@ -281,6 +392,51 @@ describe('isDependentConfigurationActionable', () => {
281392
).toBe(true)
282393
})
283394

395+
it('shows a required field that a parent re-pick blanked (it blocks Sync)', () => {
396+
const spreadsheet = field({
397+
subBlockKey: 'spreadsheetId',
398+
currentValue: 'doc-old',
399+
providesContextKey: 'spreadsheetId',
400+
})
401+
const sheet = field({
402+
subBlockKey: 'sheetName',
403+
required: true,
404+
currentValue: 'Sheet1',
405+
consumesContextKeys: ['spreadsheetId'],
406+
})
407+
const next = applyDependentRepick(
408+
{},
409+
spreadsheet,
410+
[spreadsheet, sheet],
411+
'doc-new',
412+
effectiveDependentValue(spreadsheet, {}, false)
413+
)
414+
415+
// The sync gate reads the same blank the selector shows, so the field gates and is visible.
416+
expect(effectiveDependentValue(sheet, next, false)).toBe('')
417+
expect(
418+
isDependentConfigurationActionable(sheet, next, {
419+
parentResolved: true,
420+
parentChanged: false,
421+
copying: false,
422+
})
423+
).toBe(true)
424+
})
425+
426+
it('shows a required field the user emptied themselves (it blocks Sync)', () => {
427+
const sheet = field({ subBlockKey: 'sheetName', required: true, currentValue: 'Sheet1' })
428+
const next = applyDependentRepick({}, sheet, [sheet], '', 'Sheet1')
429+
430+
expect(effectiveDependentValue(sheet, next, false)).toBe('')
431+
expect(
432+
isDependentConfigurationActionable(sheet, next, {
433+
parentResolved: true,
434+
parentChanged: false,
435+
copying: false,
436+
})
437+
).toBe(true)
438+
})
439+
284440
it('hides dependents until their parent is resolved', () => {
285441
expect(
286442
isDependentConfigurationActionable(

apps/sim/ee/workspace-forking/components/fork-sync/dependent-value.ts

Lines changed: 76 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -10,18 +10,46 @@ function sameDependencyScope(left: ForkDependentReconfig, right: ForkDependentRe
1010
}
1111

1212
/**
13-
* Store a dependent re-pick and clear every selector transitively scoped by it. Empty-string
14-
* overrides are intentional: an absent override means "fall back to the stored value", while a
15-
* changed provider makes every stored descendant stale for both mapped and copied parents.
13+
* Marker stored for a descendant whose value an in-session parent re-pick invalidated, kept
14+
* distinct from the user's own empty pick. It reads as blank everywhere it is consumed - the
15+
* selector, the in-block chain context, and the sync gate - but is never submitted: the user
16+
* has not chosen a replacement, so no override is written for it. A `''` the user picked
17+
* themselves IS submitted and does clear the target.
18+
*
19+
* What the omission buys differs by path. On Save nothing rewrites the target draft, so its
20+
* stored value survives outright - including across an undo (re-pick away, then back), where
21+
* the parent nets out unchanged so `clearDependentsOnRemap` never fires and an explicit `''`
22+
* would land on a value nobody touched. On Sync the written state is source-derived and
23+
* `clearDependentsOnRemap` already blanks every top-level dependent of a remapped parent, so
24+
* omission preserves nothing there; what it prevents is an explicit `''` reaching the fields
25+
* that pass does not cover - nested `tools[i].param` values in particular, which
26+
* `applyNestedToolOverrides` would otherwise blank.
27+
*
28+
* The escaped NUL prefix keeps it disjoint from every real selector value (ids, names, label
29+
* paths) - no selector can produce one, so it can never collide with a genuine pick.
30+
*/
31+
export const DEPENDENT_CLEARED_BY_PARENT = '\u0000fork-sync:cleared-by-parent'
32+
33+
/**
34+
* Store a dependent re-pick and invalidate every selector transitively scoped by it, marking
35+
* each with `DEPENDENT_CLEARED_BY_PARENT`: a changed provider makes every stored descendant
36+
* stale for both mapped and copied parents, but only the fields the user reviews themselves
37+
* get written to the target.
38+
*
39+
* `previousValue` is the field's effective value before the pick. `onChange` fires even when
40+
* the user re-selects the value the field already had, and that pick moves no scope, so nothing
41+
* below it went stale - the cascade is skipped. Omit it only where no prior value is known.
1642
*/
1743
export function applyDependentRepick(
1844
reconfig: Record<string, string>,
1945
changedField: ForkDependentReconfig,
2046
blockFields: ForkDependentReconfig[],
21-
value: string
47+
value: string,
48+
previousValue?: string
2249
): Record<string, string> {
2350
const changedKey = dependentKey(changedField)
2451
const nextState = { ...reconfig, [changedKey]: value }
52+
if (previousValue !== undefined && previousValue === value) return nextState
2553
if (!changedField.providesContextKey) return nextState
2654

2755
const pendingContextKeys = [changedField.providesContextKey]
@@ -41,7 +69,7 @@ export function applyDependentRepick(
4169
}
4270

4371
visitedFields.add(fieldKey)
44-
nextState[fieldKey] = ''
72+
nextState[fieldKey] = DEPENDENT_CLEARED_BY_PARENT
4573
if (field.providesContextKey) pendingContextKeys.push(field.providesContextKey)
4674
}
4775
}
@@ -51,16 +79,19 @@ export function applyDependentRepick(
5179

5280
/**
5381
* The value sent + displayed for a dependent: the user's in-session re-pick if present, else the
54-
* stored value (`currentValue`). Blank when the parent target changed in-session, since the old
55-
* stored value was for the previous parent and won't resolve against the new one. Shared by the
56-
* sync gate + payload build and the per-block selector so the rule can't drift between them.
82+
* stored value (`currentValue`). Blank when the parent target changed in-session, or when an
83+
* in-block parent re-pick invalidated it, since the old stored value was for the previous parent
84+
* and won't resolve against the new one. Shared by the sync gate and the per-block selector so
85+
* the rule can't drift between them. What gets SUBMITTED is `submittedDependentValue`, which
86+
* additionally omits the fields the user has not reviewed.
5787
*/
5888
export function effectiveDependentValue(
5989
field: ForkDependentReconfig,
6090
reconfig: Record<string, string>,
6191
parentChanged: boolean
6292
): string {
6393
const repicked = reconfig[dependentKey(field)]
94+
if (repicked === DEPENDENT_CLEARED_BY_PARENT) return ''
6495
if (repicked !== undefined) return repicked
6596
return parentChanged ? '' : field.currentValue
6697
}
@@ -78,10 +109,41 @@ export function effectiveCopyDependentValue(
78109
reconfig: Record<string, string>
79110
): string {
80111
const repicked = reconfig[dependentKey(field)]
112+
if (repicked === DEPENDENT_CLEARED_BY_PARENT) return ''
81113
if (repicked !== undefined) return repicked
82114
return field.currentValue || field.sourceValue
83115
}
84116

117+
/**
118+
* Whether an in-block parent re-pick invalidated this field and the user has not re-picked it
119+
* since. Such a field shows blank but is not submitted: blanking the target on the user's behalf
120+
* would destroy a stored value they never chose to clear.
121+
*/
122+
export function isDependentClearedByParent(
123+
field: ForkDependentReconfig,
124+
reconfig: Record<string, string>
125+
): boolean {
126+
return reconfig[dependentKey(field)] === DEPENDENT_CLEARED_BY_PARENT
127+
}
128+
129+
/**
130+
* The value this dependent contributes to the submitted mapping, or `undefined` when it must be
131+
* OMITTED so the target keeps what it already stores. Only a field the user reviewed is written:
132+
* an explicit empty pick clears the target, while a value merely invalidated by a parent re-pick
133+
* is left alone (it is on screen and blank, and if it is required the sync gate blocks until the
134+
* user picks one - `effectiveDependentValue` reports it as blank).
135+
*/
136+
export function submittedDependentValue(
137+
field: ForkDependentReconfig,
138+
reconfig: Record<string, string>,
139+
state: { copying: boolean; parentChanged: boolean }
140+
): string | undefined {
141+
if (isDependentClearedByParent(field, reconfig)) return undefined
142+
return state.copying
143+
? effectiveCopyDependentValue(field, reconfig)
144+
: effectiveDependentValue(field, reconfig, state.parentChanged)
145+
}
146+
85147
export interface DependentConfigurationState {
86148
parentResolved: boolean
87149
parentChanged: boolean
@@ -92,6 +154,12 @@ export interface DependentConfigurationState {
92154
* Whether a dependent selector needs to be shown. A changed or copied parent requires review
93155
* because its children resolve in a different scope. An unchanged mapping only needs a selector
94156
* when a required value is missing; its stored values are already valid and sync-ready.
157+
*
158+
* A field a parent re-pick invalidated reads as blank through `effectiveDependentValue`, so a
159+
* REQUIRED one stays on screen here and keeps gating Sync. An optional one drops out of the
160+
* default view - `getDisplayedDependentFields` brings it back under explicit edit mode - and
161+
* hiding it loses nothing, because `submittedDependentValue` omits it from the payload rather
162+
* than blanking the target.
95163
*/
96164
export function isDependentConfigurationActionable(
97165
field: ForkDependentReconfig,

apps/sim/ee/workspace-forking/components/fork-sync/fork-sync-view.tsx

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -204,10 +204,11 @@ function DependentSelector({
204204
reconfig,
205205
setReconfig,
206206
}: DependentSelectorProps) {
207-
const effectiveValue = (f: ForkDependentReconfig) =>
207+
const effectiveValueIn = (f: ForkDependentReconfig, state: Record<string, string>) =>
208208
copying
209-
? effectiveCopyDependentValue(f, reconfig)
210-
: effectiveDependentValue(f, reconfig, parentChanged)
209+
? effectiveCopyDependentValue(f, state)
210+
: effectiveDependentValue(f, state, parentChanged)
211+
const effectiveValue = (f: ForkDependentReconfig) => effectiveValueIn(f, reconfig)
211212
const { providedValues, providedContextKeys } = blockChainState(block, field, effectiveValue)
212213
// Disabled until every in-block parent it depends on has a value, so a child never queries
213214
// a stale upstream value.
@@ -230,7 +231,17 @@ function DependentSelector({
230231
enabled={parentValue !== '' && ready}
231232
value={effectiveValue(field)}
232233
onChange={(value) =>
233-
setReconfig((current) => applyDependentRepick(current, field, block.fields, value))
234+
setReconfig((current) =>
235+
// The pre-pick value comes from the state being updated, so re-selecting the value
236+
// already shown is recognised as the no-op it is and leaves descendants intact.
237+
applyDependentRepick(
238+
current,
239+
field,
240+
block.fields,
241+
value,
242+
effectiveValueIn(field, current)
243+
)
244+
)
234245
}
235246
title={field.title}
236247
/>

0 commit comments

Comments
 (0)