codeant-ai-for-open-source[bot] commented on code in PR #42602: URL: https://github.com/apache/superset/pull/42602#discussion_r3681021545
########## superset-frontend/packages/superset-core/src/theme/SupersetThemeProvider.test.tsx: ########## @@ -0,0 +1,160 @@ +/** + * 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. + */ +import { render, screen, act } from '@testing-library/react'; +import { theme as antdThemeImport } from 'antd'; +import { Theme } from './Theme'; + +// SupersetThemeProvider stores theme state via React.useState, then +// registers a listener (in a useEffect, on mount) that calls setThemeState +// whenever a *later* call to setConfig/toggleDarkMode runs on the same +// Theme instance. Every provider currently mounted from that instance +// listens independently, so toggling the instance updates all of them, not +// just the most recently rendered one. Consumers +// (docs/src/components/StorybookWrapper.jsx in particular) rely on this: +// they call toggleDarkMode() from outside, on one or more already-mounted +// providers sharing a single Theme instance, expecting it to propagate to +// all of them. +// +// The probe below reads the theme via antd's theme.useToken() -- the same +// context-consumption path every real antd component (Button, Input, ...) +// uses internally -- rather than reading themeObject.theme directly off the +// singleton. That distinction matters: React bails out of re-rendering a +// child whose element reference didn't change (the common "static children +// prop" case, true here since <Probe /> is passed once and never +// recreated), UNLESS that child consumes a React Context whose value +// changed, which bypasses the bail-out. A probe reading the plain object +// directly would misleadingly appear "not updated" even though every real +// themed component downstream re-renders correctly. +function makeProbe() { + let renderCount = 0; + let lastColorBgBase: string | undefined; + function Probe() { + const { token } = antdThemeImport.useToken(); + renderCount += 1; + lastColorBgBase = token.colorBgBase; + return <div data-test="probe" />; Review Comment: **Suggestion:** The probe renders `data-test="probe"`, but `getByTestId('probe')` searches for `data-testid="probe"`. Consequently, the first regression test throws before exercising `toggleDarkMode`, causing the newly added test suite to fail and masking the intended provider behavior. [api mismatch] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Superset theme regression test fails immediately. - ❌ Initial toggle propagation behavior remains unvalidated. - ⚠️ CI cannot pass with the new test suite. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=24257d278a524aadb2921f0af3e112fe&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=24257d278a524aadb2921f0af3e112fe&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/packages/superset-core/src/theme/SupersetThemeProvider.test.tsx **Line:** 51:51 **Comment:** *Api Mismatch: The probe renders `data-test="probe"`, but `getByTestId('probe')` searches for `data-testid="probe"`. Consequently, the first regression test throws before exercising `toggleDarkMode`, causing the newly added test suite to fail and masking the intended provider behavior. 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%2F42602&comment_hash=ee6b62f82dc27fddff8012dad5c0be815af506e1b0dc31cf907ae5828166ac00&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42602&comment_hash=ee6b62f82dc27fddff8012dad5c0be815af506e1b0dc31cf907ae5828166ac00&reaction=dislike'>👎</a> ########## docs/src/components/StorybookWrapper.jsx: ########## @@ -78,15 +80,39 @@ function getProviders() { return container || document.body; }; + // `themeObject` is a module-level singleton (superset-core/src/theme + // index.tsx: `Theme.fromConfig()`), created once with no dark/light + // config, so SupersetThemeProvider always rendered whatever that default + // algorithm was -- it had no way to know about Docusaurus's theme toggle. + // Docusaurus tracks the toggle in React context (useColorMode), so + // mirror it onto the singleton via the toggleDarkMode() method Theme + // already exposes for exactly this purpose. + // + // Use useLayoutEffect (not useEffect) so the sync runs before the + // browser paints. This component only ever mounts client-side (it's + // built inside a BrowserOnly callback), so there's no SSR mismatch + // concern -- and running synchronously before paint avoids a brief + // flash of the singleton's previous palette when a page loads directly + // in dark mode or the toggle fires during route navigation. + function ThemeSync({ children }) { + const { colorMode } = useColorMode(); + React.useLayoutEffect(() => { + themeObject.toggleDarkMode(colorMode === 'dark'); + }, [colorMode]); Review Comment: **Suggestion:** The layout effect updates the singleton before `SupersetThemeProvider` registers its listener in a passive `useEffect`. On an initial dark-mode render, the provider's state is initialized from the old light/default palette, the notification has no subscribers, and the provider remains light until a later toggle. Synchronize the provider's initial state with the current theme or register the listener before performing the initial toggle. [stale reference] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Live component demos initially render with the wrong palette. - ⚠️ Direct dark-mode navigation requires a second toggle. - ⚠️ All providers sharing the singleton can start stale. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1f76876f44b549c28be01a2ea5dfcae1&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=1f76876f44b549c28be01a2ea5dfcae1&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:** docs/src/components/StorybookWrapper.jsx **Line:** 99:101 **Comment:** *Stale Reference: The layout effect updates the singleton before `SupersetThemeProvider` registers its listener in a passive `useEffect`. On an initial dark-mode render, the provider's state is initialized from the old light/default palette, the notification has no subscribers, and the provider remains light until a later toggle. Synchronize the provider's initial state with the current theme or register the listener before performing the initial toggle. 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%2F42602&comment_hash=60bd178344c8a2ee6b8e8705a6282f22ac9fcf1499a3e2aa7a44cbe8643ca55f&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42602&comment_hash=60bd178344c8a2ee6b8e8705a6282f22ac9fcf1499a3e2aa7a44cbe8643ca55f&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]
