Feat/errors tab panel#8807
Conversation
- Define PromptError interface in executionStore - Add hasAnyError computed getter for centralized error detection - Register 'errors' tab in rightSidePanelStore - Extract shared type definitions (ErrorItem, ErrorCardData, ErrorGroup)
- Add TabErrors component with grouped, searchable error display - Add ErrorNodeCard component for individual error rendering - Support prompt-level, node validation, and runtime errors - Add i18n keys for error-related UI strings - Remove unused modelErrors and searchNodesOrInputs i18n keys
- Show error indicator badge on errors tab in RightSidePanel - Replace duplicated hasPromptError logic with store getter - Add per-node error detection and See Error link in SectionWidgets - Store prompt-level errors separately when no node errors exist - Improve error handling comments in app.ts
- Add TabErrors unit tests covering all error types and search - Add ErrorNodeCard stories for visual regression testing
- Remove export from PromptError (used only within executionStore) - Remove export from ErrorItem (used only within types.ts)
|
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:
📝 WalkthroughWalkthroughAdds a new consolidated "Errors" tab and related UI (TabErrors, ErrorNodeCard), types, stories and tests; extends execution and right-side-panel stores to track prompt/node/runtime errors; app-level hooks open the errors panel on prompt errors; UI indicators and localization keys added. Changes
Sequence Diagram(s)sequenceDiagram
participant Executor as Executor
participant App as app.ts
participant Store as ExecutionStore
participant RightPanel as RightSidePanel
participant UI as TabErrors / ErrorNodeCard
Executor->>App: emit PromptExecutionError / runtime error
App->>Store: set `lastPromptError` / update `lastNodeErrors`
App->>Store: clear selection & open right-side panel (tab: "errors")
Store-->>RightPanel: `hasAnyError` / `lastPromptError` update
RightPanel->>UI: render TabErrors -> ErrorNodeCard(s)
UI->>Store: user actions (locateNode / enterSubgraph / copyToClipboard)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: ❌ 518 passed, 1 failed · 4 flaky ❌ Failed Tests📊 Browser Reports
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@src/components/rightSidePanel/errors/ErrorNodeCard.vue`:
- Around line 86-92: Replace the array-based :class binding in ErrorNodeCard.vue
(the element conditioned on error.details) with the cn() utility: import cn from
'@/utils/tailwindUtil' and call cn() to combine the static classes ('rounded-lg
bg-secondary-background-hover p-2.5 overflow-y-auto border
border-interface-stroke/30') with the conditional runtime height class based on
error.isRuntimeError so class merging follows repo conventions; keep the
v-if="error.details" and the same class strings but pass them to cn() instead of
using :class="[]".
In `@src/components/rightSidePanel/errors/TabErrors.vue`:
- Around line 237-245: The locateNode function currently guards
canvasStore.canvas before awaiting navigateToGraph, but uses a non-null
assertion canvasStore.canvas! afterwards which could be null after the async
gap; modify locateNode to re-check canvasStore.canvas after the await (or store
a local reference if appropriate) and only call animateToBounds when the canvas
is still present. Specifically, update the locateNode implementation (which uses
getNodeByExecutionId, navigateToGraph and canvasStore.canvas.animateToBounds) to
perform a second null/undefined check on canvasStore.canvas (or capture it into
a local variable before the await and validate it) prior to calling
animateToBounds to avoid the unsafe non-null assertion.
- Around line 361-369: The two anchor tags that call openGitHubIssues and
contactSupport are not keyboard-accessible; replace each <a ...
`@click`="openGitHubIssues"> and <a ... `@click`="contactSupport"> with semantic
<button> elements styled to look like links (keeping classes and the click
handlers) or, if you must keep anchors, add a valid href, role="button",
tabindex="0" and keydown handlers that trigger openGitHubIssues/contactSupport
on Enter/Space; ensure you update the template entries that reference those
handlers (openGitHubIssues and contactSupport) and preserve existing classes and
i18n calls.
- Around line 185-187: Remove the redundant searcher function and its prop
binding: delete the async function searcher(query: string) { searchQuery.value =
query } and remove the :searcher="searcher" prop on the FormSearchInput (the
v-model="searchQuery" already handles updates); rely on the existing computed
filteredGroups in TabErrors for filtering and ensure no other code references
the searcher symbol.
In `@src/components/rightSidePanel/parameters/SectionWidgets.vue`:
- Around line 94-97: The navigateToErrorTab function should clear any current
selection before opening the global Errors tab so it becomes available; update
navigateToErrorTab to call this.canvas.deselectAll() (or canvas.deselectAll())
before calling rightSidePanelStore.openPanel('errors'), or if there is an active
node selection, route to that node's per-node 'error' tab instead (check
selection state and call openPanel('error') for the selected node); adjust
navigateToErrorTab accordingly to implement this selection check and deselect
behavior.
🧹 Nitpick comments (5)
src/components/rightSidePanel/errors/TabErrors.test.ts (3)
36-36: Avoidanytype fori18n.The coding guidelines prohibit using
any. Use the return type ofcreateI18ninstead:Proposed fix
- let i18n: any + let i18n: ReturnType<typeof createI18n>As per coding guidelines: "Never use
anytype; use proper TypeScript types."
112-115: Replaceas anycasts withvi.mocked().Lines 114 and 135 use
as anyto mock return values. Use Vitest'svi.mocked()utility instead, which preserves type safety.Proposed fix (line 114 shown; apply same pattern at line 135)
- ;(getNodeByExecutionId as any).mockReturnValue({ title: 'CLIP Text Encode' }) + vi.mocked(getNodeByExecutionId).mockReturnValue({ title: 'CLIP Text Encode' } as any)Or better yet, create a properly-typed partial mock object. The same pattern applies to line 180 for
useCopyToClipboard.As per coding guidelines: "Never use
as anytype assertions; fix the underlying type issue instead." For mocking in Vitest, leverage Vitest's utilities where possible.
62-86: Prefer function declaration formountComponent.Proposed fix
- const mountComponent = (initialState = {}) => { - return mount(TabErrors, { + function mountComponent(initialState = {}) { + return mount(TabErrors, { ... - }) - } + }) + }As per coding guidelines: "Do not use function expressions if it's possible to use function declarations instead."
src/scripts/app.ts (1)
1466-1472:respError.details || ""coerces falsy values to empty string.This is fine for a string field, but
?? ""would be more precise (only catchingnull/undefined). Minor nit — no behavioral difference expected here sincedetailsis always a string or absent.src/components/rightSidePanel/errors/TabErrors.vue (1)
270-276: Considernoopener,noreferreronwindow.open.While GitHub is a trusted target, it's good practice to include window features to prevent the opened page from accessing
window.opener:Proposed fix
- window.open(url, '_blank') + window.open(url, '_blank', 'noopener,noreferrer')
- TabErrors.test.ts: use proper i18n type, vi.mocked(), function declaration - TabErrors.vue: remove redundant searcher function, use optional chaining on canvas, add noopener/noreferrer to window.open, replace <a> with <button> for a11y - ErrorNodeCard.vue: use cn() for class merging, add cn import - app.ts: use ?? instead of || for nullish coalescing
Only show the error indicator and 'See Error' navigation button when no canvas items are selected (workflow overview state). When a node is actively selected, the button is hidden to avoid confusing UX of deselecting and jumping to the global errors tab.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/components/rightSidePanel/errors/TabErrors.vue`:
- Line 241: The call await navigateToGraph(graphNode.graph as LGraph) can mask a
missing or wrong type because graphNode.graph may be undefined or not an LGraph;
update the code that uses getNodeByExecutionId to first verify graphNode.graph
exists and is the correct type (e.g., check graphNode && graphNode.graph &&
graphNode.graph instanceof LGraph or use a type guard) before calling
navigateToGraph, and handle the fallback (log/notify or skip navigation) when
graph is missing or is a Subgraph so you don't blindly cast to LGraph.
🧹 Nitpick comments (3)
src/components/rightSidePanel/errors/TabErrors.vue (1)
359-367: Consider using the sharedButtoncomponent withvariant="link"instead of raw<button>elements.Per codebase conventions, raw
<button>HTML elements should be replaced with the sharedButtoncomponent from@/components/ui/button/Button.vue. These inline link-styled buttons could usevariant="link"for design system consistency.That said, since these are inline within a
<p>paragraph, the shared Button may need additional styling to render inline. This is a low-priority improvement.src/components/rightSidePanel/parameters/SectionWidgets.vue (1)
184-226: Template changes are well-integrated.The error icon, destructive label styling, and "See Error" button are cleanly added within the existing layout. Good use of the shared
Buttoncomponent (line 206) with@click.stopto prevent accordion toggle.flex-wrapon line 184 handles overflow gracefully.One minor nit: line 192 uses a ternary
:classbinding that could usecn()for consistency with the codebase convention, though it's not the banned[]syntax.Optional: use cn() for class merging
+import { cn } from '@/utils/tailwindUtil' ... <span - class="truncate" - :class="nodeHasError ? 'text-destructive-background-hover' : ''" + :class="cn('truncate', nodeHasError && 'text-destructive-background-hover')" >src/components/rightSidePanel/errors/TabErrors.test.ts (1)
112-131:as anyfor mock return values — consider using a narrower partial type.Lines 114 and 135 use
as anyforgetNodeByExecutionIdmock returns. Per coding guidelines,as anyshould be avoided. Since onlytitleis needed, a targeted cast would be safer:Suggested improvement
- vi.mocked(getNodeByExecutionId).mockReturnValue({ title: 'CLIP Text Encode' } as any) + vi.mocked(getNodeByExecutionId).mockReturnValue({ title: 'CLIP Text Encode' } as ReturnType<typeof getNodeByExecutionId>)Alternatively, if that's too verbose, a dedicated
createMockNodehelper withsatisfies Partial<LGraphNode>could be used across tests.
- TabErrors.vue: guard graphNode.graph before navigateToGraph, use shared Button component for footer help links - TabErrors.test.ts: replace 'as any' with ReturnType<typeof getNodeByExecutionId> - SectionWidgets.vue: use cn() for class merging, restrict error indicator to workflow overview via canvasStore.selectedItems check - Formatting: apply oxc auto-fixes across all changed files
viva-jinyi
left a comment
There was a problem hiding this comment.
Our project convention is that code should be self-expressive and comments should be minimized. It would be better to remove decorative separator comments (e.g. // ─── Sample Data ───...) and redundant JSDoc that just restates parameter names. Also, In our project, <template> goes at the top and <script> at the bottom.
src/stores/executionStore.ts
Outdated
| return nodeLocatorId | ||
| } | ||
|
|
||
| /** Prompt-level errors without node IDs (e.g. invalid_prompt, prompt_no_outputs) */ |
There was a problem hiding this comment.
nit: Could you move this part to the top of the file?
| " | ||
| > | ||
| <p | ||
| class="m-0 text-[11px] text-muted-foreground break-words whitespace-pre-wrap font-mono leading-relaxed" |
There was a problem hiding this comment.
Can you use text-sm or text-xs instead of fixed font size?
| variant="secondary" | ||
| size="sm" | ||
| class="w-full justify-center gap-2 h-8 text-[11px]" | ||
| @click=" |
There was a problem hiding this comment.
I think it would be better to make this into a function
| */ | ||
| const nodeHasError = computed(() => { | ||
| // Only show error indicator in workflow overview (nothing selected on canvas) | ||
| if (canvasStore.selectedItems.length > 0) return false |
There was a problem hiding this comment.
very nit
if (canvasStore.selectedItems.length > 0 || !targetNode.value) return false
src/locales/en/main.json
Outdated
| "desc": "The workflow data sent to the server is empty. This may be an unexpected system error." | ||
| } | ||
| }, | ||
| "errorHelpPrefix": "For more help, ", |
There was a problem hiding this comment.
Instead of splitting the help sentence into separate tokens (errorHelpPrefix, errorHelpOr, etc.), it would be better to use interpolation variables for github and support.
src/components/TopMenuSection.vue
Outdated
| return releaseRedDot || shouldShowConflictRedDot.value | ||
| }) | ||
|
|
||
| const { hasAnyError: hasPromptError } = storeToRefs(executionStore) |
There was a problem hiding this comment.
hasAnyError is renamed to hasPromptError via destructuring, but this computed covers node errors and runtime errors too — not just prompt errors. Why was it renamed?
| variant="secondary" | ||
| size="sm" | ||
| class="rounded-lg text-sm shrink-0" | ||
| @click.stop="emit('enterSubgraph', card.nodeId!)" |
There was a problem hiding this comment.
nit: Can we use a fallback instead of non-null assertion(!)?
There was a problem hiding this comment.
Done.
Replaced inline card.nodeId! assertions with dedicated handler functions (handleLocateNode, handleEnterSubgraph) that guard with if (card.nodeId) before emitting.
src/scripts/app.ts
Outdated
| } else { | ||
| useDialogService().showExecutionErrorDialog(detail) | ||
| } | ||
| this.canvas.deselectAll() |
There was a problem hiding this comment.
I think deselecting nodes without user intent is not the right UX. If a user is in the middle of configuring a node and an error occurs, having their selection forcefully cleared is disruptive. Errors should be notified, not result in clearing the user's current working context.
There was a problem hiding this comment.
I understand the concern. However, this error overview is ultimately designed as a complete replacement for the error modal dialog.
Consider the scenario: a user is editing a specific node (right side panel shows that node's parameters) and clicks the Run button. If an error occurs but we don't deselect, the right side panel stays on that node's parameter tab — and the user has no immediate visual feedback that something went wrong. The error information would be hidden behind a tab they'd have to manually navigate to.
With the current behavior, deselectAll() switches the right side panel to the Workflow Overview, which automatically surfaces the Errors tab with a clear summary of what failed. This mirrors the UX intent of the old modal dialog — errors demand immediate attention — but in a less intrusive, non-blocking way.
That said, I'm open to exploring alternatives if needed. For example, we could show a toast notification instead and let the user decide when to view the errors. But that would mean users could miss critical errors, which is the exact problem the error modal was solving. What do you think?
There was a problem hiding this comment.
@viva-jinyi
To address the concern you raised, I’ve discussed this with the team and we are trying out a non-intrusive overlay positioned below the TopMenuSection (PR #8875).
This allows us to notify the user of errors without interrupting their current workflow or forcing a change in node selection. We are currently awaiting feedback from the QA team on this improved UX.
| const nodeId = targetNode.value.id | ||
|
|
||
| // Check lastNodeErrors (validation errors from 400 Bad Request) | ||
| if (executionStore.lastNodeErrors) { |
There was a problem hiding this comment.
nodeHasError iterates over all keys in lastNodeErrors and calls getNodeByExecutionId (which traverses the graph) for each one. Since SectionWidgets is rendered per-node, this results in O(n²) complexity. Could we instead pre-compute a Set<nodeId> of error node IDs in the store as a computed, so each component can just do set.has(myId) for O(1) lookup?
There was a problem hiding this comment.
Done.
The per-node iteration over lastNodeErrors with getNodeByExecutionId has been replaced with a pre-computed errorNodeIds Set in the execution store, giving each component an O(1) set.has() lookup.
The graph traversal now runs once in the store's computed, not per-SectionWidgets instance.
- Fix misleading hasAnyError alias in TopMenuSection - Replace non-null assertion with nullish coalescing fallback - Add pre-computed errorNodeIds Set in executionStore for O(1) lookup - Consolidate fragmented i18n tokens into single interpolated key using <i18n-t> component - Remove decorative separator comments and redundant JSDoc
When clicking See Error from a node's widget section, the errors tab now expands only the group containing that node and collapses all others.
AustinMroz
left a comment
There was a problem hiding this comment.
Jin Yi was already super thorough here. I only saw a single trivial nit.
| </div> | ||
|
|
||
| <!-- Fixed Footer: Help Links --> | ||
| <div class="shrink-0 border-t border-interface-stroke px-4 py-4 min-w-0"> |
There was a problem hiding this comment.
| <div class="shrink-0 border-t border-interface-stroke px-4 py-4 min-w-0"> | |
| <div class="shrink-0 border-t border-interface-stroke p-4 min-w-0"> |
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 — 905 kB (baseline 888 kB) • 🔴 +17.4 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 — 427 kB (baseline 427 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 — 36.6 kB (baseline 36.6 kB) • ⚪ 0 BReusable component library chunks
Status: 5 added / 5 removed Data & Services — 2.16 MB (baseline 2.16 MB) • 🔴 +2.4 kBStores, services, APIs, and repositories
Status: 13 added / 13 removed Utilities & Hooks — 237 kB (baseline 237 kB) • ⚪ 0 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.36 MB (baseline 7.36 MB) • 🔴 +802 BBundles that do not match a named category
Status: 58 added / 58 removed |
Add `Comfy.RightSidePanel.ShowErrorsTab` setting (default: false) to control the visibility of the errors tab in the right side panel. When disabled: - Error/Errors tabs are hidden from the side panel - Side panel no longer auto-opens on execution errors - See Error button and Vue node error button are no-ops - Error dialogs continue to show normally Changes: - coreSettings.ts: Add new experimental boolean setting - apiSchema.ts: Register setting ID in zSettings schema - settings.json: Add English i18n keys - RightSidePanel.vue: Gate error/errors tab visibility on setting - app.ts: Gate auto-open panel on execution/prompt errors - SectionWidgets.vue: Gate navigateToErrorTab on setting - LGraphNode.vue: Gate node error button click on setting
b821c4e to
3fa2e92
Compare
|
I’ve added a commit to register a new setting, Comfy.RightSidePanel.ShowErrorsTab, and wrapped the new features behind this flag to keep them hidden by default. (Apologies for not doing this in the first place!) Because I wanted to implement a solid baseline where the new UI and core logic work together, this PR has grown quite large. Please let me know if the size is an issue—I’m happy to discuss how to break it down further if you’d prefer! |
…ddress review feedback
- Extract error grouping/search logic into useErrorGroups.ts composable
- buildErrorGroups: pure function with dedicated process helpers
(processPromptError, processNodeErrors, processExecutionError)
- searchErrorGroups: Fuse.js-based fuzzy search
- resolveNodeInfo: pre-computes graphNodeId to avoid redundant lookups
- Extract canvas navigation into useFocusNode.ts composable
- focusNode, enterSubgraph, navigateToGraph functions
- Simplify TabErrors.vue by delegating business logic to composables
- Address reviewer feedback on executionStore.ts:
- Move PromptError interface to apiSchema.ts as zod schema
- Clear lastPromptError in resetExecutionState
- Use isEmpty (es-toolkit) for hasNodeError
- Scope errorNodeIds to active graph to prevent cross-scope
ID collisions; rename to activeGraphErrorNodeIds
- Fix search filter test for Fuse.js compatibility
- Remove redundant copyToClipboard wrapper; useCopyToClipboard already handles errors internally with toast notifications - Replace hardcoded GitHub issues URL with staticUrls.githubIssues from useExternalLink composable - Add JSDoc to focusedErrorNodeId clarifying its lifecycle (set by SectionWidgets, consumed/cleared by TabErrors) - Replace instanceof Subgraph with type-only import and isRootGraph check to avoid importing the full class

Summary
Add a dedicated Errors tab to the Right Side Panel that displays prompt-level, node validation, and runtime execution errors in a unified, searchable, grouped view — replacing the need to rely solely on modal dialogs for error inspection.
Changes
errors/directory):TabErrors.vue— Main error tab with search, grouping by class type, and canvas navigation (locate node / enter subgraph).ErrorNodeCard.vue— Renders a single error card with node ID badge, title, action buttons, and error details.types.ts— Shared type definitions (ErrorItem,ErrorCardData,ErrorGroup).executionStore.ts— AddedPromptErrorinterface,lastPromptErrorref andhasAnyErrorcomputed getter. ClearslastPromptErroralongside existing error state on execution start and graph clear.rightSidePanelStore.ts— Registered'errors'as a valid tab value.app.ts— On prompt submission failure (PromptExecutionError), stores prompt-level errors (when no node errors exist) intolastPromptError. On both runtime execution error and prompt error, deselects all nodes and opens the errors tab automatically.RightSidePanel.vue— Shows the'errors'tab (with ⚠ icon) when errors exist and no node is selected. Routes toTabErrorscomponent.TopMenuSection.vue— Highlights the action bar with a red border when any error exists, usinghasAnyError.SectionWidgets.vue— Detects per-node errors by matching execution IDs to graph node IDs. Shows an error icon (⚠) and "See Error" button that navigates to the errors tab.en/main.json— Added i18n keys:errors,noErrors,enterSubgraph,seeError,promptErrors.*, anderrorHelp*.TabErrors.test.ts) covering prompt/node/runtime errors, search filtering, and clipboard copy.ErrorNodeCard.stories.ts) for badge visibility, subgraph buttons, multiple errors, runtime tracebacks, and prompt-only errors.vue-i18n,pinia,primevue)Related Work
Architecture
Review Focus
Error normalization logic (
TabErrors.vueL75–150): Three different error sources (prompt, node validation, runtime) are normalized into a commonErrorGroup → ErrorCardData → ErrorItemhierarchy. Edge cases to verify:Canvas navigation (
TabErrors.vueL210–250): ThelocateNodeandenterSubgraphfunctions navigate to potentially nested subgraphs. The doublerequestAnimationFrameis required due to LiteGraph's asynchronous subgraph switching — worth verifying this timing is sufficient.Store getter consolidation:
hasAnyErrorreplaces duplicated logic inTopMenuSectionandRightSidePanel. Confirm that the reactive dependency chain works correctly (it depends on 3 separate refs).Coexistence with upstream
TabError.vue: The singular'error'tab (upstream, PR New bottom button and badges #8603) and our plural'errors'tab serve different purposes but share similar naming. Consider whether a unified approach is preferred.Test Results
Screenshots (if applicable)
┆Issue is synchronized with this Notion page by Unito