Skip to content

Conversation

artemmufazalov
Copy link
Member

@artemmufazalov artemmufazalov commented Jul 31, 2025

Closes #2496

Stand: https://nda.ya.ru/t/74h1yKcy7H5Yhu - it has hardcoded backend response just to show how it works

  • Added majorIndex and minorIndex to versions data to ensure proper sorting
  • Refactored versions files so they support new data format (but no changes to Versions tab yet)
  • Create new VersionsBar component (old VersionsBar inside Versions tab will be replaced next)

Versions tab will be modified in this issue: #2477

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.36 MB | Main: 85.35 MB
Diff: +0.02 MB (0.02%)

⚠️ Bundle size increased. Please review.

ℹ️ 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.

@artemmufazalov artemmufazalov marked this pull request as ready for review July 31, 2025 13:33
@artemmufazalov artemmufazalov requested a review from Copilot July 31, 2025 13:36
Copilot

This comment was marked as outdated.

Copy link

claude bot commented Jul 31, 2025

Claude finished @artemmufazalov's task —— View job


Code Review for PR #2642: feat(Clusters): rework versions progress bar

Review Checklist:

  • Read repository guidelines (CLAUDE.md)
  • Review TypeScript type safety and interfaces
  • Check React patterns and Gravity UI usage
  • Verify i18n implementation
  • Review API patterns and Redux usage
  • Check BEM naming conventions
  • Assess performance considerations
  • Review testing patterns
  • Provide comprehensive feedback

Review Summary

Overall, this PR makes good improvements to the versions handling system. The addition of majorIndex and minorIndex provides better control over version sorting. However, there are several issues that need to be addressed before merging.

🚨 Critical Issues

1. TypeScript Interface Naming Convention Violation

According to the repository guidelines, all API types must be prefixed with 'T'. The new interfaces in src/utils/versions/types.ts don't follow this convention:

  • VersionValue → Should be TVersionValue
  • VersionWithColorIndexes → Should be TVersionWithColorIndexes
  • PreparedVersion → Should be TPreparedVersion
  • PreparedVersions → Should be TPreparedVersions

2. Missing i18n Registration Call

The new VersionsBar component is missing the required i18n registration call:

src/components/VersionsBar/VersionsBar.tsx:22

import i18n from './i18n';

// Missing: registerKeysets() call
// Should be: const i18n = registerKeysets(COMPONENT, {en});

The component imports i18n but doesn't call registerKeysets() as required by the guidelines.

⚠️ Important Issues

3. Magic Numbers Without Constants

The component uses magic numbers without extracting them to named constants:

src/components/VersionsBar/VersionsBar.tsx:14-16

const TRUNCATION_THRESHOLD = 4;
// One more line for Show more / Hide button
const MAX_DISPLAYED_VERSIONS = TRUNCATION_THRESHOLD - 1;

Consider adding a comment explaining why 4 was chosen as the threshold.

4. Event Handler Type Can Be Simplified

src/components/VersionsBar/VersionsBar.tsx:51-56

