send source columns in generic sql op form also#1702
Conversation
WalkthroughThis change adds client-side state management for source columns in the GenericSqlOpForm component. It introduces a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/TransformWorkflow/FlowEditor/Components/OperationPanel/Forms/GenericSqlOpForm.tsx (1)
135-142:⚠️ Potential issue | 🟠 MajorAdd
actionto theuseEffectdependency array.The effect branches on
action(line 137) to determine whether to fetch edit config or source columns, butactionis missing from the dependencies (line 142). SinceOperationFormdoesn't remount whenactionchanges viapanelOpFormState.current, the effect won't re-run when the mode switches between'create'and'edit'/'view'. This causes stale initialization—the form may fetch the wrong data or load with outdated state.Fix
- }, [session, node]); + }, [session, node, action]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/TransformWorkflow/FlowEditor/Components/OperationPanel/Forms/GenericSqlOpForm.tsx` around lines 135 - 142, The useEffect that decides between fetchAndSetConfigForEdit() and fetchAndSetSourceColumns() depends on the variable action but does not include it in the dependency array; update the dependency array of the useEffect (the effect that checks node?.data.isDummy and calls fetchAndSetConfigForEdit or fetchAndSetSourceColumns) to include action alongside session and node so the effect re-runs when action changes and the form initializes correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/components/TransformWorkflow/FlowEditor/Components/OperationPanel/Forms/GenericSqlOpForm.tsx`:
- Around line 51-55: Guard against undefined source/output columns before
updating state and before sending payloads: in fetchAndSetSourceColumns use
node.data.output_columns ?? [] when calling setSrcColumns so srcColumns is
always a string[]; likewise when destructuring source_columns from
operation_config.config ensure a default (e.g., const { source_columns = [] } =
operation_config.config || {}) so source_columns is never undefined; finally
ensure the payloads built for CreateOperationNodePayload and
EditOperationNodePayload use srcColumns ?? [] (or the guarded source_columns) so
the API always receives a string[].
---
Outside diff comments:
In
`@src/components/TransformWorkflow/FlowEditor/Components/OperationPanel/Forms/GenericSqlOpForm.tsx`:
- Around line 135-142: The useEffect that decides between
fetchAndSetConfigForEdit() and fetchAndSetSourceColumns() depends on the
variable action but does not include it in the dependency array; update the
dependency array of the useEffect (the effect that checks node?.data.isDummy and
calls fetchAndSetConfigForEdit or fetchAndSetSourceColumns) to include action
alongside session and node so the effect re-runs when action changes and the
form initializes correctly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/components/TransformWorkflow/FlowEditor/Components/OperationPanel/Forms/GenericSqlOpForm.tsx
| const fetchAndSetSourceColumns = async () => { | ||
| if (node) { | ||
| setSrcColumns(node.data.output_columns); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# First, find the target file
fd "GenericSqlOpForm.tsx" -HI
# Also search for related type definition files
fd "transform-v2.types.ts" -HI
fd "OperationFormProps" -HIRepository: DalgoT4D/webapp
Length of output: 187
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get line count for GenericSqlOpForm.tsx
wc -l src/components/TransformWorkflow/FlowEditor/Components/OperationPanel/Forms/GenericSqlOpForm.tsx
# Read the relevant portions of GenericSqlOpForm.tsx
echo "=== Lines 40-130 of GenericSqlOpForm.tsx ==="
sed -n '40,130p' src/components/TransformWorkflow/FlowEditor/Components/OperationPanel/Forms/GenericSqlOpForm.tsx
echo ""
echo "=== Lines 70-90 of GenericSqlOpForm.tsx ==="
sed -n '70,90p' src/components/TransformWorkflow/FlowEditor/Components/OperationPanel/Forms/GenericSqlOpForm.tsxRepository: DalgoT4D/webapp
Length of output: 3436
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the types file
wc -l src/types/transform-v2.types.ts
echo "=== Full transform-v2.types.ts ==="
cat -n src/types/transform-v2.types.tsRepository: DalgoT4D/webapp
Length of output: 4599
🏁 Script executed:
#!/bin/bash
set -eulo pipefail
# Search for GenericDataConfig
rg "GenericDataConfig" -nC 5
# Also check if there are imports in GenericSqlOpForm.tsx
echo "=== Imports and type usage in GenericSqlOpForm.tsx ==="
sed -n '1,40p' src/components/TransformWorkflow/FlowEditor/Components/OperationPanel/Forms/GenericSqlOpForm.tsxRepository: DalgoT4D/webapp
Length of output: 4314
Guard source_columns and output_columns defaults before setting state.
At line 53, setSrcColumns(node.data.output_columns) doesn't handle cases where output_columns is undefined. Similarly, at line 121, source_columns is destructured from operation_config.config (typed as any), which may not have this field. If either is missing, srcColumns becomes undefined and gets posted to the API at lines 75 and 87, violating the string[] requirement in CreateOperationNodePayload and EditOperationNodePayload.
Proposed fix
- const fetchAndSetSourceColumns = async () => {
- if (node) {
- setSrcColumns(node.data.output_columns);
- }
- };
+ const fetchAndSetSourceColumns = () => {
+ setSrcColumns(node?.data?.output_columns ?? []);
+ };
@@
const { sql_statement_2, sql_statement_1, source_columns }: GenericDataConfig =
operation_config.config;
- setSrcColumns(source_columns);
+ setSrcColumns(source_columns ?? []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/components/TransformWorkflow/FlowEditor/Components/OperationPanel/Forms/GenericSqlOpForm.tsx`
around lines 51 - 55, Guard against undefined source/output columns before
updating state and before sending payloads: in fetchAndSetSourceColumns use
node.data.output_columns ?? [] when calling setSrcColumns so srcColumns is
always a string[]; likewise when destructuring source_columns from
operation_config.config ensure a default (e.g., const { source_columns = [] } =
operation_config.config || {}) so source_columns is never undefined; finally
ensure the payloads built for CreateOperationNodePayload and
EditOperationNodePayload use srcColumns ?? [] (or the guarded source_columns) so
the API always receives a string[].
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1702 +/- ##
==========================================
+ Coverage 75.34% 75.36% +0.02%
==========================================
Files 107 107
Lines 7966 7973 +7
Branches 1927 1931 +4
==========================================
+ Hits 6002 6009 +7
+ Misses 1927 1840 -87
- Partials 37 124 +87 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| inputName = 'chained'; | ||
| } | ||
|
|
||
| const fetchAndSetSourceColumns = async () => { |
There was a problem hiding this comment.
why async ? we are not awaiting it anywhere
There was a problem hiding this comment.
We dont need to await for an api call always. It can also run asynchronous in a non-blocking way. its like trigger it and forget.
Unless if we had some loading and couldn't move forward without it the results of this api, we would use await.
2 participants
Summary by CodeRabbit