[feat] Add Storybook stories for Error Tab missing assets UX#8988
Closed
[feat] Add Storybook stories for Error Tab missing assets UX#8988
Conversation
…erlay - Remove TabError.vue; consolidate all error display into TabErrors.vue and remove the separate 'error' tab type from rightSidePanelStore - Single-node selection mode: regroup errors by message instead of class_type and render ErrorNodeCard in compact mode (hiding redundant header/message) - Container node support: detect internal errors in subgraph/group nodes by matching execution ID prefixes against selected container node IDs - SectionWidgets: show error badge and 'See Error' button for subgraph/group nodes that contain child-node errors, navigating directly to the errors tab - Add ErrorOverlay component: floating card after execution failure showing a deduplicated error summary with 'Dismiss' and 'See Errors' actions; 'See Errors' deselects all nodes and opens Errors tab in overview mode - Add isErrorOverlayOpen, showErrorOverlay, dismissErrorOverlay to executionStore; reset overlay state on execution_start
- Add allErrorExecutionIds computed to executionStore to centralize error ID collection from lastNodeErrors and lastExecutionError - Add hasInternalErrorForNode helper to executionStore to encapsulate prefix-based container error detection - Replace duplicated error ID collection and prefix checks in RightSidePanel and SectionWidgets with store computed/helper - Split errorGroups into allErrorGroups (unfiltered) and tabErrorGroups (selection-filtered) in useErrorGroups - Add filterBySelection param (default: false) to addNodeErrorToGroup, processNodeErrors, processExecutionError - Add groupedErrorMessages computed derived from allErrorGroups for deduped message list used by ErrorOverlay - Migrate ErrorOverlay business logic to useErrorGroups composable, removing inline groupedErrors computed and redundant totalErrorCount wrapper
…or navigation & format
…ed by reset
- Replace :class='cn('...')' with static class and remove unused cn import
in ErrorOverlay
- Fix prompt-only cloud validation errors being wiped by resetExecutionState
by capturing the error before reset and applying it after
- Replace isEmpty(node_id) with explicit presence check to prevent numeric node IDs from being misclassified as service-level errors - Guard exception_type in message template to avoid 'null: ...' output when exception_type is absent - Remove unused isEmpty import from es-toolkit/compat
…ErrorExecutionIds - Add lastNodeErrors.value = null in handleExecutionStart to prevent stale node errors from persisting into the next execution - Guard against null/undefined node_id before pushing to allErrorExecutionIds
This commit adds 7 comprehensive Storybook stories for the error tab to collect design feedback.
Contributor
📝 WalkthroughWalkthroughThis PR implements a comprehensive error overlay system that adds a floating error display panel, refactors error grouping and navigation logic, introduces enhanced error state management, and includes multiple Storybook story components for testing error workflows. Configuration updates exempt Storybook stories from i18n validation. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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 |
|
Playwright: ✅ 510 passed, 0 failed · 8 flaky 📊 Browser Reports
|
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 02/19/2026, 04:48:31 PM UTC 🔗 Links🎉 Your Storybook is ready for review! |
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 21.7 kB (baseline 21.7 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 921 kB (baseline 914 kB) • 🔴 +7.31 kBGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 69 kB (baseline 69 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 9 added / 9 removed Panels & Settings — 430 kB (baseline 430 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 10 added / 10 removed User & Accounts — 16.1 kB (baseline 16.1 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 5 added / 5 removed Editors & Dialogs — 785 B (baseline 785 B) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 1 added / 1 removed UI Components — 42.5 kB (baseline 42.5 kB) • ⚪ 0 BReusable component library chunks
Status: 5 added / 5 removed Data & Services — 2.17 MB (baseline 2.17 MB) • 🔴 +3.12 kBStores, services, APIs, and repositories
Status: 13 added / 13 removed Utilities & Hooks — 237 kB (baseline 237 kB) • 🔴 +1 BHelpers, composables, and utility bundles
Status: 15 added / 15 removed Vendor & Third-Party — 8.69 MB (baseline 8.69 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 7.45 MB (baseline 7.45 MB) • 🔴 +302 BBundles that do not match a named category
Status: 57 added / 57 removed |
![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: 8
🧹 Nitpick comments (6)
src/components/rightSidePanel/errors/__stories__/MockOSSMissingModel.vue (2)
397-405: Avoid scoped<style>; use Tailwind utilities or shared classes.Consider moving the no-scrollbar styling to a Tailwind utility or a shared/global class so the component stays style-block free.
As per coding guidelines: "Use Tailwind 4 for styling in Vue components; avoid
<style>blocks".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/rightSidePanel/errors/__stories__/MockOSSMissingModel.vue` around lines 397 - 405, The component contains a scoped <style> block defining .no-scrollbar; remove that style block from MockOSSMissingModel.vue and replace usages of the .no-scrollbar class with a global/shared solution: either add the scrollbar CSS (the existing rules for .no-scrollbar and ::-webkit-scrollbar) into the global stylesheet/Tailwind entry (e.g., app CSS or a Tailwind layer) or create a Tailwind utility/class that applies those properties, then update the component to use that global/shared class so the Vue file has no <style> block and styling follows the Tailwind/global guidelines.
252-257: Replace:classarray bindings withcn().The chevron class array (and similar bindings) should use
cn()for tailwind-merge behavior and guideline consistency.As per coding guidelines: "Use cn() utility from '@/utils/tailwindUtil' to merge class names; never use :class=\"[]\" syntax."♻️ Example refactor
+import { cn } from '@/utils/tailwindUtil' @@ - :class="[ - category !== 'VAE' ? 'group-hover:text-white' : '', - collapsedCategories[category] ? '-rotate-180' : '' - ]" + :class=" + cn( + category !== 'VAE' && 'group-hover:text-white', + collapsedCategories[category] && '-rotate-180' + ) + "🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/rightSidePanel/errors/__stories__/MockOSSMissingModel.vue` around lines 252 - 257, Replace the array-style Vue class binding on the chevron icon with the cn() utility: import cn from '@/utils/tailwindUtil' and change the :class="[...]" usage on the <i> element (and any similar :class="[]" bindings in this file) to use :class="cn(...)" so conditional classes are passed as arguments to cn (e.g., category !== 'VAE' && 'group-hover:text-white', collapsedCategories[category] && '-rotate-180') alongside the static classes; ensure you keep the existing static classes (like "icon-[lucide--chevron-up] size-4 text-[`#8a8a8a`] transition-all") outside or inside cn as appropriate.src/components/rightSidePanel/errors/__stories__/MockManagerDialog.vue (2)
130-136: Usecn()instead of:class="[]"arrays.The dynamic class array should be merged with
cn()to match the repo rule.Proposed fix
- :class="[ - 'rounded-xl border p-4 flex flex-col gap-3 cursor-pointer transition-colors', - i === 0 && selectedPack - ? 'border-[`#0b8ce9`]/70 ring-1 ring-[`#0b8ce9`]/40' - : 'border-[`#494a50`] hover:border-[`#55565e`]' - ]" + :class="cn( + 'rounded-xl border p-4 flex flex-col gap-3 cursor-pointer transition-colors', + i === 0 && selectedPack + ? 'border-[`#0b8ce9`]/70 ring-1 ring-[`#0b8ce9`]/40' + : 'border-[`#494a50`] hover:border-[`#55565e`]' + )"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.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/rightSidePanel/errors/__stories__/MockManagerDialog.vue` around lines 130 - 136, Replace the array-style dynamic class binding with the cn() utility and add the import; specifically, in MockManagerDialog.vue stop using :class="[]" and instead call cn(...) to merge the base classes with the conditional ones (preserving the same strings and condition i === 0 && selectedPack ? 'border-[`#0b8ce9`]/70 ring-1 ring-[`#0b8ce9`]/40' : 'border-[`#494a50`] hover:border-[`#55565e`]'), and add the import statement import { cn } from '@/utils/tailwindUtil' in the script block so the template uses cn to produce the same combined class string.
160-173: Replace raw<button>elements with the shared Button component.Use the design-system
Buttonfor consistency (icon-only buttons can usevariant="textonly"+ proper aria-labels).Based on learnings: In the ComfyUI_frontend Vue codebase, replace raw HTML elements with the shared Button component located at src/components/ui/button/Button.vue.Proposed fix
- <button - class="flex items-center justify-center text-[`#8a8a8a`] hover:text-white p-1" - style="background:none;border:none;outline:none;cursor:pointer;" - > - <i class="icon-[lucide--panel-right] size-4" /> - </button> + <Button + variant="textonly" + size="icon" + class="text-[`#8a8a8a`] hover:text-white" + aria-label="Collapse panel" + > + <i class="icon-[lucide--panel-right] size-4" /> + </Button> <!-- Close X Icon --> - <button - class="flex items-center justify-center text-[`#8a8a8a`] hover:text-white p-1" - style="background:none;border:none;outline:none;cursor:pointer;" - `@click`="emit('close')" - > - <i class="icon-[lucide--x] size-4" /> - </button> + <Button + variant="textonly" + size="icon" + class="text-[`#8a8a8a`] hover:text-white" + aria-label="Close" + `@click`="emit('close')" + > + <i class="icon-[lucide--x] size-4" /> + </Button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/rightSidePanel/errors/__stories__/MockManagerDialog.vue` around lines 160 - 173, The two raw <button> elements (the panel-right icon button and the Close X button that calls emit('close')) should be replaced with the shared Button component (src/components/ui/button/Button.vue) using variant="textonly" and appropriate aria-labels; for the close button preserve the `@click`="emit('close')" binding by moving it to the Button and ensure the icon markup (i.e., <i class="icon-[lucide--panel-right] ..."> and <i class="icon-[lucide--x] ...">) is placed inside the Button slot or its default content so styling remains consistent.src/components/rightSidePanel/errors/__stories__/MockCloudMissingModelBasic.vue (1)
49-54: Interval/timeout timers are never cleaned up on unmount.When navigating between Storybook stories, the component unmounts but
setInterval(download timers) andsetTimeout(input reveal timers) keep firing, causing stale updates and potential errors. AddonUnmountedcleanup:Proposed fix
-import { ref, watch, computed } from 'vue' +import { ref, watch, computed, onUnmounted } from 'vue'Then at the end of the script setup:
onUnmounted(() => { for (const id in downloadTimers.value) { clearInterval(downloadTimers.value[id]) } for (const id in inputTimeouts.value) { clearTimeout(inputTimeouts.value[id]) } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/rightSidePanel/errors/__stories__/MockCloudMissingModelBasic.vue` around lines 49 - 54, The component creates timers in inputTimeouts (setTimeout) and downloadTimers (setInterval) but never clears them on unmount; add an onUnmounted hook that iterates over downloadTimers.value and calls clearInterval for each stored interval and over inputTimeouts.value and calls clearTimeout for each stored timeout to prevent stale callbacks and errors (refer to the inputTimeouts and downloadTimers refs and the setInterval/setTimeout usage to locate where to clear them).src/components/rightSidePanel/errors/__stories__/MockOSSMissingNodePack.vue (1)
74-104: StalesetTimeouttimers on unmount.
onInstallandonInstallAllschedule timeouts that are never tracked or cleared on component unmount. When switching stories in Storybook, these fire against stale reactive state. Consider storing timer IDs and clearing them inonUnmounted, similar to the pattern already used byresetAll.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/rightSidePanel/errors/__stories__/MockOSSMissingNodePack.vue` around lines 74 - 104, The onInstall and onInstallAll functions schedule setTimeouts that are not tracked or cleared on unmount; add a timers collection (e.g., an array of timer IDs) and push each setTimeout return value from onInstall and onInstallAll into it, ensure resetAll also clears and empties this collection, and register an onUnmounted hook that iterates the timers, calls clearTimeout on each ID, and empties the collection so installStates/hasSuccessfulInstall/MOCK_MISSING_PACKS-related timeouts cannot run after unmount.
🤖 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/components/rightSidePanel/errors/__stories__/MockCloudMissingModelBasic.vue`:
- Line 305: The TransitionGroup child uses v-show so leave hooks never fire;
change the rendering logic in the loop (the element with v-for="model in models"
and :key="model.id") to use v-if="!removedModels[model.id]" instead of v-show,
or alternatively filter the models in a computed (e.g., computedFilteredModels)
and iterate over that list so the key is removed from the DOM and
TransitionGroup/@leave transitions on the surrounding <TransitionGroup> will run
correctly.
In `@src/components/rightSidePanel/errors/__stories__/MockManagerDialog.vue`:
- Around line 12-18: The props block currently uses withDefaults and the props
object (withDefaults and props); replace this with typed destructuring from
defineProps — e.g., use defineProps<...>() and destructure selectedPack with a
default (selectedPack = null) instead of withDefaults, and update all
template/script references from props.selectedPack to selectedPack (search for
usages of props and withDefaults and update references accordingly).
In
`@src/components/rightSidePanel/errors/__stories__/StoryOSSMissingNodePackFlow.vue`:
- Around line 36-63: The template in StoryOSSMissingNodePackFlow.vue contains
hardcoded user-facing strings ("ComfyUI Canvas" and "Click the
buttons…"/statusLog fallback) that must be localized; replace these literals
with vue-i18n translation lookups (e.g. $t('...')) in the template where the
text nodes appear (the element showing "ComfyUI Canvas" and the v-else fallback
that says "Click the buttons in the right-side error tab" and any statusLog
default label), add corresponding keys to src/locales/en/main.json (e.g.
comfyCanvas, clickButtonsRightErrorTab, statusLogDefault or similar), and ensure
the story registers or provides the vue-i18n instance so $t is available in this
component during story rendering.
- Around line 15-33: Replace the hard-coded user-visible strings in functions
log, openManager, closeManager, and onLocate with vue-i18n keys: import/use the
i18n instance (e.g., useI18n or this.$t) and call t('...') instead of literal
strings; add corresponding keys (e.g., rightSidePanel.errors.openManager,
rightSidePanel.errors.managerClosed, rightSidePanel.errors.locatingOnCanvas and
any status fallback) to src/locales/en/main.json and use those keys when calling
log (e.g., log(t('rightSidePanel.errors.openManager', { name:
pack.displayName.split('//')[0].trim() }))). Ensure interpolation for
pack.displayName is used where needed and remove the original English literals.
In `@src/components/rightSidePanel/errors/TabErrors.stories.ts`:
- Around line 82-92: The Story Cloud_MissingNodePacks currently listens for a
non-existent `@log` event on MockCloudMissingNodePack; since
MockCloudMissingNodePack defines defineEmits<{ 'locate': [pack: MissingNodePack]
}>(), replace the dead `@log` listener with `@locate` (e.g., `@locate`="msg =>
console.log('Locate:', msg)") or simply remove the handler if you don't need to
react to the emit; update the template in the Cloud_MissingNodePacks render to
reference MockCloudMissingNodePack and its 'locate' emit accordingly.
In `@src/components/rightSidePanel/errors/useErrorGroups.ts`:
- Around line 41-66: The resolveNodeInfo function uses Number(parentId) when
calling app.rootGraph.getNodeById, which breaks for non-numeric IDs; update
resolveNodeInfo to pass the string parentId directly to
app.rootGraph.getNodeById (or the graph API's expected string id variant)
instead of coercing to Number, and ensure isParentGroupNode detection
(isGroupNode) and title resolution (parentNode?.title) use that string-based
lookup; reference resolveNodeInfo, parseNodeExecutionId,
app.rootGraph.getNodeById, isGroupNode, and resolveNodeDisplayName when making
the change.
In `@src/locales/en/main.json`:
- Around line 1364-1367: There is a duplicate localization key "Error System" in
settingsCategories (settingsCategories.Error System); remove the redundant entry
so only one "Error System": "Error System" mapping remains in
src/locales/en/main.json, ensuring translators see a single canonical key and no
earlier duplicate is left behind.
In `@src/stores/executionStore.ts`:
- Around line 445-459: The service-level error message can include
"undefined"/"null" because exception_message is interpolated directly; in
handleServiceLevelError update the construction of lastPromptError.value.message
to guard against missing exception_message (e.g., use a conditional that appends
": " + exception_message only when exception_message is
non-null/non-undefined/non-empty, otherwise fall back to exception_type or an
empty string), ensuring you still set type and details as currently done; keep
calls to clearInitializationByPromptId and resetExecutionState unchanged.
---
Duplicate comments:
In `@src/components/rightSidePanel/errors/__stories__/MockCloudMissingModel.vue`:
- Around line 61-66: Add an onUnmounted cleanup that iterates over both
inputTimeouts and downloadTimers to clear each stored timer (clearTimeout for
inputTimeouts entries and clearInterval for downloadTimers entries) and then
reset those refs to empty objects; locate the refs inputTimeouts and
downloadTimers in MockCloudMissingModel.vue and implement the cleanup using
Vue's onUnmounted() so timers are cleared when the component unmounts.
- Line 338: In MockCloudMissingModel.vue the leave transitions don’t run because
each item is hidden with v-show instead of being removed from the list; replace
the v-for over "models" that uses v-show="!removedModels[model.id]" with a v-for
over a computed/derived list (e.g., "visibleModels" computed as models.filter(m
=> !removedModels[m.id])) and remove the v-show from the template so the
<TransitionGroup> sees items actually removed; keep the same :key="model.id" and
adjust any references to "models" in that block to use the new computed
"visibleModels".
---
Nitpick comments:
In
`@src/components/rightSidePanel/errors/__stories__/MockCloudMissingModelBasic.vue`:
- Around line 49-54: The component creates timers in inputTimeouts (setTimeout)
and downloadTimers (setInterval) but never clears them on unmount; add an
onUnmounted hook that iterates over downloadTimers.value and calls clearInterval
for each stored interval and over inputTimeouts.value and calls clearTimeout for
each stored timeout to prevent stale callbacks and errors (refer to the
inputTimeouts and downloadTimers refs and the setInterval/setTimeout usage to
locate where to clear them).
In `@src/components/rightSidePanel/errors/__stories__/MockManagerDialog.vue`:
- Around line 130-136: Replace the array-style dynamic class binding with the
cn() utility and add the import; specifically, in MockManagerDialog.vue stop
using :class="[]" and instead call cn(...) to merge the base classes with the
conditional ones (preserving the same strings and condition i === 0 &&
selectedPack ? 'border-[`#0b8ce9`]/70 ring-1 ring-[`#0b8ce9`]/40' :
'border-[`#494a50`] hover:border-[`#55565e`]'), and add the import statement import
{ cn } from '@/utils/tailwindUtil' in the script block so the template uses cn
to produce the same combined class string.
- Around line 160-173: The two raw <button> elements (the panel-right icon
button and the Close X button that calls emit('close')) should be replaced with
the shared Button component (src/components/ui/button/Button.vue) using
variant="textonly" and appropriate aria-labels; for the close button preserve
the `@click`="emit('close')" binding by moving it to the Button and ensure the
icon markup (i.e., <i class="icon-[lucide--panel-right] ..."> and <i
class="icon-[lucide--x] ...">) is placed inside the Button slot or its default
content so styling remains consistent.
In `@src/components/rightSidePanel/errors/__stories__/MockOSSMissingModel.vue`:
- Around line 397-405: The component contains a scoped <style> block defining
.no-scrollbar; remove that style block from MockOSSMissingModel.vue and replace
usages of the .no-scrollbar class with a global/shared solution: either add the
scrollbar CSS (the existing rules for .no-scrollbar and ::-webkit-scrollbar)
into the global stylesheet/Tailwind entry (e.g., app CSS or a Tailwind layer) or
create a Tailwind utility/class that applies those properties, then update the
component to use that global/shared class so the Vue file has no <style> block
and styling follows the Tailwind/global guidelines.
- Around line 252-257: Replace the array-style Vue class binding on the chevron
icon with the cn() utility: import cn from '@/utils/tailwindUtil' and change the
:class="[...]" usage on the <i> element (and any similar :class="[]" bindings in
this file) to use :class="cn(...)" so conditional classes are passed as
arguments to cn (e.g., category !== 'VAE' && 'group-hover:text-white',
collapsedCategories[category] && '-rotate-180') alongside the static classes;
ensure you keep the existing static classes (like "icon-[lucide--chevron-up]
size-4 text-[`#8a8a8a`] transition-all") outside or inside cn as appropriate.
In `@src/components/rightSidePanel/errors/__stories__/MockOSSMissingNodePack.vue`:
- Around line 74-104: The onInstall and onInstallAll functions schedule
setTimeouts that are not tracked or cleared on unmount; add a timers collection
(e.g., an array of timer IDs) and push each setTimeout return value from
onInstall and onInstallAll into it, ensure resetAll also clears and empties this
collection, and register an onUnmounted hook that iterates the timers, calls
clearTimeout on each ID, and empties the collection so
installStates/hasSuccessfulInstall/MOCK_MISSING_PACKS-related timeouts cannot
run after unmount.
| <Transition :css="false" @enter="enterTransition" @leave="leaveTransition"> | ||
| <div v-if="!collapsedCategories[category]" class="pt-2"> | ||
| <TransitionGroup :css="false" @enter="enterTransition" @leave="leaveTransition"> | ||
| <div v-for="model in models" v-show="!removedModels[model.id]" :key="model.id" class="flex flex-col w-full mb-6 last:mb-4"> |
Contributor
There was a problem hiding this comment.
v-show inside <TransitionGroup> won't trigger leave transitions.
TransitionGroup hooks (@leave) only fire when an element is removed from the DOM via v-if (or when the key disappears from the list). v-show just toggles display: none, so leaveTransition will never be called — models will vanish instantly instead of animating out.
Switch to v-if or filter the list in a computed to exclude removed models:
Proposed fix
- <TransitionGroup :css="false" `@enter`="enterTransition" `@leave`="leaveTransition">
- <div v-for="model in models" v-show="!removedModels[model.id]" :key="model.id" class="flex flex-col w-full mb-6 last:mb-4">
+ <TransitionGroup :css="false" `@enter`="enterTransition" `@leave`="leaveTransition">
+ <div v-for="model in models.filter(m => !removedModels[m.id])" :key="model.id" class="flex flex-col w-full mb-6 last:mb-4">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <div v-for="model in models" v-show="!removedModels[model.id]" :key="model.id" class="flex flex-col w-full mb-6 last:mb-4"> | |
| <div v-for="model in models.filter(m => !removedModels[m.id])" :key="model.id" class="flex flex-col w-full mb-6 last:mb-4"> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/components/rightSidePanel/errors/__stories__/MockCloudMissingModelBasic.vue`
at line 305, The TransitionGroup child uses v-show so leave hooks never fire;
change the rendering logic in the loop (the element with v-for="model in models"
and :key="model.id") to use v-if="!removedModels[model.id]" instead of v-show,
or alternatively filter the models in a computed (e.g., computedFilteredModels)
and iterate over that list so the key is removed from the DOM and
TransitionGroup/@leave transitions on the surrounding <TransitionGroup> will run
correctly.
Comment on lines
+12
to
+18
| // Props / Emits | ||
| const props = withDefaults( | ||
| defineProps<{ | ||
| selectedPack?: MissingNodePack | null | ||
| }>(), | ||
| { selectedPack: null } | ||
| ) |
Contributor
There was a problem hiding this comment.
Avoid withDefaults for props; use typed destructuring with defaults.
Repo standard prefers const { prop = default } = defineProps<...>() instead of withDefaults. Update template references from props.selectedPack to selectedPack accordingly.
Proposed fix
-const props = withDefaults(
- defineProps<{
- selectedPack?: MissingNodePack | null
- }>(),
- { selectedPack: null }
-)
+const { selectedPack = null } = defineProps<{
+ selectedPack?: MissingNodePack | null
+}>()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Props / Emits | |
| const props = withDefaults( | |
| defineProps<{ | |
| selectedPack?: MissingNodePack | null | |
| }>(), | |
| { selectedPack: null } | |
| ) | |
| // Props / Emits | |
| const { selectedPack = null } = defineProps<{ | |
| selectedPack?: MissingNodePack | null | |
| }>() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/rightSidePanel/errors/__stories__/MockManagerDialog.vue`
around lines 12 - 18, The props block currently uses withDefaults and the props
object (withDefaults and props); replace this with typed destructuring from
defineProps — e.g., use defineProps<...>() and destructure selectedPack with a
default (selectedPack = null) instead of withDefaults, and update all
template/script references from props.selectedPack to selectedPack (search for
usages of props and withDefaults and update references accordingly).
Comment on lines
+15
to
+33
| function log(msg: string) { | ||
| statusLog.value = msg | ||
| } | ||
|
|
||
| function openManager(pack: MissingNodePack) { | ||
| selectedPack.value = pack | ||
| isManagerOpen.value = true | ||
| log(`ⓘ Opening Manager: "${pack.displayName.split('//')[0].trim()}"`) | ||
| } | ||
|
|
||
| function closeManager() { | ||
| isManagerOpen.value = false | ||
| selectedPack.value = null | ||
| log('Manager closed') | ||
| } | ||
|
|
||
| function onLocate(pack: MissingNodePack) { | ||
| log(`◎ Locating on canvas: "${pack.displayName.split('//')[0].trim()}"`) | ||
| } |
Contributor
There was a problem hiding this comment.
Localize log/status strings.
The log messages are user-visible in the mock UI; move these strings into i18n keys to match the repo standard. As per coding guidelines: Use vue-i18n for ALL user-facing strings, configured in src/locales/en/main.json.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/components/rightSidePanel/errors/__stories__/StoryOSSMissingNodePackFlow.vue`
around lines 15 - 33, Replace the hard-coded user-visible strings in functions
log, openManager, closeManager, and onLocate with vue-i18n keys: import/use the
i18n instance (e.g., useI18n or this.$t) and call t('...') instead of literal
strings; add corresponding keys (e.g., rightSidePanel.errors.openManager,
rightSidePanel.errors.managerClosed, rightSidePanel.errors.locatingOnCanvas and
any status fallback) to src/locales/en/main.json and use those keys when calling
log (e.g., log(t('rightSidePanel.errors.openManager', { name:
pack.displayName.split('//')[0].trim() }))). Ensure interpolation for
pack.displayName is used where needed and remove the original English literals.
Comment on lines
+36
to
+63
| <template> | ||
| <!-- ComfyUI layout simulation: canvas + right side panel + manager overlay --> | ||
| <div class="relative w-full h-full flex overflow-hidden bg-[#0d0e10]"> | ||
|
|
||
| <!-- Canvas area --> | ||
| <div class="flex-1 min-w-0 relative flex flex-col items-center justify-center gap-4 overflow-hidden"> | ||
| <!-- Grid background --> | ||
| <div | ||
| class="absolute inset-0 opacity-15" | ||
| style="background-image: repeating-linear-gradient(#444 0 1px, transparent 1px 100%), repeating-linear-gradient(90deg, #444 0 1px, transparent 1px 100%); background-size: 32px 32px;" | ||
| /> | ||
| <div class="relative z-10 flex flex-col items-center gap-4"> | ||
| <div class="text-[#8a8a8a]/30 text-sm select-none">ComfyUI Canvas</div> | ||
| <div class="flex gap-5 flex-wrap justify-center px-8"> | ||
| <div v-for="i in 4" :key="i" class="w-[160px] h-[80px] rounded-lg border border-[#3a3b3d] bg-[#1a1b1d]/80 flex flex-col p-3 gap-2"> | ||
| <div class="h-3 w-24 rounded bg-[#2a2b2d]" /> | ||
| <div class="h-2 w-16 rounded bg-[#2a2b2d]" /> | ||
| <div class="h-2 w-20 rounded bg-[#2a2b2d]" /> | ||
| </div> | ||
| </div> | ||
| <div class="flex items-center justify-center min-h-[36px]"> | ||
| <div | ||
| v-if="statusLog" | ||
| class="px-4 py-1.5 rounded-lg text-xs text-center bg-blue-950/70 border border-blue-500/40 text-blue-300" | ||
| >{{ statusLog }}</div> | ||
| <div v-else class="px-4 py-1.5 text-xs text-[#8a8a8a]/30 border border-dashed border-[#2a2b2d] rounded-lg"> | ||
| Click the buttons in the right-side error tab | ||
| </div> |
Contributor
There was a problem hiding this comment.
Localize visible UI text in the story template.
Text like “ComfyUI Canvas” and “Click the buttons…” should be sourced via i18n even in story mocks. As per coding guidelines: Use vue-i18n for ALL user-facing strings, configured in src/locales/en/main.json.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/components/rightSidePanel/errors/__stories__/StoryOSSMissingNodePackFlow.vue`
around lines 36 - 63, The template in StoryOSSMissingNodePackFlow.vue contains
hardcoded user-facing strings ("ComfyUI Canvas" and "Click the
buttons…"/statusLog fallback) that must be localized; replace these literals
with vue-i18n translation lookups (e.g. $t('...')) in the template where the
text nodes appear (the element showing "ComfyUI Canvas" and the v-else fallback
that says "Click the buttons in the right-side error tab" and any statusLog
default label), add corresponding keys to src/locales/en/main.json (e.g.
comfyCanvas, clickButtonsRightErrorTab, statusLogDefault or similar), and ensure
the story registers or provides the vue-i18n instance so $t is available in this
component during story rendering.
Comment on lines
+82
to
+92
| export const Cloud_MissingNodePacks: Story = { | ||
| name: '[Cloud] Missing Node Pack', | ||
| render: () => ({ | ||
| components: { MockCloudMissingNodePack }, | ||
| template: ` | ||
| <div class="h-[800px] border border-[#494a50] rounded-lg overflow-hidden"> | ||
| <MockCloudMissingNodePack @log="msg => console.log('Log:', msg)" /> | ||
| </div> | ||
| ` | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
@log handler won't fire — MockCloudMissingNodePack only emits locate.
MockCloudMissingNodePack defines defineEmits<{ 'locate': [pack: MissingNodePack] }>() — there is no log emit. The @log listener here is dead code. Either remove it or switch to @locate:
- <MockCloudMissingNodePack `@log`="msg => console.log('Log:', msg)" />
+ <MockCloudMissingNodePack `@locate`="pack => console.log('Locate:', pack)" />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export const Cloud_MissingNodePacks: Story = { | |
| name: '[Cloud] Missing Node Pack', | |
| render: () => ({ | |
| components: { MockCloudMissingNodePack }, | |
| template: ` | |
| <div class="h-[800px] border border-[#494a50] rounded-lg overflow-hidden"> | |
| <MockCloudMissingNodePack @log="msg => console.log('Log:', msg)" /> | |
| </div> | |
| ` | |
| }) | |
| } | |
| export const Cloud_MissingNodePacks: Story = { | |
| name: '[Cloud] Missing Node Pack', | |
| render: () => ({ | |
| components: { MockCloudMissingNodePack }, | |
| template: ` | |
| <div class="h-[800px] border border-[`#494a50`] rounded-lg overflow-hidden"> | |
| <MockCloudMissingNodePack `@locate`="pack => console.log('Locate:', pack)" /> | |
| </div> | |
| ` | |
| }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/rightSidePanel/errors/TabErrors.stories.ts` around lines 82 -
92, The Story Cloud_MissingNodePacks currently listens for a non-existent `@log`
event on MockCloudMissingNodePack; since MockCloudMissingNodePack defines
defineEmits<{ 'locate': [pack: MissingNodePack] }>(), replace the dead `@log`
listener with `@locate` (e.g., `@locate`="msg => console.log('Locate:', msg)") or
simply remove the handler if you don't need to react to the emit; update the
template in the Cloud_MissingNodePacks render to reference
MockCloudMissingNodePack and its 'locate' emit accordingly.
Comment on lines
+41
to
66
| /** | ||
| * Resolve display info for a node by its execution ID. | ||
| * For group node internals, resolves the parent group node's title instead. | ||
| */ | ||
| function resolveNodeInfo(nodeId: string) { | ||
| const graphNode = getNodeByExecutionId(app.rootGraph, nodeId) | ||
|
|
||
| const parts = parseNodeExecutionId(nodeId) | ||
| const parentId = parts && parts.length > 1 ? String(parts[0]) : null | ||
| const parentNode = parentId | ||
| ? app.rootGraph.getNodeById(Number(parentId)) | ||
| : null | ||
| const isParentGroupNode = parentNode ? isGroupNode(parentNode) : false | ||
|
|
||
| return { | ||
| title: resolveNodeDisplayName(graphNode, { | ||
| emptyLabel: '', | ||
| untitledLabel: '', | ||
| st | ||
| }), | ||
| graphNodeId: graphNode ? String(graphNode.id) : undefined | ||
| title: isParentGroupNode | ||
| ? parentNode?.title || '' | ||
| : resolveNodeDisplayName(graphNode, { | ||
| emptyLabel: '', | ||
| untitledLabel: '', | ||
| st | ||
| }), | ||
| graphNodeId: graphNode ? String(graphNode.id) : undefined, | ||
| isParentGroupNode | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid numeric coercion for parent node IDs.
Number(parentId) will fail for non-numeric node IDs, which would prevent parent group title resolution. Use the string ID directly.
Proposed fix
- const parentNode = parentId
- ? app.rootGraph.getNodeById(Number(parentId))
+ const parentNode = parentId
+ ? app.rootGraph.getNodeById(parentId)
: null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/rightSidePanel/errors/useErrorGroups.ts` around lines 41 - 66,
The resolveNodeInfo function uses Number(parentId) when calling
app.rootGraph.getNodeById, which breaks for non-numeric IDs; update
resolveNodeInfo to pass the string parentId directly to
app.rootGraph.getNodeById (or the graph API's expected string id variant)
instead of coercing to Number, and ensure isParentGroupNode detection
(isGroupNode) and title resolution (parentNode?.title) use that string-based
lookup; reference resolveNodeInfo, parseNodeExecutionId,
app.rootGraph.getNodeById, isGroupNode, and resolveNodeDisplayName when making
the change.
Comment on lines
+1364
to
1367
| "Error System": "Error System", | ||
| "Other": "Other", | ||
| "Secrets": "Secrets", | ||
| "Error System": "Error System" |
Contributor
There was a problem hiding this comment.
Remove duplicate settingsCategories.Error System key.
JSON duplicates drop the earlier entry and can confuse translators; keep a single key.
🧹 Suggested cleanup
@@
- "Workspace": "Workspace",
- "Error System": "Error System",
- "Other": "Other",
- "Secrets": "Secrets",
- "Error System": "Error System"
+ "Workspace": "Workspace",
+ "Error System": "Error System",
+ "Other": "Other",
+ "Secrets": "Secrets"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/locales/en/main.json` around lines 1364 - 1367, There is a duplicate
localization key "Error System" in settingsCategories (settingsCategories.Error
System); remove the redundant entry so only one "Error System": "Error System"
mapping remains in src/locales/en/main.json, ensuring translators see a single
canonical key and no earlier duplicate is left behind.
Comment on lines
+445
to
+459
| function handleServiceLevelError(detail: ExecutionErrorWsMessage): boolean { | ||
| const nodeId = detail.node_id | ||
| if (nodeId !== null && nodeId !== undefined && String(nodeId) !== '') | ||
| return false | ||
|
|
||
| clearInitializationByPromptId(detail.prompt_id) | ||
| resetExecutionState(detail.prompt_id) | ||
| lastPromptError.value = { | ||
| type: detail.exception_type ?? 'error', | ||
| message: detail.exception_type | ||
| ? `${detail.exception_type}: ${detail.exception_message}` | ||
| : (detail.exception_message ?? ''), | ||
| details: detail.traceback?.join('\n') ?? '' | ||
| } | ||
| return true |
Contributor
There was a problem hiding this comment.
Avoid rendering “undefined/null” in service-level error messages.
If exception_message is missing, the current template string will surface undefined/null to users. Consider guarding the message to keep it clean.
Proposed fix
- lastPromptError.value = {
- type: detail.exception_type ?? 'error',
- message: detail.exception_type
- ? `${detail.exception_type}: ${detail.exception_message}`
- : (detail.exception_message ?? ''),
- details: detail.traceback?.join('\n') ?? ''
- }
+ const exceptionType = detail.exception_type ?? 'error'
+ const exceptionMessage = detail.exception_message ?? ''
+ const message =
+ exceptionMessage.length > 0
+ ? `${exceptionType}: ${exceptionMessage}`
+ : exceptionType
+ lastPromptError.value = {
+ type: exceptionType,
+ message,
+ details: detail.traceback?.join('\n') ?? ''
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/stores/executionStore.ts` around lines 445 - 459, The service-level error
message can include "undefined"/"null" because exception_message is interpolated
directly; in handleServiceLevelError update the construction of
lastPromptError.value.message to guard against missing exception_message (e.g.,
use a conditional that appends ": " + exception_message only when
exception_message is non-null/non-undefined/non-empty, otherwise fall back to
exception_type or an empty string), ensuring you still set type and details as
currently done; keep calls to clearInitializationByPromptId and
resetExecutionState unchanged.
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
1 participant
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
Adds 7 Storybook stories for the Error Tab to collect design feedback on missing node pack and missing model UX flows across OSS and Cloud environments.
Changes
TabErrors.stories.tswith stories covering [Local OSS] missing node packs (tab-only + full layout flow), [Cloud] missing node packs, [Local OSS] missing models, [Cloud] missing models (basic + with model type selector). Each scenario backed by a dedicated mock Vue component under__stories__/.eslint.config.tsis modified solely to suppress lint errors within Storybook mock files.Review Focus
Intent
This is a draft PR for design feedback only and will not be merged. Once the final design is confirmed, the mock components will be discarded and the feature will be fully reimplemented using real components.
┆Issue is synchronized with this Notion page by Unito
Screenshots