Skip to content

Commit 6d6cf6a

Browse files
committed
fix(settings): preserve fresh snapshots and same-page history
1 parent 1000fce commit 6d6cf6a

9 files changed

Lines changed: 331 additions & 55 deletions

File tree

‎.agents/skills/add-settings-page/SKILL.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ Each grep lists candidates; review every match against the expected ones named b
6666
- Editable pages: confirm Save/Discard go through `saveDiscardActions()` and
6767
dirty is wired via `useSettingsUnsavedGuard` (called before early-return
6868
gates) — flag any hand-rolled Save button, `beforeunload`, or unsaved modal.
69-
`git grep -n "beforeunload" -- 'apps/sim/**/settings/**' 'apps/sim/ee/' 'apps/sim/components/settings/'`
69+
`git grep -n "beforeunload" -- 'apps/sim/**/settings/**' 'apps/sim/ee/' 'apps/sim/components/settings/' ':(exclude,glob)**/*.test.*'`
7070
should only hit the centralized `use-settings-browser-navigation.ts`.
7171
5. Fix each finding with the smallest structural change that satisfies the checklist;
7272
do not touch handlers, state, queries, or gate returns. A pixel-size fix swaps

‎.claude/rules/sim-settings-pages.md‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -355,8 +355,9 @@ shells. Reach for it before hand-rolling a `Chip`.
355355
- **Detail back controls** call `guard.guardBack(closeFn)`. Other destructive view
356356
transitions, such as switching an editor's direction or environment, also use
357357
the shared `requestLeave` action before resetting their draft.
358-
- **`navigationBlocked` covers pending saves and uploads.** The shared guard holds
359-
navigation until they settle. Disable or preserve edits made during requests;
358+
- **`navigationBlocked` covers pending saves and uploads.** The shared guard blocks
359+
navigation while requests are pending. Attempts are not queued; retry after they
360+
settle. Disable or preserve edits made during requests;
360361
failed saves retain drafts, and successful saves only clear committed values.
361362
- **Call the hook unconditionally, before every early-return gate** (entitlement,
362363
loading, or empty-state return), so gated renders preserve hook order.

‎.cursor/rules/sim-settings-pages.mdc‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -352,8 +352,9 @@ shells. Reach for it before hand-rolling a `Chip`.
352352
- **Detail back controls** call `guard.guardBack(closeFn)`. Other destructive view
353353
transitions, such as switching an editor's direction or environment, also use
354354
the shared `requestLeave` action before resetting their draft.
355-
- **`navigationBlocked` covers pending saves and uploads.** The shared guard holds
356-
navigation until they settle. Disable or preserve edits made during requests;
355+
- **`navigationBlocked` covers pending saves and uploads.** The shared guard blocks
356+
navigation while requests are pending. Attempts are not queued; retry after they
357+
settle. Disable or preserve edits made during requests;
357358
failed saves retain drafts, and successful saves only clear committed values.
358359
- **Call the hook unconditionally, before every early-return gate** (entitlement,
359360
loading, or empty-state return), so gated renders preserve hook order.

‎apps/sim/components/secrets/secrets-editor.test.tsx‎

Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -250,6 +250,152 @@ describe('shared secrets editor', () => {
250250
expect(left).toBe(true)
251251
})
252252

