Skip to content

Conversation

@KCSAbeywickrama
Copy link
Contributor

@KCSAbeywickrama KCSAbeywickrama commented Nov 24, 2025

Purpose

Fixes wso2/product-ballerina-integrator#1864
Additionally fixes losing focus when closing completions using the close button

Summary by CodeRabbit

  • New Features

    • Exposed a programmatic cursor-setting capability for the expression editor to allow external control of text and cursor placement.
  • Bug Fixes

    • More reliable text insertion and cursor positioning when selecting input ports in the expression editor.
    • Prevented the close button from causing unintended focus or selection changes.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 24, 2025

Walkthrough

The ExpressionEditor API gains a new imperative setCursor(value, cursorPosition) method; the DataMapper ExpressionBar now inserts generated input-access expressions at the current text cursor using the input element's selectionStart (instead of shadowRoot lookup), updates the text via handleChange, advances the cursor, and then resets the input port.

Changes

Cohort / File(s) Summary
ExpressionEditor component
workspaces/common-libs/ui-toolkit/src/components/ExpressionEditor/components/Header/ExpressionEditor.tsx
Adds useImperativeHandle exposure of setCursor(value: string, cursorPosition: number), and prevents undesired focus/selection on the close icon by wrapping it with onMouseDown={e => e.preventDefault()}.
ExpressionEditor types
workspaces/common-libs/ui-toolkit/src/components/ExpressionEditor/types/header.ts
Extends HeaderExpressionEditorRef with setCursor: (value: string, cursorPosition: number) => void.
DataMapper ExpressionBar
workspaces/ballerina/data-mapper/src/components/DataMapper/Header/ExpressionBar.tsx
Replaces shadowRoot-based cursor calculation with textFieldRef.current.inputElement.selectionStart; inserts generated input-access expression at that cursor position, calls handleChange with the updated text, moves the cursor forward by the inserted length, then calls resetInputPort(); simplifies effect and disabled-computation dependency arrays (removes textFieldRef.current).

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant DataMapper as ExpressionBar
    participant Input as TextField/InputElement
    participant Editor as ExpressionEditor

    User->>DataMapper: Selects input port (expression bar focused)
    DataMapper->>Input: read selectionStart (cursorPos)
    Note right of DataMapper `#DCEFFB`: OLD used shadowRoot-based cursor lookup
    Note right of DataMapper `#BEE7A5`: NEW uses inputElement.selectionStart
    DataMapper->>Input: Insert generated expression at cursorPos
    DataMapper->>Editor: handleChange(updatedText)
    DataMapper->>Input: set cursor to cursorPos + insertedLength
    DataMapper->>DataMapper: resetInputPort()
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Verify setCursor imperative handle signature and that it correctly updates focus/selection.
  • Confirm insertion logic uses selectionStart safely (null checks) across browsers and input ref lifecycles.
  • Check effect dependency simplifications to avoid stale closures.
  • Quick UI test for close-icon mousedown behavior.

Possibly related PRs

Suggested reviewers

  • hevayo
  • gigara

Poem

🐰 I nibble at the cursor's spot,
I tuck the field where fingers plot.
No longer shoved to start or end,
The expression finds its proper bend.
Hooray — a hop, a gentle mend! 🥕

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description check ⚠️ Warning The description is minimal and lacks most required template sections including Goals, Approach, testing details, documentation, and security checks. Expand the description to include Goals, Approach with any UI/technical details, test coverage information, documentation impact, and security verification checklist.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and clearly describes the main change: fixing expression bar focusing issues related to the data mapper component.
Linked Issues check ✅ Passed The code changes address both objectives: inserting selected fields at cursor position instead of prepending, and preventing focus loss when closing completions.
Out of Scope Changes check ✅ Passed All changes are scoped to fixing the expression bar focusing issues, including cursor position handling and focus management in the expression editor.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2139236 and 0552283.

