improvement(emcn): let a modal refuse every dismissal while an action runs - #6276
Conversation
… 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.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryMedium Risk Overview Fixes add/edit connector modals that only guarded CI runs Reviewed by Cursor Bugbot for commit 03f6449. Configure here. |
Greptile SummaryThe PR adds a shared modal dismissal interlock and applies it to confirmation and connector modals while expanding CI type-checking.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the current handler composition fixes the previously reported dismissal-guard replacement path.
|
| Filename | Overview |
|---|---|
| packages/emcn/src/components/modal/modal.tsx | Adds dismissal context and composes consumer handlers after authoritative Escape and outside-interaction guards, fully addressing the prior handler-replacement issue. |
| packages/emcn/src/components/chip-modal/chip-modal.tsx | Propagates the modal-wide interlock to ChipModal close and Cancel controls and enables it while confirmations are pending. |
| packages/emcn/src/components/chip-modal/chip-modal.test.tsx | Covers Escape and button dismissal behavior, handler composition, and pending confirmation state. |
| apps/sim/app/workspace/[workspaceId]/knowledge/[id]/components/add-connector-modal/add-connector-modal.tsx | Replaces path-specific pending guards with the shared modal-wide dismissal interlock. |
| apps/sim/app/workspace/[workspaceId]/knowledge/[id]/components/edit-connector-modal/edit-connector-modal.tsx | Uses the shared interlock to prevent every dismissal path during saves. |
| .github/workflows/test-build.yml | Expands the existing CI type-check step from the realtime workspace to all workspaces. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Action becomes pending] --> B[dismissDisabled true]
B --> C[Escape prevented]
B --> D[Outside interaction prevented]
B --> E[Header close disabled]
B --> F[Footer Cancel disabled]
G[Consumer dismissal handler] --> H[Runs after internal guard]
H --> I[Guard remains authoritative]
J[Action completes] --> K[dismissDisabled false]
K --> L[Dismissal paths restored]
Reviews (3): Last reviewed commit: "revert(ci): drop the type-check inputs a..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
… 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.
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 03f6449. Configure here.
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.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 993e8fa. Configure here.
1 participant
Summary
ChipModal/ModalContenttakedismissDisabled, which holds all four exits shut while an action is in flight: Escape, outside-click, the header close button, and the footer CancelChipConfirmModalnow passesdismissDisabled={confirm.pending}, making its TSDoc true — it promised "a single dismiss path shared by the header X / dismiss button / Escape … and disabling dismiss while the confirm is in flight", but only the dismiss button was ever guardededit-connector-modalandadd-connector-modalguardedonOpenChangeagainst a pending save, then handed the header X a directonOpenChange(false)that skipped the guard — you could close them mid-save via the Xturbo run type-checkfor every workspace (was--filter=@sim/realtimeonly)Why the guard lives in
ModalContent{...props}is spread after that component's own handlers, so a consumer-passedonEscapeKeyDown/onInteractOutsidewould silently drop the floating-layer guard whose comment explains it prevents a frozen-page bug. The flag is published through a context thatChipModalHeader,ChipModalFooterandModalHeaderread, soModalHeader's built-inDialogPrimitive.Closeis covered too — otherwise the five non-ChipModal consumers would get a modal that traps Escape and outside-click but still closes on X.closeDisabled/cancelDisabledcompose with||, matchingButtonGroupItem's precedent: an explicittruestill disables one button, an explicitfalsecannot re-enable a button whose click Radix has already been told to ignore. All 60 existingChipModalHeadersites and 28cancelDisabledconsumers are unaffected.Type-check gate
All 23 workspaces pass today, so it lands green.
packages/emcn,packages/utils,apps/desktopandapps/docshad no type check in CI;apps/sim's source was covered only as a side effect ofnext build. Addedinputsto the turbo task so non-TS changes stop busting the cache.This does not cover
apps/sim's tests — its tsconfig excludes*.test.ts(x), and including them surfaces ~2.2k errors. That's its own cleanup, not a gate to switch on here.Testing
packages/emcn/src/components/chip-modal/chip-modal.test.tsxcovers the buttons, Escape, both composition directions, andChipConfirmModal's pending state. Each assertion verified to fail with the guard reverted.Checked all 62
ChipConfirmModalusages for apendingflag that could stick — the change removes Escape as an escape hatch, so a flag that never resets would trap the user. Every manualuseStateflag resets in afinally; the rest use React Query's.isPending. None can strand.Not covered: outside-click is guarded by the same flag but is not asserted — jsdom cannot drive Radix's outside-interaction path, so the assertion could never fail. It's code-reviewed only. Nothing here has been opened in a browser.
Follow-ups (not in this PR)
cancelDisabled={pending}alone, leaving Escape and outside-click open. Each needs its pending source verified, so it's a separate reviewed change rather than a blind sweepModalContent.showCloseis destructured and never read —ChipModalpassesshowClose={false}believing it does something. Pre-existing no-opdeploy-modal'schatSubmittingis the most fragile consumer: three early returns inside itstry, correct only becausefinallyresets itType of Change
Checklist