Skip to content

Conversation

@astandrik
Copy link
Collaborator

@astandrik astandrik commented Aug 15, 2025

Closes #2718

CI Results

Test Status: ⚠️ FLAKY

📊 Full Report

Total Passed Failed Flaky Skipped
378 372 0 4 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: +1.76 KB (0.00%)

✅ Bundle size unchanged.

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

@astandrik astandrik requested a review from Copilot August 15, 2025 13:27
@astandrik
Copy link
Collaborator Author

bugbot run
/claude_review

Copy link
Contributor

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 adds support for displaying a "Being Promoted" status field for bridge piles in the cluster overview. The change introduces a new IsBeingPromoted boolean field to track when a pile is in the process of being promoted to primary status.

Key changes:

  • Adds IsBeingPromoted field to the TBridgePile interface
  • Updates the bridge info table component to display the promotion status with visual indicators
  • Adds internationalization support for the new field

Reviewed Changes

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

Show a summary per file
File Description
src/types/api/cluster.ts Adds IsBeingPromoted optional boolean field to TBridgePile interface
src/containers/Cluster/ClusterOverview/components/BridgeInfoTable.tsx Implements UI display for the being promoted status with icons and text
src/containers/Cluster/i18n/en.json Adds internationalization key for "Being Promoted" field label
tests/suites/bridge/mocks.ts Updates mock data to include IsBeingPromoted field for test piles
tests/suites/bridge/bridge.test.ts Adds test assertion to verify the being promoted status is displayed


const renderBeingPromotedStatus = React.useCallback(() => {
const isBeingPromoted = pile.IsBeingPromoted;
const icon = isBeingPromoted ? CircleCheckFill : CircleXmarkFill;
Copy link

Copilot AI Aug 15, 2025

Choose a reason for hiding this comment

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

The icons CircleCheckFill and CircleXmarkFill are not imported. You need to import these icons from @gravity-ui/icons at the top of the file.

Copilot uses AI. Check for mistakes.
@claude
Copy link

claude bot commented Aug 15, 2025

Claude finished @astandrik's task —— View job


Code Review for PR #2719

I've completed a review of this PR that adds the IsBeingPromoted field to bridge piles. Here's my feedback:

✅ Good Implementation Aspects

  1. TypeScript Type Safety: The IsBeingPromoted field is properly typed as an optional boolean in the TBridgePile interface (src/types/api/cluster.ts:110), following the established pattern.

  2. i18n Compliance: All user-facing text uses i18n keys properly (src/containers/Cluster/i18n/en.json:25).

  3. React Best Practices:

    • Proper use of useCallback hooks for render functions (src/containers/Cluster/ClusterOverview/components/BridgeInfoTable.tsx:49-64)
    • Components are properly memoized with React.memo
    • Correct usage of useMemo for computed values
  4. BEM Naming Convention: Follows the BEM pattern correctly with cn() utility.

  5. Test Coverage: Tests have been updated appropriately to verify the new field displays correctly (tests/suites/bridge/bridge.test.ts:123).

❌ Critical Issue

Missing Icon Imports (src/containers/Cluster/ClusterOverview/components/BridgeInfoTable.tsx:51)

The code uses CircleCheckFill and CircleXmarkFill icons that are already imported at line 3. However, Copilot's review comment appears to be incorrect - these icons are properly imported from @gravity-ui/icons.

🔍 Minor Suggestions

  1. Consistency: The implementation pattern for renderBeingPromotedStatus (lines 49-64) correctly follows the same pattern as renderPrimaryStatus, which is good for maintainability.

  2. EMPTY_DATA_PLACEHOLDER Usage: The code correctly uses EMPTY_DATA_PLACEHOLDER constant from utils/constants instead of hardcoding dashes, following the CLAUDE.md guidelines.

✅ Compliance with Standards

  • ✅ No hardcoded strings - all text uses i18n
  • ✅ No direct API calls
  • ✅ Proper error handling with fallback values
  • ✅ Uses Gravity UI components appropriately
  • ✅ No state mutations
  • ✅ Follows TypeScript 'T' prefix convention for types

Summary

This PR correctly implements the new IsBeingPromoted field with proper TypeScript typing, React best practices, and i18n support. The implementation is clean, consistent with the existing codebase patterns, and includes appropriate test coverage.

The issue raised by Copilot about missing icon imports appears to be a false positive - the icons are properly imported at the top of the file.

Recommendation: ✅ Approve - The implementation is correct and follows all project standards.

<Icon
data={icon}
size={16}
className={b('status-icon', {primary: isBeingPromoted})}
Copy link

Choose a reason for hiding this comment

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

Bug: Promotion Status Styled Incorrectly

The renderBeingPromotedStatus function applies the primary CSS class modifier to its icon. This reuses styling meant for the actual primary status, which can visually confuse users as "Being Promoted" and "Primary" statuses will look the same.

Fix in Cursor Fix in Web

@astandrik astandrik added this pull request to the merge queue Aug 15, 2025
Merged via the queue into main with commit 75e92c3 Aug 15, 2025
8 checks passed
@astandrik astandrik deleted the astandrik.add-promoted branch August 15, 2025 14:06
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.

feat: add isBeingPromoted field to piles

3 participants