From 44b4d77d803aa724968307cfa463f7c440791a10 Mon Sep 17 00:00:00 2001 From: Eli Bosley Date: Mon, 15 Sep 2025 16:04:03 -0400 Subject: [PATCH] fix: use virtual-modal-container (#1709) ## Summary by CodeRabbit - Bug Fixes - Ensured modals consistently render by using a dedicated container, reducing cases where dialogs failed to open or appeared in the wrong place. - Improved reliability of modal mounting during page load and navigation. - Refactor - Simplified the modal mounting mechanism to improve stability and reduce reliance on DOM structure assumptions. --- unraid-ui/src/composables/useTeleport.test.ts | 198 +++++------------- unraid-ui/src/composables/useTeleport.ts | 43 ++-- 2 files changed, 71 insertions(+), 170 deletions(-) diff --git a/unraid-ui/src/composables/useTeleport.test.ts b/unraid-ui/src/composables/useTeleport.test.ts index c7754c124..67308d3c4 100644 --- a/unraid-ui/src/composables/useTeleport.test.ts +++ b/unraid-ui/src/composables/useTeleport.test.ts @@ -1,169 +1,77 @@ import useTeleport from '@/composables/useTeleport'; +import { mount } from '@vue/test-utils'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; - -// Mock Vue's computed while preserving other exports -vi.mock('vue', async () => ({ - ...(await vi.importActual('vue')), - computed: vi.fn((fn) => { - const result = { value: fn() }; - return result; - }), -})); +import { defineComponent } from 'vue'; describe('useTeleport', () => { beforeEach(() => { // Clear the DOM before each test document.body.innerHTML = ''; - document.head.innerHTML = ''; vi.clearAllMocks(); }); afterEach(() => { - vi.restoreAllMocks(); + // Clean up virtual container if it exists + const virtualContainer = document.getElementById('unraid-api-modals-virtual'); + if (virtualContainer) { + virtualContainer.remove(); + } + // Reset the module to clear the virtualModalContainer variable + vi.resetModules(); }); - it('should return teleportTarget computed property', () => { + it('should return teleportTarget ref with correct value', () => { const { teleportTarget } = useTeleport(); - expect(teleportTarget).toBeDefined(); - expect(teleportTarget).toHaveProperty('value'); + expect(teleportTarget.value).toBe('#unraid-api-modals-virtual'); }); - it('should return #modals when element with id="modals" exists', () => { - // Create element with id="modals" - const modalsDiv = document.createElement('div'); - modalsDiv.id = 'modals'; - document.body.appendChild(modalsDiv); + it('should create virtual container element on mount with correct properties', () => { + const TestComponent = defineComponent({ + setup() { + const { teleportTarget } = useTeleport(); + return { teleportTarget }; + }, + template: '
{{ teleportTarget }}
', + }); - const { teleportTarget } = useTeleport(); - expect(teleportTarget.value).toBe('#modals'); + // Initially, virtual container should not exist + expect(document.getElementById('unraid-api-modals-virtual')).toBeNull(); + + // Mount the component + mount(TestComponent); + + // After mount, virtual container should be created with correct properties + const virtualContainer = document.getElementById('unraid-api-modals-virtual'); + expect(virtualContainer).toBeTruthy(); + expect(virtualContainer?.className).toBe('unapi'); + expect(virtualContainer?.style.position).toBe('relative'); + expect(virtualContainer?.style.zIndex).toBe('999999'); + expect(virtualContainer?.parentElement).toBe(document.body); }); - it('should prioritize #modals id over mounted unraid-modals', () => { - // Create both elements - const modalsDiv = document.createElement('div'); - modalsDiv.id = 'modals'; - document.body.appendChild(modalsDiv); + it('should reuse existing virtual container within same test', () => { + // Manually create the container first + const manualContainer = document.createElement('div'); + manualContainer.id = 'unraid-api-modals-virtual'; + manualContainer.className = 'unapi'; + manualContainer.style.position = 'relative'; + manualContainer.style.zIndex = '999999'; + document.body.appendChild(manualContainer); - const unraidModals = document.createElement('unraid-modals'); - unraidModals.setAttribute('data-vue-mounted', 'true'); - document.body.appendChild(unraidModals); + const TestComponent = defineComponent({ + setup() { + const { teleportTarget } = useTeleport(); + return { teleportTarget }; + }, + template: '
{{ teleportTarget }}
', + }); - const { teleportTarget } = useTeleport(); - expect(teleportTarget.value).toBe('#modals'); - }); + // Mount component - should not create a new container + mount(TestComponent); - it('should return mounted unraid-modals with inner #modals div', () => { - // Create mounted unraid-modals with inner modals div - const unraidModals = document.createElement('unraid-modals'); - unraidModals.setAttribute('data-vue-mounted', 'true'); - - const innerModals = document.createElement('div'); - innerModals.id = 'modals'; - unraidModals.appendChild(innerModals); - - document.body.appendChild(unraidModals); - - const { teleportTarget } = useTeleport(); - expect(teleportTarget.value).toBe('#modals'); - }); - - it('should add id to mounted unraid-modals when no inner modals div exists', () => { - // Create mounted unraid-modals without inner div - const unraidModals = document.createElement('unraid-modals'); - unraidModals.setAttribute('data-vue-mounted', 'true'); - document.body.appendChild(unraidModals); - - const { teleportTarget } = useTeleport(); - expect(unraidModals.id).toBe('unraid-modals-teleport-target'); - expect(teleportTarget.value).toBe('#unraid-modals-teleport-target'); - }); - - it('should use existing id of mounted unraid-modals if present', () => { - // Create mounted unraid-modals with existing id - const unraidModals = document.createElement('unraid-modals'); - unraidModals.setAttribute('data-vue-mounted', 'true'); - unraidModals.id = 'custom-modals-id'; - document.body.appendChild(unraidModals); - - const { teleportTarget } = useTeleport(); - expect(teleportTarget.value).toBe('#custom-modals-id'); - }); - - it('should ignore unmounted unraid-modals elements', () => { - // Create unmounted unraid-modals (without data-vue-mounted attribute) - const unraidModals = document.createElement('unraid-modals'); - document.body.appendChild(unraidModals); - - const { teleportTarget } = useTeleport(); - expect(teleportTarget.value).toBe('body'); - }); - - it('should ignore unraid-modals with data-vue-mounted="false"', () => { - // Create unraid-modals with data-vue-mounted="false" - const unraidModals = document.createElement('unraid-modals'); - unraidModals.setAttribute('data-vue-mounted', 'false'); - document.body.appendChild(unraidModals); - - const { teleportTarget } = useTeleport(); - expect(teleportTarget.value).toBe('body'); - }); - - it('should return body as fallback when no suitable target exists', () => { - // No elements in DOM - const { teleportTarget } = useTeleport(); - expect(teleportTarget.value).toBe('body'); - }); - - it('should handle multiple unraid-modals elements correctly', () => { - // Create multiple unraid-modals, only one mounted - const unmountedModals1 = document.createElement('unraid-modals'); - document.body.appendChild(unmountedModals1); - - const mountedModals = document.createElement('unraid-modals'); - mountedModals.setAttribute('data-vue-mounted', 'true'); - mountedModals.id = 'mounted-modals'; - document.body.appendChild(mountedModals); - - const unmountedModals2 = document.createElement('unraid-modals'); - document.body.appendChild(unmountedModals2); - - const { teleportTarget } = useTeleport(); - expect(teleportTarget.value).toBe('#mounted-modals'); - }); - - it('should handle nested modal elements correctly', () => { - // Create nested structure - const container = document.createElement('div'); - - const unraidModals = document.createElement('unraid-modals'); - unraidModals.setAttribute('data-vue-mounted', 'true'); - - const innerDiv = document.createElement('div'); - const innerModals = document.createElement('div'); - innerModals.id = 'modals'; - - innerDiv.appendChild(innerModals); - unraidModals.appendChild(innerDiv); - container.appendChild(unraidModals); - document.body.appendChild(container); - - const { teleportTarget } = useTeleport(); - expect(teleportTarget.value).toBe('#modals'); - }); - - it('should be reactive to DOM changes', () => { - const { teleportTarget } = useTeleport(); - - // Initially should be body - expect(teleportTarget.value).toBe('body'); - - // Add modals element - const modalsDiv = document.createElement('div'); - modalsDiv.id = 'modals'; - document.body.appendChild(modalsDiv); - - // Recreate the composable to test updated DOM state - const { teleportTarget: newTeleportTarget } = useTeleport(); - expect(newTeleportTarget.value).toBe('#modals'); + // Should still have only one container + const containers = document.querySelectorAll('#unraid-api-modals-virtual'); + expect(containers.length).toBe(1); + expect(containers[0]).toBe(manualContainer); }); }); diff --git a/unraid-ui/src/composables/useTeleport.ts b/unraid-ui/src/composables/useTeleport.ts index 75739c8c4..d0ec36663 100644 --- a/unraid-ui/src/composables/useTeleport.ts +++ b/unraid-ui/src/composables/useTeleport.ts @@ -1,31 +1,24 @@ -import { computed } from 'vue'; +import { onMounted, ref } from 'vue'; + +let virtualModalContainer: HTMLDivElement | null = null; + +const ensureVirtualContainer = () => { + if (!virtualModalContainer) { + virtualModalContainer = document.createElement('div'); + virtualModalContainer.id = 'unraid-api-modals-virtual'; + virtualModalContainer.className = 'unapi'; + virtualModalContainer.style.position = 'relative'; + virtualModalContainer.style.zIndex = '999999'; + document.body.appendChild(virtualModalContainer); + } + return virtualModalContainer; +}; const useTeleport = () => { - // Computed property that finds the correct teleport target - const teleportTarget = computed(() => { - // #modals should be unique (id), but let's be defensive - const modalsElement = document.getElementById('modals'); - if (modalsElement) return `#modals`; + const teleportTarget = ref('#unraid-api-modals-virtual'); - // Find only mounted unraid-modals components (data-vue-mounted="true") - // This ensures we don't target unmounted or duplicate elements - const mountedModals = document.querySelector('unraid-modals[data-vue-mounted="true"]'); - if (mountedModals) { - // Check if it has the inner #modals div - const innerModals = mountedModals.querySelector('#modals'); - if (innerModals && innerModals.id) { - return `#${innerModals.id}`; - } - // Use the mounted component itself as fallback - // Add a unique identifier if it doesn't have one - if (!mountedModals.id) { - mountedModals.id = 'unraid-modals-teleport-target'; - } - return `#${mountedModals.id}`; - } - - // Final fallback to body - modals component not mounted yet - return 'body'; + onMounted(() => { + ensureVirtualContainer(); }); return {