Skip to content

Commit bf78c4c

Browse files
authored
improvement(settings): unify save/discard across editable surfaces (#6173)
Save is now always rendered (primary, disabled until there is something to save) and Discard only appears once there is something to discard. Previously sandboxes showed no action at all until dirty — so a new sandbox had no visible Create button — while skills always showed a disabled Save and no Discard. - saveDiscardActions moves to @/components/settings and owns the one rule - adds SettingsActionChip / SettingsActionChips so the settings shell and the ReactNode-slot headers render actions through the same chip path - skills, secrets, connected credentials, custom tools and the secrets manager drop their hand-rolled Save chips for the shared helper - credential-detail drafts seed once per credential instead of re-seeding from every refetch, which was acting as an implicit discard
1 parent be8a93d commit bf78c4c

22 files changed

Lines changed: 297 additions & 184 deletions

File tree

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

Lines changed: 25 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -167,11 +167,32 @@ Any settings surface with editable state uses **one** shared stack — never
167167
hand-roll a Save button, a Discard button, a `beforeunload`, or an "Unsaved
168168
changes" modal:
169169

170-
- **`saveDiscardActions(config)`** (`…/components/save-discard-actions/save-discard-actions`)
171-
— returns the canonical dirty-gated **Discard + Save** `SettingsAction[]` (empty
172-
when not dirty). Spread it into a `SettingsPanel` `actions` array, beside any
170+
- **`saveDiscardActions(config)`** (`@/components/settings/save-discard-actions`)
171+
— returns the canonical **Discard + Save** `SettingsAction[]`. **Save is always
172+
rendered** (primary), disabled until there is something to save, so every
173+
editable surface announces its primary action in the same place and a create
174+
form is never a page with no visible way to commit it; **Discard appears only
175+
when dirty**. Spread it into a `SettingsPanel` `actions` array, beside any
173176
sibling actions (a detail view's Delete / Remove override). Config: `dirty`,
174-
`saving`, `onSave`, `onDiscard`, `saveDisabled?`, `saveLabel?`, `savingLabel?`.
177+
`saving`, `onSave`, `onDiscard`, `saveDisabled?`, `saveTooltip?`, `creating?`,
178+
`saveLabel?`, `savingLabel?`. Create flows pass `creating` — the
179+
Create / Creating... labels come as a pair and can never drift apart.
180+
`saveLabel`/`savingLabel` are only for genuinely bespoke wording (SSO's
181+
`Update`); never hand-roll the pair to get a create label.
182+
- **`<SaveDiscardChips {...config} />`** (same module) — the identical rule
183+
rendered as chips, for surfaces whose header takes a `ReactNode` instead of
184+
action data (`CredentialDetailLayout`: skills, secrets, connected credentials).
185+
Both stacks derive from the one function; never hand-roll a Save chip.
186+
187+
`CredentialDetailLayout` stays slot-driven for exactly two reasons: its back
188+
control is a real `<ChipLink href>` (deep-linkable / middle-clickable, which
189+
`SettingsBackAction`'s `onSelect` cannot express), and actions like
190+
`SkillImportButton` own a hidden file input and their own pending state.
191+
**Everything else in one of those headers should be `SettingsAction` data**
192+
rendered through `<SettingsActionChips actions={…} />` from
193+
`@/components/settings/settings-header` — that is the shared chip path, and it
194+
is what keeps tone/icon/variant/tooltip handling from drifting between the two
195+
shells. Reach for it before hand-rolling a `Chip`.
175196
- **`useSettingsUnsavedGuard({ isDirty })`** (`…/settings/hooks/use-settings-unsaved-guard`)
176197
— syncs the page's local `isDirty` into the shared `useSettingsDirtyStore` (so
177198
the sidebar's **section-switch** confirm + the centralized `beforeunload` both

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

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

3-
import { useCallback, useEffect, useState } from 'react'
3+
import { useCallback, useState } from 'react'
44
import { toast } from '@sim/emcn'
55
import { createLogger } from '@sim/logger'
66
import { getErrorMessage } from '@sim/utils/errors'
@@ -30,11 +30,21 @@ export function useCredentialDetailForm({
3030

3131
const [displayNameDraft, setDisplayNameDraft] = useState('')
3232
const [descriptionDraft, setDescriptionDraft] = useState('')
33+
const [seededCredentialId, setSeededCredentialId] = useState<string | null>(null)
3334

34-
useEffect(() => {
35-
setDisplayNameDraft(credential?.displayName ?? '')
36-
setDescriptionDraft(credential?.description ?? '')
37-
}, [credential?.id, credential?.displayName, credential?.description])
35+
// Seed drafts when the credential first resolves (or the route id changes); a
36+
// background refetch of the same credential must not clobber an in-progress
37+
// edit — Discard is the one way to reset.
38+
/** Applies a credential to both drafts — the one definition of "reset to server state". */
39+
const seedDrafts = useCallback((source: WorkspaceCredential) => {
40+
setDisplayNameDraft(source.displayName)
41+
setDescriptionDraft(source.description ?? '')
42+
}, [])
43+
44+
if (credential && credential.id !== seededCredentialId) {
45+
setSeededCredentialId(credential.id)
46+
seedDrafts(credential)
47+
}
3848

3949
const isDisplayNameDirty = credential ? displayNameDraft !== credential.displayName : false
4050
const isDescriptionDirty = credential
@@ -68,16 +78,22 @@ export function useCredentialDetailForm({
6878
isDescriptionDirty,
6979
displayNameDraft,
7080
descriptionDraft,
71-
updateCredential,
81+
updateCredential.mutateAsync,
82+
updateCredential.isPending,
7283
])
7384

85+
const discard = useCallback(() => {
86+
if (credential) seedDrafts(credential)
87+
}, [credential, seedDrafts])
88+
7489
return {
7590
displayNameDraft,
7691
setDisplayNameDraft,
7792
descriptionDraft,
7893
setDescriptionDraft,
7994
isDirty,
8095
save,
96+
discard,
8197
isSaving: updateCredential.isPending,
8298
handleBackClick: guard.handleBackClick,
8399
showUnsavedAlert: guard.showUnsavedAlert,

apps/sim/app/workspace/[workspaceId]/components/invite-modal/invite-modal.tsx

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,8 +40,7 @@ const MEMBERSHIP_OPTIONS = [
4040

4141
type Membership = (typeof MEMBERSHIP_OPTIONS)[number]['value']
4242

43-
const MEMBERSHIP_HINTS: Record<Membership, string> = {
44-
member: 'Joins your organization. Adds a seat.',
43+
const MEMBERSHIP_HINTS: Partial<Record<Membership, string>> = {
4544
admin: 'Joins your organization and can manage it. Adds a seat.',
4645
external:
4746
'Access to the selected workspaces only — no seat. Only available for people already on a paid Sim plan.',

apps/sim/app/workspace/[workspaceId]/integrations/connected/[credentialId]/connected-credential-detail.tsx

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import { ArrowLeft } from '@sim/emcn/icons'
1515
import { createLogger } from '@sim/logger'
1616
import { getErrorMessage } from '@sim/utils/errors'
1717
import { useRouter } from 'next/navigation'
18+
import { SaveDiscardChips } from '@/components/settings/save-discard-actions'
1819
import { writeOAuthReturnContext } from '@/lib/credentials/client-state'
1920
import { resolveCredentialDisplay } from '@/lib/integrations'
2021
import {
@@ -204,9 +205,12 @@ export function ConnectedCredentialDetail({
204205
>
205206
Disconnect
206207
</Chip>
207-
<Chip onClick={form.save} disabled={!form.isDirty || form.isSaving}>
208-
{form.isSaving ? 'Saving...' : 'Save'}
209-
</Chip>
208+
<SaveDiscardChips
209+
dirty={form.isDirty}
210+
saving={form.isSaving}
211+
onSave={form.save}
212+
onDiscard={form.discard}
213+
/>
210214
</>
211215
) : null
212216

apps/sim/app/workspace/[workspaceId]/settings/components/custom-tools/components/custom-tool-detail/custom-tool-detail.tsx

Lines changed: 12 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { ChipConfirmModal, toast } from '@sim/emcn'
55
import { ArrowLeft, Wrench } from '@sim/emcn/icons'
66
import { createLogger } from '@sim/logger'
77
import { getErrorMessage } from '@sim/utils/errors'
8+
import { saveDiscardActions } from '@/components/settings/save-discard-actions'
89
import { ResourceTile } from '@/app/workspace/[workspaceId]/components'
910
import {
1011
CredentialDetailHeading,
@@ -23,8 +24,6 @@ import {
2324
useSchemaGeneration,
2425
validateCustomToolSchema,
2526
} from '@/app/workspace/[workspaceId]/components/custom-tool-editor'
26-
import { saveDiscardActions } from '@/app/workspace/[workspaceId]/settings/components/save-discard-actions/save-discard-actions'
27-
import type { SettingsAction } from '@/app/workspace/[workspaceId]/settings/components/settings-header/settings-header'
2827
import { SettingsPanel } from '@/app/workspace/[workspaceId]/settings/components/settings-panel'
2928
import { SettingsSection } from '@/app/workspace/[workspaceId]/settings/components/settings-section/settings-section'
3029
import { useSettingsUnsavedGuard } from '@/app/workspace/[workspaceId]/settings/hooks/use-settings-unsaved-guard'
@@ -50,9 +49,9 @@ interface CustomToolDetailProps {
5049

5150
/**
5251
* Full-page custom tool editor rendered as a settings detail sub-view: a back
53-
* chip, dirty-gated Discard/Save, Delete, and the Schema and Code editors
54-
* stacked (no tabs — the page has room for both). Uses the same fields as the
55-
* canvas modal so the two surfaces never drift.
52+
* chip, Save/Discard, Delete, and the Schema and Code editors stacked (no tabs —
53+
* the page has room for both). Uses the same fields as the canvas modal so the
54+
* two surfaces never drift.
5655
*/
5756
export function CustomToolDetail({
5857
workspaceId,
@@ -200,22 +199,6 @@ export function CustomToolDetail({
200199
}
201200
}
202201

203-
/**
204-
* On create, the primary action is always visible so the page announces what
205-
* it is for — disabled until the schema is a valid function definition.
206-
* (`saveDiscardActions` is dirty-gated and would render nothing on an empty
207-
* draft.) Discard still only appears once there is something to discard.
208-
*/
209-
const createToolActions: SettingsAction[] = [
210-
...(dirty ? [{ text: 'Discard', onSelect: handleDiscard, disabled: saving }] : []),
211-
{
212-
text: saving ? 'Creating...' : 'Create',
213-
variant: 'primary' as const,
214-
onSelect: handleSave,
215-
disabled: saving || streaming || !isSchemaValid,
216-
},
217-
]
218-
219202
return (
220203
<>
221204
<SettingsPanel
@@ -224,15 +207,14 @@ export function CustomToolDetail({
224207
actions={[
225208
...(readOnly
226209
? []
227-
: isEditing
228-
? saveDiscardActions({
229-
dirty,
230-
saving,
231-
onSave: handleSave,
232-
onDiscard: handleDiscard,
233-
saveDisabled: !isSchemaValid || streaming,
234-
})
235-
: createToolActions),
210+
: saveDiscardActions({
211+
dirty,
212+
saving,
213+
onSave: handleSave,
214+
onDiscard: handleDiscard,
215+
saveDisabled: !isSchemaValid || streaming,
216+
creating: !isEditing,
217+
})),
236218
...(tool && !readOnly
237219
? [
238220
{

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import { useParams } from 'next/navigation'
88
import { useQueryState } from 'nuqs'
99
import { CodeIcon } from '@/components/icons'
1010
import { canMutateWorkspaceSettingsSection } from '@/components/settings/navigation'
11+
import { saveDiscardActions } from '@/components/settings/save-discard-actions'
1112
import type { SandboxDependencyIssue } from '@/lib/api/contracts/sandboxes'
1213
import { UnsavedChangesModal } from '@/app/workspace/[workspaceId]/components/credential-detail'
1314
import { useUserPermissionsContext } from '@/app/workspace/[workspaceId]/providers/workspace-permissions-provider'
@@ -29,7 +30,6 @@ import {
2930
type SandboxDraft,
3031
toSubmittedLines,
3132
} from '@/app/workspace/[workspaceId]/settings/components/sandboxes/utils'
32-
import { saveDiscardActions } from '@/app/workspace/[workspaceId]/settings/components/save-discard-actions/save-discard-actions'
3333
import { SettingsEmptyState } from '@/app/workspace/[workspaceId]/settings/components/settings-empty-state'
3434
import { SettingsPanel } from '@/app/workspace/[workspaceId]/settings/components/settings-panel'
3535
import { SettingsResourceRow } from '@/app/workspace/[workspaceId]/settings/components/settings-resource-row'
@@ -202,6 +202,7 @@ export function Sandboxes() {
202202
setIssues([])
203203
},
204204
saveDisabled: !canAdmin || current.name.trim().length === 0,
205+
creating: isCreating,
205206
}),
206207
...(selected && canAdmin
207208
? [

apps/sim/app/workspace/[workspaceId]/settings/components/save-discard-actions/save-discard-actions.ts

Lines changed: 0 additions & 33 deletions
This file was deleted.

apps/sim/app/workspace/[workspaceId]/settings/components/secrets/components/secrets-manager/secrets-manager.tsx

Lines changed: 13 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import { createLogger } from '@sim/logger'
66
import { useQueryClient } from '@tanstack/react-query'
77
import { useParams, useRouter } from 'next/navigation'
88
import { canMutateWorkspaceSettingsSection } from '@/components/settings/navigation'
9+
import { saveDiscardActions } from '@/components/settings/save-discard-actions'
910
import {
1011
clearPendingCredentialCreateRequest,
1112
PENDING_CREDENTIAL_CREATE_REQUEST_EVENT,
@@ -17,7 +18,6 @@ import { UnsavedChangesModal } from '@/app/workspace/[workspaceId]/components/cr
1718
import { RowActionsMenu } from '@/app/workspace/[workspaceId]/settings/components/row-actions-menu'
1819
import { SecretValueField } from '@/app/workspace/[workspaceId]/settings/components/secrets/components/secret-value-field'
1920
import { SettingsEmptyState } from '@/app/workspace/[workspaceId]/settings/components/settings-empty-state'
20-
import type { SettingsAction } from '@/app/workspace/[workspaceId]/settings/components/settings-header/settings-header'
2121
import { SettingsPanel } from '@/app/workspace/[workspaceId]/settings/components/settings-panel'
2222
import { useSettingsSearch } from '@/app/workspace/[workspaceId]/settings/components/use-settings-search'
2323
import { isValidEnvVarName } from '@/executor/constants'
@@ -981,27 +981,18 @@ export function SecretsManager() {
981981
onChange: setSearchTerm,
982982
placeholder: 'Search secrets...',
983983
}}
984-
actions={[
985-
...(hasChanges
986-
? [
987-
{
988-
text: 'Discard',
989-
onSelect: handleCancel,
990-
disabled: isListSaving,
991-
} satisfies SettingsAction,
992-
]
993-
: []),
994-
{
995-
text: isListSaving ? 'Saving...' : 'Save',
996-
onSelect: handleSave,
997-
disabled: hasConflicts || hasInvalidKeys || isLoading || !hasChanges || isListSaving,
998-
tooltip: hasConflicts
999-
? 'Resolve all conflicts before saving'
1000-
: hasInvalidKeys
1001-
? 'Fix invalid variable names before saving'
1002-
: undefined,
1003-
},
1004-
]}
984+
actions={saveDiscardActions({
985+
dirty: hasChanges,
986+
saving: isListSaving,
987+
onSave: handleSave,
988+
onDiscard: handleCancel,
989+
saveDisabled: hasConflicts || hasInvalidKeys || isLoading,
990+
saveTooltip: hasConflicts
991+
? 'Resolve all conflicts before saving'
992+
: hasInvalidKeys
993+
? 'Fix invalid variable names before saving'
994+
: undefined,
995+
})}
1005996
>
1006997
{!isLoading && (
1007998
<div className='flex flex-col gap-7'>

apps/sim/app/workspace/[workspaceId]/settings/components/secrets/hooks/use-secret-value.ts

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,5 +97,16 @@ export function useSecretValue({ workspaceId, credential }: UseSecretValueParams
9797
}
9898
}
9999

100-
return { value: draft, setValue: setDraft, canEdit, isConflicted, isDirty, save, isSaving }
100+
const discard = () => setDraft(currentValue)
101+
102+
return {
103+
value: draft,
104+
setValue: setDraft,
105+
canEdit,
106+
isConflicted,
107+
isDirty,
108+
save,
109+
discard,
110+
isSaving,
111+
}
101112
}

apps/sim/app/workspace/[workspaceId]/settings/secrets/[credentialId]/secret-detail.tsx

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import { useState } from 'react'
44
import { Chip, ChipCopyInput, ChipLink, Send } from '@sim/emcn'
55
import { ArrowLeft, Key } from '@sim/emcn/icons'
6+
import { SaveDiscardChips } from '@/components/settings/save-discard-actions'
67
import {
78
AddPeopleModal,
89
CredentialDetailHeading,
@@ -51,9 +52,12 @@ export function SecretDetail({ workspaceId, credentialId }: SecretDetailProps) {
5152
</Chip>
5253
)}
5354
{canEditValue && (
54-
<Chip onClick={valueField.save} disabled={!valueField.isDirty || valueField.isSaving}>
55-
{valueField.isSaving ? 'Saving...' : 'Save'}
56-
</Chip>
55+
<SaveDiscardChips
56+
dirty={valueField.isDirty}
57+
saving={valueField.isSaving}
58+
onSave={valueField.save}
59+
onDiscard={valueField.discard}
60+
/>
5761
)}
5862
</>
5963
) : null

0 commit comments

Comments
 (0)