Skip to content

Conversation

@artemmufazalov
Copy link
Member

@artemmufazalov artemmufazalov commented Aug 5, 2025

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

Go to Database page, Manage dropdown will be in the Header

Closes #2100

CI Results

Test Status: ⚠️ FLAKY

📊 Full Report

Total Passed Failed Flaky Skipped
358 352 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.32 MB | Main: 85.36 MB
Diff: 0.04 MB (-0.04%)

✅ 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.

@artemmufazalov artemmufazalov marked this pull request as ready for review August 5, 2025 11:33
}
});
},
className: b('remove-db'),
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to define className, just use theme: 'danger',

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you, there wasn't such option previously, fixed

align-items: center;
}

&__remove-db {
Copy link
Contributor

Choose a reason for hiding this comment

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

it may be removed after using theme: 'danger', for menu item

@astandrik astandrik requested a review from Copilot August 6, 2025 10:00
@astandrik
Copy link
Collaborator

bugbot run

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 a "Manage" dropdown menu to the Header component on database pages, providing users with edit and delete database functionality. The dropdown appears when viewing a specific database and contains actions for editing or deleting the database based on user permissions.

Key changes:

  • Integration of database management actions into the Header component
  • Addition of permission-based visibility for edit/delete operations
  • Implementation of dropdown menu with proper loading states and navigation

Reviewed Changes

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

File Description
src/store/reducers/tenant/tenant.ts Utilizes prepareTenants utility function for tenant data processing
src/containers/Header/i18n/en.json Adds internationalization keys for manage dropdown actions
src/containers/Header/Header.tsx Implements manage dropdown with edit/delete database functionality
src/containers/Header/Header.scss Adds styling for delete action with red color indicator

Comment on lines 124 to 126
const isEnoughtData = clusterName && databaseData;

if (isEditDBAvailable && onEditDB && isEnoughtData) {
Copy link

Copilot AI Aug 6, 2025

Choose a reason for hiding this comment

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

The variable name 'isEnoughtData' contains a spelling error. It should be 'isEnoughData' (missing 'u' in 'enough').

Suggested change
const isEnoughtData = clusterName && databaseData;
if (isEditDBAvailable && onEditDB && isEnoughtData) {
const isEnoughData = clusterName && databaseData;
if (isEditDBAvailable && onEditDB && isEnoughData) {

Copilot uses AI. Check for mistakes.
Comment on lines 124 to 126
const isEnoughtData = clusterName && databaseData;

if (isEditDBAvailable && onEditDB && isEnoughtData) {
Copy link

Copilot AI Aug 6, 2025

Choose a reason for hiding this comment

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

The variable 'isEnoughtData' contains a spelling error. It should be 'isEnoughData' (missing 'u' in 'enough').

Suggested change
const isEnoughtData = clusterName && databaseData;
if (isEditDBAvailable && onEditDB && isEnoughtData) {
const isEnoughData = clusterName && databaseData;
if (isEditDBAvailable && onEditDB && isEnoughData) {

Copilot uses AI. Check for mistakes.
},
});
}
if (isDeleteDBAvailable && onDeleteDB && isEnoughtData) {
Copy link

Copilot AI Aug 6, 2025

Choose a reason for hiding this comment

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

The variable 'isEnoughtData' contains a spelling error. It should be 'isEnoughData' (missing 'u' in 'enough').

Suggested change
if (isDeleteDBAvailable && onDeleteDB && isEnoughtData) {
if (isDeleteDBAvailable && onDeleteDB && isEnoughData) {

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
Copy link
Collaborator

/claude_review

@claude
Copy link

claude bot commented Aug 6, 2025

Claude finished @astandrik's task —— View job


Code Review Results

I've completed the review of this PR that adds a "Manage" dropdown to the Header component. Here are my findings:

Todo List:

  • Read all changed files to understand the implementation
  • Review TypeScript type safety and interfaces
  • Check i18n usage and no hardcoded strings
  • Validate API usage patterns (window.api)
  • Review Redux patterns and state management
  • Check React best practices and performance
  • Assess security and error handling
  • Provide comprehensive review feedback

📋 Overall Assessment

The implementation is generally well-structured and follows most YDB Embedded UI patterns, but there are several issues that need to be addressed.

✅ What's Done Right

  1. i18n Usage: All user-facing strings properly use i18n with correct naming conventions
  2. Redux Patterns: Correctly uses RTK Query with injectEndpoints pattern (src/store/reducers/tenant/tenant.ts:60)
  3. API Calls: Properly uses window.api for all API calls, no direct calls
  4. Permission Checks: Appropriate feature flag checks and permission validations
  5. Component Structure: Follows project conventions with proper file organization

🔴 Critical Issues

1. Spelling Error (src/containers/Header/Header.tsx:124)

The variable isEnoughtData should be isEnoughData. This typo appears on lines 124, 126, and 135.

2. CSS Anti-pattern (src/containers/Header/Header.scss:37-40)

Using !important is discouraged. As the reviewer suggested, use Gravity UI's theme: 'danger' instead:

// Instead of className with !important CSS
{
    text: headerKeyset('action_delete-db'),
    iconStart: <TrashBin />,
    theme: 'danger', // Add this
    action: () => { ... },
    // Remove: className: b('remove-db'),
}

3. Missing Error Handling

The delete action (src/containers/Header/Header.tsx:140-145) lacks error handling. If onDeleteDB rejects, the error is unhandled:

// Should be:
action: async () => {
    try {
        const isDeleted = await onDeleteDB({clusterName, databaseData});
        if (isDeleted) {
            const path = getClusterPath('tenants');
            history.push(path);
        }
    } catch (error) {
        // Handle error - show toast notification
    }
},

⚠️ Suggestions for Improvement

1. Performance Optimization

The menuItems array is recreated on every render (src/containers/Header/Header.tsx:120). Consider using useMemo:

const menuItems = useMemo(() => {
    const items: DropdownMenuItem[] = [];
    // ... menu item logic
    return items;
}, [isEditDBAvailable, isDeleteDBAvailable, clusterName, databaseData, onEditDB, onDeleteDB, history]);

2. Type Safety Enhancement

Consider creating a more specific type for the database data parameter instead of using the broad PreparedTenant type.

3. Loading State UX

While the loading state is handled for the dropdown button, consider disabling menu items during loading to prevent race conditions.

📝 Minor Issues

  1. Naming Convention: The PR uses isEnoughtData - beyond the typo, consider a more descriptive name like hasRequiredData or isDataComplete

  2. Code Organization: The long conditional logic for menu items (lines 126-149) could be extracted into a helper function for better readability

✅ Testing Considerations

  • No unit tests were added for the new functionality
  • Consider adding tests for:
    • Permission-based visibility of menu items
    • Navigation after successful deletion
    • Loading states during data fetch

🔒 Security Review

  • No exposed secrets or authentication tokens ✅
  • Proper permission checks before showing actions ✅
  • API calls use established patterns ✅

Summary: The implementation is solid but needs the critical issues addressed, particularly the spelling error and the CSS anti-pattern. The missing error handling should also be added before merging.

@artemmufazalov artemmufazalov requested a review from Raubzeug August 6, 2025 10:55
@artemmufazalov artemmufazalov added this pull request to the merge queue Aug 6, 2025
Merged via the queue into main with commit d123263 Aug 6, 2025
5 checks passed
@artemmufazalov artemmufazalov deleted the 2100-manage-db branch August 6, 2025 11:10
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.

Add edit and delete DB buttons to header on Tenant page

4 participants