[do not merge] App mode testing branch#9219
Draft
pythongosssss wants to merge 15 commits intomainfrom
Draft
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors app-mode state into a new Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant BuilderUI as Builder UI (Menu / Toolbar)
participant SaveHook as useBuilderSave
participant WorkflowSvc as workflowService
participant Workflow as ComfyWorkflow (store)
participant Settings as Settings Store
U->>BuilderUI: Click "Save App"
BuilderUI->>SaveHook: onSave() / setSaving(true)
SaveHook->>WorkflowSvc: saveWorkflowAs(workflow, { initialMode })
WorkflowSvc->>Workflow: set initialMode / syncLinearMode(...)
WorkflowSvc->>Workflow: persist workflow (save)
WorkflowSvc-->>SaveHook: success/failure
SaveHook-->>BuilderUI: setSaving(false) / show dialog
BuilderUI->>SaveHook: "View App" -> setMode('app')
BuilderUI->>Settings: markAsSeen() (menu indicator)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ 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 |
pythongosssss
added
the
preview-gpu
🎭 Playwright: ✅ 543 passed, 0 failed · 7 flaky📊 Browser Reports
|
🎨 Storybook: ✅ Built — View Storybook |
📦 Bundle: 4.44 MB gzip 🔴 +3.76 kBDetailsSummary
Category Glance App Entry Points — 17.9 kB (baseline 17.9 kB) • 🔴 +1 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 958 kB (baseline 1.01 MB) • 🟢 -50.6 kBGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 72.1 kB (baseline 72.1 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 10 added / 10 removed Panels & Settings — 435 kB (baseline 435 kB) • 🔴 +1 BConfiguration panels, inspectors, and settings screens
Status: 10 added / 10 removed User & Accounts — 16 kB (baseline 16 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 6 added / 6 removed Editors & Dialogs — 736 B (baseline 736 B) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 1 added / 1 removed UI Components — 47.1 kB (baseline 47.1 kB) • 🔴 +1 BReusable component library chunks
Status: 12 added / 12 removed Data & Services — 2.61 MB (baseline 2.55 MB) • 🔴 +60.2 kBStores, services, APIs, and repositories
Status: 13 added / 13 removed Utilities & Hooks — 55.5 kB (baseline 55.5 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 14 added / 14 removed Vendor & Third-Party — 8.84 MB (baseline 8.84 MB) • 🟢 -151 BExternal libraries and shared vendor chunks
Status: 3 added / 3 removed Other — 7.77 MB (baseline 7.77 MB) • 🔴 +13 BBundles that do not match a named category
Status: 74 added / 74 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/ui/TypeformPopoverButton.vue (1)
22-41:⚠️ Potential issue | 🟡 MinorAdd
aria-labelto both icon-only buttons.Both the mobile anchor button (Line 22) and the non-mobile popover trigger button (Line 35) render exclusively an icon with no visible text, making them inaccessible to screen readers.
♿ Proposed fix
<Button v-if="isMobile" as="a" :href="`https://form.typeform.com/to/${dataTfWidget}`" target="_blank" variant="inverted" class="flex h-10 items-center justify-center gap-2.5 px-3 py-2" + :aria-label="$t('feedback.openFeedback')" v-bind="$attrs" > <i class="icon-[lucide--circle-help] size-4" /> </Button> <Popover v-else> <template `#button`> <Button variant="inverted" class="flex h-10 items-center justify-center gap-2.5 px-3 py-2" + :aria-label="$t('feedback.openFeedback')" v-bind="$attrs" > <i class="icon-[lucide--circle-help] size-4" /> </Button> </template>Based on learnings: icon-only buttons require an
aria-labelsince there is no visible text to provide an accessible name.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/TypeformPopoverButton.vue` around lines 22 - 41, The icon-only buttons rendered in the mobile anchor Button (when isMobile is true) and the Popover trigger Button (inside <Popover>) lack accessible names; add an aria-label prop to both Button instances (the one using :href with dataTfWidget and the Popover trigger Button) e.g. aria-label="Open help form" or a localized equivalent so screen readers can announce their purpose; ensure both Button components receive the same accessible label text and keep v-bind="$attrs" intact.
🧹 Nitpick comments (11)
src/utils/__tests__/litegraphTestUtils.ts (2)
213-235: Considersatisfiesfor type-safe mock constructionThe
as Partial<ChangeTracker> as ChangeTrackerdouble assertion silently swallows invalid or missing properties. Usingsatisfieswould catch shape mismatches at compile time without narrowing the variable type.♻️ Proposed refactor using `satisfies`
export function createMockChangeTracker( overrides: Partial<ChangeTracker> = {} ): ChangeTracker { - const partial = { + const partial = { activeState: { last_node_id: 0, last_link_id: 0, nodes: [], links: [], groups: [], config: {}, version: 0.4 }, undoQueue: [], redoQueue: [], changeCount: 0, checkState: vi.fn(), reset: vi.fn(), restore: vi.fn(), store: vi.fn(), ...overrides - } - return partial as Partial<ChangeTracker> as ChangeTracker + } satisfies Partial<ChangeTracker> + return partial as ChangeTracker }Based on learnings, test helper functions that construct mock objects implementing an interface should use
satisfies InterfaceTypefor shape validation instead of chained type assertions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/__tests__/litegraphTestUtils.ts` around lines 213 - 235, In createMockChangeTracker, stop using the unsafe double assertion "as Partial<ChangeTracker> as ChangeTracker" and instead validate the mock shape with TypeScript's satisfies operator: declare the constructed object using "satisfies Partial<ChangeTracker>" (so the compiler checks the provided properties against ChangeTracker) and then return it cast once to ChangeTracker if needed; reference the createMockChangeTracker function and the ChangeTracker type when making this replacement.
229-232: Consider refactoring to usesatisfiesfor type safetyThe three new
vi.fn()entries (checkState,restore,store) are correctly mocked—all three methods exist onChangeTracker. However, the double type assertion on line 235 (as Partial<ChangeTracker> as ChangeTracker) should be replaced with thesatisfiespattern for better type safety:return partial satisfies ChangeTrackerThis approach provides compile-time shape validation without bypassing TypeScript's type system.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/__tests__/litegraphTestUtils.ts` around lines 229 - 232, The mock object adds checkState, restore, and store as vi.fn() but uses a double type assertion ("as Partial<ChangeTracker> as ChangeTracker"); replace that with TypeScript's "satisfies ChangeTracker" pattern to get compile-time shape checking without bypassing the type system—update the return/value expression that currently asserts to ChangeTracker to instead use "satisfies ChangeTracker", keeping the mock method names checkState, restore, and store unchanged.src/renderer/core/canvas/canvasStore.ts (1)
48-53: Useif/elseinstead of a ternary inside thesethandler — side-effectful call.The
setfunction callssetMode(...), which mutates reactive state — a side effect. Per project coding guidelines,if/elseshould be used here.♻️ Proposed fix
const linearMode = computed({ get: () => isAppMode.value, set: (val: boolean) => { - setMode(val ? 'app' : 'graph') + if (val) { + setMode('app') + } else { + setMode('graph') + } } })As per coding guidelines: "prefer if/else statements over ternary operators when performing side effects or actions."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/core/canvas/canvasStore.ts` around lines 48 - 53, The computed property linearMode currently uses a ternary inside its set handler to call setMode, which performs a side effect; replace the ternary with an explicit if/else block in the set function so it calls setMode('app') when val is true and setMode('graph') when val is false, keeping the getter as is and referencing isAppMode and setMode to locate the change.src/composables/useAppMode.ts (2)
7-7:enableAppBuilderis a hardcodedref(true)at module scope — consider deriving it or removing the indirection.As a module-level singleton that never changes in this file, it adds surface area without value. If it's meant to be a settings-driven or environment-driven flag, wire it to the appropriate source; if it's permanently
true, simply remove it from the public API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/composables/useAppMode.ts` at line 7, enableAppBuilder is a module-level ref hardcoded to true which adds unnecessary indirection; either remove the ref and return a plain true from the exported API (e.g., inside useAppMode) or wire enableAppBuilder to the real configuration source (e.g., useRuntimeConfig(), process.env, or an injected app config) so it reflects environment/settings. Locate the enableAppBuilder symbol and replace its module-scope ref(true) with one of: (a) delete the ref and inline true where used in useAppMode, or (b) initialize enableAppBuilder from the proper config API and expose it as a computed/ref so it can change with config.
9-43: EachuseAppMode()call creates independentcomputedinstances for the same derived state.
mode,isBuilderMode,isAppMode,isGraphModeare recreated on every invocation. Since this composable is called from multiple components and stores simultaneously (e.g.,canvasStore,linearOutputStore,LiteGraphCanvasSplitterOverlay,BuilderExitButton), each call site holds its own watcher chain tracking the sameworkflowStore.activeWorkflow.Consider hoisting the shared computeds to module scope alongside
enableAppBuilder, so all callers share a single reactive graph:♻️ Suggested refactor
import { computed, ref } from 'vue' import { useWorkflowStore } from '@/platform/workflow/management/stores/workflowStore' export type AppMode = 'graph' | 'app' | 'builder:select' | 'builder:arrange' const enableAppBuilder = ref(true) + +// Module-level singletons — all callers share one reactive graph +let _mode: ReturnType<typeof computed<AppMode>> | undefined +let _isBuilderMode: ReturnType<typeof computed<boolean>> | undefined +let _isAppMode: ReturnType<typeof computed<boolean>> | undefined +let _isGraphMode: ReturnType<typeof computed<boolean>> | undefined export function useAppMode() { const workflowStore = useWorkflowStore() - const mode = computed(...) - const isBuilderMode = computed(...) - const isAppMode = computed(...) - const isGraphMode = computed(...) + if (!_mode) { + _mode = computed(() => workflowStore.activeWorkflow?.activeMode ?? workflowStore.activeWorkflow?.initialMode ?? 'graph') + _isBuilderMode = computed(() => _mode!.value === 'builder:select' || _mode!.value === 'builder:arrange') + _isAppMode = computed(() => _mode!.value === 'app' || _mode!.value === 'builder:arrange') + _isGraphMode = computed(() => _mode!.value === 'graph' || _mode!.value === 'builder:select') + } ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/composables/useAppMode.ts` around lines 9 - 43, The computed properties in useAppMode() (mode, isBuilderMode, isAppMode, isGraphMode) are recreated on every call—hoist them to module scope so all callers share a single reactive graph: move the computed definitions out of the useAppMode function (alongside enableAppBuilder), reference the shared workflowStore (useWorkflowStore()) inside those module-scope computeds or via a single module-scoped getter that reads workflowStore.activeWorkflow, and then have useAppMode return the hoisted mode, isBuilderMode, isAppMode, isGraphMode and setMode wrapper that updates workflowStore.activeWorkflow.activeMode; keep using the same identifiers (mode, isBuilderMode, isAppMode, isGraphMode, setMode, useAppMode) so call sites remain unchanged.src/platform/workflow/core/services/workflowService.ts (1)
102-105: NarrowinitialModeto persistable values ('app' | 'graph') insaveWorkflowAs.Accepting full
AppModeallows builder modes at this API boundary, but serialization only supports app/graph vialinearModeboolean. Tightening the type prevents invalid mode states from entering persistence paths.As per coding guidelines "Use TypeScript for type safety".♻️ Proposed type refinement
import { useAppMode } from '@/composables/useAppMode' import type { AppMode } from '@/composables/useAppMode' + +type PersistedInitialMode = Extract<AppMode, 'app' | 'graph'> const saveWorkflowAs = async ( workflow: ComfyWorkflow, - options: { filename?: string; initialMode?: AppMode } = {} + options: { filename?: string; initialMode?: PersistedInitialMode } = {} ): Promise<boolean> => {Also applies to: 131-131
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/workflow/core/services/workflowService.ts` around lines 102 - 105, The options.initialMode parameter on saveWorkflowAs (and the similar overload at saveWorkflow) is currently typed as AppMode and should be narrowed to only the persistable modes ('app' | 'graph') to prevent builder-only modes from reaching persistence; update the function signatures for saveWorkflowAs and the other saveWorkflow variant to use initialMode?: 'app' | 'graph', update any call sites to pass only those values or map builder modes to 'app'|'graph' before calling, and ensure the existing serialization logic that derives linearMode from initialMode continues to work with the narrowed type.src/platform/workflow/core/services/workflowService.test.ts (2)
20-41:createModeTestWorkflowdoesn't setisLoaded,initialState, oractiveStaterequired byLoadedComfyWorkflow.The
LoadedComfyWorkflowinterface requiresisLoaded: true,initialState, andactiveState. Whenloaded !== false, the helper setschangeTracker,content, andoriginalContentbut omits the others before casting. Tests will pass if no code under test checks these fields, but it's fragile.Suggested improvement
if (options.loaded !== false) { workflow.changeTracker = createMockChangeTracker() workflow.content = '{}' workflow.originalContent = '{}' + ;(workflow as Record<string, unknown>).isLoaded = true + ;(workflow as Record<string, unknown>).initialState = {} + ;(workflow as Record<string, unknown>).activeState = {} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/workflow/core/services/workflowService.test.ts` around lines 20 - 41, The helper createModeTestWorkflow builds a ComfyWorkflowClass and casts it to LoadedComfyWorkflow but omits required LoadedComfyWorkflow fields; update createModeTestWorkflow so that when options.loaded !== false it also sets isLoaded = true and assigns sensible defaults for initialState and activeState (e.g., based on options.initialMode/options.activeMode or empty/default state objects) alongside changeTracker/content/originalContent so the returned object truly satisfies LoadedComfyWorkflow.
140-150: Double-cast throughPartialincreateWorkflow— consider usingsatisfies.Per project convention, test helpers constructing partial mock objects should prefer
satisfiesfor shape validation overas Partial<T> as T. This is existing code but is freshly touched.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/workflow/core/services/workflowService.test.ts` around lines 140 - 150, The helper double-casts the mock workflow; change the declaration to use TypeScript's satisfies operator for shape validation: replace the "as Partial<ComfyWorkflow>" on the wf initializer with "satisfies Partial<ComfyWorkflow>" (keeping the same object literal and properties) and remove the intermediate cast, then return wf cast once to ComfyWorkflow (keep the final return as ComfyWorkflow) so createWorkflow / wf and the ComfyWorkflow shape are validated without the double-cast.src/components/common/WorkflowActionsDropdown.vue (1)
31-33: Simplify: passmenuItemsdirectly instead of wrapping in a getter.
menuItemsis already aComputedRef, which satisfies theMaybeRefOrGetterparameter type. The wrapping getter() => menuItems.valueis unnecessary indirection.Proposed simplification
-const { hasUnseenItems, markAsSeen } = useNewMenuItemIndicator( - () => menuItems.value -) +const { hasUnseenItems, markAsSeen } = useNewMenuItemIndicator(menuItems)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/common/WorkflowActionsDropdown.vue` around lines 31 - 33, The call to useNewMenuItemIndicator currently passes a getter (() => menuItems.value); replace that with the computed ref directly by calling useNewMenuItemIndicator(menuItems) and keep the destructuring of hasUnseenItems and markAsSeen unchanged; update the invocation site where useNewMenuItemIndicator is used in WorkflowActionsDropdown.vue to pass the menuItems symbol instead of the wrapper function.src/components/builder/BuilderMenu.vue (1)
4-19: Use the sharedButtoncomponent instead of raw<button>elements.The coding guidelines and repository conventions require using the shared
Buttoncomponent at@/components/ui/button/Button.vueinstead of raw<button>elements for consistent design system styling. There are three raw<button>elements here (trigger button, save button, exit button).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."
Also applies to: 29-51
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/builder/BuilderMenu.vue` around lines 4 - 19, Replace the raw <button> elements in BuilderMenu.vue (the trigger button plus the save and exit buttons) with the shared Button component from "@/components/ui/button/Button.vue": import Button and use it in place of each raw <button>, transfer the existing attributes (aria-label, classes—prefer passing variant/size props if the Button supports them—or keep the :class binding on the Button), and move the inner content (icons and <span>) into the Button slots so styling and behavior remain identical; ensure the three occurrences (trigger, save, exit) reference the Button component and preserve the existing event handlers and bindings.src/composables/useWorkflowActionsMenu.ts (1)
86-92: Prefer immutable construction over post-declaration mutation.
item.badgeanditem.isNeware mutated after theconstdeclaration. Per the coding guidelines ("Avoid mutable state; prefer immutability and assignment at point of declaration"), building the full object in one shot is cleaner.♻️ Proposed refactor
- const item: WorkflowMenuAction = { id, label, icon, command, disabled } - if (isNew) { - item.badge = t('g.experimental') - item.isNew = true - } + const item: WorkflowMenuAction = { + id, + label, + icon, + command, + disabled, + ...(isNew && { badge: t('g.experimental'), isNew: true }) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/composables/useWorkflowActionsMenu.ts` around lines 86 - 92, The WorkflowMenuAction object is mutated after its const declaration; instead, construct it immutably in one expression: build the item for push with all properties assigned up-front (include id, label, icon, command, disabled) and conditionally add badge: t('g.experimental') and isNew: true when isNew is truthy (e.g. via spread or ternary), then call items.push(item); avoid setting item.badge/item.isNew after declaration.
🤖 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/builder/BuilderExitButton.vue`:
- Around line 23-29: The global keydown listener registered via useEventListener
in BuilderExitButton.vue calls e.stopPropagation(), which prevents other
components (dialogs/popovers) from receiving Escape; remove the
e.stopPropagation() call (or instead scope the listener to a specific container)
so only e.preventDefault() is applied and then call onExitBuilder() — locate the
window keydown handler (useEventListener(..., 'keydown', (e: KeyboardEvent) => {
... })) and delete the stopPropagation invocation or move the listener off
window to a narrower element if you prefer scoping.
- Around line 18-21: Replace the old store import/use (useAppModeStore) with the
new composable useAppMode in BuilderExitButton.vue: swap the call to
useAppModeStore() for useAppMode(), remove direct reliance on the old store’s
exitBuilder method and instead call the composable’s exitBuilder function (or
the equivalent method exposed by useAppMode), and ensure the rest of the
component continues to use the existing i18n `t` const unchanged; update any
references to methods/properties from useAppModeStore (e.g., exitBuilder) to the
matching exports from useAppMode so this file matches the PR migration.
- Around line 5-10: Replace the raw <button> with the shared Button component:
import and register the Button component (src/components/ui/button/Button.vue)
in BuilderExitButton, replace the HTML button element that calls onExitBuilder
and renders t('builderMenu.exitAppBuilder') with the Button component, forward
the click handler (onExitBuilder) and any existing classes/props
(size/variant/aria attributes) so focus ring, hover state and design tokens are
used; ensure the label stays as the Button slot or prop and remove the old
button element.
In `@src/components/builder/BuilderMenu.vue`:
- Around line 81-83: The onExitBuilder handler should accept and invoke the
popover close callback before starting the exit flow so the popover does not
remain visible; update the onExitBuilder definition (and any caller) to accept a
close: () => void parameter, call close() immediately (or before any
confirmation dialog) and then call or await appModeStore.exitBuilder(); follow
the same pattern used in OutputHistoryActiveQueueItem.vue where the popover
close is invoked prior to performing the action.
In `@src/renderer/extensions/linearMode/OutputHistoryActiveQueueItem.vue`:
- Around line 40-48: The Clear button currently enables when queueCount !== 0;
change its disabled condition to disable when queueCount <= 1 so the
Comfy.ClearPendingTasks command only runs if there are pending tasks beyond the
active one. Update the Button component usage (the prop referencing queueCount
in OutputHistoryActiveQueueItem.vue) to use ":disabled=\"queueCount <= 1\"" and
ensure the click handler still invokes clearQueue(close) unchanged; keep
references to queueCount, clearQueue and close so reviewers can locate the
change.
In `@src/stores/appModeStore.ts`:
- Around line 43-52: The watcher currently gates persisting
oldWorkflow.dirtyLinearData using isBuilderMode.value (which reflects the new
active workflow) and thus can skip saving when switching away from builder;
change the check to derive the mode from the outgoing workflow itself (e.g.,
test oldWorkflow.mode === 'builder' or call the same helper used to compute
isBuilderMode but pass oldWorkflow) before assigning
oldWorkflow.dirtyLinearData, and keep the persistence of inputs/outputs using
selectedInputs and selectedOutputs as before.
---
Outside diff comments:
In `@src/components/ui/TypeformPopoverButton.vue`:
- Around line 22-41: The icon-only buttons rendered in the mobile anchor Button
(when isMobile is true) and the Popover trigger Button (inside <Popover>) lack
accessible names; add an aria-label prop to both Button instances (the one using
:href with dataTfWidget and the Popover trigger Button) e.g. aria-label="Open
help form" or a localized equivalent so screen readers can announce their
purpose; ensure both Button components receive the same accessible label text
and keep v-bind="$attrs" intact.
---
Nitpick comments:
In `@src/components/builder/BuilderMenu.vue`:
- Around line 4-19: Replace the raw <button> elements in BuilderMenu.vue (the
trigger button plus the save and exit buttons) with the shared Button component
from "@/components/ui/button/Button.vue": import Button and use it in place of
each raw <button>, transfer the existing attributes (aria-label, classes—prefer
passing variant/size props if the Button supports them—or keep the :class
binding on the Button), and move the inner content (icons and <span>) into the
Button slots so styling and behavior remain identical; ensure the three
occurrences (trigger, save, exit) reference the Button component and preserve
the existing event handlers and bindings.
In `@src/components/common/WorkflowActionsDropdown.vue`:
- Around line 31-33: The call to useNewMenuItemIndicator currently passes a
getter (() => menuItems.value); replace that with the computed ref directly by
calling useNewMenuItemIndicator(menuItems) and keep the destructuring of
hasUnseenItems and markAsSeen unchanged; update the invocation site where
useNewMenuItemIndicator is used in WorkflowActionsDropdown.vue to pass the
menuItems symbol instead of the wrapper function.
In `@src/composables/useAppMode.ts`:
- Line 7: enableAppBuilder is a module-level ref hardcoded to true which adds
unnecessary indirection; either remove the ref and return a plain true from the
exported API (e.g., inside useAppMode) or wire enableAppBuilder to the real
configuration source (e.g., useRuntimeConfig(), process.env, or an injected app
config) so it reflects environment/settings. Locate the enableAppBuilder symbol
and replace its module-scope ref(true) with one of: (a) delete the ref and
inline true where used in useAppMode, or (b) initialize enableAppBuilder from
the proper config API and expose it as a computed/ref so it can change with
config.
- Around line 9-43: The computed properties in useAppMode() (mode,
isBuilderMode, isAppMode, isGraphMode) are recreated on every call—hoist them to
module scope so all callers share a single reactive graph: move the computed
definitions out of the useAppMode function (alongside enableAppBuilder),
reference the shared workflowStore (useWorkflowStore()) inside those
module-scope computeds or via a single module-scoped getter that reads
workflowStore.activeWorkflow, and then have useAppMode return the hoisted mode,
isBuilderMode, isAppMode, isGraphMode and setMode wrapper that updates
workflowStore.activeWorkflow.activeMode; keep using the same identifiers (mode,
isBuilderMode, isAppMode, isGraphMode, setMode, useAppMode) so call sites remain
unchanged.
In `@src/composables/useWorkflowActionsMenu.ts`:
- Around line 86-92: The WorkflowMenuAction object is mutated after its const
declaration; instead, construct it immutably in one expression: build the item
for push with all properties assigned up-front (include id, label, icon,
command, disabled) and conditionally add badge: t('g.experimental') and isNew:
true when isNew is truthy (e.g. via spread or ternary), then call
items.push(item); avoid setting item.badge/item.isNew after declaration.
In `@src/platform/workflow/core/services/workflowService.test.ts`:
- Around line 20-41: The helper createModeTestWorkflow builds a
ComfyWorkflowClass and casts it to LoadedComfyWorkflow but omits required
LoadedComfyWorkflow fields; update createModeTestWorkflow so that when
options.loaded !== false it also sets isLoaded = true and assigns sensible
defaults for initialState and activeState (e.g., based on
options.initialMode/options.activeMode or empty/default state objects) alongside
changeTracker/content/originalContent so the returned object truly satisfies
LoadedComfyWorkflow.
- Around line 140-150: The helper double-casts the mock workflow; change the
declaration to use TypeScript's satisfies operator for shape validation: replace
the "as Partial<ComfyWorkflow>" on the wf initializer with "satisfies
Partial<ComfyWorkflow>" (keeping the same object literal and properties) and
remove the intermediate cast, then return wf cast once to ComfyWorkflow (keep
the final return as ComfyWorkflow) so createWorkflow / wf and the ComfyWorkflow
shape are validated without the double-cast.
In `@src/platform/workflow/core/services/workflowService.ts`:
- Around line 102-105: The options.initialMode parameter on saveWorkflowAs (and
the similar overload at saveWorkflow) is currently typed as AppMode and should
be narrowed to only the persistable modes ('app' | 'graph') to prevent
builder-only modes from reaching persistence; update the function signatures for
saveWorkflowAs and the other saveWorkflow variant to use initialMode?: 'app' |
'graph', update any call sites to pass only those values or map builder modes to
'app'|'graph' before calling, and ensure the existing serialization logic that
derives linearMode from initialMode continues to work with the narrowed type.
In `@src/renderer/core/canvas/canvasStore.ts`:
- Around line 48-53: The computed property linearMode currently uses a ternary
inside its set handler to call setMode, which performs a side effect; replace
the ternary with an explicit if/else block in the set function so it calls
setMode('app') when val is true and setMode('graph') when val is false, keeping
the getter as is and referencing isAppMode and setMode to locate the change.
In `@src/utils/__tests__/litegraphTestUtils.ts`:
- Around line 213-235: In createMockChangeTracker, stop using the unsafe double
assertion "as Partial<ChangeTracker> as ChangeTracker" and instead validate the
mock shape with TypeScript's satisfies operator: declare the constructed object
using "satisfies Partial<ChangeTracker>" (so the compiler checks the provided
properties against ChangeTracker) and then return it cast once to ChangeTracker
if needed; reference the createMockChangeTracker function and the ChangeTracker
type when making this replacement.
- Around line 229-232: The mock object adds checkState, restore, and store as
vi.fn() but uses a double type assertion ("as Partial<ChangeTracker> as
ChangeTracker"); replace that with TypeScript's "satisfies ChangeTracker"
pattern to get compile-time shape checking without bypassing the type
system—update the return/value expression that currently asserts to
ChangeTracker to instead use "satisfies ChangeTracker", keeping the mock method
names checkState, restore, and store unchanged.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
src/components/LiteGraphCanvasSplitterOverlay.vuesrc/components/appMode/AppModeToolbar.vuesrc/components/breadcrumb/SubgraphBreadcrumb.vuesrc/components/builder/AppBuilder.vuesrc/components/builder/BuilderExitButton.vuesrc/components/builder/BuilderMenu.vuesrc/components/builder/BuilderToolbar.vuesrc/components/builder/useBuilderSave.tssrc/components/common/WorkflowActionsDropdown.vuesrc/components/common/WorkflowActionsList.test.tssrc/components/graph/GraphCanvas.vuesrc/components/sidebar/tabs/SidebarTabTemplate.vuesrc/components/topbar/WorkflowTab.vuesrc/components/ui/TypeformPopoverButton.vuesrc/composables/useAppMode.tssrc/composables/useCoreCommands.tssrc/composables/useNewMenuItemIndicator.test.tssrc/composables/useNewMenuItemIndicator.tssrc/composables/useWorkflowActionsMenu.tssrc/locales/en/main.jsonsrc/platform/settings/constants/coreSettings.tssrc/platform/workflow/core/services/workflowService.test.tssrc/platform/workflow/core/services/workflowService.tssrc/platform/workflow/management/stores/comfyWorkflow.tssrc/platform/workflow/management/stores/workflowStore.tssrc/renderer/core/canvas/canvasStore.tssrc/renderer/extensions/linearMode/LinearArrange.vuesrc/renderer/extensions/linearMode/LinearControls.vuesrc/renderer/extensions/linearMode/LinearPreview.vuesrc/renderer/extensions/linearMode/LinearWelcome.vuesrc/renderer/extensions/linearMode/OutputHistory.vuesrc/renderer/extensions/linearMode/OutputHistoryActiveQueueItem.vuesrc/renderer/extensions/linearMode/linearOutputStore.test.tssrc/renderer/extensions/linearMode/linearOutputStore.tssrc/schemas/apiSchema.tssrc/stores/appModeStore.tssrc/types/workflowMenuItem.tssrc/utils/__tests__/litegraphTestUtils.tssrc/views/GraphView.vuesrc/views/LinearView.vue
💤 Files with no reviewable changes (2)
- src/components/builder/AppBuilder.vue
- src/composables/useCoreCommands.ts
Comment on lines
+5
to
+10
| <button | ||
| class="flex h-10 min-h-8 cursor-pointer items-center justify-center rounded-lg bg-secondary-background px-3 py-2 text-sm transition-colors border-none hover:bg-secondary-background-hover" | ||
| @click="onExitBuilder" | ||
| > | ||
| {{ t('builderMenu.exitAppBuilder') }} | ||
| </button> |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace raw <button> with the shared Button component.
Using a bare <button> bypasses design-system tokens (focus ring, hover state, sizing).
♻️ Proposed fix
+<script setup lang="ts">
+import Button from '@/components/ui/button/Button.vue'
+...
+</script>
<template>
<div
class="fixed bottom-4 left-1/2 z-[1000] flex -translate-x-1/2 items-center rounded-2xl border border-border-default bg-base-background p-2 shadow-interface"
>
- <button
- class="flex h-10 min-h-8 cursor-pointer items-center justify-center rounded-lg bg-secondary-background px-3 py-2 text-sm transition-colors border-none hover:bg-secondary-background-hover"
- `@click`="onExitBuilder"
- >
+ <Button
+ variant="secondary"
+ class="h-10 min-h-8 text-sm"
+ `@click`="onExitBuilder"
+ >
{{ t('builderMenu.exitAppBuilder') }}
- </button>
+ </Button>
</div>
</template>Based on learnings: replace raw <button> HTML elements with the shared Button component at src/components/ui/button/Button.vue.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/builder/BuilderExitButton.vue` around lines 5 - 10, Replace
the raw <button> with the shared Button component: import and register the
Button component (src/components/ui/button/Button.vue) in BuilderExitButton,
replace the HTML button element that calls onExitBuilder and renders
t('builderMenu.exitAppBuilder') with the Button component, forward the click
handler (onExitBuilder) and any existing classes/props (size/variant/aria
attributes) so focus ring, hover state and design tokens are used; ensure the
label stays as the Button slot or prop and remove the old button element.
Comment on lines
+18
to
+21
| import { useAppModeStore } from '@/stores/appModeStore' | ||
|
|
||
| const { t } = useI18n() | ||
| const appModeStore = useAppModeStore() |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use useAppMode instead of useAppModeStore — inconsistent with this PR's migration.
Every other new/updated file in this PR (canvasStore.ts, linearOutputStore.ts, LinearControls.vue, LiteGraphCanvasSplitterOverlay.vue) has migrated to useAppMode(). This component is the only new file that still reaches for the old store, which also means the exitBuilder method must exist on the old store while the rest of the app reads mode state from the composable.
♻️ Proposed fix
-import { useAppModeStore } from '@/stores/appModeStore'
+import { useAppMode } from '@/composables/useAppMode'
-const appModeStore = useAppModeStore()
+const { setMode } = useAppMode()
function onExitBuilder() {
- void appModeStore.exitBuilder()
+ setMode('graph')
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/builder/BuilderExitButton.vue` around lines 18 - 21, Replace
the old store import/use (useAppModeStore) with the new composable useAppMode in
BuilderExitButton.vue: swap the call to useAppModeStore() for useAppMode(),
remove direct reliance on the old store’s exitBuilder method and instead call
the composable’s exitBuilder function (or the equivalent method exposed by
useAppMode), and ensure the rest of the component continues to use the existing
i18n `t` const unchanged; update any references to methods/properties from
useAppModeStore (e.g., exitBuilder) to the matching exports from useAppMode so
this file matches the PR migration.
Comment on lines
+23
to
+29
| useEventListener(window, 'keydown', (e: KeyboardEvent) => { | ||
| if (e.key === 'Escape' && !e.ctrlKey && !e.altKey && !e.metaKey) { | ||
| e.preventDefault() | ||
| e.stopPropagation() | ||
| onExitBuilder() | ||
| } | ||
| }) |
Contributor
There was a problem hiding this comment.
stopPropagation() on a global window keydown handler will suppress Escape for every other open dialog/popover.
This listener is registered on window, which means it sits at the top of the bubble chain. Calling e.stopPropagation() here prevents any concurrently mounted dialog, tooltip, or popover from receiving the Escape event — those will never close when the builder exit button is visible. e.preventDefault() alone is sufficient to cancel the browser default; the propagation stop should be removed (or the listener should be scoped to a more specific container).
🐛 Proposed fix
useEventListener(window, 'keydown', (e: KeyboardEvent) => {
if (e.key === 'Escape' && !e.ctrlKey && !e.altKey && !e.metaKey) {
e.preventDefault()
- e.stopPropagation()
onExitBuilder()
}
})📝 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
| useEventListener(window, 'keydown', (e: KeyboardEvent) => { | |
| if (e.key === 'Escape' && !e.ctrlKey && !e.altKey && !e.metaKey) { | |
| e.preventDefault() | |
| e.stopPropagation() | |
| onExitBuilder() | |
| } | |
| }) | |
| useEventListener(window, 'keydown', (e: KeyboardEvent) => { | |
| if (e.key === 'Escape' && !e.ctrlKey && !e.altKey && !e.metaKey) { | |
| e.preventDefault() | |
| onExitBuilder() | |
| } | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/builder/BuilderExitButton.vue` around lines 23 - 29, The
global keydown listener registered via useEventListener in BuilderExitButton.vue
calls e.stopPropagation(), which prevents other components (dialogs/popovers)
from receiving Escape; remove the e.stopPropagation() call (or instead scope the
listener to a specific container) so only e.preventDefault() is applied and then
call onExitBuilder() — locate the window keydown handler (useEventListener(...,
'keydown', (e: KeyboardEvent) => { ... })) and delete the stopPropagation
invocation or move the listener off window to a narrower element if you prefer
scoping.
Comment on lines
+81
to
+83
| function onExitBuilder() { | ||
| void appModeStore.exitBuilder() | ||
| } |
Contributor
There was a problem hiding this comment.
Popover remains open after onExitBuilder.
When the user clicks "Exit App Builder", the popover isn't explicitly closed before the exit flow begins. If exitBuilder shows a confirmation dialog, the popover will remain visible behind/alongside it. Consider accepting and invoking the popover's close callback (similar to the pattern in OutputHistoryActiveQueueItem.vue).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/builder/BuilderMenu.vue` around lines 81 - 83, The
onExitBuilder handler should accept and invoke the popover close callback before
starting the exit flow so the popover does not remain visible; update the
onExitBuilder definition (and any caller) to accept a close: () => void
parameter, call close() immediately (or before any confirmation dialog) and then
call or await appModeStore.exitBuilder(); follow the same pattern used in
OutputHistoryActiveQueueItem.vue where the popover close is invoked prior to
performing the action.
Comment on lines
+40
to
+48
| <Button | ||
| :disabled="queueCount === 0" | ||
| variant="textonly" | ||
| class="text-destructive-background px-4 text-sm" | ||
| @click="clearQueue(close)" | ||
| > | ||
| <i class="icon-[lucide--list-x]" /> | ||
| {{ t('linearMode.queue.clear') }} | ||
| </Button> |
Contributor
There was a problem hiding this comment.
Clear button should be disabled when queueCount <= 1, not just === 0.
Per the queue system semantics, queueCount = 1 means only the currently running task exists with no pending tasks. The Comfy.ClearPendingTasks command should only be enabled when queueCount > 1 to avoid attempting to clear the active/running task.
Proposed fix
<Button
- :disabled="queueCount === 0"
+ :disabled="queueCount <= 1"
variant="textonly"Based on learnings: "In the ComfyUI frontend queue system, useQueuePendingTaskCountStore().count includes the currently executing task. When count = 1, there is only the active/running task with no pending tasks."
📝 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
| <Button | |
| :disabled="queueCount === 0" | |
| variant="textonly" | |
| class="text-destructive-background px-4 text-sm" | |
| @click="clearQueue(close)" | |
| > | |
| <i class="icon-[lucide--list-x]" /> | |
| {{ t('linearMode.queue.clear') }} | |
| </Button> | |
| <Button | |
| :disabled="queueCount <= 1" | |
| variant="textonly" | |
| class="text-destructive-background px-4 text-sm" | |
| `@click`="clearQueue(close)" | |
| > | |
| <i class="icon-[lucide--list-x]" /> | |
| {{ t('linearMode.queue.clear') }} | |
| </Button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/extensions/linearMode/OutputHistoryActiveQueueItem.vue` around
lines 40 - 48, The Clear button currently enables when queueCount !== 0; change
its disabled condition to disable when queueCount <= 1 so the
Comfy.ClearPendingTasks command only runs if there are pending tasks beyond the
active one. Update the Button component usage (the prop referencing queueCount
in OutputHistoryActiveQueueItem.vue) to use ":disabled=\"queueCount <= 1\"" and
ensure the click handler still invokes clearQueue(close) unchanged; keep
references to queueCount, clearQueue and close so reviewers can locate the
change.
Comment on lines
+43
to
+52
| watch( | ||
| () => workflowStore.activeWorkflow, | ||
| (newWorkflow, oldWorkflow) => { | ||
| // Persist in-progress builder selections to the outgoing workflow | ||
| if (oldWorkflow && isBuilderMode.value) { | ||
| oldWorkflow.dirtyLinearData = { | ||
| inputs: [...selectedInputs], | ||
| outputs: [...selectedOutputs] | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Persist outgoing selections using the outgoing workflow mode, not isBuilderMode.value.
isBuilderMode.value here is derived from the current active workflow after the switch. On builder → graph tab transitions, this can skip persisting oldWorkflow.dirtyLinearData and lose in-progress builder selections.
🔧 Proposed fix
watch(
() => workflowStore.activeWorkflow,
(newWorkflow, oldWorkflow) => {
// Persist in-progress builder selections to the outgoing workflow
- if (oldWorkflow && isBuilderMode.value) {
+ const oldMode = oldWorkflow?.activeMode ?? oldWorkflow?.initialMode
+ const wasBuilderMode =
+ oldMode === 'builder:select' || oldMode === 'builder:arrange'
+ if (oldWorkflow && wasBuilderMode) {
oldWorkflow.dirtyLinearData = {
inputs: [...selectedInputs],
outputs: [...selectedOutputs]
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/stores/appModeStore.ts` around lines 43 - 52, The watcher currently gates
persisting oldWorkflow.dirtyLinearData using isBuilderMode.value (which reflects
the new active workflow) and thus can skip saving when switching away from
builder; change the check to derive the mode from the outgoing workflow itself
(e.g., test oldWorkflow.mode === 'builder' or call the same helper used to
compute isBuilderMode but pass oldWorkflow) before assigning
oldWorkflow.dirtyLinearData, and keep the persistence of inputs/outputs using
selectedInputs and selectedOutputs as before.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/platform/workflow/management/stores/comfyWorkflow.ts`:
- Around line 29-39: syncLinearMode currently writes workflow.initialMode into
target.extra.linearMode which can persist stale state; change it to derive the
effective mode from workflow.activeMode (if present) falling back to
workflow.initialMode, and set target.extra.linearMode = (effectiveMode ===
'app'); update the block inside syncLinearMode that assigns extra.linearMode
(for ComfyWorkflow and LinearModeTarget) to use this effectiveMode logic and
keep existing behavior for flushLinearData unchanged.
- Around line 42-49: The current code clears workflow.dirtyLinearData
unconditionally even if nothing was written; modify the flush logic so that you
only set workflow.dirtyLinearData = null when you actually flushed it to at
least one target: when options?.flushLinearData is true and you assign
workflow.dirtyLinearData into a target (i.e., set target.extra.linearData =
workflow.dirtyLinearData). Implement this by introducing a local flag (e.g.,
flushed = false) before iterating targets, set flushed = true when you assign to
target.extra.linearData (or detect that any target.extra.linearData was set),
and after the loop only clear workflow.dirtyLinearData if flushed is true.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
browser_tests/tests/execution.spec.ts-snapshots/execution-error-unconnected-slot-chromium-linux.pngis excluded by!**/*.pngbrowser_tests/tests/nodeSearchBox.spec.ts-snapshots/added-node-no-connection-chromium-linux.pngis excluded by!**/*.pngbrowser_tests/tests/saveImageAndWebp.spec.ts-snapshots/save-image-and-webm-preview-chromium-linux.pngis excluded by!**/*.pngbrowser_tests/tests/selectionToolbox.spec.ts-snapshots/selection-toolbox-multiple-nodes-border-chromium-linux.pngis excluded by!**/*.pngbrowser_tests/tests/vueNodes/interactions/node/imagePreview.spec.ts-snapshots/vue-node-multiple-promoted-previews-chromium-linux.pngis excluded by!**/*.png
📒 Files selected for processing (1)
src/platform/workflow/management/stores/comfyWorkflow.ts
Comment on lines
+29
to
+39
| export function syncLinearMode( | ||
| workflow: ComfyWorkflow, | ||
| targets: LinearModeTarget[], | ||
| options?: { flushLinearData?: boolean } | ||
| ): void { | ||
| for (const target of targets) { | ||
| if (!target) continue | ||
| if (workflow.initialMode === 'app' || workflow.initialMode === 'graph') { | ||
| const extra = (target.extra ??= {}) | ||
| extra.linearMode = workflow.initialMode === 'app' | ||
| } else { |
Contributor
There was a problem hiding this comment.
Persist the effective mode (activeMode over initialMode) during sync.
activeMode is documented as taking precedence, but syncLinearMode serializes only initialMode. This can persist stale mode after user mode changes.
Proposed fix
export function syncLinearMode(
workflow: ComfyWorkflow,
targets: LinearModeTarget[],
options?: { flushLinearData?: boolean }
): void {
+ const mode = workflow.activeMode ?? workflow.initialMode
for (const target of targets) {
if (!target) continue
- if (workflow.initialMode === 'app' || workflow.initialMode === 'graph') {
+ if (mode === 'app' || mode === 'graph') {
const extra = (target.extra ??= {})
- extra.linearMode = workflow.initialMode === 'app'
+ extra.linearMode = mode === 'app'
} else {
delete target.extra?.linearMode
}📝 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 function syncLinearMode( | |
| workflow: ComfyWorkflow, | |
| targets: LinearModeTarget[], | |
| options?: { flushLinearData?: boolean } | |
| ): void { | |
| for (const target of targets) { | |
| if (!target) continue | |
| if (workflow.initialMode === 'app' || workflow.initialMode === 'graph') { | |
| const extra = (target.extra ??= {}) | |
| extra.linearMode = workflow.initialMode === 'app' | |
| } else { | |
| export function syncLinearMode( | |
| workflow: ComfyWorkflow, | |
| targets: LinearModeTarget[], | |
| options?: { flushLinearData?: boolean } | |
| ): void { | |
| const mode = workflow.activeMode ?? workflow.initialMode | |
| for (const target of targets) { | |
| if (!target) continue | |
| if (mode === 'app' || mode === 'graph') { | |
| const extra = (target.extra ??= {}) | |
| extra.linearMode = mode === 'app' | |
| } else { | |
| delete target.extra?.linearMode | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/platform/workflow/management/stores/comfyWorkflow.ts` around lines 29 -
39, syncLinearMode currently writes workflow.initialMode into
target.extra.linearMode which can persist stale state; change it to derive the
effective mode from workflow.activeMode (if present) falling back to
workflow.initialMode, and set target.extra.linearMode = (effectiveMode ===
'app'); update the block inside syncLinearMode that assigns extra.linearMode
(for ComfyWorkflow and LinearModeTarget) to use this effectiveMode logic and
keep existing behavior for flushLinearData unchanged.
Comment on lines
+42
to
+49
| if (options?.flushLinearData && workflow.dirtyLinearData) { | ||
| const extra = (target.extra ??= {}) | ||
| extra.linearData = workflow.dirtyLinearData | ||
| } | ||
| } | ||
| if (options?.flushLinearData && workflow.dirtyLinearData) { | ||
| workflow.dirtyLinearData = null | ||
| } |
Contributor
There was a problem hiding this comment.
Only clear dirtyLinearData after at least one successful flush.
Current logic nulls workflow.dirtyLinearData even if no target was writable/non-null, which can drop unsaved builder selections.
Proposed fix
export function syncLinearMode(
workflow: ComfyWorkflow,
targets: LinearModeTarget[],
options?: { flushLinearData?: boolean }
): void {
+ let didFlushLinearData = false
for (const target of targets) {
if (!target) continue
@@
if (options?.flushLinearData && workflow.dirtyLinearData) {
const extra = (target.extra ??= {})
extra.linearData = workflow.dirtyLinearData
+ didFlushLinearData = true
}
}
- if (options?.flushLinearData && workflow.dirtyLinearData) {
+ if (options?.flushLinearData && didFlushLinearData) {
workflow.dirtyLinearData = null
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/platform/workflow/management/stores/comfyWorkflow.ts` around lines 42 -
49, The current code clears workflow.dirtyLinearData unconditionally even if
nothing was written; modify the flush logic so that you only set
workflow.dirtyLinearData = null when you actually flushed it to at least one
target: when options?.flushLinearData is true and you assign
workflow.dirtyLinearData into a target (i.e., set target.extra.linearData =
workflow.dirtyLinearData). Implement this by introducing a local flag (e.g.,
flushed = false) before iterating targets, set flushed = true when you assign to
target.extra.linearData (or detect that any target.extra.linearData was set),
and after the loop only clear workflow.dirtyLinearData if flushed is true.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/renderer/extensions/vueNodes/components/NodeWidgets.vue (2)
164-166:handleContextMenuis typed as(e: PointerEvent)but@contextmenuemits aMouseEvent.The
contextmenuDOM event delivers aMouseEvent, not aPointerEvent. WhilePointerEvent extends MouseEventso there's no runtime issue, the handler type is technically inverted — it should accept the broaderMouseEventtype to match the event it actually receives.♻️ Proposed fix
interface ProcessedWidget { advanced: boolean - handleContextMenu: (e: PointerEvent) => void + handleContextMenu: (e: MouseEvent) => void hasLayoutSize: boolean- const handleContextMenu = (e: PointerEvent) => { + const handleContextMenu = (e: MouseEvent) => {Also applies to: 245-254
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/extensions/vueNodes/components/NodeWidgets.vue` around lines 164 - 166, The ProcessedWidget interface incorrectly types handleContextMenu as (e: PointerEvent) => void even though the `@contextmenu` DOM event emits a MouseEvent; update the type to (e: MouseEvent) => void in the ProcessedWidget declaration and change any other occurrences where handleContextMenu (or similarly named context menu handlers) are typed as PointerEvent (including the second block around the other widget/type declarations) so the handler signature matches the actual event type emitted.
200-200:idduplicatesbareWidgetIdcomputation — use the existing variable.Line 265 repeats the same
widget.nodeId ?? nodeIdexpression already computed on line 200 asbareWidgetId. UsebareWidgetIddirectly to avoid drift.♻️ Proposed fix
id: widget.nodeId ?? nodeId, + id: bareWidgetId,i.e., at line 265:
- id: widget.nodeId ?? nodeId, + id: bareWidgetId,Also applies to: 265-265
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/extensions/vueNodes/components/NodeWidgets.vue` at line 200, The assignment for id duplicates the earlier computed bareWidgetId (const bareWidgetId = widget.nodeId ?? nodeId); replace the repeated expression (widget.nodeId ?? nodeId) used to set id with the existing bareWidgetId to avoid duplication and potential drift—update the id declaration/usage in the NodeWidgets.vue component so it references bareWidgetId instead of recomputing the expression.src/renderer/extensions/vueNodes/components/LGraphNode.vue (1)
53-56:LGraphNodeandNodeWidgetsdirectly importAppOutput/AppInputfromlinearModeextension, creating cross-module coupling.Both components are conditionally rendered via
v-if="mode === 'builder:select'", so they don't execute in graph mode. However, the imports are static, meaninglinearModecode is always bundled withvueNodesregardless of which app mode is active. While the components themselves are small, this couples two extension modules at build time.Consider using
defineAsyncComponent(already employed inwidgetRegistry.tsfor heavy components likeWidgetChartandWidgetImageCrop) to defer loading until needed, or provideAppOutput/AppInputvia a composable/provide pattern solinearModecan register them on-demand.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/extensions/vueNodes/components/LGraphNode.vue` around lines 53 - 56, LGraphNode.vue and NodeWidgets currently statically import AppOutput/AppInput from the linearMode extension, causing build-time coupling; change those static imports to lazy-loaded components using Vue's defineAsyncComponent (as used in widgetRegistry.ts for WidgetChart/WidgetImageCrop) or switch to a composable/provide pattern so linearMode registers/provides AppOutput/AppInput only when mode === 'builder:select'; update the LGraphNode.vue render bindings that reference AppOutput/AppInput (and any imports in NodeWidgets) to use the async/injected component names so the linearMode bundle is not included unless actually used.src/components/builder/AppBuilder.vue (1)
308-321: Convert output selection toggles to semantic Button components.These click targets are currently non-semantic
<div>elements. Replace them with theButtoncomponent (icon-only with i18n aria-labels) for proper keyboard accessibility and semantic HTML. The Button component supports all the styling and positioning classes needed here.Add these two translation keys to
src/locales/en/main.json:
linearMode.builder.removeOutputSelectionlinearMode.builder.addOutputSelectionSuggested change
import DraggableList from '@/components/common/DraggableList.vue' import IoItem from '@/components/builder/IoItem.vue' +import Button from '@/components/ui/button/Button.vue' import PropertiesAccordionItem from '@/components/rightSidePanel/layout/PropertiesAccordionItem.vue' @@ - <div + <Button v-if="isSelected" class="absolute -top-1/2 -right-1/2 size-full p-2 bg-warning-background rounded-lg cursor-pointer pointer-events-auto" + :aria-label="t('linearMode.builder.removeOutputSelection')" `@click.stop`=" remove(appModeStore.selectedOutputs, (k) => k === key) " `@pointerdown.stop` > <i class="icon-[lucide--check] bg-text-foreground size-full" /> - </div> - <div + </Button> + <Button v-else class="absolute -top-1/2 -right-1/2 size-full ring-warning-background/50 ring-4 ring-inset bg-component-node-background rounded-lg cursor-pointer pointer-events-auto" + :aria-label="t('linearMode.builder.addOutputSelection')" `@click.stop`="appModeStore.selectedOutputs.push(key)" `@pointerdown.stop` />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/builder/AppBuilder.vue` around lines 308 - 321, Replace the two non-semantic <div> toggles in AppBuilder.vue with the Button component: keep the same positioning and styling classes, preserve `@click.stop` handlers (use remove(appModeStore.selectedOutputs, (k) => k === key) for the selected case and appModeStore.selectedOutputs.push(key) for the else case) and keep `@pointerdown.stop`; make the Buttons icon-only (render the existing <i class="icon-[lucide--check] ..."/> inside the Button) and add i18n aria-labels using the keys linearMode.builder.removeOutputSelection and linearMode.builder.addOutputSelection. Also add those two translation keys to src/locales/en/main.json.
🤖 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/renderer/extensions/linearMode/AppInput.vue`:
- Around line 26-33: The interactive div rendered when mode === 'builder:select'
is pointer-only; make it keyboard-accessible by replacing the div with a
semantic button (or add role="button" + tabindex="0" if you must keep a div),
ensure it uses `@click`="togglePromotion" (and keep pointer handlers if needed),
add type="button" when using a button, and expose state via aria-pressed or
role="switch" bound to the promotion state used by togglePromotion so keyboard
users can toggle and screen readers get the current state; update the element
that currently has `@pointerdown.capture.stop.prevent`="togglePromotion" to also
handle keyboard activation.
- Around line 13-23: The id prop is currently declared as string but
compared/paired with NodeId values and uses loose equality; update the
defineProps generic to type id as NodeId (NodeId = number | string) so it
matches appModeStore.selectedInputs, change the comparison in matchesThis to use
strict equality (===) for id vs nodeId, and ensure togglePromotion pushes a
tuple matching [NodeId, string] (or explicitly cast id to NodeId) so
selectedInputs receives the correct type; refer to defineProps, matchesThis,
isPromoted, and togglePromotion when making these edits.
In `@src/renderer/extensions/linearMode/AppOutput.vue`:
- Around line 19-25: The matchesThis function is using loose equality (==) when
comparing id and nodeId which can cause type-coercion bugs; update matchesThis
to use strict equality (===) so it reliably compares the NodeId value (affecting
togglePromotion logic that uses matchesThis to add/remove id from
appModeStore.selectedOutputs and the isPromoted check). Ensure this change only
alters the equality operator in matchesThis and nothing else.
---
Nitpick comments:
In `@src/components/builder/AppBuilder.vue`:
- Around line 308-321: Replace the two non-semantic <div> toggles in
AppBuilder.vue with the Button component: keep the same positioning and styling
classes, preserve `@click.stop` handlers (use remove(appModeStore.selectedOutputs,
(k) => k === key) for the selected case and
appModeStore.selectedOutputs.push(key) for the else case) and keep
`@pointerdown.stop`; make the Buttons icon-only (render the existing <i
class="icon-[lucide--check] ..."/> inside the Button) and add i18n aria-labels
using the keys linearMode.builder.removeOutputSelection and
linearMode.builder.addOutputSelection. Also add those two translation keys to
src/locales/en/main.json.
In `@src/renderer/extensions/vueNodes/components/LGraphNode.vue`:
- Around line 53-56: LGraphNode.vue and NodeWidgets currently statically import
AppOutput/AppInput from the linearMode extension, causing build-time coupling;
change those static imports to lazy-loaded components using Vue's
defineAsyncComponent (as used in widgetRegistry.ts for
WidgetChart/WidgetImageCrop) or switch to a composable/provide pattern so
linearMode registers/provides AppOutput/AppInput only when mode ===
'builder:select'; update the LGraphNode.vue render bindings that reference
AppOutput/AppInput (and any imports in NodeWidgets) to use the async/injected
component names so the linearMode bundle is not included unless actually used.
In `@src/renderer/extensions/vueNodes/components/NodeWidgets.vue`:
- Around line 164-166: The ProcessedWidget interface incorrectly types
handleContextMenu as (e: PointerEvent) => void even though the `@contextmenu` DOM
event emits a MouseEvent; update the type to (e: MouseEvent) => void in the
ProcessedWidget declaration and change any other occurrences where
handleContextMenu (or similarly named context menu handlers) are typed as
PointerEvent (including the second block around the other widget/type
declarations) so the handler signature matches the actual event type emitted.
- Line 200: The assignment for id duplicates the earlier computed bareWidgetId
(const bareWidgetId = widget.nodeId ?? nodeId); replace the repeated expression
(widget.nodeId ?? nodeId) used to set id with the existing bareWidgetId to avoid
duplication and potential drift—update the id declaration/usage in the
NodeWidgets.vue component so it references bareWidgetId instead of recomputing
the expression.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/components/builder/AppBuilder.vuesrc/components/rightSidePanel/parameters/WidgetItem.vuesrc/composables/graph/useGraphNodeManager.tssrc/composables/graph/useMoreOptionsMenu.tssrc/core/graph/subgraph/promotedWidgetView.tssrc/renderer/extensions/linearMode/AppInput.vuesrc/renderer/extensions/linearMode/AppOutput.vuesrc/renderer/extensions/vueNodes/components/LGraphNode.vuesrc/renderer/extensions/vueNodes/components/NodeWidgets.vuesrc/stores/widgetValueStore.ts
💤 Files with no reviewable changes (1)
- src/stores/widgetValueStore.ts
Comment on lines
+13
to
+23
| const { id, name } = defineProps<{ id: string; name: string }>() | ||
|
|
||
| const isPromoted = computed(() => appModeStore.selectedInputs.some(matchesThis)) | ||
|
|
||
| function matchesThis([nodeId, widgetName]: [NodeId, string]) { | ||
| return id == nodeId && name === widgetName | ||
| } | ||
| function togglePromotion() { | ||
| if (isPromoted.value) remove(appModeStore.selectedInputs, matchesThis) | ||
| else appModeStore.selectedInputs.push([id, name]) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Find and inspect NodeId definition"
fd -e ts -e tsx | xargs rg -l "export.*type.*NodeId\b" | head -5
echo "---"
rg "export.*type.*NodeId\b" -A 2
echo
echo "2) Inspect appModeStore.ts for selectedInputs type"
fd appModeStore.ts -x cat -n {}
echo
echo "3) Inspect AppInput.vue current state (full file)"
fd AppInput.vue src/renderer/extensions/linearMode -x cat -n {}Repository: Comfy-Org/ComfyUI_frontend
Length of output: 7034
Align id with NodeId and use strict equality for type safety.
id is typed as string, but selectedInputs is defined as [NodeId, string][] (where NodeId = number | string). When pushing [id, name] to the store, the type mismatch creates ambiguity. Additionally, the comparison uses loose equality (==), which allows unintended coercions between string and number types.
♻️ Proposed type/equality tightening
-const { id, name } = defineProps<{ id: string; name: string }>()
+const { id, name } = defineProps<{ id: NodeId; name: string }>()
function matchesThis([nodeId, widgetName]: [NodeId, string]) {
- return id == nodeId && name === widgetName
+ return id === nodeId && name === widgetName
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/extensions/linearMode/AppInput.vue` around lines 13 - 23, The id
prop is currently declared as string but compared/paired with NodeId values and
uses loose equality; update the defineProps generic to type id as NodeId (NodeId
= number | string) so it matches appModeStore.selectedInputs, change the
comparison in matchesThis to use strict equality (===) for id vs nodeId, and
ensure togglePromotion pushes a tuple matching [NodeId, string] (or explicitly
cast id to NodeId) so selectedInputs receives the correct type; refer to
defineProps, matchesThis, isPromoted, and togglePromotion when making these
edits.
Comment on lines
+26
to
+33
| <div | ||
| v-if="mode === 'builder:select'" | ||
| class="col-span-2 flex flex-row pointer-events-auto cursor-pointer gap-1 relative" | ||
| @pointerdown.capture.stop.prevent="togglePromotion" | ||
| @click.capture.stop.prevent | ||
| @pointerup.capture.stop.prevent | ||
| @pointermove.capture.stop.prevent | ||
| > |
Contributor
There was a problem hiding this comment.
Make the promotion toggle keyboard-accessible.
The control is interactive but pointer-only (div + pointer handlers), so keyboard users cannot toggle this state.
♿ Suggested accessibility fix
- <div
+ <button
v-if="mode === 'builder:select'"
+ type="button"
class="col-span-2 flex flex-row pointer-events-auto cursor-pointer gap-1 relative"
`@pointerdown.capture.stop.prevent`="togglePromotion"
+ `@keydown.enter.capture.stop.prevent`="togglePromotion"
+ `@keydown.space.capture.stop.prevent`="togglePromotion"
`@click.capture.stop.prevent`
`@pointerup.capture.stop.prevent`
`@pointermove.capture.stop.prevent`
>
@@
- </div>
+ </button>📝 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-if="mode === 'builder:select'" | |
| class="col-span-2 flex flex-row pointer-events-auto cursor-pointer gap-1 relative" | |
| @pointerdown.capture.stop.prevent="togglePromotion" | |
| @click.capture.stop.prevent | |
| @pointerup.capture.stop.prevent | |
| @pointermove.capture.stop.prevent | |
| > | |
| <button | |
| v-if="mode === 'builder:select'" | |
| type="button" | |
| class="col-span-2 flex flex-row pointer-events-auto cursor-pointer gap-1 relative" | |
| `@pointerdown.capture.stop.prevent`="togglePromotion" | |
| `@keydown.enter.capture.stop.prevent`="togglePromotion" | |
| `@keydown.space.capture.stop.prevent`="togglePromotion" | |
| `@click.capture.stop.prevent` | |
| `@pointerup.capture.stop.prevent` | |
| `@pointermove.capture.stop.prevent` | |
| > |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/extensions/linearMode/AppInput.vue` around lines 26 - 33, The
interactive div rendered when mode === 'builder:select' is pointer-only; make it
keyboard-accessible by replacing the div with a semantic button (or add
role="button" + tabindex="0" if you must keep a div), ensure it uses
`@click`="togglePromotion" (and keep pointer handlers if needed), add
type="button" when using a button, and expose state via aria-pressed or
role="switch" bound to the promotion state used by togglePromotion so keyboard
users can toggle and screen readers get the current state; update the element
that currently has `@pointerdown.capture.stop.prevent`="togglePromotion" to also
handle keyboard activation.
Comment on lines
+19
to
+25
| function matchesThis(nodeId: NodeId) { | ||
| return id == nodeId | ||
| } | ||
| function togglePromotion() { | ||
| if (isPromoted.value) remove(appModeStore.selectedOutputs, matchesThis) | ||
| else appModeStore.selectedOutputs.push(id) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find and check NodeId type definition
rg -n 'type NodeId|interface NodeId' --type ts src/lib/litegraph/ -A 2Repository: Comfy-Org/ComfyUI_frontend
Length of output: 536
🏁 Script executed:
#!/bin/bash
# Check the actual AppOutput.vue file
cat -n src/renderer/extensions/linearMode/AppOutput.vue | head -40Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1474
🏁 Script executed:
#!/bin/bash
# Search for appModeStore definition to check selectedOutputs type
rg -n 'selectedOutputs' --type ts -C 3 src/ | head -100Repository: Comfy-Org/ComfyUI_frontend
Length of output: 2035
Use strict equality === instead of loose equality == when comparing node IDs.
The comparison in matchesThis uses == which can cause unexpected type coercion. While id (typed as string) is a valid NodeId (which is number | string), comparing a string to a NodeId that might be a number with loose equality will silently coerce types—for example, "123" == 123 evaluates to true. This can cause incorrect matches. Use === for strict comparison.
🐛 Proposed fix
function matchesThis(nodeId: NodeId) {
- return id == nodeId
+ return id === nodeId
}📝 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
| function matchesThis(nodeId: NodeId) { | |
| return id == nodeId | |
| } | |
| function togglePromotion() { | |
| if (isPromoted.value) remove(appModeStore.selectedOutputs, matchesThis) | |
| else appModeStore.selectedOutputs.push(id) | |
| } | |
| function matchesThis(nodeId: NodeId) { | |
| return id === nodeId | |
| } | |
| function togglePromotion() { | |
| if (isPromoted.value) remove(appModeStore.selectedOutputs, matchesThis) | |
| else appModeStore.selectedOutputs.push(id) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/extensions/linearMode/AppOutput.vue` around lines 19 - 25, The
matchesThis function is using loose equality (==) when comparing id and nodeId
which can cause type-coercion bugs; update matchesThis to use strict equality
(===) so it reliably compares the NodeId value (affecting togglePromotion logic
that uses matchesThis to add/remove id from appModeStore.selectedOutputs and the
isPromoted check). Ensure this change only alters the equality operator in
matchesThis and nothing else.
⚡ Performance Report
Raw data{
"timestamp": "2026-02-26T23:46:02.113Z",
"gitSha": "f7f2d97d898bbc52f405b9718446f27ced798c02",
"branch": "pysssss/app-mode-preview",
"measurements": [
{
"name": "canvas-idle",
"durationMs": 2002.7200000000107,
"styleRecalcs": 123,
"styleRecalcDurationMs": 18.248,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 386.051,
"heapDeltaBytes": -2132472
},
{
"name": "canvas-mouse-sweep",
"durationMs": 1779.944999999998,
"styleRecalcs": 173,
"styleRecalcDurationMs": 41.188,
"layouts": 12,
"layoutDurationMs": 2.84,
"taskDurationMs": 752.6919999999999,
"heapDeltaBytes": -3057024
},
{
"name": "dom-widget-clipping",
"durationMs": 617.4500000000194,
"styleRecalcs": 44,
"styleRecalcDurationMs": 12.801,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 380.481,
"heapDeltaBytes": 6620320
}
]
} |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/views/LinearView.vue (1)
147-158:⚠️ Potential issue | 🟠 MajorBoth
ControlPreviewandLinearControlswill render simultaneously whenisBuilderMode && sidebarOnLeft.The conditions on lines 147 and 148 are separate
v-ifstatements, not av-if/v-else-ifchain. WhenisBuilderModeis true andsidebarOnLeftis true, bothControlPreviewandLinearControlswill render together. Compare this to lines 102-107 which correctly usesv-else-if/v-else.🐛 Proposed fix: use proper conditional chain
<ControlPreview v-if="isBuilderMode && sidebarOnLeft" /> <LinearControls - v-if="sidebarOnLeft" + v-else-if="sidebarOnLeft" ref="linearWorkflowRef" :toast-to="unrefElement(bottomRightRef) ?? undefined" /> <div - v-else-if="activeTab" + v-else-if="!sidebarOnLeft && activeTab" class="flex h-full border-border-subtle border-l" > <ExtensionSlot :extension="activeTab" /> </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/LinearView.vue` around lines 147 - 158, The two separate v-if checks cause both ControlPreview and LinearControls to render when isBuilderMode && sidebarOnLeft; change the conditional chain so only one renders: make ControlPreview use v-if="isBuilderMode && sidebarOnLeft" and LinearControls use v-else-if="sidebarOnLeft" (keeping the ref="linearWorkflowRef" and :toast-to="unrefElement(bottomRightRef) ?? undefined" props intact) so the ExtensionSlot / activeTab branch remains the v-else-if/v-else continuation.
🧹 Nitpick comments (1)
src/renderer/extensions/linearMode/LinearControls.vue (1)
72-76: Precompute subgraph node lookup to avoid repeated scans.Line 74–76 rebuilds and searches the flattened subgraph node list on each
whileiteration. Consider building a node-id map once before the loop.♻️ Suggested refactor
const mappedSelections = computed(() => { let unprocessedInputs = [...appModeStore.selectedInputs] + const subgraphNodeMap = new Map( + [...app.rootGraph.subgraphs.values()] + .flatMap((sg) => sg.nodes) + .map((n) => [n.id, n] as const) + ) //FIXME strict typing here const processedInputs: ReturnType<typeof nodeToNodeData>[] = [] while (unprocessedInputs.length) { @@ - const node = - app.rootGraph.getNodeById(nodeId) ?? - [...app.rootGraph.subgraphs.values()] - .flatMap((sg) => sg.nodes) - .find((n) => n.id == nodeId) + const node = + app.rootGraph.getNodeById(nodeId) ?? subgraphNodeMap.get(nodeId)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/extensions/linearMode/LinearControls.vue` around lines 72 - 76, The current code repeatedly rebuilds and searches a flattened subgraph node list inside the loop; before entering the while loop, build a single lookup map (e.g., const subgraphNodeById) by iterating app.rootGraph.subgraphs and their sg.nodes once, then replace the repeated expression that computes node via app.rootGraph.getNodeById(nodeId) ?? [...app.rootGraph.subgraphs.values()].flatMap(...).find(...) with a lookup that checks app.rootGraph.getNodeById(nodeId) and then subgraphNodeById[nodeId]; update any references to nodeId and node accordingly so the loop reuses the precomputed map.
🤖 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/renderer/extensions/linearMode/ControlPreview.vue`:
- Around line 19-33: The computed mappedSelections currently throws inside
mappedSelections when a node is missing, which can crash the component; change
mappedSelections to skip/filter entries where the node lookup fails instead of
throwing: in the mappedSelections computed callback use
app.rootGraph.getNodeById(nodeId) (and the subgraph search) to locate the node,
and if the node is not found simply filter that selection out (or return null
and then compact the array), then proceed to call nodeToNodeData and remove on
the remaining nodes; ensure you reference mappedSelections, nodeToNodeData,
remove, and app.rootGraph.getNodeById when updating the logic so no exception is
thrown from the computed property.
In `@src/renderer/extensions/linearMode/LinearControls.vue`:
- Around line 332-336: The toast/message logic uses jobFinishedQueue (set in the
finally block) which can be true even on failures; replace this with an explicit
queue status state (e.g., queueStatus with values 'idle' | 'queueing' |
'success' | 'error') instead of relying on jobFinishedQueue, update the queueing
function to set queueStatus = 'queueing' before the attempt, queueStatus =
'success' inside the successful branch, queueStatus = 'error' in the catch
branch (and only use finally for cleanup without marking success), and change
the span v-text binding in LinearControls.vue to derive its message/icon from
queueStatus (or a computed that maps statuses to t('...') keys) so success
messaging is only shown when queueStatus === 'success'.
---
Outside diff comments:
In `@src/views/LinearView.vue`:
- Around line 147-158: The two separate v-if checks cause both ControlPreview
and LinearControls to render when isBuilderMode && sidebarOnLeft; change the
conditional chain so only one renders: make ControlPreview use
v-if="isBuilderMode && sidebarOnLeft" and LinearControls use
v-else-if="sidebarOnLeft" (keeping the ref="linearWorkflowRef" and
:toast-to="unrefElement(bottomRightRef) ?? undefined" props intact) so the
ExtensionSlot / activeTab branch remains the v-else-if/v-else continuation.
---
Nitpick comments:
In `@src/renderer/extensions/linearMode/LinearControls.vue`:
- Around line 72-76: The current code repeatedly rebuilds and searches a
flattened subgraph node list inside the loop; before entering the while loop,
build a single lookup map (e.g., const subgraphNodeById) by iterating
app.rootGraph.subgraphs and their sg.nodes once, then replace the repeated
expression that computes node via app.rootGraph.getNodeById(nodeId) ??
[...app.rootGraph.subgraphs.values()].flatMap(...).find(...) with a lookup that
checks app.rootGraph.getNodeById(nodeId) and then subgraphNodeById[nodeId];
update any references to nodeId and node accordingly so the loop reuses the
precomputed map.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/renderer/extensions/linearMode/ControlPreview.vuesrc/renderer/extensions/linearMode/LinearControls.vuesrc/views/LinearView.vue
Comment on lines
+19
to
+33
| const mappedSelections = computed(() => { | ||
| let unprocessedInputs = [...appModeStore.selectedInputs] | ||
| return unprocessedInputs.map(([nodeId, widgetName]) => { | ||
| const node = | ||
| app.rootGraph.getNodeById(nodeId) ?? | ||
| [...app.rootGraph.subgraphs.values()] | ||
| .flatMap((sg) => sg.nodes) | ||
| .find((n) => n.id == nodeId) | ||
| if (!node) throw new Error('missing node') | ||
|
|
||
| const nodeData = nodeToNodeData(node) | ||
| remove(nodeData.widgets ?? [], ({ name }) => widgetName !== name) | ||
| return nodeData | ||
| }) | ||
| }) |
Contributor
There was a problem hiding this comment.
Throwing inside a computed property will crash the component.
If a node cannot be found (e.g., during a transient state or after deletion), throwing an error inside a computed property will cause an unhandled exception and break the entire component. Consider graceful degradation instead.
🛠️ Proposed fix: filter out missing nodes
const mappedSelections = computed(() => {
let unprocessedInputs = [...appModeStore.selectedInputs]
- return unprocessedInputs.map(([nodeId, widgetName]) => {
+ return unprocessedInputs
+ .map(([nodeId, widgetName]) => {
const node =
app.rootGraph.getNodeById(nodeId) ??
[...app.rootGraph.subgraphs.values()]
.flatMap((sg) => sg.nodes)
.find((n) => n.id == nodeId)
- if (!node) throw new Error('missing node')
+ if (!node) return null
const nodeData = nodeToNodeData(node)
remove(nodeData.widgets ?? [], ({ name }) => widgetName !== name)
return nodeData
- })
+ })
+ .filter((n) => n !== null)
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/extensions/linearMode/ControlPreview.vue` around lines 19 - 33,
The computed mappedSelections currently throws inside mappedSelections when a
node is missing, which can crash the component; change mappedSelections to
skip/filter entries where the node lookup fails instead of throwing: in the
mappedSelections computed callback use app.rootGraph.getNodeById(nodeId) (and
the subgraph search) to locate the node, and if the node is not found simply
filter that selection out (or return null and then compact the array), then
proceed to call nodeToNodeData and remove on the remaining nodes; ensure you
reference mappedSelections, nodeToNodeData, remove, and
app.rootGraph.getNodeById when updating the logic so no exception is thrown from
the computed property.
Comment on lines
+332
to
+336
| <span | ||
| v-text=" | ||
| jobFinishedQueue ? t('queue.jobAddedToQueue') : t('queue.jobQueueing') | ||
| " | ||
| /> |
Contributor
There was a problem hiding this comment.
Avoid success messaging on failed queue attempts.
The new Line 334 success text (queue.jobAddedToQueue) depends on
jobFinishedQueue, but that flag is set to true in finally (Line 170),
including failure paths. This can show a false success toast when queueing
fails.
Track explicit queue status (queueing/success/error) and bind icon/message to
that state instead of using a single completion flag.
As per coding guidelines "Provide user-friendly and actionable error messages".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/extensions/linearMode/LinearControls.vue` around lines 332 -
336, The toast/message logic uses jobFinishedQueue (set in the finally block)
which can be true even on failures; replace this with an explicit queue status
state (e.g., queueStatus with values 'idle' | 'queueing' | 'success' | 'error')
instead of relying on jobFinishedQueue, update the queueing function to set
queueStatus = 'queueing' before the attempt, queueStatus = 'success' inside the
successful branch, queueStatus = 'error' in the catch branch (and only use
finally for cleanup without marking success), and change the span v-text binding
in LinearControls.vue to derive its message/icon from queueStatus (or a computed
that maps statuses to t('...') keys) so success messaging is only shown when
queueStatus === 'success'.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/renderer/extensions/linearMode/LinearControls.vue (1)
329-333:⚠️ Potential issue | 🟠 MajorDon’t show “added to queue” from a completion flag.
This still allows false success messaging: Line 329-333 uses
jobFinishedQueueto show success text, but Line 167-170 sets it totrueinfinallyeven on failures. Use explicit queue status (idle | queueing | success | error) and only render success on actual success.As per coding guidelines, “Implement proper error handling” and “Provide user-friendly and actionable error messages.”🐛 Proposed fix with explicit status state
-const jobFinishedQueue = ref(true) +type QueueStatus = 'idle' | 'queueing' | 'success' | 'error' +const queueStatus = ref<QueueStatus>('idle') async function runButtonClick(e: Event) { - if (!jobFinishedQueue.value) return + if (queueStatus.value === 'queueing') return try { - jobFinishedQueue.value = false + queueStatus.value = 'queueing' resetJobToastTimeout() @@ await commandStore.execute(commandId, { metadata: { subscribe_to_run: false, trigger_source: 'linear' } }) + queueStatus.value = 'success' + } catch { + queueStatus.value = 'error' + throw } finally { - //TODO: Error state indicator for failed queue? - jobFinishedQueue.value = true + // cleanup only } }-<Teleport - v-if="(!jobToastTimeout || !jobFinishedQueue) && toastTo" +<Teleport + v-if="(!jobToastTimeout || queueStatus === 'queueing') && toastTo" defer :to="toastTo" > @@ - <i v-if="jobFinishedQueue" class="icon-[lucide--check] size-5 text-muted-foreground" /> + <i + v-if="queueStatus === 'success'" + class="icon-[lucide--check] size-5 text-muted-foreground" + /> <i v-else class="icon-[lucide--loader-circle] size-4 animate-spin" /> <span v-text=" - jobFinishedQueue ? t('queue.jobAddedToQueue') : t('queue.jobQueueing') + queueStatus === 'success' + ? t('queue.jobAddedToQueue') + : queueStatus === 'error' + ? t('queue.jobFailedToQueue') + : t('queue.jobQueueing') " />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/extensions/linearMode/LinearControls.vue` around lines 329 - 333, Replace the boolean jobFinishedQueue with an explicit status state (e.g., queueStatus: 'idle' | 'queueing' | 'success' | 'error') and update places that set jobFinishedQueue (look for its assignments in the function where the queue is processed) to set queueStatus appropriately on start, on success, on error, and optionally reset in finally; then change the template span (the v-text using jobFinishedQueue) to render text only when queueStatus === 'success' (and render queueing/error messages for other statuses) so success is only shown on actual success rather than being set in finally.
🧹 Nitpick comments (1)
src/renderer/extensions/linearMode/LinearControls.vue (1)
71-75: Avoid repeated subgraph flattening inside the selection loop.This rebuilds/scan-flattens all subgraph nodes per group iteration. Precompute a lookup map once before the
whileloop to reduce repeated allocations and scans.As per coding guidelines, “When writing new code, ask if there is a simpler way to introduce the same functionality; choose the simpler approach if one exists.”♻️ Proposed refactor
const mappedSelections = computed(() => { let unprocessedInputs = [...appModeStore.selectedInputs] + const subgraphNodeById = new Map( + [...app.rootGraph.subgraphs.values()] + .flatMap((sg) => sg.nodes) + .map((n) => [String(n.id), n]) + ) //FIXME strict typing here const processedInputs: ReturnType<typeof nodeToNodeData>[] = [] while (unprocessedInputs.length) { const nodeId = unprocessedInputs[0][0] @@ - const node = - app.rootGraph.getNodeById(nodeId) ?? - [...app.rootGraph.subgraphs.values()] - .flatMap((sg) => sg.nodes) - .find((n) => n.id == nodeId) + const node = + app.rootGraph.getNodeById(nodeId) ?? + subgraphNodeById.get(String(nodeId))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/extensions/linearMode/LinearControls.vue` around lines 71 - 75, The current selection loop repeatedly recomputes [...app.rootGraph.subgraphs.values()].flatMap(...).find(...) for each nodeId which causes repeated allocations and scans; before the while loop, build a single lookup map (e.g., Map<string, Node>) by iterating app.rootGraph.subgraphs and their nodes once, then inside the loop use app.rootGraph.getNodeById(nodeId) ?? lookup.get(nodeId) instead of flattening each time; update any references to node resolution (nodeId, app.rootGraph.getNodeById, rootGraph.subgraphs) to use the precomputed map so the loop avoids redundant work.
🤖 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/builder/AppBuilder.vue`:
- Around line 310-323: Replace the non-semantic, non-keyboard-focusable <div>
selection badges with the shared Button component so the control is
keyboard-accessible and uses button semantics; keep the existing handlers (the
remove(appModeStore.selectedOutputs, (k) => k === key) call for deselect and
appModeStore.selectedOutputs.push(key) for select) but attach them to the
Button's `@click`, preserve `@pointerdown.stop` behavior, and ensure the icon-only
Button includes a clear aria-label (e.g., aria-label="Select output" / "Deselect
output") and an accessible pressed state (aria-pressed or visually-indicating
CSS) so assistive tech can tell selection state.
---
Duplicate comments:
In `@src/renderer/extensions/linearMode/LinearControls.vue`:
- Around line 329-333: Replace the boolean jobFinishedQueue with an explicit
status state (e.g., queueStatus: 'idle' | 'queueing' | 'success' | 'error') and
update places that set jobFinishedQueue (look for its assignments in the
function where the queue is processed) to set queueStatus appropriately on
start, on success, on error, and optionally reset in finally; then change the
template span (the v-text using jobFinishedQueue) to render text only when
queueStatus === 'success' (and render queueing/error messages for other
statuses) so success is only shown on actual success rather than being set in
finally.
---
Nitpick comments:
In `@src/renderer/extensions/linearMode/LinearControls.vue`:
- Around line 71-75: The current selection loop repeatedly recomputes
[...app.rootGraph.subgraphs.values()].flatMap(...).find(...) for each nodeId
which causes repeated allocations and scans; before the while loop, build a
single lookup map (e.g., Map<string, Node>) by iterating app.rootGraph.subgraphs
and their nodes once, then inside the loop use app.rootGraph.getNodeById(nodeId)
?? lookup.get(nodeId) instead of flattening each time; update any references to
node resolution (nodeId, app.rootGraph.getNodeById, rootGraph.subgraphs) to use
the precomputed map so the loop avoids redundant work.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/components/LiteGraphCanvasSplitterOverlay.vuesrc/components/builder/AppBuilder.vuesrc/components/builder/IoItem.vuesrc/components/rightSidePanel/layout/PropertiesAccordionItem.vuesrc/locales/en/main.jsonsrc/renderer/extensions/linearMode/LinearControls.vue
Comment on lines
+310
to
323
| class="absolute -top-1/2 -right-1/2 size-full p-2 bg-warning-background rounded-lg cursor-pointer pointer-events-auto" | ||
| @click.stop=" | ||
| remove(appModeStore.selectedOutputs, (k) => k === key) | ||
| " | ||
| @pointerdown.stop | ||
| > | ||
| <i class="icon-[lucide--check] bg-text-foreground size-full" /> | ||
| </div> | ||
| <div | ||
| v-else | ||
| class="absolute -top-1/2 -right-1/2 size-full ring-warning-background/50 ring-4 ring-inset bg-component-node-background rounded-lg" | ||
| class="absolute -top-1/2 -right-1/2 size-full ring-warning-background/50 ring-4 ring-inset bg-component-node-background rounded-lg cursor-pointer pointer-events-auto" | ||
| @click.stop="appModeStore.selectedOutputs.push(key)" | ||
| @pointerdown.stop | ||
| /> |
Contributor
There was a problem hiding this comment.
Selection badges need semantic, keyboard-accessible controls.
These are clickable <div> elements with pointer behavior only. They are not keyboard-focusable and icon-only actions lack an accessible name.
♿ Suggested direction
+import Button from '@/components/ui/button/Button.vue'
...
- <div
+ <Button
v-if="isSelected"
class="absolute -top-1/2 -right-1/2 size-full p-2 bg-warning-background rounded-lg cursor-pointer pointer-events-auto"
+ :aria-label="t('linearMode.builder.removeSelectedOutput')"
`@click.stop`="
remove(appModeStore.selectedOutputs, (k) => k === key)
"
`@pointerdown.stop`
>
<i class="icon-[lucide--check] bg-text-foreground size-full" />
- </div>
+ </Button>
<div
v-else
- class="absolute -top-1/2 -right-1/2 size-full ring-warning-background/50 ring-4 ring-inset bg-component-node-background rounded-lg cursor-pointer pointer-events-auto"
- `@click.stop`="appModeStore.selectedOutputs.push(key)"
+ class="absolute -top-1/2 -right-1/2 size-full ring-warning-background/50 ring-4 ring-inset bg-component-node-background rounded-lg pointer-events-auto"
+ :aria-label="t('linearMode.builder.addSelectedOutput')"
+ `@click.stop`="appModeStore.selectedOutputs.push(key)"
`@pointerdown.stop`
/>Based on learnings: "replace raw <button> HTML elements with the shared Button component" and "icon-only controls should provide a clear aria-label."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/builder/AppBuilder.vue` around lines 310 - 323, Replace the
non-semantic, non-keyboard-focusable <div> selection badges with the shared
Button component so the control is keyboard-accessible and uses button
semantics; keep the existing handlers (the remove(appModeStore.selectedOutputs,
(k) => k === key) call for deselect and appModeStore.selectedOutputs.push(key)
for select) but attach them to the Button's `@click`, preserve `@pointerdown.stop`
behavior, and ensure the icon-only Button includes a clear aria-label (e.g.,
aria-label="Select output" / "Deselect output") and an accessible pressed state
(aria-pressed or visually-indicating CSS) so assistive tech can tell selection
state.
Mixed feelings here.
- add orderable widget list - fix splitter layout issues
- Shorter tooltip delay - Never show sidebar in builder mode - Prefer widget name over node name - Non-breaking space in example node names - Remove node background colors
Separate PR was opened which was cleaner
AustinMroz
force-pushed
the
pysssss/app-mode-preview
branch
from
653d8a8 to
6eb478d
Compare
AustinMroz
added
preview-gpu
AustinMroz
removed
the
preview-gpu
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Testing branch for app mode
┆Issue is synchronized with this Notion page by Unito