[refactor] Migrate SettingDialog to BaseModalLayout design system#8270
Merged
viva-jinyi merged 24 commits intomainfrom Feb 12, 2026
Merged
[refactor] Migrate SettingDialog to BaseModalLayout design system#8270viva-jinyi merged 24 commits intomainfrom
viva-jinyi merged 24 commits intomainfrom
Conversation
dosubot
bot
added
the
size:XXL
|
Playwright: ✅ 520 passed, 0 failed · 3 flaky 📊 Browser Reports
|
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 02/12/2026, 07:20:51 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
Contributor
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughReplaces legacy settings UI with a new SettingsDialog component and a useSettingsDialog composable, removes PanelTemplate and legacy dialog exports, updates many call sites/tests to use the new composable and data-testid dialog selector, and migrates PrimeVue tabs to Rekau tab primitives. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller Component
participant UseSettings as useSettingsDialog
participant DialogService as Dialog Store/Service
participant SettingDialog as SettingDialog Component
Caller->>UseSettings: show(panel?, settingId?)
UseSettings->>DialogService: showLayoutDialog(DIALOG_KEY, SettingDialog, { defaultPanel: panel, scrollToSettingId: settingId, onClose })
DialogService->>SettingDialog: mount with props
SettingDialog->>SettingDialog: render nav, search, content
SettingDialog-->>Caller: user triggers onClose / interactions
Caller->>UseSettings: hide()
UseSettings->>DialogService: hideDialog(DIALOG_KEY)
DialogService->>SettingDialog: unmount
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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.7 kB (baseline 22.1 kB) • 🟢 -377 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 874 kB (baseline 874 kB) • 🟢 -502 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 68.9 kB (baseline 68.8 kB) • 🔴 +160 BTop-level views, pages, and routed surfaces
Status: 11 added / 11 removed Panels & Settings — 427 kB (baseline 457 kB) • 🟢 -30.2 kBConfiguration panels, inspectors, and settings screens
Status: 22 added / 23 removed User & Accounts — 16.1 kB (baseline 16 kB) • 🔴 +68 BAuthentication, profile, and account management bundles
Status: 7 added / 7 removed Editors & Dialogs — 785 B (baseline 751 B) • 🔴 +34 BModals, dialogs, drawers, and in-app editors
Status: 1 added / 1 removed UI Components — 36.6 kB (baseline 36.5 kB) • 🔴 +81 BReusable component library chunks
Status: 10 added / 10 removed Data & Services — 2.15 MB (baseline 2.11 MB) • 🔴 +46.7 kBStores, services, APIs, and repositories
Status: 14 added / 14 removed Utilities & Hooks — 237 kB (baseline 238 kB) • 🟢 -1.25 kBHelpers, composables, and utility bundles
Status: 19 added / 20 removed Vendor & Third-Party — 8.69 MB (baseline 8.67 MB) • 🔴 +14.6 kBExternal libraries and shared vendor chunks
Status: 6 added / 6 removed Other — 7.27 MB (baseline 7.29 MB) • 🟢 -25.7 kBBundles that do not match a named category
Status: 117 added / 122 removed |
🔧 Auto-fixes AppliedThis PR has been automatically updated to fix linting and formatting issues.
Changes made:
|
viva-jinyi
force-pushed
the
refactor/setting-dialog
branch
from
2121c8a to
c747132
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: 3
🤖 Fix all issues with AI agents
In `@browser_tests/tests/useSettingSearch.spec.ts`:
- Around line 53-55: Replace hardcoded '[data-testid="settings-dialog"]' locator
with the centralized TestIds constant: update the comfyPage.page.locator call
that assigns settingsDialog to use TestIds.dialogs.settings (import TestIds from
browser_tests/fixtures/selectors.ts) or, even better, refactor the test to use
the SettingDialog fixture class
(browser_tests/fixtures/components/SettingDialog.ts) which already encapsulates
the selector and interactions; ensure imports are added and all other
occurrences in this spec use TestIds.dialogs.settings or the SettingDialog
instance instead of the hardcoded string.
In `@src/platform/settings/components/ServerConfigPanel.vue`:
- Around line 27-33: The icon-only clipboard Button inside ServerConfigPanel.vue
lacks an accessible label; update the Button element that invokes
copyCommandLineArgs to include an aria-label (e.g., aria-label="Copy
command-line arguments") so screen readers can announce its purpose, and
optionally add a matching title attribute for hover/tooltips; locate the Button
that has size="icon", variant="muted-textonly" and `@click`="copyCommandLineArgs"
and add the aria-label there.
In `@src/platform/settings/components/SettingDialog.vue`:
- Around line 196-200: Remove the <style> block in SettingDialog.vue and stop
using !important; instead apply the max-width via a Tailwind class (e.g., add
max-w-[1400px] to the BaseModalLayout element or the wrapper rendered by
SettingsDialog) and remove the .settings-dialog.base-widget-layout CSS. For the
aspect-ratio override, inspect BaseModalLayout (the component that sets the base
aspect-ratio) and either fix its default, add/adjust a prop on BaseModalLayout
to control aspect-ratio, or override it with a non-!important Tailwind-friendly
approach (modify the base component rather than using !important in
SettingsDialog).
🧹 Nitpick comments (15)
src/components/dialog/GlobalDialog.vue (1)
87-97: Dead CSS:.settings-dialog-workspacerules are now unreachable.The class
settings-dialog-workspacewas removed from the template binding on line 7, so these style rules will never match any element. They should be removed to avoid confusion.♻️ Remove dead CSS
-/* Workspace mode: wider settings dialog */ -.settings-dialog-workspace { - width: 100%; - max-width: 1440px; - height: 100%; -} - -.settings-dialog-workspace .p-dialog-content { - width: 100%; - height: 100%; - overflow-y: auto; -}src/platform/settings/components/ServerConfigPanel.vue (1)
86-93: Prefer function declarations over function expressions for non-closures.
revertChangesandrestartAppcould be function declarations per project conventions.restartAppdoes capturerestartTriggered, but since it's a module-levellet, a declaration would work too.♻️ Suggested change
-const revertChanges = () => { - serverConfigStore.revertChanges() -} +function revertChanges() { + serverConfigStore.revertChanges() +} -const restartApp = async () => { - restartTriggered = true - await electronAPI().restartApp() -} +async function restartApp() { + restartTriggered = true + await electronAPI().restartApp() +}src/platform/workspace/composables/useWorkspaceUI.ts (1)
162-165: Stale JSDoc: "tab state" reference is no longer accurate.The doc comment still mentions "ensure tab state is shared across components," but tab state was removed in this change.
📝 Suggested doc update
/** * UI configuration composable derived from workspace state. * Controls what UI elements are visible/enabled based on role and workspace type. - * Uses createSharedComposable to ensure tab state is shared across components. + * Uses createSharedComposable to ensure state is shared across components. */src/components/sidebar/ComfyMenuButton.vue (1)
167-169: Type assertion masks a loosedefaultPanelparameter type.
showSettingsacceptsstring | undefinedbutsettingsDialog.show()expects a specific union type. Theas Parameters<...>cast silences the compiler but won't prevent invalid strings at runtime. Consider narrowing the parameter type to match the composable's expected type directly.♻️ Suggested narrowing
-const showSettings = (defaultPanel?: string) => { - settingsDialog.show(defaultPanel as Parameters<typeof settingsDialog.show>[0]) +const showSettings = (defaultPanel?: Parameters<typeof settingsDialog.show>[0]) => { + settingsDialog.show(defaultPanel) }browser_tests/tests/subgraph.spec.ts (1)
823-833: Use the centralizedTestIds.dialogs.settingsconstant instead of hardcoded string.Replace
'[data-testid="settings-dialog"]'with the centralized selector frombrowser_tests/fixtures/selectors.ts. The constant is available asTestIds.dialogs.settings, which should be used to construct the full selector pattern:`[data-testid="${TestIds.dialogs.settings}"]`.src/workbench/extensions/manager/composables/useManagerState.ts (1)
151-151: Consider hoistinguseSettingsDialog()to the composable scope.
useSettingsDialog()is instantiated insideopenManageron every call. Other migration sites (e.g.,useCoreCommands.tsline 77) hoist the instance to the composable's top-level scope. Moving it next touseManagerDialog()at line 23 would be consistent and avoids repeated instantiation.♻️ Suggested change
const managerDialog = useManagerDialog() + const settingsDialog = useSettingsDialog() // ... const openManager = async (options?: { // ... }): Promise<void> => { const state = managerUIState.value - const settingsDialog = useSettingsDialog() const commandStore = useCommandStore()src/stores/billingOperationStore.ts (1)
146-146: Consider hoistinguseSettingsDialog()to the store scope for consistency.
useSettingsDialog()is instantiated inline on eachhandleSuccesscall. Since this runs inside adefineStoresetup (executed once), you could hoist it alongside other composable calls to match the pattern used in other migrated files.♻️ Suggested change
export const useBillingOperationStore = defineStore('billingOperation', () => { const operations = ref<Map<string, BillingOperation>>(new Map()) + const settingsDialog = useSettingsDialog() // ... async function handleSuccess(opId: string) { // ... - useSettingsDialog().show('workspace') + settingsDialog.show('workspace')browser_tests/fixtures/components/SettingDialog.ts (1)
39-45: Prefer a locator-based assertion overwaitForSelector.
this.page.waitForSelector('.about-container')works but is less idiomatic than a retrying Playwright assertion. Consider:Suggested change
async goToAboutPanel() { const aboutButton = this.root.locator('nav [role="button"]', { hasText: 'About' }) await aboutButton.click() - await this.page.waitForSelector('.about-container') + await expect(this.root.locator('.about-container')).toBeVisible() }Note: you'll need to import
expectfrom@playwright/testin this file.As per coding guidelines: "In Playwright tests, do not use waitForTimeout; use Locator actions and retrying assertions instead." While
waitForSelectoris notwaitForTimeout, the same principle of preferring retrying assertions applies.browser_tests/tests/dialog.spec.ts (1)
284-295: Prefer locator-based assertions overwaitForSelector.Lines 284 and 293-295 use
page.waitForSelector(...). Consider usingawait expect(locator).toBeVisible()for consistency with Playwright best practices.Suggested change
- await comfyPage.page.waitForSelector('[data-testid="settings-dialog"]') + const settingsDialog = comfyPage.page.locator('[data-testid="settings-dialog"]') + await expect(settingsDialog).toBeVisible() // Open the keybinding tab - const settingsDialog = comfyPage.page.locator( - '[data-testid="settings-dialog"]' - ) await settingsDialog .locator('nav [role="button"]', { hasText: 'Keybinding' }) .click() - await comfyPage.page.waitForSelector( - '[placeholder="Search Keybindings..."]' - ) + await expect( + comfyPage.page.locator('[placeholder="Search Keybindings..."]') + ).toBeVisible()As per coding guidelines, Playwright tests should prefer retrying assertions over imperative wait methods.
browser_tests/tests/useSettingSearch.spec.ts (1)
118-124: Category selection test may silently pass without asserting.When
categoryCount <= 1, theifblock is skipped entirely and the test passes vacuously. Consider usingtest.skipor an explicit assumption (expect(categoryCount).toBeGreaterThan(1)) to make this visible in test reports rather than silently passing.Also, asserting on a specific Tailwind class (
bg-interface-menu-component-surface-selected) couples the test to styling implementation. Adata-testidoraria-selectedattribute on the active item would be more resilient.src/platform/settings/composables/useSettingUI.ts (1)
352-368: Repeated type assertion fortranslatedLabelis a type smell.The cast
(group as SettingTreeNode & { translatedLabel?: string }).translatedLabelappears twice (Lines 355 and 360) becausetranslateCategoryaugments nodes withtranslatedLabelbut the arrays are typed asSettingTreeNode[], losing that information.Consider defining a local type (e.g.,
TranslatedSettingTreeNode) or adjustingtranslateCategory's return type and the computed array types to avoid the inline assertion.♻️ Suggested approach
+interface TranslatedSettingTreeNode extends SettingTreeNode { + translatedLabel?: string +} + const navGroups = computed<NavGroupData[]>(() => - groupedMenuTreeNodes.value.map((group) => ({ + (groupedMenuTreeNodes.value as TranslatedSettingTreeNode[]).map((group) => ({ title: - (group as SettingTreeNode & { translatedLabel?: string }) - .translatedLabel ?? group.label, - items: (group.children ?? []).map((child) => ({ + group.translatedLabel ?? group.label, + items: ((group.children ?? []) as TranslatedSettingTreeNode[]).map((child) => ({ id: child.key, label: - (child as SettingTreeNode & { translatedLabel?: string }) - .translatedLabel ?? child.label, + child.translatedLabel ?? child.label, icon: CATEGORY_ICONS[child.key] ?? CATEGORY_ICONS[child.label] ?? 'icon-[lucide--plug]' })) })) )src/components/dialog/content/setting/WorkspacePanelContent.vue (1)
58-62: Usecn()utility instead of:class="[]"array syntax.The coding guidelines state: "Use
cn()utility from@/utils/tailwindUtilto merge class names; never use:class="[]"syntax."♻️ Proposed fix
- :class="[ - 'flex items-center gap-2 px-3 py-2', - item.class, - item.disabled ? 'pointer-events-auto' : '' - ]" + :class=" + cn( + 'flex items-center gap-2 px-3 py-2', + item.class, + item.disabled && 'pointer-events-auto' + ) + "You'll also need to import
cn:import { cn } from '@/utils/tailwindUtil'As per coding guidelines: "Use
cn()utility from@/utils/tailwindUtilto merge class names; never use:class="[]"syntax."src/platform/settings/components/SettingDialog.vue (3)
87-87: Avoid new PrimeVue component usage —Tagfromprimevue/tag.This is a new file introducing a PrimeVue
Tagimport. The coding guidelines state "Avoid new usage of PrimeVue components." Consider using an existing design system tag component or a simple styled<span>instead.As per coding guidelines: "Avoid new usage of PrimeVue components."
27-29:v-model:model-valueis redundant — usev-modeldirectly.
v-model:model-value="searchQuery"is equivalent tov-model="searchQuery"sincemodelValueis the defaultv-modelprop.♻️ Simplification
<SearchBox - v-model:model-value="searchQuery" + v-model="searchQuery" size="md"
155-156:Math.max(0, ...spread)on potentially large arrays.
flattenTree<SettingParams>(group).map(...)spreads an entire array intoMath.max. For very large settings trees, this could exceed the call stack limit. In practice, settings categories are small, so this is unlikely to be an issue—butreducewould be safer if the data scale is uncertain.♻️ Safer alternative
-const getGroupSortOrder = (group: SettingTreeNode): number => - Math.max(0, ...flattenTree<SettingParams>(group).map((s) => s.sortOrder ?? 0)) +const getGroupSortOrder = (group: SettingTreeNode): number => + flattenTree<SettingParams>(group).reduce( + (max, s) => Math.max(max, s.sortOrder ?? 0), + 0 + )
Comment on lines
+196
to
+200
| <style> | ||
| .settings-dialog.base-widget-layout { | ||
| max-width: 1400px; | ||
| aspect-ratio: auto !important; | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
!important and <style> block violate coding guidelines.
The guidelines state: "Never use !important" and "Use Tailwind CSS for styling; avoid <style> blocks." The aspect-ratio: auto !important; overrides a base style from BaseModalLayout—consider fixing the source of the conflicting style instead.
For max-width, a Tailwind class can be used directly on the component (e.g., max-w-[1400px]).
♻️ Proposed approach
Remove the <style> block and apply the max-width via Tailwind on the BaseModalLayout:
<BaseModalLayout
content-title=""
data-testid="settings-dialog"
- class="settings-dialog"
+ class="max-w-[1400px]"
>For the aspect-ratio: auto, investigate what BaseModalLayout sets and override it properly (e.g., via a prop or by fixing the base component's default).
As per coding guidelines: "Never use !important or the ! important prefix for Tailwind classes; instead find and correct interfering existing !important classes" and "Use Tailwind CSS for styling in Vue components; avoid <style> blocks."
🤖 Prompt for AI Agents
In `@src/platform/settings/components/SettingDialog.vue` around lines 196 - 200,
Remove the <style> block in SettingDialog.vue and stop using !important; instead
apply the max-width via a Tailwind class (e.g., add max-w-[1400px] to the
BaseModalLayout element or the wrapper rendered by SettingsDialog) and remove
the .settings-dialog.base-widget-layout CSS. For the aspect-ratio override,
inspect BaseModalLayout (the component that sets the base aspect-ratio) and
either fix its default, add/adjust a prop on BaseModalLayout to control
aspect-ratio, or override it with a non-!important Tailwind-friendly approach
(modify the base component rather than using !important in SettingsDialog).
Contributor
There was a problem hiding this comment.
There's got to be a better way
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and examine the BaseModalLayout component
cat src/components/widget/layout/BaseModalLayout.vue
Length of output: 6481
@DrJKL, absolutely! Here's the better way:
The root cause is that BaseModalLayout's scoped style sets aspect-ratio: 20/13 which has higher specificity due to Vue's scoped attribute mechanism. Here are two solutions:
Quick Fix: Use Tailwind on the Root Element
Since a child component's root node will be affected by both the parent's scoped CSS and the child's scoped CSS, you can apply Tailwind classes directly:
<BaseModalLayout
content-title=""
data-testid="settings-dialog"
class="aspect-auto max-w-[1400px]"
>Remove the <style> block entirely. The aspect-auto Tailwind class sets aspect-ratio: auto and should work because it's applied to the root element.
Better Fix: Add Props to BaseModalLayout
Modify BaseModalLayout.vue to accept optional props for controlling dimensions:
<!-- BaseModalLayout.vue -->
<script setup lang="ts">
const {
contentTitle,
rightPanelTitle,
autoAspectRatio = false, // New prop
maxWidth // New optional prop
} = defineProps<{
contentTitle: string
rightPanelTitle?: string
autoAspectRatio?: boolean
maxWidth?: string
}>()
</script>
<style scoped>
.base-widget-layout {
height: 80vh;
width: 90vw;
max-width: 1280px;
}
.base-widget-layout:not(.auto-aspect) {
aspect-ratio: 20/13;
}
`@media` (min-width: 1450px) {
.base-widget-layout {
max-width: 1724px;
}
}
</style>Then use it in SettingDialog:
<BaseModalLayout
content-title=""
data-testid="settings-dialog"
:auto-aspect-ratio="true"
max-width="1400px"
>The prop-based approach is cleaner and makes BaseModalLayout more reusable. Which approach would you prefer?
🧠 Learnings used
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 8256
File: src/components/widget/layout/BaseModalLayout.vue:39-47
Timestamp: 2026-01-24T00:06:25.318Z
Learning: In `src/components/widget/layout/BaseModalLayout.vue`, the left panel toggle button conditions are intentionally different for mobile vs desktop. The "show left panel" button uses `!notMobile && !showLeftPanel` to appear only on mobile devices when the panel is hidden, which is the intended mobile-first design pattern.
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 8256
File: src/components/widget/layout/BaseModalLayout.vue:20-28
Timestamp: 2026-01-24T00:07:58.981Z
Learning: In `src/components/widget/layout/BaseModalLayout.vue`, the left panel toggle buttons are intentionally mobile-only. The hide button (line 20) uses `!notMobile && showLeftPanel` and the show button (line 40) uses `!notMobile && !showLeftPanel`. On desktop, the left panel is persistent and doesn't need toggle controls.
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-03T20:30:57.039Z
Learning: Applies to **/*.{vue,ts,tsx} : Never use `!important` or the `!` important prefix for Tailwind classes; instead find and correct interfering existing `!important` classes
Learnt from: jtydhr88
Repo: Comfy-Org/ComfyUI_frontend PR: 7621
File: src/components/load3d/Load3DScene.vue:4-4
Timestamp: 2025-12-18T20:39:30.137Z
Learning: In src/components/load3d/Load3DScene.vue, the scoped `<style>` block with `!important` declarations for the canvas element is necessary because Three.js dynamically creates the canvas with inline styles, preventing direct application of Tailwind classes. This is a valid exception to the Tailwind-only styling guideline.
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-03T20:30:57.039Z
Learning: Applies to **/*.{vue,ts,tsx} : Never use arbitrary percentage values like `w-[80%]` when a Tailwind fraction utility exists; use `w-4/5` instead of `w-[80%]`, `w-1/2` instead of `w-[50%]`, etc.
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-03T20:30:57.039Z
Learning: Applies to **/*.vue : Use Tailwind 4 for styling in Vue components; avoid `<style>` blocks
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-03T20:30:57.039Z
Learning: Applies to **/*.{vue,ts,tsx} : Never use `dark:` Tailwind variant; use semantic values from `style.css` theme instead (e.g., `bg-node-component-surface`)
Learnt from: benceruleanlu
Repo: Comfy-Org/ComfyUI_frontend PR: 7297
File: src/components/actionbar/ComfyActionbar.vue:33-43
Timestamp: 2025-12-09T21:40:19.792Z
Learning: In the Comfy-Org/ComfyUI_frontend repository, inline Tailwind CSS class strings, even when long, are acceptable and preferred over extracting them to computed properties when the classes are static. This is a common Tailwind pattern and doesn't need to be flagged as a readability issue.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/platform/settings/components/SettingDialog.vue`:
- Around line 17-22: Replace the PrimeVue <Tag> usage in SettingDialog.vue (the
v-if check on isStaging that renders Tag) with a plain element styled using your
design system/Tailwind: remove the import/usage of 'primevue/tag', render a
<span> (or design-system component) when isStaging is true, apply appropriate
Tailwind classes for background, text, padding and rounded corners to mimic a
badge, and use your semantic color tokens (avoid dark: variants) so the badge
matches the app styles.
- Line 144: Add a watch to keep activeCategoryKey in sync with the computed
defaultCategory so it updates when feature flags/settings load asynchronously:
inside the component, call watch on defaultCategory and when newCategory is
truthy and searchQuery.value is empty, set activeCategoryKey.value =
newCategory.key; reference the existing refs/computed names activeCategoryKey,
defaultCategory, and searchQuery so the active category only auto-updates when
the user is not actively searching.
🧹 Nitpick comments (3)
src/platform/settings/components/SettingDialog.vue (3)
28-28:v-model:model-valueis redundant — usev-modeldirectly.
v-model:model-valueis equivalent tov-modelbut reads awkwardly and is non-idiomatic. Vue'sv-modelalready binds tomodelValueby default.♻️ Proposed fix
- <SearchBox - v-model:model-value="searchQuery" + <SearchBox + v-model="searchQuery"
209-222: Watcher onactiveCategoryKeywrites back to itself — potential infinite trigger.Line 211 assigns to
activeCategoryKey.valueinside its own watcher. Vue 3 does guard against infinite recursion for synchronous self-writes, but this is still a code smell that makes the data flow hard to reason about — a reader can't easily tell when the watcher re-fires.Consider moving the "restore previous key" logic into the functions that clear
activeCategoryKey(e.g.,handleSearch, thesearchResultsCategorieswatcher) rather than relying on a self-referencing watcher.Additionally, line 219 uses
querySelectorwith an interpolatednewKey:?.querySelector(`[data-nav-id="${newKey}"]`)If
newKeyever contains quotes or brackets (e.g., from user/extension-provided IDs), this breaks or could cause unexpected behavior. Consider usingquerySelectorwithCSS.escape(newKey)ordocument.querySelectorwith an attribute-value match viaElement.matches.♻️ Safer selector
void nextTick(() => { - navRef.value - ?.querySelector(`[data-nav-id="${newKey}"]`) - ?.scrollIntoView({ block: 'nearest', behavior: 'smooth' }) + const escaped = CSS.escape(newKey) + navRef.value + ?.querySelector(`[data-nav-id="${escaped}"]`) + ?.scrollIntoView({ block: 'nearest', behavior: 'smooth' }) })
146-155: Search-result category matching relies on label comparison — should use key.
searchResultsCategoriesis aSetof category labels (strings), and line 152 matches vianode.label. This has the same fragility concern as the template comparisons — ifsearchResultsCategoriescould use keys instead of labels, the match would be more robust.
|
Updating Playwright Expectations |
github-actions
bot
removed
the
New Browser Test Expectations
DrJKL
reviewed
Feb 10, 2026
- WorkspacePanelContent: use semantic <header>/<button>, simplify member count, replace :class="[]" with cn() - useSettingUI: sort CATEGORY_ICONS keys alphabetically - types.ts: sort SettingPanelType union members alphabetically
viva-jinyi
added
the
New Browser Test Expectations
github-actions
bot
removed
the
New Browser Test Expectations
DrJKL
previously approved these changes
Feb 11, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/components/dialog/content/setting/WorkspacePanelContent.vue`:
- Around line 134-139: The Tailwind class `text-bold` used in the
tabTriggerActive constant is invalid; update the tabTriggerActive string
(alongside tabTriggerBase/tabTriggerInactive references if needed) to replace
`text-bold` with the correct utility `font-bold` so the active tab text renders
bold.
🧹 Nitpick comments (8)
browser_tests/tests/subgraph.spec.ts (1)
829-840: Replace hardcoded selector strings with centralized TestIds.The
[data-testid="settings-dialog"]selector is hardcoded in two places. UseTestIds.dialogs.settingsfrombrowser_tests/fixtures/selectors.tsinstead, following the pattern used across other test files in the project.Add the import:
import { TestIds } from '../fixtures/selectors'Then replace both occurrences with
[data-testid="${TestIds.dialogs.settings}"]or use the locator directly:comfyPage.page.locator([data-testid="${TestIds.dialogs.settings}"]).src/platform/settings/composables/useSettingUI.ts (2)
372-388: Repeated type assertion fortranslatedLabel— consider a local type alias.The
as SettingTreeNode & { translatedLabel?: string }cast appears twice (lines 375 and 380). SincetranslateCategoryaugments nodes withtranslatedLabel, a small type alias would eliminate the inline casts and improve readability.♻️ Suggested improvement
Define at the top of the file (near
SettingPanelItem):type TranslatedSettingTreeNode = SettingTreeNode & { translatedLabel?: string }Then use it in
navGroups:const navGroups = computed<NavGroupData[]>(() => groupedMenuTreeNodes.value.map((group) => ({ title: - (group as SettingTreeNode & { translatedLabel?: string }) - .translatedLabel ?? group.label, + (group as TranslatedSettingTreeNode).translatedLabel ?? group.label, items: (group.children ?? []).map((child) => ({ id: child.key, label: - (child as SettingTreeNode & { translatedLabel?: string }) - .translatedLabel ?? child.label, + (child as TranslatedSettingTreeNode).translatedLabel ?? child.label, icon: CATEGORY_ICONS[child.key] ?? CATEGORY_ICONS[child.label] ?? 'icon-[lucide--plug]' })) })) )Even better, update
translateCategory's return type toTranslatedSettingTreeNodeso the groups are already properly typed and no cast is needed.
20-37: CATEGORY_ICONS keys mix labels and panel keys — consider documenting the convention.The map uses a mix of display labels (
'Appearance','Comfy','3D') and panel keys ('about','credits','server-config'). ThenavGroupscomputed correctly falls back fromchild.keytochild.label, but this dual-lookup convention is implicit. A brief inline note would help future contributors understand why both forms are present.src/platform/settings/components/SettingDialog.vue (3)
10-11: Simplifyv-model:model-valuetov-model.
v-model:model-valueis equivalent tov-modelin Vue 3. The explicit form is redundant here.♻️ Proposed fix
<SearchBox - v-model:model-value="searchQuery" + v-model="searchQuery" size="md"
146-147: Use a function declaration instead of a function expression.Per coding guidelines, prefer function declarations over function expressions when possible.
♻️ Proposed fix
-const getGroupSortOrder = (group: SettingTreeNode): number => - Math.max(0, ...flattenTree<SettingParams>(group).map((s) => s.sortOrder ?? 0)) +function getGroupSortOrder(group: SettingTreeNode): number { + return Math.max(0, ...flattenTree<SettingParams>(group).map((s) => s.sortOrder ?? 0)) +}As per coding guidelines: "Do not use function expressions if it's possible to use function declarations instead."
185-199: Side-effect–heavy watcher mixes unrelated concerns.This single watcher handles three distinct responsibilities: preventing null resets, fetching balance for credits, and scrolling the nav item into view. Consider splitting into focused watchers for clarity, though this is a minor readability nit.
src/components/dialog/content/setting/WorkspacePanelContent.vue (2)
141-143: Consider narrowingdefaultTabto a union type.
defaultTabaccepts anystring, but only'plan'and'members'are valid tab values. A literal union would catch typos at compile time and improve discoverability.Proposed fix
-const { defaultTab = 'plan' } = defineProps<{ - defaultTab?: string -}>() +type WorkspaceTab = 'plan' | 'members' + +const { defaultTab = 'plan' } = defineProps<{ + defaultTab?: WorkspaceTab +}>()
117-117: PrimeVueMenuretained — consider migrating in a follow-up.The
Menuimport fromprimevue/menuis pre-existing usage being preserved during this migration, which is reasonable for scope. A follow-up to replace it with a Reka UI equivalent (e.g.,DropdownMenu) would complete the PrimeVue removal for this component. Based on learnings: "Avoid new usage of PrimeVue components."
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
DrJKL
previously approved these changes
Feb 12, 2026
viva-jinyi
added
the
New Browser Test Expectations
github-actions
bot
removed
the
New Browser Test Expectations
DrJKL
previously approved these changes
Feb 12, 2026
viva-jinyi
added
the
New Browser Test Expectations
github-actions
bot
removed
the
New Browser Test Expectations
DrJKL
approved these changes
Feb 12, 2026
viva-jinyi
added a commit
that referenced
this pull request
Feb 15, 2026
## Summary Restores the scroll-to-setting and highlight animation that was lost during the BaseModalLayout migration in #8270. Originally implemented in #8761. ## Changes - **What**: Re-added scroll-into-view + pulse highlight logic and CSS animation to `SettingDialog.vue`, ported from the deleted `SettingDialogContent.vue` Fixes #3437 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8833-bugfix-Restore-scroll-to-setting-in-SettingDialog-3056d73d36508161abeee047a40dc1e5) by [Unito](https://www.unito.io)
2 tasks
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
3 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
Replace PrimeVue-based SettingDialog with BaseModalLayout + custom left panel, extract
useSettingsDialogcomposable, and migrate workspace tabs to design system components.Changes
BaseModalLayoutwith custom left panel (NavTitle,NavItem), search box, and workspace image supportdialogService.tsfollowinguseManagerDialogpattern; updated all 10 callersTabs/TabList/Tab/TabPanel/TabPanelswith design systemTabList/TabSettingDialogContent.vue,SettingDialogHeader.vue,PanelTemplate.vue,WorkspaceSidebarItem.vueTabPanel/PanelTemplatewrappers from all setting panels (About, Keybinding, Extension, ServerConfig, User, Workspace, Credits, Subscription)UX Improvements
aria-labelto clipboard copy button in ServerConfigPanelReview Focus
useSettingsDialogcomposable replacesdialogService.showSettingsDialog/showAboutDialogscreen-capture (3).webm
screen-capture (2).webm