Merged
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:
📝 WalkthroughWalkthroughThis PR migrates app-mode state from a centralized store to a composable per-workflow model, adds builder UI components and draggable lists, persists builder linearData on workflows, updates many components to use the new composable, and expands tests and translations for builder/app mode handling. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as Component
participant Composable as useAppMode
participant Workflow
participant Store as appModeStore
User->>UI: Click "Enter Builder"
UI->>Composable: setMode('builder:select')
Composable->>Workflow: set activeWorkflow.activeMode = 'builder:select'
Workflow-->>Composable: updated activeMode
Composable-->>UI: isBuilderMode true (reactive)
UI->>UI: Render AppBuilder component
UI->>Store: flushSelections() (if invoked)
Store->>Workflow: persist dirtyLinearData (when saved)
sequenceDiagram
actor User
participant Builder as AppBuilder
participant Draggable as DraggableList
participant Workflow
participant Service as workflowService
User->>Builder: Drag / rename inputs/outputs
Builder->>Draggable: reorder items (v-model update)
Draggable-->>Builder: new order emitted
Builder->>Workflow: update dirtyLinearData / selectedInputs
User->>Builder: Click "Save"
Builder->>Service: saveWorkflowAs({ initialMode })
Service->>Workflow: save (propagate initialMode)
Service-->>Builder: save success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🎭 Playwright: ✅ 547 passed, 0 failed · 5 flaky📊 Browser Reports
|
🎨 Storybook: ✅ Built — View Storybook |
📦 Bundle: 4.44 MB gzip 🔴 +696 BDetailsSummary
Category Glance App Entry Points — 17.9 kB (baseline 17.9 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 1.01 MB (baseline 1 MB) • 🔴 +3.94 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: 9 added / 9 removed Panels & Settings — 435 kB (baseline 435 kB) • 🟢 -56 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: 5 added / 5 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) • ⚪ 0 BReusable component library chunks
Status: 5 added / 5 removed Data & Services — 2.55 MB (baseline 2.55 MB) • 🔴 +190 BStores, 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: 12 added / 12 removed Vendor & Third-Party — 8.84 MB (baseline 8.84 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 7.77 MB (baseline 7.77 MB) • 🔴 +46 BBundles that do not match a named category
Status: 56 added / 56 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: 5
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 | 🟡 MinorIcon-only buttons are missing accessible labels.
Both the mobile button (Line 22) and the popover trigger (Line 35) render only
<i class="icon-[lucide--circle-help]" />with no visible text and noaria-label. Screen reader users cannot determine the button's purpose. Whilev-bind="$attrs"allows forwarding from a parent, this relies on every call-site providing the attribute, which is fragile.♿ Proposed fix — add a fallback `aria-label` on both buttons
<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="Feedback" 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="Feedback" v-bind="$attrs" > <i class="icon-[lucide--circle-help] size-4" /> </Button> </template>🤖 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, Icon-only buttons lack accessible labels; add an explicit aria-label fallback on both Button instances so screen readers always get a meaningful name. In the mobile branch (the Button rendered when isMobile) and in the Popover trigger (the Button inside template `#button`), add an aria-label that prefers any aria-label passed via $attrs but falls back to a clear string like "Open help form" (for the anchor variant consider "Open help form (opens in new tab)"). Ensure the attribute is applied directly on those Button components so callers can still override via v-bind="$attrs".
🧹 Nitpick comments (4)
src/composables/useAppMode.ts (2)
7-8:hasOutputsandenableAppBuilderare module-level mutable refs with no controlled setter.Both are exposed directly, meaning any consumer can write
useAppMode().hasOutputs.value = ...or hold a reference and mutate it freely. This creates invisible mutation pathways — it's unclear who "owns" these values and where they're updated. Consider exposing explicit setter functions (setHasOutputs,setEnableAppBuilder) or usingreadonly()to make the intent clear, similar to howsetModeis the designated mutator for mode.♻️ Suggested approach
const hasOutputs = ref(true) const enableAppBuilder = ref(false) export function useAppMode() { + function setHasOutputs(val: boolean) { + hasOutputs.value = val + } + function setEnableAppBuilder(val: boolean) { + enableAppBuilder.value = val + } ... return { ... - hasOutputs, - enableAppBuilder, + hasOutputs: readonly(hasOutputs), + enableAppBuilder: readonly(enableAppBuilder), + setHasOutputs, + setEnableAppBuilder, setMode } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/composables/useAppMode.ts` around lines 7 - 8, hasOutputs and enableAppBuilder are module-level mutable refs that consumers can mutate directly; change useAppMode to prevent uncontrolled external mutation by making the refs readonly when returned and adding explicit mutator functions (e.g., setHasOutputs and setEnableAppBuilder) alongside existing setMode, or alternatively return readonly(hasOutputs)/readonly(enableAppBuilder) while providing exported functions to update them from within the composable; update the public API of useAppMode to return the readonly refs plus the new setter functions and replace any internal writes to use the new setters.
10-42: Newcomputedinstances are created on everyuseAppMode()call.
mode,isBuilderMode,isAppMode, andisGraphModeare all freshly created insideuseAppMode()— calling this composable in N components creates N independent (but functionally identical) computed watchers all tracking the same underlying store state. Moving the shared computed values to module level (alongsidehasOutputs/enableAppBuilder) would allow all callers to share a single reactive dependency graph.♻️ Suggested approach — hoist shared computeds to module scope
const hasOutputs = ref(true) const enableAppBuilder = ref(false) + +const mode = computed(() => { + const wf = useWorkflowStore().activeWorkflow + return wf?.activeMode ?? wf?.initialMode ?? 'graph' +}) +const isBuilderMode = computed( + () => mode.value === 'builder:select' || mode.value === 'builder:arrange' +) +const isAppMode = computed( + () => mode.value === 'app' || mode.value === 'builder:arrange' +) +const isGraphMode = computed( + () => mode.value === 'graph' || mode.value === 'builder:select' +) export function useAppMode() { - const mode = computed(() => { ... }) - const isBuilderMode = computed(...) - const isAppMode = computed(...) - const isGraphMode = computed(...) ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/composables/useAppMode.ts` around lines 10 - 42, The computed properties mode, isBuilderMode, isAppMode, and isGraphMode are being recreated on every call to useAppMode(); hoist these computed declarations to module scope (next to existing hasOutputs/enableAppBuilder) so all callers share the same reactive instances, keep setMode as the local function that mutates useWorkflowStore().activeWorkflow, and update useAppMode() to simply return the module-scoped mode, isBuilderMode, isAppMode, isGraphMode, hasOutputs, enableAppBuilder, and setMode so no duplicate computed watchers are created.src/platform/workflow/core/services/workflowService.ts (1)
360-369:useAppMode()called transiently for a single flag read.
useAppMode()is invoked inside the asyncafterLoadNewGraphfunction solely to accessisAppMode.valueonce for the telemetry gate. This creates four computed instances (mode,isBuilderMode,isAppMode,isGraphMode) that are immediately discarded when the function returns. The same check can be derived directly without the composable:♻️ Suggested simplification
-const { isAppMode } = useAppMode() - -// Determine the initial app mode for fresh loads from serialized state. -// Tab switches don't need this — the mode is already on the workflow. -const freshLoadMode: AppMode = workflowData.extra?.linearMode - ? 'app' - : 'graph' - -if (!isAppMode.value && freshLoadMode === 'app') - useTelemetry()?.trackEnterLinear({ source: 'workflow' }) +const freshLoadMode: AppMode = workflowData.extra?.linearMode ? 'app' : 'graph' + +if (freshLoadMode === 'app') { + const wf = workflowStore.activeWorkflow + const currentMode = wf?.activeMode ?? wf?.initialMode ?? 'graph' + const currentIsAppMode = currentMode === 'app' || currentMode === 'builder:arrange' + if (!currentIsAppMode) useTelemetry()?.trackEnterLinear({ source: 'workflow' }) +}🤖 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 360 - 369, The transient call to useAppMode() inside afterLoadNewGraph should be removed; instead derive the current app mode directly from the workflow object and use that for the telemetry gate. Replace the useAppMode() invocation and the isAppMode.value check with a direct read of the workflow's mode (e.g., determine currentMode from workflow.mode or workflow.value.mode depending on how workflow is exposed) and then call useTelemetry()?.trackEnterLinear({ source: 'workflow' }) only when currentMode is not 'app' and freshLoadMode === 'app'; update the code around freshLoadMode and the telemetry condition to use this direct check (references: useAppMode(), isAppMode.value, afterLoadNewGraph, workflowData.extra?.linearMode, useTelemetry()?.trackEnterLinear).src/components/builder/useBuilderSave.ts (1)
22-30: Consider callingonBuilderSave()directly fromsetSavinginstead of via a watcher.
savingis mutated exclusively throughsetSavingandresetSaving, so thewatchadds an async tick of indirection without providing any resilience benefit. A direct call keeps the control flow synchronous and easier to trace.♻️ Suggested simplification
const saving = ref(false) - watch(saving, (value) => { - if (value) void onBuilderSave() - }) - function setSaving(value: boolean) { saving.value = value + if (value) void onBuilderSave() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/builder/useBuilderSave.ts` around lines 22 - 30, The watcher indirection is unnecessary because saving is only changed via setSaving/resetSaving; remove the watch and instead invoke onBuilderSave() directly from setSaving when setting true (use the same fire-and-forget pattern, e.g., void onBuilderSave()), while still updating saving.value; also ensure resetSaving uses setSaving(false) so all mutations go through that single function; update references to the removed watch accordingly.
🤖 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/core/services/workflowService.test.ts`:
- Around line 21-52: Replace the double-cast assertions with TypeScript's
`satisfies` to preserve shape-checking: for the changeTracker assignment in
createModeTestWorkflow, replace the "as Partial<ChangeTracker> as ChangeTracker"
double-cast by using "satisfies ChangeTracker" on the object literal passed to
markRaw (i.e. markRaw({ store: vi.fn(), reset: vi.fn(), restore: vi.fn() }
satisfies ChangeTracker)); and for makeWorkflowData, return the object literal
using "satisfies ComfyWorkflowJSON" instead of "as Partial<ComfyWorkflowJSON> as
ComfyWorkflowJSON" so the returned literal is validated against
ComfyWorkflowJSON's shape.
In `@src/renderer/core/canvas/canvasStore.ts`:
- Around line 46-51: The computed linearMode getter/setter is asymmetric:
isAppMode() treats both 'app' and 'builder:arrange' as true while the linearMode
setter only calls setMode('app' | 'graph'), which can silently demote builder
modes; update the linearMode setter (or add a clear comment above the computed)
to preserve builder context—e.g., detect current mode via isAppMode and current
mode string and, when setting true, only call setMode('app') if not already a
builder mode or otherwise leave builder:* modes unchanged; reference the
computed named linearMode, the getter isAppMode, and the mode mutator setMode
(and the 'builder:arrange' mode) when implementing or documenting this behavior.
In `@src/renderer/extensions/linearMode/OutputHistory.vue`:
- Around line 290-292: The clear button should be disabled when there are no
pending tasks (queueCount ≤ 1); update the button that triggers clearQueue to
bind its disabled state to the expression queueCount <= 1 (match the same
condition used for the badge) so the button is non-interactive when only the
active task remains; locate the element that calls clearQueue in
OutputHistory.vue (near OutputHistoryActiveQueueItem and the queueCount
references) and add the disabled binding to that button.
In `@src/renderer/extensions/linearMode/OutputHistoryActiveQueueItem.vue`:
- Around line 50-54: The badge overlay div in OutputHistoryActiveQueueItem.vue
renders a bare numeric visual ("queueCount") which will be read out of context
by screen readers; modify the element that renders the count (the div with
v-if="queueCount > 1" and v-text="queueCount") to include aria-hidden="true" so
assistive tech ignores it (the parent Button already has the descriptive
aria-label).
- Around line 40-47: The Clear Pending Tasks button should be disabled when
there are no pending tasks (queueCount <= 1); update the Button in
OutputHistoryActiveQueueItem.vue to bind its disabled state to (queueCount <= 1)
so it appears non-actionable when only the running task exists, and ensure the
click handler clearQueue(close) remains unchanged (so clicks are ignored when
disabled); reference the Button element, the queueCount reactive/computed value,
and the clearQueue method (and the Comfy.ClearPendingTasks behavior) when
applying this change.
---
Outside diff comments:
In `@src/components/ui/TypeformPopoverButton.vue`:
- Around line 22-41: Icon-only buttons lack accessible labels; add an explicit
aria-label fallback on both Button instances so screen readers always get a
meaningful name. In the mobile branch (the Button rendered when isMobile) and in
the Popover trigger (the Button inside template `#button`), add an aria-label that
prefers any aria-label passed via $attrs but falls back to a clear string like
"Open help form" (for the anchor variant consider "Open help form (opens in new
tab)"). Ensure the attribute is applied directly on those Button components so
callers can still override via v-bind="$attrs".
---
Nitpick comments:
In `@src/components/builder/useBuilderSave.ts`:
- Around line 22-30: The watcher indirection is unnecessary because saving is
only changed via setSaving/resetSaving; remove the watch and instead invoke
onBuilderSave() directly from setSaving when setting true (use the same
fire-and-forget pattern, e.g., void onBuilderSave()), while still updating
saving.value; also ensure resetSaving uses setSaving(false) so all mutations go
through that single function; update references to the removed watch
accordingly.
In `@src/composables/useAppMode.ts`:
- Around line 7-8: hasOutputs and enableAppBuilder are module-level mutable refs
that consumers can mutate directly; change useAppMode to prevent uncontrolled
external mutation by making the refs readonly when returned and adding explicit
mutator functions (e.g., setHasOutputs and setEnableAppBuilder) alongside
existing setMode, or alternatively return
readonly(hasOutputs)/readonly(enableAppBuilder) while providing exported
functions to update them from within the composable; update the public API of
useAppMode to return the readonly refs plus the new setter functions and replace
any internal writes to use the new setters.
- Around line 10-42: The computed properties mode, isBuilderMode, isAppMode, and
isGraphMode are being recreated on every call to useAppMode(); hoist these
computed declarations to module scope (next to existing
hasOutputs/enableAppBuilder) so all callers share the same reactive instances,
keep setMode as the local function that mutates
useWorkflowStore().activeWorkflow, and update useAppMode() to simply return the
module-scoped mode, isBuilderMode, isAppMode, isGraphMode, hasOutputs,
enableAppBuilder, and setMode so no duplicate computed watchers are created.
In `@src/platform/workflow/core/services/workflowService.ts`:
- Around line 360-369: The transient call to useAppMode() inside
afterLoadNewGraph should be removed; instead derive the current app mode
directly from the workflow object and use that for the telemetry gate. Replace
the useAppMode() invocation and the isAppMode.value check with a direct read of
the workflow's mode (e.g., determine currentMode from workflow.mode or
workflow.value.mode depending on how workflow is exposed) and then call
useTelemetry()?.trackEnterLinear({ source: 'workflow' }) only when currentMode
is not 'app' and freshLoadMode === 'app'; update the code around freshLoadMode
and the telemetry condition to use this direct check (references: useAppMode(),
isAppMode.value, afterLoadNewGraph, workflowData.extra?.linearMode,
useTelemetry()?.trackEnterLinear).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
src/components/appMode/AppModeToolbar.vuesrc/components/builder/BuilderToolbar.vuesrc/components/builder/useBuilderSave.tssrc/components/graph/GraphCanvas.vuesrc/components/sidebar/tabs/SidebarTabTemplate.vuesrc/components/ui/TypeformPopoverButton.vuesrc/composables/useAppMode.tssrc/locales/en/main.jsonsrc/platform/workflow/core/services/workflowService.test.tssrc/platform/workflow/core/services/workflowService.tssrc/platform/workflow/management/stores/comfyWorkflow.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/stores/appModeStore.tssrc/views/GraphView.vuesrc/views/LinearView.vue
💤 Files with no reviewable changes (1)
- src/stores/appModeStore.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/platform/workflow/core/services/workflowService.test.ts (1)
133-147: Prefersatisfiesover double assertions in the helper.The
as Partial<ComfyWorkflow>→as ComfyWorkflowpattern can hide shape drift. Usingsatisfieskeeps excess/field checking while still returning aComfyWorkflow.♻️ Suggested refactor
- } as Partial<ComfyWorkflow> + } satisfies Partial<ComfyWorkflow> return wf as ComfyWorkflow#!/bin/bash # Verify TS version supports `satisfies` (>=4.9) and check existing usage. rg -n '"typescript"\s*:' package.json pnpm-lock.yaml 2>/dev/null | head -20 rg -n "satisfies" src --type ts --type tsx | head -20Based on learnings: In test files matching **/*.test.ts under src, when creating test helper functions that construct mock objects implementing an interface (e.g., AssetItem), prefer using satisfies InterfaceType for shape validation instead of type assertions like as Partial as InterfaceType or as any.
🤖 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 133 - 147, The helper createWorkflow currently casts a partial object to ComfyWorkflow using "as Partial<ComfyWorkflow> ... as ComfyWorkflow", which can hide shape drift; change the return construction to use TypeScript's "satisfies ComfyWorkflow" on the object literal so the compiler validates fields (keep the same properties: pendingWarnings, path, isLoaded, activeState, changeTracker and the options.loadable branch) and return the object as a ComfyWorkflow-compatible value without double assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/platform/workflow/core/services/workflowService.test.ts`:
- Around line 133-147: The helper createWorkflow currently casts a partial
object to ComfyWorkflow using "as Partial<ComfyWorkflow> ... as ComfyWorkflow",
which can hide shape drift; change the return construction to use TypeScript's
"satisfies ComfyWorkflow" on the object literal so the compiler validates fields
(keep the same properties: pendingWarnings, path, isLoaded, activeState,
changeTracker and the options.loadable branch) and return the object as a
ComfyWorkflow-compatible value without double assertions.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/composables/canvas/useSelectedLiteGraphItems.test.tssrc/platform/workflow/core/services/workflowService.test.tssrc/utils/__tests__/litegraphTestUtils.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/platform/workflow/core/services/workflowService.test.ts (1)
136-150: Prefersatisfiesover double casts for the test helper object.The current
as Partial<ComfyWorkflow> ... as ComfyWorkflowpattern hides missing fields. Usingsatisfiespreserves shape validation while keeping the test helper lightweight.♻️ Proposed refactor
- const wf = { + const wf = { pendingWarnings: warnings, ...(options.loadable && { path: options.path ?? 'workflows/test.json', isLoaded: true, activeState: { nodes: [], links: [] }, changeTracker: { reset: vi.fn(), restore: vi.fn() } }) - } as Partial<ComfyWorkflow> + } satisfies Partial<ComfyWorkflow> return wf as ComfyWorkflow }#!/bin/bash # Confirm TypeScript version supports the `satisfies` operator (TS 4.9+). rg -n '"typescript"' package.json pnpm-lock.yamlBased on learnings "In test files matching **/*.test.ts under src, when creating test helper functions that construct mock objects implementing an interface (e.g., AssetItem), prefer using satisfies InterfaceType for shape validation instead of type assertions like as Partial as InterfaceType or as any."
🤖 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 136 - 150, The test helper createWorkflow uses double type assertions (as Partial<ComfyWorkflow> ... as ComfyWorkflow) which hides missing fields; replace the double cast with TypeScript's satisfies operator to enforce shape validation while keeping the object lightweight—update the created wf object expression to end with "satisfies Partial<ComfyWorkflow>" (or "satisfies ComfyWorkflow" if you include all required fields) instead of the two as-casts, keeping the same conditional loadable properties (path, isLoaded, activeState, changeTracker) and preserving pendingWarnings so the compiler validates the structure of createWorkflow against ComfyWorkflow.
🤖 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/useBuilderSave.ts`:
- Around line 39-46: The auto-resave path currently swallows errors in
useBuilderSave.ts (inside the branch handling !workflow.isTemporary &&
workflow.initialMode != null) so users get no feedback; update the catch block
for the await workflowService.saveWorkflow(workflow) call to surface the failure
rather than silently calling resetSaving(): either show a localized error
toast/dialog using the existing showErrorDialog or toast helper (provide i18n
keys, e.g. builder.save.failed and builder.save.failedDetails) including
workflow.filename and workflow.initialMode context, then call resetSaving(); or
rethrow the error to be handled by a higher-level handler if that pattern is
used elsewhere—ensure you reference workflowService.saveWorkflow,
showSuccessDialog, resetSaving and keep behavior consistent with other save
error handling in this module.
In `@src/platform/workflow/core/services/workflowService.ts`:
- Around line 10-31: linearModeToAppMode currently treats any truthy value as
boolean which can mis-map corrupted values like the string "graph"; update
linearModeToAppMode to first verify the input is a boolean (e.g., typeof
linearMode === 'boolean') and only then return 'app' for true or 'graph' for
false, otherwise return null to avoid switching modes on invalid data; modify
the function linearModeToAppMode accordingly.
---
Duplicate comments:
In `@src/platform/workflow/core/services/workflowService.test.ts`:
- Around line 136-150: The test helper createWorkflow uses double type
assertions (as Partial<ComfyWorkflow> ... as ComfyWorkflow) which hides missing
fields; replace the double cast with TypeScript's satisfies operator to enforce
shape validation while keeping the object lightweight—update the created wf
object expression to end with "satisfies Partial<ComfyWorkflow>" (or "satisfies
ComfyWorkflow" if you include all required fields) instead of the two as-casts,
keeping the same conditional loadable properties (path, isLoaded, activeState,
changeTracker) and preserving pendingWarnings so the compiler validates the
structure of createWorkflow against ComfyWorkflow.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/components/builder/useBuilderSave.tssrc/components/topbar/WorkflowTab.vuesrc/composables/useAppMode.tssrc/composables/useCoreCommands.tssrc/locales/en/main.jsonsrc/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/LinearControls.vuesrc/renderer/extensions/linearMode/OutputHistoryActiveQueueItem.vuesrc/utils/__tests__/litegraphTestUtils.tssrc/views/GraphView.vue
💤 Files with no reviewable changes (1)
- src/composables/useCoreCommands.ts
✅ Files skipped from review due to trivial changes (1)
- src/platform/workflow/management/stores/workflowStore.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/composables/useAppMode.ts
- src/renderer/extensions/linearMode/OutputHistoryActiveQueueItem.vue
- src/renderer/core/canvas/canvasStore.ts
pythongosssss
force-pushed
the
pysssss/appmode-updates
branch
from
e3fa7b6 to
1b9ed06
Compare
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/renderer/extensions/linearMode/linearOutputStore.ts (1)
246-255:⚠️ Potential issue | 🟠 MajorBootstrap active job when app mode becomes active
On Line 248, when mode flips to active, you add the
executedlistener but do not initialize state for an already-runningexecutionStore.activeJobId. After a tab/mode switch, this can leavetrackedJobIdunset and drop previews/executed outputs for the current job.💡 Proposed fix
watch( isAppMode, (active, wasActive) => { if (active) { api.addEventListener('executed', handleExecuted) + const jobId = executionStore.activeJobId + if (jobId && trackedJobId.value !== jobId) { + onJobStart(jobId) + const url = jobPreviewStore.previewsByPromptId[jobId] + if (url) onLatentPreview(jobId, url) + } } else if (wasActive) { api.removeEventListener('executed', handleExecuted) reset() } }, { immediate: true } )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/extensions/linearMode/linearOutputStore.ts` around lines 246 - 255, When switching into app mode you add the executed listener but never bootstrap state for an already-running job, so ensure after api.addEventListener('executed', handleExecuted) you check executionStore.activeJobId and initialize trackedJobId and any preview/state for that job; specifically, set trackedJobId = executionStore.activeJobId (or call the existing initializer used in this module) and invoke the same logic that would run on an 'executed' event (e.g., call handleExecuted or the routine that loads previews/outputs) so previews aren’t dropped; keep reset() for the deactivation branch as-is.
♻️ Duplicate comments (1)
src/platform/workflow/core/services/workflowService.ts (1)
29-31:⚠️ Potential issue | 🟡 MinorValidate
linearModetype before converting it toAppMode.Line 30 currently treats any truthy value as
'app'. A malformed value (for example, a string) can flip mode unexpectedly.As per coding guidelines "Validate trusted sources before processing".🛠️ Proposed fix
function linearModeToAppMode(linearMode: unknown): AppMode | null { - return linearMode == null ? null : linearMode ? 'app' : 'graph' + if (typeof linearMode !== 'boolean') return null + return linearMode ? 'app' : 'graph' }🤖 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 29 - 31, The function linearModeToAppMode incorrectly treats any truthy value as 'app'; update it to validate that linearMode is either a boolean or null before converting: if linearMode === null return null; if typeof linearMode === 'boolean' return linearMode ? 'app' : 'graph'; otherwise return null (or handle as a malformed input per policy). Change the logic inside linearModeToAppMode to explicitly check typeof linearMode === 'boolean' so only true/false map to AppMode and other types are rejected.
🧹 Nitpick comments (2)
src/components/builder/IoItem.vue (1)
38-44: Hide the menu trigger when there are no actions.When both
renameandremoveare absent,entriesis empty but the trigger still renders. Consider gating<Popover>withentries.length > 0.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/builder/IoItem.vue` around lines 38 - 44, The Popover trigger still renders even when there are no actions because <Popover :entries> is always mounted; update the IoItem.vue template to conditionally render the Popover only when the entries array has items (e.g., check entries && entries.length > 0) so the <template `#button`> and <Button> (icon-[lucide--ellipsis]) are not shown when both rename and remove are absent.src/renderer/extensions/linearMode/LinearControls.vue (1)
61-80: MakemappedSelectionsorder-independent.Current grouping relies on contiguous
selectedInputsfor the same node. Grouping bynodeIdfirst will make it deterministic and easier to maintain.As per coding guidelines "Use refactoring techniques to make complex code simpler".♻️ Suggested refactor
-import { partition, remove, takeWhile } from 'es-toolkit' +import { partition } from 'es-toolkit' @@ const mappedSelections = computed(() => { - let unprocessedInputs = [...appModeStore.selectedInputs] - //FIXME strict typing here - const processedInputs: ReturnType<typeof nodeToNodeData>[] = [] - while (unprocessedInputs.length) { - const nodeId = unprocessedInputs[0][0] - const inputGroup = takeWhile( - unprocessedInputs, - ([id]) => id === nodeId - ).map(([, widgetName]) => widgetName) - unprocessedInputs = unprocessedInputs.slice(inputGroup.length) - const node = app.rootGraph.getNodeById(nodeId) - if (!node) continue - - const nodeData = nodeToNodeData(node) - remove(nodeData.widgets ?? [], (w) => !inputGroup.includes(w.name)) - processedInputs.push(nodeData) - } - return processedInputs + const inputsByNodeId = new Map<number, Set<string>>() + for (const [nodeId, widgetName] of appModeStore.selectedInputs) { + let names = inputsByNodeId.get(nodeId) + if (!names) { + names = new Set<string>() + inputsByNodeId.set(nodeId, names) + } + names.add(widgetName) + } + + const processedInputs: ReturnType<typeof nodeToNodeData>[] = [] + for (const [nodeId, names] of inputsByNodeId) { + const node = app.rootGraph.getNodeById(nodeId) + if (!node) continue + const nodeData = nodeToNodeData(node) + nodeData.widgets = (nodeData.widgets ?? []).filter((w) => + names.has(w.name) + ) + processedInputs.push(nodeData) + } + return processedInputs })🤖 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 61 - 80, mappedSelections currently assumes appModeStore.selectedInputs for the same node are contiguous and uses takeWhile to group, which breaks if order changes; replace that logic by first building a Map (or record) keyed by nodeId that accumulates widgetNames from appModeStore.selectedInputs, then iterate the map entries, call app.rootGraph.getNodeById(nodeId) and nodeToNodeData(node), filter nodeData.widgets to only include names from the accumulated widget list (use remove or Array.prototype.filter), and push the resulting nodeData into processedInputs so grouping is deterministic and order-independent; update the mappedSelections computed to use this Map-based grouping and preserve the processedInputs type and existing helpers (nodeToNodeData, remove).
🤖 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 49-50: Guard access to node.inputs before calling .find(): replace
the unsafe lookup with a safe check such as const input =
Array.isArray(node.inputs) ? node.inputs.find((i) => i.widget?.name ===
widget.name) : undefined (or use node.inputs?.find if optional chaining is
preferred), and then keep const rename = input && (() => renameWidget(widget,
input)); this prevents runtime errors when node.inputs is absent while still
using the existing symbols node.inputs, input, rename, renameWidget, and widget.
- Around line 166-170: The computed renderedOutputs currently uses a non-null
assertion on canvas.graph (canvas.graph!.nodes) which can crash if graph is
transiently null; update renderedOutputs to guard access to canvas.graph (e.g.,
check canvas.graph before using .nodes or use optional chaining and a fallback
empty array) and still map through nodeToDisplayTuple, keeping the
appModeStore.selectedOutputs reference if needed—ensure the computed returns an
empty array when canvas.graph is null/undefined to avoid runtime errors.
- Line 41: The setup currently calls
workflowStore.activeWorkflow?.changeTracker?.reset(), which clears
unsaved-change tracking on entering builder mode; remove this call so entering
builder does not drop modification state. Locate the reset invocation
(workflowStore.activeWorkflow?.changeTracker?.reset()) in AppBuilder.vue and
delete it or restrict it to only run in explicit new-workflow flows (e.g.,
inside a createNewWorkflow or initializeNewWorkflow handler) so existing
workflows retain their changeTracker state.
In `@src/components/builder/IoItem.vue`:
- Around line 40-42: The icon-only Button in IoItem.vue (the Button wrapping <i
class="icon-[lucide--ellipsis]"/>) needs an accessible name: update that Button
instance to include a clear aria-label (e.g. aria-label="More actions" or
aria-label="Open options") or supply visually-hidden text inside the Button
(screen-reader-only span) so assistive tech can announce it; do not add
aria-labels to any sibling text buttons—only add the label to this icon-only
trigger.
In `@src/components/common/DraggableList.vue`:
- Around line 21-45: The reorder logic can call splice with invalid indices when
oldPosition or newPosition is -1; before mutating modelValue in the block using
oldPosition, newPosition, modelValue.value and draggableItem, check and guard:
compute oldPosition via the loop (or fallback using
modelValue.value.indexOf(this.draggableItem)), compute newPosition from
reorderedItems.indexOf(this.draggableItem), and if either is -1 or equal (no-op)
return early; otherwise proceed with const itemList = modelValue.value,
splice(oldPosition, 1) and splice(newPosition, 0, item) to update modelValue.
Ensure you reference getAllItems, reorderedItems, isItemToggled and isItemAbove
only for building reorderedItems and do not perform splices when indices are
invalid.
In `@src/stores/appModeStore.ts`:
- Around line 17-19: The store currently exports selectedInputs and
selectedOutputs as reactive arrays which breaks v-model reassignment from
DraggableList; change both to use ref<...>([]) instead of reactive(...), update
hasOutputs to use hasOutputs = computed(() => !!selectedOutputs.value.length),
and update all internal manipulations (push, splice, spreads, assignments) to
operate on .value (e.g., selectedOutputs.value = [...]) so the v-model bindings
in AppBuilder.vue work correctly and the proxy reference remains intact.
---
Outside diff comments:
In `@src/renderer/extensions/linearMode/linearOutputStore.ts`:
- Around line 246-255: When switching into app mode you add the executed
listener but never bootstrap state for an already-running job, so ensure after
api.addEventListener('executed', handleExecuted) you check
executionStore.activeJobId and initialize trackedJobId and any preview/state for
that job; specifically, set trackedJobId = executionStore.activeJobId (or call
the existing initializer used in this module) and invoke the same logic that
would run on an 'executed' event (e.g., call handleExecuted or the routine that
loads previews/outputs) so previews aren’t dropped; keep reset() for the
deactivation branch as-is.
---
Duplicate comments:
In `@src/platform/workflow/core/services/workflowService.ts`:
- Around line 29-31: The function linearModeToAppMode incorrectly treats any
truthy value as 'app'; update it to validate that linearMode is either a boolean
or null before converting: if linearMode === null return null; if typeof
linearMode === 'boolean' return linearMode ? 'app' : 'graph'; otherwise return
null (or handle as a malformed input per policy). Change the logic inside
linearModeToAppMode to explicitly check typeof linearMode === 'boolean' so only
true/false map to AppMode and other types are rejected.
---
Nitpick comments:
In `@src/components/builder/IoItem.vue`:
- Around line 38-44: The Popover trigger still renders even when there are no
actions because <Popover :entries> is always mounted; update the IoItem.vue
template to conditionally render the Popover only when the entries array has
items (e.g., check entries && entries.length > 0) so the <template `#button`> and
<Button> (icon-[lucide--ellipsis]) are not shown when both rename and remove are
absent.
In `@src/renderer/extensions/linearMode/LinearControls.vue`:
- Around line 61-80: mappedSelections currently assumes
appModeStore.selectedInputs for the same node are contiguous and uses takeWhile
to group, which breaks if order changes; replace that logic by first building a
Map (or record) keyed by nodeId that accumulates widgetNames from
appModeStore.selectedInputs, then iterate the map entries, call
app.rootGraph.getNodeById(nodeId) and nodeToNodeData(node), filter
nodeData.widgets to only include names from the accumulated widget list (use
remove or Array.prototype.filter), and push the resulting nodeData into
processedInputs so grouping is deterministic and order-independent; update the
mappedSelections computed to use this Map-based grouping and preserve the
processedInputs type and existing helpers (nodeToNodeData, remove).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
src/components/LiteGraphCanvasSplitterOverlay.vuesrc/components/appMode/AppModeToolbar.vuesrc/components/builder/AppBuilder.vuesrc/components/builder/BuilderToolbar.vuesrc/components/builder/IoItem.vuesrc/components/builder/useBuilderSave.tssrc/components/common/DraggableList.vuesrc/components/graph/GraphCanvas.vuesrc/components/rightSidePanel/subgraph/SubgraphEditor.vuesrc/components/rightSidePanel/subgraph/SubgraphNodeWidget.vuesrc/components/sidebar/tabs/SidebarTabTemplate.vuesrc/components/topbar/WorkflowTab.vuesrc/components/ui/TypeformPopoverButton.vuesrc/composables/useAppMode.tssrc/composables/useCoreCommands.tssrc/lib/litegraph/src/types/widgets.tssrc/locales/en/main.jsonsrc/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/platform/workflow/validation/schemas/workflowSchema.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/stores/appModeStore.tssrc/utils/__tests__/litegraphTestUtils.tssrc/views/GraphView.vuesrc/views/LinearView.vue
💤 Files with no reviewable changes (2)
- src/composables/useCoreCommands.ts
- src/lib/litegraph/src/types/widgets.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- src/components/topbar/WorkflowTab.vue
- src/utils/tests/litegraphTestUtils.ts
- src/renderer/extensions/linearMode/linearOutputStore.test.ts
- src/renderer/extensions/linearMode/OutputHistory.vue
- src/renderer/extensions/linearMode/LinearPreview.vue
- src/components/ui/TypeformPopoverButton.vue
- src/components/sidebar/tabs/SidebarTabTemplate.vue
- src/renderer/extensions/linearMode/OutputHistoryActiveQueueItem.vue
- src/composables/useAppMode.ts
- src/components/appMode/AppModeToolbar.vue
- src/components/builder/useBuilderSave.ts
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/builder/useBuilderSave.ts (1)
79-83:⚠️ Potential issue | 🟡 Minor
savingis never reset on the early return whenworkflowisnull.If
workflowStore.activeWorkflowisnullwhenhandleSaveis invoked, the function returns at line 82 without callingresetSaving()orcloseSaveDialog().savingstaystrueand the save dialog remains open until the user manually dismisses it (wheredialogComponentProps.onClose: resetSavingeventually fires). The same guard should clean up state.🐛 Proposed fix
async function handleSave(filename: string, openAsApp: boolean) { try { const workflow = workflowStore.activeWorkflow - if (!workflow) return + if (!workflow) { + closeSaveDialog() + resetSaving() + return + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/builder/useBuilderSave.ts` around lines 79 - 83, In handleSave, if workflowStore.activeWorkflow is null the function returns early but never resets the component state; update handleSave to call resetSaving() and closeSaveDialog() (or run the existing cleanup) before returning when workflow is null, or wrap the try body in a try/finally and move resetSaving()/closeSaveDialog() into finally so saving is always cleared and the dialog closed; reference the handleSave function, workflowStore.activeWorkflow guard, and the resetSaving/closeSaveDialog helpers when making the change.
♻️ Duplicate comments (1)
src/renderer/extensions/linearMode/OutputHistoryActiveQueueItem.vue (1)
41-41:⚠️ Potential issue | 🟡 MinorDisable “Clear Pending Tasks” when
queueCount <= 1.On Line 41,
:disabled="queueCount === 0"still enables the action when only the active task exists (queueCount === 1). This should be non-actionable until there are actual pending tasks.Suggested fix
- :disabled="queueCount === 0" + :disabled="queueCount <= 1"Based on learnings: In the ComfyUI frontend queue system (
useQueuePendingTaskCountStore().count), count includes the currently executing task, and “Clear Pending Tasks” should only be enabled when count > 1.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/extensions/linearMode/OutputHistoryActiveQueueItem.vue` at line 41, The "Clear Pending Tasks" button is currently disabled only when queueCount === 0 but must be disabled when there are no pending tasks (queueCount <= 1). In OutputHistoryActiveQueueItem.vue update the :disabled binding for the clear action (the element using :disabled="queueCount === 0") to check queueCount <= 1 instead so the action remains non-actionable when only the active task exists; keep the existing reactive variable name queueCount and adjust only the boolean expression.
🧹 Nitpick comments (3)
src/components/builder/useBuilderSave.ts (1)
26-34: Consider replacing the boolean-trigger watch pattern with a direct function call.The watch fires
onBuilderSavewheneversavingflips totrue, making the trigger mechanism indirect — callers manipulate a boolean flag to initiate side effects rather than calling a function. Collapsing the trigger intosetSavingdirectly keeps the intent explicit and removes the watcher.♻️ Proposed refactor
- const saving = ref(false) - - watch(saving, (value) => { - if (value) void onBuilderSave() - }) - - function setSaving(value: boolean) { - saving.value = value - } + const saving = ref(false) + + async function setSaving(value: boolean) { + saving.value = value + if (value) await onBuilderSave() + }This eliminates the watcher, makes the async nature of the trigger visible at the call site, and removes one reactive dependency from the composable. Callers that use
void setSaving(true)get the same fire-and-forget behaviour.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/builder/useBuilderSave.ts` around lines 26 - 34, The current watch on saving triggers onBuilderSave indirectly; remove the watch and make setSaving perform the trigger explicitly: keep the saving = ref(false) and the setSaving(value: boolean) function, but delete the watch(saving...) block and modify setSaving so that it updates saving.value and, if value is true, calls onBuilderSave() (use void onBuilderSave() for fire-and-forget or await it if you need sequential behavior). This keeps the saving state but collapses the trigger into setSaving and preserves callers that do setSaving(true).src/platform/workflow/core/services/workflowService.ts (2)
133-144: RedundantsyncLinearMode/checkState()in the self-overwrite branch.When
isSelfOverwriteistrue, lines 133–134 runsyncLinearModeandchangeTracker?.checkState(), then immediately delegate tosaveWorkflow(workflow)(line 137), which is a non-temporary workflow and therefore runs the same two calls again at lines 157–158. The sync and state-check are effectively applied twice with no intermediate state change.♻️ Proposed refactor — skip pre-sync in the self-overwrite sub-path
Extract the
syncLinearMode+checkStatepair into the branches that need them rather than calling unconditionally before the branch:- if (options.initialMode) workflow.initialMode = options.initialMode - - syncLinearMode(workflow, [app.rootGraph], { flushLinearData: true }) - workflow.changeTracker?.checkState() - - if (isSelfOverwrite) { - await saveWorkflow(workflow) + if (options.initialMode) workflow.initialMode = options.initialMode + + if (isSelfOverwrite) { + await saveWorkflow(workflow) // saveWorkflow syncs + checkState internally } else if (workflow.isTemporary) { + syncLinearMode(workflow, [app.rootGraph], { flushLinearData: true }) + workflow.changeTracker?.checkState() await renameWorkflow(workflow, newPath) await workflowStore.saveWorkflow(workflow) } else { + syncLinearMode(workflow, [app.rootGraph], { flushLinearData: true }) + workflow.changeTracker?.checkState() const tempWorkflow = workflowStore.saveAs(workflow, newPath) await openWorkflow(tempWorkflow) await workflowStore.saveWorkflow(tempWorkflow) }🤖 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 133 - 144, The pre-branch calls to syncLinearMode(...) and workflow.changeTracker?.checkState() are being run unconditionally and then run again inside saveWorkflow when isSelfOverwrite is true; to fix, remove the unconditional pre-sync and instead call syncLinearMode(workflow, [app.rootGraph], { flushLinearData: true }) and workflow.changeTracker?.checkState() only in the branches that actually need them (i.e., before renameWorkflow + workflowStore.saveWorkflow for workflow.isTemporary==true and before creating tempWorkflow/openWorkflow/saveWorkflow for the non-self-overwrite save-as path), leaving the saveWorkflow(workflow) path for isSelfOverwrite untouched; use the existing symbols syncLinearMode, changeTracker?.checkState, saveWorkflow, renameWorkflow, workflowStore.saveAs, openWorkflow, workflow.isTemporary and isSelfOverwrite to locate and apply the change.
133-144: RedundantsyncLinearMode/checkState()in the self-overwrite branch.When
isSelfOverwriteistrue, lines 133–134 runsyncLinearModeandchangeTracker?.checkState(), then delegate tosaveWorkflow(workflow)(line 137), which — for a non-temporary workflow — immediately repeats both calls at lines 157–158 with no state change in between.♻️ Proposed refactor
if (options.initialMode) workflow.initialMode = options.initialMode - syncLinearMode(workflow, [app.rootGraph], { flushLinearData: true }) - workflow.changeTracker?.checkState() - if (isSelfOverwrite) { - await saveWorkflow(workflow) + // saveWorkflow calls syncLinearMode + checkState internally + await saveWorkflow(workflow) } else if (workflow.isTemporary) { + syncLinearMode(workflow, [app.rootGraph], { flushLinearData: true }) + workflow.changeTracker?.checkState() await renameWorkflow(workflow, newPath) await workflowStore.saveWorkflow(workflow) } else { + syncLinearMode(workflow, [app.rootGraph], { flushLinearData: true }) + workflow.changeTracker?.checkState() const tempWorkflow = workflowStore.saveAs(workflow, newPath) await openWorkflow(tempWorkflow) await workflowStore.saveWorkflow(tempWorkflow) }🤖 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 133 - 144, The calls to syncLinearMode(...) and workflow.changeTracker?.checkState() are duplicated when isSelfOverwrite is true because saveWorkflow(workflow) repeats them; remove the redundant pre-save invocation in the isSelfOverwrite branch (or alternatively ensure syncLinearMode/checkState are only performed inside saveWorkflow) so that syncLinearMode, changeTracker?.checkState(), and subsequent persistence happen exactly once for saveWorkflow; touch the code paths around syncLinearMode, saveWorkflow(workflow), renameWorkflow(workflow, newPath), workflowStore.saveAs(workflow, newPath), openWorkflow(tempWorkflow), and workflowStore.saveWorkflow(...) to centralize the sync/check calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/components/builder/useBuilderSave.ts`:
- Around line 79-83: In handleSave, if workflowStore.activeWorkflow is null the
function returns early but never resets the component state; update handleSave
to call resetSaving() and closeSaveDialog() (or run the existing cleanup) before
returning when workflow is null, or wrap the try body in a try/finally and move
resetSaving()/closeSaveDialog() into finally so saving is always cleared and the
dialog closed; reference the handleSave function, workflowStore.activeWorkflow
guard, and the resetSaving/closeSaveDialog helpers when making the change.
---
Duplicate comments:
In `@src/renderer/extensions/linearMode/OutputHistoryActiveQueueItem.vue`:
- Line 41: The "Clear Pending Tasks" button is currently disabled only when
queueCount === 0 but must be disabled when there are no pending tasks
(queueCount <= 1). In OutputHistoryActiveQueueItem.vue update the :disabled
binding for the clear action (the element using :disabled="queueCount === 0") to
check queueCount <= 1 instead so the action remains non-actionable when only the
active task exists; keep the existing reactive variable name queueCount and
adjust only the boolean expression.
---
Nitpick comments:
In `@src/components/builder/useBuilderSave.ts`:
- Around line 26-34: The current watch on saving triggers onBuilderSave
indirectly; remove the watch and make setSaving perform the trigger explicitly:
keep the saving = ref(false) and the setSaving(value: boolean) function, but
delete the watch(saving...) block and modify setSaving so that it updates
saving.value and, if value is true, calls onBuilderSave() (use void
onBuilderSave() for fire-and-forget or await it if you need sequential
behavior). This keeps the saving state but collapses the trigger into setSaving
and preserves callers that do setSaving(true).
In `@src/platform/workflow/core/services/workflowService.ts`:
- Around line 133-144: The pre-branch calls to syncLinearMode(...) and
workflow.changeTracker?.checkState() are being run unconditionally and then run
again inside saveWorkflow when isSelfOverwrite is true; to fix, remove the
unconditional pre-sync and instead call syncLinearMode(workflow,
[app.rootGraph], { flushLinearData: true }) and
workflow.changeTracker?.checkState() only in the branches that actually need
them (i.e., before renameWorkflow + workflowStore.saveWorkflow for
workflow.isTemporary==true and before creating
tempWorkflow/openWorkflow/saveWorkflow for the non-self-overwrite save-as path),
leaving the saveWorkflow(workflow) path for isSelfOverwrite untouched; use the
existing symbols syncLinearMode, changeTracker?.checkState, saveWorkflow,
renameWorkflow, workflowStore.saveAs, openWorkflow, workflow.isTemporary and
isSelfOverwrite to locate and apply the change.
- Around line 133-144: The calls to syncLinearMode(...) and
workflow.changeTracker?.checkState() are duplicated when isSelfOverwrite is true
because saveWorkflow(workflow) repeats them; remove the redundant pre-save
invocation in the isSelfOverwrite branch (or alternatively ensure
syncLinearMode/checkState are only performed inside saveWorkflow) so that
syncLinearMode, changeTracker?.checkState(), and subsequent persistence happen
exactly once for saveWorkflow; touch the code paths around syncLinearMode,
saveWorkflow(workflow), renameWorkflow(workflow, newPath),
workflowStore.saveAs(workflow, newPath), openWorkflow(tempWorkflow), and
workflowStore.saveWorkflow(...) to centralize the sync/check calls.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/builder/useBuilderSave.tssrc/platform/workflow/core/services/workflowService.tssrc/renderer/extensions/linearMode/OutputHistoryActiveQueueItem.vue
- fix sizing of sidebars in app mode - update feedback button to match design - update job queue notification - clickable queue spinner item to allow clear queue - refactor mode out of store to specific workflow instance - support different saved vs active mode - other styling/layout tweaks
- fix hiding panel on builder:select mode - fix hasOutputs reactivity
pythongosssss
force-pushed
the
pysssss/appmode-updates
branch
from
2214f48 to
46a7108
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/ui/TypeformPopoverButton.vue (1)
22-32:⚠️ Potential issue | 🟠 MajorAdd an accessible name to both icon-only feedback buttons.
Both branches render icon-only controls without an accessible name, so screen readers cannot identify the action.
💡 Suggested fix
<Button v-if="isMobile" as="a" :href="`https://form.typeform.com/to/${dataTfWidget}`" target="_blank" variant="inverted" + :aria-label="$t('linearMode.giveFeedback')" class="flex h-10 items-center justify-center gap-2.5 px-3 py-2" v-bind="$attrs" > <i class="icon-[lucide--circle-help] size-4" /> </Button> @@ <Button variant="inverted" + :aria-label="$t('linearMode.giveFeedback')" class="flex h-10 items-center justify-center gap-2.5 px-3 py-2" v-bind="$attrs" > <i class="icon-[lucide--circle-help] size-4" /> </Button>Based on learnings: in Vue components, use
aria-labelfor icon-only buttons because they have no visible accessible name.Also applies to: 35-41
🤖 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 - 32, The icon-only controls in TypeformPopoverButton.vue (the mobile <Button as="a"> branch and the desktop <Popover><PopoverTrigger> branch) lack accessible names; add descriptive aria-label attributes to both interactive elements (e.g., aria-label="Open feedback form" or similar) so screen readers can identify the action—update the <Button> in the isMobile branch and the <PopoverTrigger> (or the element it renders) in the non-mobile branch to include the aria-label.src/platform/workflow/core/services/workflowService.ts (1)
112-146:⚠️ Potential issue | 🟡 MinorSelf-overwrite triggers the overwrite confirmation dialog
The confirm dialog (Lines 115–123) fires for all cases where
existingWorkflow && !existingWorkflow.isTemporary, including whenisSelfOverwriteistrue. A user re-saving a file under its own name will be prompted to confirm overwriting themselves, which is unexpected UX.🛠️ Suggested fix
- if (existingWorkflow && !existingWorkflow.isTemporary) { + if (existingWorkflow && !existingWorkflow.isTemporary && !isSelfOverwrite) { const res = await dialogService.confirm({ title: t('sideToolbar.workflowTab.confirmOverwriteTitle'), type: 'overwrite', message: t('sideToolbar.workflowTab.confirmOverwrite'), itemList: [newPath] }) if (res !== true) return false - if (!isSelfOverwrite) { const deleted = await deleteWorkflow(existingWorkflow, true) if (!deleted) return false - } }🤖 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 112 - 146, The confirm dialog is shown even for self-overwrites; change the conditional so dialogService.confirm is only called when an existing non-temporary workflow is NOT the same workflow (i.e., require existingWorkflow && !existingWorkflow.isTemporary && !isSelfOverwrite). Keep the isSelfOverwrite calculation as-is, skip the deleteWorkflow block for self-overwrite, and ensure the existing branches that handle isSelfOverwrite (calling saveWorkflow) remain unchanged; update the if that wraps dialogService.confirm and the subsequent deleteWorkflow flow to only run when !isSelfOverwrite so self-saves proceed directly to saveWorkflow without prompting.
♻️ Duplicate comments (1)
src/stores/appModeStore.ts (1)
17-19:⚠️ Potential issue | 🔴 Critical
reactivearrays breakv-modelreassignment fromDraggableList
selectedInputsandselectedOutputsare still declared asreactive<...>([]). WhenDraggableListinAppBuilder.vueemitsupdate:modelValuewith a new array, the v-model write (store.selectedInputs = newArray) replaces the exported proxy reference with a plain array, losing reactivity.Use
refinstead and update all access sites to.value:🔧 Suggested fix
-import { reactive, computed, watch } from 'vue' +import { ref, computed, watch } from 'vue' - const selectedInputs = reactive<[NodeId, string][]>([]) - const selectedOutputs = reactive<NodeId[]>([]) - const hasOutputs = computed(() => !!selectedOutputs.length) + const selectedInputs = ref<[NodeId, string][]>([]) + const selectedOutputs = ref<NodeId[]>([]) + const hasOutputs = computed(() => selectedOutputs.value.length > 0) function loadSelections(data: Partial<LinearData> | undefined) { - selectedInputs.splice(0, selectedInputs.length, ...(data?.inputs ?? [])) - selectedOutputs.splice(0, selectedOutputs.length, ...(data?.outputs ?? [])) + selectedInputs.value.splice(0, selectedInputs.value.length, ...(data?.inputs ?? [])) + selectedOutputs.value.splice(0, selectedOutputs.value.length, ...(data?.outputs ?? [])) } function flushSelections() { const workflow = workflowStore.activeWorkflow if (workflow) { workflow.dirtyLinearData = { - inputs: [...selectedInputs], - outputs: [...selectedOutputs] + inputs: [...selectedInputs.value], + outputs: [...selectedOutputs.value] } } } // In the watch: - oldWorkflow.dirtyLinearData = { - inputs: [...selectedInputs], - outputs: [...selectedOutputs] - } + oldWorkflow.dirtyLinearData = { + inputs: [...selectedInputs.value], + outputs: [...selectedOutputs.value] + }As per coding guidelines,
src/**/*.ts: "Usereffor reactive state". Based on learnings, this pattern (reactive arrays exported via v-model) breaks proxy reference on reassignment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stores/appModeStore.ts` around lines 17 - 19, selectedInputs and selectedOutputs are created with reactive([]) which allows a v-model write from DraggableList (update:modelValue) to replace the exported proxy with a plain array and lose reactivity; change both to use ref with proper generics (e.g. ref<Array<[NodeId,string]>> and ref<NodeId[]>) and update all access sites and assignments to use .value (including the v-model binding sites in AppBuilder.vue / DraggableList and any reads/writes), and update hasOutputs computed to reference selectedOutputs.value.length (or !!selectedOutputs.value.length); ensure exported types and usages reflect the .value access so reactivity is preserved on reassignment.
🧹 Nitpick comments (1)
src/platform/workflow/core/services/workflowService.test.ts (1)
140-150: Prefersatisfiesover double cast increateWorkflow.The
as Partial<ComfyWorkflow> as ComfyWorkflowpattern silently bypasses shape-checking. Usingsatisfiespreserves compile-time validation of the provided fields.♻️ Proposed refactor
function createWorkflow( warnings: PendingWarnings | null = null, options: { loadable?: boolean; path?: string } = {} ): ComfyWorkflow { - const wf = { + const wf = ({ pendingWarnings: warnings, ...(options.loadable && { path: options.path ?? 'workflows/test.json', isLoaded: true, activeState: { nodes: [], links: [] }, changeTracker: { reset: vi.fn(), restore: vi.fn() } }) - } as Partial<ComfyWorkflow> - return wf as ComfyWorkflow + } satisfies Partial<ComfyWorkflow>) as ComfyWorkflow }🤖 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, Replace the silent double-cast in createWorkflow by using TypeScript's satisfies operator so the returned object is validated against ComfyWorkflow without losing the final ComfyWorkflow return type; change the construction that currently uses "as Partial<ComfyWorkflow> as ComfyWorkflow" to build the object as a Partial shape and append "satisfies ComfyWorkflow" (or use an intermediate const typed with satisfies) so the compiler verifies fields while still returning the object as ComfyWorkflow from createWorkflow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/components/ui/TypeformPopoverButton.vue`:
- Around line 22-32: The icon-only controls in TypeformPopoverButton.vue (the
mobile <Button as="a"> branch and the desktop <Popover><PopoverTrigger> branch)
lack accessible names; add descriptive aria-label attributes to both interactive
elements (e.g., aria-label="Open feedback form" or similar) so screen readers
can identify the action—update the <Button> in the isMobile branch and the
<PopoverTrigger> (or the element it renders) in the non-mobile branch to include
the aria-label.
In `@src/platform/workflow/core/services/workflowService.ts`:
- Around line 112-146: The confirm dialog is shown even for self-overwrites;
change the conditional so dialogService.confirm is only called when an existing
non-temporary workflow is NOT the same workflow (i.e., require existingWorkflow
&& !existingWorkflow.isTemporary && !isSelfOverwrite). Keep the isSelfOverwrite
calculation as-is, skip the deleteWorkflow block for self-overwrite, and ensure
the existing branches that handle isSelfOverwrite (calling saveWorkflow) remain
unchanged; update the if that wraps dialogService.confirm and the subsequent
deleteWorkflow flow to only run when !isSelfOverwrite so self-saves proceed
directly to saveWorkflow without prompting.
---
Duplicate comments:
In `@src/stores/appModeStore.ts`:
- Around line 17-19: selectedInputs and selectedOutputs are created with
reactive([]) which allows a v-model write from DraggableList (update:modelValue)
to replace the exported proxy with a plain array and lose reactivity; change
both to use ref with proper generics (e.g. ref<Array<[NodeId,string]>> and
ref<NodeId[]>) and update all access sites and assignments to use .value
(including the v-model binding sites in AppBuilder.vue / DraggableList and any
reads/writes), and update hasOutputs computed to reference
selectedOutputs.value.length (or !!selectedOutputs.value.length); ensure
exported types and usages reflect the .value access so reactivity is preserved
on reassignment.
---
Nitpick comments:
In `@src/platform/workflow/core/services/workflowService.test.ts`:
- Around line 140-150: Replace the silent double-cast in createWorkflow by using
TypeScript's satisfies operator so the returned object is validated against
ComfyWorkflow without losing the final ComfyWorkflow return type; change the
construction that currently uses "as Partial<ComfyWorkflow> as ComfyWorkflow" to
build the object as a Partial shape and append "satisfies ComfyWorkflow" (or use
an intermediate const typed with satisfies) so the compiler verifies fields
while still returning the object as ComfyWorkflow from createWorkflow.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
src/components/LiteGraphCanvasSplitterOverlay.vuesrc/components/appMode/AppModeToolbar.vuesrc/components/builder/BuilderToolbar.vuesrc/components/builder/useBuilderSave.tssrc/components/graph/GraphCanvas.vuesrc/components/sidebar/tabs/SidebarTabTemplate.vuesrc/components/topbar/WorkflowTab.vuesrc/components/ui/TypeformPopoverButton.vuesrc/composables/useAppMode.tssrc/composables/useCoreCommands.tssrc/locales/en/main.jsonsrc/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/stores/appModeStore.tssrc/utils/__tests__/litegraphTestUtils.tssrc/views/GraphView.vuesrc/views/LinearView.vue
💤 Files with no reviewable changes (1)
- src/composables/useCoreCommands.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- src/renderer/extensions/linearMode/OutputHistory.vue
- src/renderer/extensions/linearMode/linearOutputStore.test.ts
- src/utils/tests/litegraphTestUtils.ts
- src/renderer/core/canvas/canvasStore.ts
- src/platform/workflow/management/stores/workflowStore.ts
- src/renderer/extensions/linearMode/OutputHistoryActiveQueueItem.vue
- src/renderer/extensions/linearMode/linearOutputStore.ts
- src/components/appMode/AppModeToolbar.vue
pythongosssss
added
the
New Browser Test Expectations
|
Updating Playwright Expectations |
pythongosssss
added
New Browser Test Expectations
github-actions
bot
removed
the
New Browser Test Expectations
dosubot
bot
added
the
size:XL
AustinMroz
previously approved these changes
Feb 25, 2026
Collaborator
AustinMroz
left a comment
AustinMroz
left a comment
There was a problem hiding this comment.
The complexity of the workflow serialzation changes is concerning, but I'm not seeing any actual bugs in the code or from testing.
⚡ Performance Report
Raw data{
"timestamp": "2026-02-26T14:14:55.438Z",
"gitSha": "613fe9f8ff1b821e72f987e53cf5a318de2da57a",
"branch": "pysssss/appmode-updates",
"measurements": [
{
"name": "canvas-idle",
"durationMs": 2048.7229999999954,
"styleRecalcs": 127,
"styleRecalcDurationMs": 22.777,
"layouts": 1,
"layoutDurationMs": 0.29200000000000004,
"taskDurationMs": 388.073,
"heapDeltaBytes": -1507192
},
{
"name": "canvas-mouse-sweep",
"durationMs": 1972.7169999999887,
"styleRecalcs": 179,
"styleRecalcDurationMs": 50.26100000000001,
"layouts": 12,
"layoutDurationMs": 3.8339999999999996,
"taskDurationMs": 874.412,
"heapDeltaBytes": -2354692
},
{
"name": "dom-widget-clipping",
"durationMs": 616.4200000000051,
"styleRecalcs": 44,
"styleRecalcDurationMs": 13.608,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 371.454,
"heapDeltaBytes": 7633868
}
]
} |
🎭 Playwright: ✅ 546 passed, 0 failed · 5 flaky📊 Browser Reports
|
AustinMroz
approved these changes
Feb 26, 2026
christian-byrne
pushed a commit
that referenced
this pull request
Feb 27, 2026
## Summary - fix sizing of sidebars in app mode - update feedback button to match design - update job queue notification - clickable queue spinner item to allow clear queue - refactor mode out of store to specific workflow instance - support different saved vs active mode - other styling/layout tweaks ## Changes - **What**: Changes the store to a composable and moves the mode state to the workflow. - This enables switching between tabs and maintaining the mode they were in ## Screenshots (if applicable) <img width="1866" height="1455" alt="image" src="https://github.com/user-attachments/assets/f9a8cd36-181f-4948-b48c-dd27bd9127cf" /> ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-9137-App-mode-more-updates-fixes-3106d73d365081a18ccff6ffe24fdec7) by [Unito](https://www.unito.io) --------- Co-authored-by: github-actions <github-actions@github.com>
This was referenced Mar 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Changes
Screenshots (if applicable)
┆Issue is synchronized with this Notion page by Unito