Skip to content

Commit 9b20724

Browse files
committed
fix(files): address editor battle-test regressions
1 parent c6c2eaa commit 9b20724

18 files changed

Lines changed: 1541 additions & 139 deletions

apps/sim/app/workspace/[workspaceId]/components/find-bar/find-bar.test.tsx

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ let container: HTMLDivElement
4646
let root: Root
4747

4848
beforeEach(() => {
49+
vi.stubGlobal('IS_REACT_ACT_ENVIRONMENT', true)
4950
container = document.createElement('div')
5051
document.body.appendChild(container)
5152
act(() => {
@@ -56,6 +57,7 @@ beforeEach(() => {
5657
afterEach(() => {
5758
act(() => root.unmount())
5859
container.remove()
60+
vi.unstubAllGlobals()
5961
})
6062

6163
function render(overrides: Partial<FindBarProps> = {}) {
@@ -182,6 +184,65 @@ describe('FindBar keyboard', () => {
182184
expect(props.onClose).toHaveBeenCalledTimes(1)
183185
})
184186

187+
it.each(['Next match', 'Previous match', 'Clear search', 'Close find'])(
188+
'handles Escape from the focused %s button',
189+
(label) => {
190+
const props = render({ query: 'a', count: 3 })
191+
const button = buttonByLabel(label)
192+
button.focus()
193+
const event = new KeyboardEvent('keydown', {
194+
key: 'Escape',
195+
bubbles: true,
196+
cancelable: true,
197+
})
198+
const parentKeyDown = vi.fn()
199+
document.body.addEventListener('keydown', parentKeyDown)
200+
act(() => button.dispatchEvent(event))
201+
document.body.removeEventListener('keydown', parentKeyDown)
202+
expect(props.onClose).toHaveBeenCalledOnce()
203+
expect(event.defaultPrevented).toBe(true)
204+
expect(parentKeyDown).not.toHaveBeenCalled()
205+
}
206+
)
207+
208+
it.each([{ isComposing: true }, { keyCode: 229 }])(
209+
'does not close while Escape belongs to composition (%j)',
210+
(init) => {
211+
const props = render({ query: 'a', count: 3 })
212+
press('Escape', init)
213+
expect(props.onClose).not.toHaveBeenCalled()
214+
}
215+
)
216+
217+
it.each(['Replace', 'All'])('retains focus after %s disables the last match', (label) => {
218+
const replace = {
219+
value: 'beta',
220+
onChange: vi.fn(),
221+
onReplace: vi.fn(),
222+
onReplaceAll: vi.fn(),
223+
canReplace: true,
224+
canReplaceAll: true,
225+
}
226+
const props = render({ query: 'alpha', count: 1, replace })
227+
act(() => buttonByLabel('Show replace').click())
228+
const button = Array.from(container.querySelectorAll('button')).find(
229+
(candidate) => candidate.textContent === label
230+
)!
231+
button.focus()
232+
act(() => button.click())
233+
render({
234+
...props,
235+
count: 0,
236+
replace: { ...replace, canReplace: false, canReplaceAll: false },
237+
})
238+
const replacement = container.querySelector('input[aria-label="Replace in document"]')
239+
expect(document.activeElement).toBe(replacement)
240+
act(() =>
241+
replacement!.dispatchEvent(new KeyboardEvent('keydown', { key: 'Escape', bubbles: true }))
242+
)
243+
expect(props.onClose).toHaveBeenCalledOnce()
244+
})
245+
185246
// Mid-debounce the visible matches still belong to the previous term, so
186247
// stepping through them would land on a cell the box no longer describes.
187248
it('commits instead of stepping while the results are stale', () => {

apps/sim/app/workspace/[workspaceId]/components/find-bar/find-bar.tsx

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

33
import type React from 'react'
4-
import { memo, useState } from 'react'
4+
import { memo, useRef, useState } from 'react'
55
import { Button, ChipInput, cn } from '@sim/emcn'
66
import { ChevronDown, ChevronRight, ChevronUp, Loader, Search, X } from '@sim/emcn/icons'
77

@@ -79,6 +79,7 @@ export const FindBar = memo(function FindBar({
7979
inputRef,
8080
replace,
8181
}: FindBarProps) {
82+
const replaceInputRef = useRef<HTMLInputElement>(null)
8283
const [showReplace, setShowReplace] = useState(false)
8384
const handleKeyDown = (e: React.KeyboardEvent<HTMLInputElement>) => {
8485
if (e.nativeEvent.isComposing || e.keyCode === 229) return
@@ -94,10 +95,6 @@ export const FindBar = memo(function FindBar({
9495
else onNext()
9596
return
9697
}
97-
if (e.key === 'Escape') {
98-
e.preventDefault()
99-
onClose()
100-
}
10198
}
10299

103100
const hasQuery = query.trim().length > 0
@@ -114,6 +111,13 @@ export const FindBar = memo(function FindBar({
114111

115112
return (
116113
<div
114+
onKeyDown={(event) => {
115+
if (event.key !== 'Escape') return
116+
event.stopPropagation()
117+
if (event.nativeEvent.isComposing || event.keyCode === 229) return
118+
event.preventDefault()
119+
onClose()
120+
}}
117121
className={cn(
118122
'absolute top-2 right-2 z-[var(--z-dropdown)] flex max-w-[calc(100%_-_1rem)] flex-col gap-1 rounded-lg border border-[var(--border)] bg-[var(--surface-1)] p-1 shadow-medium',
119123
replace && 'w-[min(400px,calc(100%_-_1rem))]'
@@ -211,6 +215,7 @@ export const FindBar = memo(function FindBar({
211215
<div className='flex items-center gap-1.5'>
212216
<span aria-hidden className='w-6 shrink-0' />
213217
<ChipInput
218+
ref={replaceInputRef}
214219
value={replace.value}
215220
placeholder='Replace'
216221
aria-label='Replace in document'
@@ -223,9 +228,6 @@ export const FindBar = memo(function FindBar({
223228
if (event.key === 'Enter' && replace.canReplace) {
224229
event.preventDefault()
225230
replace.onReplace()
226-
} else if (event.key === 'Escape') {
227-
event.preventDefault()
228-
onClose()
229231
}
230232
}}
231233
/>
@@ -234,7 +236,10 @@ export const FindBar = memo(function FindBar({
234236
variant='quiet'
235237
size='sm'
236238
disabled={!replace.canReplace}
237-
onClick={replace.onReplace}
239+
onClick={() => {
240+
replace.onReplace()
241+
replaceInputRef.current?.focus()
242+
}}
238243
>
239244
Replace
240245
</Button>
@@ -247,7 +252,10 @@ export const FindBar = memo(function FindBar({
247252
}
248253
disabled={!replace.canReplaceAll}
249254
aria-label='Replace all matches'
250-
onClick={replace.onReplaceAll}
255+
onClick={() => {
256+
replace.onReplaceAll()
257+
replaceInputRef.current?.focus()
258+
}}
251259
>
252260
All
253261
</Button>
Lines changed: 207 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,207 @@
1+
/** @vitest-environment jsdom */
2+
import { act, type ComponentProps, StrictMode, Suspense, startTransition } from 'react'
3+
import type { Editor } from '@tiptap/core'
4+
import { createRoot, type Root } from 'react-dom/client'
5+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
6+
import { RichMarkdownField } from '@/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/rich-markdown-field'
7+
8+
vi.mock(
9+
'@/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/mention',
10+
() => ({ useEditorMentions: vi.fn() })
11+
)
12+
vi.mock(
13+
'@/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/menus/bubble-menu',
14+
() => ({ EditorBubbleMenu: () => null })
15+
)
16+
vi.mock(
17+
'@/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/menus/image-menu',
18+
() => ({ ImageBubbleMenu: () => null })
19+
)
20+
vi.mock(
21+
'@/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/menus/link-hover-card',
22+
() => ({ LinkHoverCard: () => null })
23+
)
24+
25+
let host: HTMLDivElement
26+
let root: Root
27+
const pending = new Promise<void>(() => {})
28+
const suspended = vi.fn()
29+
interface BlockerProps {
30+
active: boolean
31+
}
32+
33+
function Blocker({ active }: BlockerProps) {
34+
if (active) {
35+
suspended()
36+
throw pending
37+
}
38+
return null
39+
}
40+
beforeEach(() => {
41+
vi.stubGlobal('IS_REACT_ACT_ENVIRONMENT', true)
42+
vi.useFakeTimers()
43+
suspended.mockClear()
44+
host = document.createElement('div')
45+
document.body.append(host)
46+
root = createRoot(host)
47+
})
48+
afterEach(async () => {
49+
await act(async () => root.unmount())
50+
await vi.advanceTimersByTimeAsync(10)
51+
host.remove()
52+
vi.useRealTimers()
53+
vi.unstubAllGlobals()
54+
})
55+
56+
describe('editability synchronization', () => {
57+
async function renderField(props: ComponentProps<typeof RichMarkdownField>) {
58+
await act(async () =>
59+
root.render(
60+
<StrictMode>
61+
<RichMarkdownField {...props} />
62+
</StrictMode>
63+
)
64+
)
65+
await act(async () => vi.advanceTimersByTimeAsync(10))
66+
return host.querySelector<HTMLElement & { editor: Editor }>('.tiptap')!.editor
67+
}
68+
69+
it.each([
70+
{ label: 'start streaming', initial: {}, next: { isStreaming: true, value: 'streamed' } },
71+
{
72+
label: 'finish streaming',
73+
initial: { isStreaming: true },
74+
next: { isStreaming: false, value: 'final' },
75+
},
76+
{ label: 'disable', initial: {}, next: { disabled: true } },
77+
{ label: 'enable', initial: { disabled: true }, next: { disabled: false } },
78+
])('does not report a local edit when props $label', async ({ initial, next }) => {
79+
const props = { value: 'body', onChange: vi.fn(), ...initial }
80+
const owner = await renderField(props)
81+
props.onChange.mockClear()
82+
expect(await renderField({ ...props, ...next })).toBe(owner)
83+
expect(owner.getText()).toBe(next.value ?? 'body')
84+
expect(props.onChange).not.toHaveBeenCalled()
85+
})
86+
87+
it('continues reporting actual edits after streaming completes', async () => {
88+
const props = { value: 'body', onChange: vi.fn(), isStreaming: true }
89+
const owner = await renderField(props)
90+
await renderField({ ...props, value: 'final', isStreaming: false })
91+
props.onChange.mockClear()
92+
await act(async () =>
93+
owner.commands.insertContentAt(owner.state.doc.content.size - 1, ' edited')
94+
)
95+
expect(props.onChange).toHaveBeenCalledExactlyOnceWith('final edited')
96+
})
97+
98+
it('continues reporting successful uploads after editability changes', async () => {
99+
const pending = Promise.withResolvers<{ url: string; alt: string }>()
100+
const props = {
101+
value: 'body',
102+
onChange: vi.fn(),
103+
disabled: true,
104+
uploadImage: vi.fn(() => pending.promise),
105+
}
106+
const owner = await renderField(props)
107+
await renderField({ ...props, disabled: false })
108+
props.onChange.mockClear()
109+
const event = new Event('paste', { bubbles: true, cancelable: true })
110+
Object.defineProperty(event, 'clipboardData', {
111+
value: {
112+
files: [new File(['image'], 'image.png', { type: 'image/png' })],
113+
items: [],
114+
types: ['Files'],
115+
getData: () => '',
116+
},
117+
})
118+
await act(async () => owner.view.dom.dispatchEvent(event))
119+
await act(async () => pending.resolve({ url: 'https://sim.ai/valid.png', alt: 'valid' }))
120+
expect(host.querySelector('img')?.getAttribute('alt')).toBe('valid')
121+
expect(props.onChange).toHaveBeenCalledOnce()
122+
expect(props.onChange.mock.calls[0][0]).toContain('https://sim.ai/valid.png')
123+
})
124+
})
125+
126+
describe('field callbacks remain tied to the committed render', () => {
127+
for (const action of ['edit', 'upload'] as const)
128+
for (const suspend of [false, true]) {
129+
it(`${action}, suspended=${suspend}`, async () => {
130+
const originalChange = vi.fn()
131+
const nextChange = vi.fn()
132+
const originalUpload = vi.fn().mockResolvedValue(null)
133+
const nextUpload = vi.fn().mockResolvedValue(null)
134+
const render = (next: boolean) =>
135+
root.render(
136+
<Suspense fallback='Waiting'>
137+
<RichMarkdownField
138+
value='body'
139+
onChange={next ? nextChange : originalChange}
140+
uploadImage={next ? nextUpload : originalUpload}
141+
/>
142+
<Blocker active={next && suspend} />
143+
</Suspense>
144+
)
145+
await act(async () => render(false))
146+
await act(async () => vi.advanceTimersByTimeAsync(10))
147+
const owner = host.querySelector<HTMLElement & { editor: Editor }>('.tiptap')!.editor
148+
await act(async () => {
149+
if (suspend) startTransition(() => render(true))
150+
else render(true)
151+
})
152+
if (suspend) expect(suspended).toHaveBeenCalled()
153+
expect(host.querySelector<HTMLElement & { editor: Editor }>('.tiptap')!.editor).toBe(owner)
154+
expect(owner.getText()).toBe('body')
155+
if (action === 'edit') await act(async () => owner.commands.insertContentAt(1, 'typed '))
156+
else {
157+
const event = new Event('paste', { bubbles: true, cancelable: true })
158+
Object.defineProperty(event, 'clipboardData', {
159+
value: {
160+
files: [new File(['image'], 'image.png', { type: 'image/png' })],
161+
items: [],
162+
types: ['Files'],
163+
getData: () => '',
164+
},
165+
})
166+
await act(async () => owner.view.dom.dispatchEvent(event))
167+
}
168+
const original = action === 'edit' ? originalChange : originalUpload
169+
const next = action === 'edit' ? nextChange : nextUpload
170+
expect({ committed: original.mock.calls.length, next: next.mock.calls.length }).toEqual(
171+
suspend ? { committed: 1, next: 0 } : { committed: 0, next: 1 }
172+
)
173+
})
174+
}
175+
176+
it('ignores completion after React unmount before TipTap delayed destruction', async () => {
177+
const change = vi.fn()
178+
const pendingUpload = Promise.withResolvers<{ url: string; alt: string } | null>()
179+
await act(async () =>
180+
root.render(
181+
<RichMarkdownField
182+
value='body'
183+
onChange={change}
184+
uploadImage={() => pendingUpload.promise}
185+
/>
186+
)
187+
)
188+
await act(async () => vi.advanceTimersByTimeAsync(10))
189+
const owner = host.querySelector<HTMLElement & { editor: Editor }>('.tiptap')!.editor
190+
const event = new Event('paste', { bubbles: true, cancelable: true })
191+
Object.defineProperty(event, 'clipboardData', {
192+
value: {
193+
files: [new File(['image'], 'image.png', { type: 'image/png' })],
194+
items: [],
195+
types: ['Files'],
196+
getData: () => '',
197+
},
198+
})
199+
await act(async () => owner.view.dom.dispatchEvent(event))
200+
const before = owner.getJSON()
201+
await act(async () => root.render(null))
202+
expect(owner.isDestroyed).toBe(false)
203+
await act(async () => pendingUpload.resolve({ url: 'https://sim.ai/image.png', alt: 'late' }))
204+
expect(owner.getJSON()).toEqual(before)
205+
expect(change).not.toHaveBeenCalled()
206+
})
207+
})

0 commit comments

Comments
 (0)