Skip to content

Conversation

@astandrik
Copy link
Collaborator

@astandrik astandrik commented Aug 19, 2025

Closes #2721

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.37 MB | Main: 85.37 MB
Diff: 1.38 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 19, 2025 10:22
@astandrik
Copy link
Collaborator Author

bugbot run
/claude_review

@claude
Copy link

claude bot commented Aug 19, 2025

Claude finished @astandrik's task —— View job


Code Review Results

✅ Strengths

  1. Excellent TypeScript usage: The code properly uses TypeScript types with the 'T' prefix convention (TBridgePile, TBridgeInfo) and includes proper interface definitions.

  2. React performance best practices:

    • Components are properly memoized with React.memo (src/containers/Cluster/ClusterOverview/components/BridgeInfoTable.tsx:46, 85)
    • useCallback and useMemo are correctly used to prevent unnecessary re-renders (src/containers/Cluster/ClusterOverview/components/BridgeInfoTable.tsx:47, 56, 86)
  3. Proper i18n implementation: All user-facing strings use i18n keys (field_state, field_nodes) rather than hardcoded text (src/containers/Cluster/ClusterOverview/components/BridgeInfoTable.tsx:59, 63).

  4. Clean architecture: The changes follow the project's architectural patterns with proper separation between types, components, and test files.

  5. Good test coverage: The PR includes comprehensive E2E tests covering various bridge mode scenarios.

🔍 Observations & Suggestions

  1. State theme mapping is robust (src/containers/Cluster/ClusterOverview/components/BridgeInfoTable.tsx:17-36): The getBridgePileStateTheme function properly handles all enum values with appropriate fallback to 'unknown' theme. The color coding makes sense:

    • Green for healthy states (PRIMARY, PROMOTE, SYNCHRONIZED)
    • Yellow for warning states (NOT_SYNCHRONIZED)
    • Red for critical states (SUSPENDED, DISCONNECTED)
  2. Proper use of constants: The code correctly uses EMPTY_DATA_PLACEHOLDER from constants instead of hardcoding dashes (src/containers/Cluster/ClusterOverview/components/BridgeInfoTable.tsx:49, 65, 73).

  3. BEM naming convention: Correctly uses the cn() utility with component name 'bridge-info-table' (src/containers/Cluster/ClusterOverview/components/BridgeInfoTable.tsx:15).

  4. Safe data handling: The code properly handles undefined values with appropriate fallbacks (src/containers/Cluster/ClusterOverview/components/BridgeInfoTable.tsx:48-49, 64-65).

🎯 Minor Suggestions

  1. Consider adding JSDoc comments for the TBridgePile interface fields - I see you already have some comments but they could be JSDoc style for better IDE support.

  2. Type safety improvement: The State field in TBridgePile is typed as string?, but you have a BridgePileState enum. Consider using:

    State?: BridgePileState | string;

    This would provide better type hints while still allowing unknown states from the backend.

✅ Compliance Check

  • TypeScript type safety: Proper interface usage with 'T' prefix
  • React best practices: Memoization and hooks usage
  • Redux patterns: N/A for this component
  • i18n usage: All user-facing text uses i18n
  • API patterns: Types support data from window.api calls
  • Performance: Proper memoization for list rendering
  • Error handling: Safe fallbacks for undefined data
  • Testing: Good E2E test coverage

Summary

This is a well-implemented feature that follows all the project's coding standards and best practices. The bridge information display is properly integrated with the existing cluster overview, and the code is clean, performant, and maintainable. The PR successfully addresses the 2dc format from backend as described in issue #2721.

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 updates the bridge pile state format from the backend, changing from separate boolean fields (IsPrimary, IsBeingPromoted) to a unified string-based state enum. The change simplifies the data model and improves state representation consistency.

  • Introduces a BridgePileState enum with standardized state values
  • Removes deprecated boolean fields from the TBridgePile interface
  • Updates UI components to use the new unified state field with appropriate color themes

Reviewed Changes

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

File Description
src/types/api/cluster.ts Adds BridgePileState enum and removes deprecated boolean fields from TBridgePile interface
src/containers/Cluster/ClusterOverview/components/BridgeInfoTable.tsx Updates UI to use new state field with color-coded labels instead of separate boolean status displays
tests/suites/bridge/mocks.ts Updates test mocks to use new state format and adds comprehensive test data for all state values
tests/suites/bridge/bridge.test.ts Updates test expectations to check for new state format

return 'unknown';
}

switch (state.toUpperCase()) {
Copy link

Copilot AI Aug 19, 2025

Choose a reason for hiding this comment

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

Consider using the BridgePileState enum values directly instead of converting to uppercase and comparing strings. This would be more type-safe and maintainable: switch (state as BridgePileState) {

Suggested change
switch (state.toUpperCase()) {
// Helper to map string to BridgePileState enum value
function toBridgePileState(state?: string): BridgePileState | undefined {
if (!state) {
return undefined;
}
// Normalize and match to enum
const normalized = state.trim().toUpperCase();
switch (normalized) {
case BridgePileState.PRIMARY:
return BridgePileState.PRIMARY;
case BridgePileState.PROMOTE:
return BridgePileState.PROMOTE;
case BridgePileState.SYNCHRONIZED:
return BridgePileState.SYNCHRONIZED;
case BridgePileState.NOT_SYNCHRONIZED:
return BridgePileState.NOT_SYNCHRONIZED;
case BridgePileState.SUSPENDED:
return BridgePileState.SUSPENDED;
case BridgePileState.DISCONNECTED:
return BridgePileState.DISCONNECTED;
case BridgePileState.UNSPECIFIED:
return BridgePileState.UNSPECIFIED;
default:
return undefined;
}
}
function getBridgePileStateTheme(state?: string): NonNullable<LabelProps['theme']> {
const pileState = toBridgePileState(state);
if (!pileState) {
return 'unknown';
}
switch (pileState) {

Copilot uses AI. Check for mistakes.
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 added this pull request to the merge queue Aug 19, 2025
Merged via the queue into main with commit dcded29 Aug 19, 2025
8 checks passed
@astandrik astandrik deleted the astandrik.2721 branch August 19, 2025 10:37
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.

fix: 2dc format from backend

3 participants