Skip to content

fix(dataset-editor): fix SQL expression editor extra spaces and height expansion#39248

Merged
mistercrunch merged 1 commit into
masterfrom
sql-expr-editor-fix
Apr 10, 2026
Merged

fix(dataset-editor): fix SQL expression editor extra spaces and height expansion#39248
mistercrunch merged 1 commit into
masterfrom
sql-expr-editor-fix

Conversation

@mistercrunch
Copy link
Copy Markdown
Member

@mistercrunch mistercrunch commented Apr 9, 2026

SUMMARY

Fixes multiple UX issues with the SQL expression editor in the dataset editor:

  1. Extra spaces on input: The value prop from the Field/Fieldset component chain was leaking through ...editorProps spread into ReactAce, causing it to operate in controlled mode. This produced extra spaces, cursor jumping, and sluggish input. Fixed by destructuring value out of the rest props and using it only as a fallback for defaultValue.

  2. Height expansion: maxLines defaulted to 10 for both computed column and metric expression editors, severely limiting the editor height. Increased to 25 so the editor auto-expands with content.

  3. Sluggish input: Added debounceDelay={300} to both expression editors to prevent full component tree re-renders on every keystroke.

  4. Metrics expression moved to expand form: The inline expression editor in the metrics table was cramped (240px wide). Replaced it with a read-only Typography.Text code ellipsis preview in the table cell, and added a full-width editable expression editor to the expanded form. Hovering the preview shows an "Expand row to edit" tooltip.

  5. Lock icon spacing: Added vertical padding to the EditLockContainer so the lock icon/text has proper spacing from the tab bar divider.

  6. Debounce flush on unmount: Changed componentWillUnmount from cancel() to flush() so pending debounced edits are persisted when the editor unmounts (e.g., collapsing an expanded row).

  7. AsyncEsmComponent loading spinner: Decoupled the loading spinner from requiring a height prop. The showLoadingForImport placeholder prop now defaults to true and uses a small spinner size, so async-loaded editors (like SQL expression) show a loading indicator while importing.

  8. initialValue default fix: Removed initialValue: '' from TextAreaControl.defaultProps so the initialValue ?? value fallback correctly uses value from the Field/Fieldset chain when no explicit initialValue is passed.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A - UI changes are self-evident when testing

TESTING INSTRUCTIONS

  1. Open a dataset's settings (Edit Dataset modal)
  2. Go to Metrics tab:
    • Expression column should show truncated SQL in monospace code style
    • Hover should show "Expand row to edit" tooltip
    • Expand a metric row — the SQL expression editor should appear at the top of the form with the expression loaded
    • Edit the expression — should be responsive (no lag or extra spaces)
    • The preview in the table should update when you collapse the row
  3. Go to Calculated columns tab:
    • Expand a computed column — the SQL expression editor should auto-expand up to 25 lines
    • Typing should be responsive
  4. Go to Source tab:
    • Lock icon and text should have proper spacing from the tab bar
  5. Any async-loaded Ace editor should show a small loading spinner while importing

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

🤖 Generated with Claude Code

@dosubot dosubot Bot added the change:frontend Requires changing the frontend label Apr 9, 2026
@bito-code-review
Copy link
Copy Markdown
Contributor

Yes, the suggestion is valid — the current diff uses value only as defaultValue, preventing external updates from reflecting in the editor. To enable controlled behavior when value is provided, pass it as the value prop to TextAreaEditor instead, with onChange handling, while keeping uncontrolled mode for dataset-expression contexts.

Comment thread superset-frontend/src/explore/components/controls/TextAreaControl.tsx Outdated
@mistercrunch mistercrunch force-pushed the sql-expr-editor-fix branch from 60eb309 to 8299462 Compare April 9, 2026 23:21
Copy link
Copy Markdown
Contributor

@bito-code-review bito-code-review Bot left a comment

Choose a reason for hiding this comment

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

Code Review Agent Run #1d55ee

