perf(datasource-editor): fix typing lag and scrambling in form fields

[mikebridge]

🚧🚧🚧 Still draft, but actively iterating on review feedback. Three rounds of fixes have landed, plus a scope-narrowing follow-up. 🚧🚧🚧

SUMMARY

Typing into any Dataset Editor text field (Settings → Basic Description, column descriptions, metric labels, SQL expressions) was visibly slow on real-world datasets. On fast typing, characters landed in the DOM in the order renders finished rather than the order typed, manifesting as "scrambled" input. No network I/O is involved — the bug is a React render-storm: every keystroke ran synchronous O(n) validation (duplicate checks, currency Intl.NumberFormat instantiation, folder validation) inside the setState callback, propagating through children whose props had unstable identity on every render.

Two coordinated changes land in two files (DatasourceEditor.tsx, Fieldset/index.tsx); no new npm dependencies (lodash/debounce is already transitively pinned).

1. Debounce validate with a guarded blur-flush and a synchronous unmount drain. Validation moves off the keystroke hot path via lodash.debounce(..., 300). A handleContainerBlur handler on DatasourceContainer flushes pending validation when focus actually leaves the container — e.relatedTarget is checked so intra-form tabbing between fields does not flush, preserving the debounce for the common multi-field-edit case. componentWillUnmount does a drain-then-cancel: if a validation is pending, errors are computed synchronously via a new pure computeErrors() method and propagated to the parent via props.onChange(newDatasource, errors) directly — bypassing the dead-end setState({errors}) (a no-op during unmount). Save-path safety is preserved because the parent DatasourceModal already disables Save when errors.length > 0, and the blur-flush ensures errors reflects the latest typed state by the time the Save button is reachable.

2. Memoise derived arrays (sortedMetrics, datetimeColumns, stringColumns) with one-pair (lastInput, lastOutput) caches so downstream PureComponent / React.memo children stop re-rendering on unrelated state changes. Hoist three inline itemGenerator={() => ({...})} lambdas to stable class-field arrow functions (newSpatialItem, newMetricItem, newCalculatedColumnItem).

Validation semantics are preserved — same validators, same error messages, same Save-button gate. Only the schedule and the propagation lifecycle change.

What was tried and reverted

• destroyInactiveTabPane=true (lazy-mount hidden tabs): originally shipped; reverted after review feedback. Per @kgabryje's review, the perf gain was outweighed by UX regressions — losing CollectionTable local state (search, pagination, expanded rows, sort) on every tab switch, plus losing pending debounced edits inside expanded TextAreaControl rows. The remaining changes (debounce + memoised arrays + stable handlers) carry the typing-lag fix on their own; hidden tabs still get reconciled per keystroke but their PureComponent-wrapped children short-circuit on stable props.

• IME composition guards in TextAreaControl (isComposing / justComposed flags): originally shipped to suppress per-candidate-character renders during East-Asian composition; dropped from this PR after profiling showed the guard never engages. Every TextAreaControl callsite in the codebase passes language= (markdown / sql / json), which routes through Ace's TextAreaEditor. Ace handles composition internally and bypasses React's onCompositionStart / onCompositionEnd, so the guard's flags never flipped. Profile traces with the guard in place vs. master showed identical commit fanout and per-commit duration during IME composition on the dataset Description field.

  Wiring composition handlers into the Ace branch (via editor.session DOM listeners or similar) is a separate effort and a separate PR. The fast-typing fix in this PR carries over to IME composition opportunistically — memoisation and debouncing make per-keystroke work cheaper regardless of whether the keystroke comes from a regular keypress or an IME candidate-character event. So IME users see the general perf win, just not a targeted "only one validate per composed word" behaviour.

