Skip to content

Commit 1000fce

Browse files
committed
fix(settings): reconcile saved values and pending secret edits
1 parent 22fe83b commit 1000fce

14 files changed

Lines changed: 518 additions & 123 deletions

File tree

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -66,8 +66,8 @@ 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/'`
70-
should only hit the centralized `use-settings-before-unload.ts`.
69+
`git grep -n "beforeunload" -- 'apps/sim/**/settings/**' 'apps/sim/ee/' 'apps/sim/components/settings/'`
70+
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
7373
only the size class for its exact-pixel token (`text-[12px]` → `text-caption`).

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

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -345,7 +345,13 @@ shells. Reach for it before hand-rolling a `Chip`.
345345
forms. Dirty computation stays local and derived; fetched defaults stay clean.
346346
- **`onDiscard` resets the registered draft.** The root `SettingsNavigationGuard`
347347
owns the discard dialog, internal link interception, browser Back/Forward, and
348-
refresh protection. Do not add a per-page dialog or history listener.
348+
refresh protection for registered editors. Do not add a per-page dialog or
349+
history listener for an editor using this hook. Skill create/detail pages retain
350+
their existing `useUnsavedChangesGuard` and local dialog until they migrate to
351+
the shared registration.
352+
- **Native history needs entry indexes.** The Navigation API supplies them for
353+
legacy entries. Without it, native traversal to an unindexed entry cannot be
354+
cancelled reliably; never guess a direction or rewrite the history stack.
349355
- **Detail back controls** call `guard.guardBack(closeFn)`. Other destructive view
350356
transitions, such as switching an editor's direction or environment, also use
351357
the shared `requestLeave` action before resetting their draft.

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

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -342,7 +342,13 @@ shells. Reach for it before hand-rolling a `Chip`.
342342
forms. Dirty computation stays local and derived; fetched defaults stay clean.
343343
- **`onDiscard` resets the registered draft.** The root `SettingsNavigationGuard`
344344
owns the discard dialog, internal link interception, browser Back/Forward, and
345-
refresh protection. Do not add a per-page dialog or history listener.
345+
refresh protection for registered editors. Do not add a per-page dialog or
346+
history listener for an editor using this hook. Skill create/detail pages retain
347+
their existing `useUnsavedChangesGuard` and local dialog until they migrate to
348+
the shared registration.
349+
- **Native history needs entry indexes.** The Navigation API supplies them for
350+
legacy entries. Without it, native traversal to an unindexed entry cannot be
351+
cancelled reliably; never guess a direction or rewrite the history stack.
346352
- **Detail back controls** call `guard.guardBack(closeFn)`. Other destructive view
347353
transitions, such as switching an editor's direction or environment, also use
348354
the shared `requestLeave` action before resetting their draft.

‎apps/sim/app/workspace/[workspaceId]/components/credential-detail/hooks/use-credential-detail-form.ts‎

Lines changed: 32 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import { type MouseEvent, useCallback, useState } from 'react'
44
import { toast } from '@sim/emcn'
55
import { createLogger } from '@sim/logger'
66
import { getErrorMessage } from '@sim/utils/errors'
7+
import { filterUndefined } from '@sim/utils/object'
78
import { useRouter } from 'next/navigation'
89
import { useSettingsUnsavedGuard } from '@/components/settings/use-settings-unsaved-guard'
910
import { useUpdateWorkspaceCredential, type WorkspaceCredential } from '@/hooks/queries/credentials'
@@ -33,14 +34,6 @@ interface CredentialMetadata {
3334
unredacted: boolean
3435
}
3536

