Skip to content

Commit 22fe83b

Browse files
committed
fix(settings): preserve drafts and guard settings navigation
1 parent 27cf138 commit 22fe83b

52 files changed

Lines changed: 2991 additions & 1295 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

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

Lines changed: 15 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -339,34 +339,21 @@ rendered through `<SettingsActionChips actions={…} />` from
339339
`@/components/settings/settings-header` — that is the shared chip path, and it
340340
is what keeps tone/icon/variant/tooltip handling from drifting between the two
341341
shells. Reach for it before hand-rolling a `Chip`.
342-
- **`useSettingsUnsavedGuard({ isDirty })`** (`…/settings/hooks/use-settings-unsaved-guard`)
343-
— syncs the page's local `isDirty` into the shared `useSettingsDirtyStore` (so
344-
the sidebar's **section-switch** confirm + the centralized `beforeunload` both
345-
apply for free) and returns `{ showUnsavedModal, setShowUnsavedModal, guardBack,
346-
confirmDiscard }` for a detail view's **in-view back** chip.
347-
- **Top-level pages** (whitelabeling, sso): call it **unassigned** —
348-
`useSettingsUnsavedGuard({ isDirty: hasChanges })` — they only need the
349-
store-sync; the sidebar/`beforeunload` do the rest.
350-
- **Detail sub-views** (data-retention, access-control group-detail): route the
351-
back chip through `onClick={() => guard.guardBack(closeFn)}` and render the
352-
shared `<UnsavedChangesModal open={guard.showUnsavedModal}
353-
onOpenChange={guard.setShowUnsavedModal} onDiscard={guard.confirmDiscard} />`
354-
(from `@/app/workspace/[workspaceId]/components/credential-detail`). The
355-
in-view header **Discard** chip (via `saveDiscardActions({ onDiscard })`) is a
356-
*reset to original* — distinct from the back-confirm's discard, which leaves.
357-
- **`useSettingsBeforeUnload`** is mounted by the settings shells
358-
(`settings/layout.tsx` and `components/settings/standalone-settings-shell.tsx`) —
359-
never add a per-page `beforeunload`.
360-
- **Dirty *computation* stays local** (shapes differ: field-compare vs
361-
normalize+stringify) — only how dirty is *consumed* is shared. Derive it (a
362-
`const`/`useMemo`), never store it in `useState`.
363-
- **CRITICAL — rules of hooks:** call `useSettingsUnsavedGuard(...)`
364-
**unconditionally, before every early-return gate** (entitlement / loading /
365-
not-entitled `return <SettingsEmptyState>`). A hook placed after a gate is
366-
skipped on gated renders and crashes.
367-
- The route-based credential detail keeps its own `useUnsavedChangesGuard` (it
368-
guards real `router.push` navigation + browser Back via a history sentinel);
369-
it already shares `UnsavedChangesModal`, so copy stays unified.
342+
- **`useSettingsUnsavedGuard({ isDirty, navigationBlocked, onDiscard })`** from
343+
`@/components/settings/use-settings-unsaved-guard` registers one editor with the
344+
shared settings store. Register every full-page draft, including inline creation
345+
forms. Dirty computation stays local and derived; fetched defaults stay clean.
346+
- **`onDiscard` resets the registered draft.** The root `SettingsNavigationGuard`
347+
owns the discard dialog, internal link interception, browser Back/Forward, and
348+
refresh protection. Do not add a per-page dialog or history listener.
349+
- **Detail back controls** call `guard.guardBack(closeFn)`. Other destructive view
350+
transitions, such as switching an editor's direction or environment, also use
351+
the shared `requestLeave` action before resetting their draft.
352+
- **`navigationBlocked` covers pending saves and uploads.** The shared guard holds
353+
navigation until they settle. Disable or preserve edits made during requests;
354+
failed saves retain drafts, and successful saves only clear committed values.
355+
- **Call the hook unconditionally, before every early-return gate** (entitlement,
356+
loading, or empty-state return), so gated renders preserve hook order.
370357

371358
## Detail sub-views
372359

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

