Release v1.9.0 — ComfyUI workflows as nodes

[shrimbly]

Promotes develop to master as v1.9.0. Merge #143 first — it carries the version bump and changelog.

ComfyUI workflows as nodes

Drop a ComfyUI workflow onto the canvas and it becomes a node, wired to the rest of a Node Banana pipeline.

• App Mode drives the surface. If the workflow was set up as an app in ComfyUI, the author's chosen inputs become typed handles, their widgets become inline settings, and their output nodes become typed outputs. Otherwise Node Banana detects them and asks you to confirm. Both upload formats work — the normal editor save and the API export.
• Three backends. Comfy Cloud (the default, nothing to install), a ComfyUI on this machine, or one elsewhere on the network. Chosen in Settings → ComfyUI and forwarded per request, so there is no server config.
• Blueprints. The ready-made pipelines your ComfyUI already ships, in their own tab — nothing to upload at all.
• Saved nodes. A configured Comfy node can be kept and comes back set up, not merely attached. It then appears in the canvas search, in the connection-drop menus for any handle type it matches, and in the dialog's own tab.
• Live previews. The node shows the latent forming while a run is going, instead of a spinner.
• Curve editor for ComfyUI's CURVE widget, and a dialog that lets you revisit an attached node's inputs, settings and outputs without starting over.

Also in this release

Two annotation-modal shortcut fixes (Delete no longer removes the node behind the modal; undo works in either case), one continuous outline around a running node including its settings panel, and a scrollbar restyle.

Review

Four CodeRabbit rounds, 33 findings triaged down to a clean pass. The four that mattered:

Bug	Consequence
A missing job-timeout header read as 0	Renders ran on a 1-minute timeout instead of 30 — cancelled mid-flight, GPU time spent, nothing returned
Video/audio outputs saved to the image store, hydrated from the generation store	Outputs came back empty after a reload
Required-input check and patch loop disagreed on "present"	An empty required input submitted a job with nothing patched in
Comfy app with outputs but no inputs	Dropped wires silently discarded

Findings not taken were replied to inline with reasons; CodeRabbit withdrew several after.

Known, and deliberately not fixed here

appInputHandles assigns positional handle ids (text-0), so reordering same-type inputs on an existing node can silently rebind an edge. The id is persisted in edge.targetHandle in every saved workflow, so changing it needs a load-time migration — its own change, not a release-eve edit.

Verification

2603 tests across 121 files, npm run build clean.

[coderabbitai]

Actionable comments posted: 11

Note

Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.

🟡 Minor comments (18)

README.md-100-100 (1)

100-100: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Separate workflow import from node creation.

Line 100 associates file dropping with Shift + C. Shift + C creates a new comfyApp node; dropping a workflow imports the file. Document these as separate actions so users do not expect the shortcut to load a workflow.

Suggested wording

-Any ComfyUI workflow can be dropped onto the canvas as a node (`Shift + C`).
+Drop any ComfyUI workflow onto the canvas to import it as a node. Press `Shift + C` to add a new ComfyUI app node without importing a file.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@README.md` at line 100, Update the README statement about dropping ComfyUI
workflows to separate workflow file import from node creation: document that
dropping a workflow imports it, while `Shift + C` creates a new `comfyApp` node,
without implying the shortcut loads a workflow.

src/app/globals.css-309-349 (1)

309-349: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Use kebab-case keyframe names.

Stylelint reports keyframes-name-pattern errors for dialogBackdropIn, dialogPanelIn, and dialogSectionIn. Rename the keyframes and update their animation references.

Suggested fix

-@keyframes dialogBackdropIn {
+@keyframes dialog-backdrop-in {
...
-@keyframes dialogPanelIn {
+@keyframes dialog-panel-in {
...
-@keyframes dialogSectionIn {
+@keyframes dialog-section-in {
...
-  animation: dialogBackdropIn 0.15s ease-out;
+  animation: dialog-backdrop-in 0.15s ease-out;
...
-  animation: dialogPanelIn 0.2s cubic-bezier(0.2, 0, 0, 1);
+  animation: dialog-panel-in 0.2s cubic-bezier(0.2, 0, 0, 1);
...
-  animation: dialogSectionIn 0.26s cubic-bezier(0.2, 0, 0, 1) both;
+  animation: dialog-section-in 0.26s cubic-bezier(0.2, 0, 0, 1) both;

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/app/globals.css` around lines 309 - 349, Rename the keyframes
dialogBackdropIn, dialogPanelIn, and dialogSectionIn to kebab-case names, and
update the corresponding animation declarations in .animate-dialog-backdrop,
.animate-dialog-panel, and .animate-dialog-section to reference the renamed
keyframes.

Source: Linters/SAST tools

src/components/AnnotationModal.tsx-112-140 (1)

112-140: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Let the active text input receive Escape when editing text.

When editingTextId is set and the inline input has focus, the capture handler still calls stopImmediatePropagation() for Escape before the input's onKeyDown runs. This path does not clear editingTextId, textInputPosition, or pendingTextPosition, so Escape can leave an empty annotation instead of closing the edit state. Return for the active input before the modal shortcut handling.

Add focused-input regression tests for Escape while editing an existing text annotation.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/components/AnnotationModal.tsx` around lines 112 - 140, Update
handleKeyDown so Escape returns immediately when editingTextId is set, allowing
the active text input to handle the key before modal shortcut logic runs; retain
closeModal behavior when not editing. Add focused-input regression tests
covering Escape while editing an existing text annotation.

src/lib/comfy/__tests__/engineErrors.test.ts-156-173 (1)

156-173: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Widen the timing margin to avoid a flaky assertion.

createEngineFetch sets deadline = Date.now() + timeoutMs * 2, and it checks that deadline only between attempts. With requestTimeoutMs: 40, one stalled attempt ends at about 40 ms and a second attempt can start just under the 80 ms deadline, so expected elapsed time is roughly 80–120 ms. The assertion allows 160 ms. That leaves about 40 ms of headroom. On a loaded CI runner, one scheduler stall or GC pause can exceed it and fail the run.

Raise requestTimeoutMs so the proportional margin grows. The assertion stays meaningful because it is still bounded by elapsed time rather than by the retry count.

🧪 Proposed change to widen the margin

-    const engineFetch = createEngineFetch({ retryBaseMs: 0, requestTimeoutMs: 40 });
+    const engineFetch = createEngineFetch({ retryBaseMs: 0, requestTimeoutMs: 200 });
     const began = Date.now();
     await expect(engineFetch("https://cloud.comfy.org/api/v2/jobs/job-1")).rejects.toThrow();
 
     // Bounded by elapsed time, not by the retry count: four retries of one
     // timeout would be five times the wait, and the caller checks its own
     // deadline only between requests.
-    expect(Date.now() - began).toBeLessThan(40 * 4);
+    expect(Date.now() - began).toBeLessThan(200 * 4);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/comfy/__tests__/engineErrors.test.ts` around lines 156 - 173,
Increase the requestTimeoutMs value used by createEngineFetch in the
stalled-request test so the proportional timing margin is larger and less
sensitive to CI scheduling delays. Keep the existing elapsed-time assertion and
retry-behavior coverage unchanged.

src/components/settings/ComfySettingsTab.tsx-48-52 (1)

48-52: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

The probe result survives changes to three connection fields.

The effect clears result for mode, cloudApiKey, localUrl, remoteUrl, and cloudUrl. It omits remoteApiKey, localUsesApiV2, and remoteUsesApiV2. Each of those changes the endpoint or the credential that the probe validated, so a green "Connected" label stays visible against a configuration that was never tested — the exact outcome the comment says to avoid.

🐛 Proposed fix for the dependency list

   useEffect(() => {
     setResult(null);
-  }, [settings.mode, settings.cloudApiKey, settings.localUrl, settings.remoteUrl, settings.cloudUrl]);
+  }, [
+    settings.mode,
+    settings.cloudApiKey,
+    settings.cloudUrl,
+    settings.localUrl,
+    settings.localUsesApiV2,
+    settings.remoteUrl,
+    settings.remoteApiKey,
+    settings.remoteUsesApiV2,
+  ]);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/components/settings/ComfySettingsTab.tsx` around lines 48 - 52, Update
the dependency list of the result-resetting useEffect in ComfySettingsTab to
include remoteApiKey, localUsesApiV2, and remoteUsesApiV2 alongside the existing
connection fields, ensuring any probe-relevant configuration change clears the
previous result.

src/lib/comfy/editor.ts-693-703 (1)

693-703: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Widget precedence contradicts the documented rule.

The doc comment on parseAppModeInputId (Line 626-627) states that the widget name carried by a WidgetId "is authoritative over element [1] when they disagree". Line 697 does the opposite: entry[1] wins whenever it is not null or undefined. For a WidgetId-encoded entry, a stale or renamed entry[1] then binds the control to the wrong widget key, and the binding is silently dropped later or written to the wrong input.

Either invert the precedence or correct the comment.

🐛 Proposed fix to honour the documented precedence

-    const widget = String(entry[1] ?? parsed.widget ?? "");
+    const widget = String(parsed.widget ?? entry[1] ?? "");

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/comfy/editor.ts` around lines 693 - 703, Update the input parsing
loop around parseAppModeInputId so parsed.widget takes precedence over entry[1]
when both provide values, matching the documented WidgetId behavior. Preserve
the fallback to entry[1] when parsed.widget is unavailable, then continue using
the resolved widget for deduplication and inputs.push.

src/components/nodes/ComfyAppParameters.tsx-158-250 (1)

158-250: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Associate each label with its control.

The <label> elements at Line 161, Line 203, and Line 233 wrap no control and set no htmlFor. Screen readers therefore announce these inputs without a name, and clicking the label does not move focus. The boolean branch at Line 185-196 is correct because it wraps its input.

