Skip to content

feat(TenantOverview): do not use tabs #2655

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

Merged
merged 2 commits into from
Aug 1, 2025
Merged

feat(TenantOverview): do not use tabs #2655

merged 2 commits into from
Aug 1, 2025

Conversation

Raubzeug
Copy link
Contributor

@Raubzeug Raubzeug commented Aug 1, 2025

Stand
closes #2654

CI Results

Test Status: ⚠️ FLAKY

📊 Full Report

Total Passed Failed Flaky Skipped
354 349 0 3 2
Test Changes Summary ⏭️2

⏭️ Skipped Tests (2)

  1. Scroll to row, get shareable link, navigate to URL and verify row is scrolled into view (tenant/diagnostics/tabs/queries.test.ts)
  2. Copy result button copies to clipboard (tenant/queryEditor/queryEditor.test.ts)

Bundle Size: 🔽

Current: 85.35 MB | Main: 85.36 MB
Diff: 0.01 MB (-0.01%)

✅ Bundle size decreased.

ℹ️ CI Information
  • Test recordings for failed tests are available in the full report.
  • Bundle size is measured for the entire 'dist' directory.
  • 📊 indicates links to detailed reports.
  • 🔺 indicates increase, 🔽 decrease, and ✅ no change in bundle size.

@Raubzeug
Copy link
Contributor Author

Raubzeug commented Aug 1, 2025

/claude_review

Copy link

claude bot commented Aug 1, 2025

Claude finished @Raubzeug's task —— View job


Todo List:

  • Gather context and understand the changes
  • Review TypeScript type safety and interface usage
  • Check React best practices and Gravity UI component usage
  • Verify Redux Toolkit patterns with RTK Query
  • Ensure i18n compliance (no hardcoded strings)
  • Check API call patterns (window.api usage)
  • Validate performance and security considerations
  • Review BEM naming convention usage
  • Provide comprehensive review feedback

Code Review

This PR removes tabs functionality from the TenantOverview component, simplifying the UI structure. The changes are well-executed overall, with good adherence to YDB Embedded UI standards. Here's my detailed review:

✅ Strengths

  1. Clean Removal of Tab-Based Navigation

    • Properly removed all tab-related query params handlers (useTenantNetworkQueryParams.ts, useTenantStorageQueryParams.ts)
    • Simplified component structure without breaking functionality
    • Reduced code complexity by ~227 lines net
  2. TypeScript Type Safety

    • All interfaces and types are properly maintained with 'T' prefix convention (e.g., TenantStorageProps, TenantMemoryProps)
    • Type safety preserved in all modified components
  3. i18n Compliance

    • All user-facing text uses i18n properly
    • No hardcoded strings introduced
    • i18n keys properly updated and removed unused ones
  4. Component Structure

    • Good use of Gravity UI components (Flex, InfoViewer, ProgressWrapper)
    • Proper BEM naming convention with cn() utility
    • Clean component organization

⚠️ Areas for Attention

  1. CSS Variable Addition (src/containers/App/App.scss:33-37)

    --diagnostics-section-title-margin: 20px;
    --diagnostics-section-margin: 30px;
    --diagnostics-section-table-width: 100%;

    These new CSS variables are added but I don't see them being used in the changed files. Consider removing if unused or documenting their purpose.

  2. Deleted SCSS Files

    • HoverPopup.scss and TenantStorage.scss were deleted
    • The HoverPopup component still imports styles: import './HoverPopup.scss' (line removed correctly)
    • Ensure no styling regressions from these deletions
  3. Performance Considerations

    • All tables continue to use ResizeableDataTable which supports virtual scrolling ✅
    • Proper use of React.useMemo for column definitions in TopTables.tsx:56 ✅
    • Auto-refresh patterns maintained correctly
  4. Redux/RTK Query Patterns

    • All API calls properly use RTK Query hooks (nodesApi.useGetNodesQuery, topTablesApi.useGetTopTablesQuery)
    • No direct API calls detected ✅
    • Proper polling with autoRefreshInterval

📝 Minor Suggestions

  1. EntityStatus Component (src/components/EntityStatus/EntityStatus.tsx:119)

    • The visible class logic could be simplified using a single condition:
    visible: clipboardButtonAlwaysVisible || infoIconHovered,
  2. Storage Columns (src/containers/Storage/PaginatedStorageGroupsTable/columns/columns.tsx)

    • Good use of Popover with proper className and placement
    • Consider extracting the placement array as a constant since it's used multiple times
  3. Consistency in Layout

    • TenantMemory uses <div className={b()}> wrapper while TenantNetwork and TenantStorage use <Flex direction="column" gap={4}>
    • Consider standardizing the layout approach

✅ Security & Best Practices

  • No hardcoded secrets or sensitive information
  • Proper error handling maintained in all components
  • Loading states properly handled
  • Route params validation maintained (React Router v5)