253+
it.each(['personal', 'shared'])(
254+
'resumes a skipped %s refresh after reverting the draft',
255+
async (scope) => {
256+
const original = { TOKEN: 'original' }
257+
const refreshed = { TOKEN: 'remote', REMOTE: 'new' }
258+
const props = (variables: Record<string, string>) =>
259+
scope === 'shared'
260+
? { variables }
261+
: {
262+
personal: {
263+
variables: Object.fromEntries(
264+
Object.entries(variables).map(([key, value]) => [key, { key, value }])
265+
),
266+
save: mocks.save,
267+
},
268+
}
269+
const selector =
270+
scope === 'shared'
271+
? 'input[name^="workspace_env_value_TOKEN"]'
272+
: 'input[name^="env_variable_value_"]'
273+
await render(props(original))
274+
const field = container.querySelector<HTMLInputElement>(selector)
275+
if (!field) throw new Error('Missing secret value')
276+
await change(field, 'draft')
277+
await render(props(refreshed))
278+
const retained = container.querySelector<HTMLInputElement>(selector)
279+
if (!retained) throw new Error('Missing retained draft')
280+
expect(retained.value).toBe('draft')
281+
await change(retained, 'original')
282+
const values = [...container.querySelectorAll<HTMLInputElement>('input[name*="value"]')].map(
283+
(input) => input.value
284+
)
285+
expect(values).toContain('remote')
286+
expect(values).toContain('new')
287+
let left = false
288+
act(() =>
289+
useSettingsDirtyStore.getState().requestLeave(() => {
290+
left = true
291+
})
292+
)
293+
expect(left).toBe(true)
294+
}
295+
)
296+
297+
it.each(['personal', 'shared'])(
298+
'consumes a fresh %s snapshot received before Save without hiding remote keys',
299+
async (scope) => {
300+
const empty = {}
301+
const original = { TOKEN: 'original' }
302+
const refreshed = { TOKEN: 'remote', REMOTE: 'new' }
303+
const personalOriginal = { TOKEN: { key: 'TOKEN', value: 'original' } }
304+
const personalRefreshed = {
305+
TOKEN: { key: 'TOKEN', value: 'remote' },
306+
REMOTE: { key: 'REMOTE', value: 'new' },
307+
}
308+
const props = (fresh: boolean) =>
309+
scope === 'shared'
310+
? { variables: fresh ? refreshed : original }
311+
: {
312+
variables: empty,
313+
personal: {
314+
variables: fresh ? personalRefreshed : personalOriginal,
315+
save: mocks.save,
316+
},
317+
}
318+
const selector =
319+
scope === 'shared'
320+
? 'input[name^="workspace_env_value_TOKEN"]'
321+
: 'input[name^="env_variable_value_"]'
322+
await render(props(false))
323+
const field = container.querySelector<HTMLInputElement>(selector)
324+
if (!field) throw new Error('Missing secret value')
325+
await change(field, 'submitted')
326+
await render(props(true))
327+
await act(async () => button('Save').click())
328+
expect(container.querySelector<HTMLInputElement>(selector)?.value).toBe('submitted')
329+
const values = [...container.querySelectorAll<HTMLInputElement>('input[name*="value"]')].map(
330+
(input) => input.value
331+
)
332+
expect(values).toContain('new')
333+
let left = false
334+
act(() =>
335+
useSettingsDirtyStore.getState().requestLeave(() => {
336+
left = true
337+
})
338+
)
339+
expect(left).toBe(true)
340+
}
341+
)
342+
343+
it.each([
344+
{ scope: 'personal', refresh: 'during Save' },
345+
{ scope: 'shared', refresh: 'during Save' },
346+
{ scope: 'personal', refresh: 'after Save' },
347+
{ scope: 'shared', refresh: 'after Save' },
348+
])(
349+
'acknowledges fresh $scope values received $refresh without rolling back the save',
350+
async ({ scope, refresh }) => {
351+
const request = createDeferred<void>()
352+
const original = { TOKEN: 'original' }
353+
const canonical = { TOKEN: 'submitted', REMOTE: 'canonical' }
354+
const emptyShared = {}
355+
const personalOriginal = { TOKEN: { key: 'TOKEN', value: 'original' } }
356+
const personalCanonical = {
357+
TOKEN: { key: 'TOKEN', value: 'submitted' },
358+
REMOTE: { key: 'REMOTE', value: 'canonical' },
359+
}
360+
const props = (fresh: boolean, isSaving: boolean) =>
361+
scope === 'shared'
362+
? { variables: fresh ? canonical : original, isSaving, save: () => request.promise }
363+
: {
364+
variables: emptyShared,
365+
isSaving,
366+
personal: {
367+
variables: fresh ? personalCanonical : personalOriginal,
368+
save: () => request.promise,
369+
},
370+
}
371+
const selector =
372+
scope === 'shared'
373+
? 'input[name^="workspace_env_value_TOKEN"]'
374+
: 'input[name^="env_variable_value_"]'
375+
await render(props(false, false))
376+
const field = container.querySelector<HTMLInputElement>(selector)
377+
if (!field) throw new Error('Missing secret value')
378+
await change(field, 'submitted')
379+
act(() => button('Save').click())
380+
await render(props(refresh === 'during Save', true))
381+
await act(async () => request.resolve())
382+
await render(props(refresh === 'during Save', false))
383+
expect(container.querySelector<HTMLInputElement>(selector)?.value).toBe('submitted')
384+
if (refresh === 'after Save') await render(props(true, false))
385+
const values = [...container.querySelectorAll<HTMLInputElement>('input[name*="value"]')].map(
386+
(input) => input.value
387+
)
388+
expect(values).toContain('canonical')
389+
let left = false
390+
act(() =>
391+
useSettingsDirtyStore.getState().requestLeave(() => {
392+
left = true
393+
})
394+
)
395+
expect(left).toBe(true)
396+
}
397+
)
398+
253399
it('keeps an edited personal secret and its navigation protection through a refresh', async () => {
254400
const personal = { variables: { TOKEN: { key: 'TOKEN', value: 'original' } }, save: mocks.save }
255401
await render({ personal })

‎apps/sim/components/secrets/secrets-editor.tsx‎

Lines changed: 22 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -400,9 +400,11 @@ export function SecretsEditor({
400400
const initialWorkspaceVarsRef = useRef<Record<string, string>>({})
401401
const scrollContainerRef = useRef<HTMLDivElement>(null)
402402
const initialVarsRef = useRef<UIEnvironmentVariable[]>([])
403-
const hasChangesRef = useRef(false)
404-
const hasSavedPersonalRef = useRef(false)
405-
const hasSavedWorkspaceRef = useRef(false)
403+
const acknowledgedPersonalRef = useRef<{
404+
data: typeof personalEnvData
405+
hasPersonal: boolean
406+
} | null>(null)
407+
const acknowledgedWorkspaceRef = useRef<typeof variables>(undefined)
406408

407409
const filteredEnvVars = useMemo(() => {
408410
const mapped = envVars.map((envVar, index) => ({ envVar, originalIndex: index }))
@@ -496,20 +498,17 @@ export function SecretsEditor({
496498
return personalInvalid || workspaceInvalid
497499
}, [envVars, newWorkspaceRows])
498500

499-
hasChangesRef.current = hasChanges
500-
501501
const guard = useSettingsUnsavedGuard({
502502
isDirty: hasChanges,
503503
navigationBlocked: isListSaving,
504504
onDiscard: () => resetToSaved(),
505505
})
506506

507507
useEffect(() => {
508-
if (hasChangesRef.current) return
509-
if (hasSavedPersonalRef.current) {
510-
hasSavedPersonalRef.current = false
511-
return
512-
}
508+
if (hasChanges || isListSaving) return
509+
const acknowledged = acknowledgedPersonalRef.current
510+
if (acknowledged?.data === personalEnvData && acknowledged?.hasPersonal === hasPersonal) return
511+
acknowledgedPersonalRef.current = { data: personalEnvData, hasPersonal }
513512

514513
const existingVars = Object.values(personalEnvData || {})
515514
const initialVars = [
@@ -521,17 +520,15 @@ export function SecretsEditor({
521520
]
522521
initialVarsRef.current = structuredClone(initialVars)
523522
setEnvVars(structuredClone(initialVars))
524-
}, [personalEnvData, hasPersonal])
523+
}, [personalEnvData, hasPersonal, hasChanges, isListSaving])
525524

526525
useEffect(() => {
527-
if (!variables || hasChangesRef.current) return
528-
if (hasSavedWorkspaceRef.current) {
529-
hasSavedWorkspaceRef.current = false
526+
if (!variables || hasChanges || isListSaving || acknowledgedWorkspaceRef.current === variables)
530527
return
531-
}
528+
acknowledgedWorkspaceRef.current = variables
532529
setWorkspaceDraft((current) => ({ ...current, variables }))
533530
initialWorkspaceVarsRef.current = variables
534-
}, [variables])
531+
}, [variables, hasChanges, isListSaving])
535532

536533
const scrollToBottom = useCallback(() => {
537534
requestAnimationFrame(() => {
@@ -821,15 +818,19 @@ export function SecretsEditor({
821818
mutations.push(save({ upsert: toUpsert, remove: toDelete }))
822819
}
823820

824-
hasSavedPersonalRef.current = personalChanged
825-
hasSavedWorkspaceRef.current = Boolean(workspaceChanged)
826-
827821
try {
828822
const results = await Promise.allSettled(mutations)
829823
const firstFailure = results.find((r): r is PromiseRejectedResult => r.status === 'rejected')
830824
if (firstFailure) throw firstFailure.reason
831825

832-
initialWorkspaceVarsRef.current = { ...mergedWorkspaceVars }
826+
if (personalChanged) acknowledgedPersonalRef.current = { data: personalEnvData, hasPersonal }
827+
if (workspaceChanged) acknowledgedWorkspaceRef.current = variables
828+
const savedWorkspaceVars = applyVariableEdits(
829+
before,
830+
mergedWorkspaceVars,
831+
variables ?? before
832+
)
833+
initialWorkspaceVarsRef.current = savedWorkspaceVars
833834
const savedPersonalRows = Object.entries(personalVariablesToSave).map(([key, value]) => ({
834835
key,
835836
value,
@@ -864,7 +865,7 @@ export function SecretsEditor({
864865
newWorkspaceRows.filter((row) => row.key && row.value).map((row) => [row.id, row])
865866
)
866867
setWorkspaceDraft((current) => {
867-
const variables = { ...mergedWorkspaceVars }
868+
const variables = { ...savedWorkspaceVars }
868869
for (const submitted of submittedRows.values()) {
869870
if (Object.hasOwn(workspaceVars, submitted.key))
870871
setRecordValue(variables, submitted.key, workspaceVars[submitted.key])
@@ -885,8 +886,6 @@ export function SecretsEditor({
885886
toast.success('Secrets saved')
886887
}
887888
} catch (error) {
888-
hasSavedPersonalRef.current = false
889-
hasSavedWorkspaceRef.current = false
890889
logger.error('Failed to save environment variables:', error)
891890
toast.error(getErrorMessage(error, 'Failed to save secrets'))
892891
}

‎apps/sim/components/settings/unsaved-changes-regression.test.tsx‎

Lines changed: 83 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -630,7 +630,7 @@ it.each(['Source workflow ID', 'Target workspace ID'])(
630630
}
631631
)
632632

633-
async function renderScim() {
633+
function seedScim() {
634634
const shape = getDeploymentShape()
635635
seedDeploymentShape({ ...shape, features: { ...shape.features, scim: true } })
636636
client.setQueryData(scimKeys.connection('org-a'), {
@@ -653,9 +653,91 @@ async function renderScim() {
653653
client.setQueryData(scimKeys.activity('org-a'), [])
654654
client.setQueryData(permissionGroupKeys.list('org-a'), [])
655655
client.setQueryData(permissionGroupKeys.orgWorkspaces('org-a'), [])
656+
}
657+
658+
async function renderScim() {
659+
seedScim()
656660
await render(<ScimSection organizationId='org-a' active onOpenDomains={() => {}} />)
657661
}
658662

663+
it('preserves hidden SSO and SCIM drafts when provisioning is disabled and re-enabled', async () => {
664+
seedScim()
665+
client.setQueryData(ssoKeys.providerList('org-a'), { providers: [] })
666+
client.setQueryData(domainKeys.list('org-a'), { domains: [] })
667+
client.setQueryData(organizationKeys.billing('org-a'), {
668+
data: { subscriptionPlan: 'enterprise' },
669+
})
670+
let persisted = client.getQueryData<{ connection: { status: string } }>(
671+
scimKeys.connection('org-a')
672+
)
673+
vi.stubGlobal(
674+
'fetch',
675+
vi.fn((_url: string, init?: RequestInit) => {
676+
if (init?.method === 'PUT' && persisted) {
677+
const body = JSON.parse(String(init.body)) as { status: string }
678+
persisted = { connection: { ...persisted.connection, status: body.status } }
679+
}
680+
return Promise.resolve(jsonResponse(persisted))
681+
})
682+
)
683+
await render(<SSO organizationId='org-a' />, '?sso-tab=domains')
684+
click('Domains')
685+
edit('#sso-add-domain', 'draft.example.com')
686+
click('Provisioning')
687+
select('Token expiry', 'Expires in 90 days')
688+
const toggle = container.querySelector<HTMLButtonElement>('#scim-enabled')
689+
if (!toggle) throw new Error('Missing provisioning switch')
690+
await act(async () => {
691+
toggle.click()
692+
await flushMicrotasks()
693+
await vi.advanceTimersByTimeAsync(50)
694+
})
695+
expect(persisted?.connection.status).toBe('disabled')
696+
expectLeave(false)
697+
click('Domains')
698+
expect(input('#sso-add-domain').value).toBe('draft.example.com')
699+
click('Provisioning')
700+
await act(async () => {
701+
toggle.click()
702+
await flushMicrotasks()
703+
await vi.advanceTimersByTimeAsync(50)
704+
})
705+
expect(persisted?.connection.status).toBe('active')
706+
expect(container.querySelector('button[aria-label="Token expiry"]')?.textContent).toContain(
707+
'Expires in 90 days'
708+
)
709+
expectLeave(false)
710+
})
711+
712+
it('blocks provisioning toggles while token issuance is pending', async () => {
713+
await renderScim()
714+
const request = createDeferred<Response>()
715+
vi.stubGlobal(
716+
'fetch',
717+
vi.fn((_url: string, init?: RequestInit) =>
718+
init?.method === 'POST'
719+
? request.promise
720+
: Promise.resolve(jsonResponse(client.getQueryData(scimKeys.connection('org-a'))))
721+
)
722+
)
723+
click('Issue token')
724+
await act(async () => {
725+
await flushMicrotasks()
726+
await vi.advanceTimersByTimeAsync(1)
727+
})
728+
const toggle = container.querySelector<HTMLButtonElement>('#scim-enabled')
729+
if (!toggle) throw new Error('Missing provisioning switch')
730+
expect(toggle.disabled).toBe(true)
731+
expectLeave(false)
732+
await act(async () => {
733+
request.resolve(jsonResponse({ error: 'Unavailable' }, 503))
734+
await flushMicrotasks()
735+
await vi.advanceTimersByTimeAsync(50)
736+
})
737+
expect(toggle.disabled).toBe(false)
738+
expectLeave(true)
739+
})
740+
659741
it('protects the only retrievable SCIM token until its modal is dismissed', async () => {
660742
await renderScim()
661743
vi.stubGlobal(

0 commit comments

Comments
 (0)