Lines changed: 15 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -336,34 +336,21 @@ rendered through `<SettingsActionChips actions={…} />` from
336336
`@/components/settings/settings-header` — that is the shared chip path, and it
337337
is what keeps tone/icon/variant/tooltip handling from drifting between the two
338338
shells. Reach for it before hand-rolling a `Chip`.
339-
- **`useSettingsUnsavedGuard({ isDirty })`** (`…/settings/hooks/use-settings-unsaved-guard`)
340-
— syncs the page's local `isDirty` into the shared `useSettingsDirtyStore` (so
341-
the sidebar's **section-switch** confirm + the centralized `beforeunload` both
342-
apply for free) and returns `{ showUnsavedModal, setShowUnsavedModal, guardBack,
343-
confirmDiscard }` for a detail view's **in-view back** chip.
344-
- **Top-level pages** (whitelabeling, sso): call it **unassigned** —
345-
`useSettingsUnsavedGuard({ isDirty: hasChanges })` — they only need the
346-
store-sync; the sidebar/`beforeunload` do the rest.
347-
- **Detail sub-views** (data-retention, access-control group-detail): route the
348-
back chip through `onClick={() => guard.guardBack(closeFn)}` and render the
349-
shared `<UnsavedChangesModal open={guard.showUnsavedModal}
350-
onOpenChange={guard.setShowUnsavedModal} onDiscard={guard.confirmDiscard} />`
351-
(from `@/app/workspace/[workspaceId]/components/credential-detail`). The
352-
in-view header **Discard** chip (via `saveDiscardActions({ onDiscard })`) is a
353-
*reset to original* — distinct from the back-confirm's discard, which leaves.
354-
- **`useSettingsBeforeUnload`** is mounted by the settings shells
355-
(`settings/layout.tsx` and `components/settings/standalone-settings-shell.tsx`) —
356-
never add a per-page `beforeunload`.
357-
- **Dirty *computation* stays local** (shapes differ: field-compare vs
358-
normalize+stringify) — only how dirty is *consumed* is shared. Derive it (a
359-
`const`/`useMemo`), never store it in `useState`.
360-
- **CRITICAL — rules of hooks:** call `useSettingsUnsavedGuard(...)`
361-
**unconditionally, before every early-return gate** (entitlement / loading /
362-
not-entitled `return <SettingsEmptyState>`). A hook placed after a gate is
363-
skipped on gated renders and crashes.
364-
- The route-based credential detail keeps its own `useUnsavedChangesGuard` (it
365-
guards real `router.push` navigation + browser Back via a history sentinel);
366-
it already shares `UnsavedChangesModal`, so copy stays unified.
339+
- **`useSettingsUnsavedGuard({ isDirty, navigationBlocked, onDiscard })`** from
340+
`@/components/settings/use-settings-unsaved-guard` registers one editor with the
341+
shared settings store. Register every full-page draft, including inline creation
342+
forms. Dirty computation stays local and derived; fetched defaults stay clean.
343+
- **`onDiscard` resets the registered draft.** The root `SettingsNavigationGuard`
344+
owns the discard dialog, internal link interception, browser Back/Forward, and
345+
refresh protection. Do not add a per-page dialog or history listener.
346+
- **Detail back controls** call `guard.guardBack(closeFn)`. Other destructive view
347+
transitions, such as switching an editor's direction or environment, also use
348+
the shared `requestLeave` action before resetting their draft.
349+
- **`navigationBlocked` covers pending saves and uploads.** The shared guard holds
350+
navigation until they settle. Disable or preserve edits made during requests;
351+
failed saves retain drafts, and successful saves only clear committed values.
352+
- **Call the hook unconditionally, before every early-return gate** (entitlement,
353+
loading, or empty-state return), so gated renders preserve hook order.
367354

368355
## Detail sub-views
369356