const handleShowAllVersions = (event: React.MouseEvent<HTMLButtonElement, MouseEvent>) => {
// Can be simplified to: React.MouseEvent<HTMLButtonElement>

5. Inconsistent Function Naming

As noted by the copilot reviewer:

  • src/components/VersionsBar/VersionsBar.tsx:87 - handelMouseLeave should be handleMouseLeave

🔍 Other Observations

6. Good Practices Observed

  • ✅ Proper use of BEM naming convention with cn() utility
  • ✅ Using Gravity UI components (Button, Flex, Tooltip)
  • ✅ Memoization with React.useMemo for expensive calculations
  • ✅ Comprehensive test coverage for the sortVersions function
  • ✅ Following the i18n key naming convention (action_ prefix for buttons)

7. Test Descriptions Could Be Clearer

The test descriptions mention "descending order" but the tests actually demonstrate ascending order:

  • src/utils/versions/test/sortVersions.test.ts:5
  • src/utils/versions/test/sortVersions.test.ts:37

8. Typos in Comments

  • src/utils/versions/getVersionsColors.ts:146 - "fisrt" should be "first"
  • src/utils/versions/clusterVersionColors.ts:77 - "Summ" should be "Sum"

9. Missing Error Handling

The clusterVersionColors.ts file has a logic issue on line 76-82 where the count is calculated but never assigned back to the result object.

📋 Recommendations

  1. Fix all interface naming to follow the 'T' prefix convention
  2. Properly register i18n in the VersionsBar component
  3. Fix the typos in comments and function names
  4. Consider extracting magic numbers to configuration or making them props
  5. Fix the logic error in clusterVersionColors.ts where the merged count isn't assigned back
  6. Update test descriptions to accurately reflect what they're testing

✅ Positive Aspects

  • The refactoring improves code organization and maintainability
  • Good separation of concerns with utility functions
  • Proper TypeScript usage with clear type definitions
  • Following React best practices with hooks and memoization
  • Good test coverage for the new sorting functionality

The PR is on the right track but needs these fixes before merging to maintain code quality and consistency with the project's standards.

@artemmufazalov artemmufazalov force-pushed the 2496-versions-progress-bar branch from 233c1fe to a2d1cd7 Compare July 31, 2025 13:59
@artemmufazalov artemmufazalov requested a review from Copilot July 31, 2025 14:00
Copilot

This comment was marked as outdated.

@artemmufazalov artemmufazalov requested a review from Copilot July 31, 2025 14:12
Copilot

This comment was marked as outdated.

@artemmufazalov artemmufazalov requested a review from Copilot July 31, 2025 14:20
Copilot

This comment was marked as outdated.

@artemmufazalov artemmufazalov requested a review from Copilot July 31, 2025 14:28
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 version progress bar implementation to support major and minor version indices for proper sorting, while creating a new standalone VersionsBar component for the clusters list. The changes prepare the foundation for improved version management across the application.

Key Changes

  • Added majorIndex and minorIndex fields to version data structures for better sorting control
  • Refactored version-related utilities to use a new VersionsDataMap structure instead of simple color mappings
  • Created a new standalone VersionsBar component with enhanced UI features including truncation and hover effects

Reviewed Changes

Copilot reviewed 24 out of 24 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/utils/versions/types.ts New types file defining version data structures with index support
src/utils/versions/sortVersions.ts New sorting utility that handles version ordering by major/minor indices
src/utils/versions/clusterVersionColors.ts New utility for cluster-specific version color processing
src/utils/versions/parseNodesToVersionsValues.ts Updated to use new VersionsDataMap structure
src/utils/versions/getVersionsColors.ts Refactored to support version indices and new data structure
src/components/VersionsBar/VersionsBar.tsx New component with enhanced UI features for displaying version bars
src/containers/Clusters/columns.tsx Updated to use new VersionsBar component
src/containers/Versions/*.tsx Updated to use new versionsDataMap structure
Comments suppressed due to low confidence (3)

src/utils/versions/clusterVersionColors.ts:13

  • [nitpick] The constant name 'UNDEFINED_COLOR_INDEX' could be more descriptive. Consider renaming to 'NO_COLOR_INDEX_KEY' or 'UNDEFINED_COLOR_KEY' to better indicate it's a string key used when no color index is provided.
const UNDEFINED_COLOR_INDEX = '__no_color__';

src/containers/Versions/groupNodes.test.ts:71

  • The test function is flagged for complexity but still runs. Consider breaking this test into smaller, more focused test cases to improve readability and maintainability.
    // eslint-disable-next-line complexity

src/containers/Versions/groupNodes.test.ts:108

  • The test function is flagged for complexity but still runs. Consider breaking this test into smaller, more focused test cases to improve readability and maintainability.
    // eslint-disable-next-line complexity

>
<span
onMouseEnter={() => {
setHoveredVersion(item.version);
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest to add a small debounce for setHoveredVersion(item.version). For now it blinks when I move the mouse across the screen.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added 200 ms debounce to setHoveredVersion, increased debounce for tooltip so they are the same

};

const renderButton = () => {
if (shouldTruncateVersions) {
Copy link
Contributor

Choose a reason for hiding this comment

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

we may reduce nesting with

if (!shouldTruncateVersions) {
    return null
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@artemmufazalov artemmufazalov requested a review from adameat as a code owner August 1, 2025 07:33
@artemmufazalov artemmufazalov force-pushed the 2496-versions-progress-bar branch from 4eae270 to 80b6eb5 Compare August 1, 2025 07:34
@artemmufazalov artemmufazalov force-pushed the 2496-versions-progress-bar branch from 80b6eb5 to 09c80c5 Compare August 1, 2025 07:35
@artemmufazalov artemmufazalov requested a review from Raubzeug August 1, 2025 07:36
@astandrik
Copy link
Collaborator

/claude_review

@ydb-platform ydb-platform deleted a comment from claude bot Aug 1, 2025
@artemmufazalov artemmufazalov added this pull request to the merge queue Aug 1, 2025
Merged via the queue into main with commit 7be9d5f Aug 1, 2025
7 checks passed
@artemmufazalov artemmufazalov deleted the 2496-versions-progress-bar branch August 1, 2025 08:26
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.

Rework versions progress bar in Clusters table

3 participants