📒 Files selected for processing (1)
  • workspaces/common-libs/ui-toolkit/src/components/ExpressionEditor/components/Header/ExpressionEditor.tsx (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • workspaces/common-libs/ui-toolkit/src/components/ExpressionEditor/components/Header/ExpressionEditor.tsx

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
workspaces/ballerina/data-mapper/src/components/DataMapper/Header/ExpressionBar.tsx (1)

215-215: Use consistent inputElement access pattern.

Line 215 still uses the shadowRoot approach (textFieldRef.current.shadowRoot.querySelector('input').selectionStart), while line 116 uses the cleaner inputElement API. For consistency and maintainability, use the same pattern throughout the file.

Apply this diff to align with the new pattern:

-        const cursorPosition = textFieldRef.current.shadowRoot.querySelector('input').selectionStart;
+        const cursorPosition = textFieldRef.current.inputElement?.selectionStart ?? 0;
         triggerCompletions(outputId, viewId, textFieldValue, cursorPosition);
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 816940a and 2139236.

📒 Files selected for processing (3)
  • workspaces/ballerina/data-mapper/src/components/DataMapper/Header/ExpressionBar.tsx (2 hunks)
  • workspaces/common-libs/ui-toolkit/src/components/ExpressionEditor/components/Header/ExpressionEditor.tsx (3 hunks)
  • workspaces/common-libs/ui-toolkit/src/components/ExpressionEditor/types/header.ts (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-20T06:44:07.498Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 969
File: workspaces/ballerina/data-mapper/src/components/Diagram/utils/node-utils.ts:23-44
Timestamp: 2025-11-20T06:44:07.498Z
Learning: In the data mapper node-utils.ts, when `lastViewIndex == 0`, the code is at the root view and should NOT use `focusInputRootMap` for fallback input node resolution. The truthiness check on `lastViewIndex` is intentional to skip the fallback at the root level.

Applied to files:

  • workspaces/ballerina/data-mapper/src/components/DataMapper/Header/ExpressionBar.tsx
🧬 Code graph analysis (2)
workspaces/common-libs/ui-toolkit/src/components/ExpressionEditor/components/Header/ExpressionEditor.tsx (1)
workspaces/common-libs/ui-toolkit/src/components/Codicon/Codicon.tsx (1)
  • Codicon (44-56)
workspaces/ballerina/data-mapper/src/components/DataMapper/Header/ExpressionBar.tsx (1)
workspaces/ballerina/data-mapper/src/components/Diagram/utils/modification-utils.ts (1)
  • buildInputAccessExpr (205-220)
🔇 Additional comments (4)
workspaces/common-libs/ui-toolkit/src/components/ExpressionEditor/types/header.ts (1)

25-25: LGTM! Clean API extension for cursor control.

The new setCursor method provides a straightforward way for external components to programmatically position the cursor, which aligns with the PR objective of inserting expressions at the current cursor position.

workspaces/common-libs/ui-toolkit/src/components/ExpressionEditor/components/Header/ExpressionEditor.tsx (2)

340-342: LGTM! Correct implementation of the new setCursor API.

The imperative handle method correctly delegates to the existing setCursor utility and is properly exposed through useImperativeHandle.

Also applies to: 382-382


427-444: LGTM! Prevents focus loss when closing completions.

Wrapping the close button with onMouseDown preventDefault ensures the input field retains focus when the user clicks the close button, addressing the focus-loss issue mentioned in the PR description.

workspaces/ballerina/data-mapper/src/components/DataMapper/Header/ExpressionBar.tsx (1)

108-137: Verify effect dependencies and correctness.

The effect uses focusedPort, focusedFilter, and textFieldValue (lines 114, 123) but only lists [inputPort] as a dependency. While the gating conditional on line 114 may make this intentional, please verify that stale closures won't cause issues when focusedPort/focusedFilter change between inputPort updates.

Additionally, the PR objectives mention restoring undo functionality (both undo icon and keyboard shortcut Ctrl+Z), but I don't see undo-related changes in this PR. Please confirm whether this objective is addressed elsewhere or if additional changes are needed.

@KCSAbeywickrama KCSAbeywickrama merged commit e007ce8 into wso2:main Nov 27, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Datamapper] expression editor at the top of the data mapper always prepend the selected input field to the existing expression

2 participants