codeant-ai-for-open-source[bot] commented on code in PR #38519:
URL: https://github.com/apache/superset/pull/38519#discussion_r2913354972
##########
superset-frontend/packages/superset-ui-core/src/chart/types/matrixify.ts:
##########
@@ -139,75 +141,85 @@ export interface MatrixifyConfig {
columns: MatrixifyAxisConfig;
}
+/**
+ * Check if a given axis mode is active (not disabled)
+ */
+function isAxisEnabled(mode?: MatrixifyMode): boolean {
+ return mode === 'metrics' || mode === 'dimensions';
+}
+
/**
* Helper function to extract Matrixify configuration from form data
*/
export function getMatrixifyConfig(
formData: MatrixifyFormData & any,
): MatrixifyConfig | null {
- const hasRowConfig = formData.matrixify_mode_rows;
- const hasColumnConfig = formData.matrixify_mode_columns;
+ const rowEnabled = isAxisEnabled(formData.matrixify_mode_rows);
+ const colEnabled = isAxisEnabled(formData.matrixify_mode_columns);
- if (!hasRowConfig && !hasColumnConfig) {
+ if (!rowEnabled && !colEnabled) {
return null;
}
return {
rows: {
- mode: formData.matrixify_mode_rows || 'metrics',
+ mode: formData.matrixify_mode_rows || 'disabled',
metrics: formData.matrixify_rows,
selectionMode: formData.matrixify_dimension_selection_mode_rows,
dimension: formData.matrixify_dimension_rows,
topnValue: formData.matrixify_topn_value_rows,
topnMetric: formData.matrixify_topn_metric_rows,
- topnOrder: formData.matrixify_topn_order_rows,
+ topnOrder: formData.matrixify_topn_order_rows === false ? 'asc' : 'desc',
},
columns: {
- mode: formData.matrixify_mode_columns || 'metrics',
+ mode: formData.matrixify_mode_columns || 'disabled',
metrics: formData.matrixify_columns,
selectionMode: formData.matrixify_dimension_selection_mode_columns,
dimension: formData.matrixify_dimension_columns,
topnValue: formData.matrixify_topn_value_columns,
topnMetric: formData.matrixify_topn_metric_columns,
- topnOrder: formData.matrixify_topn_order_columns,
+ topnOrder:
+ formData.matrixify_topn_order_columns === false ? 'asc' : 'desc',
Review Comment:
**Suggestion:** The column Top N sort order is also derived by treating
`matrixify_topn_order_columns` as a boolean, which means a configured value of
`'asc'` (or any non-`false` value) is always interpreted as `'desc'`, so users
can never actually get ascending order; adjust this mapping to correctly handle
both string (`'asc'`/`'desc'`) and legacy boolean values. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Matrixify column Top N ascending sort cannot be applied.
- ⚠️ Ascending column order settings are effectively ignored.
- ⚠️ Column rankings may be reversed from what users selected.
```
</details>
```suggestion
topnOrder:
formData.matrixify_topn_order_columns === 'asc' ||
formData.matrixify_topn_order_columns === false
? 'asc'
: 'desc',
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create or edit a chart whose form data uses Matrixify column Top N
options, and
configure `matrixify_topn_order_columns: 'asc'` in the chart configuration
(similar to the
existing example in
`superset/examples/misc_charts/charts/Unicode_Cloud.yaml:33-51`, where
`matrixify_topn_order_columns` is stored as a string).
2. When Matrixify processes this form data, `getMatrixifyConfig` in
`superset-frontend/packages/superset-ui-core/src/chart/types/matrixify.ts:154-185`
builds
the column axis config and sets `topnOrder` via the logic at lines 181-182:
`topnOrder:
formData.matrixify_topn_order_columns === false ? 'asc' : 'desc'`.
3. With `formData.matrixify_topn_order_columns` equal to `'asc'`, the strict
comparison to
`false` evaluates to `false`, so the resulting `config.columns.topnOrder` is
`'desc'`,
contradicting the ascending direction stored in form data.
4. Any code that uses `config.columns.topnOrder` to determine which column
dimension
values are included and in what order (e.g., in Matrixify grid generation
that consumes
`MatrixifyFormData`) will therefore always treat a user's ascending choice
as descending,
preventing ascending Top N ordering for columns.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/packages/superset-ui-core/src/chart/types/matrixify.ts
**Line:** 181:182
**Comment:**
*Logic Error: The column Top N sort order is also derived by treating
`matrixify_topn_order_columns` as a boolean, which means a configured value of
`'asc'` (or any non-`false` value) is always interpreted as `'desc'`, so users
can never actually get ascending order; adjust this mapping to correctly handle
both string (`'asc'`/`'desc'`) and legacy boolean values.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38519&comment_hash=8fde0c718ce9e1cbfb37ef9a10d4b6efe7f6271e8aad756b5267a9cc6b98fd67&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38519&comment_hash=8fde0c718ce9e1cbfb37ef9a10d4b6efe7f6271e8aad756b5267a9cc6b98fd67&reaction=dislike'>👎</a>
##########
superset-frontend/packages/superset-ui-core/src/chart/types/matrixify.ts:
##########
@@ -139,75 +141,85 @@ export interface MatrixifyConfig {
columns: MatrixifyAxisConfig;
}
+/**
+ * Check if a given axis mode is active (not disabled)
+ */
+function isAxisEnabled(mode?: MatrixifyMode): boolean {
+ return mode === 'metrics' || mode === 'dimensions';
+}
+
/**
* Helper function to extract Matrixify configuration from form data
*/
export function getMatrixifyConfig(
formData: MatrixifyFormData & any,
): MatrixifyConfig | null {
- const hasRowConfig = formData.matrixify_mode_rows;
- const hasColumnConfig = formData.matrixify_mode_columns;
+ const rowEnabled = isAxisEnabled(formData.matrixify_mode_rows);
+ const colEnabled = isAxisEnabled(formData.matrixify_mode_columns);
- if (!hasRowConfig && !hasColumnConfig) {
+ if (!rowEnabled && !colEnabled) {
return null;
}
return {
rows: {
- mode: formData.matrixify_mode_rows || 'metrics',
+ mode: formData.matrixify_mode_rows || 'disabled',
metrics: formData.matrixify_rows,
selectionMode: formData.matrixify_dimension_selection_mode_rows,
dimension: formData.matrixify_dimension_rows,
topnValue: formData.matrixify_topn_value_rows,
topnMetric: formData.matrixify_topn_metric_rows,
- topnOrder: formData.matrixify_topn_order_rows,
+ topnOrder: formData.matrixify_topn_order_rows === false ? 'asc' : 'desc',
},
columns: {
- mode: formData.matrixify_mode_columns || 'metrics',
+ mode: formData.matrixify_mode_columns || 'disabled',
metrics: formData.matrixify_columns,
selectionMode: formData.matrixify_dimension_selection_mode_columns,
dimension: formData.matrixify_dimension_columns,
topnValue: formData.matrixify_topn_value_columns,
topnMetric: formData.matrixify_topn_metric_columns,
- topnOrder: formData.matrixify_topn_order_columns,
+ topnOrder:
+ formData.matrixify_topn_order_columns === false ? 'asc' : 'desc',
},
};
}
-/**
- * Check if Matrixify is enabled and properly configured
- */
export function isMatrixifyEnabled(formData: MatrixifyFormData): boolean {
- // Check if either vertical or horizontal layout is enabled
- const hasVerticalLayout = formData.matrixify_enable_vertical_layout === true;
- const hasHorizontalLayout =
- formData.matrixify_enable_horizontal_layout === true;
+ if (formData.matrixify_enable !== true) {
Review Comment:
**Suggestion:** The new global enable switch is treated as strictly required
(`matrixify_enable` must be `true`), which means any existing saved charts that
have valid matrixify row/column configuration but no `matrixify_enable` field
(i.e., all charts created before this feature) will now be treated as having
matrixify disabled, so the matrix grid will never render for them until users
manually edit and resave each chart; for backward compatibility, the code
should only treat an explicit `false` as disabling matrixify and treat
`undefined` as "legacy enabled" when the axis modes/configuration are valid.
[logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Existing matrixify-configured charts lose matrix grid behavior.
- ⚠️ Users must manually resave charts to re-enable Matrixify.
- ⚠️ Explore header no longer shows Matrixified tag for legacy charts.
```
</details>
```suggestion
// Explicitly turning the switch off disables Matrixify,
// but a missing switch (legacy charts) should still be treated
// as enabled when there is valid axis configuration.
if (formData.matrixify_enable === false) {
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Seed or load example charts so that the Unicode Cloud slice is created;
its
configuration is defined in
`superset/examples/misc_charts/charts/Unicode_Cloud.yaml:22-52`, which
includes multiple
`matrixify_*` fields (e.g. `matrixify_mode_rows`, `matrixify_mode_columns`,
`matrixify_topn_value_rows`) but does not define `matrixify_enable`.
2. Open this chart in Explore; `ExploreViewContainer.mapStateToProps` in
`superset-frontend/src/explore/components/ExploreViewContainer/index.tsx:1096-1205`
builds
`form_data` from the saved slice form_data, so the runtime `form_data`
passed around
contains the `matrixify_*` fields from the YAML but still no
`matrixify_enable` property.
3. Trigger a query from Explore (Run button or Ctrl+Enter), which calls
`onQuery` in
`ExploreViewContainer/index.tsx:460-479`; this checks
`isMatrixifyEnabled(props.form_data
as MatrixifyFormData)` at line 463 to decide whether to skip the main query
for Matrixify.
4. In
`superset-frontend/packages/superset-ui-core/src/chart/types/matrixify.ts:187-225`,
`isMatrixifyEnabled` immediately executes `if (formData.matrixify_enable !==
true) {
return false; }`, so for this legacy chart (where `matrixify_enable` is
`undefined`) it
returns `false`; as a result, SuperChart (`SuperChart.tsx:191-205`) never
renders the
`MatrixifyGridRenderer`, and Explore header
(`ExploreChartHeader/index.tsx:310-312`) never
shows the "Matrixified" tag, effectively disabling Matrixify for all
pre-existing charts
until users manually edit them and toggle the new switch on.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/packages/superset-ui-core/src/chart/types/matrixify.ts
**Line:** 188:188
**Comment:**
*Logic Error: The new global enable switch is treated as strictly
required (`matrixify_enable` must be `true`), which means any existing saved
charts that have valid matrixify row/column configuration but no
`matrixify_enable` field (i.e., all charts created before this feature) will
now be treated as having matrixify disabled, so the matrix grid will never
render for them until users manually edit and resave each chart; for backward
compatibility, the code should only treat an explicit `false` as disabling
matrixify and treat `undefined` as "legacy enabled" when the axis
modes/configuration are valid.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38519&comment_hash=b99bb106c85a400115832f6f42002ca2d046cde2db4a199754d8a85311a13892&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38519&comment_hash=b99bb106c85a400115832f6f42002ca2d046cde2db4a199754d8a85311a13892&reaction=dislike'>👎</a>
##########
superset-frontend/packages/superset-ui-core/src/chart/types/matrixify.ts:
##########
@@ -139,75 +141,85 @@ export interface MatrixifyConfig {
columns: MatrixifyAxisConfig;
}
+/**
+ * Check if a given axis mode is active (not disabled)
+ */
+function isAxisEnabled(mode?: MatrixifyMode): boolean {
+ return mode === 'metrics' || mode === 'dimensions';
+}
+
/**
* Helper function to extract Matrixify configuration from form data
*/
export function getMatrixifyConfig(
formData: MatrixifyFormData & any,
): MatrixifyConfig | null {
- const hasRowConfig = formData.matrixify_mode_rows;
- const hasColumnConfig = formData.matrixify_mode_columns;
+ const rowEnabled = isAxisEnabled(formData.matrixify_mode_rows);
+ const colEnabled = isAxisEnabled(formData.matrixify_mode_columns);
- if (!hasRowConfig && !hasColumnConfig) {
+ if (!rowEnabled && !colEnabled) {
return null;
}
return {
rows: {
- mode: formData.matrixify_mode_rows || 'metrics',
+ mode: formData.matrixify_mode_rows || 'disabled',
metrics: formData.matrixify_rows,
selectionMode: formData.matrixify_dimension_selection_mode_rows,
dimension: formData.matrixify_dimension_rows,
topnValue: formData.matrixify_topn_value_rows,
topnMetric: formData.matrixify_topn_metric_rows,
- topnOrder: formData.matrixify_topn_order_rows,
+ topnOrder: formData.matrixify_topn_order_rows === false ? 'asc' : 'desc',
Review Comment:
**Suggestion:** The row Top N sort order is derived by treating
`matrixify_topn_order_rows` as a boolean (`=== false ? 'asc' : 'desc'`), but in
existing configurations (e.g. YAML examples) this field can be the string
`'asc'` or `'desc'`; with the current logic, `'asc'` (and any non-`false`
value) incorrectly maps to `'desc'`, so ascending order can never be selected.
Update the mapping to correctly interpret both string (`'asc'`/`'desc'`) and
legacy boolean values. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Matrixify row Top N ascending sort cannot be achieved.
- ⚠️ User-configured ascending row order is silently ignored.
- ⚠️ Row rankings may appear reversed from user expectations.
```
</details>
```suggestion
topnOrder:
formData.matrixify_topn_order_rows === 'asc' ||
formData.matrixify_topn_order_rows === false
? 'asc'
: 'desc',
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create or edit a chart whose form data includes Matrixify row Top N
settings, and set
`matrixify_topn_order_rows: 'asc'` in the chart configuration (this mirrors
how similar
fields are stored in
`superset/examples/misc_charts/charts/Unicode_Cloud.yaml:33-51`,
where `matrixify_topn_order_rows` is persisted as a string value).
2. When the Matrixify code processes form data, `getMatrixifyConfig` in
`superset-frontend/packages/superset-ui-core/src/chart/types/matrixify.ts:154-185`
is
called (it is exported and used by other Matrixify utilities and tests), and
it constructs
the row axis config, including `topnOrder:
formData.matrixify_topn_order_rows === false ?
'asc' : 'desc'` at line 172.
3. With `formData.matrixify_topn_order_rows` equal to the string `'asc'`,
the strict
comparison to the boolean `false` evaluates to `false`, so `topnOrder` in
the returned
config is set to `'desc'` despite the ascending value in form data.
4. Any downstream logic that uses `config.rows.topnOrder` to decide the sort
direction for
Top N rows (e.g., in the Matrixify grid generation that imports
`MatrixifyFormData` as
seen via `MatrixifyGridGenerator.ts:29,111,209`) will therefore always treat
the
configuration as descending, making an ascending row Top N selection
impossible in
practice.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/packages/superset-ui-core/src/chart/types/matrixify.ts
**Line:** 172:172
**Comment:**
*Logic Error: The row Top N sort order is derived by treating
`matrixify_topn_order_rows` as a boolean (`=== false ? 'asc' : 'desc'`), but in
existing configurations (e.g. YAML examples) this field can be the string
`'asc'` or `'desc'`; with the current logic, `'asc'` (and any non-`false`
value) incorrectly maps to `'desc'`, so ascending order can never be selected.
Update the mapping to correctly interpret both string (`'asc'`/`'desc'`) and
legacy boolean values.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38519&comment_hash=7a99a1ab21e5620e44f7f6d0eb2184716e345bc6e6daa640f8143f49f6a41907&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38519&comment_hash=7a99a1ab21e5620e44f7f6d0eb2184716e345bc6e6daa640f8143f49f6a41907&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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
