Skip to content

refactor(spx-gui): rebuild modal infrastructure & remove the naive-ui dependency#3037

Merged
nighca merged 11 commits into
goplus:devfrom
cn0809:modal
Apr 30, 2026
Merged

refactor(spx-gui): rebuild modal infrastructure & remove the naive-ui dependency#3037
nighca merged 11 commits into
goplus:devfrom
cn0809:modal

Conversation

@cn0809
Copy link
Copy Markdown
Collaborator

@cn0809 cn0809 commented Apr 15, 2026

close: #3016
close: #3014

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the UI library by replacing Naive UI dependencies for UIDropdown, UITooltip, and UIModal with custom implementations using @floating-ui/dom and internal stack management. Feedback suggests toggling visibility on dropdown clicks, resolving z-index conflicts between dropdowns and modals, and unifying ESC key handling across stack managers. Additionally, suggestions were made to add exit transitions to dropdowns and remove redundant nextTick() calls in the modal focus logic.

Comment thread spx-gui/src/components/ui/UIDropdown.vue
Comment thread spx-gui/src/components/ui/UIDropdown.vue Outdated
Comment thread spx-gui/src/components/ui/UIDropdown.vue
Comment thread spx-gui/src/components/ui/UIDropdown.vue Outdated
Comment thread spx-gui/src/components/ui/modal/UIModal.vue Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors spx-gui’s popup/modal infrastructure to remove remaining Naive UI modal coupling by introducing shared internal layer-stack primitives and rebuilding UIModal/modal stack behavior around them (issue #3016).

Changes:

  • Added a shared createLayerStack() + findLayerRoot() utility and refactored popup stack to use it.
  • Introduced a modal stack + useModalSurface() composable, and rebuilt UIModal / UIFullScreenModal around Teleport + internal transitions (no NModal).
  • Updated provider wiring/tests and migrated editor dropdown-modals to UIDropdownForm.

Reviewed changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
spx-gui/src/components/ui/utils/layer-stack.ts New shared layer stack primitive (registration/topmost/root attrs + DOM root lookup).
spx-gui/src/components/ui/utils/layer-stack.test.ts Unit tests for the shared layer stack + DOM root lookup.
spx-gui/src/components/ui/utils/index.ts isInPopup now detects modal roots via modal stack markers (removed Naive UI selector logic).
spx-gui/src/components/ui/popup/stack.ts Refactored popup stack to reuse shared layer stack.
spx-gui/src/components/ui/popup/stack.test.ts Removed popup stack test file (coverage now primarily via shared layer-stack tests + component tests).
spx-gui/src/components/ui/modal/stack.ts New modal stack built on shared layer stack.
spx-gui/src/components/ui/modal/use-modal-surface.ts New composable to wire modal surfaces to stack, transform-origin, and modal-local popup container.
spx-gui/src/components/ui/modal/use-modal-surface.test.ts Tests for modal surface wiring (root attrs, popup container, transform-origin behavior).
spx-gui/src/components/ui/modal/UIModal.vue Rebuilt modal as internal Teleport + Transition implementation (no Naive UI NModal).
spx-gui/src/components/ui/modal/UIModal.test.ts New tests for UIModal rendering, attrs, interaction, animation origin, and focus behavior.
spx-gui/src/components/ui/modal/UIModalProvider.vue Updated ESC handling logic to respect modal scope and ignore nested popup ESC events.
spx-gui/src/components/ui/modal/UIModalProvider.test.ts New provider tests for programmatic modals, active/topmost behavior, and ESC filtering.
spx-gui/src/components/ui/modal/UIFullScreenModal.vue Refactored fullscreen modal to use shared useModalSurface() + stack attrs.
spx-gui/src/components/ui/modal/UIFullScreenModalHeader.vue Minor simplification: inline cn(...) usage (remove computed).
spx-gui/src/components/ui/modal/UIModalClose.vue Minor simplification: inline icon size class (remove computed).
spx-gui/src/components/ui/modal/index.ts Updated modal exports to reflect new structure (removes dropdown-modal export, keeps UIModal).
spx-gui/src/components/ui/UIConfigProvider.vue Provides the new modal stack at the UI root (alongside popup stack).
spx-gui/src/components/ui/index.ts Exports UIDropdownForm.
spx-gui/src/components/ui/UIDropdownForm.vue Updated radar text + adjusted imports to match new structure.
spx-gui/src/components/ui/README.md Documentation update describing modal wrapper root-class/attr behavior.
spx-gui/src/components/editor/sprite/animation/state/BoundStateEditor.vue Migrated from UIDropdownModal to UIDropdownForm.
spx-gui/src/components/editor/sprite/animation/SoundEditor.vue Migrated from UIDropdownModal to UIDropdownForm.
spx-gui/src/components/editor/sprite/animation/DurationEditor.vue Migrated from UIDropdownModal to UIDropdownForm.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread spx-gui/src/components/ui/utils/index.ts Outdated
@fennoai
Copy link
Copy Markdown
Contributor

fennoai Bot commented Apr 22, 2026

This is a well-structured refactor. The createLayerStack abstraction cleanly unifies popup and modal stacks, the ESC scoping logic is carefully defended, and the new test coverage is comprehensive. A few issues worth addressing before merge.


ModalTransformOrigin no longer exported from the public barrel

spx-gui/src/components/ui/modal/index.ts line 7

ModalTransformOrigin was previously exported as type TransformOrigin as ModalTransformOrigin from this barrel. It is now defined and exported only from the internal use-modal-surface.ts but is not re-exported here. Callers that need to type the origin argument passed to setTransformOrigin (e.g. AssetLibraryModal.vue, SpriteGenModal.vue) must reach into the implementation file directly.

Add to modal/index.ts:

export type { ModalTransformOrigin } from './use-modal-surface'

...entry spread in register() is fragile and makes shallowReactive wrapping dead overhead

spx-gui/src/components/ui/utils/layer-stack.ts line 69

Spreading a shallowReactive entry with ...entry copies the current snapshot of its properties. For open (a Ref) the copy is safe because Ref identity is stable — but this creates a silent dependency on that guarantee. If entry were ever extended with a non-reference mutable primitive, reactivity would silently break. The shallowReactive on entry (line 60) is also dead overhead since the spread disconnects the returned registration from the entry object.

Prefer explicit property assignment:

return shallowReactive<LayerRegistration>({
  id: entry.id,
  open: entry.open,
  rootAttrs: Object.freeze({ [rootAttrName]: '' }),
  isTopmost: computed(() => getTopmostOpenEntry()?.id === entry.id),
  unregister
})

O(N²) re-evaluation of isTopmost computeds on every open/close

spx-gui/src/components/ui/utils/layer-stack.ts line 72

Every isTopmost computed calls getTopmostOpenEntry() which reads entry.open.value for each entry it scans. Each per-entry computed is therefore subscribed to the open ref of all preceding entries. When any layer opens or closes, all isTopmost computeds are invalidated and each re-runs its own O(N) scan — O(N²) total work per event. The modal stack is typically shallow but the popup stack (which shares this abstraction) can accumulate more entries.

A single stack-level computed would make all per-entry comparisons O(1):

const topmostId = computed(() => getTopmostOpenEntry()?.id)
// per-entry:
isTopmost: computed(() => topmostId.value === entry.id)

Async auto-focus watcher is missing a cancellation guard

spx-gui/src/components/ui/modal/UIModal.vue lines 113–125

The auto-focus watcher awaits two nextTick() calls before calling container.focus(), but unlike the analogous watcher in use-modal-surface.ts it has no onCleanup-based cancellation. If visible toggles or the component unmounts between the two ticks, the watcher will still proceed. The isConnected check only partially mitigates this — it runs after both ticks.

Align with the pattern in use-modal-surface.ts:

async ([visible, container, autoFocus], _, onCleanup) => {
  let cancelled = false
  onCleanup(() => { cancelled = true })
  if (!visible || !autoFocus || container == null) return
  await nextTick()
  await nextTick()
  if (cancelled || !container.isConnected) return
  container.focus()
}

isEscTargetWithinModalScope treats non-HTMLElement targets as in-scope

spx-gui/src/components/ui/modal/UIModalProvider.vue line 147

if (!(target instanceof HTMLElement)) return true

SVG elements with tabindex do receive keydown events (SVGElement extends Element, not HTMLElement). A focused SVG element outside the modal container would be incorrectly treated as in-scope, triggering an unintended modal close. Either use !(target instanceof Element) to cover SVG uniformly, or add an inline comment explaining why only HTMLElement is expected here.


Module-level mid counter is shared across all UIModalProvider instances

spx-gui/src/components/ui/modal/UIModalProvider.vue line 77

mid is a module-level variable shared across all provider instances in the same JS module scope (multiple app roots, parallel tests). This is inconsistent with the per-instance nextId in createLayerStack — notably layer-stack.test.ts explicitly asserts that IDs start from 1 for each new stack instance. Modal IDs leak between providers and test cases. Moving the counter inside setup scope or into ModalContext would make each provider instance fully isolated.


Hardcoded 300 ms cleanup timeout not tied to actual CSS transition durations

spx-gui/src/components/ui/modal/UIModalProvider.vue line 175

The 300 ms does not match UIModal.vue transitions (250 ms) or UIFullScreenModal.vue transitions (400 ms). If either changes, entries linger unnecessarily. The existing TODO acknowledges this. At minimum, extracting to a named constant and cross-referencing with the CSS values would make the coupling explicit.


Redundant ?? clientX.x fallbacks in resolveClickOrigin

spx-gui/src/components/ui/modal/use-modal-surface.ts lines 119–120

MouseEvent.clientX / clientY are always number per the DOM spec and never undefined or null. MouseEvent.x / y are aliases. The ?? fallbacks will never activate and falsely imply clientX can be absent. Plain clickEvent.clientX / clickEvent.clientY is sufficient.

Comment thread spx-gui/src/components/ui/utils/layer-stack.ts Outdated
Comment thread spx-gui/src/components/ui/modal/use-modal-surface.ts Outdated
Comment thread spx-gui/src/components/ui/modal/use-modal-surface.ts Outdated
Comment thread spx-gui/src/components/ui/modal/use-modal-surface.ts Outdated
Comment thread spx-gui/src/components/ui/modal/use-modal-surface.ts Outdated
Comment thread spx-gui/src/components/ui/modal/UIModal.vue
Comment thread spx-gui/src/components/ui/modal/UIModal.vue
Comment thread spx-gui/src/components/ui/modal/UIModal.vue
Comment thread spx-gui/src/components/ui/modal/UIModal.vue Outdated
Comment thread spx-gui/src/components/ui/modal/UIModal.vue Outdated
Comment thread spx-gui/src/components/ui/modal/UIModal.vue
Comment thread spx-gui/src/components/ui/modal/UIModal.vue Outdated
Comment thread spx-gui/src/components/ui/modal/UIModal.vue Outdated
Comment thread spx-gui/src/components/ui/modal/UIModal.vue Outdated
Comment thread spx-gui/src/components/ui/modal/UIModal.vue Outdated
Comment thread spx-gui/src/components/ui/modal/UIModal.vue Outdated
Comment thread spx-gui/src/components/ui/modal/UIModal.vue Outdated
// the resolved origin changes. The measured CSS value must be relative to the modal
// surface itself rather than viewport coordinates.
watch(
[visible, containerRef, resolvedTransformOrigin],
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

总感觉 transform origin 的逻辑还有简化空间,再看看?

Comment thread spx-gui/src/components/ui/utils/layer-stack.ts Outdated
Comment thread spx-gui/src/components/ui/UIDropdownForm.vue Outdated
Comment thread spx-gui/src/components/ui/modal/UIModal.vue Outdated
@cn0809 cn0809 changed the title refactor(spx-gui): rebuild modal infrastructure refactor(spx-gui): rebuild modal infrastructure & remove the naive-ui dependency Apr 29, 2026
Copy link
Copy Markdown
Collaborator

@nighca nighca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

总感觉 transform origin 的逻辑还有简化空间,再看看?

感觉还是有点复杂,不过不影响外部使用,先 merge

@nighca nighca merged commit e96324a into goplus:dev Apr 30, 2026
4 checks passed
const modalContainer = ref<HTMLElement>()
const currentModals = shallowReactive<ModalInfo[]>([])
const emitter: ModalEvents = new Emitter()
let nextModalId = 0
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

发现这个其实不是 next modal ID,而是 next modal ID 减 1..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replace Naive UI modal infrastructure Replace Naive UI global provider and theme plumbing

3 participants