Skip to content

feat(Versions): redesign #2707

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat(Versions): redesign #2707

wants to merge 3 commits into from

Conversation

artemmufazalov
Copy link
Member

@artemmufazalov artemmufazalov commented Aug 14, 2025

Closes #2477

Stand: https://nda.ya.ru/t/IElwFcP57HcVvu

CI Results

Test Status: ⚠️ FLAKY

📊 Full Report

Total Passed Failed Flaky Skipped
358 352 0 4 2
Test Changes Summary ✨1 ⏭️2 🗑️11

✨ New Tests (1)

  1. Info tab shows healthcheck status (tenant/diagnostics/tabs/info.test.ts)

⏭️ 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)

🗑️ Deleted Tests (11)

  1. off: no Pile Name column and no group-by option (bridge/bridge.test.ts)
  2. on: shows Pile Name column (bridge/bridge.test.ts)
  3. on: shows Pile Name (bridge/bridge.test.ts)
  4. off: hides Pile Name (bridge/bridge.test.ts)
  5. on: shows Pile Name and group-by option (bridge/bridge.test.ts)
  6. off: hides Pile Name and group-by option (bridge/bridge.test.ts)
  7. off: does not show Bridge piles section (bridge/bridge.test.ts)
  8. on: shows Bridge piles section with data (bridge/bridge.test.ts)
  9. Info tab shows healthcheck status when there are issues (tenant/diagnostics/tabs/info.test.ts)
  10. Info tab hides healthcheck status when status is GOOD with no issues (tenant/diagnostics/tabs/info.test.ts)
  11. Info tab shows healthcheck status when status is GOOD but has issues (tenant/diagnostics/tabs/info.test.ts)

Bundle Size: 🔺

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

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

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 implements a redesign of the Versions component, improving the user interface and adding new functionality for better version management visualization. The redesign introduces tabbed navigation, better grouping controls, and enhanced visual components.

Key changes:

  • Refactored version data types from VersionValue to PreparedVersion with enhanced data structure
  • Added node type filtering with tabbed navigation (Storage, Database, Other nodes)
  • Replaced tree-based UI with expandable block components for better UX
  • Enhanced the VersionsBar component with improved sizing and title display options

Reviewed Changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
src/utils/versions/types.ts Removed deprecated VersionValue interface
src/utils/versions/parseNodesToVersionsValues.ts Refactored functions to use PreparedVersion type and removed value normalization logic
src/containers/Versions/utils.ts Updated hook names and removed total parameter from parsing functions
src/containers/Versions/types.ts Updated interface to use PreparedVersion and added isDatabase flag
src/containers/Versions/i18n/en.json Added new internationalization keys for redesigned interface
src/containers/Versions/groupNodes.ts Updated to use new type and added isDatabase property
src/containers/Versions/constants.ts Added node type constants and titles
src/containers/Versions/VersionsBlock/ New expandable block component replacing tree view
src/containers/Versions/Versions.tsx Major redesign with URL parameter management and tabbed navigation
src/containers/Versions/Versions.scss Updated styles for new layout
src/containers/Versions/NodesTreeTitle/ Enhanced title component with better icons and layout
src/containers/Versions/GroupedNodesTree/ Refactored to use new block component instead of tree view
src/containers/Cluster/VersionsBar/ Removed old VersionsBar implementation
src/components/VersionsBar/ Enhanced VersionsBar with improved sizing and title options

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

@artemmufazalov
Copy link
Member Author

bugbot run
/claude_review

Copy link

cursor bot commented Aug 14, 2025

Skipping Bugbot: Unable to authenticate your request. Please make sure Bugbot is properly installed and configured for this repository.

Copy link

claude bot commented Aug 14, 2025

Claude finished @artemmufazalov's task —— View job


Code Review for feat(Versions): redesign

I've completed my review of this PR that redesigns the Versions component. Here's my comprehensive feedback:

Review Tasks:

  • Review TypeScript type safety and interface naming conventions
  • Check React patterns and Gravity UI component usage
  • Verify Redux Toolkit and RTK Query patterns
  • Ensure all user-facing text uses i18n
  • Check API calls go through window.api
  • Verify BEM naming convention
  • Look for performance and security issues

