Skip to content

Commit c3ff520

Browse files
committed
fix(secrets): address review findings on secret descriptions
- Patch the credential detail cache optimistically on update. `onMutate` cancelled the detail query but only patched the lists, so a detail-backed editor stayed dirty after a successful save until the refetch landed — long enough for Discard to restore the pre-save value over the committed one, and for Back to open the unsaved-changes guard. - Memoize `useSecretValue`'s returned callbacks and object, per the hook convention, so the composed form's save/discard stop churning per render. - Reject a description on a personal secret in the domain layer rather than only at the v2 boundary. The internal credential update path accepted one for any type, writing data every reader hides. - Normalize an empty description to null so the API and UI agree. - Correct the secrets documentation, which described a Display Name field the detail view does not have and omitted the scope rule. - Drop the CLI's copy of the 500-character bound; it can't import the contract, so a copy only drifts from the message the API already returns. - Collapse a redundant save guard and align the description write gate with the render gate. Leaves the integrations credential page byte-identical to staging.
1 parent c60e92e commit c3ff520

17 files changed

Lines changed: 297 additions & 76 deletions

File tree

apps/docs/content/docs/en/platform/credentials.mdx

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -95,14 +95,15 @@ Click **Details** on any secret row to open its detail view.
9595

9696
<Image
9797
src="/static/secrets/secret-details.png"
98-
alt="Secret details view showing Display Name, Description, and Members sections"
98+
alt="Secret details view showing Key, Value, Description, and Members sections"
9999
width={700}
100100
height={400}
101101
/>
102102

103103
From here you can:
104104

105-
- Edit the **Display Name** and **Description**
105+
- View the **Key** and edit the **Value**
106+
- Edit the **Description** — an optional note telling teammates what the secret is for. Workspace secrets only; a personal secret is not shared, so it has none
106107
- Manage **Members** — invite teammates by email and assign them an **Admin** or **Member** role
107108

108109
Click **Save** to apply changes, or **Back** to return to the list.

