korbit-ai[bot] commented on code in PR #35308:
URL: https://github.com/apache/superset/pull/35308#discussion_r2412833619
##########
superset-frontend/packages/superset-core/src/ui/theme/index.tsx:
##########
@@ -87,3 +87,4 @@ export type {
// Export theme utility functions
export * from './utils/themeUtils';
+export * from './utils';
Review Comment:
### Overlapping wildcard exports may cause namespace conflicts
<sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Potential namespace collision due to overlapping wildcard exports from
'./utils/themeUtils' and './utils' directories.
###### Why this matters
If both './utils/themeUtils' and './utils' export items with the same names,
this will cause TypeScript compilation errors or runtime conflicts. The
'./utils' export may also re-export everything from './utils/themeUtils',
leading to duplicate exports.
###### Suggested change ∙ *Feature Preview*
Verify that './utils' and './utils/themeUtils' don't export conflicting
names. If './utils' already re-exports from './utils/themeUtils', remove the
redundant line:
```typescript
// Remove this line if ./utils already exports themeUtils
// export * from './utils/themeUtils';
export * from './utils';
```
Alternatively, use named exports to avoid conflicts:
```typescript
export * from './utils/themeUtils';
export { specificUtility1, specificUtility2 } from './utils';
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d0994f3a-75f5-4504-9afe-caacc9c6fc77/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d0994f3a-75f5-4504-9afe-caacc9c6fc77?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d0994f3a-75f5-4504-9afe-caacc9c6fc77?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d0994f3a-75f5-4504-9afe-caacc9c6fc77?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d0994f3a-75f5-4504-9afe-caacc9c6fc77)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:18771602-1609-4b65-8c7b-33f282051b7c -->
[](18771602-1609-4b65-8c7b-33f282051b7c)
##########
superset-frontend/packages/superset-core/src/ui/testing.tsx:
##########
@@ -0,0 +1,47 @@
+/**
+ * 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 userEvent from '@testing-library/user-event';
+import { ReactElement } from 'react';
+import { render, RenderOptions } from '@testing-library/react';
+import '@testing-library/jest-dom';
+import { themeObject } from './theme';
+
+// Define the wrapper component outside
+const Providers = ({ children }: { children: React.ReactNode }) => (
+ <themeObject.SupersetThemeProvider>
+ {children}
+ </themeObject.SupersetThemeProvider>
+);
Review Comment:
### Inflexible Test Provider Wrapper <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The Providers component is hardcoded to only wrap components with the theme
provider, making it inflexible for tests that might need additional providers
(e.g., Redux, Router, etc.).
###### Why this matters
As the application grows, tests will likely need different combinations of
providers. The current design will require creating multiple wrapper components
or modifying this one repeatedly.
###### Suggested change ∙ *Feature Preview*
Create a composable provider system that allows tests to specify which
providers they need:
```typescript
type TestProviderProps = {
children: React.ReactNode;
withTheme?: boolean;
withRouter?: boolean;
withRedux?: boolean;
};
const TestProviders = ({ children, withTheme = true, ...props }:
TestProviderProps) => {
let wrapped = children;
if (withTheme) {
wrapped =
<themeObject.SupersetThemeProvider>{wrapped}</themeObject.SupersetThemeProvider>;
}
// Add other conditional providers as needed
return wrapped;
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f5133a77-2d8c-460d-8b1e-8ca81d4ea7a1/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f5133a77-2d8c-460d-8b1e-8ca81d4ea7a1?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f5133a77-2d8c-460d-8b1e-8ca81d4ea7a1?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f5133a77-2d8c-460d-8b1e-8ca81d4ea7a1?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f5133a77-2d8c-460d-8b1e-8ca81d4ea7a1)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:00aff757-1c1a-4c47-a9ef-4ccfff3b6f80 -->
[](00aff757-1c1a-4c47-a9ef-4ccfff3b6f80)
##########
superset-frontend/packages/superset-ui-core/src/index.ts:
##########
@@ -37,3 +36,4 @@ export * from './ui-overrides';
export * from './hooks';
export * from './currency-format';
export * from './time-comparison';
+export * from '@apache-superset/core';
Review Comment:
### Circular Dependency in Core Package <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code introduces a circular dependency by re-exporting everything from
'@apache-superset/core' while being part of the core package itself.
###### Why this matters
Circular dependencies can lead to initialization issues, make the codebase
harder to maintain, and potentially cause runtime errors. They also make it
difficult to understand the true dependencies between modules.
###### Suggested change ∙ *Feature Preview*
Remove the circular dependency by either:
1. Removing the re-export if it's not needed
2. Explicitly importing and re-exporting only the required components
3. Restructuring the code to avoid the circular dependency pattern
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a7547d2f-c1e5-4cfd-983c-0286f69aed22/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a7547d2f-c1e5-4cfd-983c-0286f69aed22?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a7547d2f-c1e5-4cfd-983c-0286f69aed22?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a7547d2f-c1e5-4cfd-983c-0286f69aed22?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a7547d2f-c1e5-4cfd-983c-0286f69aed22)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:bc95c844-5ae8-4873-a2a9-daed490a9235 -->
[](bc95c844-5ae8-4873-a2a9-daed490a9235)
--
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]
