Copilot commented on code in PR #36929:
URL: https://github.com/apache/superset/pull/36929#discussion_r2676877595
##########
superset-frontend/packages/superset-core/src/ui/translation/Translator.ts:
##########
@@ -17,7 +17,7 @@
* under the License.
*/
import UntypedJed from 'jed';
-import logging from '../utils/logging';
+import logging from '../../utils/logging';
Review Comment:
The import path uses relative paths across multiple directory levels
(`../../`). Consider using an absolute import from the package root for better
maintainability and clarity.
```suggestion
import logging from 'src/utils/logging';
```
##########
superset-frontend/packages/superset-ui-core/test/utils/featureFlag.test.ts:
##########
@@ -51,7 +56,7 @@ test('returns false and raises console error if feature flags
have not been init
Object.defineProperty(window, 'featureFlags', {
value: undefined,
});
- expect(uiCore.isFeatureEnabled(uiCore.FeatureFlag.DrillBy)).toEqual(false);
+ expect(isFeatureEnabled(FeatureFlag.DrillBy)).toEqual(false);
expect(uiCore.logging.error).toHaveBeenCalled();
expect(logging).toHaveBeenCalledWith('Failed to query feature flag
DRILL_BY');
Review Comment:
Line 61 references an undefined variable `logging`. This should be
`uiCore.logging.error` to match the mock being tested on line 60.
```suggestion
expect(uiCore.logging.error).toHaveBeenCalledWith('Failed to query feature
flag DRILL_BY');
```
##########
superset-frontend/packages/superset-core/src/utils/logging.ts:
##########
@@ -31,7 +31,7 @@ const logger = {
};
/**
- * Superset frontend logger, currently just an alias to console.
+ * Superset logger, currently just an alias to console.
Review Comment:
The comment was updated from 'Superset frontend logger' to 'Superset
logger', but this is still in the frontend package. The comment should clarify
that this is for frontend logging to avoid confusion.
```suggestion
* Superset frontend logger, currently just an alias to the browser console.
```
##########
superset-frontend/packages/superset-ui-core/src/components/ThemedAgGridReact/ThemedAgGridReact.test.tsx:
##########
@@ -20,12 +20,12 @@ import { render, screen } from '@superset-ui/core/spec';
import { AgGridReact } from 'ag-grid-react';
import { createRef } from 'react';
import { ThemeProvider, supersetTheme } from '@apache-superset/core';
-import * as themeUtils from '@apache-superset/core/ui/theme/utils/themeUtils';
+import * as uiModule from '@apache-superset/core/ui';
import { ThemedAgGridReact } from './index';
// Mock useThemeMode hook
-jest.mock('@apache-superset/core/ui/theme/utils/themeUtils', () => ({
- ...jest.requireActual('@apache-superset/core/ui/theme/utils/themeUtils'),
+jest.mock('@apache-superset/core/ui', () => ({
+ ...jest.requireActual('@apache-superset/core/ui'),
Review Comment:
The mock is now targeting the entire `@apache-superset/core/ui` module
instead of the more specific `theme/utils/themeUtils` path. This broader mock
could hide issues with other exports from the ui module. Consider maintaining
the more specific mock target if possible.
```suggestion
jest.mock('@apache-superset/core/ui/theme/utils/themeUtils', () => ({
...jest.requireActual('@apache-superset/core/ui/theme/utils/themeUtils'),
```
--
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]