apps/docs/openapi-v2-resources.json

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -5175,7 +5175,7 @@
51755175
"writeOnly": true
51765176
},
51775177
"description": {
5178-
"description": "What the secret is for, shown to teammates. Omit to leave an existing description untouched; pass null to clear it. Workspace scope only.",
5178+
"description": "What the secret is for, shown to teammates. Omit to leave an existing description untouched; pass null or an empty string to clear it. Workspace scope only.",
51795179
"anyOf": [
51805180
{
51815181
"type": "string",
@@ -5190,14 +5190,7 @@
51905190
"required": ["workspaceId", "scope", "value"],
51915191
"additionalProperties": false,
51925192
"title": "Set secret request",
5193-
"description": "Ownership scope and write-only value for the secret.",
5194-
"examples": [
5195-
{
5196-
"workspaceId": "a91c4b2e-6d3f-4e8a-b5c7-0d9e2f1a8c64",
5197-
"scope": "workspace",
5198-
"value": "YOUR_SECRET_VALUE"
5199-
}
5200-
]
5193+
"description": "Ownership scope and write-only value for the secret."
52015194
},
52025195
"V2SecretDeleteData": {
52035196
"type": "object",

apps/sim/app/api/v2/secrets/[name]/route.test.ts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,20 @@ describe('/api/v2/secrets/[name]', () => {
169169
expect(mocks.set.mock.calls[0][0].input).not.toHaveProperty('description')
170170
})
171171

172+
it('normalizes an empty description to null so it matches the UI clear path', async () => {
173+
await PUT(
174+
request('PUT', {
175+
workspaceId: WORKSPACE_ID,
176+
scope: 'workspace',
177+
value: 'secret-value',
178+
description: ' ',
179+
}),
180+
context
181+
)
182+
183+
expect(mocks.set.mock.calls[0][0].input.description).toBeNull()
184+
})
185+
172186
it('rejects a description on a personal secret rather than dropping it', async () => {
173187
const response = await PUT(
174188
request('PUT', {

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

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,11 @@ const logger = createLogger('CredentialDetailForm')
1616
export interface CredentialDetailFormSection {
1717
isDirty: boolean
1818
isSaving: boolean
19-
/** Resolves false when the write failed, which stops the metadata save from committing alone. */
19+
/**
20+
* Resolves true when the caller may proceed — including when there was nothing
21+
* to write. False only when a write was attempted and failed, which stops the
22+
* metadata save from committing alone.
23+
*/
2024
save: () => Promise<boolean>
2125
discard: () => void
2226
}
@@ -81,11 +85,8 @@ export function useCredentialDetailForm({
8185

8286
const save = useCallback(async () => {
8387
if (!credential || isSaving) return
84-
const savesMetadata = isAdmin && isMetadataDirty
85-
if (!savesMetadata && !isSectionDirty) return
86-
8788
if (isSectionDirty && !(await section?.save())) return
88-
if (!savesMetadata) return
89+
if (!isAdmin || !isMetadataDirty) return
8990

9091
try {
9192
await updateCredential.mutateAsync({

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -267,7 +267,7 @@ export function ConnectedCredentialDetail({
267267
maxLength={500}
268268
autoComplete='off'
269269
data-lpignore='true'
270-
viewOnly={!isAdmin}
270+
disabled={!isAdmin}
271271
/>
272272
</DetailSection>
273273

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

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -420,10 +420,9 @@ export function SecretsManager() {
420420
}, [envVars, searchTerm])
421421

422422
/**
423-
* Matches description as well as key, so a secret documented as "prod billing"
424-
* is findable by that wording. The row has no description column, so a match on
425-
* it is only legible on the secret's detail page. Personal secrets carry no
426-
* shared description and stay key-only.
423+
* The row has no description column, so a description-only match is legible
424+
* only on the secret's detail page. Personal secrets carry no shared
425+
* description and stay key-only.
427426
*/
428427
const filteredWorkspaceEntries = useMemo(() => {
429428
const entries = Object.entries(workspaceVars)

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

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

3-
import { useState } from 'react'
3+
import { useCallback, useMemo, useState } from 'react'
44
import { toast } from '@sim/emcn'
55
import { createLogger } from '@sim/logger'
66
import { getErrorMessage } from '@sim/utils/errors'
@@ -69,8 +69,7 @@ export function useSecretValue({ workspaceId, credential }: UseSecretValueParams
6969
const isDirty = draft !== currentValue
7070
const isSaving = savePersonal.isPending || upsertWorkspace.isPending
7171

72-
/** Resolves false when the write failed, so a combined save can stop before its next step. */
73-
const save = async (): Promise<boolean> => {
72+
const save = useCallback(async (): Promise<boolean> => {
7473
if (!credential || !canEdit || isConflicted || !isDirty || isSaving) return true
7574
try {
7675
if (isPersonal) {
@@ -98,18 +97,40 @@ export function useSecretValue({ workspaceId, credential }: UseSecretValueParams
9897
logger.error('Failed to save secret value', error)
9998
return false
10099
}
101-
}
102-
103-
const discard = () => setDraft(currentValue)
104-
105-
return {
106-
value: draft,
107-
setValue: setDraft,
100+
}, [
101+
credential,
108102
canEdit,
109103
isConflicted,
110104
isDirty,
111-
save,
112-
discard,
113105
isSaving,
114-
}
106+
isPersonal,
107+
envKey,
108+
draft,
109+
workspaceId,
110+
refetchPersonal,
111+
savePersonal.mutateAsync,
112+
upsertWorkspace.mutateAsync,
113+
])
114+
115+
const discard = useCallback(() => setDraft(currentValue), [currentValue])
116+
117+
/**
118+
* Memoized so the object itself is stable, not just its callbacks: consumers
119+
* pass the whole value as one unit into {@link useCredentialDetailForm}'s
120+
* `section`, where a fresh object each render would churn the combined
121+
* save/discard identities regardless of the callbacks inside it.
122+
*/
123+
return useMemo(
124+
() => ({
125+
value: draft,
126+
setValue: setDraft,
127+
canEdit,
128+
isConflicted,
129+
isDirty,
130+
save,
131+
discard,
132+
isSaving,
133+
}),
134+
[draft, canEdit, isConflicted, isDirty, save, discard, isSaving]
135+
)
115136
}

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

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -35,9 +35,17 @@ export function SecretDetail({ workspaceId, credentialId }: SecretDetailProps) {
3535

3636
const valueField = useSecretValue({ workspaceId, credential })
3737

38+
/**
39+
* Description is workspace-only because `env_personal` credentials are
40+
* per-workspace mirrors of one user-global secret, so one saved here would
41+
* exist in this workspace alone — and a personal secret has no teammates to
42+
* inform. Gates the write and the render alike, so the two cannot disagree.
43+
*/
44+
const isWorkspaceSecretAdmin = isAdmin && !isPersonal
45+
3846
const form = useCredentialDetailForm({
3947
credential,
40-
isAdmin,
48+
isAdmin: isWorkspaceSecretAdmin,
4149
backHref: secretsHref,
4250
section: valueField,
4351
})
@@ -49,14 +57,6 @@ export function SecretDetail({ workspaceId, credentialId }: SecretDetailProps) {
4957
)
5058

5159
const canEditValue = valueField.canEdit && !valueField.isConflicted
52-
/**
53-
* Gates workspace-secret administration — Share and Description alike.
54-
* Description is workspace-only because `env_personal` credentials are
55-
* per-workspace mirrors of one user-global secret, so one saved here would
56-
* exist in this workspace alone — and a personal secret has no teammates to
57-
* inform.
58-
*/
59-
const isWorkspaceSecretAdmin = isAdmin && !isPersonal
6060

6161
const actions =
6262
credential && (isWorkspaceSecretAdmin || canEditValue) ? (
Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
import { beforeEach, describe, expect, it, vi } from 'vitest'
2+
3+
const { queryClient } = vi.hoisted(() => ({
4+
queryClient: {
5+
cancelQueries: vi.fn().mockResolvedValue(undefined),
6+
invalidateQueries: vi.fn().mockResolvedValue(undefined),
7+
getQueryData: vi.fn(),
8+
getQueriesData: vi.fn(() => []),
9+
setQueryData: vi.fn(),
10+
setQueriesData: vi.fn(),
11+
},
12+
}))
13+
14+
vi.mock('@tanstack/react-query', () => ({
15+
keepPreviousData: {},
16+
useQuery: vi.fn(),
17+
useQueryClient: vi.fn(() => queryClient),
18+
useMutation: vi.fn((options) => options),
19+
}))
20+
21+
vi.mock('@/lib/api/client/request', () => ({ requestJson: vi.fn() }))
22+
23+
import { useUpdateWorkspaceCredential } from '@/hooks/queries/credentials'
24+
25+
const CREDENTIAL_ID = 'cred-1'
26+
27+
const existing = {
28+
id: CREDENTIAL_ID,
29+
workspaceId: 'workspace-1',
30+
type: 'env_workspace' as const,
31+
displayName: 'STRIPE_API_KEY',
32+
description: 'old description',
33+
providerId: null,
34+
accountId: null,
35+
envKey: 'STRIPE_API_KEY',
36+
envOwnerUserId: null,
37+
createdBy: 'user-1',
38+
createdAt: '2026-01-01T00:00:00.000Z',
39+
updatedAt: '2026-01-02T00:00:00.000Z',
40+
role: 'admin' as const,
41+
}
42+
43+
/** Replays the detail-cache updater the mutation hands to `setQueryData`. */
44+
function detailAfterMutate(cached: typeof existing | null) {
45+
const detailCall = queryClient.setQueryData.mock.calls.find(
46+
([key]) => Array.isArray(key) && key.includes('detail')
47+
)
48+
const updater = detailCall?.[1] as (old: unknown) => unknown
49+
return updater(cached)
50+
}
51+
52+
describe('useUpdateWorkspaceCredential optimistic detail cache', () => {
53+
beforeEach(() => {
54+
vi.clearAllMocks()
55+
queryClient.getQueryData.mockReturnValue(existing)
56+
queryClient.getQueriesData.mockReturnValue([])
57+
})
58+
59+
it('patches the detail cache so a detail-backed editor stops being dirty after save', async () => {
60+
const mutation = useUpdateWorkspaceCredential() as any
61+
await mutation.onMutate({ credentialId: CREDENTIAL_ID, description: 'new description' })
62+
63+
expect(detailAfterMutate(existing)).toMatchObject({ description: 'new description' })
64+
})
65+
66+
it('clears the detail description when the edit passes null', async () => {
67+
const mutation = useUpdateWorkspaceCredential() as any
68+
await mutation.onMutate({ credentialId: CREDENTIAL_ID, description: null })
69+
70+
expect(detailAfterMutate(existing)).toMatchObject({ description: null })
71+
})
72+
73+
it('leaves untouched fields alone when only displayName changes', async () => {
74+
const mutation = useUpdateWorkspaceCredential() as any
75+
await mutation.onMutate({ credentialId: CREDENTIAL_ID, displayName: 'RENAMED' })
76+
77+
expect(detailAfterMutate(existing)).toMatchObject({
78+
displayName: 'RENAMED',
79+
description: 'old description',
80+
})
81+
})
82+
83+
it('rolls the detail cache back when the update fails', async () => {
84+
const mutation = useUpdateWorkspaceCredential() as any
85+
const context = await mutation.onMutate({
86+
credentialId: CREDENTIAL_ID,
87+
description: 'new description',
88+
})
89+
queryClient.setQueryData.mockClear()
90+
91+
mutation.onError(new Error('boom'), { credentialId: CREDENTIAL_ID }, context)
92+
93+
expect(queryClient.setQueryData).toHaveBeenCalledWith(
94+
expect.arrayContaining(['detail']),
95+
existing
96+
)
97+
})
98+
})

apps/sim/hooks/queries/credentials.ts

Lines changed: 33 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -156,35 +156,53 @@ export function useUpdateWorkspaceCredential() {
156156
const previousLists = queryClient.getQueriesData<WorkspaceCredential[]>({
157157
queryKey: workspaceCredentialKeys.lists(),
158158
})
159+
const previousDetail = queryClient.getQueryData<WorkspaceCredential | null>(
160+
workspaceCredentialKeys.detail(variables.credentialId)
161+
)
162+
163+
/** Applies the in-flight edit to one cached credential. */
164+
const withEdit = (cred: WorkspaceCredential): WorkspaceCredential => ({
165+
...cred,
166+
...(variables.displayName !== undefined ? { displayName: variables.displayName } : {}),
167+
...(variables.description !== undefined
168+
? { description: variables.description ?? null }
169+
: {}),
170+
})
171+
172+
/*
173+
* The detail cache is patched alongside the lists, not just cancelled: a
174+
* detail-backed editor compares its drafts against this entry to decide
175+
* whether it is dirty, so leaving it stale keeps the surface dirty after a
176+
* successful save until the `onSettled` refetch lands — long enough for
177+
* Discard to restore the pre-save value over the committed one.
178+
*/
179+
queryClient.setQueryData<WorkspaceCredential | null>(
180+
workspaceCredentialKeys.detail(variables.credentialId),
181+
(old) => (old ? withEdit(old) : old)
182+
)
159183

160184
queryClient.setQueriesData<WorkspaceCredential[]>(
161185
{ queryKey: workspaceCredentialKeys.lists() },
162186
(old) => {
163187
if (!old) return old
164-
return old.map((cred) =>
165-
cred.id === variables.credentialId
166-
? {
167-
...cred,
168-
...(variables.displayName !== undefined
169-
? { displayName: variables.displayName }
170-
: {}),
171-
...(variables.description !== undefined
172-
? { description: variables.description ?? null }
173-
: {}),
174-
}
175-
: cred
176-
)
188+
return old.map((cred) => (cred.id === variables.credentialId ? withEdit(cred) : cred))
177189
}
178190
)
179191

180-
return { previousLists }
192+
return { previousLists, previousDetail }
181193
},
182-
onError: (_err, _variables, context) => {
194+
onError: (_err, variables, context) => {
183195
if (context?.previousLists) {
184196
for (const [queryKey, data] of context.previousLists) {
185197
queryClient.setQueryData(queryKey, data)
186198
}
187199
}
200+
if (context?.previousDetail !== undefined) {
201+
queryClient.setQueryData(
202+
workspaceCredentialKeys.detail(variables.credentialId),
203+
context.previousDetail
204+
)
205+
}
188206
},
189207
onSettled: (_data, _error, variables) => {
190208
queryClient.invalidateQueries({

0 commit comments

Comments
 (0)