Summary

This is a well-executed refactoring that successfully removes tab-based navigation while maintaining all functionality. The code follows YDB Embedded UI standards consistently, with proper TypeScript typing, i18n compliance, and Redux patterns. The minor suggestions above are optional improvements that could enhance code maintainability.

The PR achieves its goal of simplifying the TenantOverview component structure without introducing regressions or breaking changes.

@astandrik
Copy link
Collaborator

bugbot run

@astandrik astandrik requested a review from Copilot August 1, 2025 10:21
Copy link
Contributor

@Copilot Copilot AI left a 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 refactors the TenantOverview component to remove tab-based interfaces and display all sections directly. The changes eliminate complex tabbed navigation in favor of a simpler, more direct presentation of storage, network, and memory information.

  • Remove tab-based UI patterns from TenantStorage and TenantNetwork components
  • Consolidate popover styling with a global CSS class
  • Update i18n strings to reflect the new direct display approach

Reviewed Changes

Copilot reviewed 24 out of 24 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/utils/constants.ts Adds global YDB_POPOVER_CLASS_NAME constant for consistent popover styling
src/store/reducers/tenant/types.ts Removes storage and network tab type definitions
src/store/reducers/tenant/constants.ts Removes storage and network tab ID constants
src/containers/Tenant/Diagnostics/TenantOverview/i18n/en.json Updates i18n strings to remove tab-specific language and use direct titles
src/containers/Tenant/Diagnostics/TenantOverview/getSectionTitle.tsx Removes utility for generating dynamic section titles with links
src/containers/Tenant/Diagnostics/TenantOverview/TenantStorage/useTenantStorageQueryParams.ts Removes storage tab query parameter handling
src/containers/Tenant/Diagnostics/TenantOverview/TenantStorage/TopTables.tsx Refactors column generation and removes title prop
src/containers/Tenant/Diagnostics/TenantOverview/TenantStorage/TopGroups.tsx Removes dynamic title generation and link handling
src/containers/Tenant/Diagnostics/TenantOverview/TenantStorage/TenantStorage.tsx Replaces TabProvider with direct Flex layout using StatsWrapper
src/containers/Tenant/Diagnostics/TenantOverview/TenantStorage/TenantStorage.scss Removes tab-specific styling
src/containers/Tenant/Diagnostics/TenantOverview/TenantNetwork/useTenantNetworkQueryParams.ts Removes network tab query parameter handling
src/containers/Tenant/Diagnostics/TenantOverview/TenantNetwork/TopNodesBySkew.tsx Removes dynamic title generation and simplifies component
src/containers/Tenant/Diagnostics/TenantOverview/TenantNetwork/TopNodesByPing.tsx Removes dynamic title generation and simplifies component
src/containers/Tenant/Diagnostics/TenantOverview/TenantNetwork/TenantNetwork.tsx Replaces TabProvider with direct Flex layout using StatsWrapper
src/containers/Tenant/Diagnostics/TenantOverview/TenantMemory/TopNodesByMemory.tsx Removes dynamic title generation and simplifies component
src/containers/Tenant/Diagnostics/TenantOverview/TenantMemory/TenantMemory.tsx Wraps TopNodesByMemory with StatsWrapper for consistent layout
src/containers/Storage/PaginatedStorageGroupsTable/columns/columns.tsx Applies global popover class to encryption indicator
src/containers/App/App.scss Defines global popover styling
src/components/SpeedMultiMeter/SpeedMultiMeter.tsx Applies global popover class
src/components/SpeedMultiMeter/SpeedMultiMeter.scss Removes component-specific popover styling
src/components/HoverPopup/HoverPopup.tsx Applies global popover class
src/components/HoverPopup/HoverPopup.scss Removes component-specific popover styling
src/components/EntityStatus/EntityStatus.tsx Applies global popover class
src/components/EntityStatus/EntityStatus.scss Removes component-specific popover styling

title={i18n('title_nodes-by-ping')}
allEntitiesLink={getTenantPath({
...query,
[TenantTabsGroups.diagnosticsTab]: TENANT_DIAGNOSTICS_TABS_IDS.nodes,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should lead to Network tab

title={i18n('title_nodes-by-skew')}
allEntitiesLink={getTenantPath({
...query,
[TenantTabsGroups.diagnosticsTab]: TENANT_DIAGNOSTICS_TABS_IDS.nodes,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should lead to Network tab

@Raubzeug Raubzeug requested a review from artemmufazalov August 1, 2025 11:22
@Raubzeug Raubzeug added this pull request to the merge queue Aug 1, 2025
Merged via the queue into main with commit 4fb69ad Aug 1, 2025
5 checks passed
@Raubzeug Raubzeug deleted the tenant-overview branch August 1, 2025 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve Tenant Overview
3 participants