Skip to content

Revert "Fix datamapper button not appearing" #1895

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
KCSAbeywickrama merged 1 commit into from
Apr 3, 2026
+20 −34
Merged

Revert "Fix datamapper button not appearing" #1895

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions workspaces/ballerina/ballerina-side-panel/src/components/Form/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,7 @@ export interface FormProps {
formDiagnostics?: { message: string; severity: "ERROR" | "WARNING" | "INFO" }[];
formDiagnosticsAction?: React.ReactNode;
preserveOrder?: boolean;
handleSelectedTypeChange?: (type: string | CompletionItem) => void | Promise<void>;
handleSelectedTypeChange?: (type: string | CompletionItem) => void;
scopeFieldAddon?: React.ReactNode;
onChange?: (fieldKey: string, value: any, allValues: FormValues) => void;
injectedComponents?: {
Expand Down Expand Up @@ -664,10 +664,12 @@ export const Form = forwardRef((props: FormProps, _ref) => {
openSubPanel(updatedSubPanel);
};

const handleOnTypeChange = () => {
getVisualiableFields();
};

const handleNewTypeSelected = (type: string | CompletionItem) => {
Promise.resolve(handleSelectedTypeChange?.(type)).catch((error) => {
console.error("Error in handleSelectedTypeChange", error);
});
handleSelectedTypeChange && handleSelectedTypeChange(type);
}

const getVisualiableFields = () => {
Expand Down Expand Up @@ -1102,6 +1104,7 @@ export const Form = forwardRef((props: FormProps, _ref) => {
autoFocus={firstEditableFieldIndex === formFields.indexOf(updatedField) && !hideSaveButton}
recordTypeFields={recordTypeFields}
onIdentifierEditingStateChange={handleIdentifierEditingStateChange}
handleOnTypeChange={handleOnTypeChange}
setSubComponentEnabled={setIsSubComponentEnabled}
handleNewTypeSelected={handleNewTypeSelected}
onBlur={handleOnBlur}
Expand Down Expand Up @@ -1202,6 +1205,7 @@ export const Form = forwardRef((props: FormProps, _ref) => {
handleOnFieldFocus={handleOnFieldFocus}
recordTypeFields={recordTypeFields}
onIdentifierEditingStateChange={handleIdentifierEditingStateChange}
handleOnTypeChange={handleOnTypeChange}
onBlur={handleOnBlur}
/>
</S.Row>
Expand Down Expand Up @@ -1241,6 +1245,7 @@ export const Form = forwardRef((props: FormProps, _ref) => {
handleOnFieldFocus={handleOnFieldFocus}
recordTypeFields={recordTypeFields}
onIdentifierEditingStateChange={handleIdentifierEditingStateChange}
handleOnTypeChange={handleOnTypeChange}
onBlur={handleOnBlur}
handleFormValidation={handleFormValidation}
/>
Expand Down Expand Up @@ -1270,6 +1275,7 @@ export const Form = forwardRef((props: FormProps, _ref) => {
((open: boolean, newType?: string | NodeProperties) => handleOpenRecordEditor(open, typeField, newType))
}
handleOnFieldFocus={handleOnFieldFocus}
handleOnTypeChange={handleOnTypeChange}
recordTypeFields={recordTypeFields}
onIdentifierEditingStateChange={handleIdentifierEditingStateChange}
handleNewTypeSelected={handleNewTypeSelected}
Expand All @@ -1285,6 +1291,7 @@ export const Form = forwardRef((props: FormProps, _ref) => {
recordTypeFields={recordTypeFields}
onIdentifierEditingStateChange={handleIdentifierEditingStateChange}
handleNewTypeSelected={handleNewTypeSelected}
handleOnTypeChange={handleOnTypeChange}
onBlur={handleOnBlur}
handleFormValidation={handleFormValidation}
/>
Expand Down
4 changes: 1 addition & 3 deletions workspaces/ballerina/ballerina-visualizer/src/views/BI/Forms/DeclareVariableForm/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,7 @@ export const VariableForm = (props: FormProps) => {
}, [props.formFields]);

const handleOnTypeChange = (type: string | CompletionItem) => {
Promise.resolve(handleSelectedTypeChange?.(type)).catch((error) => {
console.error("Error in handleSelectedTypeChange", error);
});
handleSelectedTypeChange(type);
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

handleSelectedTypeChange is optional on FormProps, but it’s being called unconditionally here. If a caller doesn’t provide it, this will throw at runtime when the user changes the type. Guard with optional chaining (or a default no-op) similar to the previous implementation, and keep error handling if the callback can fail.

Suggested change
handleSelectedTypeChange(type);
handleSelectedTypeChange?.(type);

Copilot uses AI. Check for mistakes.
};
Comment on lines 35 to 37
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Missing null check after removing optional chaining.

The handleSelectedTypeChange prop is optional (per FormProps), but the revert removed the optional chaining safeguard (?.). If this prop is ever undefined, the call will throw a TypeError.

While FormGenerator always passes this prop when rendering VariableForm, the component interface still allows it to be omitted. Consider adding a guard:

🛡️ Proposed fix to restore defensive check 
 const handleOnTypeChange = (type: string | CompletionItem) => {
-    handleSelectedTypeChange(type);
+    handleSelectedTypeChange?.(type);
 };
📝 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
const handleOnTypeChange = (type: string | CompletionItem) => {
Promise.resolve(handleSelectedTypeChange?.(type)).catch((error) => {
console.error("Error in handleSelectedTypeChange", error);
});
handleSelectedTypeChange(type);
};
const handleOnTypeChange = (type: string | CompletionItem) => {
handleSelectedTypeChange?.(type);
};
🤖 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/DeclareVariableForm/index.tsx`
around lines 35 - 37, The call to handleSelectedTypeChange inside
handleOnTypeChange can throw because handleSelectedTypeChange is optional in
FormProps; add a defensive null-check or optional chaining before invoking it
(e.g., if (handleSelectedTypeChange) handleSelectedTypeChange(type) or
handleSelectedTypeChange?.(type)) so handleOnTypeChange safely handles the case
when handleSelectedTypeChange is undefined; update the handleOnTypeChange
function to perform this guard using the existing parameter names.

return (
<>
Expand Down
35 changes: 8 additions & 27 deletions workspaces/ballerina/ballerina-visualizer/src/views/BI/Forms/FormGenerator/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -522,7 +522,7 @@ export const FormGenerator = forwardRef<FormExpressionEditorRef, FormProps>(func
const updatedField = { ...field };

const isRepeatableList = field.types?.length === 1 && getPrimaryInputType(field.types)?.fieldType === "REPEATABLE_LIST";
const selectedInputType = isRepeatableList ? getPrimaryInputType(field.types) : field.types?.find(t => t.selected);
const selectedInputType = isRepeatableList? getPrimaryInputType(field.types) : field.types?.find(t => t.selected);

const nodeProperties = nodeWithDiagnostics?.properties as any;
let propertyDiagnostics: any = nodeProperties?.[field.key]?.diagnostics?.diagnostics;
Expand Down Expand Up @@ -922,7 +922,7 @@ export const FormGenerator = forwardRef<FormExpressionEditorRef, FormProps>(func

return Object.values(nodeProperties).some((property) => {
let diagnostics: DiagnosticMessage[] = [];
if (property?.types?.length === 1 && getPrimaryInputType(property.types)?.fieldType === "REPEATABLE_LIST") {
if ( property?.types?.length === 1 && getPrimaryInputType(property.types)?.fieldType === "REPEATABLE_LIST") {
// For repeatable list, check diagnostics for each element in the list
const valueDiagnostics = (property.value as any[])?.map((val) => val?.diagnostics?.diagnostics ?? []).flat() ?? [];
diagnostics = [...diagnostics, ...valueDiagnostics];
Expand Down Expand Up @@ -1399,24 +1399,13 @@ export const FormGenerator = forwardRef<FormExpressionEditorRef, FormProps>(func
/**
* Handles type selection from completion items (used in type editor)
*/
const handleSelectedTypeChange = async (type: CompletionItem | string) => {
try {
if (typeof type === "string") {
await handleSelectedTypeByName(type);
return;
}
else {
// If the type is a Completion item, then it can be found in the reference types.
// Which cannot be an imported type.
importsCodedataRef.current = null;
await fetchVisualizableFields(fileName, (type as CompletionItem).label);
}
setSelectedType(type);
updateRecordTypeFields(type);
}
catch (error) {
console.error("Error handling selected type change", error);
const handleSelectedTypeChange = (type: CompletionItem | string) => {
if (typeof type === "string") {
handleSelectedTypeByName(type);
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

handleSelectedTypeByName is async, but handleSelectedTypeChange now calls it without awaiting or handling rejections. This can lead to unhandled promise rejections (and out-of-order state updates) when type resolution fails or the RPC call throws. Consider making handleSelectedTypeChange async and awaiting the call, or explicitly catching/logging the promise.

Suggested change
handleSelectedTypeByName(type);
void handleSelectedTypeByName(type).catch((error) => {
console.error("Failed to handle selected type by name:", error);
});

Copilot uses AI. Check for mistakes.
return;
}
setSelectedType(type);
updateRecordTypeFields(type);
};

const findMatchedType = (items: TypeHelperItem[], typeName: string) => {
Expand Down Expand Up @@ -1474,23 +1463,15 @@ export const FormGenerator = forwardRef<FormExpressionEditorRef, FormProps>(func
const handleSelectedTypeByName = async (typeName: string) => {
// Early return for invalid input
if (!typeName || typeName.length === 0) {
importsCodedataRef.current = null;
await fetchVisualizableFields(fileName, typeName);
setValueTypeConstraints('');
return;
}

const type = await searchImportedTypeByName(typeName);
if (!type) {
importsCodedataRef.current = null;
await fetchVisualizableFields(fileName, typeName);
setValueTypeConstraints('');
return;
}
Comment on lines 1463 to 1474
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When typeName is empty/invalid or the type can’t be found, this function now returns without updating/clearing visualizableField (and without resetting importsCodedataRef). Since visualizableField is used to decide whether the Data Mapper button is enabled and to provide default values, it can become stale after a type is cleared/changed. Consider explicitly resetting visualizableField (and importsCodedataRef.current) on these early-return paths, or triggering a refresh that results in a cleared state.

Copilot uses AI. Check for mistakes.
else {
importsCodedataRef.current = type.codedata;
await fetchVisualizableFields(fileName, typeName);
}

setValueTypeConstraints(type.insertText);
// Create the record type field for expression
Expand Down
Loading