Redesign image generation node with unified headers and Next.js Image migration #2051
Conversation
… icon spacing; tidy EditableText padding control
…nel; unify model parameter UI using SettingRow; remove duplicate provider/model selects
…odel parameter spacing; align image-gen overlays and output label with text-gen
…thumbnails to match main
|
|
Finished running flow.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRefactors many properties panels to use a new NodePanelHeader (docs/delete), redesigns the image-generation node into prompt + generation overlays with new hooks and UI (GenerateCtaButton, ImageCard, Lightbox), removes legacy source utilities and pluralize dependency, and migrates several Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ImageGenPanel
participant PromptPanel
participant GenerationPanel
participant OverlayManager
User->>ImageGenPanel: Open image-generation node
ImageGenPanel->>PromptPanel: render(with onExpand)
ImageGenPanel->>GenerationPanel: render(with onExpand)
User->>PromptPanel: Click expand
PromptPanel->>OverlayManager: open PromptEditor overlay
OverlayManager->>User: show expanded PromptEditor
User->>PromptPanel: Select model / edit prompt
PromptPanel->>ImageGenPanel: update node model/data (disconnect invalid connections)
User->>GenerationPanel: Click Generate or Expand
GenerationPanel->>OverlayManager: open Generation overlay
OverlayManager->>User: show expanded Generation view
User->>OverlayManager: Close overlay
OverlayManager->>ImageGenPanel: sync live prompt/state back
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (4)**/*.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/development-guide.mdc)
Files:
**/*.tsx📄 CodeRabbit inference engine (.cursor/rules/development-guide.mdc)
Files:
**/*📄 CodeRabbit inference engine (.cursor/rules/naming-guide.mdc)
Files:
**/*.{js,jsx,ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/naming-guide.mdc)
Files:
🧬 Code graph analysis (1)internal-packages/workflow-designer-ui/src/editor/properties-panel/web-page-node-properties-panel/index.tsx (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (2)
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 |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||||||||
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…ize exports; minor lint fixes
…e unused imports/vars
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal-packages/workflow-designer-ui/src/editor/properties-panel/image-generation-node-properties-panel/sources/use-connected-sources.ts (1)
121-121: Fix typo in error message.The error message contains a typo: "vectore" should be "vector".
Apply this diff to fix the typo:
- throw new Error("vectore store can not be connected"); + throw new Error("vector store can not be connected");internal-packages/workflow-designer-ui/src/editor/properties-panel/action-node-properties-panel/index.tsx (1)
38-41: Fix sourceNodes filter to exclude undefinedCurrent filter keeps undefined (undefined !== null is true), risking runtime errors.
- sourceNodes: connectedInputs - .map((input) => input.connectedOutput?.node as Node) - .filter((node) => node !== null), + sourceNodes: connectedInputs + .map((input) => input.connectedOutput?.node) + .filter((n): n is Node => Boolean(n)),internal-packages/workflow-designer-ui/src/editor/properties-panel/image-generation-node-properties-panel/index.tsx (1)
50-50: ComputenodesfromconnectedSourcesfor the expanded PromptEditor.The
connectedSourcesare retrieved but not transformed into thenodesarray needed by the expandedPromptEditor. Following the pattern inprompt-panel.tsx(lines 44-50), add:const { all: connectedSources } = useConnectedSources(node); +const nodes = useMemo( + () => + connectedSources + .map((source) => Node.safeParse(source.node)) + .map((parse) => (parse.success ? parse.data : null)) + .filter((data) => data !== null), + [connectedSources], +);Then pass
nodesto the expandedPromptEditorat line 303.
♻️ Duplicate comments (4)
internal-packages/workflow-designer-ui/src/editor/properties-panel/ui/generate-cta-button.tsx (1)
26-27: Remove stray console.log from onClickDebug logging should not ship.
- onClick={() => { - console.log("GenerateCtaButton clicked!"); - onClick(); - }} + onClick={() => { + onClick(); +}}internal-packages/workflow-designer-ui/src/editor/properties-panel/image-generation-node-properties-panel/index.tsx (3)
102-125: Remove unused dead code.The
_disconnectInvalidConnectionsfunction is never called. This functionality has been moved toprompt-panel.tsx(lines 58-79).
135-170: Remove debug console.log statements.Console.log statements at lines 136, 138, 143, and 148 should be removed before merging to production.
303-319: Missingnodesprop in expanded PromptEditor.The expanded
PromptEditor(line 303) is missing thenodesprop that the inlinePromptPanelprovides. Looking atprompt-panel.tsx(lines 152-166), the inline editor includes:nodes={nodes}This prop enables proper text editor functionality with connected sources. Add it for consistency:
<PromptEditor key={`expanded-${editorVersion}-${node.id}`} value={livePrompt ?? node.content.prompt} onValueChange={(value) => { updateNodeDataContent(node, { prompt: value }); }} + nodes={nodes} connectedSources={connectedSources.map(({ node, output }) => ({ node, output, }))}
🧹 Nitpick comments (31)
internal-packages/workflow-designer-ui/src/editor/properties-panel/image-generation-node-properties-panel/sources/use-connected-sources.ts (1)
1-22: Type definition improves type safety.The new local
ConnectedSource<T>generic type with explicitconnectionproperty is a good improvement for type safety and clarity.Note: A similar
ConnectedSourcetype exists inpackages/text-editor/src/react/component.tsx(lines 123-126) but without theconnectionproperty. Consider consolidating these types into a shared location to avoid confusion and maintain consistency across the codebase.docs/packages-license.md (1)
4-24: Add generation metadata for reproducibilityRecommend adding a short header with generation timestamp, command, and commit (e.g., “Generated by: pnpm license-finder at on ”). Improves auditability and CI drift detection.
internal-packages/ui/components/model-picker.tsx (2)
17-18: Remove unuseddisabledReasonproperty or document its purpose.The
disabledReasonproperty is defined in the type but never used in the component (line 214 explicitly notes it's not shown). This creates unnecessary confusion.Either remove it from the type definition:
disabled?: boolean; - disabledReason?: string; }>;Or, if it's intentionally kept for future use or backward compatibility, add a JSDoc comment explaining this:
disabled?: boolean; + /** Reserved for future use. Currently not displayed in UI. */ disabledReason?: string; }>;
180-221: Consider adding an empty state message.When search or filter results in no matching models, the dropdown displays blank content without feedback to the user. Adding an empty state message would improve the user experience.
Add an empty state check before rendering the model list:
<div className="mt-[0px] mx-[4px] pr-[4px]"> <div className="flex flex-col gap-[6px] max-h-[220px] overflow-y-auto"> + {filteredGroups.every((g) => g.models.length === 0) ? ( + <div className="text-center py-8 text-text-muted text-[14px]"> + No models found + </div> + ) : ( {filteredGroups.map((group) => ( <div key={group.provider} className="flex flex-col gap-[4px]"> {group.label ? ( <div className="text-[11px] text-secondary px-[4px]"> {group.label} </div> ) : null} {group.models.map((m) => { const isDisabled = Boolean(m.disabled); return ( <button key={`${group.provider}-${m.id}`} type="button" onClick={() => { if (isDisabled) return; onSelect(group.provider, m.id); setOpen(false); }} aria-disabled={isDisabled} className={clsx( "flex gap-[12px] items-center p-[4px] rounded-[4px] text-left", isDisabled ? "opacity-50 cursor-not-allowed" : "hover:bg-white/5 focus:bg-white/5 cursor-pointer", )} > <div className="flex items-center gap-[8px]"> <p className="text-[14px] text-left text-nowrap"> {m.label || m.id} </p> {m.badge} </div> {/* disabled reason text intentionally not shown (design: gray-out only) */} </button> ); })} </div> ))} + )} </div> </div>packages/giselle/src/engine/generations/generate-image.ts (2)
179-205: Consider extracting common image processing logic.The three provider functions share nearly identical image processing logic (detect type, generate ID, store, create Image object). Consider extracting this into a shared helper function to reduce duplication.
Example extraction:
async function processGeneratedImages({ images, context, runningGeneration, }: { images: Array<{ uint8Array: Uint8Array; base64: string }>; context: GiselleEngineContext; runningGeneration: RunningGeneration; }): Promise<Image[]> { const contents = await Promise.all( images.map(async (image) => { const imageType = detectImageType(image.uint8Array); if (imageType === null) { return null; } const id = ImageId.generate(); const filename = `${id}.${imageType.ext}`; await setGeneratedImage({ storage: context.storage, generation: runningGeneration, generatedImage: { uint8Array: image.uint8Array, base64: image.base64, } satisfies GeneratedImageData, generatedImageFilename: filename, }); return { id, contentType: imageType.contentType, filename, pathname: `generations/${runningGeneration.id}/generated-images/${filename}`, } satisfies Image; }), ); return contents.filter((result) => result !== null); }Then each provider function can call:
const contents = await processGeneratedImages({ images: result.images, context, runningGeneration, });Also applies to: 262-288, 330-356
152-163: Consider adding separators when concatenating message content.The prompt extraction logic concatenates text content from multiple messages without separators. This could result in run-together text (e.g., "HelloWorld" instead of "Hello World"), potentially affecting prompt quality.
Consider adding separators:
let prompt = ""; for (const message of messages) { if (!Array.isArray(message.content)) { continue; } for (const content of message.content) { if (content.type !== "text") { continue; } - prompt += content.text; + prompt += (prompt ? " " : "") + content.text; } }Or extract this logic to a shared helper function to reduce duplication.
Also applies to: 232-243
internal-packages/workflow-designer-ui/src/ui/constants.ts (2)
1-1: Consider camelCase + export a helper type for consumers.To align with our TS naming guideline (camelCase for vars), consider
thumbHeight. Also export a size key type to avoid magic strings at call sites.-export const THUMB_HEIGHT = { sm: 220, md: 260 } as const; +export const thumbHeight = { sm: 220, md: 260 } as const; +export type ThumbSize = keyof typeof thumbHeight;
2-3: Remove commented-out exports or move them to design tokens.Commented code adds noise and can drift. Either delete or re‑introduce as live tokens in a dedicated design tokens module.
internal-packages/workflow-designer-ui/src/editor/properties-panel/image-generation-node-properties-panel/models/fal.tsx (1)
29-52: LGTM – consistent with SettingRow pattern.Layout and a11y look good (label ↔ htmlFor/id). If
Selectsupports generics, you can drop the cast by typing the option union.- onValueChange={(value) => + onValueChange={(value: FalLanguageModelData["configurations"]["size"]) => updateConfiguration({ size: value }) }internal-packages/workflow-designer-ui/src/ui/hooks.ts (3)
3-4: Import React types explicitly.Avoid referencing the global
Reactnamespace for types; import the type to keep TS happy across configs.-import { useEffect, useState } from "react"; +import { useEffect, useState, type RefObject } from "react";Then change parameter types to
RefObject<HTMLElement>.
13-34: Handle ref element swaps; reduce initial layout flicker.Current effect depends on
refidentity, not the underlyingref.current. If the element changes, observers won’t reattach. Also, measure synchronously to avoid a frame of 0.-export function useOverlayBottom(ref: React.RefObject<HTMLElement>) { +export function useOverlayBottom(ref: RefObject<HTMLElement>) { const [overlayBottomPx, setOverlayBottomPx] = useState(0); - useEffect(() => { + useEffect(() => { const el = ref.current; if (!el) { setOverlayBottomPx(0); return; } - const update = () => setOverlayBottomPx(el.offsetHeight || 0); + const update = () => setOverlayBottomPx(el.offsetHeight || 0); update(); const ro = new ResizeObserver(update); ro.observe(el); window.addEventListener("resize", update); return () => { ro.disconnect(); window.removeEventListener("resize", update); }; - }, [ref]); + // Re-run when the concrete element changes + }, [ref.current]);
36-64: Broaden container fallback and throttle scroll/resize updates.Relying on
.closest(".relative")may fail if the container lacks that class. Also, unthrottled scroll/resize can be chatty.-export function useElementTopPx(ref: React.RefObject<HTMLElement>) { +export function useElementTopPx(ref: RefObject<HTMLElement>) { const [topPx, setTopPx] = useState(0); useEffect(() => { const el = ref.current; if (!el) { setTopPx(0); return; } - const update = () => { + let raf = 0; + const getContainerRect = () => { + const c = el.closest(".relative") as HTMLElement | null; + // fallback to offsetParent if no .relative ancestor + const fallback = (el.offsetParent as HTMLElement | null) ?? null; + return (c ?? fallback)?.getBoundingClientRect(); + }; + const updateSync = () => { const rect = el.getBoundingClientRect(); - const container = el.closest(".relative") as HTMLElement | null; - const containerRect = container?.getBoundingClientRect(); + const containerRect = getContainerRect(); setTopPx(containerRect ? rect.top - containerRect.top : 0); - }; - update(); - const ro = new ResizeObserver(update); + }; + const update = () => { + cancelAnimationFrame(raf); + raf = requestAnimationFrame(updateSync); + }; + updateSync(); + const ro = new ResizeObserver(updateSync); ro.observe(el); window.addEventListener("resize", update); window.addEventListener("scroll", update, true); return () => { ro.disconnect(); + cancelAnimationFrame(raf); window.removeEventListener("resize", update); window.removeEventListener("scroll", update, true); }; - }, [ref]); + }, [ref.current]);internal-packages/workflow-designer-ui/src/editor/properties-panel/image-generation-node-properties-panel/generation-panel.tsx (1)
43-48: Remove unused_handleGenerateor wire it to the Empty state.The handler is defined but never used. Either pass it into
Empty(and add anonGenerateprop there) or delete the handler and theonClickGenerateButtonprop from this component.-const _handleGenerate = useCallback(() => { - if (onClickGenerateButton) { - onClickGenerateButton(); - } -}, [onClickGenerateButton]);internal-packages/workflow-designer-ui/src/editor/properties-panel/image-generation-node-properties-panel/prompt-panel.tsx (1)
46-53: Narrow the type afterfilterto avoid(Node | null)[].Without a type guard, TS may retain
nullin the array, breakingnodes.map((n) => n.id).-const nodes = useMemo( - () => - connectedSources - .map((source) => Node.safeParse(source.node)) - .map((parse) => (parse.success ? parse.data : null)) - .filter((data) => data !== null), - [connectedSources], -); +const nodes = useMemo( + () => + connectedSources + .map((source) => Node.safeParse(source.node)) + .map((p) => (p.success ? p.data : null)) + .filter((d): d is Node => d !== null), + [connectedSources], +);internal-packages/workflow-designer-ui/src/ui/image-card.tsx (3)
17-23: Addsizesprop to Next.js Image for better optimization.The Next.js Image component should include a
sizesprop to help the framework select appropriate image sizes for different viewports. Without it, Next.js may serve suboptimal image sizes.Apply this diff:
<Image src={src} alt="generated file" width={512} height={512} + sizes="(max-width: 768px) 100vw, 512px" className="h-full w-auto object-contain rounded-[8px]" />
19-19: Improve alt text descriptiveness.The alt text "generated file" is generic. Consider making it more descriptive or allowing it to be customized via props for better accessibility.
For example:
-<Image - src={src} - alt="generated file" +<Image + src={src} + alt="Generated image"Or add an
altprop to the component if different descriptions are needed per usage.
26-42: Consider adding visible button hover states.While the icons have hover effects (scale and translate), the buttons themselves lack visible hover/focus states, which may impact discoverability and accessibility.
Consider adding button hover styles:
<button type="button" onClick={onDownload} - className="p-2" + className="p-2 rounded hover:bg-white/20 focus-visible:outline focus-visible:outline-2 focus-visible:outline-white transition-colors" title="Download" >internal-packages/workflow-designer-ui/src/editor/properties-panel/ui/node-panel-header.tsx (2)
60-68: Consider adding delete confirmation.The delete button triggers
onDelete()immediately without confirmation. For a destructive action like node deletion, users might benefit from a confirmation dialog to prevent accidental deletions.Consider adding a confirmation step:
import { useState } from "react"; // In component: const [showConfirm, setShowConfirm] = useState(false); // Replace direct onClick: onClick={() => { if (window.confirm("Are you sure you want to delete this node?")) { onDelete(); } }}Alternatively, implement a more sophisticated confirmation modal for better UX.
33-57: Consider extracting inline SVG to a reusable component.The external link SVG icon is defined inline. If this icon is used elsewhere or might be reused, extracting it to a shared icon component would improve maintainability.
For example, create
ExternalLinkIconcomponent:// components/icons/external-link-icon.tsx export function ExternalLinkIcon({ className }: { className?: string }) { return ( <svg className={className} width="14" height="14" viewBox="0 0 24 24" // ... rest of SVG > {/* paths */} </svg> ); }Then use:
<ExternalLinkIcon className="size-[14px]" />internal-packages/workflow-designer-ui/src/editor/properties-panel/image-generation-node-properties-panel/models/openai.tsx (2)
72-75: Avoid iterator name shadowing imported symbolsShadowing openaiImageQuality/openaiImageBackground/openaiImageModeration in map callbacks hurts readability and can confuse tooling.
- options={openaiImageQuality.options.map((openaiImageQuality) => ({ - value: openaiImageQuality, - label: openaiImageQuality, - }))} + options={openaiImageQuality.options.map((opt) => ({ + value: opt, + label: opt, + }))} - options={openaiImageBackground.options.map((openaiImageBackground) => ({ - value: openaiImageBackground, - label: openaiImageBackground, - }))} + options={openaiImageBackground.options.map((opt) => ({ + value: opt, + label: opt, + }))} - options={openaiImageModeration.options.map((openaiImageModeration) => ({ - value: openaiImageModeration, - label: openaiImageModeration, - }))} + options={openaiImageModeration.options.map((opt) => ({ + value: opt, + label: opt, + }))}Also applies to: 101-106, 132-137
23-31: Ensure label-for/id uniqueness per instanceHard-coded ids ("size", "quality", etc.) can collide if multiple panels mount. Use useId to namespace.
+import { useId } from "react"; ... export function OpenAIImageModelPanel({ languageModel, onModelChange }: { ... }) { - return ( + const idBase = useId(); + return ( <div className="flex flex-col gap-[8px]"> <div className="grid grid-cols-1 gap-[8px]"> <SettingRow - label={<label htmlFor="size" className="text-text text-[14px]">Size</label>} + label={<label htmlFor={`${idBase}-size`} className="text-text text-[14px]">Size</label>} > <Select - id="size" + id={`${idBase}-size`} ... - label={<label htmlFor="quality" className="text-text text-[14px]">Quality</label>} + label={<label htmlFor={`${idBase}-quality`} className="text-text text-[14px]">Quality</label>} <Select - id="quality" + id={`${idBase}-quality`} ... - label={<label htmlFor="background" className="text-text text-[14px]">Background</label>} + label={<label htmlFor={`${idBase}-background`} className="text-text text-[14px]">Background</label>} <Select - id="background" + id={`${idBase}-background`} ... - label={<label htmlFor="moderation" className="text-text text-[14px]">Moderation</label>} + label={<label htmlFor={`${idBase}-moderation`} className="text-text text-[14px]">Moderation</label>} <Select - id="moderation" + id={`${idBase}-moderation`}Also applies to: 52-60, 81-89, 112-120
internal-packages/workflow-designer-ui/src/ui/lightbox.tsx (1)
38-45: Prevent accidental close and add Image sizes/altStop click bubbling on the content container and provide sizes for Next Image; allow custom alt.
-export function Lightbox({ - src, - onClose, -}: { - src: string; - onClose: () => void; -}) { +export function Lightbox({ + src, + onClose, + alt = "Generated content", +}: { + src: string; + onClose: () => void; + alt?: string; +}) { ... - <div className="relative max-w-[95vw] max-h-[95vh] w-[95vw] h-[95vh]"> + <div + className="relative max-w-[95vw] max-h-[95vh] w-[95vw] h-[95vh]" + onClick={(e) => e.stopPropagation()} + > <Image src={src} - alt="Generated content" + alt={alt} fill + sizes="95vw" style={{ objectFit: "contain" }} />internal-packages/workflow-designer-ui/src/ui/editable-text.tsx (1)
43-47: Only emit changes when value actually changed; add Escape to cancelReduces unnecessary store updates and improves UX.
-const updateText = useCallback(() => { - setEdit(false); - const newTextValue = inputRef.current?.value ?? ""; - onValueChange?.(newTextValue); -}, [onValueChange]); +const updateText = useCallback(() => { + setEdit(false); + const newTextValue = inputRef.current?.value ?? ""; + const prev = text ?? ""; + if (newTextValue !== prev) onValueChange?.(newTextValue); +}, [onValueChange, text]); ... onKeyDown={(e) => { if (e.key === "Enter") { e.preventDefault(); updateText(); } + if (e.key === "Escape") { + e.preventDefault(); + setEdit(false); + if (inputRef.current) inputRef.current.value = text ?? ""; + } }}Also applies to: 63-69
internal-packages/workflow-designer-ui/src/editor/properties-panel/web-page-node-properties-panel/index.tsx (2)
246-251: Omit undefined propPassing onDelete={undefined} is redundant; omit for clarity.
- <NodePanelHeader - node={node} - onChangeName={(name) => updateNodeData(node, { name })} - docsUrl="https://docs.giselles.ai/en/glossary/web-page-node" - onDelete={undefined} - /> + <NodePanelHeader + node={node} + onChangeName={(name) => updateNodeData(node, { name })} + docsUrl="https://docs.giselles.ai/en/glossary/web-page-node" + />
201-207: Propagate actual error message on fetch failureCurrently errorMessage is set to the URL, losing context. Capture the thrown error message.
-} catch (_err) { +} catch (_err) { + const message = + _err instanceof Error ? _err.message : String(_err ?? "Unknown error"); const failedWebPage: WebPage = { id: newWebPage.id, status: "failed", url: newWebPage.url, - errorMessage: newWebPage.url, + errorMessage: message, };internal-packages/workflow-designer-ui/src/editor/properties-panel/ui/generate-cta-button.tsx (1)
23-37: Expose generating state to ATAdd aria-busy and better label when generating for screen readers.
-<button +<button type="button" onClick={() => { onClick(); }} disabled={!isGenerating && isEmpty} + aria-busy={isGenerating} + aria-live="polite" + aria-label={isGenerating ? "Stop generation" : (isEmpty ? emptyLabel : idleLabel)}Also applies to: 43-49
internal-packages/workflow-designer-ui/src/editor/properties-panel/action-node-properties-panel/index.tsx (1)
63-67: Disable Run Action when inputs invalidPrevents needless error flashes; handleClick can still guard.
- <Button type="button" onClick={handleClick} className="w-full"> + <Button type="button" onClick={handleClick} className="w-full" disabled={!isValid}>internal-packages/workflow-designer-ui/src/editor/properties-panel/image-generation-node-properties-panel/index.tsx (2)
172-236: Consider extracting repeated positioning logic into a custom hook.Three similar
useEffectblocks calculate overlay positions usingResizeObserverand window event listeners. This pattern could be extracted into a reusable hook (e.g.,useElementPosition(ref)) to reduce duplication and improve maintainability.Example:
function useElementTopPx(ref: RefObject<HTMLElement>) { const [topPx, setTopPx] = useState(0); useEffect(() => { const el = ref.current; if (!el) { setTopPx(0); return; } const update = () => { const rect = el.getBoundingClientRect(); const container = el.closest(".relative"); const containerRect = container?.getBoundingClientRect(); setTopPx(containerRect ? rect.top - containerRect.top : 0); }; update(); const ro = new ResizeObserver(update); ro.observe(el); window.addEventListener("resize", update); window.addEventListener("scroll", update, true); return () => { ro.disconnect(); window.removeEventListener("resize", update); window.removeEventListener("scroll", update, true); }; }, []); return topPx; }
286-343: Consider adding focus trap for overlay dialogs.The expanded overlay dialogs (prompt and generation) have good ARIA attributes but might benefit from a focus trap to prevent keyboard navigation from escaping the modal. Consider using a library like
focus-trap-reactor ensuring Tab/Shift+Tab cycles within the dialog.Also applies to: 346-383
internal-packages/workflow-designer-ui/src/editor/workspace-tour/workspace-tour.tsx (2)
561-567: Simplify arrow Image props to avoid redundancy.Both arrow images specify
widthprops that match theclassNamevalues (w-[150px]), creating redundancy. For fixed-size decorative images, you can rely on the className for sizing or omit explicit dimensions when using static imports.Apply this diff to simplify the step2Arrow usage:
<Image src={step2Arrow} alt="Arrow pointing to toolbar" - width={150} - height={100} className="absolute bottom-[-100px] left-[calc(50%-200px)] translate-x-[-50%] z-[60] w-[150px] h-auto pointer-events-none arrow-animation" />Apply this diff to simplify the step5Arrow usage:
<Image src={step5Arrow} alt="Arrow pointing to tabs" - width={150} - height={100} className="absolute top-[-110px] left-[calc(50%-190px)] z-[60] w-[150px] h-auto pointer-events-none arrow-animation" />Also applies to: 695-701
744-750: Consider omitting explicit width/height for consistency.Similar to the TourCard image, the docsImage uses responsive sizing classes (
w-full h-full object-cover) while also specifying explicitwidth={800}andheight={300}. For static imports, these explicit dimensions are optional and may not match the actual image file dimensions.Apply this diff for consistency and to let Next.js handle dimensions:
<Image src={docsImage} alt="Tour step tutorial" - width={800} - height={300} className="w-full h-full object-cover object-top" />
| {group.models.map((m) => { | ||
| const isDisabled = Boolean(m.disabled); | ||
| return ( | ||
| <button | ||
| key={`${group.provider}-${m.id}`} | ||
| type="button" | ||
| onClick={() => { | ||
| if (isDisabled) return; | ||
| onSelect(group.provider, m.id); | ||
| setOpen(false); | ||
| }} | ||
| aria-disabled={isDisabled} | ||
| className={clsx( | ||
| "flex gap-[12px] items-center p-[4px] rounded-[4px] text-left", | ||
| isDisabled | ||
| ? "opacity-50 cursor-not-allowed" | ||
| : "hover:bg-white/5 focus:bg-white/5 cursor-pointer", | ||
| )} | ||
| > | ||
| <div className="flex items-center gap-[8px]"> | ||
| <p className="text-[14px] text-left text-nowrap"> | ||
| {m.label || m.id} | ||
| </p> | ||
| {m.badge} | ||
| </div> | ||
| {/* disabled reason text intentionally not shown (design: gray-out only) */} | ||
| </button> | ||
| ); | ||
| })} |
There was a problem hiding this comment.
Use disabled attribute instead of aria-disabled for better accessibility.
The current implementation uses aria-disabled while still keeping the button focusable and in the keyboard tab order. When isDisabled is true, keyboard users can focus the button but activation does nothing, creating a confusing experience.
According to WAI-ARIA best practices, aria-disabled should only be used when an element needs to remain in the tab order for context. For truly non-interactive items, the HTML disabled attribute is more appropriate as it removes the element from the tab order entirely.
Apply this diff to use the native disabled attribute:
{group.models.map((m) => {
const isDisabled = Boolean(m.disabled);
return (
<button
key={`${group.provider}-${m.id}`}
type="button"
+ disabled={isDisabled}
onClick={() => {
- if (isDisabled) return;
onSelect(group.provider, m.id);
setOpen(false);
}}
- aria-disabled={isDisabled}
className={clsx(
"flex gap-[12px] items-center p-[4px] rounded-[4px] text-left",
isDisabled
- ? "opacity-50 cursor-not-allowed"
- : "hover:bg-white/5 focus:bg-white/5 cursor-pointer",
+ ? "opacity-50 cursor-not-allowed disabled:pointer-events-none"
+ : "hover:bg-white/5 focus:bg-white/5 cursor-pointer enabled:hover:bg-white/5",
)}
>Note: If you need disabled items to remain in tab order for specific UX reasons, please document that decision and ensure proper keyboard event handling (Enter/Space keys) is implemented.
📝 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.
| {group.models.map((m) => { | |
| const isDisabled = Boolean(m.disabled); | |
| return ( | |
| <button | |
| key={`${group.provider}-${m.id}`} | |
| type="button" | |
| onClick={() => { | |
| if (isDisabled) return; | |
| onSelect(group.provider, m.id); | |
| setOpen(false); | |
| }} | |
| aria-disabled={isDisabled} | |
| className={clsx( | |
| "flex gap-[12px] items-center p-[4px] rounded-[4px] text-left", | |
| isDisabled | |
| ? "opacity-50 cursor-not-allowed" | |
| : "hover:bg-white/5 focus:bg-white/5 cursor-pointer", | |
| )} | |
| > | |
| <div className="flex items-center gap-[8px]"> | |
| <p className="text-[14px] text-left text-nowrap"> | |
| {m.label || m.id} | |
| </p> | |
| {m.badge} | |
| </div> | |
| {/* disabled reason text intentionally not shown (design: gray-out only) */} | |
| </button> | |
| ); | |
| })} | |
| {group.models.map((m) => { | |
| const isDisabled = Boolean(m.disabled); | |
| return ( | |
| <button | |
| key={`${group.provider}-${m.id}`} | |
| type="button" | |
| disabled={isDisabled} | |
| onClick={() => { | |
| onSelect(group.provider, m.id); | |
| setOpen(false); | |
| }} | |
| className={clsx( | |
| "flex gap-[12px] items-center p-[4px] rounded-[4px] text-left", | |
| isDisabled | |
| ? "opacity-50 cursor-not-allowed disabled:pointer-events-none" | |
| : "hover:bg-white/5 focus:bg-white/5 cursor-pointer enabled:hover:bg-white/5", | |
| )} | |
| > | |
| <div className="flex items-center gap-[8px]"> | |
| <p className="text-[14px] text-left text-nowrap"> | |
| {m.label || m.id} | |
| </p> | |
| {m.badge} | |
| </div> | |
| {/* disabled reason text intentionally not shown (design: gray-out only) */} | |
| </button> | |
| ); | |
| })} |
🤖 Prompt for AI Agents
internal-packages/ui/components/model-picker.tsx lines 189-217: replace the use
of aria-disabled with the native disabled attribute on the <button> when
isDisabled is true so the button is removed from tab order and behaves natively
for keyboard users; keep the early return in onClick (or rely on the disabled
attribute) and adjust className logic so disabled state still applies opacity
and cursor-not-allowed while removing hover/focus styles for that state; ensure
no other code relies on the element remaining focusable (if it must remain
focusable, document and implement explicit keyboard handling instead).
| export function GenerationPanel({ | ||
| node, | ||
| onClickGenerateButton, | ||
| onExpand, | ||
| isExpanded, | ||
| }: { | ||
| node: ImageGenerationNode; | ||
| onClickGenerateButton?: () => void; | ||
| onExpand?: () => void; | ||
| isExpanded?: boolean; | ||
| }) { |
There was a problem hiding this comment.
🧩 Analysis chain
Verify: Is the generate CTA intentionally removed from the empty state?
onClickGenerateButton is no longer used and <Empty /> has no CTA. If generation is now triggered solely from elsewhere (e.g., a global GenerateCtaButton), all call sites should drop onClickGenerateButton. Otherwise, reintroduce the CTA.
Also applies to: 49-51
🏁 Script executed:
#!/bin/bash
# Find usages of GenerationPanel and check if onClickGenerateButton is still passed.
rg -nP --type=ts --type=tsx -C2 '\bGenerationPanel\s*\('
rg -nP --type=ts --type=tsx -C2 'onClickGenerateButton='Length of output: 55
</script>
🏁 Script executed:
#!/bin/bash
# Search for GenerationPanel usages without --type flag
rg -nP '\bGenerationPanel\s*\(' -g '*.ts' -g '*.tsx' -C2
echo "---"
# Search for onClickGenerateButton parameter passing
rg -nP 'onClickGenerateButton=' -g '*.ts' -g '*.tsx' -C2
echo "---"
# Inspect the file to see lines 49-51
cat internal-packages/workflow-designer-ui/src/editor/properties-panel/image-generation-node-properties-panel/generation-panel.tsx | sed -n '1,60p'Length of output: 8156
</script>
🏁 Script executed:
#!/bin/bash
# Get full file content to see where _handleGenerate is used
wc -l internal-packages/workflow-designer-ui/src/editor/properties-panel/image-generation-node-properties-panel/generation-panel.tsx
echo "---Full File---"
cat internal-packages/workflow-designer-ui/src/editor/properties-panel/image-generation-node-properties-panel/generation-panel.tsx
echo "---"
# Search for _handleGenerate usage
rg '_handleGenerate' internal-packages/workflow-designer-ui/src/editor/properties-panel/image-generation-node-properties-panel/generation-panel.tsx -n
echo "---"
# Check GenerationView component definition
fd GenerationView.tsx -g '*.tsx' | head -5Length of output: 3004
Remove unused onClickGenerateButton callback or add CTA button in Empty state
The onClickGenerateButton parameter is still being passed by all call sites (generateImage, generateText, generate), but the _handleGenerate callback defined at line 43 is never invoked. The Empty() component (lines 13–22) has no CTA button, and neither does the rendered GenerationView. Either:
- Remove
onClickGenerateButtonfrom the function signature and all call sites, or - Add a button in the
Emptycomponent that calls_handleGenerate()to trigger generation
🤖 Prompt for AI Agents
internal-packages/workflow-designer-ui/src/editor/properties-panel/image-generation-node-properties-panel/generation-panel.tsx
lines 26-36: the onClickGenerateButton prop is passed from callers but never
used because the Empty view has no CTA; add a visible CTA button in the Empty()
component that calls the existing _handleGenerate (or invokes
onClickGenerateButton if you prefer using props) so generation actually
triggers, and ensure the button is accessible (label/aria) and styled
consistently with other panel buttons; alternatively, if you decide not to
expose a CTA, remove onClickGenerateButton from this component and all call
sites to avoid dead code.
| <div | ||
| role="dialog" | ||
| aria-label="Image viewer" | ||
| className="fixed inset-0 bg-background/95 z-[9999] flex items-center justify-center cursor-pointer" | ||
| onClick={onClose} | ||
| onKeyDown={(e) => { | ||
| if (e.key === "Escape" || e.key === "Enter" || e.key === " ") { | ||
| onClose(); | ||
| } | ||
| }} | ||
| > |
There was a problem hiding this comment.
Modal a11y: add focus management and aria-modal
onKeyDown on a non-focusable div won’t fire reliably; also missing aria-modal. Focus the overlay and listen on window for Escape.
-import { createPortal } from "react-dom";
+import { createPortal } from "react-dom";
+import { useEffect, useRef } from "react";
...
export function Lightbox({ src, onClose }: { src: string; onClose: () => void; }) {
if (typeof document === "undefined") return null;
+ const overlayRef = useRef<HTMLDivElement>(null);
+ useEffect(() => {
+ overlayRef.current?.focus();
+ const onKey = (e: KeyboardEvent) => {
+ if (e.key === "Escape") onClose();
+ };
+ window.addEventListener("keydown", onKey);
+ return () => window.removeEventListener("keydown", onKey);
+ }, [onClose]);
return createPortal(
- <div
+ <div
role="dialog"
+ aria-modal="true"
+ tabIndex={-1}
+ ref={overlayRef}
aria-label="Image viewer"
className="fixed inset-0 bg-background/95 z-[9999] flex items-center justify-center cursor-pointer"
onClick={onClose}
- onKeyDown={(e) => {
- if (e.key === "Escape" || e.key === "Enter" || e.key === " ") {
- onClose();
- }
- }}
>Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In internal-packages/workflow-designer-ui/src/ui/lightbox.tsx around lines 16 to
26, the overlay div is not focusable and lacks aria-modal, and its onKeyDown
won't fire reliably; make the overlay focusable (add tabIndex={-1} or 0), set
aria-modal="true", move keyboard Escape handling to a window keydown listener
(mounted on open, removed on cleanup) to reliably catch Escape, keep or add a
key handler on the overlay for Enter/Space if you want activation via keyboard,
and implement focus management by saving document.activeElement before opening,
programmatically focusing the overlay when opened, and restoring focus when
closed.
- Remove console.log debugging statements - Refactor to use useOverlayBottom and useElementTopPx hooks - Remove unused variables: _uiState, _models, _disconnectInvalidConnections - Remove unused imports: ImageGenerationLanguageModelData, isSupportedConnection, language models, useMemo, useModelEligibility, deleteConnection - Fix TypeScript types in hooks to accept null refs 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal-packages/workflow-designer-ui/src/editor/properties-panel/image-generation-node-properties-panel/index.tsx (1)
162-178: Missing requirednodesprop in expanded PromptEditor.The expanded
PromptEditoris missing thenodesprop that is provided in the regularPromptPanel(seeprompt-panel.tsxlines 30-188 in relevant snippets). This prop is required for proper text editor functionality with connected sources and could cause runtime errors or missing editor features.Apply this diff to add the missing prop:
<div className="flex-1 min-h-0 flex flex-col overflow-hidden rounded-[8px] bg-background"> <PromptEditor key={`expanded-${editorVersion}-${node.id}`} value={livePrompt ?? node.content.prompt} onValueChange={(value) => { updateNodeDataContent(node, { prompt: value }); }} + nodes={connectedSources + .map((source) => source.node) + .filter((n): n is NonNullable<typeof n> => n !== null)} connectedSources={connectedSources.map(({ node, output }) => ({ node, output, }))}Alternatively, extract the
nodescomputation fromPromptPaneland reuse it:const nodes = useMemo( () => connectedSources .map((source) => Node.safeParse(source.node)) .map((parse) => (parse.success ? parse.data : null)) .filter((data) => data !== null), [connectedSources], );Then pass it to both the regular and expanded editors.
🧹 Nitpick comments (3)
internal-packages/workflow-designer-ui/src/editor/properties-panel/ui/generate-cta-button.tsx (1)
1-63: LGTM! Clean implementation with proper state handling and styling.The component is well-structured with appropriate TypeScript typing, follows coding guidelines (PascalCase component name, camelCase for props/variables), and implements correct logic for the disabled state and label switching. The inline animations and conditional shortcut hint are well executed.
Note: The past review comment flagging a debug
console.logstatement at lines 25-26 has been resolved—the current code correctly wiresonClickanddisabledprops without any debug artifacts.Optional enhancement: The keyboard shortcut hint currently displays
⌘ + Enter(Mac-specific). Consider detecting the user's OS and showingCtrl + Enteron Windows/Linux for a more universal experience, though this is not essential for the current implementation.internal-packages/workflow-designer-ui/src/editor/properties-panel/image-generation-node-properties-panel/index.tsx (2)
50-50: Misleading variable naming convention.The
_overlayBottomPxvariable uses an underscore prefix, which typically indicates an unused or private variable. However, this variable is actively used in lines 156 and 216 for overlay positioning. Remove the underscore prefix to follow proper naming conventions.Apply this diff:
- const _overlayBottomPx = useOverlayBottom(generateCtaRef); + const overlayBottomPx = useOverlayBottom(generateCtaRef);And update the usage sites:
style={{ top: 0, - bottom: _overlayBottomPx, + bottom: overlayBottomPx, paddingBottom: 12,
144-242: Consider extracting common overlay pattern.The expanded prompt overlay (lines 144-202) and expanded generation overlay (lines 204-242) share nearly identical structure: dialog wrapper, content area, minimize button, and backdrop. Consider extracting a reusable
ExpandableOverlaycomponent to reduce duplication and improve maintainability.Example structure:
<ExpandableOverlay isExpanded={isPromptExpanded} onClose={() => { setIsPromptExpanded(false); setEditorVersion((v) => v + 1); }} transformOriginY={promptTopPx} bottomOffset={overlayBottomPx} ariaLabel="Expanded prompt editor" > {/* Content */} </ExpandableOverlay>This would centralize the transition logic, ARIA attributes, and minimize/backdrop button patterns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal-packages/workflow-designer-ui/src/editor/properties-panel/image-generation-node-properties-panel/index.tsx(4 hunks)internal-packages/workflow-designer-ui/src/editor/properties-panel/ui/generate-cta-button.tsx(1 hunks)internal-packages/workflow-designer-ui/src/ui/hooks.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal-packages/workflow-designer-ui/src/ui/hooks.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development-guide.mdc)
**/*.{ts,tsx}: Use Biome for formatting with tab indentation and double quotes
Follow organized imports pattern (enabled in biome.json)
Use TypeScript for type safety; avoidanytypes
Use Next.js patterns for web applications
Use async/await for asynchronous code rather than promises
Error handling: use try/catch blocks and propagate errors appropriately
Use kebab-case for all filenames (e.g.,user-profile.ts)
Use camelCase for variables, functions, and methods (e.g.,userEmail)
Use prefixes likeis,has,can,shouldfor boolean variables and functions for clarity
Use verbs or verb phrases that clearly indicate purpose for function naming (e.g.,calculateTotalPrice(), notprocess())If breaking changes are introduced in new AI SDK versions, update code to accommodate those changes
**/*.{ts,tsx}: Avoid using theanytype in TypeScript
Use async/await for asynchronous code and include proper error handling
Variables and functions should be camelCase
Boolean variables and functions should use is/has/can/should prefixes where appropriate
Function names should clearly indicate their purpose
Files:
internal-packages/workflow-designer-ui/src/editor/properties-panel/image-generation-node-properties-panel/index.tsxinternal-packages/workflow-designer-ui/src/editor/properties-panel/ui/generate-cta-button.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/development-guide.mdc)
**/*.tsx: Use functional components with React hooks
Use PascalCase for React components and classes (e.g.,UserProfile)
**/*.tsx: React components should use React hooks and Next.js patterns
Component identifiers (names) should be PascalCase
Files:
internal-packages/workflow-designer-ui/src/editor/properties-panel/image-generation-node-properties-panel/index.tsxinternal-packages/workflow-designer-ui/src/editor/properties-panel/ui/generate-cta-button.tsx
**/*
📄 CodeRabbit inference engine (.cursor/rules/naming-guide.mdc)
All filenames should use kebab-case (lowercase with hyphens)
Files:
internal-packages/workflow-designer-ui/src/editor/properties-panel/image-generation-node-properties-panel/index.tsxinternal-packages/workflow-designer-ui/src/editor/properties-panel/ui/generate-cta-button.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/naming-guide.mdc)
**/*.{js,jsx,ts,tsx}: React components and classes should use PascalCase
Variables, functions, and methods should use camelCase
Use verbs or verb phrases for function names; names should clearly indicate what the function does; avoid ambiguous names that could lead to misuse
Use nouns or noun phrases for variable names; names should describe what the variable represents; avoid single-letter variables except in very short scopes
Use prefixes like 'is', 'has', 'can', 'should' for both variables and functions returning boolean values; make the true/false meaning clear
Files:
internal-packages/workflow-designer-ui/src/editor/properties-panel/image-generation-node-properties-panel/index.tsxinternal-packages/workflow-designer-ui/src/editor/properties-panel/ui/generate-cta-button.tsx
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
PR: giselles-ai/giselle#0
File: .cursor/rules/edit-workspace-tour.mdc:0-0
Timestamp: 2025-07-21T22:29:07.662Z
Learning: Applies to internal-packages/workflow-designer-ui/src/editor/workspace-tour/workspace-tour.tsx : Update the `imageSrc` prop in the appropriate step component in `workspace-tour.tsx` to use a new image
Learnt from: CR
PR: giselles-ai/giselle#0
File: .cursor/rules/edit-workspace-tour.mdc:0-0
Timestamp: 2025-07-21T22:29:07.662Z
Learning: Applies to internal-packages/workflow-designer-ui/src/editor/workspace-tour/workspace-tour.tsx : Update imports and references to images in `workspace-tour.tsx` when changing tour visuals
Learnt from: CR
PR: giselles-ai/giselle#0
File: .cursor/rules/edit-workspace-tour.mdc:0-0
Timestamp: 2025-07-21T22:29:07.662Z
Learning: Applies to internal-packages/workflow-designer-ui/src/editor/workspace-tour/workspace-tour.tsx : Import new images at the top of `workspace-tour.tsx` when adding or changing step visuals
Learnt from: CR
PR: giselles-ai/giselle#0
File: .cursor/rules/edit-workspace-tour.mdc:0-0
Timestamp: 2025-07-21T22:29:07.662Z
Learning: Applies to internal-packages/workflow-designer-ui/src/editor/workspace-tour/assets/* : Replace GIFs and images in the `assets/` directory to change tour visuals
📚 Learning: 2025-07-21T22:29:07.662Z
Learnt from: CR
PR: giselles-ai/giselle#0
File: .cursor/rules/edit-workspace-tour.mdc:0-0
Timestamp: 2025-07-21T22:29:07.662Z
Learning: Applies to internal-packages/workflow-designer-ui/src/editor/workspace-tour/workspace-tour.tsx : Update the `imageSrc` prop in the appropriate step component in `workspace-tour.tsx` to use a new image
Applied to files:
internal-packages/workflow-designer-ui/src/editor/properties-panel/image-generation-node-properties-panel/index.tsx
📚 Learning: 2025-10-24T01:27:17.214Z
Learnt from: CR
PR: giselles-ai/giselle#0
File: internal-packages/workflow-designer-ui/src/editor/properties-panel/trigger-node-properties-panel/providers/github-trigger/AGENTS.md:0-0
Timestamp: 2025-10-24T01:27:17.214Z
Learning: Applies to internal-packages/workflow-designer-ui/src/editor/properties-panel/trigger-node-properties-panel/providers/github-trigger/github-trigger-properties-panel.tsx : Wrap state updates that transition between reconfiguring/configured in startTransition for consistent UI updates
Applied to files:
internal-packages/workflow-designer-ui/src/editor/properties-panel/image-generation-node-properties-panel/index.tsx
📚 Learning: 2025-07-21T22:29:07.662Z
Learnt from: CR
PR: giselles-ai/giselle#0
File: .cursor/rules/edit-workspace-tour.mdc:0-0
Timestamp: 2025-07-21T22:29:07.662Z
Learning: Applies to internal-packages/workflow-designer-ui/src/editor/workspace-tour/workspace-tour.tsx : Update imports and references to images in `workspace-tour.tsx` when changing tour visuals
Applied to files:
internal-packages/workflow-designer-ui/src/editor/properties-panel/image-generation-node-properties-panel/index.tsx
📚 Learning: 2025-09-02T05:50:06.317Z
Learnt from: CR
PR: giselles-ai/giselle#0
File: internal-packages/workflow-designer-ui/src/new-editor/AGENTS.md:0-0
Timestamp: 2025-09-02T05:50:06.317Z
Learning: Applies to internal-packages/workflow-designer-ui/src/new-editor/**/*.tsx : Avoid passing large props through multiple levels; prefer local selection from the store at leaf components.
Applied to files:
internal-packages/workflow-designer-ui/src/editor/properties-panel/image-generation-node-properties-panel/index.tsx
📚 Learning: 2025-09-02T05:50:06.317Z
Learnt from: CR
PR: giselles-ai/giselle#0
File: internal-packages/workflow-designer-ui/src/new-editor/AGENTS.md:0-0
Timestamp: 2025-09-02T05:50:06.317Z
Learning: Applies to internal-packages/workflow-designer-ui/src/new-editor/components/node/node.tsx : In Node component, select exactly the node data, derived connection ids, and UI flags needed; provide a custom equality function combining strict reference checks with shallow for arrays.
Applied to files:
internal-packages/workflow-designer-ui/src/editor/properties-panel/image-generation-node-properties-panel/index.tsx
📚 Learning: 2025-10-24T01:27:17.214Z
Learnt from: CR
PR: giselles-ai/giselle#0
File: internal-packages/workflow-designer-ui/src/editor/properties-panel/trigger-node-properties-panel/providers/github-trigger/AGENTS.md:0-0
Timestamp: 2025-10-24T01:27:17.214Z
Learning: Applies to internal-packages/workflow-designer-ui/src/editor/properties-panel/trigger-node-properties-panel/providers/github-trigger/github-trigger-properties-panel.tsx : Model node state as configured, reconfiguring, and implicit unconfigured; render GitHubTriggerConfiguredView for configured, GitHubTriggerReconfiguringView for reconfiguring, and integration-branching UI for unconfigured
Applied to files:
internal-packages/workflow-designer-ui/src/editor/properties-panel/image-generation-node-properties-panel/index.tsx
📚 Learning: 2025-09-02T05:50:06.317Z
Learnt from: CR
PR: giselles-ai/giselle#0
File: internal-packages/workflow-designer-ui/src/new-editor/AGENTS.md:0-0
Timestamp: 2025-09-02T05:50:06.317Z
Learning: Applies to internal-packages/workflow-designer-ui/src/new-editor/**/*.ts : Lift actions into the store (e.g., expose updateNode) and invoke them from components performing mutations.
Applied to files:
internal-packages/workflow-designer-ui/src/editor/properties-panel/image-generation-node-properties-panel/index.tsx
🧬 Code graph analysis (1)
internal-packages/workflow-designer-ui/src/editor/properties-panel/image-generation-node-properties-panel/index.tsx (8)
packages/giselle/src/react/flow/hooks/use-workflow-designer.ts (1)
useWorkflowDesigner(5-13)internal-packages/workflow-designer-ui/src/ui/hooks.ts (3)
useOverlayBottom(13-34)useElementTopPx(36-64)useLivePrompt(6-11)internal-packages/workflow-designer-ui/src/editor/properties-panel/ui/node-panel-header.tsx (1)
NodePanelHeader(8-73)internal-packages/workflow-designer-ui/src/editor/properties-panel/image-generation-node-properties-panel/prompt-panel.tsx (1)
PromptPanel(31-189)internal-packages/workflow-designer-ui/src/editor/properties-panel/image-generation-node-properties-panel/generation-panel.tsx (1)
GenerationPanel(26-93)internal-packages/workflow-designer-ui/src/editor/properties-panel/ui/generate-cta-button.tsx (1)
GenerateCtaButton(5-63)internal-packages/workflow-designer-ui/src/editor/lib/validate-prompt.ts (1)
isPromptEmpty(6-12)internal-packages/ui/components/prompt-editor.tsx (1)
PromptEditor(28-97)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: codeql / languages (javascript) / Perform CodeQL for javascript
- GitHub Check: Cursor Bugbot
🔇 Additional comments (3)
internal-packages/workflow-designer-ui/src/editor/properties-panel/image-generation-node-properties-panel/index.tsx (3)
1-26: LGTM: Clean import organization.The imports are well-organized and follow the project's conventions. All imported modules are used appropriately throughout the component.
63-94: LGTM: Well-structured generation callback.The
generateImagefunction properly validates usage limits and prompt content before triggering generation. Error handling via toasts and the dependencies array are correctly configured.
99-142: LGTM: Clean panel structure with proper state management.The header integration with
NodePanelHeader, the main content area layout, and the generate button are well-structured. The overlay expansion callbacks properly manage state transitions.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
/pr-enrichment |
|
Finished running flow.
|
giselles-ai
bot
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Overview
This PR redesigns the image generation node interface to improve user experience and code maintainability. The changes unify node panel headers across all node types, streamline the image generation workflow by removing unnecessary complexity, and modernize image handling with Next.js Image components. These improvements create a more consistent and intuitive interface while reducing technical debt.
Changes
UI/UX Improvements
NodePanelHeadercomponent with consistent documentation links and delete actions across all node types (text, image, query, trigger, file, vector-store)Technical Enhancements
<img>tags with Next/Image components via newImageCardandLightboxcomponents for optimized loading and viewingGenerateCtaButtonfor consistent primary actions and new UI hooks (useOverlayBottom,useElementTopPx,useLivePrompt)ProTagand contextual parameter display for different providers (Fal/OpenAI)Code Cleanup
pluralizeand@types/pluralizepackagesModelPickerto usearia-disabledfor better semantic HTMLImage.pathnameno longer includes leading slash - consumers must adjust URL joining logicworkflow-designer-uino longer exportstypes/use-source-categories/utilsand Input panel componentsdisabledattribute toaria-disabled, removed disabled reason tooltipremotePatternsconfiguration innext.config.jsTesting
Review Notes
remotePatternsare configured for external image sources (GitHub avatars, etc.)Related Issues
Note
Redesigns the image generation node with a unified panel header across nodes, moves primary actions into full-width CTAs, and migrates image rendering to Next/Image with lightbox and download support.
NodePanelHeaderand applies it to action, file, image, query, text, trigger, vector-store, and web-page panels.GenerateCtaButton.<img>with Next/Image across designer UI (newImageCard,Lightbox) and settings/tour UIs; adds optimized thumbnails.${client.basePath}/${content.pathname}to${client.basePath}${content.pathname}.GenerationViewloading to a single spinner; introducesTHUMB_HEIGHTand thumbnail layout.ModelPickerwith plan gating (ProTag); adds provider-specific parameter panels (FalModelPanel,OpenAIImageModelPanel).aria-disabledfor disabled models.useOverlayBottom,useElementTopPx,useLivePrompt; refinesEditableTextAPI; updates panel internals/exports.use-model-eligibility.pluralizeand@types/pluralize; updates licenses doc accordingly.Written by Cursor Bugbot for commit 2ad3c15. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
UI/UX Improvements
Bug Fixes
Chores