Open
Conversation
Contributor
WalkthroughThe ForkForm component is refactored to adopt a Provider-based architecture with enhanced form management and type field support for branches. FormGenerator is updated to integrate stack-based type creation modals with breadcrumb navigation, wiring a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ForkForm
participant Provider
participant TypeEditor
participant FormGenerator
participant DynamicModal
User->>ForkForm: Interact with branch type field
ForkForm->>Provider: Request form context
Provider->>ForkForm: Provide form control & diagnostics
ForkForm->>TypeEditor: Click edit type
TypeEditor->>ForkForm: Trigger openRecordEditor callback
ForkForm->>FormGenerator: Call openRecordEditor(isOpen, formValues, editingField)
FormGenerator->>DynamicModal: Render type creation modal (depth 1)
User->>DynamicModal: Edit/create type
DynamicModal->>FormGenerator: Return newType value
FormGenerator->>ForkForm: Pass newType to form state
ForkForm->>Provider: Update branch-${index}-type value
Provider->>User: Display updated type in form
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (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}
Contributor
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 (2)
workspaces/ballerina/ballerina-visualizer/src/views/BI/Forms/ForkForm/index.tsx (1)
249-264:⚠️ Potential issue | 🟡 Minor
removeWorkerdoes not clean up stale form values for the last branch index.After filtering out the removed branch and shifting values upward, the form values for the old last index (
branch-${branches.length - 1}andbranch-${branches.length - 1}-type) remain registered and populated. This could cause the save handler to pick up ghost data from the removed branch.Proposed fix — unregister or clear the old last index
for (let i = index + 1; i < branches.length; i++) { const value = getValues(`branch-${i}`); setValue(`branch-${i - 1}`, value); const typeValue = getValues(`branch-${i}-type`); setValue(`branch-${i - 1}-type`, typeValue); } + // Clean up stale values for the removed last branch index + const lastIndex = branches.length - 1; + setValue(`branch-${lastIndex}`, undefined); + setValue(`branch-${lastIndex}-type`, undefined); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-visualizer/src/views/BI/Forms/ForkForm/index.tsx` around lines 249 - 264, The removeWorker function leaves stale form entries for the old last branch index after shifting values; after calling setBranches and shifting values with getValues/setValue (in removeWorker), clear or unregister the leftover fields for the previous last index (e.g., call unregister or setValue('', undefined) for `branch-${branches.length - 1}` and `branch-${branches.length - 1}-type`) so the removed branch's data is not retained by the form; ensure you reference and update the same form API used elsewhere (getValues, setValue, unregister) within removeWorker.workspaces/ballerina/ballerina-visualizer/src/views/BI/Forms/FormGenerator/index.tsx (1)
533-545:⚠️ Potential issue | 🟡 MinorUse the correct pattern for extracting
typefromNodePropertiesobjects.When
newTypeis aNodePropertiesobject, the ternary on line 543 incorrectly falls through tof[editingField?.key], discarding the object. Compare this with the same logic inFormGeneratorNew/index.tsx:821, which properly extracts thetypeproperty fromNodeProperties:Corrected pattern
// Current (incorrect): const newTypeValue = typeof newType === 'string' ? newType : f[editingField?.key]; // Should be: const newTypeValue = newType ? (typeof newType === 'string' ? newType : (newType as NodeProperties)?.type || newType) : f[editingField?.key];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-visualizer/src/views/BI/Forms/FormGenerator/index.tsx` around lines 533 - 545, In handleOpenTypeEditor update the newTypeValue logic to handle NodeProperties correctly: when newType is provided and is a string use it; when newType is an object (NodeProperties) extract its .type (or fall back to the object if .type is missing); only when newType is not provided fall back to f[editingField?.key]; update the computation of newTypeValue before calling setTypeEditorState (symbols: handleOpenTypeEditor, newTypeValue, NodeProperties, setTypeEditorState, fields, setBaseFields).
🧹 Nitpick comments (3)
workspaces/ballerina/ballerina-visualizer/src/views/BI/Forms/ForkForm/index.tsx (2)
48-60: Import statement placed after non-import code.The
importon line 60 is placed after theFieldGroupstyled component definition (lines 49–59). ES module imports are hoisted so this won't cause a runtime error, but it hurts readability and violates the convention of grouping all imports at the top of the file.Move the import on line 60 above the
FieldGroupdeclaration (i.e., after line 46).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-visualizer/src/views/BI/Forms/ForkForm/index.tsx` around lines 48 - 60, Move the import for convertNodePropertyToFormField and removeDuplicateDiagnostics so all imports are grouped at the top of the file above the styled component declaration; specifically, place the line importing convertNodePropertyToFormField and removeDuplicateDiagnostics before the FieldGroup styled component declaration (which is declared as const FieldGroup = styled.div`...`) to restore conventional import ordering and improve readability.
352-359:getFormValuesredundantly re-reads values already ingetValues().Since all branch values are set via
setValue, they are already in the form state. The spread{ ...getValues() }already contains everybranch-${index}andbranch-${index}-typekey, so theforEachloop overwrites them with identical values. This is harmless but unnecessary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-visualizer/src/views/BI/Forms/ForkForm/index.tsx` around lines 352 - 359, getFormValues redundantly re-reads branch fields already present in the object created by {...getValues()}; remove the unnecessary branches.forEach loop and simply return the form values obtained from getValues (or spread it once into a FormValues object) so branch-${index} and branch-${index}-type aren’t re-read; update the getFormValues function (which references getValues, branches, FormValues) to return the form state directly.workspaces/ballerina/ballerina-visualizer/src/views/BI/Forms/FormGenerator/index.tsx (1)
1337-1391: DynamicModal + breadcrumb + FormTypeEditor stack is duplicated three times.The Fork form block (lines 1353–1389), the Variable form block (lines 1479–1513), and the default form block (lines 1605–1639) contain near-identical DynamicModal rendering logic with breadcrumbs and
FormTypeEditor. Consider extracting a sharedTypeCreationModalStackcomponent to reduce duplication and ensure consistency.Note: the Fork/Variable sections show breadcrumbs when
stack.slice(0, i + 1).length > 1, while the default form uses> 2. Verify this divergence is intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-visualizer/src/views/BI/Forms/FormGenerator/index.tsx` around lines 1337 - 1391, Duplicate DynamicModal + breadcrumb + FormTypeEditor rendering appears across the Fork/Variable/default blocks; extract a reusable component (e.g., TypeCreationModalStack) that accepts props: stack, typeEditorState, handleTypeEditorStateChange, refetchStates, isGraphql, getNewTypeCreateForm, onTypeChange, onSaveType and peekTypeStack to render the modal list, breadcrumb (using BreadcrumbContainer/BreadcrumbSeparator/BreadcrumbItem), and FormTypeEditor; replace the three duplicated blocks with this new component and pass the appropriate refetchStates[i] and other handlers; also reconcile the breadcrumb condition (currently stack.slice(0, i + 1).length > 1 in Fork/Variable vs > 2 in default) — decide which is correct and use the same condition in the new component.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/Forms/ForkForm/index.tsx`:
- Around line 391-430: The map callback for branches.map can return undefined
when branch.properties?.variable is falsy and also accesses
branch.properties.type without a guard; change the logic to first filter
branches for those with branch.properties?.variable (e.g., branches.filter(b =>
b.properties?.variable).map(...)) so the callback always returns a React node,
and before calling convertNodePropertyToFormField for the type, guard or provide
a safe fallback for branch.properties.type (e.g., check if
branch.properties?.type exists and only create/type the typeField when present,
or pass an empty/default NodeProperties) — update the code around branches.map,
convertNodePropertyToFormField, variableField and typeField creation and the
removeWorker/openRecordEditor usages accordingly.
---
Outside diff comments:
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/Forms/ForkForm/index.tsx`:
- Around line 249-264: The removeWorker function leaves stale form entries for
the old last branch index after shifting values; after calling setBranches and
shifting values with getValues/setValue (in removeWorker), clear or unregister
the leftover fields for the previous last index (e.g., call unregister or
setValue('', undefined) for `branch-${branches.length - 1}` and
`branch-${branches.length - 1}-type`) so the removed branch's data is not
retained by the form; ensure you reference and update the same form API used
elsewhere (getValues, setValue, unregister) within removeWorker.
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/Forms/FormGenerator/index.tsx`:
- Around line 533-545: In handleOpenTypeEditor update the newTypeValue logic to
handle NodeProperties correctly: when newType is provided and is a string use
it; when newType is an object (NodeProperties) extract its .type (or fall back
to the object if .type is missing); only when newType is not provided fall back
to f[editingField?.key]; update the computation of newTypeValue before calling
setTypeEditorState (symbols: handleOpenTypeEditor, newTypeValue, NodeProperties,
setTypeEditorState, fields, setBaseFields).
---
Nitpick comments:
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/Forms/ForkForm/index.tsx`:
- Around line 48-60: Move the import for convertNodePropertyToFormField and
removeDuplicateDiagnostics so all imports are grouped at the top of the file
above the styled component declaration; specifically, place the line importing
convertNodePropertyToFormField and removeDuplicateDiagnostics before the
FieldGroup styled component declaration (which is declared as const FieldGroup =
styled.div`...`) to restore conventional import ordering and improve
readability.
- Around line 352-359: getFormValues redundantly re-reads branch fields already
present in the object created by {...getValues()}; remove the unnecessary
branches.forEach loop and simply return the form values obtained from getValues
(or spread it once into a FormValues object) so branch-${index} and
branch-${index}-type aren’t re-read; update the getFormValues function (which
references getValues, branches, FormValues) to return the form state directly.
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/Forms/FormGenerator/index.tsx`:
- Around line 1337-1391: Duplicate DynamicModal + breadcrumb + FormTypeEditor
rendering appears across the Fork/Variable/default blocks; extract a reusable
component (e.g., TypeCreationModalStack) that accepts props: stack,
typeEditorState, handleTypeEditorStateChange, refetchStates, isGraphql,
getNewTypeCreateForm, onTypeChange, onSaveType and peekTypeStack to render the
modal list, breadcrumb (using
BreadcrumbContainer/BreadcrumbSeparator/BreadcrumbItem), and FormTypeEditor;
replace the three duplicated blocks with this new component and pass the
appropriate refetchStates[i] and other handlers; also reconcile the breadcrumb
condition (currently stack.slice(0, i + 1).length > 1 in Fork/Variable vs > 2 in
default) — decide which is correct and use the same condition in the new
component.
Comment on lines
+391
to
+430
| {branches.map((branch, index) => { | ||
| if (branch.properties?.variable) { | ||
| const variableField = convertNodePropertyToFormField(`branch-${index}`, branch.properties.variable); | ||
| variableField.types = [{ fieldType: "IDENTIFIER", selected: false }] | ||
| const typeField = convertNodePropertyToFormField(`branch-${index}-type`, branch.properties.type); | ||
| typeField.types = [{ fieldType: "TYPE", selected: false }] | ||
| variableField.label = "Worker " + (index + 1); | ||
| return ( | ||
| <FormStyles.Row key={variableField.key}> | ||
| <FieldGroup> | ||
| <div> | ||
| <EditorFactory field={variableField} /> | ||
| </div> | ||
| <div> | ||
| <TypeEditor | ||
| field={typeField} | ||
| openRecordEditor={(open: boolean, newType?: string | NodeProperties) => | ||
| openRecordEditor && openRecordEditor(open, getFormValues(), typeField, newType) | ||
| } | ||
| /> | ||
| </div> | ||
|
|
||
| <div style={{ position: 'absolute', top: 8, right: 8 }}> | ||
| {branches.length > 2 && ( | ||
| <Codicon | ||
| name="close" | ||
| sx={{ cursor: 'pointer', opacity: 0.6, '&:hover': { opacity: 1 } }} | ||
| onClick={() => { | ||
| if (branches.length > 2) { | ||
| removeWorker(index); | ||
| } | ||
| }} | ||
| /> | ||
| )} | ||
| </div> | ||
| </FieldGroup> | ||
| </FormStyles.Row> | ||
| ); | ||
| } | ||
| })} |
Contributor
There was a problem hiding this comment.
Map callback does not return on all paths — will produce undefined entries.
When branch.properties?.variable is falsy, the .map() callback returns undefined implicitly. Biome flagged this as well. Use .filter().map() or add an explicit return null for the else case.
Additionally, line 395 accesses branch.properties.type without a guard. If a pre-existing branch lacks a type property, convertNodePropertyToFormField receives undefined, likely causing a runtime error.
Proposed fix
- {branches.map((branch, index) => {
- if (branch.properties?.variable) {
- const variableField = convertNodePropertyToFormField(`branch-${index}`, branch.properties.variable);
- variableField.types = [{ fieldType: "IDENTIFIER", selected: false }]
- const typeField = convertNodePropertyToFormField(`branch-${index}-type`, branch.properties.type);
+ {branches.map((branch, index) => {
+ if (branch.properties?.variable) {
+ const variableField = convertNodePropertyToFormField(`branch-${index}`, branch.properties.variable);
+ variableField.types = [{ fieldType: "IDENTIFIER", selected: false }]
+ if (!branch.properties.type) {
+ return null;
+ }
+ const typeField = convertNodePropertyToFormField(`branch-${index}-type`, branch.properties.type);
typeField.types = [{ fieldType: "TYPE", selected: false }]
variableField.label = "Worker " + (index + 1);
return (
...
);
}
+ return null;
})}📝 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.
Suggested change
| {branches.map((branch, index) => { | |
| if (branch.properties?.variable) { | |
| const variableField = convertNodePropertyToFormField(`branch-${index}`, branch.properties.variable); | |
| variableField.types = [{ fieldType: "IDENTIFIER", selected: false }] | |
| const typeField = convertNodePropertyToFormField(`branch-${index}-type`, branch.properties.type); | |
| typeField.types = [{ fieldType: "TYPE", selected: false }] | |
| variableField.label = "Worker " + (index + 1); | |
| return ( | |
| <FormStyles.Row key={variableField.key}> | |
| <FieldGroup> | |
| <div> | |
| <EditorFactory field={variableField} /> | |
| </div> | |
| <div> | |
| <TypeEditor | |
| field={typeField} | |
| openRecordEditor={(open: boolean, newType?: string | NodeProperties) => | |
| openRecordEditor && openRecordEditor(open, getFormValues(), typeField, newType) | |
| } | |
| /> | |
| </div> | |
| <div style={{ position: 'absolute', top: 8, right: 8 }}> | |
| {branches.length > 2 && ( | |
| <Codicon | |
| name="close" | |
| sx={{ cursor: 'pointer', opacity: 0.6, '&:hover': { opacity: 1 } }} | |
| onClick={() => { | |
| if (branches.length > 2) { | |
| removeWorker(index); | |
| } | |
| }} | |
| /> | |
| )} | |
| </div> | |
| </FieldGroup> | |
| </FormStyles.Row> | |
| ); | |
| } | |
| })} | |
| {branches.map((branch, index) => { | |
| if (branch.properties?.variable) { | |
| const variableField = convertNodePropertyToFormField(`branch-${index}`, branch.properties.variable); | |
| variableField.types = [{ fieldType: "IDENTIFIER", selected: false }] | |
| if (!branch.properties.type) { | |
| return null; | |
| } | |
| const typeField = convertNodePropertyToFormField(`branch-${index}-type`, branch.properties.type); | |
| typeField.types = [{ fieldType: "TYPE", selected: false }] | |
| variableField.label = "Worker " + (index + 1); | |
| return ( | |
| <FormStyles.Row key={variableField.key}> | |
| <FieldGroup> | |
| <div> | |
| <EditorFactory field={variableField} /> | |
| </div> | |
| <div> | |
| <TypeEditor | |
| field={typeField} | |
| openRecordEditor={(open: boolean, newType?: string | NodeProperties) => | |
| openRecordEditor && openRecordEditor(open, getFormValues(), typeField, newType) | |
| } | |
| /> | |
| </div> | |
| <div style={{ position: 'absolute', top: 8, right: 8 }}> | |
| {branches.length > 2 && ( | |
| <Codicon | |
| name="close" | |
| sx={{ cursor: 'pointer', opacity: 0.6, '&:hover': { opacity: 1 } }} | |
| onClick={() => { | |
| if (branches.length > 2) { | |
| removeWorker(index); | |
| } | |
| }} | |
| /> | |
| )} | |
| </div> | |
| </FieldGroup> | |
| </FormStyles.Row> | |
| ); | |
| } | |
| return null; | |
| })} |
🧰 Tools
🪛 Biome (2.3.14)
[error] 391-391: This callback passed to map() iterable method should always return a value.
Add missing return statements so that this callback returns a value on all execution paths.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/Forms/ForkForm/index.tsx`
around lines 391 - 430, The map callback for branches.map can return undefined
when branch.properties?.variable is falsy and also accesses
branch.properties.type without a guard; change the logic to first filter
branches for those with branch.properties?.variable (e.g., branches.filter(b =>
b.properties?.variable).map(...)) so the callback always returns a React node,
and before calling convertNodePropertyToFormField for the type, guard or provide
a safe fallback for branch.properties.type (e.g., check if
branch.properties?.type exists and only create/type the typeField when present,
or pass an empty/default NodeProperties) — update the code around branches.map,
convertNodePropertyToFormField, variableField and typeField creation and the
removeWorker/openRecordEditor usages accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Fix issues in the Fork form where key UI interactions were not working correctly:
These issues negatively impacted the Fork configuration workflow and usability.
Partially Resolves: wso2/product-ballerina-integrator#2419
Goals
Approach
Summary by CodeRabbit
New Features
Improvements