codeant-ai-for-open-source[bot] commented on code in PR #42602:
URL: https://github.com/apache/superset/pull/42602#discussion_r3679366692
##########
docs/src/components/StorybookWrapper.jsx:
##########
@@ -78,15 +80,32 @@ 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.
+ function ThemeSync({ children }) {
+ const { colorMode } = useColorMode();
+ React.useEffect(() => {
+ themeObject.toggleDarkMode(colorMode === 'dark');
+ }, [colorMode]);
+ return children;
+ }
+
SupersetProviders = ({ children }) => (
- <themeObject.SupersetThemeProvider>
- <ConfigProvider
- getPopupContainer={getPopupContainer}
- getTargetContainer={() => document.body}
- >
- <App>{children}</App>
- </ConfigProvider>
- </themeObject.SupersetThemeProvider>
+ <ThemeSync>
+ <themeObject.SupersetThemeProvider>
Review Comment:
**Suggestion:** All story instances bind their provider to the same
module-level `themeObject`, whose `updateProviders` callback is overwritten
whenever a provider renders. With multiple demos mounted, the last provider
wins, so `toggleDarkMode` updates only that demo while earlier demos retain the
old Ant Design theme. Store provider update callbacks per mounted provider or
use an independently scoped theme instance. [stale reference]
<details>
<summary><b>Severity Level:</b> Critical ๐จ</summary>
```mdx
- โ Earlier live demos remain in the previous color mode.
- โ Component pages show inconsistent themes after toggling.
- โ ๏ธ Re-render order determines which demo updates.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=61d420e6d7d8465fa6429ca8b1c32ea7&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=61d420e6d7d8465fa6429ca8b1c32ea7&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:** 100:100
**Comment:**
*Stale Reference: All story instances bind their provider to the same
module-level `themeObject`, whose `updateProviders` callback is overwritten
whenever a provider renders. With multiple demos mounted, the last provider
wins, so `toggleDarkMode` updates only that demo while earlier demos retain the
old Ant Design theme. Store provider update callbacks per mounted provider or
use an independently scoped theme instance.
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=947b9c29abfe58ef0cf5b8ca415acfc6cc83efa94123f245754646e56a0ad301&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42602&comment_hash=947b9c29abfe58ef0cf5b8ca415acfc6cc83efa94123f245754646e56a0ad301&reaction=dislike'>๐</a>
##########
docs/src/components/StorybookWrapper.jsx:
##########
@@ -78,15 +80,32 @@ 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.
+ function ThemeSync({ children }) {
+ const { colorMode } = useColorMode();
+ React.useEffect(() => {
+ themeObject.toggleDarkMode(colorMode === 'dark');
+ }, [colorMode]);
Review Comment:
**Suggestion:** The theme is synchronized only in a passive effect, so a
demo first renders with the singleton's previous palette and switches to the
Docusaurus palette only after the initial commit. When the site starts in dark
mode, this produces a visible light-mode flash and can expose incorrectly
themed components during the initial render. [logic error]
<details>
<summary><b>Severity Level:</b> Major โ ๏ธ</summary>
```mdx
- โ ๏ธ Dark-mode pages briefly render light-themed demos.
- โ ๏ธ Initial component styles can visibly flash.
- โ ๏ธ Route navigation can repeat the palette transition.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=82bbe9c421cd4f868dc83b2dcd9e6c86&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=82bbe9c421cd4f868dc83b2dcd9e6c86&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:** 91:94
**Comment:**
*Logic Error: The theme is synchronized only in a passive effect, so a
demo first renders with the singleton's previous palette and switches to the
Docusaurus palette only after the initial commit. When the site starts in dark
mode, this produces a visible light-mode flash and can expose incorrectly
themed components during the initial render.
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=d390aade9884208cc8b5ae92bfb19955aa2127762b452f1ce50a9576e4974ceb&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42602&comment_hash=d390aade9884208cc8b5ae92bfb19955aa2127762b452f1ce50a9576e4974ceb&reaction=dislike'>๐</a>
##########
superset-frontend/packages/superset-core/src/theme/SupersetThemeProvider.test.tsx:
##########
@@ -0,0 +1,100 @@
+/**
+ * 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
+// overrides Theme#updateProviders (a no-op by default) to call setThemeState
+// whenever a *later* call to setConfig/toggleDarkMode runs on the same
+// instance. Consumers (docs/src/components/StorybookWrapper.jsx in
+// particular) rely on this: they call toggleDarkMode() from outside, on an
+// already-mounted provider, expecting it to propagate.
+//
+// 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.
+test('an already-mounted SupersetThemeProvider re-renders context-consuming
children when toggleDarkMode is called on the same instance', () => {
+ const themeObject = Theme.fromConfig();
+ let renderCount = 0;
+ let lastColorBgBase: string | undefined;
+
+ function Probe() {
+ const { token } = antdThemeImport.useToken();
+ renderCount += 1;
+ lastColorBgBase = token.colorBgBase;
+ return <div data-test="probe" />;
+ }
+
+ render(
+ <themeObject.SupersetThemeProvider>
+ <Probe />
+ </themeObject.SupersetThemeProvider>,
+ );
+
+ expect(screen.getByTestId('probe')).toBeTruthy();
+ const rendersBefore = renderCount;
+ const tokenBefore = lastColorBgBase;
+
+ act(() => {
+ themeObject.toggleDarkMode(true);
+ });
+
+ expect(renderCount).toBeGreaterThan(rendersBefore);
+ expect(lastColorBgBase).not.toBe(tokenBefore);
+});
+
+test('a toggleDarkMode call on an unmounted (or different) theme instance does
not affect a mounted provider', () => {
Review Comment:
**Suggestion:** This test claims to cover an unmounted theme instance, but
it never unmounts `mounted`; it only toggles a different still-mounted
instance. Consequently, it does not verify that a provider's callback is
cleaned up or harmless after unmounting. Either perform an actual unmount
before the toggle or rename the test to cover only the different-instance case.
[incomplete implementation]
<details>
<summary><b>Severity Level:</b> Minor ๐งน</summary>
```mdx
- โ ๏ธ Provider unmount behavior remains untested.
- โ ๏ธ Test title overstates lifecycle coverage.
- โ ๏ธ Future callback-cleanup regressions could pass unnoticed.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5535716f6bb34caa8f37443abc0ef8a9&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=5535716f6bb34caa8f37443abc0ef8a9&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:** 70:70
**Comment:**
*Incomplete Implementation: This test claims to cover an unmounted
theme instance, but it never unmounts `mounted`; it only toggles a different
still-mounted instance. Consequently, it does not verify that a provider's
callback is cleaned up or harmless after unmounting. Either perform an actual
unmount before the toggle or rename the test to cover only the
different-instance case.
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=5c27e24319051ba16584cd6de05964ff12a2d22fe40d2fb6a6c6f660d735cd05&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42602&comment_hash=5c27e24319051ba16584cd6de05964ff12a2d22fe40d2fb6a6c6f660d735cd05&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]
