Fix recursive type creation iss #1348
Conversation
WalkthroughThis PR introduces fieldIndex tracking to support nested type creation within the type editor system. When users create new types for specific fields, the fieldIndex is now preserved through stack operations and propagated upward when saving, enabling parent types to correctly update their field references with the newly created child types. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Editor as Type Editor
participant Stack as Type Stack Manager
participant Parent as Parent Type
participant Field as Field Handler
User->>Editor: Click "Create nested type" for field
Editor->>Stack: getNewTypeCreateForm(fieldIndex, typeName?)
Stack->>Stack: Preserve fieldIndex on current top
Stack->>Stack: Push new type creation frame
Stack->>Editor: Render nested type form
User->>Editor: Fill nested type details
User->>Editor: Save nested type
Editor->>Stack: onSaveType(type)
Stack->>Stack: Check if stack.length > 1
alt Parent exists on stack
Stack->>Parent: Retrieve parent from stack
Stack->>Field: Determine field type (returnType vs member)
Field->>Parent: Update field at fieldIndex with type name
Stack->>Stack: Update parent in stack
Stack->>Stack: Pop nested type from top
else No parent
Stack->>Stack: Pop type
end
Stack->>Editor: Refresh state with updated stack
Editor->>User: Nested type created and field updated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
workspaces/ballerina/type-editor/src/TypeEditor/ContextBasedTypeEditor/ContextTypeCreator.tsx (1)
193-200: Avoid duplicateonTypeChangeduring rename.
handleSetType()now triggersonTypeChange, buteditTypeNamealready callsonTypeChange(renamedType, true), so renames will emit two change events. Consider adding a flag to skip the default notification when a rename-specific notification will follow.🛠️ Suggested adjustment
- const handleSetType = (type: Type) => { - onTypeChange(type); + const handleSetType = (type: Type, notifyChange = true) => { + if (notifyChange) { + onTypeChange(type); + } replaceTop({ type: type, isDirty: true }) setType(type); }- handleSetType(renamedType); - onTypeChange(renamedType, true); + handleSetType(renamedType, false); + onTypeChange(renamedType, true);workspaces/ballerina/ballerina-visualizer/src/views/BI/Forms/FormGenerator/index.tsx (1)
317-327: PreservefieldIndexwhen it is 0.The current truthy check drops index
0, so the first field loses its association and later propagation can fail. Use a nullish check instead.🧩 Proposed fix
- if (newStack[newStack.length - 1].fieldIndex) { - item.fieldIndex = newStack[newStack.length - 1].fieldIndex; - } + const prevFieldIndex = newStack[newStack.length - 1].fieldIndex; + if (prevFieldIndex !== undefined) { + item.fieldIndex = prevFieldIndex; + }workspaces/ballerina/type-editor/src/Context/index.tsx (1)
47-52: Fix defaultonTypeCreatevalue to match the updated signature requiringfieldIndex.The default in Context/index.tsx (line 66) is
onTypeCreate: () => {}, which doesn't satisfy the new signature(fieldIndex: number, typeName?: string) => voiddeclared at line 51. Update the default to accept the fieldIndex parameter:onTypeCreate: () => Promise.resolve({} as AddImportItemResponse)or an appropriate no-op that accepts the parameters. All call sites in TypeField.tsx properly pass fieldIndex; the issue is only with the default fallback value.workspaces/ballerina/ballerina-visualizer/src/views/GraphQLDiagram/index.tsx (1)
136-143: PreservefieldIndexwhen it is 0.The truthy check skips index
0, so the first field loses its index and later updates can target the wrong member (or crash). Use an explicitundefinedcheck.🛠️ Suggested fix
- if (newStack[newStack.length - 1].fieldIndex) { - item.fieldIndex = newStack[newStack.length - 1].fieldIndex; - } + const prevFieldIndex = newStack[newStack.length - 1].fieldIndex; + if (prevFieldIndex !== undefined) { + item.fieldIndex = prevFieldIndex; + }workspaces/ballerina/ballerina-visualizer/src/views/BI/Forms/FormGeneratorNew/index.tsx (1)
284-291: PreservefieldIndexwhen it is 0.The truthy check drops index
0, which can break updates for the first member/function. Use an explicitundefinedcheck.🛠️ Suggested fix
- if (newStack[newStack.length - 1].fieldIndex) { - item.fieldIndex = newStack[newStack.length - 1].fieldIndex; - } + const prevFieldIndex = newStack[newStack.length - 1].fieldIndex; + if (prevFieldIndex !== undefined) { + item.fieldIndex = prevFieldIndex; + }workspaces/ballerina/type-editor/src/TypeEditor/TypeField.tsx (1)
29-43: Prevent undefinedfieldIndexfrom reachingonTypeCreate.
fieldIndexis optional here, butonTypeCreatenow expects a number. With non‑strict null checks,undefinedcan slip through and mis-associate the created type with the wrong field. Prefer makingfieldIndexrequired (or explicitly guard and surface an error when missing).✅ Suggested fix (make fieldIndex required)
interface TypeFieldProps { type: string | Type; memberName: string; onChange: (value: string) => void; onUpdateImports: (imports: Imports) => void; placeholder?: string; sx?: React.CSSProperties; onValidationError?: (isError: boolean) => void; rootType: Type; isAnonymousRecord?: boolean; label?: string; required?: boolean; autoFocus?: boolean; - fieldIndex?: number; + fieldIndex: number; }Also applies to: 209-212
🤖 Fix all issues with AI agents
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/Forms/FormGenerator/index.tsx`:
- Around line 1308-1314: The function getNewTypeCreateForm currently mutates the
object returned by peekTypeStack and unconditionally assigns fieldIndex which
can overwrite an existing index with undefined; instead, when currentTopItem
exists only set fieldIndex if the incoming fieldIndex is not undefined and avoid
in-place mutation—create a shallow copy of currentTopItem, set copy.fieldIndex =
fieldIndex only when fieldIndex !== undefined, then call replaceTop(copy); leave
pushTypeStack(getDefaultValue(typeName)) unchanged.
- Around line 1083-1100: The onSaveType handler mutates newTop by indexing
newTop.fieldIndex into newTop.type.functions or newTop.type.members without
checking that fieldIndex is defined and within bounds; add a defensive guard in
onSaveType: verify newTop.fieldIndex is a finite number and between 0 and the
length-1 of the appropriate array (newTop.type.functions for CLASS, otherwise
newTop.type.members) before assigning returnType/type, and bail out or skip the
assignment if the guard fails; keep the rest of the flow (updating newStack,
popping, setStack, setRefetchForCurrentModal) unchanged.
In
`@workspaces/ballerina/ballerina-visualizer/src/views/GraphQLDiagram/index.tsx`:
- Around line 367-377: In onSaveType, guard access to newTop.fieldIndex and
newTop.type.members before assigning: verify newTop.fieldIndex is a valid number
and that newTop.type and Array.isArray(newTop.type.members) are present and
newTop.fieldIndex is within bounds (0 <= fieldIndex < members.length); only then
perform newTop.type.members[newTop.fieldIndex].type = currentTop!.type.name and
update/pop the newStack via setStack; if the guard fails, handle gracefully
(skip the assignment and still adjust stack or log/return) to avoid exceptions
from missing/out-of-range indexes.
In `@workspaces/ballerina/ballerina-visualizer/src/views/TypeDiagram/index.tsx`:
- Around line 127-134: The current replaceTop function drops a fieldIndex of 0
due to a truthy check; update the conditional in replaceTop (operating on
StackItem and setStack) to test explicitly for undefined (e.g., !== undefined or
typeof ... !== 'undefined') when copying newStack[newStack.length -
1].fieldIndex into item.fieldIndex so zero is preserved.
| const onSaveType = (type: Type | string) => { | ||
| handleValueTypeConstChange(typeof type === 'string' ? type : (type as Type).name); | ||
| if (stack.length > 0) { | ||
| if (stack.length > 1) { | ||
| const newStack = [...stack] | ||
| const currentTop = newStack[newStack.length - 1]; | ||
| const newTop = newStack[newStack.length - 2]; | ||
| if (newTop.type.codedata.node === "CLASS") { | ||
| newTop.type.functions[newTop.fieldIndex!].returnType = currentTop!.type.name; | ||
| } | ||
| else { | ||
| newTop.type.members[newTop.fieldIndex!].type = currentTop!.type.name; | ||
| } | ||
| newStack[newStack.length - 2] = newTop; | ||
| newStack.pop(); | ||
| setStack(newStack); | ||
| } | ||
| setRefetchForCurrentModal(true); |
There was a problem hiding this comment.
Guard against missing/out-of-range fieldIndex before indexing.
fieldIndex is optional and currently asserted non-null. If it’s undefined (or out of bounds), this will throw when accessing functions[...] or members[...]. Add a defensive guard to avoid runtime errors.
🧩 Suggested guard
- if (newTop.type.codedata.node === "CLASS") {
- newTop.type.functions[newTop.fieldIndex!].returnType = currentTop!.type.name;
- }
- else {
- newTop.type.members[newTop.fieldIndex!].type = currentTop!.type.name;
- }
+ const fieldIndex = newTop.fieldIndex;
+ if (fieldIndex === undefined) {
+ return;
+ }
+ if (newTop.type.codedata.node === "CLASS") {
+ const fn = newTop.type.functions?.[fieldIndex];
+ if (!fn) { return; }
+ fn.returnType = currentTop!.type.name;
+ } else {
+ const member = newTop.type.members?.[fieldIndex];
+ if (!member) { return; }
+ member.type = currentTop!.type.name;
+ }📝 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 onSaveType = (type: Type | string) => { | |
| handleValueTypeConstChange(typeof type === 'string' ? type : (type as Type).name); | |
| if (stack.length > 0) { | |
| if (stack.length > 1) { | |
| const newStack = [...stack] | |
| const currentTop = newStack[newStack.length - 1]; | |
| const newTop = newStack[newStack.length - 2]; | |
| if (newTop.type.codedata.node === "CLASS") { | |
| newTop.type.functions[newTop.fieldIndex!].returnType = currentTop!.type.name; | |
| } | |
| else { | |
| newTop.type.members[newTop.fieldIndex!].type = currentTop!.type.name; | |
| } | |
| newStack[newStack.length - 2] = newTop; | |
| newStack.pop(); | |
| setStack(newStack); | |
| } | |
| setRefetchForCurrentModal(true); | |
| const onSaveType = (type: Type | string) => { | |
| handleValueTypeConstChange(typeof type === 'string' ? type : (type as Type).name); | |
| if (stack.length > 0) { | |
| if (stack.length > 1) { | |
| const newStack = [...stack] | |
| const currentTop = newStack[newStack.length - 1]; | |
| const newTop = newStack[newStack.length - 2]; | |
| const fieldIndex = newTop.fieldIndex; | |
| if (fieldIndex === undefined) { | |
| return; | |
| } | |
| if (newTop.type.codedata.node === "CLASS") { | |
| const fn = newTop.type.functions?.[fieldIndex]; | |
| if (!fn) { return; } | |
| fn.returnType = currentTop!.type.name; | |
| } else { | |
| const member = newTop.type.members?.[fieldIndex]; | |
| if (!member) { return; } | |
| member.type = currentTop!.type.name; | |
| } | |
| newStack[newStack.length - 2] = newTop; | |
| newStack.pop(); | |
| setStack(newStack); | |
| } | |
| setRefetchForCurrentModal(true); |
🤖 Prompt for AI Agents
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/Forms/FormGenerator/index.tsx`
around lines 1083 - 1100, The onSaveType handler mutates newTop by indexing
newTop.fieldIndex into newTop.type.functions or newTop.type.members without
checking that fieldIndex is defined and within bounds; add a defensive guard in
onSaveType: verify newTop.fieldIndex is a finite number and between 0 and the
length-1 of the appropriate array (newTop.type.functions for CLASS, otherwise
newTop.type.members) before assigning returnType/type, and bail out or skip the
assignment if the guard fails; keep the rest of the flow (updating newStack,
popping, setStack, setRefetchForCurrentModal) unchanged.
| const getNewTypeCreateForm = (fieldIndex?: number, typeName?: string) => { | ||
| const currentTopItem = peekTypeStack(); | ||
| if (currentTopItem) { | ||
| currentTopItem.fieldIndex = fieldIndex; | ||
| replaceTop(currentTopItem); | ||
| } | ||
| pushTypeStack(getDefaultValue(typeName)); |
There was a problem hiding this comment.
Avoid clearing an existing fieldIndex when none is provided.
Because fieldIndex is optional, assigning it unconditionally can overwrite a previously stored index with undefined. Guard the assignment (and avoid mutating state objects in place).
🧩 Proposed fix
- if (currentTopItem) {
- currentTopItem.fieldIndex = fieldIndex;
- replaceTop(currentTopItem);
- }
+ if (currentTopItem && fieldIndex !== undefined) {
+ replaceTop({ ...currentTopItem, fieldIndex });
+ }📝 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 getNewTypeCreateForm = (fieldIndex?: number, typeName?: string) => { | |
| const currentTopItem = peekTypeStack(); | |
| if (currentTopItem) { | |
| currentTopItem.fieldIndex = fieldIndex; | |
| replaceTop(currentTopItem); | |
| } | |
| pushTypeStack(getDefaultValue(typeName)); | |
| const getNewTypeCreateForm = (fieldIndex?: number, typeName?: string) => { | |
| const currentTopItem = peekTypeStack(); | |
| if (currentTopItem && fieldIndex !== undefined) { | |
| replaceTop({ ...currentTopItem, fieldIndex }); | |
| } | |
| pushTypeStack(getDefaultValue(typeName)); |
🤖 Prompt for AI Agents
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/Forms/FormGenerator/index.tsx`
around lines 1308 - 1314, The function getNewTypeCreateForm currently mutates
the object returned by peekTypeStack and unconditionally assigns fieldIndex
which can overwrite an existing index with undefined; instead, when
currentTopItem exists only set fieldIndex if the incoming fieldIndex is not
undefined and avoid in-place mutation—create a shallow copy of currentTopItem,
set copy.fieldIndex = fieldIndex only when fieldIndex !== undefined, then call
replaceTop(copy); leave pushTypeStack(getDefaultValue(typeName)) unchanged.
| const onSaveType = () => { | ||
| if (stack.length > 0) { | ||
| if (stack.length > 1) { | ||
| const newStack = [...stack] | ||
| const currentTop = newStack[newStack.length - 1]; | ||
| const newTop = newStack[newStack.length - 2]; | ||
| newTop.type.members[newTop.fieldIndex!].type = currentTop!.type.name; | ||
| newStack[newStack.length - 2] = newTop; | ||
| newStack.pop(); | ||
| setStack(newStack); | ||
| } |
There was a problem hiding this comment.
Guard fieldIndex/member existence before assignment.
newTop.fieldIndex! and newTop.type.members[...] can throw if the index is missing or out of range (e.g., due to a dropped 0). Add a defensive check before writing.
🛠️ Suggested fix
- newTop.type.members[newTop.fieldIndex!].type = currentTop!.type.name;
+ const fieldIndex = newTop.fieldIndex;
+ if (fieldIndex === undefined || !newTop.type?.members?.[fieldIndex]) {
+ return;
+ }
+ newTop.type.members[fieldIndex].type = currentTop.type.name;🤖 Prompt for AI Agents
In `@workspaces/ballerina/ballerina-visualizer/src/views/GraphQLDiagram/index.tsx`
around lines 367 - 377, In onSaveType, guard access to newTop.fieldIndex and
newTop.type.members before assigning: verify newTop.fieldIndex is a valid number
and that newTop.type and Array.isArray(newTop.type.members) are present and
newTop.fieldIndex is within bounds (0 <= fieldIndex < members.length); only then
perform newTop.type.members[newTop.fieldIndex].type = currentTop!.type.name and
update/pop the newStack via setStack; if the guard fails, handle gracefully
(skip the assignment and still adjust stack or log/return) to avoid exceptions
from missing/out-of-range indexes.
| const replaceTop = (item: StackItem) => { | ||
| if (stack.length <= 1) return; | ||
| if (stack.length === 0) return; | ||
| setStack((prev) => { | ||
| const newStack = [...prev]; | ||
| //preserve fieldIndex if exists | ||
| if (newStack[newStack.length - 1].fieldIndex) { | ||
| item.fieldIndex = newStack[newStack.length - 1].fieldIndex; | ||
| } |
There was a problem hiding this comment.
Preserve fieldIndex when it is 0.
The truthy check drops index 0, which breaks updates for the first field. Use an explicit undefined check.
🛠️ Suggested fix
- if (newStack[newStack.length - 1].fieldIndex) {
- item.fieldIndex = newStack[newStack.length - 1].fieldIndex;
- }
+ const prevFieldIndex = newStack[newStack.length - 1].fieldIndex;
+ if (prevFieldIndex !== undefined) {
+ item.fieldIndex = prevFieldIndex;
+ }📝 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 replaceTop = (item: StackItem) => { | |
| if (stack.length <= 1) return; | |
| if (stack.length === 0) return; | |
| setStack((prev) => { | |
| const newStack = [...prev]; | |
| //preserve fieldIndex if exists | |
| if (newStack[newStack.length - 1].fieldIndex) { | |
| item.fieldIndex = newStack[newStack.length - 1].fieldIndex; | |
| } | |
| const replaceTop = (item: StackItem) => { | |
| if (stack.length === 0) return; | |
| setStack((prev) => { | |
| const newStack = [...prev]; | |
| //preserve fieldIndex if exists | |
| const prevFieldIndex = newStack[newStack.length - 1].fieldIndex; | |
| if (prevFieldIndex !== undefined) { | |
| item.fieldIndex = prevFieldIndex; | |
| } |
🤖 Prompt for AI Agents
In `@workspaces/ballerina/ballerina-visualizer/src/views/TypeDiagram/index.tsx`
around lines 127 - 134, The current replaceTop function drops a fieldIndex of 0
due to a truthy check; update the conditional in replaceTop (operating on
StackItem and setStack) to test explicitly for undefined (e.g., !== undefined or
typeof ... !== 'undefined') when copying newStack[newStack.length -
1].fieldIndex into item.fieldIndex so zero is preserved.
2 participants
Purpose
Recursive type creation was not correctly updating the field type with the newly created type name. When a type was created from within a field (recursive flow), the editor failed to propagate the saved type name back to the originating field, leaving the field type in an inconsistent or default state.
This resulted in broken recursive type definitions and incorrect type graphs.
Resolves: wso2/product-ballerina-integrator#1740
Goals
Approach
fieldIndexwhen initiating recursive type creation.fieldIndexto identify the correct field in the stack.Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.