✅ Strengths

  1. Excellent i18n implementation: All user-facing text properly uses i18n keys following the naming convention (action_, title_, etc.)
  2. Strong TypeScript usage: Proper use of Zod schemas for URL parameter validation with fallbacks
  3. Good React patterns: Proper use of useMemo and useCallback for performance optimization
  4. BEM naming convention: Consistent use of cn() utility across all components
  5. Gravity UI integration: Proper use of Gravity UI components throughout
  6. URL parameter management: Excellent implementation using use-query-params with Zod validation

🔍 Issues Found

1. Hardcoded String in Select Label

File: src/containers/Versions/Versions.tsx:138

The Select component has a hardcoded label that should use i18n:

label={i18n('group-by')}  // Currently: "Group by:"

The i18n key exists (group-by), but the format should follow the naming convention (e.g., field_group_by or label_group_by).

2. Missing JSDoc Documentation

As noted by Copilot, several interfaces lack JSDoc documentation:

  • VersionsBlockProps in src/containers/Versions/VersionsBlock/VersionsBlock.tsx:10
  • NODE_TYPES constant in src/containers/Versions/constants.ts:9
  • VersionTitleProps in src/components/VersionsBar/VersionsBar.tsx:163
  • VersionCircleProps in src/components/VersionsBar/VersionsBar.tsx:202

3. Type Naming Convention

The types follow the project convention correctly - API types use 'T' prefix (e.g., TClusterInfo), while internal types don't (e.g., PreparedVersion). ✅

📊 Performance Considerations

  1. Good use of React.memo: The code properly uses useMemo for:

    • Filtering nodes by type (tenantNodes, storageNodes, otherNodes)
    • Computing displayed versions
    • Debounced hover handlers
  2. Potential improvement: Consider memoizing the render functions in Versions.tsx (e.g., renderStorageNodes, renderDatabaseNodes) if they become performance bottlenecks with large datasets.

🔒 Security & Best Practices

  1. No security issues found: No hardcoded secrets, proper data handling
  2. Error handling: Component handles undefined/null data gracefully
  3. API calls: Correctly uses RTK Query hooks (no direct API calls found)

💡 Suggestions

  1. Consider extracting magic numbers in VersionsBar.tsx:

    const TRUNCATION_THRESHOLD = 4;
    const MAX_DISPLAYED_VERSIONS = TRUNCATION_THRESHOLD - 1;

    These could be made configurable props if different views need different thresholds.

  2. Bundle size increase: The PR increases bundle size by ~10KB. While not critical, consider if any dependencies can be optimized or if code splitting could help.

Conclusion

This is a well-implemented redesign that follows the project's coding standards and best practices. The main issue is the i18n key naming convention for the "Group by" label. The missing JSDoc comments are minor but would improve code documentation.

The redesign improves UX with better visualization through the enhanced VersionsBar, cleaner organization with tabbed navigation, and maintains good performance characteristics.

@artemmufazalov artemmufazalov marked this pull request as ready for review August 14, 2025 10:37
@astandrik
Copy link
Collaborator

bugbot run

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no bugs!


astandrik
astandrik previously approved these changes Aug 15, 2025
Copy link
Collaborator

@astandrik astandrik left a comment

Choose a reason for hiding this comment

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

Some minor issues, but generally approve

size={size}
/>
))}
<Flex>{renderButton()}</Flex>
Copy link
Collaborator

Choose a reason for hiding this comment

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

renderButton renders one element - why do we need Flex here?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's needed to limit button width. Replaced it with css width: max-content

Screenshot 2025-08-15 at 16 28 50

count?: number;
isDimmed: boolean;
onMouseEnter: () => void;
onMouseLeave: () => void;
Copy link
Collaborator

Choose a reason for hiding this comment

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

VoidFunction type


React.useEffect(() => {
toggleBlock(expanded);
setIsOpened(expanded);
Copy link
Collaborator

Choose a reason for hiding this comment

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

whats the difference between isOpened and expanded here?

Maybe better to pass setExpanded from props and remove local state and useEffect

Copy link
Member Author

Choose a reason for hiding this comment

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

expanded here is external state, it is applied to all blocks, it is changed when I click "Expand all" or "Collapse all"

isOpened - state of the specific block, I change it by clicking on header, state of other blocks is not changed

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 tab acording to design
2 participants