Derive an id from param.id and pair it with htmlFor.

♿ Proposed fix for the number and text branch

 function ComfyParameterInputInner({ param, value, onChange }: ComfyParameterInputProps) {
   const label = shortLabel(param);
+  const controlId = `comfy-param-${param.id.replace(/[^a-zA-Z0-9_-]+/g, "-")}`;

-        <label className="text-[11px] text-neutral-400 shrink-0" title={param.description}>
+        <label htmlFor={controlId} className="text-[11px] text-neutral-400 shrink-0" title={param.description}>
           {label}
         </label>
         <div className="flex-1 min-w-0 flex items-center gap-1">
           <input
+            id={controlId}
             type={isNumber ? "number" : "text"}

Apply the same pairing to the select at Line 164 and the textarea at Line 206.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/components/nodes/ComfyAppParameters.tsx` around lines 158 - 250,
Associate every non-wrapping control label in the parameter renderer with its
control by deriving a stable id from param.id. Add matching htmlFor values to
the labels and id values to the select, textarea, and number/text input in the
enum, multiline, and default branches; leave the boolean branch unchanged
because its label already wraps the input.

src/components/nodes/ComfyCurveEditor.tsx-137-155 (1)

137-155: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

The point key changes during a drag and breaks pointer capture.

key={${index}-${point[0]}} includes the moving x coordinate. Every moveCurvePoint call for an interior point produces a new key, so React unmounts the old circle and mounts a new one. The browser releases pointer capture with the removed element, and the capture set at Line 154 is lost after the first move. Dragging then relies on the SVG-level onPointerMove, so tracking stops as soon as the pointer leaves the SVG box, and the hasPointerCapture check in endDrag no longer matches.

Key by index instead. The point count only changes on add and remove, where a full remount is correct.

🐛 Proposed fix for the unstable key

-              key={`${index}-${point[0]}`}
+              key={index}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/components/nodes/ComfyCurveEditor.tsx` around lines 137 - 155, Update the
circle key in the curve.points rendering within ComfyCurveEditor so it uses the
stable point index only, not the mutable point[0] coordinate. Preserve
remounting behavior when points are added or removed while keeping the same
circle mounted during dragging so pointer capture remains active.

src/lib/comfy/curve.ts-189-201 (1)

189-201: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Interior clamp can invert when neighbours are closer than 2 * MIN_POINT_GAP.

normalizeCurve only drops points whose x gap is below 1e-6, so an imported workflow can carry points 0.001 apart. For such a triple, lower exceeds upper at Line 196-197, and Math.min(Math.max(clamp01(x), lower), upper) returns upper — a value below the previous point. The returned points array is then unsorted, and sampleCurve plus curvePath both assume ascending x, so the plotted curve stops matching the stored value.

Clamp to the valid range only when it is non-empty.

🐛 Proposed fix for the inverted clamp

   if (!isEnd) {
     const lower = points[index - 1]![0] + MIN_POINT_GAP;
     const upper = points[index + 1]![0] - MIN_POINT_GAP;
-    point[0] = Math.min(Math.max(clamp01(x), lower), upper);
+    point[0] =
+      lower > upper
+        ? (points[index - 1]![0] + points[index + 1]![0]) / 2
+        : Math.min(Math.max(clamp01(x), lower), upper);
   }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/comfy/curve.ts` around lines 189 - 201, Update the interior-point
branch in moveCurvePoint so it only applies the lower/upper x clamp when the
computed range is non-empty (lower <= upper); when neighbours are too close and
the range is inverted, preserve the existing x value or otherwise avoid
producing an x below the previous point. Keep endpoint handling and y clamping
unchanged.

src/lib/comfy/inspect.ts-300-312 (1)

300-312: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Drop STRING boundary slots from connectable inputs if alsoBind must be preserved.

blueprintAppMode adds connectAs: "text" for all STRING slots, so inspect.ts turns them into ComfyAppInputs. alsoBind is kept only for parameters, and ComfyAppInput has no carry-through field, so a STRING boundary slot wiring multiple inputs can expose one text handle while only its primary target is bound at run time. Add an alsoBind carrier/assignment path for text inputs, or keep bound STRING slots as parameters instead of converting them to handles.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/comfy/inspect.ts` around lines 300 - 312, Update the
connectable-input handling in the inspect flow around connectAs and
inputFromCandidate so STRING slots with entry.alsoBind are not converted into
ComfyAppInputs that lose secondary bindings. Either preserve alsoBind through
the text-input carrier and runtime assignment path, or route bound STRING
candidates through the existing paramFromCandidate/params path while retaining
all alsoBind targets.

src/components/nodes/ComfyAppNode.tsx-406-414 (1)

406-414: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Handle a rejected file.text().

file.text() rejects when the browser cannot read the dropped file. The .then chain has no .catch, so the rejection is unhandled, the import dialog never opens, and the user sees nothing. Add a .catch that still opens the dialog so the failure is reported where every other bad file is reported.

🐛 Proposed fix

-        void file.text().then((text) => {
-          try {
-            onDropWorkflow({ workflow: JSON.parse(text), filename: file.name });
-          } catch {
-            // The dialog is where a bad file gets explained; it reports the
-            // same way whether the JSON or the workflow inside it is at fault.
-            onDropWorkflow({ workflow: text, filename: file.name });
-          }
-        });
+        void file
+          .text()
+          .then((text) => {
+            try {
+              onDropWorkflow({ workflow: JSON.parse(text), filename: file.name });
+            } catch {
+              // The dialog is where a bad file gets explained; it reports the
+              // same way whether the JSON or the workflow inside it is at fault.
+              onDropWorkflow({ workflow: text, filename: file.name });
+            }
+          })
+          .catch(() => {
+            onDropWorkflow({ workflow: null, filename: file.name });
+          });

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/components/nodes/ComfyAppNode.tsx` around lines 406 - 414, Add a
rejection handler to the file.text() promise chain in the drop workflow logic so
read failures still call onDropWorkflow with the file context and trigger the
import dialog’s existing error reporting path. Preserve the current JSON parsing
and fallback behavior for successfully read files.

Source: Linters/SAST tools

src/components/WorkflowCanvas.tsx-2092-2095 (1)

2092-2095: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Report a failed loadWorkflow.

reader.onload is async and await loadWorkflow(...) has no try/catch. A rejection becomes an unhandled promise rejection, and the user gets no message — unlike the parse failure at line 2089 and the unknown-format case at line 2106, which both alert.

🐛 Proposed fix

           if (isNodeBananaWorkflow(parsed)) {
-            await loadWorkflow(parsed as WorkflowFile);
+            try {
+              await loadWorkflow(parsed as WorkflowFile);
+            } catch (err) {
+              console.error("Failed to load workflow:", err);
+              alert("Failed to load workflow file");
+            }
             return;
           }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/components/WorkflowCanvas.tsx` around lines 2092 - 2095, Wrap the await
loadWorkflow(parsed as WorkflowFile) call inside the reader.onload handler in
try/catch, and alert the caught error using the same user-facing error handling
pattern as the parse-failure and unknown-format branches. Preserve the
successful return path while ensuring loadWorkflow rejections do not become
unhandled.

src/components/modals/ComfyWorkflowImportModal.tsx-355-359 (1)

355-359: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Re-check cancelled after readError resolves.

Line 355 checks cancelled before readError runs. readError awaits response.json(), so the effect can be cancelled during that await. setError and setMissingNodes then write state for a workflow the dialog no longer shows. The success path already re-checks at line 361; do the same on the error path.

🐛 Proposed fix

         if (cancelled) return;
         if (!response.ok) {
-          setError(await readError(response, "Could not re-read this workflow."));
+          const message = await readError(response, "Could not re-read this workflow.");
+          if (!cancelled) setError(message);
           return;
         }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/components/modals/ComfyWorkflowImportModal.tsx` around lines 355 - 359,
Update the error path in the workflow import effect around readError and
setError to re-check cancelled after readError resolves, before performing any
state updates. Preserve the existing response error handling and ensure
cancelled workflows do not call setError or setMissingNodes, matching the
success path’s cancellation guard.

Source: Linters/SAST tools

src/components/ConnectionDropMenu.tsx-761-767 (1)

761-767: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Remove Comfy app from 3D source options.

The 3D source branch connects a dragged 3D output to a new comfyApp node, but handleMenuSelect sets no output handle for comfyApp, so the dropped wire is discarded. A Comfy app contract has no 3d inputs, and getNodeHandles("comfyApp") only declares 3D as an output, so this entry belongs on the source-side output list rather than creating a node with a lost 3D input.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/components/ConnectionDropMenu.tsx` around lines 761 - 767, Remove the
`comfyApp` entry from the 3D source options in `ConnectionDropMenu`, leaving it
available only in the appropriate output/source-side list. Do not alter the
existing `handleMenuSelect` behavior or Comfy app node configuration.

src/lib/comfy/server/legacyEngine.ts-65-76 (1)

65-76: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Guard against a null output node, as collectOutputFiles does.

Line 70 reads .text from node directly. collectOutputFiles uses node ?? {} at line 53 for the same value from the same source. If the engine reports null for one output node, this function raises a TypeError and fails a run that had finished.

🛡️ Proposed fix

   for (const [nodeId, node] of Object.entries(outputs ?? {})) {
-    const raw = (node as { text?: unknown }).text;
+    const raw = (node ?? {} as { text?: unknown }).text;

Write it as const raw = (node as { text?: unknown } | null)?.text; if you prefer optional chaining.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/comfy/server/legacyEngine.ts` around lines 65 - 76, Update
collectOutputText so reading text from each output node is null-safe, matching
the guard used by collectOutputFiles. Adjust the raw text access to handle a
null node without throwing, while preserving the existing array filtering,
trimming, and result collection behavior.

src/lib/comfy/server/engine.ts-165-179 (1)

165-179: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Scan errors even when cause yields no code.

Line 170 returns as soon as cause is present. If that cause chain holds no code, the function returns null and never reaches the errors array at line 171. An AggregateError that carries both a code-less cause and per-address entries then reports no code.

createEngineFetch in src/lib/comfy/server/fetch.ts tests that code against CONNECT_FAILURE at line 198, so a missed code drops a safe connect-phase retry.

🐛 Proposed fix

   const cause = (error as { cause?: unknown }).cause;
-  if (cause) return errorCode(cause);
+  if (cause) {
+    const fromCause = errorCode(cause);
+    if (fromCause) return fromCause;
+  }
   const nested = (error as { errors?: unknown }).errors;

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/comfy/server/engine.ts` around lines 165 - 179, Update errorCode so a
present cause is checked recursively but does not terminate the search when it
returns null; continue scanning the errors array and return the first nested
code found. Preserve the existing null result when neither the cause chain nor
errors entries contain a code.

src/lib/comfy/server/run.ts-95-98 (1)

95-98: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

A cleared text parameter falls back to the author's value.

Line 97 skips any param whose value is "". The user then gets the value the workflow author saved, not an empty value. For a prompt or a filename parameter, that result is hard to explain: the field looks empty in the node, and the run uses different text.

The undefined and null checks match the documented intent at lines 67-73. The "" check goes further. Consider skipping only undefined and null, and treating "" as a deliberate clear for string-typed params.

🛡️ Proposed fix

   for (const param of app.params) {
     const value = params[param.id];
-    if (value === undefined || value === null || value === "") continue;
+    // `""` is a deliberate clear for a text param; only an absent value keeps
+    // the author's default.
+    if (value === undefined || value === null) continue;

Confirm the intent before you change it. A blank combo or numeric widget must still keep the author's value.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/comfy/server/run.ts` around lines 95 - 98, Update the parameter loop
in the run flow to skip only undefined and null values, allowing an empty string
to be assigned for string-typed parameters so cleared text fields remain
cleared. Preserve the author’s fallback for blank combo or numeric widgets by
validating the parameter type before accepting "" and continuing to omit it for
non-string parameters.

src/app/api/comfy/shared.ts-78-107 (1)

78-107: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

DATA_URL rejects a data URL that carries a media-type parameter.

The pattern places (;base64)? directly after the media type. A data URL with any other parameter therefore fails the whole match:

• data:image/png;base64,AAAA → matches.
• data:text/plain;charset=utf-8;base64,AAAA → group 1 takes text/plain, (;base64)? cannot match ;charset=utf-8, and the required , cannot match. exec returns null.

decodeDataUrl then returns null. src/app/api/comfy/run/route.ts Line 77 converts that into a 400 with "Could not read the media connected to ...", which names the wrong cause for a valid input.

Allow arbitrary parameters between the media type and the optional ;base64.

Separately, Buffer.from(payload, "base64") does not throw on malformed base64; it decodes the valid prefix and stops. Malformed input therefore yields truncated bytes rather than null. If a strict reject is wanted, compare the re-encoded length against the payload.

🐛 Proposed fix for the media-type parameter

-const DATA_URL = /^data:([^;,]+)?(;base64)?,/;
+// media type, then any number of `;key=value` parameters, then optional `;base64`
+const DATA_URL = /^data:([^;,]+)?((?:;[^;,]*)*?)(;base64)?,/;

   const contentType = match[1] || "application/octet-stream";
   const payload = value.slice(match[0].length);
   try {
-    const bytes = match[2]
+    const bytes = match[3]
       ? new Uint8Array(Buffer.from(payload, "base64"))
       : new TextEncoder().encode(decodeURIComponent(payload));

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/app/api/comfy/shared.ts` around lines 78 - 107, Update DATA_URL and
decodeDataUrl to accept arbitrary data-URL media-type parameters between the
MIME type and optional ;base64 marker, including inputs such as charset=utf-8.
Preserve contentType as the base media type and ensure the delimiter still
requires a comma; also validate base64 payloads strictly by rejecting malformed
input rather than decoding only a valid prefix.

🧹 Nitpick comments (15)

src/lib/comfy/__tests__/engineErrors.test.ts (1)

199-213: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the call count so the test proves cancellation.

This test asserts only that the call rejects. noAddressAnswered() carries code ETIMEDOUT, which the retry loop treats as a connection failure and would otherwise repeat. The test therefore passes whether or not the caller-abort branch stops the retries. Capture the stub and assert it ran once.

♻️ Proposed change to pin the cancellation behavior

   it("stops when the caller cancels", async () => {
     const controller = new AbortController();
-    vi.stubGlobal("fetch", async () => {
+    const inner = vi.fn(async () => {
       controller.abort();
       throw noAddressAnswered();
     });
+    vi.stubGlobal("fetch", inner);
 
     const engineFetch = createEngineFetch({ retryBaseMs: 0 });
     await expect(
       engineFetch("https://cloud.comfy.org/api/v2/jobs", {
         method: "POST",
         signal: controller.signal,
       })
     ).rejects.toThrow();
+    expect(inner).toHaveBeenCalledTimes(1);
   });

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/comfy/__tests__/engineErrors.test.ts` around lines 199 - 213, Update
the “stops when the caller cancels” test to retain the fetch stub reference and
assert it was called exactly once after engineFetch rejects. Keep the existing
AbortController setup and rejection assertion, ensuring the test specifically
verifies that cancellation prevents retry attempts.

src/lib/comfy/__tests__/run.test.ts (1)

62-69: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Assert the lower bound of hashSeed too.

The test proves only the upper bound. A negative return value would pass this test, and ComfyUI rejects a negative seed. Add a non-negative assertion so the range is fully pinned.

♻️ Proposed assertion

     // ComfyUI declares seed maxima up to 2^64-1, which JSON cannot round-trip.
     expect(hashSeed("anything")).toBeLessThanOrEqual(Number.MAX_SAFE_INTEGER);
+    expect(hashSeed("anything")).toBeGreaterThanOrEqual(0);
+    expect(Number.isInteger(hashSeed("anything"))).toBe(true);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/comfy/__tests__/run.test.ts` around lines 62 - 69, Update the
hashSeed test in the “deterministic and stays inside the safe integer range”
case to also assert that hashSeed("anything") is greater than or equal to zero,
preserving the existing determinism, uniqueness, and upper-bound assertions.

src/lib/comfy/server/import.ts (1)

114-147: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Scope missing to the blueprint being imported.

editorNodeTypes(file) at Line 117 and Line 125 walks the root nodes and every subgraph definition in the file. In the blueprint branch, only the selected blueprint is converted, but convert reports missing from the whole file. A file carrying several blueprints can therefore tell the user to install a node pack that the selected blueprint never uses.

Compute the missing list from the lifted workflow instead.

♻️ Proposed scoping change

   if (options.blueprintId) {
     const { workflow, instanceNodeId, skippedOutputs, unsupportedInputs } =
       blueprintToWorkflowFile(file, options.blueprintId);
-    const graph = convert(workflow, objectInfo, missing, engine.label);
+    const blueprintMissing = editorNodeTypes(workflow).filter((type) => !objectInfo[type]);
+    const graph = convert(workflow, objectInfo, blueprintMissing, engine.label);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/comfy/server/import.ts` around lines 114 - 147, Scope missing-node
detection to the selected blueprint’s lifted workflow in the blueprint branch:
derive the workflow via blueprintToWorkflowFile before filtering node types,
then compute and refresh missing using that workflow rather than
editorNodeTypes(file). Keep the non-blueprint path using the full file and pass
the blueprint-specific missing list to convert.

src/components/WorkflowCanvas.tsx (1)

746-748: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

This branch duplicates the fallthrough below it.

The added clause returns sourceType === "audio" && targetType === "audio", which is exactly what line 749 already returns for the same inputs. Line 744 only returns early for output and router targets, so a comfyApp end always reaches line 749 with the same result. Remove the clause, or state what it must diverge on.

♻️ Proposed change

-        if (sourceNode?.type === "comfyApp" || targetNode?.type === "comfyApp") {
-          return sourceType === "audio" && targetType === "audio";
-        }
         return sourceType === "audio" && targetType === "audio";

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/components/WorkflowCanvas.tsx` around lines 746 - 748, Remove the
redundant comfyApp condition from the connection validation logic near the
existing output/router early return, leaving the fallthrough audio-type check
unchanged. Do not alter behavior for output or router targets.

src/lib/comfy/reconfigure.ts (1)

11-11: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider exporting bindingKey instead of duplicating it.

ComfyWorkflowImportModal.tsx line 95 defines the same ${nodeId}:${inputKey} helper. Both copies must stay identical, because withAppLabels and the modal's roles map key on the same string. Export this one and import it in the modal.

♻️ Proposed change

-const bindingKey = (nodeId: string, inputKey: string): string => `${nodeId}:${inputKey}`;
+export const bindingKey = (nodeId: string, inputKey: string): string => `${nodeId}:${inputKey}`;

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/comfy/reconfigure.ts` at line 11, Export the bindingKey helper from
reconfigure.ts, then remove the duplicate helper in ComfyWorkflowImportModal.tsx
and import/reuse bindingKey for the modal’s roles map. Preserve the existing
`${nodeId}:${inputKey}` key format.

src/lib/comfy/server/run.ts (1)

21-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

The modulo in hashSeed never applies.

h is reduced to a signed 32-bit integer by | 0, so Math.abs(h) is at most 2^31. MAX_SEED is Number.MAX_SAFE_INTEGER, which is 2^53 − 1. The % MAX_SEED operation is therefore always a no-op, and the comment at line 21 describes a bound the code does not need to enforce here.

Drop the modulo, or state that the 32-bit reduction is what keeps the value safe.

♻️ Proposed simplification

-/** Seeds must stay inside the range JSON can round-trip without precision loss. */
-const MAX_SEED = Number.MAX_SAFE_INTEGER;
-
 /** A deterministic seed derived from a run key. */
 export function hashSeed(key: string): number {
   let h = 0;
+  // `| 0` keeps the accumulator a signed 32-bit integer, which is well inside
+  // the range JSON round-trips without precision loss.
   for (let i = 0; i < key.length; i += 1) h = (Math.imul(h, 31) + key.charCodeAt(i)) | 0;
-  return Math.abs(h) % MAX_SEED;
+  return Math.abs(h);
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/comfy/server/run.ts` around lines 21 - 29, Update hashSeed to remove
the redundant modulo by MAX_SEED, or revise the nearby comment to state that the
signed 32-bit reduction already keeps the result within the safe range; keep the
existing deterministic hashing behavior unchanged.

src/lib/comfy/settings.ts (2)

32-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicated job-timeout bounds and clamp logic in src/lib/comfy/settings.ts and src/lib/comfy/server/connection.ts. Both files declare MIN_JOB_TIMEOUT_MS and MAX_JOB_TIMEOUT_MS with the same values, and both repeat the identical Number.isFinite clamp expression. The two copies can drift, and connection.ts already imports COMFY_DEFAULT_JOB_TIMEOUT_MS from settings.ts, so one shared helper covers both.

• src/lib/comfy/settings.ts#L32-L33: export the two bounds and add a clampJobTimeoutMs helper, then call it in normalizeComfySettings.
• src/lib/comfy/server/connection.ts#L13-L14: delete the local bound declarations, import clampJobTimeoutMs from ../settings, and call it in connectionFromRequest.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/comfy/settings.ts` around lines 32 - 33, Centralize job-timeout
bounds and clamping: in src/lib/comfy/settings.ts:32-33, export
MIN_JOB_TIMEOUT_MS and MAX_JOB_TIMEOUT_MS, add clampJobTimeoutMs using the
shared finite-value clamp logic, and use it from normalizeComfySettings. In
src/lib/comfy/server/connection.ts:13-14, remove the local bounds, import
clampJobTimeoutMs from ../settings, and use it in connectionFromRequest.

---

35-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Move ComfySettings into the Comfy type contract module.

src/lib/comfy/settings.ts declares ComfySettings and imports ComfyBackendMode/ComfyConnection from src/lib/comfy/types.ts, so keep matching interfaces in one Comfy contract location. Update src/types/index.ts if that remains the required central export.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/comfy/settings.ts` around lines 35 - 65, Move the ComfySettings
interface from the settings module into the Comfy contract module alongside
ComfyBackendMode and ComfyConnection, updating imports and references
accordingly. Preserve all fields and documentation, remove the duplicate
declaration, and update src/types/index.ts if it is the central export required
to expose ComfySettings.

Source: Coding guidelines

src/lib/comfy/server/sdkEngine.ts (1)

254-287: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

toEngineError throws instead of returning on one branch.

Line 277 throws from a function whose declared return type is ComfyEngineError. Every call site writes throw toEngineError(...), so the behavior is correct today. The signature does not describe it, and a later refactor that assigns the result instead of throwing it would silently swallow the cancellation.

Return the abort case to the caller as a decision, or split the check out.

♻️ Proposed refactor

-  if (error instanceof DOMException && error.name === "AbortError") {
-    // Propagate cancellation untouched — the caller distinguishes it.
-    throw error;
-  }

Then guard at each call site, for example in upload:

     } catch (error) {
+      // Cancellation is not an engine failure — the caller distinguishes it.
+      if (isAbort(error)) throw error;
       throw toEngineError(error, `${this.label} rejected the upload of ${input.filename}`, this.label);
     }

with a shared helper:

const isAbort = (error: unknown): boolean =>
  error instanceof DOMException && error.name === "AbortError";

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/comfy/server/sdkEngine.ts` around lines 254 - 287, Update
toEngineError so it always returns a ComfyEngineError as declared, removing the
internal AbortError throw; preserve cancellation by handling the abort decision
at each caller before invoking toEngineError, using a shared isAbort helper if
appropriate. Update every throw toEngineError call site, including upload, to
rethrow AbortError unchanged and retain existing error conversion for all other
errors.

src/app/api/comfy/__tests__/poll.route.test.ts (1)

1-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add coverage for the limit split the docblock describes, or narrow the docblock.

Lines 1-10 explain that a status poll and a collect need different limits, and give the 45 s and 73 s figures. No test asserts which limit the route applies to which request. The four tests cover the collect / no-collect behavior only. A regression that reunites the two limits would still pass.

cancel: vi.fn() at line 21 is also never asserted. Remove it, or add the cancellation case.

Run the following script to find the limit the route actually selects, so the assertion can target it:

#!/bin/bash
# Description: Locate the per-request-kind timeout selection in the poll route and its shared helpers.
set -euo pipefail
fd -t f 'route.ts' src/app/api/comfy --exec rg -n -C4 'maxDuration|timeout|collect|cancel' {}
fd -t f 'shared.ts' src/app/api/comfy --exec rg -n -C4 'maxDuration|timeout' {}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/app/api/comfy/__tests__/poll.route.test.ts` around lines 1 - 29, Add
tests in the poll route suite that assert status-poll and collect requests
select their distinct timeout limits, using the route’s actual timeout
configuration symbols and preserving the documented 45-second versus longer
collection behavior. Also remove the unused cancel mock from engineFromRequest,
or add a test covering the route’s cancellation path and asserting that mock is
called.

src/app/api/comfy/shared.ts (1)

118-123: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Bound and normalize ext.

contentType reaches this function from the caller's data URL, where decodeDataUrl captures the media type with [^;,]+. A path separator cannot survive split("/")[1], and slug filters out /, so the filename cannot escape a directory. The remaining gap is shape rather than traversal: ext is unbounded in length and may hold %, spaces, or unicode, and it is sent to the engine upload endpoint as-is.

Constrain ext to a short alphanumeric token, and fall back to bin.

♻️ Proposed hardening

 export function uploadFilename(name: string, contentType: string, bytes: Uint8Array): string {
-  const ext = contentType.split("/")[1]?.split("+")[0] ?? "bin";
+  const raw = contentType.split("/")[1]?.split("+")[0] ?? "";
+  const ext = /^[a-zA-Z0-9]{1,8}$/.test(raw) ? raw.toLowerCase() : "bin";
   const slug = name.replace(/[^a-zA-Z0-9._-]+/g, "-").slice(0, 24) || "input";

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/app/api/comfy/shared.ts` around lines 118 - 123, Update uploadFilename to
normalize and bound the ext value derived from contentType: retain only
alphanumeric characters, limit it to a short fixed length, and use "bin" when
the result is empty. Keep the existing slug and hash naming behavior unchanged.

scripts/comfy-smoke.mjs (2)

200-219: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

A catalog fetch failure leaves the recorded corpus inconsistent.

Line 200 fetches /api/object_info with no error handling, after the per-blueprint files were already written on Lines 189-192. If that fetch fails, record throws and leaves fresh blueprint recordings beside a stale object-info.json and manifest.json.

src/lib/comfy/__tests__/catalog.test.ts then runs against a mismatched corpus. A newly recorded blueprint can name a node class the stale catalog does not hold, so the hermetic test fails with an unknown-node-type error rather than a clear "recording incomplete" signal.

Collect the blueprints and the catalog first, then write all three artifacts together.

♻️ Proposed fix — fetch the catalog before writing

Collect blueprint payloads in memory during the loop, then fetch the catalog, then write:

-      writeFileSync(
-        join(FIXTURES, "blueprints", `${id}.json`),
-        JSON.stringify({ name: entry.name ?? id, why, workflow })
-      );
-      recorded.push({ id, name: entry.name ?? id, why });
+      pending.push({
+        path: join(FIXTURES, "blueprints", `${id}.json`),
+        body: JSON.stringify({ name: entry.name ?? id, why, workflow }),
+      });
+      recorded.push({ id, name: entry.name ?? id, why });
       ok(id);

   const catalog = await engineJson("/api/object_info");
   const trimmed = {};
   for (const type of [...classTypes].sort()) {
     if (catalog[type]) trimmed[type] = trimEntry(catalog[type]);
   }
+
+  // Everything fetched — now commit the corpus as one set.
+  for (const { path, body } of pending) writeFileSync(path, body);
   writeFileSync(join(FIXTURES, "object-info.json"), JSON.stringify(trimmed));

Declare const pending = []; beside recorded on Line 175.

As per coding guidelines: "Maintain the ComfyUI Blueprint corpus regression test using the recorded catalog and workflows; it must run hermetically in CI."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/comfy-smoke.mjs` around lines 200 - 219, Update the recording flow
around the blueprint loop and catalog fetch to collect each blueprint payload in
memory alongside recorded metadata instead of writing files immediately. Fetch
and validate /api/object_info before any artifact writes, then write the pending
blueprint files, object-info.json, and manifest.json together only after all
collection succeeds, preserving the existing artifact contents and naming.

Source: Coding guidelines

---

317-323: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

A non-transient poll failure leaves the job running.

Line 322 returns without cancelling. The job stays queued or running on the engine and keeps consuming GPU credits. The timeout branch on Line 311 does cancel, so the intent is established; this path misses it.

The file header states that run costs credits on Cloud, and run() iterates the whole corpus, so a repeated route failure abandons one live job per blueprint.

Cancel before returning on this path.

♻️ Proposed fix

     const polled = await nb("/api/comfy/poll", { jobId, app });
     if (!polled.res.ok) {
       // A route that says it could not reach the engine is reporting the
       // network, not a verdict — the render is very likely still going.
       if (polled.json?.transient) continue;
+      await nb("/api/comfy/poll", { jobId, app, cancel: true }).catch(() => {});
       return { id, ok: false, stage: "run", error: polled.json?.error ?? polled.text.slice(0, 200) };
     }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/comfy-smoke.mjs` around lines 317 - 323, Update the non-transient
poll-failure branch in run() to cancel the active job before returning the
run-stage error. Reuse the same cancellation behavior already used by the
timeout path, ensuring cancellation is attempted for polled.res.ok failures
where polled.json?.transient is false.

src/lib/comfy/server/index.ts (1)

66-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

options.signal is accepted but never used.

getObjectInfo declares signal?: AbortSignal and never reads it. The doc comment on Line 77 explains why the signal must not reach engine.objectInfo(). That reasoning is correct for the shared fetch, but the parameter still advertises cancellation that the function cannot provide. Callers such as src/app/api/comfy/status/route.ts Line 40 pass request.signal and get no cancellation.

Either remove the option, or honor it on the caller's side only by racing the shared promise against the signal. The second form cancels the caller's wait without aborting the shared fetch.

♻️ Option A — drop the unused option

 export async function getObjectInfo(
   engine: ComfyEngine,
-  options: { force?: boolean; signal?: AbortSignal } = {}
+  options: { force?: boolean } = {}
 ): Promise<ComfyObjectInfo> {

Then remove signal from the call sites, for example in src/app/api/comfy/status/route.ts:

const catalog = await getObjectInfo(engine).catch(() => null);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/comfy/server/index.ts` around lines 66 - 87, Update getObjectInfo so
options.signal is either removed from its API and all call sites stop passing
it, or the caller’s wait races the shared catalog promise against the signal
without aborting engine.objectInfo(). Preserve the shared catalogCache behavior
and ensure cancellation only affects the requesting caller.

src/app/api/comfy/poll/route.ts (1)

52-57: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Validate body.app alongside body.jobId.

The route guards jobId but never checks app. nameFailedOutput on Line 83 and collectRun on Line 97 both read body.app. A request that omits app, or sends it malformed, therefore fails inside those helpers and returns a generic 500 through comfyErrorResponse. src/app/api/comfy/run/route.ts Line 53 validates the same field before use.

Add a matching guard, placed after the cancel branch so a cancel still works without app.

♻️ Proposed guard

     if (body.cancel) {
       await engine.cancel(body.jobId, request.signal);
       return NextResponse.json<ComfyPollResponse>({
         success: true,
         polling: false,
         status: "cancelled",
       });
     }
+
+    if (!body.app?.outputs) {
+      return NextResponse.json(
+        { success: false, error: "This node has no ComfyUI workflow attached yet." },
+        { status: 400 }
+      );
+    }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/app/api/comfy/poll/route.ts` around lines 52 - 57, Update the POST
handler’s request validation to validate body.app in the same manner as the
existing run route, placing the guard after the cancel branch so cancellation
remains valid without app. Reject missing or malformed app values before
nameFailedOutput and collectRun execute, while preserving the existing jobId
validation and error response behavior.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: a1a632db-960a-42fb-bdc7-7b80c1c2c9a1

📥 Commits

Reviewing files that changed from the base of the PR and between 6b63f08 and 92e5799.

⛔ Files ignored due to path filters (1)

• package-lock.json is excluded by !**/package-lock.json

📒 Files selected for processing (88)

• CLAUDE.md
• README.md
• package.json
• scripts/comfy-smoke.mjs
• src/app/api/comfy/__tests__/poll.route.test.ts
• src/app/api/comfy/blueprints/route.ts
• src/app/api/comfy/inspect/route.ts
• src/app/api/comfy/poll/route.ts
• src/app/api/comfy/run/route.ts
• src/app/api/comfy/shared.ts
• src/app/api/comfy/status/route.ts
• src/app/globals.css
• src/components/AnnotationModal.tsx
• src/components/ConnectionDropMenu.tsx
• src/components/GlobalImageHistory.tsx
• src/components/KeyboardShortcutsDialog.tsx
• src/components/ProjectSetupModal.tsx
• src/components/WorkflowCanvas.tsx
• src/components/__tests__/AnnotationModal.test.tsx
• src/components/__tests__/ComfyAppNode.test.tsx
• src/components/__tests__/ConnectionDropMenu.test.tsx
• src/components/icons/ComfyMark.tsx
• src/components/icons/ComfyWordmark.tsx
• src/components/modals/ComfyWorkflowImportModal.tsx
• src/components/nodes/ComfyAppNode.tsx
• src/components/nodes/ComfyAppParameters.tsx
• src/components/nodes/ComfyCurveEditor.tsx
• src/components/nodes/FloatingNodeHeader.tsx
• src/components/nodes/index.ts
• src/components/settings/ComfySettingsTab.tsx
• src/lib/comfy/__tests__/catalog.test.ts
• src/lib/comfy/__tests__/curve.test.ts
• src/lib/comfy/__tests__/detect.test.ts
• src/lib/comfy/__tests__/editor.test.ts
• src/lib/comfy/__tests__/engineErrors.test.ts
• src/lib/comfy/__tests__/fixtures/catalog/blueprints/canny_to_video_ltx_2_0.json
• src/lib/comfy/__tests__/fixtures/catalog/blueprints/character_replacement_scail_2_base.json
• src/lib/comfy/__tests__/fixtures/catalog/blueprints/color_curves.json
• src/lib/comfy/__tests__/fixtures/catalog/blueprints/crop_images_3x3.json
• src/lib/comfy/__tests__/fixtures/catalog/blueprints/image_captioning_gemini.json
• src/lib/comfy/__tests__/fixtures/catalog/blueprints/image_edit_qwen_2509.json
• src/lib/comfy/__tests__/fixtures/catalog/blueprints/image_outpainting_qwen_image.json
• src/lib/comfy/__tests__/fixtures/catalog/blueprints/image_segmentation_sam3.json
• src/lib/comfy/__tests__/fixtures/catalog/blueprints/image_to_gaussian_splat_triposplat.json
• src/lib/comfy/__tests__/fixtures/catalog/blueprints/image_to_model_hunyuan3d_2_1.json
• src/lib/comfy/__tests__/fixtures/catalog/blueprints/merge_videos.json
• src/lib/comfy/__tests__/fixtures/catalog/blueprints/select_per_line_text_by_index.json
• src/lib/comfy/__tests__/fixtures/catalog/blueprints/text_to_audio_ace_step_1_5.json
• src/lib/comfy/__tests__/fixtures/catalog/blueprints/text_to_image_ernie_image.json
• src/lib/comfy/__tests__/fixtures/catalog/blueprints/text_to_video_ltx_2_3.json
• src/lib/comfy/__tests__/fixtures/catalog/blueprints/text_to_video_wan_2_2.json
• src/lib/comfy/__tests__/fixtures/catalog/manifest.json
• src/lib/comfy/__tests__/fixtures/catalog/object-info.json
• src/lib/comfy/__tests__/graph.test.ts
• src/lib/comfy/__tests__/inspect.test.ts
• src/lib/comfy/__tests__/reconfigure.test.ts
• src/lib/comfy/__tests__/run.test.ts
• src/lib/comfy/__tests__/settings.test.ts
• src/lib/comfy/buildApp.ts
• src/lib/comfy/curve.ts
• src/lib/comfy/detect.ts
• src/lib/comfy/editor.ts
• src/lib/comfy/graph.ts
• src/lib/comfy/inspect.ts
• src/lib/comfy/nodeSchema.ts
• src/lib/comfy/reconfigure.ts
• src/lib/comfy/server/connection.ts
• src/lib/comfy/server/engine.ts
• src/lib/comfy/server/fetch.ts
• src/lib/comfy/server/import.ts
• src/lib/comfy/server/index.ts
• src/lib/comfy/server/legacyEngine.ts
• src/lib/comfy/server/run.ts
• src/lib/comfy/server/sdkEngine.ts
• src/lib/comfy/settings.ts
• src/lib/comfy/types.ts
• src/lib/quickstart/validation.ts
• src/store/execution/__tests__/comfyAppExecutor.test.ts
• src/store/execution/comfyAppExecutor.ts
• src/store/execution/executeNode.ts
• src/store/execution/index.ts
• src/store/utils/__tests__/comfyConnectedInputs.test.ts
• src/store/utils/connectedInputs.ts
• src/store/utils/nodeDefaults.ts
• src/store/workflowStore.ts
• src/types/nodes.ts
• src/utils/downloadMedia.ts
• src/utils/mediaStorage.ts

[coderabbitai]

📐 Maintainability & Code Quality | 🟠 Major | ⚖️ Poor tradeoff

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Look for any non-pointer editing path for curve parameters.
set -euo pipefail

rg -nP --type=tsx --type=ts -C3 '\b(curve|Curve)\b' src/components src/store | rg -n 'tabIndex|onKeyDown|role=|aria-' || echo "no keyboard/aria affordance found near curve editing"

Repository: shrimbly/node-banana

Length of output: 243

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Locate ComfyCurveEditor and curve-related files =="
fd -a 'ComfyCurveEditor\.tsx|.*curve.*|.*Curve.*' src || true

echo
echo "== Search for curve text/tabindex/keydown/aria in ts/tsx without file type =="
rg -n --glob '*.{ts,tsx}' -C2 -i '\b(curve|Curve)\b|tabIndex|tabindex|tab index|tab_index|onKeyDown|keydown|aria-|role=' src || true

echo
echo "== Read ComfyCurveEditor outline and relevant source =="
wc -l src/components/nodes/ComfyCurveEditor.tsx
ast-grep outline src/components/nodes/ComfyCurveEditor.tsx --view expanded || true
sed -n '1,240p' src/components/nodes/ComfyCurveEditor.tsx

Repository: shrimbly/node-banana

Length of output: 50377

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== ComfyCurveEditor source =="
sed -n '1,240p' src/components/nodes/ComfyCurveEditor.tsx

echo
echo "== EaseCurveNode source focused area =="
sed -n '1,220p' src/components/nodes/EaseCurveNode.tsx

echo
echo "== Precise curve + keyboard/focus affordances in src/components/nodes =="
rg -n --glob '*.{ts,tsx}' -C2 \
  '(sourceType|type|nodeType).*easeCurve|easeCurve.*|ComfyCurveEditor|\\btabIndex\\b|tabIndex=|onKeyDown|onKeyPress|role="button"|role=\x27button\x27|aria-label|setDragging|removeCurvePoint|addCurvePoint|moveCurvePoint' \
  src/components/nodes src/store src/lib || true

Repository: shrimbly/node-banana

Length of output: 50376

---

Add keyboard access for the CURVE editor.

ComfyCurveEditor only exposes pointer, double-click, and context-menu handlers; the container has no focus role, so keyboard-only users can reset the curve but cannot add, move, or remove points. Add a focusable editing path, such as making the selected point receive keyboard focus and nudge via arrow keys or provide x/y numeric controls for that point.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/components/nodes/ComfyCurveEditor.tsx` around lines 108 - 171, Add
keyboard editing to the point controls rendered in ComfyCurveEditor: make points
focusable and handle arrow-key events to nudge the focused point, while
preserving endpoint restrictions and existing pointer removal behavior. Ensure
keyboard users can also remove the focused non-endpoint point through an
appropriate key action, and expose an accessible label or role describing each
point’s editable position.

[shrimbly]

Skipping this one.

It is a fair accessibility gap, but adding keyboard editing to the curve widget — focusable points, arrow-key nudging, a key to remove one, plus the labels to describe them — is a feature, not a correction to something that is wrong. The brief for this pass was to fix defects without changing how the editor behaves, and this would change it.

What did get fixed there in #141 is the drag itself: the circle was keyed on its own moving x, so React remounted it on every frame and the browser released pointer capture with the removed element. A drag stopped tracking as soon as the pointer left the SVG.

[coderabbitai]

@shrimbly Acknowledged. The keyboard editing gap remains valid, but it is outside the scope of this defect-fix pass.

The key change in #141 addresses the separate drag defect. Keeping the circle mounted preserves pointer capture when its x-coordinate changes.

🐇

_{You are interacting with an AI system.}

[coderabbitai]

🧹 Nitpick comments (2)

src/lib/comfy/__tests__/editor.test.ts (1)

873-878: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Strengthen the off-by-one assertion.

.not.toBe(1344) passes when graph["105:1"] is undefined, when inputs.prompt is absent, and for any wrong value other than that one number. Test 1 already asserts prompt is "a red car", so this test adds little.

An off-by-one from counting first_frame shifts every value and leaves the last widget-backed slot unset. Assert that directly.

♻️ Proposed change to assert the shift directly

   it("does not spend a value on a slot that is fed by a link", () => {
     // `first_frame` reaches a socket, so it carries no widget value. Counting
     // it would shift every value after it by one.
     const graph = convertEditorGraph(promotedFile(), promotedCatalog);
-    expect(graph["105:1"]?.inputs.prompt).not.toBe(1344);
+    // Counting `first_frame` would shift every value one slot earlier and
+    // leave the final widget-backed slot without a promoted value.
+    expect(graph["105:1"]?.inputs.prompt).toBe("a red car");
+    expect(graph["105:3"]?.inputs.vae_name).toBe("audio.safetensors");
   });

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/comfy/__tests__/editor.test.ts` around lines 873 - 878, Strengthen
the assertion in the “does not spend a value on a slot that is fed by a link”
test by verifying the last widget-backed slot is populated with its expected
value, rather than only asserting prompt is not 1344. Use the existing graph
structure and expected fixture value to ensure the test fails when counting
first_frame shifts subsequent values and leaves the final slot unset.

src/lib/comfy/editor.ts (1)

704-709: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Nested subgraph instances are still dropped.

promotedBindings searches only file.nodes. An App Mode entry that names an instance nested inside another subgraph definition finds no match, so the function returns [] and the control disappears without a message. That is the same silent drop this change removes for top-level instances.

The fix is a recursive search across definitions.subgraphs[*].nodes, with the resolved prefix carried into resolveProxied. If nested App Mode promotion is out of scope for this PR, a short note here would record the limit.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/comfy/editor.ts` around lines 704 - 709, Update promotedBindings to
recursively search nodes inside definitions.subgraphs[*].nodes when resolving an
instance, not only file.nodes. Carry the resolved nested prefix through
resolveProxied so nested subgraph instances are promoted instead of returning []
silently; preserve the existing top-level lookup behavior.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/lib/comfy/__tests__/editor.test.ts`:
- Around line 873-878: Strengthen the assertion in the “does not spend a value
on a slot that is fed by a link” test by verifying the last widget-backed slot
is populated with its expected value, rather than only asserting prompt is not
1344. Use the existing graph structure and expected fixture value to ensure the
test fails when counting first_frame shifts subsequent values and leaves the
final slot unset.

In `@src/lib/comfy/editor.ts`:
- Around line 704-709: Update promotedBindings to recursively search nodes
inside definitions.subgraphs[*].nodes when resolving an instance, not only
file.nodes. Carry the resolved nested prefix through resolveProxied so nested
subgraph instances are promoted instead of returning [] silently; preserve the
existing top-level lookup behavior.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: af3990c5-8d38-46f0-b9de-d954dba41110

📥 Commits

Reviewing files that changed from the base of the PR and between 92e5799 and 605eb74.

📒 Files selected for processing (4)

• src/lib/comfy/__tests__/editor.test.ts
• src/lib/comfy/__tests__/inspect.test.ts
• src/lib/comfy/editor.ts
• src/lib/comfy/inspect.ts

🚧 Files skipped from review as they are similar to previous changes (2)

• src/lib/comfy/tests/inspect.test.ts
• src/lib/comfy/inspect.ts

[shrimbly]

Worked through the review — fixes are on #141 (feature/comfyui-workflow-nodes → develop), which lands here once merged. 2594 tests across 121 files, clean build.

Replied inline on the three major comments not taken. Here are the reasons for the minor and nitpick ones I left alone.

src/components/AnnotationModal.tsx — Escape while editing text. Already handled. The capture handler's Escape branch clears editingTextId, textInputPosition and pendingTextPosition itself before returning, so the edit state does close — the input's own onKeyDown never needing to run is the design, not a leak.

src/app/globals.css — kebab-case keyframe names. No stylelint in this repo, so the rule is the reviewer's, not the project's. Every keyframe in that file is camelCase (flowPulse, fanEnter, fanExit), and renaming only the three newest would make the file inconsistent with itself for no gain.

src/components/ConnectionDropMenu.tsx — Comfy app in the 3D source list. THREE_D_SOURCE_OPTIONS is offered when dragging from a 3D input, so it lists nodes that produce 3D — and a Comfy app can, since its outputs come from the attached workflow. The dropped wire is deliberately not landed on a fresh Comfy node: it has no workflow and therefore no handles, so handleMenuSelect sets _autoOpenImport and opens the dialog instead. That is uniform across every handle type, not specific to 3D.

src/lib/comfy/server/run.ts — "" skipped for a parameter. Intended. The node renders a parameter's default as the field's placeholder, so an empty box reads as "using the default" — which is exactly what the run then does. Treating "" as a deliberate clear would make an untouched-looking field silently override the author's value.

src/lib/comfy/editor.ts — widget precedence. Taken as a documentation fix rather than a code one. Element [1] should win: it is the field the frontend writes on a rename or re-promote, and it is also the boundary slot name promotedBindings needs when the id turns out to name a subgraph instance — where the name inside the WidgetId would be the wrong one. Inverting it against a passing Blueprint corpus, on no evidence they ever disagree, is the risky half of the choice you offered. The comment now describes the fallback the code implements.

src/lib/comfy/server/sdkEngine.ts — toEngineError throws on one branch. Correct that the signature under-describes it. Every call site is throw toEngineError(...), so behaviour is right today, and the fix touches six sites to change nothing observable. Left as-is rather than spend the churn in a bug-fix pass.

src/lib/comfy/settings.ts — move ComfySettings into types.ts. types.ts is the wire contract shared with the server; settings.ts is the browser-side store shape, which is a different thing that happens to reference it. The other half of that comment — the duplicated timeout bounds and clamp — was real and is fixed: one exported clampJobTimeoutMs, used by both.

poll.route.test.ts — coverage for the limit split. The two limits are the client's, in comfyAppExecutor, not the route's. Narrowed the docblock to say so, and added the missing cases: the cancel path (with its mock now asserted), cancelling without a contract, and a poll arriving without one.

Everything else in the review is fixed. Thanks — the input-presence mismatch and the video/audio store mix-up were both real bugs with no test to catch them, and both have one now.

[coderabbitai]

Actionable comments posted: 5

🧹 Nitpick comments (4)

src/app/api/comfy/__tests__/run.route.test.ts (1)

100-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Prove the uploadInputs mock is wired before asserting it was not called.

No test asserts that uploadInputs is ever called. If the mock path stops matching the import used by src/app/api/comfy/run/route.ts, this assertion still passes and the test proves nothing. Add one case that reaches the upload step, for example a valid run, and assert the call count there.

♻️ Suggested addition

   it("submits when the required input carries text", async () => {
     const body = await (await call({ prompt: "a dog" })).json();
 
     expect(body).toMatchObject({ success: true, jobId: "job-1" });
     expect(submit).toHaveBeenCalledOnce();
+    // Also anchors the module mock: a path that no longer matches the route's
+    // import would leave the negative assertion below vacuously true.
+    expect(uploadInputs).toHaveBeenCalledOnce();
     const graph = submit.mock.calls[0]![0] as Record<string, { inputs: Record<string, unknown> }>;
     expect(graph["3"]!.inputs.text).toBe("a dog");
   });

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/app/api/comfy/__tests__/run.route.test.ts` around lines 100 - 106, Add a
test in the run route suite that submits a valid run reaching the upload phase
and asserts the uploadInputs mock is called, confirming the mock is wired to the
route’s imported symbol. Keep the existing missing-input test and its not-called
assertion unchanged.

src/lib/comfy/library.ts (1)

95-106: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Report the storage quota only for a quota failure.

write accesses window.localStorage without the isBrowser() guard that listSavedComfyNodes and subscribeSavedComfyNodes use. Any failure inside the try block, including a missing window during server rendering or a JSON.stringify failure, becomes the message "There is no room left to save this node." That message directs the user to delete saved nodes, which cannot fix those causes. Guard for the browser and re-raise unexpected errors.

🛠️ Proposed fix

 function write(entries: SavedComfyNode[]): void {
+  if (!isBrowser()) return;
+  const payload = JSON.stringify(entries);
   try {
-    window.localStorage.setItem(COMFY_APPS_KEY, JSON.stringify(entries));
-  } catch {
+    window.localStorage.setItem(COMFY_APPS_KEY, payload);
+  } catch (error) {
     // Almost always the storage quota. Nothing here can free space safely —
     // the other keys are the user's projects and costs — so it is reported.
-    throw new Error(
-      "There is no room left to save this node. Delete a saved node and try again."
-    );
+    if (error instanceof DOMException) {
+      throw new Error(
+        "There is no room left to save this node. Delete a saved node and try again."
+      );
+    }
+    throw error;
   }
   notify();
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/comfy/library.ts` around lines 95 - 106, Update write to use the
existing isBrowser() guard before accessing window.localStorage, and only
convert genuine storage-quota failures into the user-facing “no room left”
error. Re-raise unexpected errors, including server-rendering access failures
and JSON.stringify errors, without replacing their original details; preserve
notify() after a successful write.

src/lib/comfy/reconfigure.ts (1)

11-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consolidate the duplicated binding id helper.

src/lib/comfy/inspect.ts and src/lib/comfy/reconfigure.ts each define the same ${nodeId}:${inputKey} helper. Export one shared helper and use it from both modules so the binding id cannot drift between inspection, labels, and the import dialog’s roles map.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/comfy/reconfigure.ts` around lines 11 - 18, Consolidate the duplicate
binding-id helper by retaining the exported bindingKey function in
reconfigure.ts as the single implementation, then update inspect.ts to import
and use it instead of defining its own `${nodeId}:${inputKey}` helper. Preserve
all existing callers and ensure inspection, labels, and the import dialog roles
map use the shared result.

src/lib/comfy/__tests__/previews.test.ts (1)

123-161: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a case for the frame-level node_id fallback.

previews resolves the node with envelopeNodeId(payload) ?? String(data.node_id ?? ""). Every preview fixture here uses a metadata envelope, so the fallback branch is never exercised. A self-hosted engine sends the classic envelope, which carries no metadata, and only the frame field names the node. One case locks that path down.

💚 Proposed test case

+  it("falls back to the frame's own node id when the envelope has no metadata", async () => {
+    const frames = await collect(
+      engineYielding([
+        { event: "preview", data: { node_id: "9", data_base64: Buffer.from(classic(JPEG_BYTES)).toString("base64") } },
+      ])
+    );
+
+    expect(frames).toHaveLength(1);
+    expect(frames[0]!.nodeId).toBe("9");
+  });

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/comfy/__tests__/previews.test.ts` around lines 123 - 161, Add a test
in the “previews” suite covering a preview whose envelope has no node metadata
but whose frame data contains node_id. Assert that the yielded frame uses the
frame-level node ID, while preserving the existing data URL expectations and
other preview behavior.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/app/api/comfy/preview/route.ts`:
- Around line 49-68: Track whether the ReadableStream has been cancelled in the
stream closure around the start and cancel methods, and have the finally block
call controller.close() only when cancellation has not already closed the
stream. Update the visible stream lifecycle logic without changing the existing
upstream cleanup through frames.return(undefined).

In `@src/components/__tests__/ComfyWorkflowImportModal.test.tsx`:
- Around line 124-134: Ensure the setItem spy created in the “reports a failed
save rather than losing it” test is restored even when assertions fail by adding
suite-level afterEach cleanup with vi.restoreAllMocks(), or by wrapping the test
actions and assertion in try/finally. Remove reliance on the trailing
setItem.mockRestore() alone.

In `@src/lib/comfy/library.ts`:
- Around line 48-57: Update the SavedComfyNode validation in the parse filter to
require candidate.app.outputs, candidate.app.params, and candidate.app.inputs
all be arrays, so incomplete entries are rejected before seedFromSavedComfyNode
or appToInputSchema consumes them.

In `@src/lib/comfy/settings.ts`:
- Around line 42-47: Update clampJobTimeoutMs to reject null and empty-string
inputs before converting with Number, so absent values return
COMFY_DEFAULT_JOB_TIMEOUT_MS instead of being clamped to the minimum. In
connectionFromRequest, pass the raw COMFY_HEADERS.jobTimeout header value to
clampJobTimeoutMs rather than pre-converting it.

In `@src/utils/__tests__/comfyOutputStorage.test.ts`:
- Around line 82-92: Update the generation stub in the save/load handlers to
store each generation’s output kind alongside its value, then have
/api/load-generation return only the matching video or audio field instead of
populating both. Adjust the written.generations assertions to remove any kind
prefix before comparing stored values, while preserving the existing save and
hydration test behavior.

---

Nitpick comments:
In `@src/app/api/comfy/__tests__/run.route.test.ts`:
- Around line 100-106: Add a test in the run route suite that submits a valid
run reaching the upload phase and asserts the uploadInputs mock is called,
confirming the mock is wired to the route’s imported symbol. Keep the existing
missing-input test and its not-called assertion unchanged.

In `@src/lib/comfy/__tests__/previews.test.ts`:
- Around line 123-161: Add a test in the “previews” suite covering a preview
whose envelope has no node metadata but whose frame data contains node_id.
Assert that the yielded frame uses the frame-level node ID, while preserving the
existing data URL expectations and other preview behavior.

In `@src/lib/comfy/library.ts`:
- Around line 95-106: Update write to use the existing isBrowser() guard before
accessing window.localStorage, and only convert genuine storage-quota failures
into the user-facing “no room left” error. Re-raise unexpected errors, including
server-rendering access failures and JSON.stringify errors, without replacing
their original details; preserve notify() after a successful write.

In `@src/lib/comfy/reconfigure.ts`:
- Around line 11-18: Consolidate the duplicate binding-id helper by retaining
the exported bindingKey function in reconfigure.ts as the single implementation,
then update inspect.ts to import and use it instead of defining its own
`${nodeId}:${inputKey}` helper. Preserve all existing callers and ensure
inspection, labels, and the import dialog roles map use the shared result.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 1e5a55c4-c2da-462e-99f3-5814a2b14d28

📥 Commits

Reviewing files that changed from the base of the PR and between 969adf0 and 15a1807.

📒 Files selected for processing (58)

• CLAUDE.md
• README.md
• scripts/comfy-smoke.mjs
• src/app/api/comfy/__tests__/poll.route.test.ts
• src/app/api/comfy/__tests__/run.route.test.ts
• src/app/api/comfy/__tests__/shared.test.ts
• src/app/api/comfy/blueprints/route.ts
• src/app/api/comfy/inspect/route.ts
• src/app/api/comfy/poll/route.ts
• src/app/api/comfy/preview/route.ts
• src/app/api/comfy/run/route.ts
• src/app/api/comfy/shared.ts
• src/app/api/comfy/status/route.ts
• src/app/globals.css
• src/components/ConnectionDropMenu.tsx
• src/components/NodeSearchMenu.tsx
• src/components/WorkflowCanvas.tsx
• src/components/__tests__/BaseNode.test.tsx
• src/components/__tests__/ComfyAppPreview.test.tsx
• src/components/__tests__/ComfyNodePreview.test.tsx
• src/components/__tests__/ComfyWorkflowImportModal.test.tsx
• src/components/__tests__/ConnectionDropMenu.test.tsx
• src/components/__tests__/NodeSearchMenu.test.tsx
• src/components/modals/ComfyNodePreview.tsx
• src/components/modals/ComfyWorkflowImportModal.tsx
• src/components/nodes/BaseNode.tsx
• src/components/nodes/ComfyAppNode.tsx
• src/components/nodes/ComfyAppParameters.tsx
• src/components/nodes/ComfyCurveEditor.tsx
• src/components/settings/ComfySettingsTab.tsx
• src/hooks/useComfyPreview.ts
• src/hooks/useSavedComfyNodes.ts
• src/lib/comfy/__tests__/curve.test.ts
• src/lib/comfy/__tests__/engineErrors.test.ts
• src/lib/comfy/__tests__/inspect.test.ts
• src/lib/comfy/__tests__/library.test.ts
• src/lib/comfy/__tests__/previews.test.ts
• src/lib/comfy/__tests__/run.test.ts
• src/lib/comfy/curve.ts
• src/lib/comfy/editor.ts
• src/lib/comfy/inspect.ts
• src/lib/comfy/library.ts
• src/lib/comfy/nodeSchema.ts
• src/lib/comfy/reconfigure.ts
• src/lib/comfy/server/connection.ts
• src/lib/comfy/server/engine.ts
• src/lib/comfy/server/fetch.ts
• src/lib/comfy/server/import.ts
• src/lib/comfy/server/index.ts
• src/lib/comfy/server/legacyEngine.ts
• src/lib/comfy/server/run.ts
• src/lib/comfy/server/sdkEngine.ts
• src/lib/comfy/settings.ts
• src/lib/comfy/types.ts
• src/store/execution/comfyAppExecutor.ts
• src/types/nodes.ts
• src/utils/__tests__/comfyOutputStorage.test.ts
• src/utils/mediaStorage.ts

🚧 Files skipped from review as they are similar to previous changes (29)

• src/components/tests/BaseNode.test.tsx
• README.md
• src/app/globals.css
• src/app/api/comfy/shared.ts
• src/lib/comfy/server/connection.ts
• src/app/api/comfy/status/route.ts
• src/utils/mediaStorage.ts
• src/lib/comfy/types.ts
• src/app/api/comfy/poll/route.ts
• src/app/api/comfy/inspect/route.ts
• src/lib/comfy/tests/curve.test.ts
• src/app/api/comfy/run/route.ts
• src/lib/comfy/server/import.ts
• src/components/settings/ComfySettingsTab.tsx
• src/components/nodes/ComfyAppParameters.tsx
• src/lib/comfy/server/legacyEngine.ts
• src/lib/comfy/server/index.ts
• src/lib/comfy/server/fetch.ts
• src/store/execution/comfyAppExecutor.ts
• src/types/nodes.ts
• src/lib/comfy/server/run.ts
• src/components/nodes/BaseNode.tsx
• src/lib/comfy/editor.ts
• src/lib/comfy/inspect.ts
• src/app/api/comfy/blueprints/route.ts
• src/components/nodes/ComfyAppNode.tsx
• src/lib/comfy/curve.ts
• scripts/comfy-smoke.mjs
• src/lib/comfy/server/engine.ts

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/lib/comfy/server/fetch.ts`:
- Around line 229-237: Update engineFetch so the caller signal selection
preserves input Request signals: use init.signal when provided, otherwise use
input.signal before creating the combined timeout signal. Ensure fetch receives
a signal that still honors an input Request’s signal when engineFetch is called
without init.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 7408ad2f-e963-4d36-9d34-3edf7453f3b8

📥 Commits

Reviewing files that changed from the base of the PR and between 15a1807 and a083898.

📒 Files selected for processing (21)

• scripts/comfy-smoke.mjs
• src/app/api/comfy/__tests__/run.route.test.ts
• src/app/api/comfy/preview/route.ts
• src/components/WorkflowCanvas.tsx
• src/components/__tests__/ComfyAppPreview.test.tsx
• src/components/__tests__/ComfyWorkflowImportModal.test.tsx
• src/components/modals/ComfyWorkflowImportModal.tsx
• src/components/nodes/BaseNode.tsx
• src/components/settings/ComfySettingsTab.tsx
• src/hooks/useComfyPreview.ts
• src/lib/comfy/__tests__/engineErrors.test.ts
• src/lib/comfy/__tests__/library.test.ts
• src/lib/comfy/__tests__/previews.test.ts
• src/lib/comfy/__tests__/settings.test.ts
• src/lib/comfy/library.ts
• src/lib/comfy/server/connection.ts
• src/lib/comfy/server/fetch.ts
• src/lib/comfy/server/legacyEngine.ts
• src/lib/comfy/server/sdkEngine.ts
• src/lib/comfy/settings.ts
• src/utils/__tests__/comfyOutputStorage.test.ts

🚧 Files skipped from review as they are similar to previous changes (12)

• src/components/nodes/BaseNode.tsx
• src/lib/comfy/tests/previews.test.ts
• src/components/tests/ComfyWorkflowImportModal.test.tsx
• src/utils/tests/comfyOutputStorage.test.ts
• src/lib/comfy/server/connection.ts
• src/hooks/useComfyPreview.ts
• src/lib/comfy/server/legacyEngine.ts
• src/lib/comfy/library.ts
• src/lib/comfy/tests/settings.test.ts
• src/lib/comfy/settings.ts
• src/components/WorkflowCanvas.tsx
• src/components/modals/ComfyWorkflowImportModal.tsx

[coderabbitai]

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

rg -n -C 4 'const caller = init\?\.signal|fetch\(input, \{ \.\.\.init, signal \}\)' \
  src/lib/comfy/server/fetch.ts
rg -n -C 4 'createEngineFetch|AbortController|new Request' \
  src/lib/comfy/__tests__/engineErrors.test.ts

Repository: shrimbly/node-banana

Length of output: 6726

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the relevant function signature/body.
sed -n '180,260p' src/lib/comfy/server/fetch.ts

# Inspect nearby tests to see whether Request.signal is covered at all.
sed -n '300,390p' src/lib/comfy/__tests__/engineErrors.test.ts

# Check imports and type references around createEngineFetch.
sed -n '1,80p' src/lib/comfy/server/fetch.ts

Repository: shrimbly/node-banana

Length of output: 10466

---

🌐 Web query:

MDN fetch Request constructor signal RequestInit signal overrides Request.signal

💡 Result:

When using the Request constructor, there is no conflict between a property named "Request.signal" and the "signal" property within the RequestInit options object, because "Request.signal" is not an input parameter for the constructor [1][2][3]. The Request constructor accepts two arguments: the resource (URL or Request object) and an optional RequestInit object [1][2]. The RequestInit object is where you define the signal property to associate an AbortSignal with the request [2]. Once a Request object is created, its "signal" property (Request.signal) becomes a read-only attribute that returns the AbortSignal associated with that specific request [4][3]. If you are concerned about precedence when combining requests and options: 1. When constructing a new Request from an existing Request object and a RequestInit object, the options provided in the RequestInit object override the corresponding options from the original Request [1]. 2. When passing a Request object to a fetch call along with a separate RequestInit object, the options in the RequestInit object passed to fetch take precedence over the options already set on the Request object [2]. In summary, Request.signal is a property of the resulting object, not a parameter of the constructor, so it cannot "override" an input signal; rather, the signal provided in the RequestInit object determines the signal that the resulting Request object will hold [4][2].

Citations:

• 1: https://developer.mozilla.org/en-US/docs/Web/API/Request/Request
• 2: https://developer.mozilla.org/en-US/docs/Web/API/RequestInit
• 3: https://developer.mozilla.org/en-US/docs/Web/API/Request
• 4: https://developer.mozilla.org/en-US/docs/Web/API/Request/signal

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Search all usages of createEngineFetch and engineFetch to confirm whether callers can pass Request objects.
rg -n -C 3 'createEngineFetch\(|\belongingEngineFetch\b|engineFetch\(' src/lib compliant --exclude-dir=node_modules || true

# Search all tests that call createEngineFetch/test it with Request instances or check request.signal.
rg -n -C 4 'engineFetch\(|new Request|Request\(|\bsignal\b' src/lib/comfy/__tests__/engineErrors.test.ts src/lib/comfy/__tests__ src/lib/comfy/server || true

Repository: shrimbly/node-banana

Length of output: 50376

---

🌐 Web query:

Node.js fetch RequestInit signal overrides Request signal fetch passed Request plus options signal semantics

💡 Result:

In Node.js, as in the web Fetch specification, when you pass a Request object to the fetch function along with an additional RequestInit options object, the options provided in the second argument take precedence [1][2][3]. Regarding the AbortSignal, the fetch function does not simply override one signal with another; rather, the Fetch specification requires the resulting Request's signal to follow the signal provided in the fetch call's RequestInit [4][5]. Key Semantics: 1. Precedence: When fetch(request, options) is called, any properties defined in the options object (including signal) override the corresponding properties already present on the Request object [1][2][3]. 2. Signal Behavior: If a signal is provided in the RequestInit object passed to fetch, it effectively becomes the controlling signal for that fetch operation [2]. If the original Request object also had a signal, the implementation must ensure the final request respects the signal provided in the call [4][5]. 3. Implementation Note: In Node.js (via the undici-based fetch implementation), creating a new Request using new Request(request, init) or calling fetch(request, init) is the standard way to ensure these overrides are applied correctly [1][6]. Failing to merge these correctly—such as by ignoring the second argument—is a common source of bugs where headers, signals, or other configuration options are silently dropped [1][6]. If you are encountering issues with signal propagation, ensure you are not accidentally discarding the RequestInit object when wrapping or proxying fetch calls [1][6]. Furthermore, be aware that in newer Node.js versions, the runtime strictly enforces that the signal passed to RequestInit must be a native AbortSignal instance; non-native or polyfilled signals may require normalization to avoid type errors [7].

Citations:

• 1: fix(instrumentation-fetch): preserve init overrides when input is a Request object open-telemetry/opentelemetry-js#6525
• 2: https://developer.mozilla.org/en-US/docs/Web/API/RequestInit
• 3: https://developer.mozilla.org/en-US/docs/Web/API/Request/Request
• 4: Request object's AbortSignal does not abort with the expected reason nodejs/node#43874
• 5: https://fetch.spec.whatwg.org/
• 6: open-telemetry/opentelemetry-js@1b8a6a2
• 7: fix: normalise non-native AbortSignal for Node 24+ compatibility Vermonster/fhir-kit-client#205

---

Preserve the signal on an input Request.

engineFetch only uses init.signal, but line 239 passes a new signal to fetch. Because fetch(Request, RequestInit) lets RequestInit.signal take precedence, an engineFetch(request) call with no init loses request.signal. Use input.signal when init.signal is absent.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/comfy/server/fetch.ts` around lines 229 - 237, Update engineFetch so
the caller signal selection preserves input Request signals: use init.signal
when provided, otherwise use input.signal before creating the combined timeout
signal. Ensure fetch receives a signal that still honors an input Request’s
signal when engineFetch is called without init.