Update package configurable view and enhance state machine actions #924
Conversation
Fix E2E tests
…project path handling
…anization and package names
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ 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 |
axewilledge
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the Ballerina package configurable view and improves state machine actions for better org/package tracking. The changes focus on fixing the state management for organization and package information across views, adding loading states, and improving error handling.
- Added proper org/package name tracking through state machine transitions
- Introduced loading indicators for configurable variables view
- Enhanced error handling in DMC file formatting
- Added string value wrapping logic for configurable variables
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| workspaces/mi/mi-extension/src/rpc-managers/mi-data-mapper/rpc-manager.ts | Added error handling for DMC file formatting operations |
| workspaces/ballerina/ballerina-visualizer/src/views/BI/Configurables/ViewConfigurableVariables/index.tsx | Added loading state management and refined useEffect dependencies for org/package changes |
| workspaces/ballerina/ballerina-visualizer/src/views/BI/Configurables/ConfigurableItem/index.tsx | Implemented string value wrapping/unwrapping logic for configurable variables |
| workspaces/ballerina/ballerina-visualizer/src/views/BI/Configurables/ConfigurableItem/ConfigObjectEditor.tsx | Added string value quote wrapping for parsed configuration objects |
| workspaces/ballerina/ballerina-extension/src/utils/config.ts | Refactored getPackageName to getOrgAndPackageName to return both org and package information |
| workspaces/ballerina/ballerina-extension/src/stateMachine.ts | Enhanced state machine to track and assign org/package through state transitions and view operations |
| workspaces/ballerina/ballerina-core/src/interfaces/extended-lang-client.ts | Added org field to ProjectInfo interface to support org tracking alongside orgName |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| const getPlainValue = (value: string) => { | ||
| if (configVariable.properties?.type?.value === 'string' && /^".*"$/.test(value)) { | ||
| return value.replace(/^"|"$/g, ''); | ||
| } | ||
| return value; | ||
| } |
There was a problem hiding this comment.
Missing semicolons after function declarations. While JavaScript/TypeScript allows this, it's inconsistent with the coding style used elsewhere in the file where semicolons are present after other statements. Add semicolons at the end of both function declarations for consistency.
| } | |
| const getPlainValue = (value: string) => { | |
| if (configVariable.properties?.type?.value === 'string' && /^".*"$/.test(value)) { | |
| return value.replace(/^"|"$/g, ''); | |
| } | |
| return value; | |
| } | |
| }; | |
| const getPlainValue = (value: string) => { | |
| if (configVariable.properties?.type?.value === 'string' && /^".*"$/.test(value)) { | |
| return value.replace(/^"|"$/g, ''); | |
| } | |
| return value; | |
| }; |
|
|
||
| const handleTextAreaChange = (value: any) => { | ||
| if (configVariable.properties?.type?.value === 'string' && !/^".*"$/.test(value)) { | ||
| value = `"${value}"`; |
There was a problem hiding this comment.
The string wrapping logic doesn't handle strings that already contain internal quotes or need escaping. For example:
he said "hello"→"he said "hello""(produces invalid JSON/string literal)- Values with newlines or special characters may not be properly escaped
Consider escaping internal quotes before wrapping, or validate that the value doesn't contain unescaped quotes:
if (configVariable.properties?.type?.value === 'string' && !/^".*"$/.test(value)) {
// Escape internal quotes
value = `"${value.replace(/"/g, '\\"')}"`;
}| value = `"${value}"`; | |
| value = JSON.stringify(value); |
| // Check if there are string values that are not wrapped in quotes | ||
| Object.keys(parsedValue).forEach(key => { | ||
| if (typeof parsedValue[key] === 'string' && !/^".*"$/.test(parsedValue[key])) { | ||
| parsedValue[key] = `"${parsedValue[key]}"`; |
There was a problem hiding this comment.
Same issue as in ConfigurableItem/index.tsx: The string wrapping logic doesn't escape internal quotes. When wrapping string values that contain quotes, this will produce invalid JSON. Consider escaping quotes before wrapping:
if (typeof parsedValue[key] === 'string' && !/^".*"$/.test(parsedValue[key])) {
parsedValue[key] = `"${parsedValue[key].replace(/"/g, '\\"')}"`;
}| parsedValue[key] = `"${parsedValue[key]}"`; | |
| parsedValue[key] = JSON.stringify(parsedValue[key]); |
| useEffect(() => { | ||
| getConfigVariables(); | ||
| }, [props]); | ||
| setIsLoading(true); | ||
| getConfigVariables(true); | ||
| }, [props.org, props.package]); |
There was a problem hiding this comment.
The useEffect hook calls getConfigVariables(true) but doesn't include it in the dependency array. This can lead to stale closures where the function captures old values of state variables like rpcClient, selectedModule, etc.
Consider either:
- Wrapping
getConfigVariableswithuseCallbackand adding it to the dependency array - Disabling the ESLint rule with a comment if the omission is intentional
- Refactoring to avoid the dependency by inlining the logic
| } | ||
| extension.hasPullModuleResolved = false; | ||
| extension.hasPullModuleNotification = false; | ||
| const { orgName, packageName } = getOrgAndPackageName(StateMachine.context().projectInfo, viewLocation.projectPath); |
There was a problem hiding this comment.
Potential issue: The getOrgAndPackageName function is called with viewLocation.projectPath which might be undefined. While the function handles this by returning empty strings, this could lead to incorrect org/package information being set in the view location. Consider validating that viewLocation.projectPath exists before calling this function, or provide a fallback to StateMachine.context().projectPath.
| const { orgName, packageName } = getOrgAndPackageName(StateMachine.context().projectInfo, viewLocation.projectPath); | |
| const { orgName, packageName } = getOrgAndPackageName( | |
| StateMachine.context().projectInfo, | |
| viewLocation.projectPath || StateMachine.context().projectPath | |
| ); |
| } catch (error) { | ||
| console.error("Error occurred while formatting DMC file: ", error); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Potential issue: If edits is null or undefined (which can happen if no formatter is available for the document type), calling workspaceEdit.set(uri, edits) at line 181 may cause an error. Add a null check after the try-catch block:
if (!edits) {
console.warn("No formatting edits returned for DMC file");
return;
}| } | |
| } | |
| if (!edits) { | |
| console.warn("No formatting edits returned for DMC file"); | |
| return; | |
| } |
| data = (variables as any).configVariables; | ||
| errorMsg = (variables as any).errorMsg; | ||
| }); | ||
| }) |
There was a problem hiding this comment.
Avoid automated semicolon insertion (90% of all statements in the enclosing function have an explicit semicolon).
| }) | |
| }); |
4 participants
Purpose
Goals
Approach
UI Component Development
npm run storybookfrom the root directory to view current components.Manage Icons
User stories
Release note
Documentation
Training
Certification
Marketing
Automation tests
Security checks
Samples
Related PRs
Migrations (if applicable)
Test environment
Learning