Fix number input validation is not fully enforced and refactor the configuration objects for the expression editors #1155
Conversation
|
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 WalkthroughCentralizes editor configuration and compatibility checks by removing the secondary Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
workspaces/ballerina/ballerina-side-panel/src/components/editors/ExpressionEditor.tsx (1)
52-52: Remove unused import.
NumberExpressionEditorConfigis imported on line 52 but not used in this file. The configuration is accessed indirectly throughgetEditorConfiguration().-import { NumberExpressionEditorConfig } from './MultiModeExpressionEditor/Configurations';workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/Configurations.tsx (2)
98-102: Add explicit return type annotation.The method is missing an explicit return type annotation. While TypeScript can infer it, adding
: booleanimproves consistency with the base class signature and enhances code documentation.🔎 Apply this diff to add the return type:
- getIsValueCompatible(expValue: string) { + getIsValueCompatible(expValue: string): boolean { const suffix = this.getSerializationSuffix(); const prefix = this.getSerializationPrefix(); return (expValue.trim().startsWith(prefix) && expValue.trim().endsWith(suffix)) }
192-192: Consider refining the decimal regex pattern.The regex
/^\d*\.?\d*$/allows a single dot"."as a valid value, which is not a valid number. While this might be acceptable as an intermediate typing state, it could potentially cause issues if persisted.Consider using a more restrictive pattern:
DECIMAL_INPUT_REGEX = /^(\d+\.?\d*|\d*\.\d+|)$/;This pattern ensures:
- Empty string is allowed (for clearing input)
- At least one digit must be present if a dot is used
- Valid:
"","123","123.45",".5","5."- Invalid:
"."
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
workspaces/ballerina/ballerina-side-panel/src/components/editors/ExpressionEditor.tsx(5 hunks)workspaces/ballerina/ballerina-side-panel/src/components/editors/ExpressionField.tsx(4 hunks)workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/ChipExpressionDefaultConfig.ts(1 hunks)workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/Configurations.tsx(3 hunks)workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/DynamicArrayBuilder/DynamicArrayBuilder.tsx(1 hunks)workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/NumberExpressionEditor/NumberEditor.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-24T22:16:28.380Z
Learnt from: dan-niles
Repo: wso2/vscode-extensions PR: 980
File: workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/RichTextTemplateEditor/RichTextTemplateEditor.tsx:384-412
Timestamp: 2025-11-24T22:16:28.380Z
Learning: In the RichTextTemplateEditor component (workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/RichTextTemplateEditor/RichTextTemplateEditor.tsx), token fetching on external `value` prop changes is intentionally disabled. Users cannot edit both the minimized and expanded editors simultaneously, so tokens only need to be generated based on user typing in the expanded editor view, not on external prop updates.
Applied to files:
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/Configurations.tsxworkspaces/ballerina/ballerina-side-panel/src/components/editors/ExpressionField.tsxworkspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/DynamicArrayBuilder/DynamicArrayBuilder.tsxworkspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/NumberExpressionEditor/NumberEditor.tsxworkspaces/ballerina/ballerina-side-panel/src/components/editors/ExpressionEditor.tsx
📚 Learning: 2025-11-25T06:34:10.812Z
Learnt from: CR
Repo: wso2/vscode-extensions PR: 0
File: workspaces/ballerina/component-diagram/AGENTS.md:0-0
Timestamp: 2025-11-25T06:34:10.812Z
Learning: Applies to workspaces/ballerina/component-diagram/**/*.config.{js,ts} : Use minimatch-compatible glob patterns for file matching in build and test configuration files
Applied to files:
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/Configurations.tsx
📚 Learning: 2025-11-25T06:34:10.812Z
Learnt from: CR
Repo: wso2/vscode-extensions PR: 0
File: workspaces/ballerina/component-diagram/AGENTS.md:0-0
Timestamp: 2025-11-25T06:34:10.812Z
Learning: Applies to workspaces/ballerina/component-diagram/src/**/*.{ts,tsx} : Define all constants (node types, sizing, spacing) in src/resources/constants.ts and import them where needed instead of hardcoding values
Applied to files:
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/Configurations.tsxworkspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/NumberExpressionEditor/NumberEditor.tsx
📚 Learning: 2025-11-24T14:51:49.267Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 998
File: workspaces/ballerina/data-mapper/src/components/DataMapper/Header/ExpressionBar.tsx:113-132
Timestamp: 2025-11-24T14:51:49.267Z
Learning: In workspaces/ballerina/data-mapper/src/components/DataMapper/Header/ExpressionBar.tsx, if `textFieldRef.current` is not undefined, `textFieldRef.current.inputElement` is guaranteed to exist. If `inputElement` doesn't exist when `current` exists, it's a fatal error that should reach the error boundary rather than being handled with defensive null checks.
Applied to files:
workspaces/ballerina/ballerina-side-panel/src/components/editors/ExpressionField.tsxworkspaces/ballerina/ballerina-side-panel/src/components/editors/ExpressionEditor.tsx
🧬 Code graph analysis (3)
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/Configurations.tsx (1)
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/ChipExpressionDefaultConfig.ts (1)
ChipExpressionEditorDefaultConfiguration(22-50)
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/DynamicArrayBuilder/DynamicArrayBuilder.tsx (1)
workspaces/ballerina/ballerina-side-panel/src/components/editors/ExpressionField.tsx (1)
getEditorConfiguration(108-121)
workspaces/ballerina/ballerina-side-panel/src/components/editors/ExpressionEditor.tsx (3)
workspaces/ballerina/ballerina-core/src/utils/form-property-utils.ts (1)
getPrimaryInputType(21-24)workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/utils.ts (1)
getInputModeFromTypes(44-59)workspaces/ballerina/ballerina-side-panel/src/components/editors/ExpressionField.tsx (1)
getEditorConfiguration(108-121)
🔇 Additional comments (11)
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/DynamicArrayBuilder/DynamicArrayBuilder.tsx (1)
101-101: LGTM!The call site correctly updated to match the simplified
getEditorConfiguration(inputMode)signature. This is consistent with the refactored API inExpressionField.tsx.workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/ChipExpressionDefaultConfig.ts (1)
47-49: LGTM!Good addition of the base
getIsValueCompatiblemethod. Returningtrueby default is the correct approach—subclasses likeNumberExpressionEditorConfigcan override with specific validation logic while other configurations inherit permissive behavior.workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/NumberExpressionEditor/NumberEditor.tsx (1)
22-22: LGTM!Good refactor moving the configuration import to the centralized
Configurationsmodule. This improves maintainability and ensures consistent behavior across components using number validation.workspaces/ballerina/ballerina-side-panel/src/components/editors/ExpressionField.tsx (2)
108-121: LGTM!The refactored
getEditorConfigurationfunction is cleaner with the single parameter signature. The switch statement correctly handles TEXT, TEMPLATE, NUMBER, and SQL modes with their respective configurations, while the default case appropriately falls back toChipExpressionEditorDefaultConfigurationfor other modes like EXP.
226-226: Consistent update.Call site correctly updated to use the simplified single-argument signature.
workspaces/ballerina/ballerina-side-panel/src/components/editors/ExpressionEditor.tsx (4)
533-539: Good centralization of the value compatibility check.The
isSwitchToPrimaryModeSafefunction cleanly replaces the previous per-mode safety checks by delegating to the polymorphicgetIsValueCompatiblemethod. This approach is extensible—new modes only need to overridegetIsValueCompatiblein their configuration class.One consideration: for modes not explicitly handled in
getEditorConfiguration(e.g., BOOLEAN, ENUM, RECORD, ARRAY), the defaultChipExpressionEditorDefaultConfigurationis used, which always returnstruefromgetIsValueCompatible. This is likely intentional since those modes don't have text-based validation requirements.
548-552: Correct integration with centralized validation.The mode change handler now properly uses
isSwitchToPrimaryModeSafeto determine if the current value is compatible with the target mode, showing a warning when validation fails.
560-566: Key fix for the PR objective.Adding
InputMode.NUMBERto theshouldClearValuearray ensures that when switching from expression mode to number mode with an incompatible value, the value is reset to an empty string. This directly addresses the linked issue #2130.
449-452: Correct initialization logic.Using
isSwitchToPrimaryModeSafe(field?.value as string)for initial mode determination ensures consistent validation behavior on component mount—if the existing value isn't compatible with the primary mode, it falls back to expression mode.workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/Configurations.tsx (2)
26-26: LGTM: Import statement is correct.The
EditorStateimport is necessary for thechangeFilterimplementation inNumberExpressionEditorConfig.
219-221: LGTM: Validation logic for mode switching is correct.The
getIsValueCompatiblemethod correctly validates the entire value against the regex. This properly enforces the PR requirement to validate values when switching from Expression mode to Number mode.Note: The effectiveness of this validation depends on the regex pattern. See the earlier comment about refining
DECIMAL_INPUT_REGEX.
...ina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/Configurations.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/Configurations.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-24T22:16:28.380Z
Learnt from: dan-niles
Repo: wso2/vscode-extensions PR: 980
File: workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/RichTextTemplateEditor/RichTextTemplateEditor.tsx:384-412
Timestamp: 2025-11-24T22:16:28.380Z
Learning: In the RichTextTemplateEditor component (workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/RichTextTemplateEditor/RichTextTemplateEditor.tsx), token fetching on external `value` prop changes is intentionally disabled. Users cannot edit both the minimized and expanded editors simultaneously, so tokens only need to be generated based on user typing in the expanded editor view, not on external prop updates.
Applied to files:
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/Configurations.tsx
📚 Learning: 2025-11-25T06:34:10.812Z
Learnt from: CR
Repo: wso2/vscode-extensions PR: 0
File: workspaces/ballerina/component-diagram/AGENTS.md:0-0
Timestamp: 2025-11-25T06:34:10.812Z
Learning: Applies to workspaces/ballerina/component-diagram/**/*.config.{js,ts} : Use minimatch-compatible glob patterns for file matching in build and test configuration files
Applied to files:
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/Configurations.tsx
📚 Learning: 2025-11-25T06:34:10.812Z
Learnt from: CR
Repo: wso2/vscode-extensions PR: 0
File: workspaces/ballerina/component-diagram/AGENTS.md:0-0
Timestamp: 2025-11-25T06:34:10.812Z
Learning: Applies to workspaces/ballerina/component-diagram/src/**/*.{ts,tsx} : Define all constants (node types, sizing, spacing) in src/resources/constants.ts and import them where needed instead of hardcoding values
Applied to files:
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/Configurations.tsx
📚 Learning: 2025-11-24T14:51:49.267Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 998
File: workspaces/ballerina/data-mapper/src/components/DataMapper/Header/ExpressionBar.tsx:113-132
Timestamp: 2025-11-24T14:51:49.267Z
Learning: In workspaces/ballerina/data-mapper/src/components/DataMapper/Header/ExpressionBar.tsx, if `textFieldRef.current` is not undefined, `textFieldRef.current.inputElement` is guaranteed to exist. If `inputElement` doesn't exist when `current` exists, it's a fatal error that should reach the error boundary rather than being handled with defensive null checks.
Applied to files:
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/Configurations.tsx
🧬 Code graph analysis (1)
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/Configurations.tsx (1)
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/ChipExpressionDefaultConfig.ts (1)
ChipExpressionEditorDefaultConfiguration(22-50)
🔇 Additional comments (3)
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/Configurations.tsx (3)
26-26: LGTM: Import supports the new changeFilter implementation.The
EditorStateimport is correctly added to enable numeric validation viachangeFilterin the newNumberExpressionEditorConfigclass.
98-102: LGTM: Compatibility check correctly validates template format.The
getIsValueCompatiblemethod properly verifies that expression values conform to the string template format by checking for the required prefix and suffix.
200-212: Excellent fix: Now validates complete resulting document.The changeFilter implementation correctly addresses the critical issue flagged in the previous review. By validating
tr.newDoc.toString()instead of individual inserted fragments, it now properly prevents invalid inputs like "123.." from being accepted.
| } | ||
|
|
||
| export class NumberExpressionEditorConfig extends ChipExpressionEditorDefaultConfiguration { | ||
| DECIMAL_INPUT_REGEX = /^\d*\.?\d*$/; |
There was a problem hiding this comment.
Regex allows invalid decimal input consisting of only a period.
The regex /^\d*\.?\d*$/ accepts "." (a period with no digits) as valid because both \d* quantifiers match empty strings. This means users can enter or switch to a value of just "." which is not a valid decimal number.
Consider requiring at least one digit to be present:
🔎 Apply this diff to require at least one digit:
- DECIMAL_INPUT_REGEX = /^\d*\.?\d*$/;
+ DECIMAL_INPUT_REGEX = /^(\d+\.?\d*|\d*\.\d+)?$/;This pattern ensures that if a decimal point is present, there must be at least one digit either before or after it, while still allowing empty strings and intermediate states like "5." or ".5" during typing.
Also applies to: 214-216
🤖 Prompt for AI Agents
In
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/Configurations.tsx
around line 192 (and also replace the similar occurrences at lines 214-216), the
current DECIMAL_INPUT_REGEX /^\d*\.?\d*$/ allows a lone "." as valid; replace it
with a regex that still permits empty and intermediate typing states but
requires at least one digit when a decimal point is present, e.g. use a pattern
that matches either digits with optional fractional part or a leading dot
followed by digits and allow the whole thing to be empty (for example:
^(?:\d+\.?\d*|\.\d+)?$), and update both occurrences accordingly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
workspaces/ballerina/ballerina-side-panel/src/components/editors/ExpressionEditor.tsx (1)
454-454: Consider validating the type offield?.valuebefore the assertion.The type assertion
field?.value as stringcould be unsafe iffield.valueis not actually a string (e.g., number, boolean, object). While the truthy check inisSwitchToPrimaryModeSafeprovides some protection for falsy values, non-string truthy values could be passed togetIsValueCompatible, which expects a string parameter.🔎 Consider this more type-safe approach:
- if (!newInputMode || !isSwitchToPrimaryModeSafe(field?.value as string)) { + const fieldValue = typeof field?.value === 'string' ? field.value : ''; + if (!newInputMode || !isSwitchToPrimaryModeSafe(fieldValue)) { setInputMode(InputMode.EXP); return; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
workspaces/ballerina/ballerina-side-panel/src/components/editors/ExpressionEditor.tsx(5 hunks)workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/DynamicArrayBuilder/DynamicArrayBuilder.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-24T14:51:49.267Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 998
File: workspaces/ballerina/data-mapper/src/components/DataMapper/Header/ExpressionBar.tsx:113-132
Timestamp: 2025-11-24T14:51:49.267Z
Learning: In workspaces/ballerina/data-mapper/src/components/DataMapper/Header/ExpressionBar.tsx, if `textFieldRef.current` is not undefined, `textFieldRef.current.inputElement` is guaranteed to exist. If `inputElement` doesn't exist when `current` exists, it's a fatal error that should reach the error boundary rather than being handled with defensive null checks.
Applied to files:
workspaces/ballerina/ballerina-side-panel/src/components/editors/ExpressionEditor.tsx
📚 Learning: 2025-11-24T22:16:28.380Z
Learnt from: dan-niles
Repo: wso2/vscode-extensions PR: 980
File: workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/RichTextTemplateEditor/RichTextTemplateEditor.tsx:384-412
Timestamp: 2025-11-24T22:16:28.380Z
Learning: In the RichTextTemplateEditor component (workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/RichTextTemplateEditor/RichTextTemplateEditor.tsx), token fetching on external `value` prop changes is intentionally disabled. Users cannot edit both the minimized and expanded editors simultaneously, so tokens only need to be generated based on user typing in the expanded editor view, not on external prop updates.
Applied to files:
workspaces/ballerina/ballerina-side-panel/src/components/editors/ExpressionEditor.tsxworkspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/DynamicArrayBuilder/DynamicArrayBuilder.tsx
📚 Learning: 2025-11-27T07:59:33.534Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 897
File: workspaces/ballerina/data-mapper/src/components/DataMapper/SidePanel/QueryClauses/ClausesPanel.tsx:50-59
Timestamp: 2025-11-27T07:59:33.534Z
Learning: In workspaces/ballerina/data-mapper/src/components/DataMapper/SidePanel/QueryClauses/ClausesPanel.tsx, the `clause.properties.expression` property in the `fillDefaults` function does not require defensive null/undefined checks because it's a required property enforced by form validation in ClauseEditor.
Applied to files:
workspaces/ballerina/ballerina-side-panel/src/components/editors/ExpressionEditor.tsx
🧬 Code graph analysis (2)
workspaces/ballerina/ballerina-side-panel/src/components/editors/ExpressionEditor.tsx (3)
workspaces/ballerina/ballerina-core/src/utils/form-property-utils.ts (1)
getPrimaryInputType(21-24)workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/utils.ts (1)
getInputModeFromTypes(44-59)workspaces/ballerina/ballerina-side-panel/src/components/editors/ExpressionField.tsx (1)
getEditorConfiguration(108-121)
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/DynamicArrayBuilder/DynamicArrayBuilder.tsx (1)
workspaces/ballerina/ballerina-side-panel/src/components/editors/ExpressionField.tsx (1)
getEditorConfiguration(108-121)
🔇 Additional comments (4)
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/DynamicArrayBuilder/DynamicArrayBuilder.tsx (1)
101-101: LGTM! Correctly aligned with refactored API signature.The change correctly updates the function call to match the new single-parameter signature of
getEditorConfiguration(inputMode). This aligns with the PR's objective to centralize editor configuration logic and remove duplication.workspaces/ballerina/ballerina-side-panel/src/components/editors/ExpressionEditor.tsx (3)
546-559: LGTM - Centralized safety check addresses the validation bypass issue.The refactored mode change handler now uses the centralized
isSwitchToPrimaryModeSafecheck to validate whether switching modes is safe. This properly addresses the PR objective by preventing invalid expression values from bypassing number validation when switching to number mode. The warning popup mechanism gives users clear control over the mode switch.
565-571: LGTM - Adding NUMBER mode ensures value clearing on mode switch.Adding
InputMode.NUMBERto theshouldClearValuearray ensures that when users switch from expression mode to number mode (after confirming the warning), the value is properly reset to an empty string. This completes the validation enforcement by giving users a clean slate in number mode, preventing invalid expression values from persisting.
538-544: All editor configurations implementgetIsValueCompatible, including the baseChipExpressionEditorDefaultConfigurationwhich provides a default implementation returningtrue. The switch statement ingetEditorConfigurationcovers allInputModecases (TEXT, TEMPLATE, NUMBER, SQL) and includes a default fallback to the base configuration. No runtime error will occur.
…into fix-editor-minor-bugs
…vscode-extensions-public into fix-editor-minor-bugs
2 participants
Purpose
The expression editor allows users to enter values in expression mode and later switch to number mode. In this flow, numeric validation was bypassed, allowing invalid values to get passed in to the number mode without being revalidated. This resulted in inconsistent validation behavior and potential runtime issues when invalid values were saved.
This PR addresses the validation gap and improves the maintainability of the editor configuration logic.
Resolves: wso2/product-ballerina-integrator#2130
Goals
Approach
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.