[refactor] Unify small modal dialog styles with showSmallLayoutDialog#8834
Merged
[refactor] Unify small modal dialog styles with showSmallLayoutDialog#8834
Conversation
viva-jinyi
added
enhancement
dosubot
bot
added
the
size:L
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 02/20/2026, 03:54:55 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
|
Playwright: ✅ 522 passed, 0 failed · 2 flaky 📊 Browser Reports
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces dedicated small-layout dialog composables (MissingModels, MissingNodes, ImportFailedNode, NodeConflict), removes several specialized dialog functions from the central dialog service in favor of Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Component / Service
participant Composable as Dialog_Composable
participant Service as Dialog_Service
participant Store as Dialog_Store
participant Parts as Header_Content_Footer
Caller->>Composable: show(options)
Composable->>Service: showSmallLayoutDialog(options...)
Service->>Store: openDialog(key, componentRefs, mergedProps)
Store->>Parts: render(header, content, footer with props)
Parts->>Store: user action (e.g., button click) -> request close
Store-->>Composable: dialog closed / lifecycle complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 21.4 kB (baseline 21.7 kB) • 🟢 -330 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 914 kB (baseline 914 kB) • 🟢 -194 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 68.6 kB (baseline 69 kB) • 🟢 -474 BTop-level views, pages, and routed surfaces
Status: 11 added / 11 removed Panels & Settings — 430 kB (baseline 430 kB) • 🟢 -629 BConfiguration panels, inspectors, and settings screens
Status: 22 added / 22 removed User & Accounts — 16 kB (baseline 16.1 kB) • 🟢 -158 BAuthentication, profile, and account management bundles
Status: 7 added / 7 removed Editors & Dialogs — 706 B (baseline 785 B) • 🟢 -79 BModals, dialogs, drawers, and in-app editors
Status: 1 added / 1 removed UI Components — 42.3 kB (baseline 42.5 kB) • 🟢 -236 BReusable component library chunks
Status: 11 added / 11 removed Data & Services — 2.4 MB (baseline 2.17 MB) • 🔴 +230 kBStores, services, APIs, and repositories
Status: 14 added / 15 removed Utilities & Hooks — 57.6 kB (baseline 237 kB) • 🟢 -180 kBHelpers, composables, and utility bundles
Status: 18 added / 20 removed Vendor & Third-Party — 8.69 MB (baseline 8.69 MB) • 🟢 -68 BExternal libraries and shared vendor chunks
Status: 6 added / 6 removed Other — 7.38 MB (baseline 7.45 MB) • 🟢 -70.4 kBBundles that do not match a named category
Status: 112 added / 122 removed |
DrJKL
reviewed
Feb 12, 2026
src/workbench/extensions/manager/composables/useImportFailedDetection.test.ts
Outdated
Show resolved
Hide resolved
DrJKL
reviewed
Feb 12, 2026
src/workbench/extensions/manager/composables/useImportFailedDetection.test.ts
Outdated
Show resolved
Hide resolved
DrJKL
reviewed
Feb 12, 2026
src/workbench/extensions/manager/composables/useImportFailedNodeDialog.ts
Outdated
Show resolved
Hide resolved
DrJKL
previously approved these changes
Feb 12, 2026
viva-jinyi
force-pushed
the
refactor/small-dialog-a
branch
from
217d3c4 to
b3a10b6
Compare
viva-jinyi
added
the
New Browser Test Expectations
viva-jinyi
force-pushed
the
refactor/small-dialog-a
branch
from
848e92c to
29847d7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/composables/useHelpCenter.ts (1)
74-83:voidis stale now thatshow()is synchronous.
show()fromuseNodeConflictDialogdoesn't return a Promise, sovoidno longer serves its original no-floating-promises purpose and is misleading.♻️ Proposed cleanup
const showConflictModal = () => { - void showNodeConflictDialog({ + showNodeConflictDialog({ showAfterWhatsNew: true, dialogComponentProps: { onClose: () => { markConflictsAsSeen() } } }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/composables/useHelpCenter.ts` around lines 74 - 83, The showConflictModal wrapper is using a now-stale leading void when calling showNodeConflictDialog even though useNodeConflictDialog().show() is synchronous; remove the misleading void and call showNodeConflictDialog(...) directly in showConflictModal (retain the same options including showAfterWhatsNew and dialogComponentProps.onClose calling markConflictsAsSeen) so the call reflects its synchronous nature.src/services/dialogService.ts (1)
378-391:!Tailwind important prefixes violate the coding guidelines.The
ptconfig contains!p-0,!m-0,!w-7,!h-7,!border-none,!outline-none,!p-2, and!m-1.5— all using the!importantprefix. The guidelines require finding and correcting the interfering PrimeVue base classes instead of overriding them here.As per coding guidelines: "Never use
!importantor the!important prefix for Tailwind classes; instead find and correct interfering existing!importantclasses."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/dialogService.ts` around lines 378 - 391, The pt configuration is using Tailwind's `!` important prefix (e.g., in pt.root, pt.header, pt.content, pt.footer, and pt.pcCloseButton.root class strings like '!p-0', '!m-0', '!w-7', etc.), which violates guidelines; remove all `!` prefixes from those class values and instead fix the underlying PrimeVue base-class conflict by adjusting component-specific CSS or theme overrides (e.g., increase selector specificity or target the PrimeVue wrapper/close-button classes in the stylesheet) so the intended padding, margin, sizing and border/outline styles apply without using `!important`. Ensure the keys referenced (pt, pt.root, pt.header, pt.content, pt.footer, pt.pcCloseButton.root) retain the plain Tailwind classes (no `!`) after you remove the prefixes and implement the CSS override in the theme or component stylesheet.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/composables/useMissingModelsDialog.ts`:
- Around line 15-21: The show function in useMissingModelsDialog currently calls
dialogStore.showDialog directly so MissingModelsWarning won't get the unified
small-dialog pt styling; verify whether MissingModelsWarning intentionally
applies its own styles, otherwise change useMissingModelsDialog.show to call
useDialogService().showSmallLayoutDialog({ key: DIALOG_KEY, component:
MissingModelsWarning, props }) instead of dialogStore.showDialog(...) so the
shared bg-base-background/border-border-default and close-button styles are
applied; reference DIALOG_KEY, MissingModelsWarning, dialogStore.showDialog, and
useDialogService().showSmallLayoutDialog when making the change.
---
Nitpick comments:
In `@src/composables/useHelpCenter.ts`:
- Around line 74-83: The showConflictModal wrapper is using a now-stale leading
void when calling showNodeConflictDialog even though
useNodeConflictDialog().show() is synchronous; remove the misleading void and
call showNodeConflictDialog(...) directly in showConflictModal (retain the same
options including showAfterWhatsNew and dialogComponentProps.onClose calling
markConflictsAsSeen) so the call reflects its synchronous nature.
In `@src/services/dialogService.ts`:
- Around line 378-391: The pt configuration is using Tailwind's `!` important
prefix (e.g., in pt.root, pt.header, pt.content, pt.footer, and
pt.pcCloseButton.root class strings like '!p-0', '!m-0', '!w-7', etc.), which
violates guidelines; remove all `!` prefixes from those class values and instead
fix the underlying PrimeVue base-class conflict by adjusting component-specific
CSS or theme overrides (e.g., increase selector specificity or target the
PrimeVue wrapper/close-button classes in the stylesheet) so the intended
padding, margin, sizing and border/outline styles apply without using
`!important`. Ensure the keys referenced (pt, pt.root, pt.header, pt.content,
pt.footer, pt.pcCloseButton.root) retain the plain Tailwind classes (no `!`)
after you remove the prefixes and implement the CSS override in the theme or
component stylesheet.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/workbench/extensions/manager/composables/useImportFailedDetection.test.ts (2)
40-40: Use the imported function reference as thedescribelabel instead of a string literal.
useImportFailedDetectionis already imported on line 6 and can be passed directly as the suite name.-describe('useImportFailedDetection', () => { +describe(useImportFailedDetection, () => {Based on learnings: "follow the
vitest/prefer-describe-function-titlerule by usingdescribe(ComponentOrFunction, ...)instead of a string literal description when naming test suites."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workbench/extensions/manager/composables/useImportFailedDetection.test.ts` at line 40, Replace the string literal test suite label with the imported function reference: change the describe call that currently uses the string 'useImportFailedDetection' to pass the imported symbol useImportFailedDetection directly (e.g., describe(useImportFailedDetection, ...)), ensuring the existing import on line 6 remains and the test suite name now references the actual function.
8-29: Consider usingvi.hoisted()for mock functions referenced invi.mock()factories.
mockIsPackInstalled,mockGetConflictsForPackageByID, andmockShoware mutable module-levelvi.fn()instances referenced insidevi.mock()factory closures. While this works in practice (factories execute lazily after variable initialization), the idiomatic Vitest pattern for this scenario isvi.hoisted(), which makes the initialization order explicit and safe.♻️ Proposed refactor using
vi.hoisted()+const { mockIsPackInstalled, mockGetConflictsForPackageByID, mockShow } = + vi.hoisted(() => ({ + mockIsPackInstalled: vi.fn(), + mockGetConflictsForPackageByID: vi.fn(), + mockShow: vi.fn() + })) + -const mockIsPackInstalled = vi.fn() -const mockGetConflictsForPackageByID = vi.fn() -const mockShow = vi.fn() vi.mock('@/workbench/extensions/manager/stores/comfyManagerStore', () => ({ useComfyManagerStore: () => ({ isPackInstalled: mockIsPackInstalled }) })) vi.mock('@/workbench/extensions/manager/stores/conflictDetectionStore', () => ({ useConflictDetectionStore: () => ({ getConflictsForPackageByID: mockGetConflictsForPackageByID }) })) vi.mock( '@/workbench/extensions/manager/composables/useImportFailedNodeDialog', () => ({ useImportFailedNodeDialog: () => ({ show: mockShow }) }) )Based on learnings: "Keep module mocks contained; do not use global mutable state within test files; use
vi.hoisted()if necessary for per-test Arrange phase manipulation."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workbench/extensions/manager/composables/useImportFailedDetection.test.ts` around lines 8 - 29, The test uses module-level mutable vi.fn() instances (mockIsPackInstalled, mockGetConflictsForPackageByID, mockShow) inside vi.mock() factories; replace those vi.fn() initializations with hoisted mocks by creating them via vi.hoisted(() => vi.fn()) so the mocks are initialized in a stable order before the mock factories run, and keep the rest of the vi.mock(...) factories referencing the same symbol names (mockIsPackInstalled, mockGetConflictsForPackageByID, mockShow) so per-test manipulation still works.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/composables/useMissingModelsDialog.ts`:
- Line 18: The test mock for useDialogService is missing the
showSmallLayoutDialog function, causing TypeError when
missingModelsDialog.show() calls showSmallLayoutDialog; update the test mock in
workflowService.test.ts to include showSmallLayoutDialog: vi.fn() on the mocked
return of useDialogService(), or alternatively stub the composable directly by
mocking useMissingModelsDialog to provide a show() implementation—this ensures
missingModelsDialog.show() can destructure and call showSmallLayoutDialog
without throwing.
In `@src/platform/workflow/core/services/workflowService.ts`:
- Around line 462-469: Test failures are caused by missing mocks for the dialog
helper used by missingNodesDialog.show and missingModelsDialog.show; update the
test setup in workflowService.test.ts to mock useDialogService (or directly mock
useMissingModelsDialog) to provide a stub implementation of
showSmallLayoutDialog so those call sites succeed; specifically add a mock for
useDialogService that exports showSmallLayoutDialog (or adjust the existing
useMissingModelsDialog mock) and ensure missingNodesDialog.show and
missingModelsDialog.show will call the mocked showSmallLayoutDialog during
tests.
---
Duplicate comments:
In `@src/composables/useMissingModelsDialog.ts`:
- Around line 17-23: No change required—the show function correctly routes
through showSmallLayoutDialog now; keep the implementation in show(props:
ComponentAttrs<typeof MissingModelsWarning>) that calls showSmallLayoutDialog
with key DIALOG_KEY, component MissingModelsWarning, and props as-is to maintain
the unified small-layout pt behavior.
---
Nitpick comments:
In
`@src/workbench/extensions/manager/composables/useImportFailedDetection.test.ts`:
- Line 40: Replace the string literal test suite label with the imported
function reference: change the describe call that currently uses the string
'useImportFailedDetection' to pass the imported symbol useImportFailedDetection
directly (e.g., describe(useImportFailedDetection, ...)), ensuring the existing
import on line 6 remains and the test suite name now references the actual
function.
- Around line 8-29: The test uses module-level mutable vi.fn() instances
(mockIsPackInstalled, mockGetConflictsForPackageByID, mockShow) inside vi.mock()
factories; replace those vi.fn() initializations with hoisted mocks by creating
them via vi.hoisted(() => vi.fn()) so the mocks are initialized in a stable
order before the mock factories run, and keep the rest of the vi.mock(...)
factories referencing the same symbol names (mockIsPackInstalled,
mockGetConflictsForPackageByID, mockShow) so per-test manipulation still works.
DrJKL
reviewed
Feb 18, 2026
Contributor
There was a problem hiding this comment.
Pretty sure we still need this.
Contributor
There was a problem hiding this comment.
Might need to pnpm i to update the Vite rolldown types.
Contributor
There was a problem hiding this comment.
We'll fix it again in another PR 😉
DrJKL
previously approved these changes
Feb 18, 2026
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
viva-jinyi
added
New Browser Test Expectations
|
Updating Playwright Expectations |
github-actions
bot
removed
the
New Browser Test Expectations
DrJKL
approved these changes
Feb 20, 2026
christian-byrne
added a commit
that referenced
this pull request
Feb 21, 2026
Rolldown requires strictExecutionOrder to guarantee Storybook's internal module globals (__STORYBOOK_MODULE_*) are defined before they are referenced. Without it, Chromatic fails with: Error: __STORYBOOK_MODULE_CORE_EVENTS_PREVIEW_ERRORS__ is not defined Accidentally removed in #8834 as a merge artifact. Amp-Thread-ID: https://ampcode.com/threads/T-019c7dd0-e214-731a-b7de-ec7b524eb7fc
DrJKL
pushed a commit
that referenced
this pull request
Feb 21, 2026
## Summary Restore `strictExecutionOrder: true` in `.storybook/main.ts` rolldownOptions, accidentally removed in #8834 as a merge artifact. ## Problem Chromatic visual regression tests fail on `version-bump-*` release branches with: ``` Error: __STORYBOOK_MODULE_CORE_EVENTS_PREVIEW_ERRORS__ is not defined ``` Rolldown without `strictExecutionOrder` doesn't guarantee module execution order. Storybook's internal module system defines `__STORYBOOK_MODULE_*` globals during initialization — without strict ordering, downstream code references them before they're defined. Only `version-bump-*` branches are affected because the `chromatic-deployment` CI job (which actually loads and extracts stories at runtime) is gated to those branches. ## Changes - Restore `strictExecutionOrder: true` in `.storybook/main.ts` rolldownOptions output config ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-9038-fix-restore-strictExecutionOrder-for-Storybook-Chromatic-builds-30e6d73d365081489c53cea131b97a2f) by [Unito](https://www.unito.io)
huntcsg
pushed a commit
that referenced
this pull request
Feb 21, 2026
…#8834) ## Summary Extract a shared `showSmallLayoutDialog` utility and move dialog-specific logic into composables, unifying the duplicated `pt` configurations across small modal dialogs. ## Changes - **`showSmallLayoutDialog`**: Added to `dialogService.ts` with a single unified `pt` config for all small modal dialogs (missing nodes, missing models, import failed, node conflict) - **Composables**: Extracted 4 dialog functions from `dialogService` into dedicated composables following the `useSettingsDialog` / `useModelSelectorDialog` pattern: - `useMissingNodesDialog` - `useMissingModelsDialog` - `useImportFailedNodeDialog` - `useNodeConflictDialog` - Each composable uses direct imports, synchronous `show()`, `hide()`, and a `DIALOG_KEY` constant - Updated all call sites (`app.ts`, `useHelpCenter`, `PackEnableToggle`, `PackInstallButton`, `useImportFailedDetection`) ## Review Focus - Unified `pt` config removes minor style variations between dialogs — intentional design unification ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8834-refactor-Unify-small-modal-dialog-styles-with-showSmallLayoutDialog-3056d73d365081b6963beffc0e5943bf) by [Unito](https://www.unito.io) --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: github-actions <github-actions@github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Extract a shared
showSmallLayoutDialogutility and move dialog-specific logic into composables, unifying the duplicatedptconfigurations across small modal dialogs.Changes
showSmallLayoutDialog: Added todialogService.tswith a single unifiedptconfig for all small modal dialogs (missing nodes, missing models, import failed, node conflict)dialogServiceinto dedicated composables following theuseSettingsDialog/useModelSelectorDialogpattern:useMissingNodesDialoguseMissingModelsDialoguseImportFailedNodeDialoguseNodeConflictDialogshow(),hide(), and aDIALOG_KEYconstantapp.ts,useHelpCenter,PackEnableToggle,PackInstallButton,useImportFailedDetection)Review Focus
ptconfig removes minor style variations between dialogs — intentional design unification┆Issue is synchronized with this Notion page by Unito