Expression editor bug fixes and maintainability improvement #906
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 WalkthroughReplaces the legacy chip-based expression editor with a new CodeMirror 6–backed implementation: removes Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CM as ChipExpressionEditor (CodeMirror)
participant Utils as CodeUtils
participant RPC as Expression RPC
participant Helper as HelperPane
User->>CM: Type / navigate
activate CM
CM->>Utils: onChange listener -> compute value & cursor
alt tokens stale
CM->>RPC: fetch tokens (value, fileName, startLine)
RPC-->>CM: token stream
CM->>Utils: update tokenField & decorations
end
CM->>User: render updated document + chips
deactivate CM
Note over User,Helper: helper / completion flow
User->>CM: request helper/completion
CM->>Utils: compute helper anchor (cursor coords)
CM->>Helper: open at coords with getHelperPane
Helper->>CM: onChange (selected item) -> insert/replace text
CM->>CM: update value, tokens, and selection
Helper-->>User: close/update
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas to focus:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
senithkay
changed the title
![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 (1)
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/utils.ts (1)
947-1004: Arrow‑right caret move usesARROW_LEFT_MARKERIn
moveCaretForward, the change marker passed toonExpressionChangeisARROW_LEFT_MARKER:const newCursorPosition = getAbsoluteCaretPositionFromModel(newExpressionModel); onExpressionChange?.(newExpressionModel, newCursorPosition, ARROW_LEFT_MARKER);This is called from the ArrowRight handler, while
moveToNextElementusesARROW_RIGHT_MARKER. If callers distinguish left vs right markers, this will report the wrong origin.Switch this to
ARROW_RIGHT_MARKERfor consistency.- const newCursorPosition = getAbsoluteCaretPositionFromModel(newExpressionModel); - onExpressionChange?.(newExpressionModel, newCursorPosition, ARROW_LEFT_MARKER); + const newCursorPosition = getAbsoluteCaretPositionFromModel(newExpressionModel); + onExpressionChange?.(newExpressionModel, newCursorPosition, ARROW_RIGHT_MARKER);
🧹 Nitpick comments (6)
workspaces/ballerina/ballerina-side-panel/src/components/editors/ExpandedEditor/modes/ExpressionMode.tsx (1)
22-47: ExpressionMode correctly switches to ChipExpressionBaseComponent2; consider simplifying onChangeUsing
ChipExpressionBaseComponent2withisInExpandedMode={true}andisExpandedVersion={true}looks aligned with the new base component’s API, and forwarding the cursor position throughhandleChangekeeps upstream consumers informed.If
EditorModeExpressionProps["onChange"]already has the(value: string, cursorPosition: number) => voidsignature, you can simplify by passingonChangedirectly instead of wrapping it inhandleChange.Also applies to: 51-61
workspaces/ballerina/ballerina-side-panel/src/components/editors/ExpressionField.tsx (1)
33-34: ChipExpressionBaseComponent2 wiring looks correct; consider cleaning up legacy helper-pane propsThe switch to
ChipExpressionBaseComponent2withisExpandedVersion={false}and forwardingonChange,value,fileName,targetLineRange,extractArgsFromFunction, andonOpenExpandedModeis consistent with the new base component API.However,
ExpressionFieldstill defines helper-pane–related props (isHelperPaneOpen,changeHelperPaneState,helperPaneHeight,helperPaneWidth,helperPaneZIndex,onToggleHelperPane) that are no longer used in the chip path. If there’s no remaining external control of the helper pane, consider:
- Removing these props from
ExpressionField(and call sites), or- Rewiring them into the new helper-pane implementation if external control is still required.
This will reduce confusion and keep the API surface aligned with the actual behavior.
Also applies to: 152-165
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/ChipExpressionEditor.tsx (1)
82-89: Overall ChipExpressionBaseComponent2 structure is solid; a couple of small robustness tweaksThe overall structure around
EditorState/EditorView, token refetch scheduling, and helper-pane anchoring looks good and matches the intended behavior described in the PR.Two minor robustness/readability tweaks you might consider:
- Empty-string handling in token refetch effect
useEffect(() => { if (!props.value || !viewRef.current) return; // ... }, [props.value, props.fileName, props.targetLineRange?.startLine, isTokenUpdateScheduled]);Using
!props.valuemeans the token update logic is skipped when the expression is the empty string, which can leave stale tokens/chip decorations when a user clears the field. If you want clearing the expression to also clear tokens, drop that check and rely on the RPC returning an empty stream, or explicitly dispatch an empty token list whenprops.value === "".
- Type the
viewReffor better safetyconst viewRef = useRef(null);can be tightened to:
const viewRef = useRef<EditorView | null>(null);to avoid
anycreeping in and to make future refactors safer.These are not blockers but would make the component more predictable and easier to maintain.
Also applies to: 195-207, 260-265, 286-327
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/utils.ts (2)
245-255: Align token type unions with newTokenTypeto avoid unsafe casts
getTokenTypeFromIndexnow returnsTokenType = 'variable' | 'property' | 'parameter', butcreateExpressionModelFromTokensstill casts that to'variable' | 'function' | 'literal':const tokenType = getTokenTypeFromIndex(tokenTypeIndex); expressionModel.push({ ... type: tokenType as 'variable' | 'function' | 'literal' });This cast is now misleading: at runtime you can have
'property'/'parameter', which are outside the declared union and can break type-based narrowing elsewhere.Consider updating
ExpressionModel["type"](in./types) and this assignment so the unions line up, e.g. something along the lines of:type: tokenType as TokenType | 'literal'or, ideally, give
ExpressionModel.typethat union directly so no cast is needed.Also applies to: 313-320
1212-1264: Parsed tokens and completion filtering: good structure, but tightly coupled toCompletionItem.kindThe new parsed‑token helpers and completion filter look coherent:
getParsedExpressionTokensreuses the same token chunk shape andgetTokenTypeFromIndex, which keeps token positions/types aligned.getWordBeforeCursorPositionis a reasonable utility for prefix extraction.filterCompletionsByPrefixAndTypeenforces:
- with empty prefix: only
kind === 'field'- with non‑empty prefix: only
kindin'function' | 'variable' | 'field'and labelstartsWith(prefix)(case‑insensitive).A couple of points to double‑check:
- Ensure
CompletionItem.kindin@wso2/ui-toolkitis guaranteed to use exactly'function' | 'variable' | 'field'(and that you don’t need to surface other kinds like'parameter'or'keyword'), otherwise completions will silently be dropped.- If
CompletionItem.valuecan be undefined, you may want to fall back tolabelforapplyin the CodeMirror completion mapping to avoid inserting"undefined".These are behavioral choices, but worth confirming against the actual
CompletionItemcontract and UX expectations.workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/CodeUtils.ts (1)
345-378: Completion source wiring is good; consider tightening mapping and contractsThe completion source integrates nicely with
filterCompletionsByPrefixAndTypeand CodeMirror’sCompletionContext:
- Uses
matchBefore(/\w*/)with explicit triggering rules.- Suppresses completions after
'+'and':', matching the intended UX.- Maps
CompletionIteminto CM6 completion objects.Minor suggestions:
- If
CompletionItem.valueis optional, considerapply: item.value ?? item.labelto avoid inserting"undefined"in edge cases.- Confirm that the
typestring passed to CM (item.kind || "variable") aligns with the allowed CM completiontypevalues and with whatCompletionItem.kindproduces.These are small guardrails but help keep the integration resilient to upstream type/shape changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
common/config/rush/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
workspaces/ballerina/ballerina-side-panel/package.json(1 hunks)workspaces/ballerina/ballerina-side-panel/src/components/editors/ExpandedEditor/modes/ExpressionMode.tsx(3 hunks)workspaces/ballerina/ballerina-side-panel/src/components/editors/ExpressionField.tsx(2 hunks)workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/ChipExpressionBaseComponent.tsx(0 hunks)workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/CodeUtils.ts(1 hunks)workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/ChipExpressionEditor.tsx(1 hunks)workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/HelperPane.tsx(1 hunks)workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/HelperPaneToggleButton.tsx(1 hunks)workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/styles.tsx(1 hunks)workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/utils.ts(8 hunks)
💤 Files with no reviewable changes (1)
- workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/ChipExpressionBaseComponent.tsx
🧰 Additional context used
🧬 Code graph analysis (7)
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/ChipExpressionEditor.tsx (4)
workspaces/ballerina/ballerina-side-panel/src/components/Form/types.ts (1)
HelperpaneOnChangeOptions(95-98)workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/CodeUtils.ts (11)
buildNeedTokenRefetchListner(287-306)buildOnChangeListner(308-343)buildOnFocusListner(244-275)CursorInfo(28-32)buildOnFocusOutListner(277-285)buildCompletionSource(345-378)buildHelperPaneKeymap(380-391)expressionEditorKeymap(216-240)chipPlugin(184-214)tokenField(158-182)tokensChangeEffect(155-155)workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/HelperPaneToggleButton.tsx (1)
HelperPaneToggleButton(27-60)workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/styles.tsx (2)
ChipEditorContainer(45-52)FloatingButtonContainer(224-231)
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/HelperPaneToggleButton.tsx (1)
workspaces/common-libs/ui-toolkit/src/components/Codicon/Codicon.tsx (1)
Codicon(44-56)
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/HelperPane.tsx (2)
workspaces/ballerina/ballerina-side-panel/src/components/Form/types.ts (1)
HelperpaneOnChangeOptions(95-98)workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/styles.tsx (1)
ContextMenuContainer(83-94)
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/CodeUtils.ts (2)
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/utils.ts (3)
ParsedToken(1212-1217)getParsedExpressionTokens(1219-1245)filterCompletionsByPrefixAndType(1253-1264)workspaces/common-libs/ui-toolkit/src/components/ExpressionEditor/constants/form.ts (1)
HELPER_PANE_WIDTH(21-21)
workspaces/ballerina/ballerina-side-panel/src/components/editors/ExpandedEditor/modes/ExpressionMode.tsx (1)
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/ChipExpressionEditor.tsx (1)
ChipExpressionBaseComponent2(79-330)
workspaces/ballerina/ballerina-side-panel/src/components/editors/ExpressionField.tsx (1)
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/ChipExpressionEditor.tsx (1)
ChipExpressionBaseComponent2(79-330)
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/utils.ts (2)
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/TokenizedExpression.tsx (1)
getTokenTypeFromIndex(25-32)workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/CodeUtils.ts (1)
TokenType(34-34)
🔇 Additional comments (6)
workspaces/ballerina/ballerina-side-panel/package.json (1)
31-35: CodeMirror and markdown toolbar dependency additions look consistentThe newly added
@codemirror/*and@github/markdown-toolbar-elementdeps fit the new editor usage. Just make sure:
- The extension bundler config is compatible with these ESM packages.
- We don’t accidentally pull in duplicate versions of CodeMirror across workspaces.
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/styles.tsx (1)
83-94: Context menu z‑index bump is reasonableRaising
ContextMenuContainertoz-index: 1600keeps it above the skeleton loader and floating buttons but still below completions/description overlays. That ordering makes sense for the new helper/completion UX.workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/HelperPane.tsx (1)
24-52: HelperPane wrapper is well-factored and matches intended usageProps, ref forwarding, and mouse event handling look correct for positioning and keeping focus within the editor. Fixed
"3/4"height is fine for now and can be parameterized later if needed.workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/CodeUtils.ts (3)
36-99: Chip widget implementation and selection behavior look solid
createChipandChipWidgetcorrectly render inline tokens, style them byTokenType, and dispatch a selection on click. UsingDecoration.replacewithinclusive: falsematches the intended chip behavior for CM6.
155-182: Token field update strategy aligns tokens with document changes
tokenField’s use oftr.changes.mapPosto remap existing token ranges, coupled with recomputing fromtokensChangeEffectand filtering viaremoveChipEffect, is a good pattern for keeping token decorations in sync with document edits and explicit token refreshes.Also applies to: 184-214
380-391: Escape key handling for helper pane is straightforward and appropriate
buildHelperPaneKeymapcleanly scopes Escape handling to when the helper pane is actually open and otherwise lets the default behavior proceed. This fits well with the editor’s keymap layering.
| export const buildOnFocusListner = (onTrigger: (cursor: CursorInfo) => void) => { | ||
| const shouldOpenHelperPaneListner = EditorView.updateListener.of((update) => { | ||
| if (update.focusChanged) { | ||
| if (!update.view.hasFocus) { | ||
| return; | ||
| } | ||
|
|
||
| const cursorPosition = update.view.state.selection.main; | ||
| const coords = update.view.coordsAtPos(cursorPosition.to); | ||
|
|
||
| if (coords && coords.top && coords.left) { | ||
| const editorRect = update.view.dom.getBoundingClientRect(); | ||
| //+5 is to position a little be below the cursor | ||
| //otherwise it overlaps with the cursor | ||
| let relativeTop = coords.bottom - editorRect.top + 5; | ||
| let relativeLeft = coords.left - editorRect.left; | ||
|
|
||
| const HELPER_PANE_WIDTH = 300; | ||
| const viewportWidth = window.innerWidth; | ||
| const absoluteLeft = coords.left; | ||
| const overflow = absoluteLeft + HELPER_PANE_WIDTH - viewportWidth; | ||
|
|
||
| if (overflow > 0) { | ||
| relativeLeft -= overflow; | ||
| } | ||
|
|
||
| onTrigger({ top: relativeTop, left: relativeLeft, position: cursorPosition }); | ||
| } | ||
| } | ||
| }); | ||
| return shouldOpenHelperPaneListner; | ||
| }; |
There was a problem hiding this comment.
Cursor coordinate checks and helper pane width handling could be more robust
Two small issues in the helper‑pane positioning helpers:
- In
buildOnFocusListneryou guard with truthiness checks:
if (coords && coords.top && coords.left) {
...
}This will skip valid positions when coords.top or .left is 0. Prefer explicit null/undefined checks, as used later in buildOnChangeListner:
if (coords && coords.top !== null && coords.left !== null) {
...
}- Both
buildOnFocusListnerandbuildOnChangeListnerhard‑codeHELPER_PANE_WIDTH = 300, while the shared constant in the UI toolkit is350. Divergence here will cause misalignment near the right viewport edge and makes future width changes error‑prone.
It would be safer to source the width from a single shared constant or pass it in as a parameter instead of duplicating the literal.
Also applies to: 308-343
🤖 Prompt for AI Agents
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/CodeUtils.ts
lines 244-275 (also apply same change to 308-343): change the truthy checks for
cursor coordinates to explicit null/undefined checks (e.g., coords && coords.top
!== null && coords.left !== null) so zero values are accepted, and replace the
hard-coded HELPER_PANE_WIDTH = 300 with a single source of truth (import the
shared UI toolkit constant or accept the width as a parameter) and use that
value for overflow calculation to keep positioning consistent with the rest of
the app.
...s/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/ChipExpressionEditor.tsx
Outdated
Show resolved
Hide resolved
...s/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/ChipExpressionEditor.tsx
Show resolved
Hide resolved
...editors/MultiModeExpressionEditor/ChipExpressionEditor/components/HelperPaneToggleButton.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
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-side-panel/src/components/editors/ExpressionField.tsx (1)
153-165: Critical: Type mismatch will cause runtime crash whentargetLineRangeis undefined
ChipExpressionEditorComponentdeclarestargetLineRange: LineRangeas a required prop (line 73 in ChipExpressionEditor.tsx), butExpressionFieldreceives it as optional (targetLineRange?: LineRangeat line 40). WhentargetLineRangeisundefined, the component will crash at line 245 in ChipExpressionEditor.tsx when accessingprops.targetLineRange.startLine.Make
targetLineRangerequired inExpressionFieldprops, or guard all accesses inChipExpressionEditorComponent:export interface ExpressionField { ... - targetLineRange?: LineRange; + targetLineRange: LineRange;Or if it must remain optional, update
ChipExpressionEditorComponentto handleundefined(see the review comment on ChipExpressionEditor.tsx for the guarding approach).workspaces/ballerina/ballerina-side-panel/src/components/editors/ExpandedEditor/modes/ExpressionMode.tsx (1)
51-61:targetLineRangeis optional but required downstream - will crash if undefinedExpressionMode receives
targetLineRangeas optional fromEditorModeExpressionProps(line 7 of types.ts:targetLineRange?: LineRange;), but passes it directly toChipExpressionEditorComponentwhich requires it (ChipExpressionEditor.tsx:targetLineRange: LineRange;without?). ChipExpressionEditor accesses it unsafely at line 245 (props.targetLineRange.startLine) without null checks.Fix: Either make
targetLineRangerequired inEditorModeExpressionProps, or add a guard inExpressionMode.tsxto conditionally render the component only whentargetLineRangeis defined.
♻️ Duplicate comments (2)
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/ChipExpressionEditor.tsx (2)
232-257: Critical: Unguarded access to optionaltargetLineRangewill crashLines 245 and 257 access
props.targetLineRange.startLinewithout checking iftargetLineRangeis defined. This will throw a runtime error iftargetLineRangeisundefined.Apply this guard to the token update effect:
useEffect(() => { - if (!props.value || !viewRef.current) return; + if (!viewRef.current) return; + if (!props.targetLineRange) { + // No target line information; skip token fetch + setIsTokenUpdateScheduled(false); + return; + } + if (!props.value) return; + const updateEditorState = async () => { const currentDoc = viewRef.current!.state.doc.toString(); const isExternalUpdate = props.value !== currentDoc; if (!isTokenUpdateScheduled && !isExternalUpdate) return; const currentSelection = viewRef.current!.state.selection.main; const tokenStream = await expressionEditorRpcManager?.getExpressionTokens( props.value, props.fileName, props.targetLineRange.startLine ); setIsTokenUpdateScheduled(false); if (tokenStream) { viewRef.current!.dispatch({ effects: tokensChangeEffect.of(tokenStream), changes: { from: 0, to: currentDoc.length, insert: props.value }, selection: { anchor: currentSelection.anchor, head: currentSelection.head } }); } }; updateEditorState(); -}, [props.value, props.fileName, props.targetLineRange.startLine, isTokenUpdateScheduled]); +}, [props.value, props.fileName, props.targetLineRange?.startLine, isTokenUpdateScheduled]);
73-73: Critical: RequiredtargetLineRangewill cause runtime crashes
targetLineRangeis declared as required but callers (ExpressionField.tsx line 160, and potentially ExpressionMode.tsx line 56) may passundefined. This causes a runtime crash at line 245 when accessingprops.targetLineRange.startLine, and in the dependency array at line 257.This is the same issue flagged in previous reviews.
Make
targetLineRangeoptional and guard all accesses:export type ChipExpressionEditorComponentProps = { ... - targetLineRange: LineRange; + targetLineRange?: LineRange;Then guard the token update effect (see next comment for the fix at lines 232-257).
🧹 Nitpick comments (3)
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/ChipExpressionEditor.tsx (3)
135-155: HACK acknowledged: Function argument extraction workaroundThe HACK comment notes that the current API doesn't send argument templates in the correct format ($1, $2, etc.), requiring an explicit extraction call. This is a temporary workaround that increases complexity and requires an additional async API call.
Consider opening an issue to track fixing the API response format so argument templates are correctly provided, eliminating the need for
extractArgsFromFunctionand simplifying this logic.Would you like me to draft an issue describing the API improvement needed?
178-178: ExtractHELPER_PANE_WIDTHas a module-level constantThe value
300is hardcoded in multiple locations (here, and inbuildOnFocusListnerandbuildOnChangeListnerin CodeUtils.ts lines 230 and 260). Extracting it as a shared constant improves maintainability.+const HELPER_PANE_WIDTH = 300; + export const ChipExpressionEditorComponent = (props: ChipExpressionEditorComponentProps) => { ... - const HELPER_PANE_WIDTH = 300;And update CodeUtils.ts to import and use the same constant.
260-261: Informal comment toneThe comment "just don't touch this" is casual. Consider a more professional explanation of why the ref pattern is necessary.
-// this keeps completions ref updated -// just don't touch this. +// Keep completions ref updated to avoid stale closure in autocompletion source. +// The completion source is built once; this ref ensures it always sees current completions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
workspaces/ballerina/ballerina-side-panel/src/components/editors/ExpandedEditor/modes/ExpressionMode.tsx(3 hunks)workspaces/ballerina/ballerina-side-panel/src/components/editors/ExpressionField.tsx(2 hunks)workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/CodeUtils.ts(1 hunks)workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/ChipExpressionEditor.tsx(1 hunks)workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/HelperPaneToggleButton.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/HelperPaneToggleButton.tsx
- workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/CodeUtils.ts
🧰 Additional context used
🧬 Code graph analysis (3)
workspaces/ballerina/ballerina-side-panel/src/components/editors/ExpressionField.tsx (1)
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/ChipExpressionEditor.tsx (1)
ChipExpressionEditorComponent(79-330)
workspaces/ballerina/ballerina-side-panel/src/components/editors/ExpandedEditor/modes/ExpressionMode.tsx (1)
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/ChipExpressionEditor.tsx (1)
ChipExpressionEditorComponent(79-330)
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/ChipExpressionEditor.tsx (5)
workspaces/ballerina/ballerina-side-panel/src/components/Form/types.ts (1)
HelperpaneOnChangeOptions(95-98)workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/CodeUtils.ts (12)
buildNeedTokenRefetchListner(287-306)buildOnChangeListner(308-343)buildOnFocusListner(244-275)CursorInfo(28-32)buildOnFocusOutListner(277-285)buildCompletionSource(345-378)buildHelperPaneKeymap(380-391)chipPlugin(184-214)tokenField(158-182)chipTheme(101-112)completionTheme(114-153)tokensChangeEffect(155-155)workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/HelperPaneToggleButton.tsx (1)
HelperPaneToggleButton(27-64)workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/styles.tsx (2)
ChipEditorContainer(45-52)FloatingButtonContainer(224-231)workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/FloatingButtonIcons.tsx (3)
CloseHelperButton(32-41)OpenHelperButton(21-30)ExpandButton(43-49)
🔇 Additional comments (2)
workspaces/ballerina/ballerina-side-panel/src/components/editors/ExpandedEditor/modes/ExpressionMode.tsx (1)
45-47: LGTM: handleChange wrapper correctly adapts onChange signatureThe wrapper properly forwards both the updated value and cursor position to maintain compatibility with the new
ChipExpressionEditorComponentAPI.workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/ChipExpressionEditor.tsx (1)
122-124: LGTM: Stale closure issue for Escape key handler is fixedThe helper pane keymap now correctly uses a getter function
() => helperPaneState.isOpeninstead of capturing a snapshot value. This ensures the Escape key handler always checks the current open state, resolving the issue flagged in previous reviews.Based on past_review_comments: the previous implementation captured
helperPaneState.isOpenonce and the Escape handler never closed the pane; this implementation now passes a getter tobuildHelperPaneKeymapwhich is correctly invoked at runtime.
...s/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/ChipExpressionEditor.tsx
Outdated
Show resolved
Hide resolved
...s/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/ChipExpressionEditor.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/ChipExpressionEditor.tsx (5)
78-80: Track the TODO for expressionHeight refactor.The TODO suggests accepting a style object instead of just height for more styling flexibility, which would improve the component API. Consider creating an issue to track this enhancement.
Do you want me to open a new issue to track this refactor task?
139-159: Track the technical debt indicated by the HACK comment.The workaround for incorrect API response format (lines 139-143) adds complexity and requires an extra API call to extract function arguments. The proper fix is for the API to send templates with
$1, $2, ...placeholders built-in, eliminating the need forextractArgsFromFunction.Do you want me to open a new issue to track fixing the API response format so this workaround can be removed?
278-282: Improve the comment to explain the ref pattern.The comment "just don't touch this" is unclear and unprofessional. Consider replacing it with an explanation of why the ref is needed.
Apply this diff:
- // this keeps completions ref updated - // just don't touch this. + // Keep completionsRef in sync with props.completions so the completion source + // (built once at line 124) always reads the latest completion items without + // requiring editor recreation. useEffect(() => { completionsRef.current = props.completions; }, [props.completions]);
284-302: Exclude toggle button from click-outside detection to prevent unnecessary state updates.When the helper pane is open and the user clicks the toggle button, the
mousedownhandler closes the pane, then the button'sonClickimmediately reopens it. While the end result is correct, this causes two state updates instead of one.Apply this diff to exclude the toggle button from click-outside logic:
const handleClickOutside = (event: MouseEvent) => { if (!helperPaneState.isOpen) return; const target = event.target as Element; const isClickInsideEditor = editorRef.current?.contains(target); const isClickInsideHelperPane = helperPaneRef.current?.contains(target); + const isClickOnToggleButton = helperPaneToggleButtonRef.current?.contains(target); - if (!isClickInsideEditor && !isClickInsideHelperPane) { + if (!isClickInsideEditor && !isClickInsideHelperPane && !isClickOnToggleButton) { setHelperPaneState(prev => ({ ...prev, isOpen: false })); } };
313-329: Extract duplicated height calculation logic.The height calculation is duplicated between lines 315-319 and 324-328, and the existing
getHeightValuehelper (lines 200-204) is unused in the render. This complex nested ternary should be computed once and reused.Apply this diff to eliminate duplication:
+ const containerHeight = props.isInExpandedMode + ? '100%' + : props.expressionHeight + ? getHeightValue(props.expressionHeight) + : 'auto'; + return ( <> {props.isExpandedVersion && ( <HelperPaneToggleButton ref={helperPaneToggleButtonRef} isOpen={helperPaneState.isOpen} onClick={handleHelperPaneManualToggle} /> )} <ChipEditorContainer ref={fieldContainerRef} style={{ position: 'relative', - height: props.isInExpandedMode - ? '100%' - : props.expressionHeight - ? (typeof props.expressionHeight === 'number' ? `${props.expressionHeight}px` : props.expressionHeight) - : 'auto' + height: containerHeight }}> {!props.isInExpandedMode && <FXButton />} <div style={{ position: 'relative', width: '100%', height: '100%' }}> <div ref={editorRef} style={{ - height: props.isInExpandedMode - ? '100%' - : props.expressionHeight - ? (typeof props.expressionHeight === 'number' ? `${props.expressionHeight}px` : props.expressionHeight) - : 'auto' + height: containerHeight }} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/ChipExpressionEditor.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/ChipExpressionEditor.tsx (4)
workspaces/ballerina/ballerina-side-panel/src/components/Form/types.ts (1)
HelperpaneOnChangeOptions(95-98)workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/CodeUtils.ts (13)
buildNeedTokenRefetchListner(287-306)buildOnChangeListner(308-343)buildOnFocusListner(244-275)CursorInfo(28-32)buildOnFocusOutListner(277-285)buildCompletionSource(345-378)buildHelperPaneKeymap(380-391)expressionEditorKeymap(216-240)chipPlugin(184-214)tokenField(158-182)chipTheme(101-112)completionTheme(114-153)tokensChangeEffect(155-155)workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/HelperPaneToggleButton.tsx (1)
HelperPaneToggleButton(27-64)workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/styles.tsx (2)
ChipEditorContainer(45-52)FloatingButtonContainer(224-231)
🔇 Additional comments (4)
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/ChipExpressionEditor.tsx (4)
84-95: LGTM!State and refs are properly initialized with correct typing.
97-128: LGTM!Event listener setup is correct. The helper pane auto-open/close logic (lines 108-112) appropriately shows the helper for empty expressions, trigger characters, and range selections while hiding it during normal typing—with the manual toggle providing user control when needed.
171-197: LGTM!Position calculation correctly handles viewport overflow on the horizontal axis. The vertical positioning relies on the button placement which should be reasonable for the UI context.
200-204: LGTM!Simple utility function for normalizing height values.
| useEffect(() => { | ||
| if (!editorRef.current) return; | ||
| const startState = EditorState.create({ | ||
| doc: props.value ?? "", | ||
| extensions: [ | ||
| history(), | ||
| keymap.of([...helperPaneKeymap, ...expressionEditorKeymap]), | ||
| autocompletion({ | ||
| override: [completionSource], | ||
| activateOnTyping: true, | ||
| closeOnBlur: true | ||
| }), | ||
| chipPlugin, | ||
| tokenField, | ||
| chipTheme, | ||
| completionTheme, | ||
| EditorView.lineWrapping, | ||
| needTokenRefetchListner, | ||
| handleChangeListner, | ||
| handleFocusListner, | ||
| handleFocusOutListner, | ||
| ...(props.isInExpandedMode | ||
| ? [EditorView.theme({ | ||
| "&": { height: "100%" }, | ||
| ".cm-scroller": { overflow: "auto" } | ||
| })] | ||
| : props.expressionHeight | ||
| ? [EditorView.theme({ | ||
| "&": { height: getHeightValue(props.expressionHeight) }, | ||
| ".cm-scroller": { overflow: "auto" } | ||
| })] | ||
| : []) | ||
| ] | ||
| }); | ||
| const view = new EditorView({ | ||
| state: startState, | ||
| parent: editorRef.current | ||
| }); | ||
| viewRef.current = view; | ||
| return () => { | ||
| view.destroy(); | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that prop callbacks remain stable or use refs to avoid stale closures.
The event listeners (handleChangeListner, handleFocusListner, etc.) are created on every render but only consumed during the initial editor setup (line 248: empty dependency array). They capture props.onChange and potentially other props at mount time. If parent components pass new function references for onChange on subsequent renders, the editor will continue calling the stale initial callback.
Options to resolve:
- Use a ref for
onChangeand other callbacks, and dereference inside the listeners - Document that callbacks must be stable (wrapped in
useCallbackby callers) - Recreate the editor (or update listeners) when callbacks change
Verify how callers pass the onChange prop. If it's not stable (e.g., inline arrow function or changes on each render), the listeners will call stale functions. Run the following script to check usage patterns:
🏁 Script executed:
#!/bin/bash
# Check how ChipExpressionEditorComponent is invoked to verify onChange stability
rg -A 5 -B 2 'ChipExpressionEditorComponent' --type=tsx --type=ts -g '!**/ChipExpressionEditor.tsx'Length of output: 27
[/skip_cloning]
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for ChipExpressionEditorComponent usage without file type restriction
# Also search for the component file to understand what it exports
# First, find the component definition
fd -e tsx -e ts | xargs grep -l "export.*ChipExpressionEditor" | head -5
echo "---"
# Then search for usages - search more broadly
rg "ChipExpressionEditorComponent|<ChipExpressionEditor" -A 5 -B 2 --type=ts -g '!**/ChipExpressionEditor.tsx' 2>/dev/null || rg "ChipExpressionEditorComponent|<ChipExpressionEditor" -A 5 -B 2 -g '!**/ChipExpressionEditor.tsx'
Length of output: 4393
🏁 Script executed:
#!/bin/bash
# Check ExpressionField.tsx for onChange prop definition
echo "=== ExpressionField.tsx onChange definition ==="
rg -B 20 "onChange={onChange}" workspaces/ballerina/ballerina-side-panel/src/components/editors/ExpressionField.tsx | head -40
echo ""
echo "=== ExpressionMode.tsx handleChange definition ==="
rg -B 20 "onChange={handleChange}" workspaces/ballerina/ballerina-side-panel/src/components/editors/ExpandedEditor/modes/ExpressionMode.tsx | head -40Length of output: 327
🏁 Script executed:
#!/bin/bash
# Use fixed string matching to avoid regex issues
echo "=== ExpressionField.tsx - function signature and onChange setup ==="
cat workspaces/ballerina/ballerina-side-panel/src/components/editors/ExpressionField.tsx | head -60
echo ""
echo "=== ExpressionMode.tsx - function signature and handleChange setup ==="
cat workspaces/ballerina/ballerina-side-panel/src/components/editors/ExpandedEditor/modes/ExpressionMode.tsx | head -80Length of output: 4590
🏁 Script executed:
#!/bin/bash
# Look for the ExpressionField component function implementation and how it uses onChange
echo "=== ExpressionField.tsx - component implementation ==="
cat workspaces/ballerina/ballerina-side-panel/src/components/editors/ExpressionField.tsx | tail -n +50
echo ""
echo "=== Check if handleChange in ExpressionMode uses useCallback ==="
cat workspaces/ballerina/ballerina-side-panel/src/components/editors/ExpandedEditor/modes/ExpressionMode.tsxLength of output: 6081
Fix stale closure bug: wrap callbacks in useCallback or use refs.
The verification confirms the original concern. In ExpressionMode.tsx, handleChange is created on every render without useCallback, creating a new function reference each time. Since ChipExpressionEditor initializes the editor only once (empty dependency array), the listeners close over the initial handleChange. On subsequent renders of ExpressionMode, a new handleChange is created but the editor continues to invoke the stale closure from mount time.
// ExpressionMode.tsx - current (buggy)
const handleChange = (updatedValue: string, updatedCursorPosition: number) => {
onChange(updatedValue, updatedCursorPosition);
};Fix: Wrap in useCallback:
const handleChange = useCallback((updatedValue: string, updatedCursorPosition: number) => {
onChange(updatedValue, updatedCursorPosition);
}, [onChange]);Alternatively, in ChipExpressionEditor.tsx, use a ref for callbacks to always read the current prop value from the ref on invocation, rather than capturing at mount time.
🤖 Prompt for AI Agents
In
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/ChipExpressionEditor.tsx
around lines 206-248, the editor effect mounts once and captures callback
references at mount time, causing stale closures for handlers like handleChange
from ExpressionMode; fix by ensuring callbacks are stable or read via refs:
either (preferred) wrap the handleChange in ExpressionMode.tsx with useCallback
with [onChange] as dependency so its reference is stable across renders, or
(alternate) in ChipExpressionEditor replace direct closure captures with refs
(e.g., currentHandlerRef.current = handler on each render and invoke the ref
inside editor listeners) so the editor always calls the latest handler.
| }, []); | ||
|
|
||
| useEffect(() => { | ||
| if (!props.value || !viewRef.current) return; |
There was a problem hiding this comment.
Empty string value prevents token updates.
The guard if (!props.value || !viewRef.current) return; will return early when props.value is an empty string '' (which is falsy). This prevents the effect from running when the user clears the expression, potentially leaving stale tokens displayed instead of clearing them.
Apply this diff to allow empty string values:
- if (!props.value || !viewRef.current) return;
+ if (props.value === undefined || props.value === null || !viewRef.current) return;Alternatively, if empty string should skip token fetch but still allow editor sync:
- if (!props.value || !viewRef.current) return;
+ if (!viewRef.current) return;📝 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.
| if (!props.value || !viewRef.current) return; | |
| if (props.value === undefined || props.value === null || !viewRef.current) return; |
🤖 Prompt for AI Agents
In
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/ChipExpressionEditor.tsx
around line 251, the current guard `if (!props.value || !viewRef.current)
return;` incorrectly treats an empty string as falsy and prevents the effect
from running when the expression is cleared; change the guard to only skip on
null/undefined (e.g., `if (props.value == null || !viewRef.current) return;`) so
empty string values are allowed to proceed and clear tokens; alternatively, if
clearing should skip token fetch but still sync the editor, split the logic:
allow empty string to update the editor state but conditionally skip fetching
tokens when `props.value === ''`.
…into merge-with-bi
2 participants
Purpose
Resolves: wso2/product-ballerina-integrator#1867
Goals
Approach
Summary by CodeRabbit
Chores
Refactor
New Features
Style