Actionable Suggestions - 1
  • superset-frontend/src/explore/components/controls/TextAreaControl.tsx - 1
Additional Suggestions - 1
  • superset-frontend/src/explore/components/controls/TextAreaControl.tsx - 1
    • Redundant CSS property · Line 180-180
      Textarea elements have 'overflow: auto' by default, so explicitly setting it when resize is enabled is unnecessary.
Review Details
  • Files reviewed - 2 · Commit Range: 60eb309..60eb309
    • superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx
    • superset-frontend/src/explore/components/controls/TextAreaControl.tsx
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 9, 2026

Codecov Report

❌ Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 64.38%. Comparing base (5815665) to head (099fa5f).
⚠️ Report is 19 commits behind head on master.

Files with missing lines Patch % Lines
...ui-core/src/components/AsyncEsmComponent/index.tsx 83.33% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master   #39248   +/-   ##
=======================================
  Coverage   64.37%   64.38%           
=======================================
  Files        2550     2550           
  Lines      132180   132183    +3     
  Branches    30661    30660    -1     
=======================================
+ Hits        85096    85104    +8     
+ Misses      45599    45594    -5     
  Partials     1485     1485           
Flag Coverage Δ
javascript 65.92% <90.90%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…t expansion

Prevent the `value` prop from leaking through to ReactAce via rest
spread, which caused the editor to operate in controlled mode and
produce extra spaces on input. Also increase maxLines from 10 to 25
for both computed column and metric expression editors so the editor
auto-expands with content, and add overflow:auto when resize is
enabled so the CSS resize handle works properly.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@bito-code-review bito-code-review Bot left a comment

Choose a reason for hiding this comment

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

Code Review Agent Run #b11494

Actionable Suggestions - 1
  • superset-frontend/src/explore/components/controls/TextAreaControl.tsx - 1
    • Inconsistent prop handling between textarea implementations · Line 159-165
Review Details
  • Files reviewed - 3 · Commit Range: 099fa5f..099fa5f
    • superset-frontend/packages/superset-ui-core/src/components/AsyncEsmComponent/index.tsx
    • superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx
    • superset-frontend/src/explore/components/controls/TextAreaControl.tsx
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

@mistercrunch
Copy link
Copy Markdown
Member Author

Acknowledged — this is an intentional design choice. See replies on the inline review comments for the full rationale. In short: controlled mode (passing value) is the root cause of the extra-spaces/cursor-jumping bug. We use uncontrolled mode (defaultValue) deliberately.

Copy link
Copy Markdown
Contributor

@aminghadersohi aminghadersohi left a comment

Choose a reason for hiding this comment

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

LGTM — a couple of notes for awareness:

  1. showLoadingForImport default change (falsetrue in AsyncEsmComponent) — this is a global behavioral change affecting all async-loaded components, not just the SQL expression editor. Worth keeping an eye on whether unexpected spinners pop up elsewhere. If they do, might be better to pass the prop explicitly where needed instead of changing the default.

  2. cancel()flush() on unmount — makes sense here to persist pending debounced edits when collapsing a row, but note this changes behavior for all TextAreaControl unmount paths.

  3. Metrics expression move to expand form, maxLines bump, debounce, and the value destructuring fix all look good.

@mistercrunch mistercrunch merged commit 0aa8cac into master Apr 10, 2026
85 of 87 checks passed
@mistercrunch mistercrunch deleted the sql-expr-editor-fix branch April 10, 2026 19:12
michael-s-molina pushed a commit that referenced this pull request Apr 13, 2026
…t expansion (#39248)

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
(cherry picked from commit 0aa8cac)
qfcwell pushed a commit to qfcwell/superset that referenced this pull request May 12, 2026
…t expansion (apache#39248)

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
@github-actions github-actions Bot added 🍒 6.1.0 Cherry-picked to 6.1.0 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels labels May 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels change:frontend Requires changing the frontend packages preset-io size/M 🍒 6.1.0 Cherry-picked to 6.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants