Skip to content

Commit 2c120c3

Browse files
authored
improvement(emcn): let a modal refuse every dismissal while an action runs (#6276)
* improvement(emcn): let a modal refuse every dismissal while an action runs ChipConfirmModal's docs promised "a single dismiss path shared by the header X / dismiss button / Escape … and disabling dismiss while the confirm is in flight". Only the dismiss button was ever guarded — Escape, outside-click and the header X all still closed a confirmation mid-delete. Two knowledge-base connector modals had the same shape: they guarded onOpenChange against a pending save, then handed the header X a direct onOpenChange(false) that skipped the guard. A modal now states the interlock once, as `dismissDisabled` on ChipModal or ModalContent, and the primitive holds all four exits shut. ModalContent owns the Radix paths because `{...props}` is spread after its own handlers, so a consumer-passed onEscapeKeyDown/onInteractOutside would silently drop the floating-layer guard; it publishes the flag through a context that ChipModalHeader, ChipModalFooter and ModalHeader read. The two narrow props compose with `||`, so an explicit `true` still disables a single button and an explicit `false` cannot punch a hole in the root's guarantee. Also turns on `turbo run type-check` for every workspace. packages/emcn, packages/utils, apps/desktop and apps/docs had no type check in CI at all — only @sim/realtime did — and apps/sim's source was covered solely as a side effect of `next build`. All 23 workspaces pass today, so it lands green. * fix(emcn): compose consumer dismiss handlers instead of replacing the guard ModalContent's own onEscapeKeyDown/onInteractOutside sit before the `{...props}` spread, so a consumer passing either replaced them — dropping both the dismissDisabled interlock and the floating-layer guard that keeps a popper dismissal from closing the modal and freezing the page. The TSDoc argued the guard had to live here for exactly that reason, then left the same spread able to defeat it. Both handlers are now destructured out of props and invoked after the guard, so the guard always runs and a consumer can still observe or extend the event. No consumer passes either today, so this was latent rather than live. * revert(ci): drop the type-check inputs allowlist The allowlist traded correctness for a modest cache win, in a gate whose only job is catching type errors. `resolveJsonModule` and `allowJs` are both on, so .json and .js files participate in type checking and were absent from the list — `lib/integrations/availability.ts` imports the generated `integrations.json`, which means regenerating that file would not have invalidated the cache and CI would have replayed a stale pass over a changed type. Back to Turbo's default (every non-gitignored file in the package): conservative, but a type-check gate that can serve a stale success is worse than a slow one.
1 parent 6c10ac2 commit 2c120c3

7 files changed

Lines changed: 347 additions & 52 deletions

File tree

.github/workflows/test-build.yml

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -178,8 +178,14 @@ jobs:
178178
fi
179179
bun run check:migrations "$BASE_REF"
180180
181-
- name: Type-check realtime server
182-
run: bunx turbo run type-check --filter=@sim/realtime
181+
# Every workspace, not just realtime. packages/emcn, packages/utils,
182+
# apps/desktop and apps/docs had no type check in CI at all; apps/sim's
183+
# source was covered only as a side effect of `next build` in the separate
184+
# Build App job. Note this does NOT cover apps/sim's tests — its tsconfig
185+
# excludes *.test.ts(x), and including them today surfaces ~2.2k errors,
186+
# so that is its own cleanup rather than a gate to switch on here.
187+
- name: Type-check all workspaces
188+
run: bunx turbo run type-check
183189

184190
# cloud-review-tools.test.ts runs the real helper on the runner, which shells
185191
# out to rg. Blacksmith's image ships it, GitHub's doesn't.

apps/sim/app/workspace/[workspaceId]/knowledge/[id]/components/add-connector-modal/add-connector-modal.tsx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -235,9 +235,10 @@ export function AddConnectorModal({
235235
<>
236236
<ChipModal
237237
open={open}
238-
onOpenChange={(val) => !isCreating && onOpenChange(val)}
238+
onOpenChange={onOpenChange}
239239
srTitle={step === 'select-type' ? 'Connect Source' : `Configure ${connectorConfig?.name}`}
240240
size='md'
241+
dismissDisabled={isCreating}
241242
>
242243
<ChipModalHeader onClose={() => onOpenChange(false)}>
243244
{step === 'configure' ? (
@@ -428,7 +429,6 @@ export function AddConnectorModal({
428429
{step === 'configure' && (
429430
<ChipModalFooter
430431
onCancel={() => onOpenChange(false)}
431-
cancelDisabled={isCreating}
432432
primaryAction={{
433433
label: isCreating ? 'Connecting…' : 'Connect & Sync',
434434
onClick: handleSubmit,

apps/sim/app/workspace/[workspaceId]/knowledge/[id]/components/edit-connector-modal/edit-connector-modal.tsx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -269,9 +269,10 @@ export function EditConnectorModal({
269269
return (
270270
<ChipModal
271271
open={open}
272-
onOpenChange={(val) => !isSaving && onOpenChange(val)}
272+
onOpenChange={onOpenChange}
273273
srTitle={`Edit ${displayName}`}
274274
size='md'
275+
dismissDisabled={isSaving}
275276
>
276277
<ChipModalHeader icon={Icon ?? null} onClose={() => onOpenChange(false)}>
277278
Edit {displayName}
@@ -312,7 +313,6 @@ export function EditConnectorModal({
312313
{activeTab === 'settings' && (
313314
<ChipModalFooter
314315
onCancel={() => onOpenChange(false)}
315-
cancelDisabled={isSaving}
316316
primaryAction={{
317317
label: isSaving ? 'Saving…' : 'Save',
318318
onClick: handleSave,
Lines changed: 219 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,219 @@
1+
/**
2+
* @vitest-environment jsdom
3+
*/
4+
import { act, type ReactNode } from 'react'
5+
import { createRoot, type Root } from 'react-dom/client'
6+
import { afterEach, describe, expect, it, vi } from 'vitest'
7+
import { Modal, ModalContent, ModalHeader } from '../modal/modal'
8+
import { ChipConfirmModal, ChipModal, ChipModalFooter, ChipModalHeader } from './chip-modal'
9+
10+
vi.mock('next/navigation', () => ({
11+
usePathname: () => '/workspace/workspace-1/home',
12+
}))
13+
14+
let root: Root | null = null
15+
let container: HTMLDivElement | null = null
16+
17+
function mount(ui: ReactNode) {
18+
;(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true
19+
container = document.createElement('div')
20+
document.body.appendChild(container)
21+
root = createRoot(container)
22+
act(() => root?.render(ui))
23+
}
24+
25+
afterEach(() => {
26+
if (root) act(() => root?.unmount())
27+
container?.remove()
28+
root = null
29+
container = null
30+
})
31+
32+
/** The dialog panel Radix renders, which owns the Escape/outside-click handlers. */
33+
function dialog(): HTMLElement {
34+
const node = document.querySelector<HTMLElement>('[role="dialog"]')
35+
if (!node) throw new Error('Dialog did not render')
36+
return node
37+
}
38+
39+
function buttonByText(text: string): HTMLButtonElement {
40+
const match = Array.from(document.querySelectorAll('button')).find((button) =>
41+
button.textContent?.includes(text)
42+
)
43+
if (!match) throw new Error(`No button containing "${text}"`)
44+
return match as HTMLButtonElement
45+
}
46+
47+
function closeButton(): HTMLButtonElement {
48+
const match = Array.from(document.querySelectorAll('button')).find((button) =>
49+
button.querySelector('.sr-only')?.textContent?.includes('Close')
50+
)
51+
if (!match) throw new Error('Close button did not render')
52+
return match as HTMLButtonElement
53+
}
54+
55+
function pressEscape() {
56+
act(() => {
57+
dialog().dispatchEvent(
58+
new KeyboardEvent('keydown', { key: 'Escape', bubbles: true, cancelable: true })
59+
)
60+
})
61+
}
62+
63+
function Harness({
64+
onOpenChange,
65+
dismissDisabled,
66+
}: {
67+
onOpenChange: (open: boolean) => void
68+
dismissDisabled?: boolean
69+
}) {
70+
return (
71+
<ChipModal
72+
open
73+
onOpenChange={onOpenChange}
74+
srTitle='Test modal'
75+
dismissDisabled={dismissDisabled}
76+
>
77+
<ChipModalHeader onClose={() => onOpenChange(false)}>Title</ChipModalHeader>
78+
<ChipModalFooter
79+
onCancel={() => onOpenChange(false)}
80+
primaryAction={{ label: 'Save', onClick: () => {} }}
81+
/>
82+
</ChipModal>
83+
)
84+
}
85+
86+
describe('ChipModal dismissDisabled', () => {
87+
it('closes through every path when not set', () => {
88+
const onOpenChange = vi.fn()
89+
mount(<Harness onOpenChange={onOpenChange} />)
90+
91+
expect(closeButton().disabled).toBe(false)
92+
expect(buttonByText('Cancel').disabled).toBe(false)
93+
94+
pressEscape()
95+
expect(onOpenChange).toHaveBeenCalledWith(false)
96+
})
97+
98+
// Outside-click is guarded by the same flag but jsdom cannot drive Radix's
99+
// outside-interaction path, so asserting it here could never fail.
100+
it('blocks the close button, Cancel and Escape when set', () => {
101+
const onOpenChange = vi.fn()
102+
mount(<Harness onOpenChange={onOpenChange} dismissDisabled />)
103+
104+
expect(closeButton().disabled).toBe(true)
105+
expect(buttonByText('Cancel').disabled).toBe(true)
106+
107+
pressEscape()
108+
expect(onOpenChange).not.toHaveBeenCalled()
109+
})
110+
111+
// Either flag disables: an explicit `false` must not re-enable a button whose
112+
// click Radix has already been told to ignore.
113+
it('cannot be re-enabled by an explicit closeDisabled or cancelDisabled of false', () => {
114+
const onOpenChange = vi.fn()
115+
mount(
116+
<ChipModal open onOpenChange={onOpenChange} srTitle='Test modal' dismissDisabled>
117+
<ChipModalHeader onClose={() => onOpenChange(false)} closeDisabled={false}>
118+
Title
119+
</ChipModalHeader>
120+
<ChipModalFooter
121+
onCancel={() => onOpenChange(false)}
122+
cancelDisabled={false}
123+
primaryAction={{ label: 'Save', onClick: () => {} }}
124+
/>
125+
</ChipModal>
126+
)
127+
128+
expect(closeButton().disabled).toBe(true)
129+
expect(buttonByText('Cancel').disabled).toBe(true)
130+
})
131+
132+
it('still lets an explicit true disable a button on its own', () => {
133+
const onOpenChange = vi.fn()
134+
mount(
135+
<ChipModal open onOpenChange={onOpenChange} srTitle='Test modal'>
136+
<ChipModalHeader onClose={() => onOpenChange(false)} closeDisabled>
137+
Title
138+
</ChipModalHeader>
139+
<ChipModalFooter
140+
onCancel={() => onOpenChange(false)}
141+
primaryAction={{ label: 'Save', onClick: () => {} }}
142+
/>
143+
</ChipModal>
144+
)
145+
146+
expect(closeButton().disabled).toBe(true)
147+
expect(buttonByText('Cancel').disabled).toBe(false)
148+
})
149+
})
150+
151+
describe('ModalContent dismissDisabled', () => {
152+
it('runs a consumer escape handler without letting it drop the guard', () => {
153+
const onOpenChange = vi.fn()
154+
const onEscapeKeyDown = vi.fn()
155+
mount(
156+
<Modal open onOpenChange={onOpenChange}>
157+
<ModalContent srTitle='Guarded' dismissDisabled onEscapeKeyDown={onEscapeKeyDown}>
158+
<input aria-label='Field' />
159+
</ModalContent>
160+
</Modal>
161+
)
162+
163+
pressEscape()
164+
expect(onEscapeKeyDown).toHaveBeenCalled()
165+
expect(onOpenChange).not.toHaveBeenCalled()
166+
})
167+
168+
it('disables the built-in ModalHeader close button', () => {
169+
const onOpenChange = vi.fn()
170+
mount(
171+
<Modal open onOpenChange={onOpenChange}>
172+
<ModalContent srTitle='Guarded' dismissDisabled>
173+
<ModalHeader>Title</ModalHeader>
174+
</ModalContent>
175+
</Modal>
176+
)
177+
178+
expect(closeButton().disabled).toBe(true)
179+
})
180+
})
181+
182+
describe('ChipConfirmModal pending', () => {
183+
it('holds every exit shut while the confirm runs', () => {
184+
const onOpenChange = vi.fn()
185+
mount(
186+
<ChipConfirmModal
187+
open
188+
onOpenChange={onOpenChange}
189+
title='Delete key'
190+
text='This cannot be undone.'
191+
confirm={{ label: 'Delete', onClick: () => {}, pending: true, pendingLabel: 'Deleting...' }}
192+
/>
193+
)
194+
195+
expect(closeButton().disabled).toBe(true)
196+
expect(buttonByText('Cancel').disabled).toBe(true)
197+
expect(buttonByText('Deleting...').disabled).toBe(true)
198+
199+
pressEscape()
200+
expect(onOpenChange).not.toHaveBeenCalled()
201+
})
202+
203+
it('dismisses normally when the confirm is idle', () => {
204+
const onOpenChange = vi.fn()
205+
mount(
206+
<ChipConfirmModal
207+
open
208+
onOpenChange={onOpenChange}
209+
title='Delete key'
210+
text='This cannot be undone.'
211+
confirm={{ label: 'Delete', onClick: () => {} }}
212+
/>
213+
)
214+
215+
expect(closeButton().disabled).toBe(false)
216+
act(() => closeButton().click())
217+
expect(onOpenChange).toHaveBeenCalledWith(false)
218+
})
219+
})

0 commit comments

Comments
 (0)