‎apps/sim/app/layout.tsx‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import type { Metadata, Viewport } from 'next'
33
import Script from 'next/script'
44
import { NuqsAdapter } from 'nuqs/adapters/next/app'
55
import { BrandedLayout } from '@/components/branded-layout'
6+
import { SettingsNavigationGuard } from '@/components/settings/settings-navigation-guard'
67
import { PasteAdmissionGuard } from '@/app/_shell/paste-admission-guard'
78
import { BrowserTelemetry } from '@/app/_shell/providers/browser-telemetry'
89
import { PostHogProvider } from '@/app/_shell/providers/posthog-provider'
@@ -45,6 +46,7 @@ export default function RootLayout({ children }: { children: React.ReactNode })
4546
const themeCSS = generateThemeCSS()
4647
const application = (
4748
<ToastProvider>
49+
<SettingsNavigationGuard />
4850
<DesktopUpdateNotification />
4951
<PasteAdmissionGuard />
5052
<PostHogProvider consentRequired={isHosted}>

‎apps/sim/app/o/[organizationId]/settings/integrations/sources/[connectorId]/source-detail.tsx‎

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@ import type { ResourceScope } from '@/lib/core/resource-scope'
1414
import { organizationRoutes } from '@/lib/navigation/paths'
1515
import { describeSearchSource } from '@/lib/sim-search/source-identity'
1616
import { useOrganizationContext } from '@/app/o/[organizationId]/providers/organization-provider'
17-
import { UnsavedChangesModal } from '@/app/workspace/[workspaceId]/components/credential-detail/components/unsaved-changes-modal'
1817
import { ConnectorActionFeedback } from '@/app/workspace/[workspaceId]/knowledge/[id]/components/connectors-section/connector-actions'
1918
import { getConnectorSyncState } from '@/app/workspace/[workspaceId]/knowledge/[id]/components/connectors-section/connector-sync-state'
2019
import { useConnectorActions } from '@/app/workspace/[workspaceId]/knowledge/[id]/components/connectors-section/use-connector-actions'
@@ -341,7 +340,11 @@ function SourceSettingsForm({
341340
isSearchIndex: true,
342341
onSaved,
343342
})
344-
const guard = useSettingsUnsavedGuard({ isDirty: form.dirty, navigationBlocked: form.saving })
343+
const guard = useSettingsUnsavedGuard({
344+
isDirty: form.dirty,
345+
navigationBlocked: form.saving,
346+
onDiscard,
347+
})
345348
return (
346349
<SourcePanel
347350
connector={connector}
@@ -370,11 +373,6 @@ function SourceSettingsForm({
370373
<div className='-mx-2 flex flex-col gap-4'>
371374
<ConnectorSettingsFields {...form.fieldsProps} />
372375
</div>
373-
<UnsavedChangesModal
374-
open={guard.showUnsavedModal}
375-
onOpenChange={guard.setShowUnsavedModal}
376-
onDiscard={guard.confirmDiscard}
377-
/>
378376
</SourcePanel>
379377
)
380378
}

‎apps/sim/app/o/[organizationId]/settings/layout.tsx‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,15 +2,13 @@
22

33
import type { ReactNode } from 'react'
44
import { SettingsPendingSection } from '@/components/settings/settings-pending-section'
5-
import { useSettingsBeforeUnload } from '@/components/settings/use-settings-before-unload'
65
import { resolveOrganizationSurfaceHeaderMeta } from '@/app/o/[organizationId]/settings/navigation'
76

87
interface OrganizationSettingsLayoutProps {
98
children: ReactNode
109
}
1110

1211
export default function OrganizationSettingsLayout({ children }: OrganizationSettingsLayoutProps) {
13-
useSettingsBeforeUnload()
1412
return (
1513
<div className='flex h-full flex-col bg-[var(--bg)]'>
1614
<SettingsPendingSection resolveMeta={resolveOrganizationSurfaceHeaderMeta}>
Lines changed: 121 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,13 @@
11
'use client'
22

3-
import { useCallback, useState } from 'react'
3+
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 { useUnsavedChangesGuard } from '@/app/workspace/[workspaceId]/components/credential-detail/hooks/use-unsaved-changes-guard'
7+
import { useRouter } from 'next/navigation'
8+
import { useSettingsUnsavedGuard } from '@/components/settings/use-settings-unsaved-guard'
89
import { useUpdateWorkspaceCredential, type WorkspaceCredential } from '@/hooks/queries/credentials'
10+
import { useSettingsDirtyStore } from '@/stores/settings/dirty/store'
911

1012
const logger = createLogger('CredentialDetailForm')
1113

@@ -25,6 +27,20 @@ export interface CredentialDetailFormSection {
2527
discard: () => void
2628
}
2729

30+
interface CredentialMetadata {
31+
displayName: string
32+
description: string
33+
unredacted: boolean
34+
}
35+
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+
2844
interface UseCredentialDetailFormParams {
2945
workspaceId?: string
3046
credential: WorkspaceCredential | null
@@ -33,9 +49,7 @@ interface UseCredentialDetailFormParams {
3349
backHref: string
3450
/**
3551
* An additional editable section on the page, folded into one dirty state, one
36-
* save, and one unsaved-changes guard. Two independent guards on a page cannot
37-
* coexist: each seeds its own same-URL history entry while dirty, so Back would
38-
* pop only one of them and leave the other stranded.
52+
* save, and one unsaved-changes guard.
3953
*/
4054
section?: CredentialDetailFormSection
4155
}
@@ -55,43 +69,101 @@ export function useCredentialDetailForm({
5569
section,
5670
}: UseCredentialDetailFormParams) {
5771
const updateCredential = useUpdateWorkspaceCredential(workspaceId)
72+
const isSaving = updateCredential.isPending || (section?.isSaving ?? false)
5873

59-
const [displayNameDraft, setDisplayNameDraft] = useState('')
60-
const [descriptionDraft, setDescriptionDraft] = useState('')
61-
const [unredactedDraft, setUnredactedDraft] = useState(false)
62-
const [seededCredentialId, setSeededCredentialId] = useState<string | null>(null)
63-
64-
// Seed drafts when the credential first resolves (or the route id changes); a
65-
// background refetch of the same credential must not clobber an in-progress
66-
// edit — Discard is the one way to reset.
67-
/** Applies a credential to every draft — the one definition of "reset to server state". */
68-
const seedDrafts = useCallback((source: WorkspaceCredential) => {
69-
setDisplayNameDraft(source.displayName)
70-
setDescriptionDraft(source.description ?? '')
71-
setUnredactedDraft(source.unredacted)
72-
}, [])
73-
74-
if (credential && credential.id !== seededCredentialId) {
75-
setSeededCredentialId(credential.id)
76-
seedDrafts(credential)
74+
const savedValues = {
75+
displayName: credential?.displayName ?? '',
76+
description: credential?.description ?? '',
77+
unredacted: credential?.unredacted ?? false,
7778
}
79+
const [draft, setDraft] = useState<{
80+
credentialId: string
81+
baseline: typeof savedValues
82+
values: typeof savedValues
83+
} | null>(null)
84+
85+
if (draft && credential && draft.credentialId !== credential.id) setDraft(null)
7886

79-
const isDisplayNameDirty = credential ? displayNameDraft !== credential.displayName : false
80-
const isDescriptionDirty = credential
81-
? descriptionDraft !== (credential.description || '')
82-
: false
83-
const isUnredactedDirty = credential ? unredactedDraft !== credential.unredacted : false
87+
const values = draft?.values ?? savedValues
88+
const baseline = draft?.baseline ?? savedValues
89+
const displayNameDraft = values.displayName
90+
const descriptionDraft = values.description
91+
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
8495
const isMetadataDirty = isDisplayNameDirty || isDescriptionDirty || isUnredactedDirty
96+
97+
const updateDraft = useCallback(
98+
(change: Partial<typeof savedValues>) => {
99+
if (!credential) return
100+
setDraft((current) => {
101+
const baseline = current?.baseline ?? {
102+
displayName: credential.displayName,
103+
description: credential.description ?? '',
104+
unredacted: credential.unredacted,
105+
}
106+
const values = { ...(current?.values ?? baseline), ...change }
107+
if (!isSaving && sameMetadata(values, baseline)) return null
108+
return { credentialId: credential.id, baseline, values }
109+
})
110+
},
111+
[credential, isSaving]
112+
)
113+
const setDisplayNameDraft = useCallback(
114+
(displayName: string) => updateDraft({ displayName }),
115+
[updateDraft]
116+
)
117+
const setDescriptionDraft = useCallback(
118+
(description: string) => updateDraft({ description }),
119+
[updateDraft]
120+
)
121+
const setUnredactedDraft = useCallback(
122+
(unredacted: boolean) => updateDraft({ unredacted }),
123+
[updateDraft]
124+
)
125+
85126
const isSectionDirty = section?.isDirty ?? false
86127
const isDirty = isMetadataDirty || isSectionDirty
87-
const isSaving = updateCredential.isPending || (section?.isSaving ?? false)
128+
const router = useRouter()
129+
const discard = useCallback(() => {
130+
if (isSaving) return
131+
setDraft(null)
132+
section?.discard()
133+
}, [isSaving, section])
134+
135+
const guard = useSettingsUnsavedGuard({
136+
isDirty,
137+
navigationBlocked: isSaving,
138+
onDiscard: discard,
139+
})
140+
const handleBackClick = useCallback(
141+
(event: MouseEvent<HTMLAnchorElement>) => {
142+
const { isDirty, navigationBlocked } = useSettingsDirtyStore.getState()
143+
if (!isDirty && !navigationBlocked) return
144+
event.preventDefault()
145+
guard.guardBack(() => router.push(backHref))
146+
},
147+
[guard.guardBack, router, backHref]
148+
)
88149

89-
const guard = useUnsavedChangesGuard({ isDirty, backHref })
150+
const releaseUnchangedDraft = useCallback(() => {
151+
setDraft((current) =>
152+
current && sameMetadata(current.values, current.baseline) ? null : current
153+
)
154+
}, [])
90155

91156
const save = useCallback(async () => {
92157
if (!credential || isSaving) return
93-
if (isSectionDirty && !(await section?.save())) return
94-
if (!isAdmin || !isMetadataDirty) return
158+
const submitted = draft
159+
if (isSectionDirty && !(await section?.save())) {
160+
releaseUnchangedDraft()
161+
return
162+
}
163+
if (!isAdmin || !isMetadataDirty) {
164+
releaseUnchangedDraft()
165+
return
166+
}
95167

96168
try {
97169
await updateCredential.mutateAsync({
@@ -100,16 +172,29 @@ export function useCredentialDetailForm({
100172
...(isDescriptionDirty ? { description: descriptionDraft.trim() || null } : {}),
101173
...(isUnredactedDirty ? { unredacted: unredactedDraft } : {}),
102174
})
103-
if (isDisplayNameDirty) setDisplayNameDraft((value) => value.trim())
104-
if (isDescriptionDirty) setDescriptionDraft((value) => value.trim())
175+
setDraft((current) => {
176+
if (current === submitted) return null
177+
if (!current || !submitted || current.credentialId !== submitted.credentialId)
178+
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+
}
187+
})
105188
} catch (error) {
189+
releaseUnchangedDraft()
106190
toast.error("Couldn't save changes", {
107191
description: getErrorMessage(error, 'Please try again in a moment.'),
108192
})
109193
logger.error('Failed to save credential details', error)
110194
}
111195
}, [
112196
credential,
197+
draft,
113198
isAdmin,
114199
isMetadataDirty,
115200
isSectionDirty,
@@ -122,13 +207,9 @@ export function useCredentialDetailForm({
122207
descriptionDraft,
123208
unredactedDraft,
124209
updateCredential.mutateAsync,
210+
releaseUnchangedDraft,
125211
])
126212

127-
const discard = useCallback(() => {
128-
if (credential) seedDrafts(credential)
129-
section?.discard()
130-
}, [credential, section, seedDrafts])
131-
132213
return {
133214
displayNameDraft,
134215
setDisplayNameDraft,
@@ -140,9 +221,6 @@ export function useCredentialDetailForm({
140221
save,
141222
discard,
142223
isSaving,
143-
handleBackClick: guard.handleBackClick,
144-
showUnsavedAlert: guard.showUnsavedAlert,
145-
setShowUnsavedAlert: guard.setShowUnsavedAlert,
146-
confirmDiscard: guard.confirmDiscard,
224+
handleBackClick,
147225
}
148226
}

0 commit comments

Comments
 (0)