Comfy-Org / Comfy-Org/ComfyUI_frontend

Dialog architecture: retire the non-modal container workaround stack (root fix + regression harness, successor to FE-578)

Open
#13,558 6 comments 2 reactions 1 assignee Claimed by @dante01yoon View on GitHub
area:ui area:vue-migration refactor
Dominant language
TypeScript
Stars
2k
Forks
699
Avg merge
1d 7h
Merged PRs (30d)
490

Description

## Summary

The dialog subsystem's PrimeVue → Reka-UI migration (Linear epic **FE-571**) is mechanically ~90% complete — the renderer cutover landed at Phase 6a — but the **architectural** decision it deferred, the non-modal container pattern, is generating a recurring class of "Settings dialog dismisses / loses its backdrop / stacks wrong" bugs. This issue is a handoff: current state, why the churn happens, backport guidance, and a proposed path forward. Permalinks below are pinned to `main@2ef341dcd8`.

## Root cause: `modal: false` on a container dialog

Phase 3 ([#12182](https://github.com/Comfy-Org/ComfyUI_frontend/pull/12182)) moved Settings to the Reka renderer and set `modal: false`, because Reka's `modal: true` enables a focus-trap and sets `pointer-events: none` on everything outside the `DialogContent`, which breaks the nested confirm/edit dialogs Settings hosts (both PrimeVue and Reka, all teleported to `body`).

- [`useSettingsDialog.ts` L35-L46 — Settings opts into Reka, goes non-modal, and (post-#13510) opts out of focus-outside dismissal](https://github.com/Comfy-Org/ComfyUI_frontend/blob/2ef341dcd8f7bf60df796d743f480c6be92cecbc/src/platform/settings/composables/useSettingsDialog.ts#L35-L46)

```ts
dialogComponentProps: {
renderer: 'reka',
// Reka's modal focus-trap + body pointer-events:none breaks nested
// PrimeVue dialogs (Edit Keybinding, Overwrite confirm, etc.).
modal: false,
// A nested dialog closing can move focus onto an app element once the
// focused row is removed; a non-modal dialog reads that as an outside
// focus and dismisses itself, so opt out. (added by #13510)
dismissOnFocusOutside: false,
...
}
```

Going non-modal disables Reka `DismissableLayer`'s native "is this interaction inside my layer stack?" awareness. Every outside-interaction now has to be hand-classified in a bridge module, and the modal-only `DialogOverlay` no longer renders (so the scrim had to be restored manually — [#13502](https://github.com/Comfy-Org/ComfyUI_frontend/pull/13502)).

## The workaround stack (what exists on `main` today)

**1. A hand-maintained overlay allowlist.** `isInsideOverlay()` calls `preventDefault()` on dismissal when the interaction target is inside any body-portaled overlay we know about:

- [`rekaPrimeVueBridge.ts` L6-L17 — the two allowlists (PrimeVue overlays + Reka portals)](https://github.com/Comfy-Org/ComfyUI_frontend/blob/2ef341dcd8f7bf60df796d743f480c6be92cecbc/src/components/dialog/rekaPrimeVueBridge.ts#L6-L17)
- [`rekaPrimeVueBridge.ts` L28-L50 — `onRekaPointerDownOutside` (also gates on `isActive` for stacked dialogs)](https://github.com/Comfy-Org/ComfyUI_frontend/blob/2ef341dcd8f7bf60df796d743f480c6be92cecbc/src/components/dialog/rekaPrimeVueBridge.ts#L28-L50)

```ts
const PRIMEVUE_OVERLAY_SELECTORS =
'.p-select-overlay, .p-colorpicker-panel, .p-popover, .p-autocomplete-overlay, .p-overlay, .p-overlay-mask, .p-dialog'
const REKA_PORTAL_SELECTORS =
'[data-reka-popper-content-wrapper], [data-reka-dialog-content], [data-reka-menu-content], [data-reka-context-menu-content], [role="dialog"], [role="menu"], [role="listbox"], [role="tooltip"]'
```

**2. A per-dialog escape hatch for the case an allowlist structurally can't cover.** [#13510](https://github.com/Comfy-Org/ComfyUI_frontend/pull/13510) added `dismissOnFocusOutside` because focus can land on an *ordinary app element* (after the focused list row is deleted), which no portal selector can enumerate:

- [`rekaPrimeVueBridge.ts` L64-L75 — `onRekaFocusOutside` with the opt-out](https://github.com/Comfy-Org/ComfyUI_frontend/blob/2ef341dcd8f7bf60df796d743f480c6be92cecbc/src/components/dialog/rekaPrimeVueBridge.ts#L64-L75)

```ts
export function onRekaFocusOutside(
event: OutsideEvent,
options: { dismissOnFocusOutside?: boolean } = {}
) {
if (options.dismissOnFocusOutside === false) {
event.preventDefault()
return
}
if (isInsideOverlay(event.detail.originalEvent.target)) {
event.preventDefault()
}
}
```

- [`GlobalDialog.vue` L35-L37 — the template wiring that forwards `dialogComponentProps` into the handler](https://github.com/Comfy-Org/ComfyUI_frontend/blob/2ef341dcd8f7bf60df796d743f480c6be92cecbc/src/components/dialog/GlobalDialog.vue#L35-L37)
- [`dialogStore.ts` L41-L48 — the `dismissOnFocusOutside?: boolean` prop](https://github.com/Comfy-Org/ComfyUI_frontend/blob/2ef341dcd8f7bf60df796d743f480c6be92cecbc/src/stores/dialogStore.ts#L41-L48)

**3. A dual-renderer branch still shipping.** The PrimeVue `` path is unreachable at runtime (default flipped to `reka` in Phase 6a) but still compiled in:

- [`dialogStore.ts` L24-L29 — `DialogRenderer` escape-hatch comment pointing at FE-578](https://github.com/Comfy-Org/ComfyUI_frontend/blob/2ef341dcd8f7bf60df796d743f480c6be92cecbc/src/stores/dialogStore.ts#L24-L29)
- [`GlobalDialog.vue` L93-L128 — the legacy `` branch + `primevue/dialog` import (deleted by #12611)](https://github.com/Comfy-Org/ComfyUI_frontend/blob/2ef341dcd8f7bf60df796d743f480c6be92cecbc/src/components/dialog/GlobalDialog.vue#L93-L128)

```ts
/**
* `'primevue'` is the legacy PrimeVue `Dialog` escape hatch, kept only until
* the branch is deleted in the Phase 6 cleanup (FE-578).
*/
type DialogRenderer = 'primevue' | 'reka'
```

## The recurring bug class

Every one of these is the same non-modal-container problem surfacing on a new overlay type:

| PR | Symptom patched |
|---|---|
| [#12038](https://github.com/Comfy-Org/ComfyUI_frontend/pull/12038) | Reka overlays sinking behind PrimeVue dialogs (backported core/cloud 1.43) |
| [#12665](https://github.com/Comfy-Org/ComfyUI_frontend/pull/12665) | Add-Secret dialog rendering behind Settings (FE-939) |
| [#13092](https://github.com/Comfy-Org/ComfyUI_frontend/pull/13092) / [#13317](https://github.com/Comfy-Org/ComfyUI_frontend/pull/13317) | pricing dialog width + stacking regressions |
| [#13502](https://github.com/Comfy-Org/ComfyUI_frontend/pull/13502) | backdrop scrim gone (reka only renders `DialogOverlay` for modal roots) |
| [#13510](https://github.com/Comfy-Org/ComfyUI_frontend/pull/13510) | self-dismiss when focus falls to an app element |
| [#13464](https://github.com/Comfy-Org/ComfyUI_frontend/pull/13464) *(open)* | allowlist `+= .p-toast` (focus recovery to a toast close button) |
| [#13487](https://github.com/Comfy-Org/ComfyUI_frontend/pull/13487) *(open)* | allowlist `+= [aria-haspopup="menu\|dialog\|listbox"]` (menu trigger click) |

`isInsideOverlay`'s allowlist has grown 4×. Two open PRs (#13464, #13487) extend the same ~15-line block from different branches and **will conflict with each other and with `main` post-#13510**; a third ([#12611](https://github.com/Comfy-Org/ComfyUI_frontend/pull/12611), Phase 6b) rewrites the surrounding file.

## Coverage gap

The only end-to-end guard for this behavior is `@cloud`-tagged, which `playwright.config.ts` `grepInvert`s out of the default project — so a regression in the `onRekaFocusOutside` wiring stays green under `pnpm test:unit` and the default browser run. There is no mounted-component test asserting the `dismissOnFocusOutside` plumbing.

## Release / backport guidance

`#13510` (and the whole self-dismiss cluster) **only applies where Settings renders via the non-modal Reka path.** Migration cut points:

| Branch | Cut point | Settings renderer | Exposed? | Backport #13510? |
|---|---|---|---|---|
| `cloud/1.44`, `cloud/1.45` | ≤ Phase 2 (no bridge) | PrimeVue | no | **No — N/A** (files/anchors don't exist; semantic no-op) |
| `cloud/1.46` | Phase 5 | Reka, `modal:false` | yes | Valid target |
| `cloud/1.47` | Phase 6a cutover | Reka, `modal:false` | yes | **Cleanest target** |
| `main` | 6a + fixes | Reka, `modal:false` | patched | n/a |

⚠️ There is an experiment branch `fix/cloud-1.45-dialog-reka-cutover` that flips 1.45's default renderer to Reka. If it ships, 1.45 inherits this entire bug class and would need #13510 **+ #13502 + the allowlist entries** to come with it — flag before merging.

## Proposed path forward

1. **Serialize the three concurrent bridge edits under one owner** (dante01yoon owns FE-571/#12611). Land order: rebase the allowlist adds (#13464, #13487) onto post-#13510 first, then #12611 (Phase 6b deletion) last. Current trajectory guarantees conflicts and risks silently dropping a fix.
2. **Add a default-project (non-`@cloud`) regression harness**: mount a non-modal container dialog and assert it does **not** dismiss when the outside interaction/focus lands on each overlay class — portal, `.p-toast`, `[aria-haspopup]` trigger, and a plain app element after DOM removal. This bug has recurred 6+ times with no cheap guard.
3. **Attack the root, not the next symptom.** Once the nested dialogs are all-Reka (`NodeSearchBox` is the notable PrimeVue holdout), Settings can restore `modal: true` **or** adopt Reka's nested `DismissableLayer` stack, where child layers are natively recognized as "inside" — which retires the allowlist *and* the per-dialog opt-outs. Note this is **not** delivered by FE-578/#12611 (that keeps the `.p-dialog` guards and the bridge file); track it as the explicit successor to FE-578.
4. **Write the missing ADR.** There is no accepted ADR for the dialog architecture (ADR-0004 "Fork PrimeVue" is *Rejected*; the plan lives only in FE-578 code breadcrumbs). Capture the non-modal-container decision and the target end-state so the team stops drifting into per-surface patches.

## References

- Linear: **FE-571** (epic), **FE-578** (Phase 6 cleanup, open), FE-940 (self-registering z-index, open), FE-941 (VideoHelp z-order, open)
- Notion: **PRD: Design System Cleanup** (To Do) — strangler off PrimeVue → shadcn-vue + Reka, one dialog pattern, core PrimeVue removal targeted 2026-08-31, gated by an ESLint "no new PrimeVue" freeze
- GitHub: #12611 (Phase 6b), #13464 + #13487 (concurrent allowlist adds), #12758 (FE-624 standardize confirm/prompt), #11059 (RFC: decompose `dialogService`)

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.