korbit-ai[bot] commented on code in PR #34560: URL: https://github.com/apache/superset/pull/34560#discussion_r2270915032
########## superset/migrations/versions/2025-08-05_14-38_c233f5365c9e_is_system_default_is_system_dark.py: ########## @@ -0,0 +1,87 @@ +# 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. +"""is_system_default-is_system_dark + +Revision ID: c233f5365c9e +Revises: cd1fb11291f2 +Create Date: 2025-08-05 14:38:55.782777 + +""" + +import sqlalchemy as sa +from alembic import op +from sqlalchemy import false + +from superset.migrations.shared.utils import ( + add_columns, + create_index, + drop_columns, + drop_index, +) + +# revision identifiers, used by Alembic. +revision = "c233f5365c9e" +down_revision = "cd1fb11291f2" + + +def upgrade(): + # Add columns as nullable first + add_columns( + "themes", + sa.Column("is_system_default", sa.Boolean(), nullable=True), + sa.Column("is_system_dark", sa.Boolean(), nullable=True), + ) + + # Update existing rows to have False values using SQLAlchemy + connection = op.get_bind() + themes_table = sa.table( + "themes", + sa.column("is_system_default", sa.Boolean()), + sa.column("is_system_dark", sa.Boolean()), + ) + + connection.execute( + themes_table.update() + .where(themes_table.c.is_system_default.is_(None)) + .values(is_system_default=false()) + ) Review Comment: ### Missing Transaction in Theme Migration Updates <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The update statements are not wrapped in a transaction, which could lead to partial updates if the migration fails midway. ###### Why this matters If the migration fails after the first update but before the second one, the database will be left in an inconsistent state with only one column updated. ###### Suggested change ∙ *Feature Preview* Wrap both update statements in a transaction: ```python with op.get_context().begin_transaction(): connection.execute( themes_table.update() .where(themes_table.c.is_system_default.is_(None)) .values(is_system_default=false()) ) connection.execute( themes_table.update() .where(themes_table.c.is_system_dark.is_(None)) .values(is_system_dark=false()) ) ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/45836de3-d77f-4342-97f8-fad60709b469/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/45836de3-d77f-4342-97f8-fad60709b469?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/45836de3-d77f-4342-97f8-fad60709b469?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/45836de3-d77f-4342-97f8-fad60709b469?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/45836de3-d77f-4342-97f8-fad60709b469) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:e5dd38eb-663c-4f51-bc09-18393b96c65d --> [](e5dd38eb-663c-4f51-bc09-18393b96c65d) ########## superset-frontend/src/pages/ThemeList/index.tsx: ########## @@ -111,6 +119,13 @@ function ThemesList({ const canImport = hasPerm('can_write'); const canApply = hasPerm('can_write'); // Only users with write permission can apply themes + // Get theme settings from bootstrap data + const bootstrapData = getBootstrapData(); + const themeData = bootstrapData?.common?.theme || {}; Review Comment: ### Unsafe Type Casting of Security-Critical Permission Data <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The themeData object is being type cast with 'as any' which bypasses TypeScript's type checking when accessing enableUiThemeAdministration. ###### Why this matters Using 'as any' to bypass type checking could allow unauthorized access to theme administration features if the bootstrap data was tampered with client-side. ###### Suggested change ∙ *Feature Preview* ```typescript interface ThemeData { enableUiThemeAdministration?: boolean; } const themeData = (bootstrapData?.common?.theme || {}) as ThemeData; ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6a07c9a7-51e9-4209-ad1c-3df0593d9900/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6a07c9a7-51e9-4209-ad1c-3df0593d9900?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6a07c9a7-51e9-4209-ad1c-3df0593d9900?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6a07c9a7-51e9-4209-ad1c-3df0593d9900?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6a07c9a7-51e9-4209-ad1c-3df0593d9900) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:deaa8e2b-60c0-4ad0-b593-3efa6a5952a8 --> [](deaa8e2b-60c0-4ad0-b593-3efa6a5952a8) ########## superset/migrations/versions/2025-08-05_14-38_c233f5365c9e_is_system_default_is_system_dark.py: ########## @@ -0,0 +1,87 @@ +# 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. +"""is_system_default-is_system_dark + +Revision ID: c233f5365c9e +Revises: cd1fb11291f2 +Create Date: 2025-08-05 14:38:55.782777 + +""" + +import sqlalchemy as sa +from alembic import op +from sqlalchemy import false + +from superset.migrations.shared.utils import ( + add_columns, + create_index, + drop_columns, + drop_index, +) + +# revision identifiers, used by Alembic. +revision = "c233f5365c9e" +down_revision = "cd1fb11291f2" + + +def upgrade(): + # Add columns as nullable first + add_columns( + "themes", + sa.Column("is_system_default", sa.Boolean(), nullable=True), + sa.Column("is_system_dark", sa.Boolean(), nullable=True), + ) + + # Update existing rows to have False values using SQLAlchemy + connection = op.get_bind() + themes_table = sa.table( + "themes", + sa.column("is_system_default", sa.Boolean()), + sa.column("is_system_dark", sa.Boolean()), + ) + + connection.execute( + themes_table.update() + .where(themes_table.c.is_system_default.is_(None)) + .values(is_system_default=false()) + ) + + connection.execute( + themes_table.update() + .where(themes_table.c.is_system_dark.is_(None)) + .values(is_system_dark=false()) + ) + + # Now make the columns non-nullable + # MySQL requires explicit type specification for CHANGE/MODIFY operations + with op.batch_alter_table("themes") as batch_op: + batch_op.alter_column( + "is_system_default", existing_type=sa.Boolean(), nullable=False + ) + batch_op.alter_column( + "is_system_dark", existing_type=sa.Boolean(), nullable=False + ) + + # Create indexes + create_index("themes", "idx_theme_is_system_default", ["is_system_default"]) + create_index("themes", "idx_theme_is_system_dark", ["is_system_dark"]) + + +def downgrade(): + drop_index("themes", "idx_theme_is_system_default") + drop_index("themes", "idx_theme_is_system_dark") + drop_columns("themes", "is_system_dark", "is_system_default") Review Comment: ### Data Loss in Theme Migration Downgrade <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The downgrade function does not preserve the data from the dropped columns, which could lead to data loss if a rollback is needed. ###### Why this matters If the migration needs to be rolled back, all theme configuration data for system default and dark mode will be permanently lost. ###### Suggested change ∙ *Feature Preview* Add data backup before dropping columns: ```python def downgrade(): # Optionally backup data if needed for future migrations connection = op.get_bind() themes_table = sa.Table('themes', sa.MetaData(), sa.Column('is_system_default', sa.Boolean()), sa.Column('is_system_dark', sa.Boolean()) ) backup_data = connection.execute( sa.select([ themes_table.c.is_system_default, themes_table.c.is_system_dark ]) ).fetchall() # Store backup data in a new table or file if needed drop_index("themes", "idx_theme_is_system_default") drop_index("themes", "idx_theme_is_system_dark") drop_columns("themes", "is_system_dark", "is_system_default") ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7ca8d4bd-189a-46b1-af05-9c170e0504f7/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7ca8d4bd-189a-46b1-af05-9c170e0504f7?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7ca8d4bd-189a-46b1-af05-9c170e0504f7?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7ca8d4bd-189a-46b1-af05-9c170e0504f7?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7ca8d4bd-189a-46b1-af05-9c170e0504f7) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:57544526-3c28-4509-be1c-762e72b745b7 --> [](57544526-3c28-4509-be1c-762e72b745b7) ########## superset-frontend/src/theme/ThemeController.ts: ########## @@ -128,18 +119,15 @@ export class ThemeController { const { bootstrapDefaultTheme, bootstrapDarkTheme, - bootstrapThemeSettings, hasCustomThemes, }: BootstrapThemeData = this.loadBootstrapData(); this.hasCustomThemes = hasCustomThemes; - this.themeSettings = bootstrapThemeSettings || {}; // Set themes based on bootstrap data availability if (this.hasCustomThemes) { - this.darkTheme = bootstrapDarkTheme || bootstrapDefaultTheme || null; - this.defaultTheme = - bootstrapDefaultTheme || bootstrapDarkTheme || defaultTheme; + this.darkTheme = bootstrapDarkTheme; + this.defaultTheme = bootstrapDefaultTheme || defaultTheme; } else { Review Comment: ### Missing Dark Theme Null Check <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The controller sets darkTheme unconditionally when hasCustomThemes is true, without checking if bootstrapDarkTheme is null, which could lead to unexpected behavior. ###### Why this matters This could cause the dark theme functionality to appear available when it's not, leading to theme switching UI being enabled when it shouldn't be. ###### Suggested change ∙ *Feature Preview* Add a null check for bootstrapDarkTheme to ensure proper initialization: ```typescript if (this.hasCustomThemes) { this.darkTheme = bootstrapDarkTheme || null; this.defaultTheme = bootstrapDefaultTheme || defaultTheme; } ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cd5909c9-8dec-4034-ad0d-beb41ebf764c/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cd5909c9-8dec-4034-ad0d-beb41ebf764c?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cd5909c9-8dec-4034-ad0d-beb41ebf764c?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cd5909c9-8dec-4034-ad0d-beb41ebf764c?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cd5909c9-8dec-4034-ad0d-beb41ebf764c) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:e8900e4b-4288-4f4d-ad81-bd702ef9d2db --> [](e8900e4b-4288-4f4d-ad81-bd702ef9d2db) -- 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-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
