From b68ee600c1e2206a3c87f3ac01e28af2a7331c04 Mon Sep 17 00:00:00 2001 From: yyh <92089059+lyzno1@users.noreply.github.com> Date: Wed, 4 Mar 2026 11:13:43 +0800 Subject: [PATCH] refactor: migrate workflow onboarding modal to base dialog (#32915) --- .../base/ui/dialog/__tests__/index.spec.tsx | 48 +- web/app/components/base/ui/dialog/index.tsx | 35 +- .../components/workflow-children.tsx | 1 - .../__tests__/index.spec.tsx | 461 +++++------------- .../__tests__/start-node-option.spec.tsx | 1 - .../workflow-onboarding-modal/index.tsx | 62 +-- .../start-node-option.tsx | 13 +- .../start-node-selection-panel.tsx | 7 +- web/docs/overlay-migration.md | 32 +- web/eslint-suppressions.json | 16 - 10 files changed, 239 insertions(+), 437 deletions(-) diff --git a/web/app/components/base/ui/dialog/__tests__/index.spec.tsx b/web/app/components/base/ui/dialog/__tests__/index.spec.tsx index 0861fff603..2e52bd547a 100644 --- a/web/app/components/base/ui/dialog/__tests__/index.spec.tsx +++ b/web/app/components/base/ui/dialog/__tests__/index.spec.tsx @@ -1,11 +1,13 @@ import { Dialog as BaseDialog } from '@base-ui/react/dialog' -import { render, screen } from '@testing-library/react' -import { describe, expect, it } from 'vitest' +import { fireEvent, render, screen } from '@testing-library/react' +import { describe, expect, it, vi } from 'vitest' import { Dialog, DialogClose, + DialogCloseButton, DialogContent, DialogDescription, + DialogPortal, DialogTitle, DialogTrigger, } from '../index' @@ -29,7 +31,7 @@ describe('Dialog wrapper', () => { }) describe('Props', () => { - it('should not render close button when closable is omitted', () => { + it('should not render close button when DialogCloseButton is not provided', () => { render( @@ -41,20 +43,47 @@ describe('Dialog wrapper', () => { expect(screen.queryByRole('button', { name: 'Close' })).not.toBeInTheDocument() }) - it('should render close button when closable is true', () => { + it('should render explicit close button with custom aria-label', () => { render( - + + Dialog body , ) - const dialog = screen.getByRole('dialog') - const closeButton = screen.getByRole('button', { name: 'Close' }) + expect(screen.getByRole('button', { name: 'Dismiss dialog' })).toBeInTheDocument() + }) - expect(dialog).toContainElement(closeButton) - expect(closeButton).toHaveAttribute('aria-label', 'Close') + it('should render default close button label when aria-label is omitted', () => { + render( + + + + Dialog body + + , + ) + + expect(screen.getByRole('button', { name: 'Close' })).toBeInTheDocument() + }) + + it('should forward close button props to base primitive', () => { + const onClick = vi.fn() + render( + + + + Dialog body + + , + ) + + const closeButton = screen.getByTestId('close-button') + expect(closeButton).toBeDisabled() + fireEvent.click(closeButton) + expect(onClick).not.toHaveBeenCalled() }) }) @@ -65,6 +94,7 @@ describe('Dialog wrapper', () => { expect(DialogTitle).toBe(BaseDialog.Title) expect(DialogDescription).toBe(BaseDialog.Description) expect(DialogClose).toBe(BaseDialog.Close) + expect(DialogPortal).toBe(BaseDialog.Portal) }) }) }) diff --git a/web/app/components/base/ui/dialog/index.tsx b/web/app/components/base/ui/dialog/index.tsx index 605fccee09..b86f94c46f 100644 --- a/web/app/components/base/ui/dialog/index.tsx +++ b/web/app/components/base/ui/dialog/index.tsx @@ -16,22 +16,42 @@ export const DialogTrigger = BaseDialog.Trigger export const DialogTitle = BaseDialog.Title export const DialogDescription = BaseDialog.Description export const DialogClose = BaseDialog.Close +export const DialogPortal = BaseDialog.Portal + +type DialogCloseButtonProps = Omit, 'children'> + +export function DialogCloseButton({ + className, + 'aria-label': ariaLabel = 'Close', + ...props +}: DialogCloseButtonProps) { + return ( + + + ) +} type DialogContentProps = { children: React.ReactNode className?: string overlayClassName?: string - closable?: boolean } export function DialogContent({ children, className, overlayClassName, - closable = false, }: DialogContentProps) { return ( - + - {closable && ( - - - - )} {children} - + ) } diff --git a/web/app/components/workflow-app/components/workflow-children.tsx b/web/app/components/workflow-app/components/workflow-children.tsx index 2634e8da2a..0fbb399dd7 100644 --- a/web/app/components/workflow-app/components/workflow-children.tsx +++ b/web/app/components/workflow-app/components/workflow-children.tsx @@ -147,7 +147,6 @@ const WorkflowChildren = () => { handleSyncWorkflowDraft(true, false, { onSuccess: () => { autoGenerateWebhookUrl(newNode.id) - console.log('Node successfully saved to draft') }, onError: () => { console.error('Failed to save node to draft') diff --git a/web/app/components/workflow-app/components/workflow-onboarding-modal/__tests__/index.spec.tsx b/web/app/components/workflow-app/components/workflow-onboarding-modal/__tests__/index.spec.tsx index ca627f9679..af38ca113f 100644 --- a/web/app/components/workflow-app/components/workflow-onboarding-modal/__tests__/index.spec.tsx +++ b/web/app/components/workflow-app/components/workflow-onboarding-modal/__tests__/index.spec.tsx @@ -1,65 +1,32 @@ +import type { ReactNode } from 'react' import { fireEvent, render, screen, waitFor } from '@testing-library/react' import userEvent from '@testing-library/user-event' -import * as React from 'react' import { BlockEnum } from '@/app/components/workflow/types' import WorkflowOnboardingModal from '../index' -// Mock Modal component -vi.mock('@/app/components/base/modal', () => ({ - default: function MockModal({ - isShow, - onClose, - children, - closable, +vi.mock('@/app/components/workflow/block-selector', () => ({ + default: function MockNodeSelector({ + open, + onSelect, + trigger, }: { - isShow: boolean - onClose?: () => void - children?: React.ReactNode - closable?: boolean + open?: boolean + onSelect: (type: BlockEnum, config?: Record) => void + trigger?: ((open: boolean) => ReactNode) | ReactNode }) { - if (!isShow) - return null - return ( -
- {closable && ( - +
+ {typeof trigger === 'function' ? trigger(Boolean(open)) : trigger} + {open && ( +
+ + +
)} - {children} -
- ) - }, -})) - -// Mock StartNodeSelectionPanel (using real component would be better for integration, -// but for this test we'll mock to control behavior) -vi.mock('../start-node-selection-panel', () => ({ - default: function MockStartNodeSelectionPanel({ - onSelectUserInput, - onSelectTrigger, - }: { - onSelectUserInput?: () => void - onSelectTrigger?: (type: BlockEnum, config?: Record) => void - }) { - return ( -
- - -
) }, @@ -79,401 +46,292 @@ describe('WorkflowOnboardingModal', () => { vi.clearAllMocks() }) - // Helper function to render component const renderComponent = (props = {}) => { return render() } + const getBackdrop = () => document.body.querySelector('.bg-workflow-canvas-canvas-overlay') + const getUserInputHeading = () => screen.getByRole('heading', { name: 'workflow.onboarding.userInputFull' }) + const getTriggerHeading = () => screen.getByRole('heading', { name: 'workflow.onboarding.trigger' }) - // Rendering tests (REQUIRED) describe('Rendering', () => { it('should render without crashing', () => { - // Arrange & Act renderComponent() - // Assert expect(screen.getByRole('dialog')).toBeInTheDocument() }) - it('should render modal when isShow is true', () => { - // Arrange & Act + it('should render dialog when isShow is true', () => { renderComponent({ isShow: true }) - // Assert - expect(screen.getByTestId('modal')).toBeInTheDocument() + expect(screen.getByRole('dialog')).toBeInTheDocument() }) - it('should not render modal when isShow is false', () => { - // Arrange & Act + it('should not render dialog when isShow is false', () => { renderComponent({ isShow: false }) - // Assert - expect(screen.queryByTestId('modal')).not.toBeInTheDocument() + expect(screen.queryByRole('dialog')).not.toBeInTheDocument() }) - it('should render modal title', () => { - // Arrange & Act + it('should render title', () => { renderComponent() - // Assert expect(screen.getByText('workflow.onboarding.title')).toBeInTheDocument() }) - it('should render modal description', () => { - // Arrange & Act - const { container } = renderComponent() + it('should render description', () => { + renderComponent() - // Assert - Check both parts of description (separated by link) - const descriptionDiv = container.querySelector('.body-xs-regular.leading-4') - expect(descriptionDiv).toBeInTheDocument() - expect(descriptionDiv).toHaveTextContent('workflow.onboarding.description') + expect(screen.getByText('workflow.onboarding.description')).toBeInTheDocument() }) it('should render StartNodeSelectionPanel', () => { - // Arrange & Act renderComponent() - // Assert - expect(screen.getByTestId('start-node-selection-panel')).toBeInTheDocument() + expect(getUserInputHeading()).toBeInTheDocument() + expect(getTriggerHeading()).toBeInTheDocument() }) - it('should render ESC tip when modal is shown', () => { - // Arrange & Act + it('should render ESC tip when shown', () => { renderComponent({ isShow: true }) - // Assert expect(screen.getByText('workflow.onboarding.escTip.press')).toBeInTheDocument() expect(screen.getByText('workflow.onboarding.escTip.key')).toBeInTheDocument() expect(screen.getByText('workflow.onboarding.escTip.toDismiss')).toBeInTheDocument() }) - it('should not render ESC tip when modal is hidden', () => { - // Arrange & Act + it('should not render ESC tip when hidden', () => { renderComponent({ isShow: false }) - // Assert expect(screen.queryByText('workflow.onboarding.escTip.press')).not.toBeInTheDocument() }) it('should have correct styling for title', () => { - // Arrange & Act renderComponent() - // Assert const title = screen.getByText('workflow.onboarding.title') expect(title).toHaveClass('title-2xl-semi-bold') expect(title).toHaveClass('text-text-primary') }) - it('should have modal close button', () => { - // Arrange & Act + it('should have close button', () => { renderComponent() - // Assert - expect(screen.getByTestId('modal-close-button')).toBeInTheDocument() + expect(screen.getByRole('button', { name: 'Close' })).toBeInTheDocument() + }) + + it('should render workflow canvas backdrop when shown', () => { + renderComponent({ isShow: true }) + + const backdrop = getBackdrop() + expect(backdrop).toBeInTheDocument() + expect(backdrop).not.toHaveClass('opacity-20') }) }) - // Props tests (REQUIRED) describe('Props', () => { it('should accept isShow prop', () => { - // Arrange & Act const { rerender } = renderComponent({ isShow: false }) - // Assert - expect(screen.queryByTestId('modal')).not.toBeInTheDocument() + expect(screen.queryByRole('dialog')).not.toBeInTheDocument() - // Act rerender() - // Assert - expect(screen.getByTestId('modal')).toBeInTheDocument() + expect(screen.getByRole('dialog')).toBeInTheDocument() }) it('should accept onClose prop', () => { - // Arrange const customOnClose = vi.fn() - // Act renderComponent({ onClose: customOnClose }) - // Assert - expect(screen.getByTestId('modal')).toBeInTheDocument() + expect(screen.getByRole('dialog')).toBeInTheDocument() }) it('should accept onSelectStartNode prop', () => { - // Arrange const customHandler = vi.fn() - // Act renderComponent({ onSelectStartNode: customHandler }) - // Assert - expect(screen.getByTestId('start-node-selection-panel')).toBeInTheDocument() - }) - - it('should handle undefined onClose gracefully', () => { - // Arrange & Act - expect(() => { - renderComponent({ onClose: undefined }) - }).not.toThrow() - - // Assert - expect(screen.getByTestId('modal')).toBeInTheDocument() - }) - - it('should handle undefined onSelectStartNode gracefully', () => { - // Arrange & Act - expect(() => { - renderComponent({ onSelectStartNode: undefined }) - }).not.toThrow() - - // Assert - expect(screen.getByTestId('modal')).toBeInTheDocument() + expect(getUserInputHeading()).toBeInTheDocument() }) }) - // User Interactions - Start Node Selection describe('User Interactions - Start Node Selection', () => { it('should call onSelectStartNode with Start block when user input is selected', async () => { - // Arrange const user = userEvent.setup() renderComponent() - // Act - const userInputButton = screen.getByTestId('select-user-input') - await user.click(userInputButton) + await user.click(getUserInputHeading()) - // Assert expect(mockOnSelectStartNode).toHaveBeenCalledTimes(1) expect(mockOnSelectStartNode).toHaveBeenCalledWith(BlockEnum.Start) }) - it('should call onClose after selecting user input', async () => { - // Arrange + it('should not call onClose when selecting user input (parent handles closing)', async () => { const user = userEvent.setup() renderComponent() - // Act - const userInputButton = screen.getByTestId('select-user-input') - await user.click(userInputButton) + await user.click(getUserInputHeading()) - // Assert - expect(mockOnClose).toHaveBeenCalledTimes(1) + expect(mockOnClose).not.toHaveBeenCalled() }) it('should call onSelectStartNode with trigger type when trigger is selected', async () => { - // Arrange const user = userEvent.setup() renderComponent() - // Act - const triggerButton = screen.getByTestId('select-trigger-schedule') - await user.click(triggerButton) + await user.click(getTriggerHeading()) + await user.click(screen.getByTestId('select-trigger-schedule')) - // Assert expect(mockOnSelectStartNode).toHaveBeenCalledTimes(1) expect(mockOnSelectStartNode).toHaveBeenCalledWith(BlockEnum.TriggerSchedule, undefined) }) - it('should call onClose after selecting trigger', async () => { - // Arrange + it('should not call onClose when selecting trigger (parent handles closing)', async () => { const user = userEvent.setup() renderComponent() - // Act - const triggerButton = screen.getByTestId('select-trigger-schedule') - await user.click(triggerButton) + await user.click(getTriggerHeading()) + await user.click(screen.getByTestId('select-trigger-schedule')) - // Assert - expect(mockOnClose).toHaveBeenCalledTimes(1) + expect(mockOnClose).not.toHaveBeenCalled() }) it('should pass tool config when selecting trigger with config', async () => { - // Arrange const user = userEvent.setup() renderComponent() - // Act - const webhookButton = screen.getByTestId('select-trigger-webhook') - await user.click(webhookButton) + await user.click(getTriggerHeading()) + await user.click(screen.getByTestId('select-trigger-webhook')) - // Assert expect(mockOnSelectStartNode).toHaveBeenCalledTimes(1) expect(mockOnSelectStartNode).toHaveBeenCalledWith(BlockEnum.TriggerWebhook, { config: 'test' }) - expect(mockOnClose).toHaveBeenCalledTimes(1) + expect(mockOnClose).not.toHaveBeenCalled() }) }) - // User Interactions - Modal Close - describe('User Interactions - Modal Close', () => { + describe('User Interactions - Dialog Close', () => { it('should call onClose when close button is clicked', async () => { - // Arrange const user = userEvent.setup() renderComponent() - // Act - const closeButton = screen.getByTestId('modal-close-button') - await user.click(closeButton) + await user.click(screen.getByRole('button', { name: 'Close' })) - // Assert expect(mockOnClose).toHaveBeenCalledTimes(1) }) it('should not call onSelectStartNode when closing without selection', async () => { - // Arrange const user = userEvent.setup() renderComponent() - // Act - const closeButton = screen.getByTestId('modal-close-button') - await user.click(closeButton) + await user.click(screen.getByRole('button', { name: 'Close' })) - // Assert expect(mockOnSelectStartNode).not.toHaveBeenCalled() expect(mockOnClose).toHaveBeenCalledTimes(1) }) + + it('should call onClose exactly once when close button is clicked (no double-close)', async () => { + const user = userEvent.setup() + const onClose = vi.fn() + renderComponent({ onClose }) + + await user.click(screen.getByRole('button', { name: 'Close' })) + + expect(onClose).toHaveBeenCalledTimes(1) + }) + + it('should not call onClose when clicking backdrop', async () => { + const user = userEvent.setup() + renderComponent() + + const backdrop = getBackdrop() + expect(backdrop).toBeInTheDocument() + if (!backdrop) + throw new Error('backdrop should exist when dialog is open') + + await user.click(backdrop) + + expect(mockOnClose).not.toHaveBeenCalled() + }) }) - // Keyboard Event Handling describe('Keyboard Event Handling', () => { it('should call onClose when ESC key is pressed', () => { - // Arrange renderComponent({ isShow: true }) - // Act - fireEvent.keyDown(document, { key: 'Escape', code: 'Escape' }) + fireEvent.keyDown(screen.getByRole('dialog'), { key: 'Escape' }) - // Assert expect(mockOnClose).toHaveBeenCalledTimes(1) }) - it('should not call onClose when other keys are pressed', () => { - // Arrange - renderComponent({ isShow: true }) - - // Act - fireEvent.keyDown(document, { key: 'Enter', code: 'Enter' }) - fireEvent.keyDown(document, { key: 'Tab', code: 'Tab' }) - fireEvent.keyDown(document, { key: 'a', code: 'KeyA' }) - - // Assert - expect(mockOnClose).not.toHaveBeenCalled() - }) - - it('should not call onClose when ESC is pressed but modal is hidden', () => { - // Arrange + it('should not call onClose when ESC is pressed but dialog is hidden', () => { renderComponent({ isShow: false }) - // Act - fireEvent.keyDown(document, { key: 'Escape', code: 'Escape' }) + fireEvent.keyDown(document, { key: 'Escape' }) - // Assert expect(mockOnClose).not.toHaveBeenCalled() }) - it('should clean up event listener on unmount', () => { - // Arrange + it('should clean up on unmount', () => { const { unmount } = renderComponent({ isShow: true }) - // Act unmount() - fireEvent.keyDown(document, { key: 'Escape', code: 'Escape' }) + fireEvent.keyDown(document, { key: 'Escape' }) - // Assert expect(mockOnClose).not.toHaveBeenCalled() }) - it('should update event listener when isShow changes', () => { - // Arrange + it('should respond to ESC based on open state', () => { const { rerender } = renderComponent({ isShow: true }) - // Act - Press ESC when shown - fireEvent.keyDown(document, { key: 'Escape', code: 'Escape' }) - - // Assert + fireEvent.keyDown(screen.getByRole('dialog'), { key: 'Escape' }) expect(mockOnClose).toHaveBeenCalledTimes(1) - // Act - Hide modal and clear mock mockOnClose.mockClear() rerender() - // Act - Press ESC when hidden - fireEvent.keyDown(document, { key: 'Escape', code: 'Escape' }) - - // Assert + fireEvent.keyDown(document, { key: 'Escape' }) expect(mockOnClose).not.toHaveBeenCalled() }) - - it('should handle multiple ESC key presses', () => { - // Arrange - renderComponent({ isShow: true }) - - // Act - fireEvent.keyDown(document, { key: 'Escape', code: 'Escape' }) - fireEvent.keyDown(document, { key: 'Escape', code: 'Escape' }) - fireEvent.keyDown(document, { key: 'Escape', code: 'Escape' }) - - // Assert - expect(mockOnClose).toHaveBeenCalledTimes(3) - }) }) - // Edge Cases (REQUIRED) describe('Edge Cases', () => { - it('should handle rapid modal show/hide toggling', async () => { - // Arrange + it('should handle rapid show/hide toggling', async () => { const { rerender } = renderComponent({ isShow: false }) - // Assert - expect(screen.queryByTestId('modal')).not.toBeInTheDocument() + expect(screen.queryByRole('dialog')).not.toBeInTheDocument() - // Act rerender() + expect(screen.getByRole('dialog')).toBeInTheDocument() - // Assert - expect(screen.getByTestId('modal')).toBeInTheDocument() - - // Act rerender() - - // Assert await waitFor(() => { - expect(screen.queryByTestId('modal')).not.toBeInTheDocument() + expect(screen.queryByRole('dialog')).not.toBeInTheDocument() }) }) it('should handle selecting multiple nodes in sequence', async () => { - // Arrange const user = userEvent.setup() const { rerender } = renderComponent() - // Act - Select user input - await user.click(screen.getByTestId('select-user-input')) - - // Assert + await user.click(getUserInputHeading()) expect(mockOnSelectStartNode).toHaveBeenCalledWith(BlockEnum.Start) - expect(mockOnClose).toHaveBeenCalledTimes(1) + expect(mockOnClose).not.toHaveBeenCalled() - // Act - Re-show modal and select trigger - mockOnClose.mockClear() mockOnSelectStartNode.mockClear() rerender() + await user.click(getTriggerHeading()) await user.click(screen.getByTestId('select-trigger-schedule')) - - // Assert expect(mockOnSelectStartNode).toHaveBeenCalledWith(BlockEnum.TriggerSchedule, undefined) - expect(mockOnClose).toHaveBeenCalledTimes(1) + expect(mockOnClose).not.toHaveBeenCalled() }) it('should handle prop updates correctly', () => { - // Arrange const { rerender } = renderComponent({ isShow: true }) - // Assert - expect(screen.getByTestId('modal')).toBeInTheDocument() + expect(screen.getByRole('dialog')).toBeInTheDocument() - // Act - Update props const newOnClose = vi.fn() const newOnSelectStartNode = vi.fn() rerender( @@ -484,169 +342,120 @@ describe('WorkflowOnboardingModal', () => { />, ) - // Assert - Modal still renders with new props - expect(screen.getByTestId('modal')).toBeInTheDocument() + expect(screen.getByRole('dialog')).toBeInTheDocument() }) - it('should handle onClose being called multiple times', async () => { - // Arrange - const user = userEvent.setup() - renderComponent() - - // Act - await user.click(screen.getByTestId('modal-close-button')) - await user.click(screen.getByTestId('modal-close-button')) - - // Assert - expect(mockOnClose).toHaveBeenCalledTimes(2) - }) - - it('should maintain modal state when props change', () => { - // Arrange + it('should maintain dialog when props change', () => { const { rerender } = renderComponent({ isShow: true }) - // Assert - expect(screen.getByTestId('modal')).toBeInTheDocument() + expect(screen.getByRole('dialog')).toBeInTheDocument() - // Act - Change onClose handler const newOnClose = vi.fn() rerender() - // Assert - Modal should still be visible - expect(screen.getByTestId('modal')).toBeInTheDocument() + expect(screen.getByRole('dialog')).toBeInTheDocument() }) }) - // Accessibility Tests describe('Accessibility', () => { it('should have dialog role', () => { - // Arrange & Act renderComponent() - // Assert expect(screen.getByRole('dialog')).toBeInTheDocument() }) it('should have proper heading hierarchy', () => { - // Arrange & Act - const { container } = renderComponent() + renderComponent() - // Assert - const heading = container.querySelector('h3') + const heading = screen.getByRole('heading', { name: 'workflow.onboarding.title' }) expect(heading).toBeInTheDocument() expect(heading).toHaveTextContent('workflow.onboarding.title') }) - it('should have keyboard navigation support via ESC key', () => { - // Arrange + it('should expose dialog accessible name from title', () => { + renderComponent() + + expect(screen.getByRole('dialog', { name: 'workflow.onboarding.title' })).toBeInTheDocument() + }) + + it('should support ESC key dismissal', () => { renderComponent({ isShow: true }) - // Act - fireEvent.keyDown(document, { key: 'Escape', code: 'Escape' }) + fireEvent.keyDown(screen.getByRole('dialog'), { key: 'Escape' }) - // Assert expect(mockOnClose).toHaveBeenCalledTimes(1) }) it('should have visible ESC key hint', () => { - // Arrange & Act renderComponent({ isShow: true }) - // Assert - ShortcutsName component renders keys in div elements with system-kbd class const escKey = screen.getByText('workflow.onboarding.escTip.key') - // ShortcutsName renders a
with class system-kbd, not a element expect(escKey.closest('.system-kbd')).toBeInTheDocument() }) it('should have descriptive text for ESC functionality', () => { - // Arrange & Act renderComponent({ isShow: true }) - // Assert expect(screen.getByText('workflow.onboarding.escTip.press')).toBeInTheDocument() expect(screen.getByText('workflow.onboarding.escTip.toDismiss')).toBeInTheDocument() }) it('should have proper text color classes', () => { - // Arrange & Act renderComponent() - // Assert const title = screen.getByText('workflow.onboarding.title') expect(title).toHaveClass('text-text-primary') }) }) - // Integration Tests describe('Integration', () => { it('should complete full flow of selecting user input node', async () => { - // Arrange const user = userEvent.setup() renderComponent() - // Assert - Initial state - expect(screen.getByTestId('modal')).toBeInTheDocument() + expect(screen.getByRole('dialog')).toBeInTheDocument() expect(screen.getByText('workflow.onboarding.title')).toBeInTheDocument() - expect(screen.getByTestId('start-node-selection-panel')).toBeInTheDocument() + expect(getUserInputHeading()).toBeInTheDocument() - // Act - Select user input - await user.click(screen.getByTestId('select-user-input')) + await user.click(getUserInputHeading()) - // Assert - Callbacks called expect(mockOnSelectStartNode).toHaveBeenCalledWith(BlockEnum.Start) - expect(mockOnClose).toHaveBeenCalledTimes(1) + expect(mockOnClose).not.toHaveBeenCalled() }) it('should complete full flow of selecting trigger node', async () => { - // Arrange const user = userEvent.setup() renderComponent() - // Assert - Initial state - expect(screen.getByTestId('modal')).toBeInTheDocument() + expect(screen.getByRole('dialog')).toBeInTheDocument() - // Act - Select trigger + await user.click(getTriggerHeading()) await user.click(screen.getByTestId('select-trigger-webhook')) - // Assert - Callbacks called with config expect(mockOnSelectStartNode).toHaveBeenCalledWith(BlockEnum.TriggerWebhook, { config: 'test' }) - expect(mockOnClose).toHaveBeenCalledTimes(1) + expect(mockOnClose).not.toHaveBeenCalled() }) it('should render all components in correct hierarchy', () => { - // Arrange & Act - const { container } = renderComponent() + renderComponent() - // Assert - Modal is the root - expect(screen.getByTestId('modal')).toBeInTheDocument() - - // Assert - Header elements - const heading = container.querySelector('h3') - expect(heading).toBeInTheDocument() - - // Assert - Selection panel - expect(screen.getByTestId('start-node-selection-panel')).toBeInTheDocument() - - // Assert - ESC tip + const dialog = screen.getByRole('dialog') + expect(dialog).toBeInTheDocument() + expect(screen.getByText('workflow.onboarding.title')).toBeInTheDocument() + expect(getUserInputHeading()).toBeInTheDocument() expect(screen.getByText('workflow.onboarding.escTip.key')).toBeInTheDocument() + expect(dialog).not.toContainElement(screen.getByText('workflow.onboarding.escTip.key')) }) it('should coordinate between keyboard and click interactions', async () => { - // Arrange const user = userEvent.setup() renderComponent() - // Act - Click close button - await user.click(screen.getByTestId('modal-close-button')) - - // Assert + await user.click(screen.getByRole('button', { name: 'Close' })) expect(mockOnClose).toHaveBeenCalledTimes(1) - // Act - Clear and try ESC key mockOnClose.mockClear() - fireEvent.keyDown(document, { key: 'Escape', code: 'Escape' }) - - // Assert + fireEvent.keyDown(screen.getByRole('dialog'), { key: 'Escape' }) expect(mockOnClose).toHaveBeenCalledTimes(1) }) }) diff --git a/web/app/components/workflow-app/components/workflow-onboarding-modal/__tests__/start-node-option.spec.tsx b/web/app/components/workflow-app/components/workflow-onboarding-modal/__tests__/start-node-option.spec.tsx index 04c223499a..2739c51b62 100644 --- a/web/app/components/workflow-app/components/workflow-onboarding-modal/__tests__/start-node-option.spec.tsx +++ b/web/app/components/workflow-app/components/workflow-onboarding-modal/__tests__/start-node-option.spec.tsx @@ -47,7 +47,6 @@ describe('StartNodeOption', () => { // Assert const title = screen.getByText('Test Title') expect(title).toBeInTheDocument() - expect(title).toHaveClass('system-md-semi-bold') expect(title).toHaveClass('text-text-primary') }) diff --git a/web/app/components/workflow-app/components/workflow-onboarding-modal/index.tsx b/web/app/components/workflow-app/components/workflow-onboarding-modal/index.tsx index 16bae51246..8db281bda0 100644 --- a/web/app/components/workflow-app/components/workflow-onboarding-modal/index.tsx +++ b/web/app/components/workflow-app/components/workflow-onboarding-modal/index.tsx @@ -1,12 +1,8 @@ 'use client' import type { FC } from 'react' import type { PluginDefaultValue } from '@/app/components/workflow/block-selector/types' -import { - useCallback, - useEffect, -} from 'react' import { useTranslation } from 'react-i18next' -import Modal from '@/app/components/base/modal' +import { Dialog, DialogCloseButton, DialogContent, DialogDescription, DialogPortal, DialogTitle } from '@/app/components/base/ui/dialog' import ShortcutsName from '@/app/components/workflow/shortcuts-name' import { BlockEnum } from '@/app/components/workflow/types' import StartNodeSelectionPanel from './start-node-selection-panel' @@ -24,63 +20,39 @@ const WorkflowOnboardingModal: FC = ({ }) => { const { t } = useTranslation() - const handleSelectUserInput = useCallback(() => { - onSelectStartNode(BlockEnum.Start) - onClose() // Close modal after selection - }, [onSelectStartNode, onClose]) - - const handleTriggerSelect = useCallback((nodeType: BlockEnum, toolConfig?: PluginDefaultValue) => { - onSelectStartNode(nodeType, toolConfig) - onClose() // Close modal after selection - }, [onSelectStartNode, onClose]) - - useEffect(() => { - const handleEsc = (e: KeyboardEvent) => { - if (e.key === 'Escape' && isShow) - onClose() - } - document.addEventListener('keydown', handleEsc) - return () => document.removeEventListener('keydown', handleEsc) - }, [isShow, onClose]) - return ( - <> - + + +
- {/* Header */}
-

+ {t('onboarding.title', { ns: 'workflow' })} -

-
+ + {t('onboarding.description', { ns: 'workflow' })} -
+
- {/* Content */} onSelectStartNode(BlockEnum.Start)} + onSelectTrigger={onSelectStartNode} />
- + - {/* ESC tip below modal */} - {isShow && ( -
+ +
{t('onboarding.escTip.press', { ns: 'workflow' })} {t('onboarding.escTip.toDismiss', { ns: 'workflow' })}
- )} - + +
) } diff --git a/web/app/components/workflow-app/components/workflow-onboarding-modal/start-node-option.tsx b/web/app/components/workflow-app/components/workflow-onboarding-modal/start-node-option.tsx index 77f2a842c9..8b1ce699e7 100644 --- a/web/app/components/workflow-app/components/workflow-onboarding-modal/start-node-option.tsx +++ b/web/app/components/workflow-app/components/workflow-onboarding-modal/start-node-option.tsx @@ -1,6 +1,5 @@ 'use client' import type { FC, ReactNode } from 'react' -import { cn } from '@/utils/classnames' type StartNodeOptionProps = { icon: ReactNode @@ -20,22 +19,18 @@ const StartNodeOption: FC = ({ return (
- {/* Icon */}
{icon}
- {/* Text content */}
-

+

{title} {subtitle && ( - + {' '} {subtitle} @@ -44,7 +39,7 @@ const StartNodeOption: FC = ({

-

+

{description}

diff --git a/web/app/components/workflow-app/components/workflow-onboarding-modal/start-node-selection-panel.tsx b/web/app/components/workflow-app/components/workflow-onboarding-modal/start-node-selection-panel.tsx index 6d13cbf6a8..b4a1cd135b 100644 --- a/web/app/components/workflow-app/components/workflow-onboarding-modal/start-node-selection-panel.tsx +++ b/web/app/components/workflow-app/components/workflow-onboarding-modal/start-node-selection-panel.tsx @@ -21,10 +21,6 @@ const StartNodeSelectionPanel: FC = ({ const { t } = useTranslation() const [showTriggerSelector, setShowTriggerSelector] = useState(false) - const handleTriggerClick = useCallback(() => { - setShowTriggerSelector(true) - }, []) - const handleTriggerSelect = useCallback((nodeType: BlockEnum, toolConfig?: PluginDefaultValue) => { setShowTriggerSelector(false) onSelectTrigger(nodeType, toolConfig) @@ -67,10 +63,9 @@ const StartNodeSelectionPanel: FC = ({ )} title={t('onboarding.trigger', { ns: 'workflow' })} description={t('onboarding.triggerDescription', { ns: 'workflow' })} - onClick={handleTriggerClick} + onClick={() => setShowTriggerSelector(true)} /> )} - popupClassName="z-[1200]" />
) diff --git a/web/docs/overlay-migration.md b/web/docs/overlay-migration.md index 77c5fe5b04..ffe54afa1a 100644 --- a/web/docs/overlay-migration.md +++ b/web/docs/overlay-migration.md @@ -1,10 +1,14 @@ # Overlay Migration Guide -This document tracks the migration away from legacy `portal-to-follow-elem` APIs. +This document tracks the migration away from legacy overlay APIs. ## Scope -- Deprecated API: `@/app/components/base/portal-to-follow-elem` +- Deprecated imports: + - `@/app/components/base/portal-to-follow-elem` + - `@/app/components/base/tooltip` + - `@/app/components/base/modal` + - `@/app/components/base/select` (including `custom` / `pure`) - Replacement primitives: - `@/app/components/base/ui/tooltip` - `@/app/components/base/ui/dropdown-menu` @@ -15,33 +19,33 @@ This document tracks the migration away from legacy `portal-to-follow-elem` APIs ## ESLint policy -- `no-restricted-imports` blocks new usage of `portal-to-follow-elem`. -- The rule is enabled for normal source files and test files are excluded. -- Legacy `app/components/base/*` callers are temporarily allowlisted in ESLint config. +- `no-restricted-imports` blocks all deprecated imports listed above. +- The rule is enabled for normal source files (`.ts` / `.tsx`) and test files are excluded. +- Legacy `app/components/base/*` callers are temporarily allowlisted in `OVERLAY_MIGRATION_LEGACY_BASE_FILES` (`web/eslint.constants.mjs`). - New files must not be added to the allowlist without migration owner approval. ## Migration phases 1. Business/UI features outside `app/components/base/**` - - Migrate old calls to semantic primitives. - - Keep `eslint-suppressions.json` stable or shrinking. + - Migrate old calls to semantic primitives from `@/app/components/base/ui/**`. + - Keep deprecated imports out of newly touched files. 1. Legacy base components in allowlist - Migrate allowlisted base callers gradually. - - Remove migrated files from allowlist immediately. + - Remove migrated files from `OVERLAY_MIGRATION_LEGACY_BASE_FILES` immediately. 1. Cleanup - - Remove remaining suppressions for `no-restricted-imports`. - - Remove legacy `portal-to-follow-elem` implementation. + - Remove remaining allowlist entries. + - Remove legacy overlay implementations when import count reaches zero. -## Suppression maintenance +## Allowlist maintenance - After each migration batch, run: ```sh -pnpm eslint --prune-suppressions --pass-on-unpruned-suppressions +pnpm -C web lint:fix --prune-suppressions ``` -- Never increase suppressions to bypass new code. -- Prefer direct migration over adding suppression entries. +- If a migrated file was in the allowlist, remove it from `web/eslint.constants.mjs` in the same PR. +- Never increase allowlist scope to bypass new code. ## React Refresh policy for base UI primitives diff --git a/web/eslint-suppressions.json b/web/eslint-suppressions.json index 4b64291612..f0eb575d8d 100644 --- a/web/eslint-suppressions.json +++ b/web/eslint-suppressions.json @@ -6298,9 +6298,6 @@ } }, "app/components/workflow-app/components/workflow-children.tsx": { - "no-console": { - "count": 1 - }, "ts/no-explicit-any": { "count": 3 } @@ -6310,19 +6307,6 @@ "count": 2 } }, - "app/components/workflow-app/components/workflow-onboarding-modal/index.tsx": { - "no-restricted-imports": { - "count": 1 - }, - "tailwindcss/enforce-consistent-class-order": { - "count": 3 - } - }, - "app/components/workflow-app/components/workflow-onboarding-modal/start-node-option.tsx": { - "tailwindcss/enforce-consistent-class-order": { - "count": 2 - } - }, "app/components/workflow-app/hooks/use-DSL.ts": { "ts/no-explicit-any": { "count": 1