Conversation
Co-authored-by: George Djabarov <djabarovgeorge@users.noreply.github.com>
…de functionality - Updated `updateStepInWorkflow` to handle undefined control values more gracefully. - Added `enableOverrides` and `disableOverrides` methods to `SaveFormContext` for better control state management. - Integrated override functionality in `CustomStepControls` to manage default values and form state. - Modified `EditStepTemplateV2Page` to utilize new override methods and manage form data accordingly.
✅ Deploy preview added
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Hey there and thank you for opening this pull request! 👋 We require pull request titles to follow specific formatting rules and it looks like your proposed title needs to be adjusted. Your PR title is: Requirements:
Expected format: Details: PR title must end with 'fixes TICKET-ID' (e.g., 'fixes NOV-123') or include ticket ID in branch name |
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
WalkthroughThis pull request introduces override control mechanisms for step controls in the workflow editor. Changes include modifying 🚥 Pre-merge checks | ✅ 1 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
djabarovgeorge
changed the title
djabarovgeorge
changed the title
djabarovgeorge
changed the title
![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
🤖 Fix all issues with AI agents
In
`@apps/dashboard/src/components/workflow-editor/steps/controls/custom-step-controls.tsx`:
- Line 106: The line assigning defaultValues calls
buildDefaultValuesOfDataSchema(step.controls.dataSchema ?? {}) and can throw if
step.controls is undefined; change the access to use optional chaining (i.e.,
step.controls?.dataSchema ?? {}) so buildDefaultValuesOfDataSchema always
receives a safe object; update the expression where defaultValues is defined
(referenced symbol: defaultValues and function buildDefaultValuesOfDataSchema)
to use step.controls?.dataSchema as the nullable source.
In `@apps/dashboard/src/pages/edit-step-template-v2.tsx`:
- Around line 19-25: The code uses step?.controls.values which can throw when
step exists but controls is undefined; update the logic around
hasOverrides/initialFormValues/shouldSaveOnAutosaveRef and the useForm call to
safely access controls by using step?.controls?.values (or a safe fallback like
{}) instead of step?.controls.values so initialFormValues never dereferences
undefined; ensure hasOverrides computation still checks step?.controlValues and
that useForm receives a valid default values object.
- Around line 19-21: The ref shouldSaveOnAutosaveRef is initialized once with
the initial hasOverrides value and never updated when step loads; add a sync so
that whenever hasOverrides becomes true after step loads the ref is updated.
Specifically, add an effect that watches hasOverrides (derived from
step?.controlValues) and sets shouldSaveOnAutosaveRef.current = hasOverrides so
autosave is enabled for pre-existing overrides; reference the hasOverrides
constant, shouldSaveOnAutosaveRef ref, and the step controlValues when
implementing the useEffect.
| const defaultValues = buildDefaultValuesOfDataSchema(step?.controls.dataSchema ?? {}); | ||
| if (!workflow || !step) return; | ||
|
|
||
| const defaultValues = buildDefaultValuesOfDataSchema(step.controls.dataSchema ?? {}); |
There was a problem hiding this comment.
Potential runtime error if step.controls is undefined.
Line 106 accesses step.controls.dataSchema without optional chaining on controls. While the guard on line 104 ensures step is defined, step.controls could still be undefined if the step hasn't loaded its control schema. Consider using step.controls?.dataSchema.
Proposed fix
- const defaultValues = buildDefaultValuesOfDataSchema(step.controls.dataSchema ?? {});
+ const defaultValues = buildDefaultValuesOfDataSchema(step.controls?.dataSchema ?? {});📝 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.
| const defaultValues = buildDefaultValuesOfDataSchema(step.controls.dataSchema ?? {}); | |
| const defaultValues = buildDefaultValuesOfDataSchema(step.controls?.dataSchema ?? {}); |
🤖 Prompt for AI Agents
In
`@apps/dashboard/src/components/workflow-editor/steps/controls/custom-step-controls.tsx`
at line 106, The line assigning defaultValues calls
buildDefaultValuesOfDataSchema(step.controls.dataSchema ?? {}) and can throw if
step.controls is undefined; change the access to use optional chaining (i.e.,
step.controls?.dataSchema ?? {}) so buildDefaultValuesOfDataSchema always
receives a safe object; update the expression where defaultValues is defined
(referenced symbol: defaultValues and function buildDefaultValuesOfDataSchema)
to use step.controls?.dataSchema as the nullable source.
| const hasOverrides = step?.controlValues != null && Object.keys(step?.controlValues ?? {}).length > 0; | ||
| const initialFormValues = hasOverrides ? step?.controlValues : step?.controls.values; | ||
| const shouldSaveOnAutosaveRef = useRef(hasOverrides); | ||
|
|
||
| const form = useForm({ | ||
| defaultValues, | ||
| values: step?.controls.values, | ||
| values: initialFormValues, |
There was a problem hiding this comment.
step?.controls.values can throw if controls is undefined.
On line 20, step?.controls.values uses optional chaining only on step, not on controls. If step exists but step.controls is undefined, this will throw a TypeError. This mirrors the same pattern in custom-step-controls.tsx line 106.
Proposed fix
- const initialFormValues = hasOverrides ? step?.controlValues : step?.controls.values;
+ const initialFormValues = hasOverrides ? step?.controlValues : step?.controls?.values;📝 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.
| const hasOverrides = step?.controlValues != null && Object.keys(step?.controlValues ?? {}).length > 0; | |
| const initialFormValues = hasOverrides ? step?.controlValues : step?.controls.values; | |
| const shouldSaveOnAutosaveRef = useRef(hasOverrides); | |
| const form = useForm({ | |
| defaultValues, | |
| values: step?.controls.values, | |
| values: initialFormValues, | |
| const hasOverrides = step?.controlValues != null && Object.keys(step?.controlValues ?? {}).length > 0; | |
| const initialFormValues = hasOverrides ? step?.controlValues : step?.controls?.values; | |
| const shouldSaveOnAutosaveRef = useRef(hasOverrides); | |
| const form = useForm({ | |
| defaultValues, | |
| values: initialFormValues, |
🤖 Prompt for AI Agents
In `@apps/dashboard/src/pages/edit-step-template-v2.tsx` around lines 19 - 25, The
code uses step?.controls.values which can throw when step exists but controls is
undefined; update the logic around
hasOverrides/initialFormValues/shouldSaveOnAutosaveRef and the useForm call to
safely access controls by using step?.controls?.values (or a safe fallback like
{}) instead of step?.controls.values so initialFormValues never dereferences
undefined; ensure hasOverrides computation still checks step?.controlValues and
that useForm receives a valid default values object.
| const hasOverrides = step?.controlValues != null && Object.keys(step?.controlValues ?? {}).length > 0; | ||
| const initialFormValues = hasOverrides ? step?.controlValues : step?.controls.values; | ||
| const shouldSaveOnAutosaveRef = useRef(hasOverrides); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, locate and view the edit-step-template-v2.tsx file around the lines in question
echo "=== Viewing edit-step-template-v2.tsx lines 1-50 ==="
head -50 apps/dashboard/src/pages/edit-step-template-v2.tsx
echo ""
echo "=== Line count of the file ==="
wc -l apps/dashboard/src/pages/edit-step-template-v2.tsxRepository: novuhq/novu
Length of output: 2125
🏁 Script executed:
#!/bin/bash
# Check the useWorkflow hook and step loading pattern
echo "=== Looking for useWorkflow hook definition ==="
fd -t f 'workflow-provider' apps/dashboard/src --exec cat {} \; 2>/dev/null | head -100
echo ""
echo "=== Search for step usage in the context ==="
rg -n 'const.*step.*=|step\s*\?' apps/dashboard/src/pages/edit-step-template-v2.tsx | head -20Repository: novuhq/novu
Length of output: 4325
🏁 Script executed:
#!/bin/bash
# Check if there are any guards for undefined step at the start of component
echo "=== Check full context of edit-step-template-v2.tsx around lines 19-21 ==="
sed -n '1,80p' apps/dashboard/src/pages/edit-step-template-v2.tsxRepository: novuhq/novu
Length of output: 2817
🏁 Script executed:
#!/bin/bash
# Find the complete step value in the WorkflowProvider context
echo "=== Search for where step is set in context value ==="
rg -n -A 200 'export const WorkflowProvider' apps/dashboard/src/components/workflow-editor/workflow-provider.tsx | grep -A 100 'useMemo\|step:' | head -80Repository: novuhq/novu
Length of output: 2908
🏁 Script executed:
#!/bin/bash
# Check if there's a route guard that ensures step exists before rendering EditStepTemplateV2Page
echo "=== Search for route protection or guards for EditStepTemplateV2Page ==="
rg -r -n 'EditStepTemplateV2Page\|edit-step-template-v2' apps/dashboard/src --type tsx --type ts | head -20Repository: novuhq/novu
Length of output: 172
🏁 Script executed:
#!/bin/bash
# Look for where the context value is created with step
echo "=== Full WorkflowProvider to find step in context value ==="
sed -n '1,250p' apps/dashboard/src/components/workflow-editor/workflow-provider.tsxRepository: novuhq/novu
Length of output: 8000
🏁 Script executed:
#!/bin/bash
# View the complete file to see if there's any guard at the component level
echo "=== Full EditStepTemplateV2Page.tsx ==="
cat apps/dashboard/src/pages/edit-step-template-v2.tsxRepository: novuhq/novu
Length of output: 4503
🏁 Script executed:
#!/bin/bash
# Check if there's a route guard or parent component
echo "=== Search for where EditStepTemplateV2Page is used/rendered ==="
rg -n 'EditStepTemplateV2Page' apps/dashboard/src --type ts -A 2 -B 2Repository: novuhq/novu
Length of output: 1316
Sync shouldSaveOnAutosaveRef when step loads with existing overrides.
useRef(hasOverrides) captures the initial false value on first render (before step is available). When step loads asynchronously with existing controlValues, hasOverrides becomes true, but the ref remains false. This means autosave is disabled for pre-existing overrides until enableOverrides() is called, defeating the intent to auto-save edits when overrides already exist.
The component has guards that prevent rendering until step is available (lines 110–125), but the ref is still initialized with hasOverrides = false on the initial render before those guards take effect.
Proposed fix — sync ref when step loads with existing overrides
const shouldSaveOnAutosaveRef = useRef(hasOverrides);
+
+ useEffect(() => {
+ if (hasOverrides) {
+ shouldSaveOnAutosaveRef.current = true;
+ }
+ }, [hasOverrides]);📝 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.
| const hasOverrides = step?.controlValues != null && Object.keys(step?.controlValues ?? {}).length > 0; | |
| const initialFormValues = hasOverrides ? step?.controlValues : step?.controls.values; | |
| const shouldSaveOnAutosaveRef = useRef(hasOverrides); | |
| const hasOverrides = step?.controlValues != null && Object.keys(step?.controlValues ?? {}).length > 0; | |
| const initialFormValues = hasOverrides ? step?.controlValues : step?.controls.values; | |
| const shouldSaveOnAutosaveRef = useRef(hasOverrides); | |
| useEffect(() => { | |
| if (hasOverrides) { | |
| shouldSaveOnAutosaveRef.current = true; | |
| } | |
| }, [hasOverrides]); |
🤖 Prompt for AI Agents
In `@apps/dashboard/src/pages/edit-step-template-v2.tsx` around lines 19 - 21, The
ref shouldSaveOnAutosaveRef is initialized once with the initial hasOverrides
value and never updated when step loads; add a sync so that whenever
hasOverrides becomes true after step loads the ref is updated. Specifically, add
an effect that watches hasOverrides (derived from step?.controlValues) and sets
shouldSaveOnAutosaveRef.current = hasOverrides so autosave is enabled for
pre-existing overrides; reference the hasOverrides constant,
shouldSaveOnAutosaveRef ref, and the step controlValues when implementing the
useEffect.
3 participants
What changed? Why was the change needed?
Screenshots
Expand for optional sections
Related enterprise PR
Special notes for your reviewer