Refactor TypeScript types and improve type safety across components#28081
Refactor TypeScript types and improve type safety across components#28081chirag-madlani wants to merge 4 commits into
Conversation
- Updated various components to replace `any` with more specific types, enhancing type safety and clarity. - Modified `DomainTreeView` to include `loadDomains` in dependency array for `useEffect`. - Improved test files by removing unnecessary `eslint-disable` comments and replacing `any` with appropriate types. - Refined `NodeSuggestions` to use a more specific type for `selectRef`. - Changed `WorkflowHistory` interface to use `unknown` instead of `any` for `variables`. - Enhanced `ServiceConnectionDetails` and `TeamsSelectableNew` components to use `unknown` for state management. - Updated utility functions and context definitions to replace `any` with `unknown`, ensuring better type safety. - Removed redundant code and improved readability in `getKeyValues` function in `ServiceConnectionDetailsUtils`. - General cleanup of comments and code structure for better maintainability.
github-actions
Bot
added
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| function checkDuplicates(callsMap, scope) { | ||
| for (const [signature, data] of callsMap.entries()) { | ||
| if (data.count >= threshold) { | ||
| data.nodes.forEach((node, index) => { | ||
| if (index > 0) { | ||
| context.report({ | ||
| node, | ||
| messageId: 'duplicateApiCall', | ||
| data: { | ||
| apiCall: signature, | ||
| count: data.count, | ||
| scope, | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🚨 Bug: ESLint rule never fires due to wrong Map passed to checkDuplicates
In no-duplicate-api-calls.js, the CallExpression handler stores API call data in a nested Map structure: currentComponent.calls.get('global') returns an inner Map of signature -> {count, nodes, apiFunction}. However, the exit handler at line 252-253 passes the outer map (component.calls) to checkDuplicates(). This means checkDuplicates iterates over entries like ['global', Map], where data is a Map object — not {count, nodes}. Since Map.count is undefined, the condition data.count >= threshold will never be true, and the rule will silently never report any violations.
Pass the inner 'global' map to checkDuplicates instead of the outer map:
if (isComponent && componentStack.length > 0) {
const component = componentStack.pop();
const globalCalls = component.calls.get('global');
if (globalCalls) {
checkDuplicates(globalCalls, `component "${component.name}"`);
}
if (componentStack.length === 0) {
currentComponent = null;
} else {
currentComponent = componentStack[componentStack.length - 1];
}
}
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
| NODE_HEIGHT: 36, | ||
| })); | ||
|
|
||
| const mockNavigate = jest.fn(); |
There was a problem hiding this comment.
⚠️ Bug: Duplicate mockNavigate declaration will cause test failure
The diff adds const mockNavigate = jest.fn(); at line 69, but there is already an identical declaration at line 420 in the same file. In strict mode or with let/const, this causes a SyntaxError: Identifier 'mockNavigate' has already been declared. The pre-existing declaration at line 420 should be removed, or the new one at line 69 should replace it.
Remove the new duplicate declaration at line 69 since the one at line 420 already exists (or vice versa):
// Remove line 69: const mockNavigate = jest.fn();
// Keep only the single declaration at line 420
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
| const handleCheckboxChange = (fieldName: string) => { | ||
| setCheckedItems((prev) => | ||
| prev.includes(fieldName) | ||
| ? prev.filter((item) => item !== fieldName) | ||
| : [...prev, fieldName] | ||
| ); | ||
| }; |
There was a problem hiding this comment.
⚠️ Performance: useMemo invalidated every render due to unstable dependency
handleCheckboxChange is added to the menuItems useMemo dependency array but is not wrapped in useCallback. Since it's a plain function declaration, it gets a new reference on every render, causing menuItems to recompute every time — completely defeating the purpose of useMemo.
Wrap handleCheckboxChange in useCallback to stabilize its reference:
const handleCheckboxChange = useCallback((fieldName: string) => {
setCheckedItems((prev) =>
prev.includes(fieldName)
? prev.filter((item) => item !== fieldName)
: [...prev, fieldName]
);
}, []);
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
There was a problem hiding this comment.
Pull request overview
This PR tightens TypeScript typing across multiple UI utilities/components (replacing any with unknown/more specific types) and introduces a new custom ESLint rule (custom-rules/no-duplicate-api-calls) with accompanying documentation to help detect redundant API calls in React components.
Changes:
- Replaced several
anyusages withunknown/ stricter generics across UI utilities, hooks, and component props/refs. - Added and wired a custom ESLint rule (
no-duplicate-api-calls) into the UI’s flat ESLint config, plus documentation (README/config/quick start) and an example file. - Minor refactors to improve hook dependency correctness and internal helper organization.
Reviewed changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| openmetadata-ui/src/main/resources/ui/src/utils/WorkflowConfigUtils.ts | Narrows workflow definition typing for workflow config helpers. |
| openmetadata-ui/src/main/resources/ui/src/utils/ServiceConnectionDetailsUtils.tsx | Refactors schema/key-value rendering with stricter types and helper extraction. |
| openmetadata-ui/src/main/resources/ui/src/utils/QueryBuilderUtils.tsx | Replaces Record<string, any> with Record<string, unknown> in query filter conversion. |
| openmetadata-ui/src/main/resources/ui/src/utils/ExtensionPointRegistry.ts | Changes default generic from any to unknown for extension contributions. |
| openmetadata-ui/src/main/resources/ui/src/utils/EntitySummaryPanelUtilsV1.tsx | Tightens table render record typing (any → unknown). |
| openmetadata-ui/src/main/resources/ui/src/utils/CustomizaNavigation/CustomizeNavigation.test.ts | Removes inline ESLint suppression comment near any cast. |
| openmetadata-ui/src/main/resources/ui/src/utils/CSV/CSVUtilsClassBase.tsx | Introduces typed aliases to replace RenderEditCellProps<any, any>. |
| openmetadata-ui/src/main/resources/ui/src/utils/Auth/TokenService/TokenServiceUtil.test.ts | Removes inline ESLint suppression comments around any casts. |
| openmetadata-ui/src/main/resources/ui/src/pages/SwaggerPage/RapiDocReact.tsx | Narrows Rapidoc event callback types (any → Record<string, unknown>). |
| openmetadata-ui/src/main/resources/ui/src/interface/data-insight.interface.ts | Narrows Recharts tooltip generics for data insight tooltip props. |
| openmetadata-ui/src/main/resources/ui/src/hooks/usePubSub.ts | Switches pub/sub callback generics default from any to unknown. |
| openmetadata-ui/src/main/resources/ui/src/contexts/WorkflowModeContext.tsx | Narrows workflowDefinition context prop type (any → Record<string, unknown>). |
| openmetadata-ui/src/main/resources/ui/src/components/Settings/Users/UsersProfile/UserProfileRoles/UserProfileRoles.component.tsx | Attempts to type the Select ref more strictly. |
| openmetadata-ui/src/main/resources/ui/src/components/Settings/Team/TeamsSelectable/TeamsSelectableNew.tsx | Attempts to type forwardRef and TreeSelect ref more strictly. |
| openmetadata-ui/src/main/resources/ui/src/components/Settings/Services/ServiceConnectionDetails/ServiceConnectionDetails.component.tsx | Changes schema state typing (any → unknown). |
| openmetadata-ui/src/main/resources/ui/src/components/SearchSettings/TermBoost/TermBoost.tsx | Narrows option parameter typing in tag change handler. |
| openmetadata-ui/src/main/resources/ui/src/components/SearchSettings/FilterConfiguration/FilterConfiguration.tsx | Moves checkbox handler and adjusts hook dependencies. |
| openmetadata-ui/src/main/resources/ui/src/components/MyData/RightSidebar/FollowingWidget.tsx | Narrows helper parameter typing for entity icon selection. |
| openmetadata-ui/src/main/resources/ui/src/components/KnowledgeGraph/KnowledgeGraph.test.tsx | Adds a shared mockNavigate for router mocking. |
| openmetadata-ui/src/main/resources/ui/src/components/Glossary/GlossaryTerms/tabs/WorkFlowTab/WorkFlowHistory.interface.ts | Replaces Record<string, any> with Record<string, unknown>. |
| openmetadata-ui/src/main/resources/ui/src/components/Entity/EntityLineage/NodeSuggestions.component.tsx | Narrows ref type from any to a minimal focusable interface. |
| openmetadata-ui/src/main/resources/ui/src/components/Entity/EntityLineage/NodeChildren/NodeChildren.component.test.tsx | Removes inline ESLint suppression comments around any usage. |
| openmetadata-ui/src/main/resources/ui/src/components/DomainListing/components/DomainTreeView.tsx | Adds missing dependency to a callback dependency array. |
| openmetadata-ui/src/main/resources/ui/src/components/DataProducts/DataProductsPage/DataProductsPage.component.tsx | Reorders fetch helpers without changing behavior. |
| openmetadata-ui/src/main/resources/ui/src/components/common/ListView/ListView.component.tsx | Changes default generic type from any to Record<string, unknown>. |
| openmetadata-ui/src/main/resources/ui/src/components/common/atoms/asyncTreeSelect/useTreeData.tsx | Moves helper function earlier for readability/organization. |
| openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/FeedEditor/FeedEditor.tsx | Narrows mention item typing (Record<string, any> → Record<string, unknown>). |
| openmetadata-ui/src/main/resources/ui/eslint.config.mjs | Registers custom-rules plugin, enables no-duplicate-api-calls, and escalates no-explicit-any. |
| openmetadata-ui/src/main/resources/ui/eslint-rules/test-example.tsx | Adds an example demonstrating duplicate API call detection (documentation/demo). |
| openmetadata-ui/src/main/resources/ui/eslint-rules/README.md | Adds full documentation for the custom ESLint rule(s). |
| openmetadata-ui/src/main/resources/ui/eslint-rules/QUICK_START.md | Adds quick-start usage guide for the new rule. |
| openmetadata-ui/src/main/resources/ui/eslint-rules/no-duplicate-api-calls.js | Implements the no-duplicate-api-calls custom ESLint rule. |
| openmetadata-ui/src/main/resources/ui/eslint-rules/index.js | Exposes the custom rule via a plugin-style rules export. |
| openmetadata-ui/src/main/resources/ui/eslint-rules/CONFIGURATION.md | Adds detailed configuration/troubleshooting guide for the rule. |
Comments suppressed due to low confidence (1)
openmetadata-ui/src/main/resources/ui/src/components/Settings/Team/TeamsSelectable/TeamsSelectableNew.tsx:33
- This component declares
forwardRef<HTMLDivElement, ...>and then casts the ref for an AntDTreeSelect. AntDTreeSelectrefs are not DOM element refs (they expose imperative methods likefocus/blur). UsingHTMLDivElementhere can break callers expecting the correct ref shape. Use AntD’s select ref type (BaseSelectRef/RefSelectProps, depending on your AntD version) instead ofHTMLDivElement.
const TeamsSelectableNew = forwardRef<HTMLDivElement, TeamsSelectableProps>(
(
{
showTeamsAlert,
onSelectionChange,
| checkDuplicates(component.calls, `component "${component.name}"`); | ||
|
|
| const options = context.options[0] || {}; | ||
| const threshold = options.threshold || 2; | ||
| const checkUseEffect = options.checkUseEffect !== false; | ||
| const checkCallbacks = options.checkCallbacks !== false; | ||
| const allowedDuplicates = new Set(options.allowedDuplicates || []); |
| objectName === 'axios' || | ||
| objectName === 'APIClient' || | ||
| ['get', 'post', 'put', 'patch', 'delete'].includes(propertyName) || | ||
| apiCallPatterns.some((pattern) => { |
| if ( | ||
| serviceType === EntityType.PIPELINE_SERVICE && | ||
| key === 'connection' && | ||
| value.type?.toString().toLowerCase() === 'airflow' | ||
| ) { |
| return renderInputField( | ||
| key, | ||
| value as string, |
| render: (name: string, record: Record<string, unknown>) => ( | ||
| <div className="d-inline-flex" style={{ maxWidth: '68%' }}> | ||
| <span className="break-word">{record.displayName || name}</span> | ||
| </div> |
| className: 'menu-items', | ||
| }), | ||
| [entityFields, checkedItems] | ||
| [entityFields, checkedItems, handleCheckboxChange] | ||
| ); |
| open={isDropdownOpen} | ||
| options={useRolesOption} | ||
| popupClassName="roles-custom-dropdown-class" | ||
| ref={dropdownRef as any} | ||
| ref={dropdownRef as React.Ref<HTMLDivElement> | undefined} | ||
| tagRender={TagRenderer} |
🔴 Playwright Results — 1 failure(s), 17 flaky✅ 4069 passed · ❌ 1 failed · 🟡 17 flaky · ⏭️ 86 skipped
Genuine Failures (failed on all attempts)❌
|
Code Review 🚫 Blocked 1 resolved / 4 findingsRefactors TypeScript types and improves testing infrastructure, but currently blocked due to a broken ESLint rule logic, duplicate mock declarations in tests, and an unstable useMemo dependency. 🚨 Bug: ESLint rule never fires due to wrong Map passed to checkDuplicates📄 openmetadata-ui/src/main/resources/ui/eslint-rules/no-duplicate-api-calls.js:189-203 📄 openmetadata-ui/src/main/resources/ui/eslint-rules/no-duplicate-api-calls.js:252-253 📄 openmetadata-ui/src/main/resources/ui/eslint-rules/no-duplicate-api-calls.js:263-277 In Pass the inner 'global' map to checkDuplicates instead of the outer map
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Gitar
This pull request introduces comprehensive documentation and configuration for a new custom ESLint rule,
no-duplicate-api-calls, aimed at improving code quality in the OpenMetadata UI by detecting and preventing redundant API calls in React components. The changes include detailed usage instructions, configuration options, troubleshooting guides, and example patterns for refactoring, along with the rule's implementation and registration in the ESLint plugin system.Documentation and Usage Guides:
README.mdwith a full overview of theno-duplicate-api-callsrule, including its purpose, configuration options, detected patterns, and implementation details. Example code snippets demonstrate both violations and best practices.CONFIGURATION.mddetailing how to configure the rule, adjust severity, set exceptions, handle file exclusions, and migrate existing codebases. Includes troubleshooting and best practice recommendations.QUICK_START.mdproviding a concise, example-driven introduction to the rule, how to fix violations, suppress warnings, and answers to common questions.Rule Implementation:
no-duplicate-api-callsrule in the ESLint plugin entry point (index.js), making it available for use in the codebase.Describe your changes:
Fixes #
I worked on ... because ...
Type of change:
High-level design:
N/A — small change.
Tests:
Use cases covered
Unit tests
Backend integration tests
Ingestion integration tests
Playwright (UI) tests
Manual testing performed
UI screen recording / screenshots:
Not applicable.
Checklist:
Fixes <issue-number>: <short explanation>Fixes #<issue-number>above.Summary by Gitar
CSVUtilsClassBase.tsxby correctly wrappingEditCellPropsreturns.FollowingWidget.tsx,data-insight.interface.ts, andAppPlugin.ts.CustomizeNavigation.test.tsto use actual icon components instead of string mock-icons.TokenServiceUtil.test.tsand removed unused test code inNodeChildren.component.test.tsx.ui-checkstyle.ymlby adjusting job dependency structures and formatting configurations.This will update automatically on new commits.