feat: enhance workflow loading logic and improve node handling in CreateWorkFlow component#43
Conversation
…ateWorkFlow component
📝 WalkthroughWalkthroughCreateWorkFlow now initializes diagram nodes/edges from full trigger and node objects via a new useEffect/loadWorkflow, standardizes node metadata and positions, adds lightweight node click logging, and expands GoogleSheetFormClient to accept an optional initialData prop for pre-populating credential/document/sheet state and auto-fetching related lists. Changes
Sequence Diagram(s)(Skipped — changes are component initialization and form pre-population; they do not introduce a new multi-component sequential flow that meets the diagram criteria.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)apps/web/app/components/nodes/CreateWorkFlow.tsx (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 |
![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
🧹 Nitpick comments (1)
apps/web/app/components/nodes/CreateWorkFlow.tsx (1)
295-295: Remove debug console.log statements before merging.This and other debug logs (lines 307, 308, 312) should be removed or wrapped in a development-only check before merging to main.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/app/components/nodes/CreateWorkFlow.tsx
🔇 Additional comments (1)
apps/web/app/components/nodes/CreateWorkFlow.tsx (1)
47-49: LGTM on trigger data access refactor.Accessing the full trigger object instead of just the name is appropriate given the new workflow loading logic that needs
dbId,name, andconfigproperties.
| setNodeIDType(node.id) | ||
| setCredType(node.data.type === "google_sheet" ? "google_oauth" : "") | ||
| setCredType(node.data.type.includes("google sheet") || node.data.type.includes("google_sheet") ? "google_oauth" : "") | ||
| setLoadSheet(!loadSheet) |
There was a problem hiding this comment.
Toggle behavior may cause unexpected UX when switching nodes.
setLoadSheet(!loadSheet) toggles the form. If a user clicks Google Sheet node A (form opens), then clicks Google Sheet node B, the form closes instead of staying open for node B.
Consider using setLoadSheet(true) to always open, with a separate close mechanism.
🔎 Proposed fix
- setLoadSheet(!loadSheet)
+ setLoadSheet(true)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/web/app/components/nodes/CreateWorkFlow.tsx around line 311, the current
toggle call setLoadSheet(!loadSheet) causes the sheet form to close when
switching between Google Sheet nodes; change this to setLoadSheet(true) so
selecting any Google Sheet node always opens the form, and implement/ensure a
separate explicit close handler (e.g., onCancel/onClose that calls
setLoadSheet(false)) is used to close the form instead of relying on the toggle
behavior.
Vamsi-o
left a comment
Vamsi-o
left a comment
There was a problem hiding this comment.
useEffect(()=>{
- async function loadWorkflow(){
- function loadWorkflow(){
- if (!existingTrigger && existingNodes.length === 0) return;
- const START_X = 100;
const GAP = 250;
const Y = 200;
// ...
data:{
label: existingTrigger.name,
name: existingTrigger.name,
-
type: existingTrigger.name.split(" - ")[0],
-
// ...
type: existingTrigger.type, icon: '📊', config: existingTrigger.config, }
data:{
label: node.name,
name: node.name,
icon: '⚙️',
-
type: node.name.split(" - ")[0],
-
type: node.type, config: node.config, } Make sure fix this major bug
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/web/app/components/nodes/CreateWorkFlow.tsx (2)
305-314: Google Sheet detection improved but needs cleanup.The case-insensitive check on line 306 is better than the previous implementation. However:
- Since
nodeTypeis already lowercase, you can simplify line 307 to just checknodeType.includes('google') && nodeType.includes('sheet')or similar- Remove debug console.log statements on lines 308, 309, and 313
- The toggle behavior on line 312 (
setLoadSheet(!loadSheet)) still has the issue flagged in the previous review—it will close the form when switching between Google Sheet nodes instead of keeping it open🔎 Suggested improvements
if(node.type === 'action' || node.type === 'trigger'){ - // Check by type instead of name (more reliable) const nodeType = node.data.type?.toLowerCase() || ''; - if(nodeType.includes('google_sheet') || nodeType.includes('google sheet')){ - console.log("Google Sheet node clicked") - console.log("Node ID:", node.id) + const isGoogleSheet = nodeType.includes('google') && nodeType.includes('sheet'); + if(isGoogleSheet){ setNodeIDType(node.id) setCredType("google_oauth") - setLoadSheet(!loadSheet) - console.log("Form opened") + setLoadSheet(true) } }
152-214: Previous review feedback not addressed.The issues flagged in the previous review remain unresolved:
- The function is declared
asyncbut doesn't await anything- Lines 169 and 187 still derive type from
name.split(" - ")[0]instead of usingexistingTrigger.typeandnode.type- No guard to prevent overwriting the diagram when
existingTriggerandexistingNodesare emptyPlease address the feedback from the previous review.
🧹 Nitpick comments (2)
apps/web/app/components/nodes/CreateWorkFlow.tsx (2)
47-49: Approve accessing full trigger object; remove commented debug code.Accessing the full trigger object (line 47) is correct since
loadWorkflowneeds properties likename,type, andconfig. However, the commented-out console.log on line 49 should be removed entirely rather than left as clutter.🔎 Remove commented debug code
- // console.log(`workflow from redux, TRigger: ${existingTrigger}, Nodes: ${existingNodes}`)
295-295: Remove debug logging.The console.log on line 295 appears to be debug code. Remove it or wrap it in a debug flag before production.
🔎 Remove debug logging
- console.log("Node clicked:", node.id, node.type, node.data);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/app/components/nodes/CreateWorkFlow.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/app/components/nodes/CreateWorkFlow.tsx (2)
apps/web/app/hooks/redux.ts (1)
useAppSelector(7-7)apps/web/app/types/workflow.types.ts (1)
NodeType(1-11)
🔇 Additional comments (1)
apps/web/app/components/nodes/CreateWorkFlow.tsx (1)
13-13: LGTM! Unused import removed.Removing the unused
workflowReducerimport keeps the imports clean.
…in CreateWorkFlow component
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/web/app/components/nodes/CreateWorkFlow.tsx (1)
310-318: Previous review feedback not addressed: Toggle causes form to close when switching nodes.The type detection improvements look good (case-insensitive, checks both formats). However, line 317 still uses
setLoadSheet(!loadSheet), which was flagged in a previous review. If a user clicks Google Sheet node A (form opens), then clicks Google Sheet node B, the form closes instead of staying open for node B, creating poor UX.🔎 Proposed fix to always open the form
setNodeIDType(node.id) setCredType("google_oauth") - setLoadSheet(!loadSheet) + setLoadSheet(true) console.log("Form opened")
🧹 Nitpick comments (2)
apps/web/app/components/nodes/CreateWorkFlow.tsx (2)
49-49: Consider removing commented-out code.Commented debugging code can accumulate over time. Consider removing it entirely and relying on version control if you need to reference it later.
300-300: Consider removing or conditionalizing debug logs.Debug
console.logstatements can clutter production logs. Consider removing this or wrapping it in a development-only check.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/app/components/nodes/CreateWorkFlow.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/app/components/nodes/CreateWorkFlow.tsx (2)
apps/web/app/hooks/redux.ts (1)
useAppSelector(7-7)apps/web/app/types/workflow.types.ts (1)
NodeType(1-11)
🔇 Additional comments (2)
apps/web/app/components/nodes/CreateWorkFlow.tsx (2)
13-13: LGTM! Clean import removal.Removing the unused
workflowReducerimport keeps the codebase tidy.
47-47: LGTM! Reading the full trigger object.Accessing the complete trigger object provides the necessary data for workflow reconstruction.
| useEffect(()=>{ | ||
| // Guard: only rebuild nodes/edges if there's actual stored data | ||
| if (existingNodes.length === 0 && !existingTrigger) { | ||
| return; // Keep the current placeholder state | ||
| } | ||
|
|
||
| function loadWorkflow(){ | ||
| const START_X = 100; | ||
| const GAP = 250; | ||
| const Y = 200; | ||
|
|
||
| const newNodes: NodeType[] = []; | ||
| const newEdges: EdgeType[] = []; | ||
| const type = existingTrigger?.name.split(" - ")[0] | ||
| if(existingTrigger){ | ||
| newNodes.push({ | ||
| id: `trigger-${existingTrigger.dbId}`, | ||
| type: 'trigger' as const, | ||
| position: { x: START_X, y: Y}, | ||
| data:{ | ||
| label: existingTrigger.name, | ||
| name: existingTrigger.name, | ||
| type: type, | ||
| icon: '📊', | ||
| config: existingTrigger.config, | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| existingNodes.forEach((node, index)=>{ | ||
| const nodeId = `action-${node.dbId}-${index}`; | ||
| const type = node.name.split(" - ")[0] | ||
| newNodes.push({ | ||
| id: nodeId, | ||
| type: 'action' as const, | ||
| position: { x: START_X + (index + 1) * GAP, y: Y}, | ||
| data:{ | ||
| label: node.name, | ||
| name: node.name, | ||
| icon: '⚙️', | ||
| type: type, | ||
| config: node.config, | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| const placeholderIndex = newNodes.length; | ||
| newNodes.push({ | ||
| id: `placeholder-${Date.now()}`, | ||
| type: 'placeholder' as const, | ||
| position: { x: START_X + placeholderIndex * GAP, y: Y}, | ||
| data: { label: '+' } | ||
| }); | ||
|
|
||
| for(let i=0; i<newNodes.length - 1; i++){ | ||
| newEdges.push({ | ||
| id: `edge-${i}`, | ||
| source: newNodes[i]?.id || '', | ||
| target: newNodes[i+1]?.id || '' | ||
| }); | ||
| } | ||
|
|
||
| setNodes(newNodes); | ||
| setEdges(newEdges) | ||
| } | ||
|
|
||
| loadWorkflow() | ||
| },[existingNodes, existingTrigger]) |
There was a problem hiding this comment.
Previous review feedback not addressed: Stop deriving type from name.
The previous review (lines 152-219) specifically requested using existingTrigger.type and node.type properties from Redux instead of extracting from name.split(" - ")[0]. Lines 165, 174, 183, and 192 still use the fragile name-splitting approach, which will break if the naming convention changes.
Additionally, Line 165 executes outside the if(existingTrigger) check on line 166. Given the guard on line 154 only returns when BOTH are empty, if existingNodes has items but existingTrigger is null, line 165 will set type to undefined, which then gets assigned on line 174.
🔎 Proposed fix using Redux type properties
function loadWorkflow(){
const START_X = 100;
const GAP = 250;
const Y = 200;
const newNodes: NodeType[] = [];
const newEdges: EdgeType[] = [];
- const type = existingTrigger?.name.split(" - ")[0]
if(existingTrigger){
newNodes.push({
id: `trigger-${existingTrigger.dbId}`,
type: 'trigger' as const,
position: { x: START_X, y: Y},
data:{
label: existingTrigger.name,
name: existingTrigger.name,
- type: type,
+ type: existingTrigger.type,
icon: '📊',
config: existingTrigger.config,
}
})
}
existingNodes.forEach((node, index)=>{
const nodeId = `action-${node.dbId}-${index}`;
- const type = node.name.split(" - ")[0]
newNodes.push({
id: nodeId,
type: 'action' as const,
position: { x: START_X + (index + 1) * GAP, y: Y},
data:{
label: node.name,
name: node.name,
icon: '⚙️',
- type: type,
+ type: node.type,
config: node.config,
}
});
});📝 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.
| useEffect(()=>{ | |
| // Guard: only rebuild nodes/edges if there's actual stored data | |
| if (existingNodes.length === 0 && !existingTrigger) { | |
| return; // Keep the current placeholder state | |
| } | |
| function loadWorkflow(){ | |
| const START_X = 100; | |
| const GAP = 250; | |
| const Y = 200; | |
| const newNodes: NodeType[] = []; | |
| const newEdges: EdgeType[] = []; | |
| const type = existingTrigger?.name.split(" - ")[0] | |
| if(existingTrigger){ | |
| newNodes.push({ | |
| id: `trigger-${existingTrigger.dbId}`, | |
| type: 'trigger' as const, | |
| position: { x: START_X, y: Y}, | |
| data:{ | |
| label: existingTrigger.name, | |
| name: existingTrigger.name, | |
| type: type, | |
| icon: '📊', | |
| config: existingTrigger.config, | |
| } | |
| }) | |
| } | |
| existingNodes.forEach((node, index)=>{ | |
| const nodeId = `action-${node.dbId}-${index}`; | |
| const type = node.name.split(" - ")[0] | |
| newNodes.push({ | |
| id: nodeId, | |
| type: 'action' as const, | |
| position: { x: START_X + (index + 1) * GAP, y: Y}, | |
| data:{ | |
| label: node.name, | |
| name: node.name, | |
| icon: '⚙️', | |
| type: type, | |
| config: node.config, | |
| } | |
| }); | |
| }); | |
| const placeholderIndex = newNodes.length; | |
| newNodes.push({ | |
| id: `placeholder-${Date.now()}`, | |
| type: 'placeholder' as const, | |
| position: { x: START_X + placeholderIndex * GAP, y: Y}, | |
| data: { label: '+' } | |
| }); | |
| for(let i=0; i<newNodes.length - 1; i++){ | |
| newEdges.push({ | |
| id: `edge-${i}`, | |
| source: newNodes[i]?.id || '', | |
| target: newNodes[i+1]?.id || '' | |
| }); | |
| } | |
| setNodes(newNodes); | |
| setEdges(newEdges) | |
| } | |
| loadWorkflow() | |
| },[existingNodes, existingTrigger]) | |
| useEffect(()=>{ | |
| // Guard: only rebuild nodes/edges if there's actual stored data | |
| if (existingNodes.length === 0 && !existingTrigger) { | |
| return; // Keep the current placeholder state | |
| } | |
| function loadWorkflow(){ | |
| const START_X = 100; | |
| const GAP = 250; | |
| const Y = 200; | |
| const newNodes: NodeType[] = []; | |
| const newEdges: EdgeType[] = []; | |
| if(existingTrigger){ | |
| newNodes.push({ | |
| id: `trigger-${existingTrigger.dbId}`, | |
| type: 'trigger' as const, | |
| position: { x: START_X, y: Y}, | |
| data:{ | |
| label: existingTrigger.name, | |
| name: existingTrigger.name, | |
| type: existingTrigger.type, | |
| icon: '📊', | |
| config: existingTrigger.config, | |
| } | |
| }) | |
| } | |
| existingNodes.forEach((node, index)=>{ | |
| const nodeId = `action-${node.dbId}-${index}`; | |
| newNodes.push({ | |
| id: nodeId, | |
| type: 'action' as const, | |
| position: { x: START_X + (index + 1) * GAP, y: Y}, | |
| data:{ | |
| label: node.name, | |
| name: node.name, | |
| icon: '⚙️', | |
| type: node.type, | |
| config: node.config, | |
| } | |
| }); | |
| }); | |
| const placeholderIndex = newNodes.length; | |
| newNodes.push({ | |
| id: `placeholder-${Date.now()}`, | |
| type: 'placeholder' as const, | |
| position: { x: START_X + placeholderIndex * GAP, y: Y}, | |
| data: { label: '+' } | |
| }); | |
| for(let i=0; i<newNodes.length - 1; i++){ | |
| newEdges.push({ | |
| id: `edge-${i}`, | |
| source: newNodes[i]?.id || '', | |
| target: newNodes[i+1]?.id || '' | |
| }); | |
| } | |
| setNodes(newNodes); | |
| setEdges(newEdges) | |
| } | |
| loadWorkflow() | |
| },[existingNodes, existingTrigger]) |
🤖 Prompt for AI Agents
In apps/web/app/components/nodes/CreateWorkFlow.tsx around lines 152 to 219, the
code still derives node types by splitting the name (lines ~165,174,183,192) and
declares the trigger type outside the existingTrigger guard which can produce
undefined; replace uses of name.split(" - ")[0] with the authoritative
existingTrigger.type and node.type values from Redux, move the trigger type
declaration inside the if(existingTrigger) block so it’s only read when
existingTrigger exists, and ensure each newNodes.data.type is assigned from
those Redux properties (existingTrigger.type for the trigger node and node.type
for action nodes) instead of parsing the name.
…ormClient component
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
apps/web/app/components/nodes/CreateWorkFlow.tsx (3)
166-175: Previous review feedback not addressed: Use type property instead of parsing name.Line 166 derives
typefromexistingTrigger?.name.split(" - ")[0]rather than usingexistingTrigger.typedirectly from Redux. This fragile approach will break if the naming convention changes. The same issue applies to line 184 for action nodes.🔎 Proposed fix using Redux type properties
const newNodes: NodeType[] = []; const newEdges: EdgeType[] = []; - const type = existingTrigger?.name.split(" - ")[0] if(existingTrigger){ newNodes.push({ id: `trigger-${existingTrigger.dbId}`, type: 'trigger' as const, position: { x: START_X, y: Y}, data:{ label: existingTrigger.name, name: existingTrigger.name, - type: type, + type: existingTrigger.type, icon: '📊', config: existingTrigger.config, } }) }
182-197: Same issue: Derive type from node.type instead of parsing name.Line 184 uses
node.name.split(" - ")[0]instead ofnode.type.🔎 Proposed fix
existingNodes.forEach((node, index)=>{ const nodeId = `action-${node.dbId}-${index}`; - const type = node.name.split(" - ")[0] newNodes.push({ id: nodeId, type: 'action' as const, position: { x: START_X + (index + 1) * GAP, y: Y}, data:{ label: node.name, name: node.name, icon: '⚙️', - type: type, + type: node.type, config: node.config, } }); });
320-320: Toggle behavior may cause unexpected UX when switching nodes.
setLoadSheet(!loadSheet)toggles the form. If a user clicks Google Sheet node A (form opens), then clicks Google Sheet node B, the form closes instead of staying open for node B. Consider usingsetLoadSheet(true)to always open.🔎 Proposed fix
- setLoadSheet(!loadSheet) + setLoadSheet(true)
🧹 Nitpick comments (2)
apps/web/app/components/nodes/GoogleSheetFormClient.tsx (2)
40-44: Remove debug console.log statements before merging.These logging statements appear to be debugging artifacts and should be removed or replaced with proper logging infrastructure for production code.
🔎 Proposed fix
- console.log('initial data: ', initialData) - console.log('initial document: ', selectedDocument) - console.log('initial sheet: ', selectedSheet) - console.log('initial range: ', range) - console.log("initial operation: ",operation)
50-54: Consider defensive handling for nodeType parsing.If
nodeTypedoesn't contain~,split("~")[1]returnsundefined, which then becomes an empty string. This is handled, but the parsing convention couples this component tightly to the ID format used inCreateWorkFlow. Consider documenting this contract or passingnodeIdas a separate prop for clarity.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/app/components/nodes/CreateWorkFlow.tsxapps/web/app/components/nodes/GoogleSheetFormClient.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
apps/web/app/components/nodes/GoogleSheetFormClient.tsx (3)
apps/web/app/hooks/redux.ts (1)
useAppSelector(7-7)apps/web/app/hooks/useCredential.ts (1)
useCredentials(5-33)packages/common/src/index.ts (1)
BACKEND_URL(4-4)
apps/web/app/components/nodes/CreateWorkFlow.tsx (3)
apps/web/app/hooks/redux.ts (1)
useAppSelector(7-7)apps/web/app/types/workflow.types.ts (1)
NodeType(1-11)apps/web/app/components/nodes/GoogleSheetFormClient.tsx (1)
GoogleSheetFormClient(27-368)
🔇 Additional comments (2)
apps/web/app/components/nodes/CreateWorkFlow.tsx (2)
310-322: LGTM on the Google Sheet type detection fix.The type detection logic now uses case-insensitive comparison and correctly sets
credTypeto"google_oauth". This addresses the previous review feedback about inconsistent type detection.
153-220: Good addition of workflow loading logic with proper guard.The
useEffectcorrectly guards against rebuilding nodes/edges when there's no stored data (lines 155-157), and theloadWorkflowfunction properly constructs the node graph with positions, edges, and a trailing placeholder. The dependency array correctly includesexistingNodesandexistingTrigger.
| initialData={selectedNodeConfig ? { | ||
| range: selectedNodeConfig.range as string, | ||
| operation: selectedNodeConfig.operation as string, | ||
| sheetName: selectedNodeConfig.sheetName as string, | ||
| spreadSheetId: selectedNodeConfig.spreadsheetId as string, | ||
| credentialId: selectedNodeConfig.credId as string, | ||
| } : undefined} |
There was a problem hiding this comment.
Critical: Property name mismatch will cause credentialId to not be pre-populated.
Line 348 reads selectedNodeConfig.credId, but GoogleSheetFormClient.handleSaveClick saves the config with credentialId (see line 194 in GoogleSheetFormClient.tsx). This mismatch means the credential will never be pre-populated when editing an existing node.
🔎 Proposed fix
initialData={selectedNodeConfig ? {
range: selectedNodeConfig.range as string,
operation: selectedNodeConfig.operation as string,
sheetName: selectedNodeConfig.sheetName as string,
spreadSheetId: selectedNodeConfig.spreadsheetId as string,
- credentialId: selectedNodeConfig.credId as string,
+ credentialId: selectedNodeConfig.credentialId as string,
} : undefined}🤖 Prompt for AI Agents
In apps/web/app/components/nodes/CreateWorkFlow.tsx around lines 343 to 349, the
initialData uses selectedNodeConfig.credId but the saved config property is
credentialId, so credentials never pre-populate; change the reference to
selectedNodeConfig.credentialId (and keep types consistent) so
initialData.spreadSheetId and initialData.credentialId match the keys used when
saving, ensuring the form pre-fills the credential field when editing.
| useEffect(() => { | ||
| const fetchInitialDocuments = async () => { | ||
| if (!initialData?.credentialId) return; | ||
|
|
||
| setLoading(true); | ||
| try { | ||
| const res = await fetch(`${BACKEND_URL}/node/getDocuments/${initialData.credentialId}`, { | ||
| method: 'GET', | ||
| credentials: "include", | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| }); | ||
| const data = await res.json(); | ||
| if (data.files?.length > 0) { | ||
| setDocuments(data.files); | ||
| } | ||
| } catch (error) { | ||
| console.error('Failed to fetch initial documents:', error); | ||
| } finally { | ||
| setLoading(false); | ||
| } | ||
| }; | ||
|
|
||
| fetchInitialDocuments(); | ||
| }, [initialData?.credentialId]); |
There was a problem hiding this comment.
Add HTTP response status check to prevent silent failures.
The fetch does not check res.ok before parsing JSON. If the server returns a 4xx/5xx error, res.json() may fail or return unexpected data, causing silent failures or misleading UI states.
🔎 Proposed fix
const res = await fetch(`${BACKEND_URL}/node/getDocuments/${initialData.credentialId}`, {
method: 'GET',
credentials: "include",
headers: { 'Content-Type': 'application/json' },
});
+ if (!res.ok) {
+ throw new Error(`Failed to fetch documents: ${res.status}`);
+ }
const data = await res.json();📝 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.
| useEffect(() => { | |
| const fetchInitialDocuments = async () => { | |
| if (!initialData?.credentialId) return; | |
| setLoading(true); | |
| try { | |
| const res = await fetch(`${BACKEND_URL}/node/getDocuments/${initialData.credentialId}`, { | |
| method: 'GET', | |
| credentials: "include", | |
| headers: { 'Content-Type': 'application/json' }, | |
| }); | |
| const data = await res.json(); | |
| if (data.files?.length > 0) { | |
| setDocuments(data.files); | |
| } | |
| } catch (error) { | |
| console.error('Failed to fetch initial documents:', error); | |
| } finally { | |
| setLoading(false); | |
| } | |
| }; | |
| fetchInitialDocuments(); | |
| }, [initialData?.credentialId]); | |
| useEffect(() => { | |
| const fetchInitialDocuments = async () => { | |
| if (!initialData?.credentialId) return; | |
| setLoading(true); | |
| try { | |
| const res = await fetch(`${BACKEND_URL}/node/getDocuments/${initialData.credentialId}`, { | |
| method: 'GET', | |
| credentials: "include", | |
| headers: { 'Content-Type': 'application/json' }, | |
| }); | |
| if (!res.ok) { | |
| throw new Error(`Failed to fetch documents: ${res.status}`); | |
| } | |
| const data = await res.json(); | |
| if (data.files?.length > 0) { | |
| setDocuments(data.files); | |
| } | |
| } catch (error) { | |
| console.error('Failed to fetch initial documents:', error); | |
| } finally { | |
| setLoading(false); | |
| } | |
| }; | |
| fetchInitialDocuments(); | |
| }, [initialData?.credentialId]); |
🤖 Prompt for AI Agents
In apps/web/app/components/nodes/GoogleSheetFormClient.tsx around lines 62 to
85, the fetch call parses res.json() without checking HTTP status which can
cause silent failures on 4xx/5xx responses; update the code to inspect res.ok
after the fetch, and if not ok read any error body (await res.text() or
res.json() guarded) and log or surface that error (and avoid calling
setDocuments), otherwise parse and use the JSON; ensure loading is cleared in
error and non-ok branches and return early on non-ok to prevent using invalid
data.
| useEffect(() => { | ||
| const fetchInitialSheets = async () => { | ||
| if (!initialData?.credentialId || !initialData?.spreadSheetId) return; | ||
|
|
||
| setLoading(true); | ||
| try { | ||
| const res = await fetch(`${BACKEND_URL}/node/getSheets/${initialData.credentialId}/${initialData.spreadSheetId}`, { | ||
| method: 'GET', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| credentials: "include", | ||
| }); | ||
| const data = await res.json(); | ||
| if (data.files?.data?.length > 0) { | ||
| setSheets(data.files.data); | ||
| } | ||
| } catch (error) { | ||
| console.error('Failed to fetch initial sheets:', error); | ||
| } finally { | ||
| setLoading(false); | ||
| } | ||
| }; | ||
|
|
||
| fetchInitialSheets(); | ||
| }, [initialData?.credentialId, initialData?.spreadSheetId]); |
There was a problem hiding this comment.
Add HTTP response status check (same issue as above).
Same concern applies here—missing res.ok check before parsing. Also note the response structure differs here (data.files?.data?.length) versus the documents fetch (data.files?.length), which could indicate inconsistent API responses or a bug in one of the checks.
🔎 Proposed fix
const res = await fetch(`${BACKEND_URL}/node/getSheets/${initialData.credentialId}/${initialData.spreadSheetId}`, {
method: 'GET',
headers: { 'Content-Type': 'application/json' },
credentials: "include",
});
+ if (!res.ok) {
+ throw new Error(`Failed to fetch sheets: ${res.status}`);
+ }
const data = await res.json();📝 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.
| useEffect(() => { | |
| const fetchInitialSheets = async () => { | |
| if (!initialData?.credentialId || !initialData?.spreadSheetId) return; | |
| setLoading(true); | |
| try { | |
| const res = await fetch(`${BACKEND_URL}/node/getSheets/${initialData.credentialId}/${initialData.spreadSheetId}`, { | |
| method: 'GET', | |
| headers: { 'Content-Type': 'application/json' }, | |
| credentials: "include", | |
| }); | |
| const data = await res.json(); | |
| if (data.files?.data?.length > 0) { | |
| setSheets(data.files.data); | |
| } | |
| } catch (error) { | |
| console.error('Failed to fetch initial sheets:', error); | |
| } finally { | |
| setLoading(false); | |
| } | |
| }; | |
| fetchInitialSheets(); | |
| }, [initialData?.credentialId, initialData?.spreadSheetId]); | |
| useEffect(() => { | |
| const fetchInitialSheets = async () => { | |
| if (!initialData?.credentialId || !initialData?.spreadSheetId) return; | |
| setLoading(true); | |
| try { | |
| const res = await fetch(`${BACKEND_URL}/node/getSheets/${initialData.credentialId}/${initialData.spreadSheetId}`, { | |
| method: 'GET', | |
| headers: { 'Content-Type': 'application/json' }, | |
| credentials: "include", | |
| }); | |
| if (!res.ok) { | |
| throw new Error(`Failed to fetch sheets: ${res.status}`); | |
| } | |
| const data = await res.json(); | |
| if (data.files?.data?.length > 0) { | |
| setSheets(data.files.data); | |
| } | |
| } catch (error) { | |
| console.error('Failed to fetch initial sheets:', error); | |
| } finally { | |
| setLoading(false); | |
| } | |
| }; | |
| fetchInitialSheets(); | |
| }, [initialData?.credentialId, initialData?.spreadSheetId]); |
🤖 Prompt for AI Agents
In apps/web/app/components/nodes/GoogleSheetFormClient.tsx around lines 88 to
111, the effect is parsing the fetch response without checking res.ok and
assumes a specific response shape (data.files?.data?.length) which differs from
other fetches; update the code to first check if (!res.ok) and handle/log the
non-2xx response (optionally parse error body) and return early, then call
res.json() only for successful responses, and normalize the payload when setting
sheets to accept both shapes (e.g., prefer data.files.data if present, otherwise
data.files) before calling setSheets; ensure setLoading is still toggled in
finally.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/app/components/nodes/CreateWorkFlow.tsx (1)
291-305: Missing node tracking state prevents pre-populating Google Sheet form when editing.When a Google Sheet node is clicked (lines 292-304), the code sets
credTypeand togglesloadSheet, but doesn't store which specific node was clicked or its configuration. Line 324 rendersGoogleSheetFormClientwithout aninitialDataprop.According to the AI summary,
GoogleSheetFormClientnow accepts an optionalinitialDataprop to pre-populate the form with existing node configuration (credential, document, sheet, operation, range). Without tracking the clicked node's data, users cannot edit existing Google Sheet nodes—the form always opens empty.🔎 Proposed solution
Add state to track the selected node:
const [loadSheet, setLoadSheet] = useState<boolean>(false); + const [selectedNodeId, setSelectedNodeId] = useState<string>(""); + const [selectedNodeConfig, setSelectedNodeConfig] = useState<Record<string, unknown> | null>(null);Store the node data when clicked:
if (node.type === "action" || node.type === "trigger") { if (node.data.name === "Google Sheet") { console.log("sheet called"); console.log(node.id); + setSelectedNodeId(node.id); + setSelectedNodeConfig(node.data.config || null); setCredType( node.data.type === "google_sheet" ? "google_oauth" : "" );Pass
initialDatato the form:- {loadSheet && <GoogleSheetFormClient type={credType} />} + {loadSheet && <GoogleSheetFormClient + type={credType} + nodeType={selectedNodeId} + initialData={selectedNodeConfig ? { + range: selectedNodeConfig.range as string, + operation: selectedNodeConfig.operation as string, + sheetName: selectedNodeConfig.sheetName as string, + spreadSheetId: selectedNodeConfig.spreadsheetId as string, + credentialId: selectedNodeConfig.credentialId as string, + } : undefined} + />}
♻️ Duplicate comments (2)
apps/web/app/components/nodes/CreateWorkFlow.tsx (2)
282-305: Consider the UX impact of toggle behavior when switching between Google Sheet nodes.Line 302 uses
setLoadSheet(!loadSheet)which toggles the form. If a user clicks Google Sheet node A (form opens), then clicks Google Sheet node B, the form will close instead of staying open for node B. This could be confusing.Consider using
setLoadSheet(true)to always open the form when clicking a Google Sheet node, with a separate explicit close mechanism.🔎 Proposed fix
setCredType( node.data.type === "google_sheet" ? "google_oauth" : "" ); - setLoadSheet(!loadSheet); + setLoadSheet(true); console.log("hook called");
135-202: Critical issue from previous review remains unaddressed: Stop deriving type from name.Lines 148 and 166 still parse the type by splitting
nameon" - ", which is fragile and will break if the naming convention changes. The previous review explicitly requested using the authoritativeexistingTrigger.typeandnode.typeproperties from Redux instead.Additionally, line 148 declares
typeoutside theif(existingTrigger)guard on line 149, which means whenexistingNodeshas items butexistingTriggeris null (allowed by the guard on line 137),typewill beundefinedand then assigned todata.typeon line 157.🔎 Proposed fix using Redux type properties
function loadWorkflow(){ const START_X = 100; const GAP = 250; const Y = 200; const newNodes: NodeType[] = []; const newEdges: EdgeType[] = []; - const type = existingTrigger?.name.split(" - ")[0] if(existingTrigger){ newNodes.push({ id: `trigger-${existingTrigger.dbId}`, type: 'trigger' as const, position: { x: START_X, y: Y}, data:{ label: existingTrigger.name, name: existingTrigger.name, - type: type, + type: existingTrigger.type, icon: '📊', config: existingTrigger.config, } }) } existingNodes.forEach((node, index)=>{ const nodeId = `action-${node.dbId}-${index}`; - const type = node.name.split(" - ")[0] newNodes.push({ id: nodeId, type: 'action' as const, position: { x: START_X + (index + 1) * GAP, y: Y}, data:{ label: node.name, name: node.name, icon: '⚙️', - type: type, + type: node.type, config: node.config, } }); });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/app/components/nodes/CreateWorkFlow.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/app/components/nodes/CreateWorkFlow.tsx (2)
apps/web/app/hooks/redux.ts (1)
useAppSelector(7-7)apps/web/app/types/workflow.types.ts (1)
NodeType(1-11)
3 participants
Summary by CodeRabbit
New Features
Refactor
Other
✏️ Tip: You can customize this high-level summary in your review settings.