You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "mikebridge (via GitHub)" <gi...@apache.org> on 2026/07/28 14:36:57 UTC
[PR] fix(semantic-layers): survive large multi-metric selection in Semantic View modal [superset]
mikebridge opened a new pull request, #42535:
URL: https://github.com/apache/superset/pull/42535
### SUMMARY
Selecting many metrics while creating a Semantic View crashes the application once the selection count passes a threshold ([sc-107832](https://app.shortcut.com/preset/story/107832)). Semantic layers commonly expose large metric catalogs, so this blocks a mainstream configuration path.
The metrics/dimensions pickers in the Add Semantic View modal are produced by a custom JSON Forms renderer (`MultiEnumControl` in `jsonFormsHelpers.tsx`). Four compounding defects combined into the crash:
1. **Unbounded tag rendering** — the renderer used a *direct antd* `Select` with no `maxTagCount`: one tag DOM node per selected metric, all re-rendered on every subsequent selection. (The sibling "views" picker in the same modal uses Superset's wrapped Select, whose default caps tags at 4.)
2. **Per-selection schema remount + validator-cache growth** — when the metrics field is a dependency of a dynamic field, each selection re-fetches the runtime schema; the response always got new object identities, so JSON Forms rebuilt its AJV validator and remounted the whole renderer tree per selection, while AJV cached another compiled copy of the full enum schema each time (cumulative memory growth).
3. **O(N·M) re-render churn** — the JSON Forms `config` prop was an inline literal (new identity per render → every control re-renders per selection) and the renderers rebuilt (one even sorted) their full option arrays per render, unmemoized.
4. **Latent infinite-update loop** — `ConstControl`/`ReadOnlyControl` compared array/object defaults by reference; a `default: []` field calls `handleChange` forever ("Maximum update depth exceeded").
**The fix**
- `MultiEnumControl`, `DynamicFieldControl`, and `EnumNamesControl` migrate to the wrapped `@superset-ui/core/components` `Select` — capped tag display with "+ N" overflow, `allowSelectAll={false}` (this is a pure stability fix; no new bulk-selection affordance), `ariaLabel`, the wrapped `optionFilterProps` API — with option arrays memoized. `Spin`/`Form` migrate alongside, leaving the file with **zero direct antd imports** (repo guideline: use wrapped component packages).
- `applyRuntimeSchema` skips refresh responses whose sanitized schema is unchanged, so identical refreshes cause no remount and no new AJV compilation; the schema-refresh failure path now surfaces an error toast (previously silent) with all form state preserved.
- The JSON Forms `config` prop is memoized; the views options list no longer sorts React state in place during render.
- The edit modal's read-only structure tables paginate past 100 rows instead of rendering unboundedly.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — no semantic-layer provider extension is available in the OSS repo to drive the modal end-to-end (see TESTING INSTRUCTIONS). The user-visible change: the metrics picker shows a capped tag summary ("+ N" overflow) instead of one tag per selection, matching the views picker beside it; selection state and persistence are unchanged.
### TESTING INSTRUCTIONS
Unit tests use a provider-independent 500-value enum fixture (`testFixtures.ts`) — the crash mechanics are fully client-side, so no provider extension is required:
```
cd superset-frontend
npm run test -- features/semanticLayers/ features/semanticViews/
```
Covers: tag cap + "+ N" overflow at 500 selections with the full value array propagated, no Select-all affordance, duplicated display labels individually selectable, selections surviving identical schema refreshes, `const: []` no-loop, failed-refresh toast with selections preserved, the persistence payload carrying every selected metric, and 500-row structure tables rendering paginated in the edit modal (small structures stay unpaginated).
Manual verification in a deployment with a semantic-layer extension installed: create a Semantic View from a layer exposing hundreds of metrics and select them all — the picker stays responsive and the app does not crash; saving persists the full selection.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [x] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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](https://claude.com/claude-code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(semantic-layers): survive large multi-metric selection in Semantic View modal [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42535:
URL: https://github.com/apache/superset/pull/42535#issuecomment-5107797131
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(semantic-layers): survive large multi-metric selection in Semantic View modal [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42535:
URL: https://github.com/apache/superset/pull/42535#issuecomment-5122243938
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(semantic-layers): survive large multi-metric selection in Semantic View modal [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42535:
URL: https://github.com/apache/superset/pull/42535#issuecomment-5105560910
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(semantic-layers): survive large multi-metric selection in Semantic View modal [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on PR #42535:
URL: https://github.com/apache/superset/pull/42535#issuecomment-5105768726
Ran a six-lens local review panel (React/TS, Superset-committer, Preset-style, clean-code, tidy-first, continuous-delivery) over the initial implementation; zero blockers, and the hardening round landed in c1a856ecc9:
- **Schema-refresh dedupe is now serialization-order-proof**: `applyRuntimeSchema` compares a key-order-canonical serialization (`stableSerialize`, unit-tested), so a provider that reorders JSON properties between refreshes can't silently defeat the remount/AJV dedupe. Array order stays significant on purpose — `x-propertyOrder`/enum order are meaningful. (The failure mode was fail-safe either way: a missed dedupe only reverts to slower pre-fix behavior, never wrong data.)
- **Failed refresh handling**: the error is now logged, the dependency snapshot rolls back so the same dep state retries instead of staying stale, and the toast fires once per outage rather than once per debounced attempt.
- `valuesEqual` uses lodash `isEqual` (key-order-insensitive); `EnumNamesControl`/`MultiEnumControl` memo deps are identity-stable; `DynamicFieldControl` drops its manual sort (the wrapped Select alphabetises) and gained a 500-option regression test; a modal smoke test now runs at the 500-metric acceptance scale; `MAX_TAG_COUNT` is imported instead of hardcoded; `console.error` spies restore in `try/finally`.
Deliberately deferred (follow-up material, not blockers): fake-timer rework of the debounce-dependent tests (do if CI flakes), and extracting the duplicated modal-reset block / enum-options builders / shared test helpers.
_This comment was generated by Claude (AI) on behalf of @mikebridge._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(semantic-layers): survive large multi-metric selection in Semantic View modal [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #42535:
URL: https://github.com/apache/superset/pull/42535#discussion_r3706250214
##########
superset-frontend/src/features/semanticLayers/jsonFormsHelpers.tsx:
##########
@@ -342,38 +366,53 @@ export function MultiEnumControl(props: ControlProps) {
(arraySchema.items as Record<string, unknown>) ??
({} as Record<string, unknown>);
- const enumValues = (itemsSchema.enum as unknown[]) ?? [];
- const enumNames =
- (itemsSchema['x-enumNames'] as string[]) ?? enumValues.map(String);
+ // No fallback allocations out here: a fresh ``[]`` per render would give
+ // the memo new deps every time. Fallbacks live inside the memo.
+ const enumValues = Array.isArray(itemsSchema.enum)
+ ? (itemsSchema.enum as unknown[])
+ : undefined;
+ const enumNames = Array.isArray(itemsSchema['x-enumNames'])
+ ? (itemsSchema['x-enumNames'] as string[])
+ : undefined;
- const options = enumValues.map((value, index) => ({
- value: value as string | number,
- label: enumNames[index] ?? String(value),
- }));
+ // Memoized: the host form passes a fresh ``config`` to every control on
+ // each change, so without the memo a catalog of N options is rebuilt on
+ // every one of the user's M selections (O(N·M)).
+ const options = useMemo(
+ () =>
+ (enumValues ?? []).map((value, index) => ({
+ value: value as string | number,
+ label: enumNames?.[index] ?? String(value),
+ })),
+ [enumValues, enumNames],
+ );
Review Comment:
That cast predates this PR, MultiEnumControl already asserted `string | number` before the migration. This control only ever renders semantic-layer metric/dimension catalogs, which are always strings, so I don't think it's worth chasing down here.
##########
superset-frontend/src/features/semanticLayers/jsonFormsHelpers.tsx:
##########
@@ -342,38 +366,53 @@ export function MultiEnumControl(props: ControlProps) {
(arraySchema.items as Record<string, unknown>) ??
({} as Record<string, unknown>);
- const enumValues = (itemsSchema.enum as unknown[]) ?? [];
- const enumNames =
- (itemsSchema['x-enumNames'] as string[]) ?? enumValues.map(String);
+ // No fallback allocations out here: a fresh ``[]`` per render would give
+ // the memo new deps every time. Fallbacks live inside the memo.
+ const enumValues = Array.isArray(itemsSchema.enum)
+ ? (itemsSchema.enum as unknown[])
+ : undefined;
+ const enumNames = Array.isArray(itemsSchema['x-enumNames'])
+ ? (itemsSchema['x-enumNames'] as string[])
+ : undefined;
- const options = enumValues.map((value, index) => ({
- value: value as string | number,
- label: enumNames[index] ?? String(value),
- }));
+ // Memoized: the host form passes a fresh ``config`` to every control on
+ // each change, so without the memo a catalog of N options is rebuilt on
+ // every one of the user's M selections (O(N·M)).
+ const options = useMemo(
+ () =>
+ (enumValues ?? []).map((value, index) => ({
+ value: value as string | number,
+ label: enumNames?.[index] ?? String(value),
+ })),
+ [enumValues, enumNames],
+ );
const value = Array.isArray(props.data) ? (props.data as unknown[]) : [];
const tooltip = (props.uischema?.options as Record<string, unknown>)
?.tooltip as string | undefined;
return (
- <Form.Item label={props.label} tooltip={tooltip}>
+ <FormItem label={props.label} tooltip={tooltip}>
<Select
+ ariaLabel={props.label || undefined}
mode="multiple"
value={value as (string | number)[]}
onChange={next => props.handleChange(props.path, next)}
options={options}
- style={{ width: '100%' }}
disabled={!props.enabled}
loading={!!refreshingSchema}
allowClear
- optionFilterProp="label"
+ // Stability fix only: the wrapped Select's bulk select-all/clear
+ // affordance is out of scope for this control (sc-107832).
+ allowSelectAll={false}
+ optionFilterProps={['label']}
Review Comment:
The prior antd Select had `optionFilterProp="label"` too, so this isn't a behavior change, just the wrapped Select's array-based API for the same label-only filter.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(semantic-layers): survive large multi-metric selection in Semantic View modal [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #42535:
URL: https://github.com/apache/superset/pull/42535#issuecomment-5107247726
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/42535?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `86.59794%` with `13 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 65.33%. Comparing base ([`39f9b70`](https://app.codecov.io/gh/apache/superset/commit/39f9b7029be12ce740302e29d6d652dd571465c5?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`d9cf090`](https://app.codecov.io/gh/apache/superset/commit/d9cf090301469da04a637397ccf6eb0bbcfa0181?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 8 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/42535?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...d/src/features/semanticLayers/jsonFormsHelpers.tsx](https://app.codecov.io/gh/apache/superset/pull/42535?src=pr&el=tree&filepath=superset-frontend%2Fsrc%2Ffeatures%2FsemanticLayers%2FjsonFormsHelpers.tsx&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZlYXR1cmVzL3NlbWFudGljTGF5ZXJzL2pzb25Gb3Jtc0hlbHBlcnMudHN4) | 71.79% | [11 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/42535?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...rc/features/semanticViews/AddSemanticViewModal.tsx](https://app.codecov.io/gh/apache/superset/pull/42535?src=pr&el=tree&filepath=superset-frontend%2Fsrc%2Ffeatures%2FsemanticViews%2FAddSemanticViewModal.tsx&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZlYXR1cmVzL3NlbWFudGljVmlld3MvQWRkU2VtYW50aWNWaWV3TW9kYWwudHN4) | 96.07% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/42535?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #42535 +/- ##
==========================================
+ Coverage 65.27% 65.33% +0.05%
==========================================
Files 2795 2796 +1
Lines 157835 157904 +69
Branches 36080 36106 +26
==========================================
+ Hits 103027 103164 +137
+ Misses 52816 52748 -68
Partials 1992 1992
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/42535/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [javascript](https://app.codecov.io/gh/apache/superset/pull/42535/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `71.47% <86.59%> (+0.10%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/42535?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(semantic-layers): survive large multi-metric selection in Semantic View modal [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on PR #42535:
URL: https://github.com/apache/superset/pull/42535#issuecomment-5120236122
@rusackas could you take a look at this one? The reviewer-request API call failed for me due to a permissions gap on this repo, so tagging you here instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(semantic-layers): survive large multi-metric selection in Semantic View modal [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #42535:
URL: https://github.com/apache/superset/pull/42535
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(semantic-layers): survive large multi-metric selection in Semantic View modal [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #42535:
URL: https://github.com/apache/superset/pull/42535#discussion_r3706252682
##########
superset-frontend/src/features/semanticLayers/jsonFormsHelpers.tsx:
##########
@@ -342,38 +366,53 @@ export function MultiEnumControl(props: ControlProps) {
(arraySchema.items as Record<string, unknown>) ??
({} as Record<string, unknown>);
- const enumValues = (itemsSchema.enum as unknown[]) ?? [];
- const enumNames =
- (itemsSchema['x-enumNames'] as string[]) ?? enumValues.map(String);
+ // No fallback allocations out here: a fresh ``[]`` per render would give
+ // the memo new deps every time. Fallbacks live inside the memo.
+ const enumValues = Array.isArray(itemsSchema.enum)
+ ? (itemsSchema.enum as unknown[])
+ : undefined;
+ const enumNames = Array.isArray(itemsSchema['x-enumNames'])
+ ? (itemsSchema['x-enumNames'] as string[])
+ : undefined;
- const options = enumValues.map((value, index) => ({
- value: value as string | number,
- label: enumNames[index] ?? String(value),
- }));
+ // Memoized: the host form passes a fresh ``config`` to every control on
+ // each change, so without the memo a catalog of N options is rebuilt on
+ // every one of the user's M selections (O(N·M)).
+ const options = useMemo(
+ () =>
+ (enumValues ?? []).map((value, index) => ({
+ value: value as string | number,
+ label: enumNames?.[index] ?? String(value),
+ })),
+ [enumValues, enumNames],
+ );
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not flag boolean or null enum handling in this semantic-layer catalog control; its metric and dimension catalog values are always strings.
**Applied to:**
- `superset-frontend/src/features/semanticLayers/jsonFormsHelpers.tsx`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
##########
superset-frontend/src/features/semanticLayers/jsonFormsHelpers.tsx:
##########
@@ -342,38 +366,53 @@ export function MultiEnumControl(props: ControlProps) {
(arraySchema.items as Record<string, unknown>) ??
({} as Record<string, unknown>);
- const enumValues = (itemsSchema.enum as unknown[]) ?? [];
- const enumNames =
- (itemsSchema['x-enumNames'] as string[]) ?? enumValues.map(String);
+ // No fallback allocations out here: a fresh ``[]`` per render would give
+ // the memo new deps every time. Fallbacks live inside the memo.
+ const enumValues = Array.isArray(itemsSchema.enum)
+ ? (itemsSchema.enum as unknown[])
+ : undefined;
+ const enumNames = Array.isArray(itemsSchema['x-enumNames'])
+ ? (itemsSchema['x-enumNames'] as string[])
+ : undefined;
- const options = enumValues.map((value, index) => ({
- value: value as string | number,
- label: enumNames[index] ?? String(value),
- }));
+ // Memoized: the host form passes a fresh ``config`` to every control on
+ // each change, so without the memo a catalog of N options is rebuilt on
+ // every one of the user's M selections (O(N·M)).
+ const options = useMemo(
+ () =>
+ (enumValues ?? []).map((value, index) => ({
+ value: value as string | number,
+ label: enumNames?.[index] ?? String(value),
+ })),
+ [enumValues, enumNames],
+ );
const value = Array.isArray(props.data) ? (props.data as unknown[]) : [];
const tooltip = (props.uischema?.options as Record<string, unknown>)
?.tooltip as string | undefined;
return (
- <Form.Item label={props.label} tooltip={tooltip}>
+ <FormItem label={props.label} tooltip={tooltip}>
<Select
+ ariaLabel={props.label || undefined}
mode="multiple"
value={value as (string | number)[]}
onChange={next => props.handleChange(props.path, next)}
options={options}
- style={{ width: '100%' }}
disabled={!props.enabled}
loading={!!refreshingSchema}
allowClear
- optionFilterProp="label"
+ // Stability fix only: the wrapped Select's bulk select-all/clear
+ // affordance is out of scope for this control (sc-107832).
+ allowSelectAll={false}
+ optionFilterProps={['label']}
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not flag label-only option filtering in this control; the prior Select already intentionally searched by label only, so switching to the wrapped Select's equivalent API is not a behavior change.
**Applied to:**
- `superset-frontend/src/features/semanticLayers/jsonFormsHelpers.tsx`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(semantic-layers): survive large multi-metric selection in Semantic View modal [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #42535:
URL: https://github.com/apache/superset/pull/42535#discussion_r3676011874
##########
superset-frontend/src/features/semanticLayers/jsonFormsHelpers.tsx:
##########
@@ -342,38 +366,53 @@ export function MultiEnumControl(props: ControlProps) {
(arraySchema.items as Record<string, unknown>) ??
({} as Record<string, unknown>);
- const enumValues = (itemsSchema.enum as unknown[]) ?? [];
- const enumNames =
- (itemsSchema['x-enumNames'] as string[]) ?? enumValues.map(String);
+ // No fallback allocations out here: a fresh ``[]`` per render would give
+ // the memo new deps every time. Fallbacks live inside the memo.
+ const enumValues = Array.isArray(itemsSchema.enum)
+ ? (itemsSchema.enum as unknown[])
+ : undefined;
+ const enumNames = Array.isArray(itemsSchema['x-enumNames'])
+ ? (itemsSchema['x-enumNames'] as string[])
+ : undefined;
- const options = enumValues.map((value, index) => ({
- value: value as string | number,
- label: enumNames[index] ?? String(value),
- }));
+ // Memoized: the host form passes a fresh ``config`` to every control on
+ // each change, so without the memo a catalog of N options is rebuilt on
+ // every one of the user's M selections (O(N·M)).
+ const options = useMemo(
+ () =>
+ (enumValues ?? []).map((value, index) => ({
+ value: value as string | number,
+ label: enumNames?.[index] ?? String(value),
+ })),
+ [enumValues, enumNames],
+ );
const value = Array.isArray(props.data) ? (props.data as unknown[]) : [];
const tooltip = (props.uischema?.options as Record<string, unknown>)
?.tooltip as string | undefined;
return (
- <Form.Item label={props.label} tooltip={tooltip}>
+ <FormItem label={props.label} tooltip={tooltip}>
<Select
+ ariaLabel={props.label || undefined}
mode="multiple"
value={value as (string | number)[]}
onChange={next => props.handleChange(props.path, next)}
options={options}
- style={{ width: '100%' }}
disabled={!props.enabled}
loading={!!refreshingSchema}
allowClear
- optionFilterProp="label"
+ // Stability fix only: the wrapped Select's bulk select-all/clear
+ // affordance is out of scope for this control (sc-107832).
+ allowSelectAll={false}
+ optionFilterProps={['label']}
Review Comment:
**Suggestion:** Restricting filtering to `label` removes the wrapped Select's default `value` search field. For enum fields with human-readable `x-enumNames`, users who know the stored metric identifier or ID can no longer find the option by that value. Preserve both `label` and `value` in the filter properties. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Dynamic enum fields cannot search stored identifiers.
- ⚠️ ID-based discovery fails when labels differ from values.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3a2d802a099c444ab0d5039bee40186c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=3a2d802a099c444ab0d5039bee40186c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/features/semanticLayers/jsonFormsHelpers.tsx
**Line:** 409:409
**Comment:**
*Api Mismatch: Restricting filtering to `label` removes the wrapped Select's default `value` search field. For enum fields with human-readable `x-enumNames`, users who know the stored metric identifier or ID can no longer find the option by that value. Preserve both `label` and `value` in the filter properties.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42535&comment_hash=914b3528fab648917c36eb329f93ae1f2efbf7e5c4dc3b16e121c38246a732f1&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42535&comment_hash=914b3528fab648917c36eb329f93ae1f2efbf7e5c4dc3b16e121c38246a732f1&reaction=dislike'>👎</a>
##########
superset-frontend/src/features/semanticLayers/jsonFormsHelpers.tsx:
##########
@@ -342,38 +366,53 @@ export function MultiEnumControl(props: ControlProps) {
(arraySchema.items as Record<string, unknown>) ??
({} as Record<string, unknown>);
- const enumValues = (itemsSchema.enum as unknown[]) ?? [];
- const enumNames =
- (itemsSchema['x-enumNames'] as string[]) ?? enumValues.map(String);
+ // No fallback allocations out here: a fresh ``[]`` per render would give
+ // the memo new deps every time. Fallbacks live inside the memo.
+ const enumValues = Array.isArray(itemsSchema.enum)
+ ? (itemsSchema.enum as unknown[])
+ : undefined;
+ const enumNames = Array.isArray(itemsSchema['x-enumNames'])
+ ? (itemsSchema['x-enumNames'] as string[])
+ : undefined;
- const options = enumValues.map((value, index) => ({
- value: value as string | number,
- label: enumNames[index] ?? String(value),
- }));
+ // Memoized: the host form passes a fresh ``config`` to every control on
+ // each change, so without the memo a catalog of N options is rebuilt on
+ // every one of the user's M selections (O(N·M)).
+ const options = useMemo(
+ () =>
+ (enumValues ?? []).map((value, index) => ({
+ value: value as string | number,
+ label: enumNames?.[index] ?? String(value),
+ })),
+ [enumValues, enumNames],
+ );
Review Comment:
**Suggestion:** JSON Schema enums may legally contain booleans or `null`, but this option builder passes those values directly while asserting they are `string | number`. The wrapped Select and its value-handling helpers use the narrower value contract, so such options can be rejected, rendered incorrectly, or fail to round-trip their original type through `handleChange`. Normalize or explicitly support the full set of JSON Schema enum value types before using this renderer. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Boolean and null enum choices may not round-trip.
- ⚠️ JSON Forms fields can diverge from backend schema values.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6979cfb4deeb4546a3af5377a77e3bdb&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=6979cfb4deeb4546a3af5377a77e3bdb&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/features/semanticLayers/jsonFormsHelpers.tsx
**Line:** 381:388
**Comment:**
*Type Error: JSON Schema enums may legally contain booleans or `null`, but this option builder passes those values directly while asserting they are `string | number`. The wrapped Select and its value-handling helpers use the narrower value contract, so such options can be rejected, rendered incorrectly, or fail to round-trip their original type through `handleChange`. Normalize or explicitly support the full set of JSON Schema enum value types before using this renderer.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42535&comment_hash=45a3e0a374846082091c97046f30f49b062253796e0ec004d8f25492e6a08b6b&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42535&comment_hash=45a3e0a374846082091c97046f30f49b062253796e0ec004d8f25492e6a08b6b&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(semantic-layers): survive large multi-metric selection in Semantic View modal [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #42535:
URL: https://github.com/apache/superset/pull/42535#issuecomment-5120291468
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | e420e89c1905c9e6f6664a6885c4743f92c34890 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a6a224af5ed0c0008620df4 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-42535--superset-docs-preview.netlify.app](https://deploy-preview-42535--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcoxDoAgDADAv3SWpFZF4w98Ri0QTEQI4GCMf5fthnsh5RhShRWggz3zJb65iOpxXgZS4T7roYKt-RAlmYtvz9h0xmczbWrWTDSym6xBQcRFExo3wvcD) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-42535--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org