-
Notifications
You must be signed in to change notification settings - Fork 18
fix: tune databases handler call #3221
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
8 files reviewed, no comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces a new hook useDatabasesV2 that combines checks for both the use_meta_proxy cluster setting and meta databases availability. The hook replaces direct usage of useDatabasesAvailable across multiple components to ensure database handlers are called correctly based on cluster configuration.
Key changes:
- Introduces
useDatabasesV2hook that checks bothuse_meta_proxysetting and databases availability - Replaces
useDatabasesAvailablewithuseDatabasesV2across 6 components/files - Fixes API path for
cp_databasesto prevent proxying to cluster
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/utils/hooks/useDatabasesV2.ts | New hook that combines use_meta_proxy setting check with databases availability check |
| src/store/reducers/tenant/tenant.ts | Updates import and usage to use new useDatabasesV2 hook instead of useDatabasesAvailable |
| src/services/api/meta.ts | Removes cluster name from cp_databases API path to prevent proxying to cluster |
| src/containers/Tenants/Tenants.tsx | Updates to use useDatabasesV2 hook for determining database handler availability |
| src/containers/Tenant/Diagnostics/TenantOverview/TenantOverview.tsx | Updates to use useDatabasesV2 hook for tenant info queries |
| src/containers/Header/Header.tsx | Updates to use useDatabasesV2 hook in header component |
| src/components/TenantNameWrapper/TenantNameWrapper.tsx | Updates to use useDatabasesV2 logic inline by extracting settings and checking use_meta_proxy |
| src/components/ConnectToDB/ConnectToDBDialog.tsx | Updates to use useDatabasesV2 hook for database connection info |
Comments suppressed due to low confidence (1)
src/components/TenantNameWrapper/TenantNameWrapper.tsx:46
- The hook useClusterBaseInfo is being called twice in this component - once on line 40 to get settings, and again on line 46 to get monitoring. This creates unnecessary duplicate hook calls and could impact performance.
Consider destructuring both values from a single call to useClusterBaseInfo.
const {settings} = useClusterBaseInfo();
const backend = getTenantBackend(tenant, additionalTenantsProps);
const isExternalLink = Boolean(backend);
const links = getDatabaseLinks(additionalTenantsProps, tenant?.Name, tenant?.Type);
const {monitoring: clusterMonitoring} = useClusterBaseInfo();
|
|
||
| export function useDatabasesV2() { | ||
| const {settings} = useClusterBaseInfo(); | ||
| const isMetaDatabasesAvailable = useDatabasesAvailable(); | ||
|
|
||
| return settings?.use_meta_proxy !== false && isMetaDatabasesAvailable; |
Copilot
AI
Dec 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return value of this hook should be memoized using useMemo to prevent unnecessary re-renders. The hook currently returns a newly computed boolean value on every render, even when the dependencies haven't changed. This is particularly important since this hook is used in multiple components that may render frequently (Tenants, Header, TenantOverview, etc.).
The computation involves property access and logical operations that should be memoized according to the project's React performance guidelines.
| export function useDatabasesV2() { | |
| const {settings} = useClusterBaseInfo(); | |
| const isMetaDatabasesAvailable = useDatabasesAvailable(); | |
| return settings?.use_meta_proxy !== false && isMetaDatabasesAvailable; | |
| import {useMemo} from 'react'; | |
| export function useDatabasesV2() { | |
| const {settings} = useClusterBaseInfo(); | |
| const isMetaDatabasesAvailable = useDatabasesAvailable(); | |
| return useMemo( | |
| () => settings?.use_meta_proxy !== false && isMetaDatabasesAvailable, | |
| [settings?.use_meta_proxy, isMetaDatabasesAvailable], | |
| ); |
Greptile Overview
Greptile Summary
This PR introduces a new hook
useDatabasesV2that adds an additional check for theuse_meta_proxycluster setting before using meta databases handlers, ensuring that database operations only use the meta proxy when it's both available and enabled in cluster settings.Key changes:
useDatabasesV2hook that combinesuseDatabasesAvailablecapability check withuse_meta_proxysetting checkMetaAPI.getTenantsto never proxy the/meta/cp_databasesendpoint to clusters (removedclusterNamefrom path)TenantNameWrapperto consideruse_meta_proxysetting when deciding whether to use database IDs vs namesuseDatabasesAvailablewithuseDatabasesV2across 6 components for consistencyConfidence Score: 5/5
Important Files Changed
File Analysis
useDatabasesAvailablewith additional check foruse_meta_proxysettinggetTenantsto never proxy to cluster (removed clusterName from path), maintaining consistency with backend API designuseDatabaseIdbased on bothuiFactory.useDatabaseIdanduse_meta_proxysetting, ensuring consistent database ID usageSequence Diagram
sequenceDiagram participant Component as React Component participant useDatabasesV2 as useDatabasesV2 Hook participant useClusterBaseInfo as useClusterBaseInfo Hook participant useDatabasesAvailable as useDatabasesAvailable Hook participant MetaAPI as MetaAPI.getTenants Component->>useDatabasesV2: Call hook useDatabasesV2->>useClusterBaseInfo: Get cluster settings useClusterBaseInfo-->>useDatabasesV2: Return {settings} useDatabasesV2->>useDatabasesAvailable: Check if meta databases available useDatabasesAvailable-->>useDatabasesV2: Return capability status useDatabasesV2->>useDatabasesV2: Check: settings?.use_meta_proxy !== false && isMetaDatabasesAvailable useDatabasesV2-->>Component: Return boolean (use databases v2) Component->>MetaAPI: getTenants({clusterName, database}) Note over MetaAPI: Path: /meta/cp_databases (no cluster proxy) MetaAPI-->>Component: Return tenant dataCI Results
Test Status:⚠️ FLAKY
📊 Full Report
Test Changes Summary ⏭️2
⏭️ Skipped Tests (2)
Bundle Size: ✅
Current: 62.51 MB | Main: 62.51 MB
Diff: +1.12 KB (0.00%)
✅ Bundle size unchanged.
ℹ️ CI Information