Review history (PR #39641 review by @kgabryje)

Four review points; three landed, one architectural limit explicitly documented:

#	Issue	Status
1	componentWillUnmount calling .cancel() silently dropped pending state propagation	Fixed via drain-then-cancel: split validate() into computeErrors() + setState write; on unmount, compute synchronously and call props.onChange directly. Caveat: this helps when the parent outlives the child (route changes, AsyncEsmComponent swaps); when the parent unmounts simultaneously (the typical "close modal" race), its useState setters are also no-ops, so the propagation has nowhere to land. Closing that fully would require lifting editor state out of React component-local state — an architectural change beyond this PR's scope.
2	IME guard double-fire on Firefox/Edge	Initially fixed via the justComposed flag, then the entire IME guard was dropped after investigation showed it never engaged in the dataset editor. See "What was tried and reverted" above.
3	onBlur bubbling defeats the debounce on intra-form focus changes	Fixed via the e.relatedTarget guard in handleContainerBlur (described above).
4	destroyInactiveTabPane is too aggressive	Dropped; documented under "What was tried and reverted".

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Chrome React Profiler recordings on wb_health_population (Settings → Basic Description, ~40 chars typed at normal speed). Both recordings are on the currently-shipped code (master vs the fix branch); cache-bust was verified via a temporary tab-label change so the bundle freshness on each side is unambiguous.

Metric	Before (master)	After (fix)
Total commits during typing	335	105
Commits per character typed	~8.4	~2.6
Worst typing-commit duration	~24 ms (frequent, tight clusters)	~29 ms (batched, infrequent — single post-debounce settle)

The "worst commit got slightly slower" is the expected debounce signature: many small per-keystroke renders are traded for fewer larger settle renders. The point is keeping per-keystroke work off the critical path so the frame budget is met during typing — not minimising the slowest commit.

BEFORE

AFTER

TESTING INSTRUCTIONS

1. Open the Datasets list. Click the pencil icon on a dataset with many columns (e.g., wb_health_population — ~50 columns).
2. The Dataset Editor modal opens. Click the Settings tab.
3. Type a paragraph into Basic Description at a normal touch-typing speed (~60 wpm).
   • Before: characters lag visibly, and on fast typing, letters/punctuation can land out of order.
   • After: characters appear in the order typed, no lag, no scrambling.
4. Open Chrome DevTools → Performance, record 2 s of typing. Confirm no render commit exceeds 50 ms in the DatasourceEditor path.
5. Save-path regression check: in the Columns tab, rename a column to match another column's name. Click Save immediately (within 300 ms). Expected: duplicate-name error surfaces and Save is blocked.
6. Tab-state preservation check (regression for the dropped destroyInactiveTabPane): in the Columns tab, expand a row, set a sort, type in the search filter. Switch to Metrics, then back to Columns. Expected: search term, sort, and expansion are all preserved.

Jest test suite (51 tests across 6 suites) passes unchanged:

cd superset-frontend && npm run test -- "(DatasourceEditor.test|TextAreaControl.test|DatasourceModal)"
# Test Suites: 6 passed, 6 total   Tests: 51 passed, 51 total

Design decisions captured in the spec

• Option C — blur-flush (with relatedTarget guard) was chosen over a cross-component ref-flush via the AsyncEsmComponent wrapper. Simpler, no ref plumbing, and the two-step Save confirm modal naturally produces enough delay for the blur-fired flush to settle errors before the user can click OK.
• DatasourceEditor kept as a class component (not migrated to functional). Explicitly justified — the bug is fixable surgically; a functional rewrite is out of scope per spec.md.
• 300 ms debounce window (under the 500 ms FR-003 ceiling) chosen per research.md R1.
• Architectural limit acknowledged: editor state lives in React component-local state, so the unmount-drain only helps when the parent outlives the child. Truly bulletproof unmount safety requires lifting editor state into Redux or a form-state library — out of scope, documented in spec.md FR-004a and research.md R9.
• IME guard intentionally out of scope: addressing IME composition for dataset-editor fields requires Ace-level handlers (every TextAreaControl here uses language= and routes through Ace, bypassing React composition events). That's a separate effort with its own design + cross-browser tests. Documented in research.md R6.

What's missing (why this is a draft)

• Playwright spec dataset-editor-typing.spec.ts with 25/50/100 ms keystroke speeds and the three duplicate-column-name validation scenarios — deferred to follow-up. First attempt revealed selector strategy issues (getByLabel('Default URL') doesn't resolve because Field/TextControl don't wire htmlFor/aria-labelledby; the duplicate-name error only surfaces in the disabled Save button's tooltip text per DatasourceModal:355-370, not as visible text). Reworking selectors + iterating over CI cycles wasn't worth blocking this perf fix on.
• 50-column dataset Playwright fixture — deferred with the spec above
• Jest unit test for the unmount-drain path (FR-004a) — landed alongside the existing 26 tests
• Chrome Performance profile evidence attached (SC-002)
• CI green on the existing playwright-tests (chromium) job — verifies that no existing Dataset Editor Playwright test regresses from these frontend changes (SC-005)
• Hidden-tab DOM-presence test audit — N/A after destroyInactiveTabPane was reverted

ADDITIONAL INFORMATION

• Has associated issue: internal ticket sc-104866
• Required feature flags:
• Changes UI (behavioural — typing latency; no visible UI element changes)
• Includes DB Migration
• Introduces new feature or API
• Removes existing feature or API

Constitution Compliance

Per the project constitution, Principle III (Modern Frontend Patterns) requires functional components with hooks for new code. This PR edits the existing class PureComponent DatasourceEditor rather than migrating it — justified because the bug can be resolved surgically without a functional rewrite, and the spec explicitly scopes the migration out of scope. Any new code introduced by this PR (the memoisation helpers, unmount-drain logic) could have been written functionally if not for the surrounding class context; they follow functional patterns in spirit (arrow class fields for stable identity, no this-binding ceremony, pure helpers like computeErrors() that don't depend on instance lifecycle).

🤖 Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 93.54839% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.50%. Comparing base (549aff7) to head (5a27207).

Files with missing lines	Patch %	Lines
...e/components/DatasourceEditor/DatasourceEditor.tsx	93.54%	4 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #39641   +/-   ##
=======================================
  Coverage   64.49%   64.50%           
=======================================
  Files        2566     2566           
  Lines      133972   134019   +47     
  Branches    31124    31136   +12     
=======================================
+ Hits        86403    86445   +42     
- Misses      46074    46079    +5     
  Partials     1495     1495           

Flag	Coverage Δ
javascript	66.61% <93.54%> (+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.

[netlify]

✅ Deploy Preview for superset-docs-preview ready!

Name	Link
🔨 Latest commit	2da59e8
🔍 Latest deploy log	https://app.netlify.com/projects/superset-docs-preview/deploys/69f11d25a6847c00070741b6
😎 Deploy Preview	https://deploy-preview-39641--superset-docs-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[mikebridge]

In other words, each keystroke is causing the metric to be recalculated and then this rerenders the tabs, so we're using a stable metrics reference here, not a value.

[bito-code-review]

Yes, that's correct. The code adds caching for derived arrays like sorted metrics, returning a stable reference when inputs haven't changed, preventing unnecessary recalculations on each keystroke. The debounced validation also defers heavy checks to avoid blocking the UI.

[mikebridge]

Note: An empty list causes this reference to change, but sortMetrics happens to work ok on non-empty arrays because sort mutates the array and keeps the same reference. getSortedMetrics makes both cases stable.

[mikebridge]

handleChange is now an arrow function, so it doesn't need bind, and therefore doesn't create a new function reference each time.

[mikebridge]

Changing this to an arrow function means we don't need bind (see below)

[mikebridge]

Before, validation happened with every keystroke, now input is debounced so it will happen on blur.

[mikebridge]

I prefer to make this a stable reference, even though it isn't strictly necessary. Eventually we do this for the other unstable references (onDatasourcePropChange.bind, etc.) and clean up the whole render, but that's a bit more work outside this ticket.

[mikebridge]

The string and date columns here were always creating a new reference via filter / map.

[mikebridge]

These are extracted into stable arrow-function references from inside the render

[mikebridge]

We are switching to only mounting one tab at at a time. The destroyInactiveTabPane changes the strategy to "lazy mount + unmount on hide". This is fine because the data is untouched, but it does mean that we have to be aware that we will lose state in the tabs.

[kgabryje]

Code review

Found 4 issues:

1. componentWillUnmount calls .cancel() on the new debouncedValidateAndChange, but the debounced callback runs validate(this.onChange) which propagates state to the parent via props.onChange. An in-flight edit within the 300 ms window at unmount silently drops the parent state update. PR fix(dataset-editor): fix SQL expression editor extra spaces and height expansion #39248 (commit 0aa8cace1b) already established the project pattern of .flush() over .cancel() in TextAreaControl.componentWillUnmount for the same reason — this PR re-introduces the inverse pattern.

superset/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx

Lines 2719 to 2727 in a1fd13c

	}
	componentWillUnmount() {
	this.isComponentMounted = false;
	this.debouncedValidateAndChange.cancel();
	// Abort all pending requests
	Object.values(this.abortControllers).forEach(controller => {

2. IME committed value can fire onChange twice. handleCompositionEnd clears this.isComposing = false before propagating the value. In Firefox/Edge the synthetic onChange from the composition's final character fires after compositionend (per W3C uievents #202), so by the time handleChange runs the guard if (this.isComposing) return no longer short-circuits and the committed value propagates a second time. Consider a "just-finished-composing" flag cleared on the next change event, or clearing isComposing in a microtask.

superset/superset-frontend/src/explore/components/controls/TextAreaControl.tsx

Lines 116 to 132 in a1fd13c

	handleCompositionStart = () => {
	this.isComposing = true;
	};
	handleCompositionEnd = (
	e: React.CompositionEvent<HTMLTextAreaElement | HTMLInputElement>,
	) => {
	this.isComposing = false;
	const target = e.target as HTMLTextAreaElement | HTMLInputElement;
	if (target?.value != null) {
	if (this.debouncedOnChange) {
	this.debouncedOnChange(target.value);
	} else {
	this.props.onChange?.(target.value);
	}
	}
	};

superset/superset-frontend/src/explore/components/controls/TextAreaControl.tsx

Lines 148 to 160 in a1fd13c

	}
	handleChange = (value: string | { target: { value: string } }) => {
	// Skip intermediate IME candidate events. The final committed value is
	// propagated once from handleCompositionEnd.
	if (this.isComposing) return;
	const finalValue = typeof value === 'object' ? value.target.value : value;
	if (this.debouncedOnChange) {
	this.debouncedOnChange(finalValue);
	} else {
	this.props.onChange?.(finalValue);
	}
	};

3. onBlur={flushValidation} on the outer DatasourceContainer fires on every intra-form focus change. React's onBlur bubbles, so tabbing/clicking between fields inside the container fires blur on the container and triggers debouncedValidateAndChange.flush() synchronously each time, defeating the debounce for the common editing case. Guard with e.relatedTarget and only flush when focus actually leaves the container (e.g. !e.currentTarget.contains(e.relatedTarget)).

superset/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx

Lines 1113 to 1119 in a1fd13c

	this.debouncedValidateAndChange();
	}
	flushValidation = () => {
	this.debouncedValidateAndChange.flush();
	};

superset/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx

Lines 2448 to 2456 in a1fd13c

	const sortedMetrics = this.getSortedMetrics(metrics);
	return (
	<DatasourceContainer
	data-test="datasource-editor"
	onBlur={this.flushValidation}
	>
	{this.renderErrors()}
	<Alert

4. destroyInactiveTabPane unmounts inactive tabs on switch, which resets CollectionTable local state in the previously-active tab (expanded rows, pagination, sort, search). It also creates an edit-loss path: a TextAreaControl inside an expanded row owns a 300 ms debounced onChange; switching tabs unmounts the row, and handleTabSelect only calls setState({ activeTabKey }) — it doesn't flush. Ant Design already lazy-mounts tabs on first activation; destroyInactiveTabPane is the wrong knob if the goal is lazy mount without destroy-on-deactivate.

superset/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx

Lines 2469 to 2477 in a1fd13c

	<StyledTableTabs
	id="table-tabs"
	data-test="edit-dataset-tabs"
	onChange={this.handleTabSelect}
	defaultActiveKey={activeTabKey}
	destroyInactiveTabPane
	items={[
	{
	key: TABS_KEYS.SOURCE,

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[mikebridge]

Code review

Found 4 issues:

@kgabryje I think 2 & 3 can be done easily, and probably we can revert destroyInactiveTabPanes for now so the UX stays the same. Issue 1 seems somewhat more complex than the review mentions and is a bit more non-trivial:

Issue 1: .cancel() should be .flush() on unmount

Severity: real bug, non-trivial fix

The claim: if user types something within 300 ms of closing the editor, our componentWillUnmount cancels the pending validation, dropping the parent-state propagation (since the debounced fn runs validate(this.onChange) which calls props.onChange to the parent at the end).

Catch they missed: the onChange propagation uses setState({errors}, callback) where the callback is this.onChange. React no-ops setState during unmount, and the callback never fires. So a naïve .flush() swap doesn't actually help — the synchronous setState inside validate still discards the propagation.

PR #39248's pattern works in TextAreaControl because its debounced fn is props.onChange directly, no intermediate setState. Our case is different.

Real fix: refactor validate() to expose a sync compute path (_computeErrors() -> errors[]) plus a separate setState write, then on flush-during-unmount, compute errors synchronously and
call props.onChange(newDatasource, errors) directly without going through our setState. About 30 minutes of careful work plus a unit test.

[mikebridge]

Code review

Found 4 issues:

@kgabryje Here's the summary from claude on 2, 3 & 4. I will look at the first one separately:

2 (IME double-fire on Firefox/Edge) — added a justComposed one-shot flag in TextAreaControl. Set in handleCompositionEnd, cleared on the next handleChange. Suppresses the
trailing synthetic onChange that Firefox/Edge fire after compositionend without affecting Chrome/Safari.

3 (onBlur bubbles inside the form) — replaced onBlur={this.flushValidation} with handleContainerBlur, which guards on !e.currentTarget.contains(e.relatedTarget) and only flushes
when focus actually leaves the editor. Intra-form tab navigation no longer defeats the debounce.

4 (destroyInactiveTabPane) — dropped. The UX cost (resetting CollectionTable search, pagination, and expanded-row state on every tab switch) outweighs the perf benefit, given that
the remaining changes (debounce, memoised derived arrays, stable handler identity, IME guards) carry the typing-lag fix on their own. The BEFORE/AFTER profiler comparison in the PR body
will be re-recorded.

1 (.cancel() vs .flush() on unmount) — deferred to a follow-up commit. The naïve swap doesn't work for this case: the debounced function calls validate(this.onChange), which
propagates state via setState({errors}, callback). React no-ops setState during unmount, so the callback (which calls props.onChange to the parent) never fires. PR #39248's pattern
works because TextAreaControl's debounced fn is props.onChange directly. The fix here requires refactoring validate() to expose a sync _computeErrors() path so flush-on-unmount can
propagate directly to props.onChange without relying on the component's own setState.