Skip to content

Commit f6fe47b

Browse files
waleedlatif1waleedlatif1
committed
fix(settings): retain drafts until history traversal occurs
1 parent ea7eb4d commit f6fe47b

5 files changed

Lines changed: 187 additions & 22 deletions

File tree

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -352,6 +352,11 @@ shells. Reach for it before hand-rolling a `Chip`.
352352
- **Native history needs entry indexes.** The Navigation API supplies them for
353353
legacy entries. Without it, native traversal to an unindexed entry cannot be
354354
cancelled reliably; never guess a direction or rewrite the history stack.
355+
- **History confirmation authorizes traversal before discarding.** Drafts are
356+
discarded only when the browser reports the confirmed traversal, so a no-op
357+
Back or Forward retains edits. Known cross-document traversal uses native unload
358+
protection. An unknown cross-document target on a classic History browser can
359+
require a second native confirmation after the shared dialog.
355360
- **Detail back controls** call `guard.guardBack(closeFn)`. Other destructive view
356361
transitions, such as switching an editor's direction or environment, also use
357362
the shared `requestLeave` action before resetting their draft.

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -349,6 +349,11 @@ shells. Reach for it before hand-rolling a `Chip`.
349349
- **Native history needs entry indexes.** The Navigation API supplies them for
350350
legacy entries. Without it, native traversal to an unindexed entry cannot be
351351
cancelled reliably; never guess a direction or rewrite the history stack.
352+
- **History confirmation authorizes traversal before discarding.** Drafts are
353+
discarded only when the browser reports the confirmed traversal, so a no-op
354+
Back or Forward retains edits. Known cross-document traversal uses native unload
355+
protection. An unknown cross-document target on a classic History browser can
356+
require a second native confirmation after the shared dialog.
352357
- **Detail back controls** call `guard.guardBack(closeFn)`. Other destructive view
353358
transitions, such as switching an editor's direction or environment, also use
354359
the shared `requestLeave` action before resetting their draft.

‎apps/sim/components/settings/use-settings-browser-navigation.test.tsx‎

Lines changed: 134 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,10 +26,15 @@ function Surface({ dirty, blocked }: SurfaceProps) {
2626
return null
2727
}
2828

29-
function LinkedEditor() {
29+
interface LinkedEditorProps {
30+
blocked?: boolean
31+
}
32+
33+
function LinkedEditor({ blocked }: LinkedEditorProps) {
3034
const [value, setValue] = useState('')
3135
const guard = useSettingsUnsavedGuard({
3236
isDirty: value.length > 0,
37+
navigationBlocked: blocked,
3338
onDiscard: () => setValue(''),
3439
})
3540
return (
@@ -116,6 +121,133 @@ describe('native settings navigation', () => {
116121
expect(window.history.state).toEqual(state)
117122
})
118123

124+
it.each(['first Back', 'last Forward', 'large Back', 'large Forward'] as const)(
125+
'retains a draft when %s has no destination',
126+
async (operation) => {
127+
if (operation === 'first Back') {
128+
nativeGo.call(window.history, -(window.history.length - 1))
129+
await settle()
130+
}
131+
routedPaths = []
132+
act(() => root.render(<LinkedEditor />))
133+
const field = container.querySelector<HTMLInputElement>('input[aria-label="Draft"]')
134+
const setter = Object.getOwnPropertyDescriptor(HTMLInputElement.prototype, 'value')?.set
135+
if (!field || !setter) throw new Error('Missing draft input')
136+
act(() => {
137+
setter.call(field, 'authored')
138+
field.dispatchEvent(new Event('input', { bubbles: true }))
139+
})
140+
const currentUrl = window.location.href
141+
if (operation === 'first Back') window.history.back()
142+
else if (operation === 'last Forward') window.history.forward()
143+
else window.history.go(operation === 'large Back' ? -2_147_483_647 : 2_147_483_647)
144+
act(() => useSettingsDirtyStore.getState().confirmLeave())
145+
await settle()
146+
expect(field.value).toBe('authored')
147+
expect(window.location.href).toBe(currentUrl)
148+
expect(routedPaths).toEqual([])
149+
expect(useSettingsDirtyStore.getState().pendingLeave).toBeNull()
150+
const unload = new Event('beforeunload', { cancelable: true })
151+
window.dispatchEvent(unload)
152+
expect(unload.defaultPrevented).toBe(true)
153+
}
154+
)
155+
156+
it.each([false, true])(
157+
'discards a confirmed draft only after actual traversal (unstamped: %s)',
158+
async (unstamped) => {
159+
if (unstamped) {
160+
nativePush.call(window.history, { router: 'legacy-forward' }, '', '/legacy-forward')
161+
nativeGo.call(window.history, -1)
162+
await settle()
163+
}
164+
act(() => root.render(<LinkedEditor />))
165+
const field = container.querySelector<HTMLInputElement>('input[aria-label="Draft"]')
166+
const setter = Object.getOwnPropertyDescriptor(HTMLInputElement.prototype, 'value')?.set
167+
if (!field || !setter) throw new Error('Missing draft input')
168+
act(() => {
169+
setter.call(field, 'authored')
170+
field.dispatchEvent(new Event('input', { bubbles: true }))
171+
})
172+
if (unstamped) window.history.forward()
173+
else window.history.back()
174+
act(() => useSettingsDirtyStore.getState().confirmLeave())
175+
expect(field.value).toBe('authored')
176+
await settle()
177+
expect(window.location.pathname).toBe(unstamped ? '/legacy-forward' : '/prior')
178+
expect(field.value).toBe('')
179+
expect(useSettingsDirtyStore.getState().isDirty).toBe(false)
180+
}
181+
)
182+
183+
it('retains a confirmed draft if a save begins before traversal', async () => {
184+
act(() => root.render(<LinkedEditor />))
185+
const field = container.querySelector<HTMLInputElement>('input[aria-label="Draft"]')
186+
const setter = Object.getOwnPropertyDescriptor(HTMLInputElement.prototype, 'value')?.set
187+
if (!field || !setter) throw new Error('Missing draft input')
188+
act(() => {
189+
setter.call(field, 'authored')
190+
field.dispatchEvent(new Event('input', { bubbles: true }))
191+
})
192+
window.history.back()
193+
act(() => useSettingsDirtyStore.getState().confirmLeave())
194+
act(() => root.render(<LinkedEditor blocked />))
195+
await settle()
196+
expect(field.value).toBe('authored')
197+
expect(window.location.pathname).toBe('/editor')
198+
expect(routedPaths).not.toContain('/prior')
199+
expect(useSettingsDirtyStore.getState().pendingLeave).toBeNull()
200+
act(() => root.render(<LinkedEditor />))
201+
window.history.back()
202+
act(() => useSettingsDirtyStore.getState().confirmLeave())
203+
await settle()
204+
expect(window.location.pathname).toBe('/prior')
205+
expect(field.value).toBe('')
206+
})
207+
208+
it.each([-1.5, 4_294_967_295])(
209+
'confirms the browser-converted traversal delta %s',
210+
async (delta) => {
211+
act(() => root.render(<LinkedEditor />))
212+
const field = container.querySelector<HTMLInputElement>('input[aria-label="Draft"]')
213+
const setter = Object.getOwnPropertyDescriptor(HTMLInputElement.prototype, 'value')?.set
214+
if (!field || !setter) throw new Error('Missing draft input')
215+
act(() => {
216+
setter.call(field, 'authored')
217+
field.dispatchEvent(new Event('input', { bubbles: true }))
218+
})
219+
window.history.go(delta)
220+
act(() => useSettingsDirtyStore.getState().confirmLeave())
221+
await settle()
222+
expect(window.location.pathname).toBe('/prior')
223+
expect(field.value).toBe('')
224+
expect(useSettingsDirtyStore.getState().pendingLeave).toBeNull()
225+
}
226+
)
227+
228+
it.each([false, true])(
229+
'retains a draft when an unresolved traversal only changes the hash (saving: %s)',
230+
async (blocked) => {
231+
window.history.pushState(new Date('2026-01-01T00:00:00Z'), '', '/editor#opaque')
232+
act(() => root.render(<LinkedEditor />))
233+
const field = container.querySelector<HTMLInputElement>('input[aria-label="Draft"]')
234+
const setter = Object.getOwnPropertyDescriptor(HTMLInputElement.prototype, 'value')?.set
235+
if (!field || !setter) throw new Error('Missing draft input')
236+
act(() => {
237+
setter.call(field, 'authored')
238+
field.dispatchEvent(new Event('input', { bubbles: true }))
239+
})
240+
window.history.back()
241+
act(() => useSettingsDirtyStore.getState().confirmLeave())
242+
act(() => root.render(<LinkedEditor blocked={blocked} />))
243+
await settle()
244+
expect(field.value).toBe('authored')
245+
expect(window.location.pathname).toBe('/editor')
246+
expect(window.location.hash).toBe('')
247+
expect(useSettingsDirtyStore.getState().isDirty).toBe(true)
248+
}
249+
)
250+
119251
it('does not reuse confirmation from a traversal that emitted no event for a later draft', async () => {
120252
act(() => root.render(<LinkedEditor />))
121253
const field = container.querySelector<HTMLInputElement>('input[aria-label="Draft"]')
@@ -130,7 +262,7 @@ describe('native settings navigation', () => {
130262
act(() => useSettingsDirtyStore.getState().confirmLeave())
131263
await settle()
132264
expect(window.location.pathname).toBe('/editor')
133-
expect(field.value).toBe('')
265+
expect(field.value).toBe('original draft')
134266
for (let index = 0; index < steps; index++)
135267
window.history.pushState({ router: 'later' }, '', `/later-${index}`)
136268
const currentPath = window.location.pathname

‎apps/sim/components/settings/use-settings-browser-navigation.ts‎

Lines changed: 24 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -93,14 +93,20 @@ function installBrowserNavigationGuard() {
9393
}
9494

9595
history.go = (delta) => {
96+
// History.go converts its delta to a signed 32-bit integer before traversing.
97+
delta = (delta ?? 0) | 0
9698
cancelPendingTraversal()
97-
if (!delta) {
99+
if (!delta || Math.abs(delta) >= history.length) {
98100
originalGo.call(history, delta)
99101
return
100102
}
101103
const sourceIndex = entryIndex(history.state)
102104
const targetIndex = sourceIndex === null ? null : sourceIndex + delta
103105
const target = navigation?.entries().find((entry) => entry.index === targetIndex)
106+
if (navigation?.currentEntry && (!target || !target.sameDocument)) {
107+
originalGo.call(history, delta)
108+
return
109+
}
104110
const targetRoute = navigation
105111
? target?.sameDocument && target.url
106112
? route(new URL(target.url))
@@ -113,7 +119,7 @@ function installBrowserNavigationGuard() {
113119
originalGo.call(history, delta)
114120
return
115121
}
116-
requestLeave(() => traverse(delta))
122+
requestLeave(() => traverse(delta), { discardOnConfirm: false })
117123
}
118124
history.back = () => history.go(-1)
119125
history.forward = () => history.go(1)
@@ -124,10 +130,23 @@ function installBrowserNavigationGuard() {
124130
const index = entryIndex(event.state)
125131
const destinationUrl = new URL(window.location.href)
126132
if (index !== null) trackedRoutes.set(index, route(destinationUrl))
133+
if (!restoring && route(destinationUrl) === route(currentUrl)) {
134+
currentIndex = index
135+
currentUrl = destinationUrl
136+
allowTraversal = false
137+
return
138+
}
127139
if (
128140
allowTraversal &&
129141
(index === null || currentIndex === null || index - currentIndex === pendingDelta)
130142
) {
143+
if (useSettingsDirtyStore.getState().navigationBlocked) {
144+
event.stopImmediatePropagation()
145+
allowTraversal = false
146+
originalGo.call(history, -pendingDelta)
147+
return
148+
}
149+
useSettingsDirtyStore.getState().discardDrafts()
131150
currentIndex = index ?? (currentIndex === null ? null : currentIndex + pendingDelta)
132151
allowTraversal = false
133152
restoring = false
@@ -137,15 +156,6 @@ function installBrowserNavigationGuard() {
137156
return
138157
}
139158
allowTraversal = false
140-
if (
141-
!restoring &&
142-
destinationUrl.pathname === currentUrl.pathname &&
143-
destinationUrl.search === currentUrl.search
144-
) {
145-
currentIndex = index
146-
currentUrl = destinationUrl
147-
return
148-
}
149159
if (index === null || currentIndex === null) {
150160
currentIndex = index
151161
currentUrl = destinationUrl
@@ -166,7 +176,9 @@ function installBrowserNavigationGuard() {
166176
}
167177
restoring = false
168178
const requestedDelta = pendingDelta
169-
useSettingsDirtyStore.getState().requestLeave(() => traverse(requestedDelta))
179+
useSettingsDirtyStore
180+
.getState()
181+
.requestLeave(() => traverse(requestedDelta), { discardOnConfirm: false })
170182
return
171183
}
172184
const { isDirty, navigationBlocked } = useSettingsDirtyStore.getState()

‎apps/sim/stores/settings/dirty/store.ts‎

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -12,18 +12,21 @@ interface SettingsDirtyStore {
1212
isDirty: boolean
1313
navigationBlocked: boolean
1414
guards: Record<string, SettingsGuardState>
15-
/** Leave action deferred until the user confirms discard. */
15+
/** Confirmed leave action, including its discard policy. */
1616
pendingLeave: (() => void) | null
1717
setGuard: (id: string, guard: SettingsGuardState) => void
1818
removeGuard: (id: string) => void
1919
/**
2020
* Call before leaving the current settings surface. If clean, runs `leave` immediately
2121
* and returns `true`. If dirty, stashes `leave` and returns `false` so the shared
22-
* discard dialog can confirm before running it.
22+
* discard dialog can confirm before running it. History defers discard until
23+
* an actual traversal is observed by passing `discardOnConfirm: false`.
2324
*/
24-
requestLeave: (leave: () => void) => boolean
25-
/** Discards registered drafts and runs the deferred leave action. */
25+
requestLeave: (leave: () => void, options?: { discardOnConfirm?: boolean }) => boolean
26+
/** Runs the deferred leave action after confirmation. */
2627
confirmLeave: () => void
28+
/** Discards each registered dirty draft. */
29+
discardDrafts: () => void
2730
/** Cancels a pending leave without clearing dirty state. */
2831
cancelLeave: () => void
2932
/** Resets the entire settings surface. Individual editors remove their own guard. */
@@ -58,25 +61,33 @@ export const useSettingsDirtyStore = create<SettingsDirtyStore>()(
5861

5962
removeGuard: (id) => set((state) => summarizeGuards(omit(state.guards, [id]))),
6063

61-
requestLeave: (leave) => {
64+
requestLeave: (leave, { discardOnConfirm = true } = {}) => {
6265
if (get().navigationBlocked) return false
6366
if (!get().isDirty) {
6467
leave()
6568
return true
6669
}
67-
set({ pendingLeave: leave })
70+
set({
71+
pendingLeave: () => {
72+
if (discardOnConfirm) get().discardDrafts()
73+
leave()
74+
},
75+
})
6876
return false
6977
},
7078

7179
confirmLeave: () => {
7280
const { navigationBlocked, pendingLeave } = get()
7381
if (navigationBlocked) return
7482
if (!pendingLeave) return
83+
set({ pendingLeave: null })
84+
pendingLeave()
85+
},
86+
87+
discardDrafts: () => {
7588
for (const guard of Object.values(get().guards)) {
7689
if (guard.isDirty) guard.onDiscard?.()
7790
}
78-
set({ pendingLeave: null })
79-
pendingLeave()
8091
},
8192

8293
cancelLeave: () => set({ pendingLeave: null }),

0 commit comments

Comments
 (0)