36-
function sameMetadata(left: CredentialMetadata, right: CredentialMetadata) {
37-
return (
38-
left.displayName === right.displayName &&
39-
left.description === right.description &&
40-
left.unredacted === right.unredacted
41-
)
42-
}
43-
4437
interface UseCredentialDetailFormParams {
4538
workspaceId?: string
4639
credential: WorkspaceCredential | null
@@ -78,34 +71,48 @@ export function useCredentialDetailForm({
7871
}
7972
const [draft, setDraft] = useState<{
8073
credentialId: string
81-
baseline: typeof savedValues
82-
values: typeof savedValues
74+
values: Partial<CredentialMetadata>
8375
} | null>(null)
8476

8577
if (draft && credential && draft.credentialId !== credential.id) setDraft(null)
8678

87-
const values = draft?.values ?? savedValues
88-
const baseline = draft?.baseline ?? savedValues
79+
const values = { ...savedValues, ...draft?.values }
8980
const displayNameDraft = values.displayName
9081
const descriptionDraft = values.description
9182
const unredactedDraft = values.unredacted
92-
const isDisplayNameDirty = values.displayName.trim() !== baseline.displayName.trim()
93-
const isDescriptionDirty = values.description.trim() !== baseline.description.trim()
94-
const isUnredactedDirty = values.unredacted !== baseline.unredacted
83+
const isDisplayNameDirty = values.displayName.trim() !== savedValues.displayName.trim()
84+
const isDescriptionDirty = values.description.trim() !== savedValues.description.trim()
85+
const isUnredactedDirty = values.unredacted !== savedValues.unredacted
9586
const isMetadataDirty = isDisplayNameDirty || isDescriptionDirty || isUnredactedDirty
9687

88+
if (
89+
draft &&
90+
!isSaving &&
91+
values.displayName === savedValues.displayName &&
92+
values.description === savedValues.description &&
93+
values.unredacted === savedValues.unredacted
94+
)
95+
setDraft(null)
96+
9797
const updateDraft = useCallback(
9898
(change: Partial<typeof savedValues>) => {
9999
if (!credential) return
100100
setDraft((current) => {
101-
const baseline = current?.baseline ?? {
101+
const saved = {
102102
displayName: credential.displayName,
103103
description: credential.description ?? '',
104104
unredacted: credential.unredacted,
105105
}
106-
const values = { ...(current?.values ?? baseline), ...change }
107-
if (!isSaving && sameMetadata(values, baseline)) return null
108-
return { credentialId: credential.id, baseline, values }
106+
const values = { ...current?.values, ...change }
107+
if (!isSaving) {
108+
if (values.displayName === saved.displayName) values.displayName = undefined
109+
if (values.description === saved.description) values.description = undefined
110+
if (values.unredacted === saved.unredacted) values.unredacted = undefined
111+
}
112+
const overrides = filterUndefined(values)
113+
return Object.keys(overrides).length
114+
? { credentialId: credential.id, values: overrides }
115+
: null
109116
})
110117
},
111118
[credential, isSaving]
@@ -147,21 +154,13 @@ export function useCredentialDetailForm({
147154
[guard.guardBack, router, backHref]
148155
)
149156

150-
const releaseUnchangedDraft = useCallback(() => {
151-
setDraft((current) =>
152-
current && sameMetadata(current.values, current.baseline) ? null : current
153-
)
154-
}, [])
155-
156157
const save = useCallback(async () => {
157158
if (!credential || isSaving) return
158159
const submitted = draft
159160
if (isSectionDirty && !(await section?.save())) {
160-
releaseUnchangedDraft()
161161
return
162162
}
163163
if (!isAdmin || !isMetadataDirty) {
164-
releaseUnchangedDraft()
165164
return
166165
}
167166

@@ -176,17 +175,14 @@ export function useCredentialDetailForm({
176175
if (current === submitted) return null
177176
if (!current || !submitted || current.credentialId !== submitted.credentialId)
178177
return current
179-
return {
180-
...current,
181-
baseline: {
182-
displayName: submitted.values.displayName.trim(),
183-
description: submitted.values.description.trim(),
184-
unredacted: submitted.values.unredacted,
185-
},
186-
}
178+
const values = { ...current.values }
179+
if (values.displayName === submitted.values.displayName) values.displayName = undefined
180+
if (values.description === submitted.values.description) values.description = undefined
181+
if (values.unredacted === submitted.values.unredacted) values.unredacted = undefined
182+
const overrides = filterUndefined(values)
183+
return Object.keys(overrides).length ? { ...current, values: overrides } : null
187184
})
188185
} catch (error) {
189-
releaseUnchangedDraft()
190186
toast.error("Couldn't save changes", {
191187
description: getErrorMessage(error, 'Please try again in a moment.'),
192188
})
@@ -207,7 +203,6 @@ export function useCredentialDetailForm({
207203
descriptionDraft,
208204
unredactedDraft,
209205
updateCredential.mutateAsync,
210-
releaseUnchangedDraft,
211206
])
212207

213208
return {

‎apps/sim/app/workspace/[workspaceId]/settings/components/admin/admin.tsx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -244,7 +244,7 @@ export function Admin() {
244244
<Chip
245245
aria-label={`Impersonate ${u.email}`}
246246
onClick={() => handleImpersonate(u.id, u.email)}
247-
disabled={pendingUserIds.has(u.id)}
247+
disabled={importWorkflow.isPending || pendingUserIds.has(u.id)}
248248
>
249249
{impersonatingUserId === u.id ? 'Switching...' : 'Impersonate'}
250250
</Chip>

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

Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,147 @@ async function change(input: HTMLInputElement, value: string) {
7777
}
7878

7979
describe('shared secrets editor', () => {
80+
it.each(['existing value', 'remove new row'])(
81+
'preserves the surviving shared secret when a submitted duplicate changes: %s',
82+
async (changeKind) => {
83+
const request = createDeferred<void>()
84+
const persisted: Record<string, string> = { TOKEN: 'original' }
85+
let first = true
86+
await render({
87+
variables: { TOKEN: 'original' },
88+
save: async ({ upsert, remove }) => {
89+
if (first) {
90+
first = false
91+
await request.promise
92+
}
93+
for (const key of remove) delete persisted[key]
94+
Object.assign(persisted, upsert)
95+
},
96+
})
97+
const key = container.querySelector<HTMLInputElement>('input[name^="new_workspace_key_"]')
98+
const value = container.querySelector<HTMLInputElement>('input[name^="new_workspace_value_"]')
99+
const existing = container.querySelector<HTMLInputElement>(
100+
'input[name^="workspace_env_value_TOKEN"]'
101+
)
102+
if (!key || !value || !existing) throw new Error('Missing shared secret rows')
103+
await change(key, 'TOKEN')
104+
await change(value, 'submitted')
105+
act(() => button('Save').click())
106+
if (changeKind === 'existing value') await change(existing, 'later')
107+
else {
108+
await change(key, '')
109+
await change(value, '')
110+
}
111+
await act(async () => request.resolve())
112+
await act(async () => button('Save').click())
113+
expect(persisted).toEqual({ TOKEN: changeKind === 'existing value' ? 'later' : 'original' })
114+
}
115+
)
116+
117+
it.each(['rename', 'value', 'delete'])(
118+
'rebases a new shared secret row changed during Save: %s',
119+
async (changeKind) => {
120+
const request = createDeferred<void>()
121+
const persisted: Record<string, string> = {}
122+
let first = true
123+
await render({
124+
save: async ({ upsert, remove }) => {
125+
if (first) {
126+
first = false
127+
await request.promise
128+
}
129+
for (const key of remove) delete persisted[key]
130+
Object.assign(persisted, upsert)
131+
},
132+
})
133+
const key = container.querySelector<HTMLInputElement>('input[name^="new_workspace_key_"]')
134+
const value = container.querySelector<HTMLInputElement>('input[name^="new_workspace_value_"]')
135+
if (!key || !value) throw new Error('Missing new shared secret row')
136+
await change(key, 'TOKEN')
137+
await change(value, 'submitted')
138+
act(() => button('Save').click())
139+
if (changeKind === 'rename') await change(key, 'RENAMED')
140+
else if (changeKind === 'value') await change(value, 'later')
141+
else {
142+
await change(key, '')
143+
await change(value, '')
144+
}
145+
await act(async () => request.resolve())
146+
const populated = [
147+
...container.querySelectorAll<HTMLInputElement>('input[name*="value"]'),
148+
].filter((field) => field.value)
149+
expect(populated).toHaveLength(changeKind === 'delete' ? 0 : 1)
150+
await act(async () => button('Save').click())
151+
expect(persisted).toEqual(
152+
changeKind === 'delete'
153+
? {}
154+
: changeKind === 'rename'
155+
? { RENAMED: 'submitted' }
156+
: { TOKEN: 'later' }
157+
)
158+
}
159+
)
160+
161+
it.each(['', 'remotely-populated'])(
162+
'preserves a saved empty personal secret refreshed to %s while saving another edited secret',
163+
async (remoteValue) => {
164+
let persisted: unknown
165+
const personal = {
166+
variables: {
167+
EMPTY: { key: 'EMPTY', value: '' },
168+
TOKEN: { key: 'TOKEN', value: 'original' },
169+
},
170+
save: async (variables: Record<string, string>) => {
171+
persisted = variables
172+
},
173+
}
174+
await render({ personal })
175+
const fields = [
176+
...container.querySelectorAll<HTMLInputElement>('input[name^="env_variable_value_"]'),
177+
]
178+
const token = fields.find((field) => field.value === 'original')
179+
if (!token) throw new Error('Missing saved token')
180+
await change(token, 'updated')
181+
await render({
182+
personal: {
183+
...personal,
184+
variables: { ...personal.variables, EMPTY: { key: 'EMPTY', value: remoteValue } },
185+
},
186+
})
187+
await act(async () => button('Save').click())
188+
expect(persisted).toEqual({ EMPTY: remoteValue, TOKEN: 'updated' })
189+
let left = false
190+
act(() =>
191+
useSettingsDirtyStore.getState().requestLeave(() => {
192+
left = true
193+
})
194+
)
195+
expect(left).toBe(true)
196+
}
197+
)
198+
199+
it('persists deletion of a saved empty personal secret', async () => {
200+
let persisted: unknown
201+
await render({
202+
personal: {
203+
variables: {
204+
EMPTY: { key: 'EMPTY', value: '' },
205+
TOKEN: { key: 'TOKEN', value: 'original' },
206+
},
207+
save: async (variables) => {
208+
persisted = variables
209+
},
210+
},
211+
})
212+
const empty = [
213+
...container.querySelectorAll<HTMLInputElement>('input[name^="env_variable_name_"]'),
214+
].find((field) => field.value === 'EMPTY')
215+
if (!empty) throw new Error('Missing empty secret')
216+
await change(empty, '')
217+
await act(async () => button('Save').click())
218+
expect(persisted).toEqual({ TOKEN: 'original' })
219+
})
220+
80221
it.each([
81222
{ scope: 'workspace', kind: 'key', prefix: 'new_workspace_key_' },
82223
{ scope: 'workspace', kind: 'value', prefix: 'new_workspace_value_' },

0 commit comments

Comments
 (0)