korbit-ai[bot] commented on code in PR #33116:
URL: https://github.com/apache/superset/pull/33116#discussion_r2041213826
##########
superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/customControls.tsx:
##########
@@ -57,9 +57,7 @@ export const contributionModeControl = {
};
const xAxisSortVisibility = ({ controls }: { controls: ControlStateMapping })
=>
- isSortable(controls) &&
- ensureIsArray(controls?.groupby?.value).length === 0 &&
- ensureIsArray(controls?.metrics?.value).length === 1;
+ isSortable(controls);
Review Comment:
### Over-permissive sort control visibility <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The visibility condition for x-axis sorting has been simplified to only
check isSortable(controls), removing previous checks for groupby length and
metrics length. This could expose sorting controls in scenarios where they
weren't previously available.
###### Why this matters
The removal of these conditions might lead to inappropriate sorting options
being displayed in visualizations where they don't make sense, potentially
causing confusion or incorrect data representation.
###### Suggested change ∙ *Feature Preview*
Restore the appropriate visibility conditions while maintaining the
consolidated approach:
```typescript
const xAxisSortVisibility = ({ controls }: { controls: ControlStateMapping
}) =>
isSortable(controls) && (
// Single sort case
(ensureIsArray(controls?.groupby?.value).length === 0 &&
ensureIsArray(controls?.metrics?.value).length === 1) ||
// Multi sort case
(!!ensureIsArray(controls?.groupby?.value).length ||
ensureIsArray(controls?.metrics?.value).length > 1)
);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e0e4fe38-bbcd-42f9-9abf-aa71a3e4c95f/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e0e4fe38-bbcd-42f9-9abf-aa71a3e4c95f?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e0e4fe38-bbcd-42f9-9abf-aa71a3e4c95f?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e0e4fe38-bbcd-42f9-9abf-aa71a3e4c95f?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e0e4fe38-bbcd-42f9-9abf-aa71a3e4c95f)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:2b3ac200-c321-47e4-ae2f-3cb7050bf219 -->
[](2b3ac200-c321-47e4-ae2f-3cb7050bf219)
##########
superset/migrations/versions/2025-04-13_22-10_378cecfdba9f_merge_x_axis_sort_series_with_x_axis_.py:
##########
@@ -0,0 +1,120 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""merge_x_axis_sort_series_with_x_axis_sort
+
+Revision ID: 378cecfdba9f
+Revises: 32bf93dfe2a4
+Create Date: 2025-04-13 22:10:10.836273
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = "378cecfdba9f"
+down_revision = "32bf93dfe2a4"
+
+from alembic import op # noqa: E402
+from sqlalchemy import Column, Integer, String, Text # noqa: E402
+from sqlalchemy.ext.declarative import declarative_base # noqa: E402
+
+from superset import db # noqa: E402
+from superset.migrations.shared.utils import paginated_update # noqa: E402
+from superset.utils import json # noqa: E402
+
+Base = declarative_base()
+
+
+class Slice(Base):
+ __tablename__ = "slices"
+
+ id = Column(Integer, primary_key=True)
+ params = Column(Text)
+ viz_type = Column(String(250))
+
+
+timeseries_charts = [
+ "echarts_timeseries_bar",
+ "echarts_area",
+ "echarts_timeseries_line",
+ "echarts_timeseries_scatter",
+ "echarts_timeseries_smooth",
+ "echarts_timeseries_step",
+ "echarts_timeseries",
+]
+
+
+def upgrade():
+ bind = op.get_bind()
+ session = db.Session(bind=bind)
+ for slc in paginated_update(
+ session.query(Slice).filter(Slice.viz_type.in_(timeseries_charts))
+ ):
+ try:
+ params = json.loads(slc.params)
+
+ # x_axis_sort_series only appears when there are multiple metrics
or groupby
+ if not (
+ ("metrics" in params and len(params["metrics"]) > 1)
+ or ("groupby" in params and len(params["groupby"]) > 0)
+ ):
+ continue
+
+ if "x_axis_sort_series" in params:
+ if params["x_axis_sort_series"]:
+ params["x_axis_sort"] = params["x_axis_sort_series"]
+
+ del params["x_axis_sort_series"]
+ if "x_axis_sort_series_ascending" in params:
+ params["x_axis_sort_asc"] =
params["x_axis_sort_series_ascending"]
+ del params["x_axis_sort_series_ascending"]
+
+ slc.params = json.dumps(params, sort_keys=True)
+ except Exception: # noqa: S110
+ pass
Review Comment:
### Silent Migration Failures <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Silent failure in migration code could hide critical errors and leave the
database in an inconsistent state.
###### Why this matters
If any errors occur during the migration of a slice's parameters, they are
silently ignored, which could lead to partial or failed migrations without any
indication of the problem.
###### Suggested change ∙ *Feature Preview*
At minimum, log the errors to track failed migrations:
```python
except Exception as e: # noqa: S110
logging.error(f"Failed to migrate slice {slc.id}: {str(e)}")
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b5774065-f5e1-4ec8-9b66-f51437d6c830/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b5774065-f5e1-4ec8-9b66-f51437d6c830?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b5774065-f5e1-4ec8-9b66-f51437d6c830?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b5774065-f5e1-4ec8-9b66-f51437d6c830?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b5774065-f5e1-4ec8-9b66-f51437d6c830)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:89f853cc-f5a4-4fbe-ac91-543e93ce9279 -->
[](89f853cc-f5a4-4fbe-ac91-543e93ce9279)
##########
superset/migrations/versions/2025-04-13_22-10_378cecfdba9f_merge_x_axis_sort_series_with_x_axis_.py:
##########
@@ -0,0 +1,120 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""merge_x_axis_sort_series_with_x_axis_sort
+
+Revision ID: 378cecfdba9f
+Revises: 32bf93dfe2a4
+Create Date: 2025-04-13 22:10:10.836273
+
+"""
Review Comment:
### Missing migration purpose in docstring <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The migration docstring only states what action is being performed without
explaining why this change is necessary.
###### Why this matters
Future maintainers won't understand the rationale behind this parameter
consolidation, making it harder to maintain or debug related changes.
###### Suggested change ∙ *Feature Preview*
"""merge_x_axis_sort_series_with_x_axis_sort
Revision ID: 378cecfdba9f
Revises: 32bf93dfe2a4
Create Date: 2025-04-13 22:10:10.836273
Consolidates x-axis sorting parameters in timeseries charts by merging
'x_axis_sort_series' into 'x_axis_sort' for better parameter consistency.
"""
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b8ebc135-01df-4c88-871f-abef2dc27dbe/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b8ebc135-01df-4c88-871f-abef2dc27dbe?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b8ebc135-01df-4c88-871f-abef2dc27dbe?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b8ebc135-01df-4c88-871f-abef2dc27dbe?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b8ebc135-01df-4c88-871f-abef2dc27dbe)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:dfd6f1a3-99b4-4ddf-bb66-2f2ff63c01d7 -->
[](dfd6f1a3-99b4-4ddf-bb66-2f2ff63c01d7)
--
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]
