feat: Utility Nodes #5
Conversation
- Implemented GridSplitNode to allow users to split images into a grid of tiles. - Added input controls for specifying the number of rows and columns. - Integrated image processing logic to handle tile generation using the Canvas API. - Updated workflow store to manage GridSplitNode state and validation. - Enhanced BaseNode component to support additional props for status and icon. - Created utility function `splitImageIntoTiles` for image processing. - Updated types to include GridSplitNodeData and associated interfaces.
WalkthroughThis pull request introduces the Grid Split Node, a utility node for dividing images into grid tiles. It adds comprehensive documentation, a new React component with preview and grid visualization, image-splitting utilities, type definitions, and store integration for workflow execution and tile routing. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as FloatingActionBar
participant Canvas as WorkflowCanvas
participant Store as WorkflowStore
participant Splitter as imageSplitter
participant Node as GridSplitNode
User->>UI: Add utility node (Grid Split)
UI->>Canvas: Create gridSplit node
Canvas->>Store: Initialize gridSplit with defaults<br/>(2x2, idle status)
User->>Node: Connect image input
Node->>Store: Update inputImage in nodeData
User->>Node: Adjust rows/columns<br/>(e.g., 3x3)
Node->>Store: updateNodeData({rows, columns})
Store->>Node: Compute tileCount = 9
Node->>Node: Generate 9 tile output handles
User->>Canvas: Execute workflow
Canvas->>Store: executeWorkflow()
Store->>Store: Process gridSplit node<br/>Set status = loading
Store->>Splitter: splitImageIntoTiles<br/>(imageBase64, rows, columns)
Splitter->>Splitter: Load image via Canvas API<br/>Compute tile dimensions<br/>Generate 9 PNG tiles
Splitter-->>Store: Resolve TileOutput[]<br/>(tile-0...tile-8 with base64)
Store->>Store: Build tileOutputs map<br/>Update node status = complete
Store->>Node: Publish state update
Node->>Node: Display status (complete)<br/>Show preview with grid overlay
User->>Canvas: Connect tile output to<br/>downstream node
Canvas->>Store: Validate connection<br/>(tile-* → image input)
Store->>Store: Route tile image via<br/>getConnectedInputs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~40 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/store/workflowStore.ts (1)
461-471: Missing gridSplit in createGroup's defaultNodeDimensions.The
defaultNodeDimensionsmap used for group bounding box calculation doesn't includegridSplit. This will cause it to fall back to{ width: 300, height: 280 }instead of the correct{ width: 320, height: 400 }, resulting in slightly incorrect group bounds when gridSplit nodes are grouped.Suggested fix
const defaultNodeDimensions: Record< string, { width: number; height: number } > = { imageInput: { width: 300, height: 280 }, annotation: { width: 300, height: 280 }, prompt: { width: 320, height: 220 }, nanoBanana: { width: 300, height: 300 }, llmGenerate: { width: 320, height: 360 }, output: { width: 320, height: 320 }, + gridSplit: { width: 320, height: 400 }, };
🧹 Nitpick comments (9)
Makefile (1)
17-17: Add trailing newline at end of file.Makefiles should end with a newline for POSIX compliance and better compatibility with text processing tools.
scripts/split-grid.sh (1)
62-76: Consider adding a tile count guard rail.The PRD specifies a maximum of 64 tiles to prevent UI explosion. While this is a standalone script, adding a similar guard rail would provide consistency and prevent resource exhaustion from accidentally creating thousands of tiles.
🔎 Suggested validation
TILE_WIDTH=$((WIDTH / COLS)) TILE_HEIGHT=$((HEIGHT / ROWS)) +# Check tile count limit (aligned with PRD max of 64) +TOTAL_TILES=$((ROWS * COLS)) +if [ $TOTAL_TILES -gt 64 ]; then + echo "Warning: Creating $TOTAL_TILES tiles (max recommended: 64)" + read -p "Continue? (y/N): " confirm + if [[ ! "$confirm" =~ ^[Yy]$ ]]; then + echo "Aborted." + exit 0 + fi +fi + echo "Tile size: ${TILE_WIDTH}x${TILE_HEIGHT}"src/utils/imageSplitter.ts (1)
56-76: Consider memory optimization for large grids.Creating many canvas elements (up to 64) could be memory-intensive, especially for large source images. While this is acceptable for the v1 constraints (max 64 tiles), consider reusing a single canvas instance in future optimizations.
💡 Optional optimization approach
// Reuse a single canvas and create blobs/data URLs immediately const canvas = document.createElement("canvas"); const ctx = canvas.getContext("2d"); if (!ctx) throw new Error("Failed to get 2D context"); for (let row = 0; row < rows; row++) { for (let col = 0; col < columns; col++) { // ... calculate dimensions ... canvas.width = tileWidth; canvas.height = tileHeight; ctx.clearRect(0, 0, tileWidth, tileHeight); ctx.drawImage(/* ... */); const tileBase64 = canvas.toDataURL("image/png"); tiles.push(/* ... */); } }This reduces memory footprint from O(tiles) canvas elements to O(1).
src/components/WorkflowCanvas.tsx (1)
330-330: RemovegetNodeHandlesfrom dependency array.
getNodeHandlesis a module-level function, not a React hook or state-dependent value. Including it in the dependency array is unnecessary and could confuse future maintainers about what triggers re-creation of this callback.Suggested fix
- }, [screenToFlowPosition, nodes, getNodeHandles, handleConnect]); + }, [screenToFlowPosition, nodes, handleConnect]);src/components/nodes/GridSplitNode.tsx (2)
83-104: Consider improving label-input association for accessibility.The
<label>elements are visually associated but not programmatically linked to their inputs. AddinghtmlForandidattributes would improve screen reader support.Suggested improvement
<div> - <label className="block text-xs text-gray-400 mb-1">Rows</label> + <label htmlFor={`${id}-rows`} className="block text-xs text-gray-400 mb-1">Rows</label> <input + id={`${id}-rows`} type="number" min="1" max="10" value={nodeData.rows} onChange={handleRowsChange} className="w-full px-2 py-1 text-sm bg-gray-800 border border-gray-700 rounded focus:outline-none focus:border-purple-500" /> </div> <div> - <label className="block text-xs text-gray-400 mb-1">Columns</label> + <label htmlFor={`${id}-columns`} className="block text-xs text-gray-400 mb-1">Columns</label> <input + id={`${id}-columns`} type="number" min="1" max="10"
199-202: Handle labels may overflow the canvas area.The labels are positioned with
left-full ml-2, placing them to the right of handles that are already at the right edge of the node. With many tiles, this could cause labels to extend significantly beyond the node boundary.Consider positioning labels to the left of handles (inside the node) or truncating/hiding labels when tile count is high.
src/store/workflowStore.ts (3)
640-651: GridSplit tile routing logic is correct.The code properly extracts the specific tile image using the source handle ID as the key into
tileOutputs. This enables downstream nodes to receive individual tiles from the grid.Consider removing or reducing the verbosity of the debug
console.logbefore merging to production.
1312-1380: GridSplit regeneration logic is correct but duplicates execute logic.The regeneration case properly handles errors with
returnand follows the same flow as execution. Consider extracting the shared splitting logic into a helper function to reduce duplication.
1054-1058: Consider removing or gating debug console.log statements.Multiple
console.logcalls for gridSplit debugging are present throughout the execution and regeneration logic. While useful during development, these should be removed or wrapped in a debug flag before production release.Also applies to: 1074-1078, 1104-1107, 1115-1118
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.gitignore(1 hunks)AGENTS.md(1 hunks)Makefile(1 hunks)prd-utility-nodes.md(1 hunks)scripts/split-grid.sh(1 hunks)src/components/ConnectionDropMenu.tsx(10 hunks)src/components/FloatingActionBar.tsx(11 hunks)src/components/WorkflowCanvas.tsx(33 hunks)src/components/nodes/BaseNode.tsx(3 hunks)src/components/nodes/GridSplitNode.tsx(1 hunks)src/components/nodes/index.ts(1 hunks)src/store/workflowStore.ts(27 hunks)src/types/index.ts(4 hunks)src/utils/imageSplitter.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/components/nodes/GridSplitNode.tsx (3)
src/types/index.ts (1)
GridSplitNodeData(161-168)src/store/workflowStore.ts (1)
useWorkflowStore(238-1645)src/components/nodes/BaseNode.tsx (1)
BaseNode(21-101)
src/components/ConnectionDropMenu.tsx (1)
src/types/index.ts (1)
NodeType(4-11)
src/components/FloatingActionBar.tsx (1)
src/types/index.ts (1)
NodeType(4-11)
src/store/workflowStore.ts (2)
src/types/index.ts (2)
GridSplitNodeData(161-168)NodeType(4-11)src/utils/imageSplitter.ts (1)
splitImageIntoTiles(21-108)
src/components/WorkflowCanvas.tsx (3)
src/components/nodes/GridSplitNode.tsx (1)
GridSplitNode(11-208)src/components/nodes/index.ts (1)
GridSplitNode(8-8)src/types/index.ts (2)
NanoBananaNodeData(131-141)NodeType(4-11)
🪛 checkmake (0.2.2)
Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🪛 LanguageTool
prd-utility-nodes.md
[grammar] ~174-~174: Ensure spelling is correct
Context: ... rw = W - tw*C - rh = H - th*R Remainder policy (simple + predictable): - Dis...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~267-~267: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...ot process image” | | Memory pressure / very large image | Toast warning + node ...
(EN_WEAK_ADJECTIVE)
🪛 markdownlint-cli2 (0.18.1)
prd-utility-nodes.md
90-90: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
107-107: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
126-126: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
132-132: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
136-136: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (20)
src/components/ConnectionDropMenu.tsx (5)
22-34: SVG icon formatting is purely stylistic.All SVG blocks are expanded with explicit multiline attribute formatting; no structural or behavioral changes to icons.
Also applies to: 41-53, 60-72, 80-92, 99-111, 121-133, 140-152, 163-175, 182-194, 201-214, 242-254, 261-273
391-396: onClick handler reformatting preserves callback behavior.The multiline expansion of the selection object is purely stylistic; the emitted
{ type, isAction }payload remains identical.
305-307: Ternary expressions reformatted for readability without functional change.Return values based on
handleTypeandconnectionTyperemain semantically identical.Also applies to: 310-312
328-330: Keyboard navigation and hint labels reformatted.Control flow and state updates unchanged; only line-wrapping and formatting improved for clarity.
Also applies to: 411-419
57-74: Verify intent of "gridSplit" node vs "splitGrid" action duplication.Both are added to image-connection menus with nearly identical icons ("Grid Split" vs "Split Image Grid"). The PR objectives mention one utility node (singular), but this change introduces both:
gridSplit(NodeType, not markedisAction) — can appear in workflowsplitGrid(MenuAction, markedisAction: true) — appears to trigger inline behaviorConfirm this is intentional. If both are needed, clarify their distinct purposes and usage patterns; if redundant, remove one.
Also applies to: 76-94, 217-234
src/components/FloatingActionBar.tsx (2)
104-116: LGTM: SVG formatting improvements.The multi-line formatting of SVG elements improves readability and makes attributes easier to scan.
Also applies to: 128-140, 149-161, 217-230, 241-253, 350-362, 364-376, 440-452, 467-472, 486-497, 511-522
337-337: LGTM: UtilityComboButton integration.The Utility button is properly integrated into the action bar with appropriate placement between generation and output nodes.
.gitignore (1)
40-41: LGTM!The addition of the
tiles/directory to.gitignoreis appropriate for the Grid Split feature, ensuring generated tile images are excluded from version control.src/components/nodes/index.ts (1)
8-8: LGTM!The
GridSplitNodeexport follows the established barrel pattern and integrates cleanly with the existing exports.src/types/index.ts (1)
160-168: LGTM! GridSplitNodeData interface is well-structured.The
GridSplitNodeDatainterface correctly models the Grid Split node's state:
tileOutputsasRecord<string, string>efficiently maps handle IDs to base64 images- Reuses existing
NodeStatustype for consistency- Follows the pattern established by other node data interfaces
Note: Runtime validation for
rowsandcolumns(1-10 range, max 64 tiles per PRD) should be enforced in the workflow store or component logic.prd-utility-nodes.md (1)
1-338: Excellent PRD documentation!The PRD is comprehensive and well-structured:
- Clear problem statement and success criteria
- Detailed technical specifications with deterministic split algorithm
- Explicit constraints (max 64 tiles, 1-10 rows/cols)
- Comprehensive error states and acceptance criteria
- TypeScript interfaces align with the implementation
The remainder policy (last row/col gets extra pixels) is clearly documented and matches the implementation in
imageSplitter.ts.src/utils/imageSplitter.ts (1)
43-94: LGTM! Tile splitting algorithm is correct.The implementation correctly follows the PRD specification:
- Base dimensions calculated using
Math.floorfor integer division- Remainder pixels distributed to the last column and row
- Row-major ordering matches spec:
index = row * columns + col- Handle IDs follow convention:
tile-${index}- Per-tile metadata includes all required fields (row, col, index, crop, source dimensions)
The canvas-based approach ensures pixel-perfect extraction without resampling.
src/components/WorkflowCanvas.tsx (2)
69-72: Connection validation for tile outputs looks correct.The logic properly allows
tile-*prefixed handles from gridSplit nodes to connect toimageinputs, enabling downstream nodes to receive individual tiles.
1120-1121: MiniMap color integration looks good.The purple color
#a855f7for gridSplit nodes aligns with the purple theme used in the GridSplitNode handles.src/components/nodes/GridSplitNode.tsx (3)
19-37: Input handlers correctly validate and clear tile outputs on change.The validation ensures values stay within 1-10 range, and clearing
tileOutputs: {}when grid dimensions change prevents stale tile data from being used.
183-205: Handle positioning logic is sound.The distribution from 15% to 85% avoids edge overlap, and the special case for
tileCount === 1prevents division by zero. The label switching at 16 tiles keeps the UI readable.
122-167: Preview with grid overlay implementation is clean.Using
mixBlendMode: "difference"ensures grid lines remain visible regardless of image content. The SVG percentage-based positioning scales correctly with the preview container.src/store/workflowStore.ts (3)
182-190: Default node data initialization for gridSplit is correct.The defaults (2x2 grid, idle status, empty tileOutputs) provide a sensible starting state for new Grid Split nodes.
718-733: Validation logic for Grid Split nodes is thorough.The checks ensure image input is connected and tile count stays within the 64-tile limit, matching the UI validation in GridSplitNode.
1383-1397: Downstream output node updates improve UX after regeneration.This addition ensures that output nodes immediately reflect regenerated content without requiring a full workflow execution. The scope is appropriately limited to immediate downstream output nodes.
| The application uses these models for image generation. These models are very recently released and do exist. | ||
| gemini-3-pro-image-preview | ||
| gemini-2.5-flash-preview-image-generation |
There was a problem hiding this comment.
Fix incorrect and retired model identifier in the models list.
The model gemini-2.5-flash-preview-image-generation was retired on October 31, 2025, and the name itself is inaccurate. The correct, current model identifier is gemini-2.5-flash-image. Since this is a development guide for models "very recently released," using an outdated, non-existent model name undermines credibility and will confuse developers.
🔎 Proposed fix
- gemini-3-pro-image-preview
- gemini-2.5-flash-preview-image-generation
+ gemini-3-pro-image-preview
+ gemini-2.5-flash-image📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| The application uses these models for image generation. These models are very recently released and do exist. | |
| gemini-3-pro-image-preview | |
| gemini-2.5-flash-preview-image-generation | |
| The application uses these models for image generation. These models are very recently released and do exist. | |
| gemini-3-pro-image-preview | |
| gemini-2.5-flash-image |
🤖 Prompt for AI Agents
In AGENTS.md around lines 5 to 7, the models list contains a retired/inaccurate
identifier; replace the incorrect "gemini-2.5-flash-preview-image-generation"
entry with the current, correct model identifier "gemini-2.5-flash-image" and
ensure the surrounding text still accurately reflects that these are recently
released models.
| dev: | ||
| npm run dev | ||
|
|
||
| sync-fork: | ||
| @echo "Fetching from upstream..." | ||
| git fetch upstream | ||
| @echo "Merging upstream/main into current branch..." | ||
| git merge upstream/main | ||
| @echo "Pushing to origin..." | ||
| git push origin | ||
| @echo "Fork synced successfully!" | ||
|
|
||
| setup-upstream: | ||
| @echo "Setting up upstream remote..." | ||
| @read -p "Enter upstream repository URL: " url; \ | ||
| git remote add upstream $$url | ||
| @echo "Upstream remote added. Run 'make sync-fork' to sync." No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add .PHONY declarations for all targets.
All three targets (dev, sync-fork, setup-upstream) are phony (not files) and should be declared to avoid conflicts with files of the same name and ensure correct behavior.
🔎 Proposed fix
+.PHONY: dev sync-fork setup-upstream
+
dev:
npm run dev
sync-fork:
@echo "Fetching from upstream..."
git fetch upstream
@echo "Merging upstream/main into current branch..."
git merge upstream/main
@echo "Pushing to origin..."
git push origin
@echo "Fork synced successfully!"
setup-upstream:
@echo "Setting up upstream remote..."
@read -p "Enter upstream repository URL: " url; \
git remote add upstream $$url
@echo "Upstream remote added. Run 'make sync-fork' to sync."📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| dev: | |
| npm run dev | |
| sync-fork: | |
| @echo "Fetching from upstream..." | |
| git fetch upstream | |
| @echo "Merging upstream/main into current branch..." | |
| git merge upstream/main | |
| @echo "Pushing to origin..." | |
| git push origin | |
| @echo "Fork synced successfully!" | |
| setup-upstream: | |
| @echo "Setting up upstream remote..." | |
| @read -p "Enter upstream repository URL: " url; \ | |
| git remote add upstream $$url | |
| @echo "Upstream remote added. Run 'make sync-fork' to sync." | |
| .PHONY: dev sync-fork setup-upstream | |
| dev: | |
| npm run dev | |
| sync-fork: | |
| @echo "Fetching from upstream..." | |
| git fetch upstream | |
| @echo "Merging upstream/main into current branch..." | |
| git merge upstream/main | |
| @echo "Pushing to origin..." | |
| git push origin | |
| @echo "Fork synced successfully!" | |
| setup-upstream: | |
| @echo "Setting up upstream remote..." | |
| @read -p "Enter upstream repository URL: " url; \ | |
| git remote add upstream $$url | |
| @echo "Upstream remote added. Run 'make sync-fork' to sync." |
🧰 Tools
🪛 checkmake (0.2.2)
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🤖 Prompt for AI Agents
In Makefile around lines 1 to 17, the targets dev, sync-fork and setup-upstream
are phony but not declared; add a .PHONY declaration listing these three targets
near the top of the file so Make won’t treat them as files and to ensure correct
behavior when files with those names exist.
| ``` | ||
| Image Grid Split Node → [image input] [tile image outputs...] | ||
| ``` |
There was a problem hiding this comment.
Add language specifier to fenced code block.
The code block at line 90 is missing a language specifier. Adding it improves syntax highlighting and tooling support.
🔎 Proposed fix
-```
+```text
Image Grid Split Node → [image input] [tile image outputs...]</details>
As per static analysis hints.
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
90-90: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In prd-utility-nodes.md around lines 90 to 92, the fenced code block lacks a
language specifier; update the backticks to include an appropriate language
(e.g., "text" or "plain") so the block becomes a labeled fenced code block which
enables proper syntax highlighting and tooling support.
| if [ $# -lt 3 ]; then | ||
| echo "Usage: $0 <input-image> <rows> <cols> [output-dir]" | ||
| echo "" | ||
| echo "Arguments:" | ||
| echo " input-image Path to the input image file" | ||
| echo " rows Number of rows in the grid" | ||
| echo " cols Number of columns in the grid" | ||
| echo " output-dir Output directory (default: ./tiles)" | ||
| echo "" | ||
| echo "Example:" | ||
| echo " $0 contact-sheet.jpg 2 3 ./output" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add validation for rows and columns parameters.
The script accepts rows and cols without validating they are positive integers. Invalid values (e.g., 0, negative, non-numeric) could cause unexpected behavior or cryptic errors from ffmpeg.
🔎 Proposed validation
ROWS="$2"
COLS="$3"
OUTPUT_DIR="${4:-./tiles}"
+# Validate rows and columns are positive integers
+if ! [[ "$ROWS" =~ ^[1-9][0-9]*$ ]]; then
+ echo "Error: Rows must be a positive integer (got: $ROWS)"
+ exit 1
+fi
+
+if ! [[ "$COLS" =~ ^[1-9][0-9]*$ ]]; then
+ echo "Error: Columns must be a positive integer (got: $COLS)"
+ exit 1
+fi
+
# Check if input file existsCommittable suggestion skipped: line range outside the PR's diff.
| function UtilityComboButton() { | ||
| const [isOpen, setIsOpen] = useState(false); | ||
| const menuRef = useRef<HTMLDivElement>(null); | ||
| const addNode = useWorkflowStore((state) => state.addNode); | ||
| const { screenToFlowPosition } = useReactFlow(); | ||
|
|
||
| useEffect(() => { | ||
| const handleClickOutside = (event: MouseEvent) => { | ||
| if (menuRef.current && !menuRef.current.contains(event.target as Node)) { | ||
| setIsOpen(false); | ||
| } | ||
| }; | ||
|
|
||
| if (isOpen) { | ||
| document.addEventListener("mousedown", handleClickOutside); | ||
| } | ||
|
|
||
| return () => { | ||
| document.removeEventListener("mousedown", handleClickOutside); | ||
| }; | ||
| }, [isOpen]); | ||
|
|
||
| const handleAddNode = (type: NodeType) => { | ||
| const center = getPaneCenter(); | ||
| const position = screenToFlowPosition({ | ||
| x: center.x + Math.random() * 100 - 50, | ||
| y: center.y + Math.random() * 100 - 50, | ||
| }); | ||
|
|
||
| addNode(type, position); | ||
| setIsOpen(false); | ||
| }; | ||
|
|
||
| const handleDragStart = (event: React.DragEvent, type: NodeType) => { | ||
| event.dataTransfer.setData("application/node-type", type); | ||
| event.dataTransfer.effectAllowed = "copy"; | ||
| setIsOpen(false); | ||
| }; | ||
|
|
||
| return ( | ||
| <div className="relative" ref={menuRef}> | ||
| <button | ||
| onClick={() => setIsOpen(!isOpen)} | ||
| className="px-2.5 py-1.5 text-[11px] font-medium text-neutral-400 hover:text-neutral-100 hover:bg-neutral-700 rounded transition-colors flex items-center gap-1" | ||
| > | ||
| Utility | ||
| <svg | ||
| className={`w-3 h-3 transition-transform ${ | ||
| isOpen ? "rotate-180" : "" | ||
| }`} | ||
| fill="none" | ||
| viewBox="0 0 24 24" | ||
| stroke="currentColor" | ||
| strokeWidth={2} | ||
| > | ||
| <path | ||
| strokeLinecap="round" | ||
| strokeLinejoin="round" | ||
| d="M5 15l7-7 7 7" | ||
| /> | ||
| </svg> | ||
| </button> | ||
|
|
||
| {isOpen && ( | ||
| <div className="absolute bottom-full left-0 mb-2 bg-neutral-800 border border-neutral-700 rounded-lg shadow-xl overflow-hidden min-w-[140px]"> | ||
| <button | ||
| onClick={() => handleAddNode("gridSplit")} | ||
| draggable | ||
| onDragStart={(e) => handleDragStart(e, "gridSplit")} | ||
| className="w-full px-3 py-2 text-left text-[11px] font-medium text-neutral-300 hover:bg-neutral-700 hover:text-neutral-100 transition-colors flex items-center gap-2 cursor-grab active:cursor-grabbing" | ||
| > | ||
| <svg | ||
| className="w-4 h-4" | ||
| fill="none" | ||
| viewBox="0 0 24 24" | ||
| stroke="currentColor" | ||
| strokeWidth={1.5} | ||
| > | ||
| <path | ||
| strokeLinecap="round" | ||
| strokeLinejoin="round" | ||
| d="M3.75 6A2.25 2.25 0 016 3.75h2.25A2.25 2.25 0 0110.5 6v2.25a2.25 2.25 0 01-2.25 2.25H6a2.25 2.25 0 01-2.25-2.25V6zM3.75 15.75A2.25 2.25 0 016 13.5h2.25a2.25 2.25 0 012.25 2.25V18a2.25 2.25 0 01-2.25 2.25H6A2.25 2.25 0 013.75 18v-2.25zM13.5 6a2.25 2.25 0 012.25-2.25H18A2.25 2.25 0 0120.25 6v2.25A2.25 2.25 0 0118 10.5h-2.25a2.25 2.25 0 01-2.25-2.25V6zM13.5 15.75a2.25 2.25 0 012.25-2.25H18a2.25 2.25 0 012.25 2.25V18A2.25 2.25 0 0118 20.25h-2.25A2.25 2.25 0 0113.5 18v-2.25z" | ||
| /> | ||
| </svg> | ||
| Grid Split | ||
| </button> | ||
| </div> | ||
| )} | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Refactor to eliminate code duplication with GenerateComboButton.
The UtilityComboButton component duplicates nearly all logic from GenerateComboButton (lines 57-168): state management, click-outside handling, node addition, drag-start behavior, and JSX structure. The only differences are the button label and menu items.
Consider extracting a reusable ComboButton component that accepts configuration props:
interface ComboButtonItem {
type: NodeType;
label: string;
icon: React.ReactNode;
}
interface ComboButtonProps {
label: string;
items: ComboButtonItem[];
}This would reduce code duplication by ~50%, improve maintainability, and make it easier to add future combo buttons.
🔎 Example refactored implementation
+interface ComboButtonItem {
+ type: NodeType;
+ label: string;
+ icon: React.ReactNode;
+}
+
+interface ComboButtonProps {
+ label: string;
+ items: ComboButtonItem[];
+}
+
+function ComboButton({ label, items }: ComboButtonProps) {
+ const [isOpen, setIsOpen] = useState(false);
+ const menuRef = useRef<HTMLDivElement>(null);
+ const addNode = useWorkflowStore((state) => state.addNode);
+ const { screenToFlowPosition } = useReactFlow();
+
+ useEffect(() => {
+ const handleClickOutside = (event: MouseEvent) => {
+ if (menuRef.current && !menuRef.current.contains(event.target as Node)) {
+ setIsOpen(false);
+ }
+ };
+
+ if (isOpen) {
+ document.addEventListener("mousedown", handleClickOutside);
+ }
+
+ return () => {
+ document.removeEventListener("mousedown", handleClickOutside);
+ };
+ }, [isOpen]);
+
+ const handleAddNode = (type: NodeType) => {
+ const center = getPaneCenter();
+ const position = screenToFlowPosition({
+ x: center.x + Math.random() * 100 - 50,
+ y: center.y + Math.random() * 100 - 50,
+ });
+
+ addNode(type, position);
+ setIsOpen(false);
+ };
+
+ const handleDragStart = (event: React.DragEvent, type: NodeType) => {
+ event.dataTransfer.setData("application/node-type", type);
+ event.dataTransfer.effectAllowed = "copy";
+ setIsOpen(false);
+ };
+
+ return (
+ <div className="relative" ref={menuRef}>
+ <button
+ onClick={() => setIsOpen(!isOpen)}
+ className="px-2.5 py-1.5 text-[11px] font-medium text-neutral-400 hover:text-neutral-100 hover:bg-neutral-700 rounded transition-colors flex items-center gap-1"
+ >
+ {label}
+ <svg
+ className={`w-3 h-3 transition-transform ${
+ isOpen ? "rotate-180" : ""
+ }`}
+ fill="none"
+ viewBox="0 0 24 24"
+ stroke="currentColor"
+ strokeWidth={2}
+ >
+ <path
+ strokeLinecap="round"
+ strokeLinejoin="round"
+ d="M5 15l7-7 7 7"
+ />
+ </svg>
+ </button>
+
+ {isOpen && (
+ <div className="absolute bottom-full left-0 mb-2 bg-neutral-800 border border-neutral-700 rounded-lg shadow-xl overflow-hidden min-w-[140px]">
+ {items.map((item) => (
+ <button
+ key={item.type}
+ onClick={() => handleAddNode(item.type)}
+ draggable
+ onDragStart={(e) => handleDragStart(e, item.type)}
+ className="w-full px-3 py-2 text-left text-[11px] font-medium text-neutral-300 hover:bg-neutral-700 hover:text-neutral-100 transition-colors flex items-center gap-2 cursor-grab active:cursor-grabbing"
+ >
+ {item.icon}
+ {item.label}
+ </button>
+ ))}
+ </div>
+ )}
+ </div>
+ );
+}Then replace both GenerateComboButton and UtilityComboButton with:
<ComboButton
label="Generate"
items={[
{ type: "nanoBanana", label: "Image", icon: <svg>...</svg> },
{ type: "llmGenerate", label: "Text (LLM)", icon: <svg>...</svg> }
]}
/>
<ComboButton
label="Utility"
items={[
{ type: "gridSplit", label: "Grid Split", icon: <svg>...</svg> }
]}
/>🤖 Prompt for AI Agents
src/components/FloatingActionBar.tsx lines 170-260: UtilityComboButton
duplicates nearly all logic from GenerateComboButton (state, click-outside
handling, add/drag behaviors and JSX); extract a reusable ComboButton component
that takes props { label: string, items: Array<{ type: NodeType, label: string,
icon: React.ReactNode }> } and moves shared logic (useState/isOpen, menuRef +
click-outside useEffect, handleAddNode, handleDragStart, rendering of button +
menu) into it, then replace both GenerateComboButton and UtilityComboButton with
instances of ComboButton that pass different label and items to remove
duplication and centralize behavior.
| status?: string; | ||
| icon?: ReactNode; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find and examine the BaseNode.tsx file
find . -name "BaseNode.tsx" -type f 2>/dev/null | head -5Repository: shrimbly/node-banana
Length of output: 99
🏁 Script executed:
#!/bin/bash
# Also search for the file path mentioned in the review
if [ -f "src/components/nodes/BaseNode.tsx" ]; then
echo "File found. Reading content:"
cat -n "src/components/nodes/BaseNode.tsx"
else
echo "File not found at specified path. Searching for it..."
find . -name "BaseNode.tsx" -type f 2>/dev/null
fiRepository: shrimbly/node-banana
Length of output: 3535
🏁 Script executed:
#!/bin/bash
# Search for where BaseNode is used with status or icon props
rg -n "BaseNode" --type tsx --type ts -g "!BaseNode.tsx" -A5 -B2 2>/dev/null | grep -E "(status|icon|BaseNode)" | head -50Repository: shrimbly/node-banana
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Search for BaseNode component usage across the codebase
rg -n "BaseNode" --type tsx --type ts -g "!BaseNode.tsx" 2>/dev/null | head -100Repository: shrimbly/node-banana
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Search specifically for status or icon being passed to BaseNode
rg -n -B5 -A5 "<BaseNode" --type tsx --type ts 2>/dev/null | grep -E "(status|icon|<BaseNode)" | head -50Repository: shrimbly/node-banana
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Search for imports of BaseNode
rg -n "from.*BaseNode|import.*BaseNode" --type tsx --type ts 2>/dev/nullRepository: shrimbly/node-banana
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Search for GridSplitNode which was mentioned in the review
rg -n "GridSplitNode" --type tsx --type ts 2>/dev/nullRepository: shrimbly/node-banana
Length of output: 46
🏁 Script executed:
#!/bin/bash
# List all files in the nodes directory to see what exists
ls -la src/components/nodes/ 2>/dev/null || find . -path "*/components/nodes*" -type f 2>/dev/null | head -20Repository: shrimbly/node-banana
Length of output: 826
🏁 Script executed:
#!/bin/bash
# Read GridSplitNode to see if it relates to status/icon props
cat -n src/components/nodes/GridSplitNode.tsxRepository: shrimbly/node-banana
Length of output: 8113
Remove unused status and icon props from BaseNode interface.
These props are declared in the interface but never destructured or rendered in the component. They're not passed by any parent components (including GridSplitNode, which manages status internally). Remove lines 17-18 to eliminate dead code.
🤖 Prompt for AI Agents
In src/components/nodes/BaseNode.tsx around lines 17-18, the BaseNode interface
declares unused props "status?: string;" and "icon?: ReactNode;"; remove those
two lines from the interface to eliminate dead code, then run the TypeScript
build/IDE checks and adjust or delete any now-unused imports or references if
the compiler surfaces any remaining usages.
| ? "border-2 border-blue-500 ring-2 ring-blue-500/20" | ||
| : "border border-neutral-700" | ||
| } | ||
| ${hasError ? "!border-red-500" : ""} |
There was a problem hiding this comment.
Critical: Fix Tailwind v4 important modifier syntax.
The important modifier syntax !border-red-500 is invalid in Tailwind v4. According to Tailwind v4 documentation, the ! modifier must come after the utility name, not before.
🔎 Required fix
- ${hasError ? "!border-red-500" : ""}
+ ${hasError ? "border-red-500!" : ""}Based on library documentation: Tailwind CSS v4 introduced a breaking change where the important modifier moved from the beginning (!text-3xl) to the end (text-3xl!).
🤖 Prompt for AI Agents
In src/components/nodes/BaseNode.tsx around line 86, the Tailwind important
modifier is using the old prefix syntax `!border-red-500`; change this to the
Tailwind v4 suffix form by outputting `border-red-500!` when hasError is true
(e.g. `${hasError ? "border-red-500!" : ""}`) so the generated class string uses
the new correct important modifier placement.
| case "gridSplit": | ||
| return { inputs: ["image"], outputs: ["tile"] }; // Dynamic outputs handled in node |
There was a problem hiding this comment.
Static "tile" output may not match dynamic handle IDs in multi-select connections.
The getNodeHandles returns outputs: ["tile"], but the actual GridSplitNode generates handles like tile-0, tile-1, etc. This mismatch could cause the multi-select connection logic (line 213) to skip gridSplit nodes when checking nodeHandles.outputs.includes(connection.sourceHandle).
Consider either:
- Updating this to return a pattern indicator, or
- Adjusting the check at line 213 to handle prefix matching for tile handles
Potential fix approach
// In handleConnect around line 211-217:
const nodeHandles = getNodeHandles(node.type || "");
-if (
- !nodeHandles.outputs.includes(connection.sourceHandle as string)
-) {
+const sourceHandle = connection.sourceHandle as string;
+const hasMatchingOutput = nodeHandles.outputs.includes(sourceHandle) ||
+ (node.type === "gridSplit" && sourceHandle.startsWith("tile-"));
+if (!hasMatchingOutput) {
// This node doesn't have the same output handle type, skip it
return;
}Committable suggestion skipped: line range outside the PR's diff.
| if (!image) { | ||
| console.error("[GridSplit] No image input found"); | ||
| updateNodeData(node.id, { | ||
| status: "error", | ||
| error: "Missing image input", | ||
| inputImage: null, | ||
| tileOutputs: {}, | ||
| }); | ||
| break; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the structure of executeWorkflow and the gridSplit case
cat -n src/store/workflowStore.ts | sed -n '1000,1150p'Repository: shrimbly/node-banana
Length of output: 5637
🏁 Script executed:
# Search for updateNodeData calls with error status and what follows them
rg -n "updateNodeData.*status.*error" src/store/workflowStore.ts -A 5Repository: shrimbly/node-banana
Length of output: 544
🏁 Script executed:
# Verify broader pattern by checking if other cases use return
rg -n "case \"" src/store/workflowStore.ts | head -20Repository: shrimbly/node-banana
Length of output: 479
🏁 Script executed:
# Check the loop structure around the switch statement
rg -n "for.*nodesToExecute" src/store/workflowStore.ts -A 2Repository: shrimbly/node-banana
Length of output: 46
🏁 Script executed:
# Get broader context around executeWorkflow function
sed -n '950,1000p' src/store/workflowStore.tsRepository: shrimbly/node-banana
Length of output: 1717
GridSplit uses break on error while other node types use return, creating inconsistent error handling.
When gridSplit fails (missing input or too many tiles), it uses break to exit the switch case but allows execution to continue. Other node types like nanoBanana and llmGenerate use return to halt the entire workflow. Since gridSplit failures prevent the node from producing output, downstream nodes expecting tiles will receive empty data. Align with the pattern used by other nodes by using return, or add a comment if non-blocking errors are intentional.
🤖 Prompt for AI Agents
In src/store/workflowStore.ts around lines 1060 to 1068, GridSplit uses "break"
when an image is missing which only exits the switch and lets execution
continue; replace the "break" with a "return" (matching nanoBanana/llmGenerate
behavior) so the function halts the workflow on this error and prevents
downstream nodes from receiving empty/invalid tile data, and ensure the
updateNodeData error state remains set before returning.
| export async function splitImageIntoTiles( | ||
| imageBase64: string, | ||
| rows: number, | ||
| columns: number | ||
| ): Promise<TileOutput[]> { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add input validation for rows and columns.
The function doesn't validate that rows and columns are positive integers within the allowed range. Per the PRD, valid ranges are 1-10 for rows/columns, with a maximum of 64 tiles total.
🔎 Proposed validation
export async function splitImageIntoTiles(
imageBase64: string,
rows: number,
columns: number
): Promise<TileOutput[]> {
+ // Validate inputs per PRD constraints
+ if (!Number.isInteger(rows) || rows < 1 || rows > 10) {
+ throw new Error(`Invalid rows: ${rows}. Must be an integer between 1 and 10.`);
+ }
+ if (!Number.isInteger(columns) || columns < 1 || columns > 10) {
+ throw new Error(`Invalid columns: ${columns}. Must be an integer between 1 and 10.`);
+ }
+ if (rows * columns > 64) {
+ throw new Error(`Too many tiles: ${rows * columns}. Maximum is 64.`);
+ }
+
return new Promise((resolve, reject) => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function splitImageIntoTiles( | |
| imageBase64: string, | |
| rows: number, | |
| columns: number | |
| ): Promise<TileOutput[]> { | |
| export async function splitImageIntoTiles( | |
| imageBase64: string, | |
| rows: number, | |
| columns: number | |
| ): Promise<TileOutput[]> { | |
| // Validate inputs per PRD constraints | |
| if (!Number.isInteger(rows) || rows < 1 || rows > 10) { | |
| throw new Error(`Invalid rows: ${rows}. Must be an integer between 1 and 10.`); | |
| } | |
| if (!Number.isInteger(columns) || columns < 1 || columns > 10) { | |
| throw new Error(`Invalid columns: ${columns}. Must be an integer between 1 and 10.`); | |
| } | |
| if (rows * columns > 64) { | |
| throw new Error(`Too many tiles: ${rows * columns}. Maximum is 64.`); | |
| } | |
| return new Promise((resolve, reject) => { |
🤖 Prompt for AI Agents
In src/utils/imageSplitter.ts around lines 21 to 25, add input validation at the
start of splitImageIntoTiles: verify that rows and columns are integers, >0 and
<=10, and that rows * columns <= 64; if any check fails, throw or return a
rejected Promise with a clear error message (e.g., "rows and columns must be
integers between 1 and 10" or "total tiles (rows*columns) must not exceed 64");
perform type checks (Number.isInteger) to avoid NaN/floats and run these
validations before any image processing logic.
|
closing in favor of #4 |
1 participant
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.