Skip to content

Conversation

Raubzeug
Copy link
Contributor

@Raubzeug Raubzeug commented Sep 11, 2025

Closes #2847

Stand

CI Results

Test Status: ⚠️ FLAKY

📊 Full Report

Total Passed Failed Flaky Skipped
378 373 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.44 MB | Main: 85.43 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.
## Greptile Summary

Updated On: 2025-09-11 15:49:06 UTC

This PR implements a comprehensive refactoring to support relative schema object paths throughout the YDB Embedded UI. The changes introduce a new SchemaPathParam type containing both path and databaseFullPath properties, enabling the application to work with relative paths while maintaining full path context for API calls.

Key changes include:

  1. New Type System: Added SchemaPathParam type in src/types/api/common.ts to standardize how schema paths are passed throughout the application
  2. Base API Enhancement: Modified BaseYdbAPI.getSchemaPath() to convert absolute paths to relative ones by removing the database path prefix when useRelativePath is enabled
  3. Component Interface Updates: Over 80 components updated to accept both database (relative identifier) and databaseFullPath (absolute path) parameters
  4. Query Builder Centralization: Created new src/utils/shardsQueryBuilders.ts with reusable SQL query builders that include relative path display logic
  5. Consistent Naming: Standardized parameter naming from inconsistent tenantName usage to consistent database terminology
  6. Test Updates: All test files updated to use the new database constant instead of the deprecated tenantName

The refactoring enables cleaner URLs and more intuitive navigation by displaying relative paths in components like TopShards and Storage Diagnostics while maintaining backward compatibility with existing absolute path functionality.

Important Files Changed

Changed Files
Filename Score Overview
src/types/api/common.ts 5/5 Added new SchemaPathParam type definition for relative path support
src/services/api/base.ts 4/5 Refactored getSchemaPath method to use SchemaPathParam object structure
src/utils/shardsQueryBuilders.ts 4/5 New utility file with centralized SQL query builders and relative path logic
src/containers/Tenant/Diagnostics/AccessRights/components/RightsTable/RevokeAllRightsDialog.tsx 1/5 Critical bug: databaseFullPath parameter missing from NiceModal.show() call
src/containers/Tenant/Diagnostics/AccessRights/components/RightsTable/Actions.tsx 2/5 Similar bug where databaseFullPath not passed to revoke dialog
src/containers/Tenant/Diagnostics/AccessRights/components/ChangeOwnerDialog.tsx 2/5 Missing databaseFullPath parameter in getChangeOwnerDialog function
src/store/reducers/viewSchema/viewSchema.ts 3/5 Potential issue with simple string replacement for path conversion
src/store/reducers/shardsWorkload/shardsWorkload.ts 4/5 Refactored to use centralized query builders with improved maintainability
src/services/api/viewer.ts 4/5 Updated 16 methods to use SchemaPathParam instead of string paths
src/containers/Tenant/Tenant.tsx 4/5 Complex refactor with proper database name resolution and path handling
tests/utils/constants.ts 5/5 Simple rename from tenantName to database constant
src/containers/Heatmap/Heatmap.tsx 4/5 Added databaseFullPath prop for relative path support
src/components/ShardsTable/ShardsTable.tsx 4/5 Changed schemaPath to databaseFullPath for better semantic clarity
src/store/reducers/header/types.ts 4/5 Updated breadcrumb interfaces to support new database naming conventions
[Additional 70+ files with scores 4-5/5] 4-5/5 Consistent parameter renames and API updates for relative path support

Confidence score: 2/5

  • This PR contains critical bugs in AccessRights dialogs that will cause runtime failures when users try to revoke permissions or change ownership
  • Several dialog functions are missing required databaseFullPath parameters, leading to undefined values being passed to components that expect them
  • Pay close attention to AccessRights dialog components, particularly RevokeAllRightsDialog.tsx, Actions.tsx, and ChangeOwnerDialog.tsx where parameters are not properly passed through modal creation functions

Sequence Diagram

sequenceDiagram
    participant User
    participant UI as "Frontend UI"
    participant BaseAPI as "BaseYdbAPI"
    participant SchemeAPI as "SchemeAPI"
    participant Backend as "YDB Backend"

    User->>UI: Navigate to tenant/schema path
    UI->>BaseAPI: Initialize with useRelativePath config
    
    alt Relative paths enabled
        UI->>BaseAPI: Request schema data with full path
        BaseAPI->>BaseAPI: Call getSchemaPath(path, databaseFullPath)
        BaseAPI->>BaseAPI: Convert absolute path to relative
        Note over BaseAPI: path.slice(databaseFullPath.length + 1)
        BaseAPI->>Backend: Send request with relative path
        Backend-->>BaseAPI: Return schema data
        BaseAPI-->>UI: Return processed response
    else Relative paths disabled
        UI->>BaseAPI: Request schema data with full path
        BaseAPI->>Backend: Send request with original absolute path
        Backend-->>BaseAPI: Return schema data
        BaseAPI-->>UI: Return response
    end
    
    User->>UI: Create directory action
    UI->>SchemeAPI: createSchemaDirectory(database, path)
    SchemeAPI->>BaseAPI: Call getSchemaPath() for path conversion
    BaseAPI->>BaseAPI: Transform path based on useRelativePath setting
    SchemeAPI->>Backend: POST /scheme/directory with processed path
    Backend-->>SchemeAPI: Return success/error
    SchemeAPI-->>UI: Return operation result
    UI->>User: Show success/error feedback
Loading

@Raubzeug Raubzeug linked an issue Sep 11, 2025 that may be closed by this pull request
@astandrik astandrik requested a review from Copilot September 11, 2025 15:43
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 pull request refactors the codebase to better support relative paths by transitioning from using database names as parameters to a more structured approach that separates database identifiers from full database paths. The main goal is to improve path handling throughout the tenant and diagnostics components.

Key changes include:

  • Replacing tenantName parameters with database parameters
  • Adding databaseFullPath parameters to maintain full path context
  • Updating path transformation utilities to handle relative paths correctly

Reviewed Changes

Copilot reviewed 123 out of 123 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tests/utils/constants.ts Renamed tenantName constant to database for consistency
Multiple test files Updated test parameters from tenantName to database across tenant test suites
src/utils/shardsQueryBuilders.ts New utility file for building shards-related queries with improved path handling
Type definition files Updated type definitions to include databaseFullPath parameters and SchemaPathParam type
Store reducers Refactored API calls to use new path parameter structure with relative path support
Service API files Updated API methods to accept SchemaPathParam objects instead of string paths
Container components Updated component props and API calls to use new database/path parameter structure
Comments suppressed due to low confidence (1)

src/containers/Tenant/ObjectSummary/transformPath.ts:21

  • [nitpick] The function comment and variable names don't match the refactored implementation. The comment still mentions 'dbName' parameter but the function now uses 'databaseFullPath'. The variable 'normalizedDbName' should be renamed to 'normalizedDatabaseFullPath' for clarity.
export function transformPath(path: string, databaseFullPath: string): string {
    // Normalize the path and dbName by removing leading/trailing slashes
    const normalizedPath = path.replace(/^\/+|\/+$/g, '');
    const normalizedDbName = databaseFullPath.replace(/^\/+|\/+$/g, '');

    if (!normalizedPath.startsWith(normalizedDbName)) {
        return normalizedPath || '/';
    }
    if (normalizedPath === normalizedDbName) {
        return `/${normalizedPath}`;
    }

    let result = normalizedPath.slice(normalizedDbName.length);

    // Remove leading slash if present, unless the result would be empty
    result = result.replace(/^\/+/, '') || '/';

    return result;
}

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Reviewing changes made in this pull request

timeout?: number;
}) => {
try {
const relativePath = path.replace(databaseFullPath, '');
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: Using simple string replace could be problematic. If path contains databaseFullPath as a substring multiple times or in unexpected positions, this could produce incorrect results. Consider using a more robust approach like checking if path starts with databaseFullPath and removing only the prefix.

@astandrik
Copy link
Collaborator

bugbot run
/claude_review

Copy link

claude bot commented Sep 11, 2025

Claude finished @astandrik's task —— View job


Code Review for PR #2877: fix: refactor relative paths support

I've reviewed this PR focusing on code standards, TypeScript safety, React patterns, and critical requirements for the YDB Embedded UI codebase. Here are my findings:

🔴 Critical Bugs

1. Missing databaseFullPath parameter in modal dialogs (HIGH PRIORITY)

src/containers/Tenant/Diagnostics/AccessRights/components/RightsTable/RevokeAllRightsDialog.tsx:30-35

// Bug: databaseFullPath is not passed to NiceModal.show()
export async function getRevokeAllRightsDialog({
    path,
    database,
    databaseFullPath, // ← Accepted but not used!
}: GetRevokeAllRightsDialogProps): Promise<boolean> {
    return await NiceModal.show(CHANGE_OWNER_DIALOG, {
        id: CHANGE_OWNER_DIALOG,
        path,
        database,
        // Missing: databaseFullPath
    });
}

src/containers/Tenant/Diagnostics/AccessRights/components/ChangeOwnerDialog.tsx:25-30

// Same bug in ChangeOwnerDialog
export async function getChangeOwnerDialog({
    path,
    database,
    databaseFullPath, // ← Accepted but not used!
}: GetChangeOwnerDialogProps): Promise<boolean> {
    return await NiceModal.show(CHANGE_OWNER_DIALOG, {
        id: CHANGE_OWNER_DIALOG,
        path,
        database,
        // Missing: databaseFullPath
    });
}

These bugs will cause runtime failures when users attempt to revoke rights or change ownership, as the dialogs expect databaseFullPath but receive undefined.

🟡 Code Quality Issues

2. SQL Injection Risk

src/utils/shardsQueryBuilders.ts:98

return `Path='${path}' OR Path LIKE '${path}/%'`;

Direct string interpolation in SQL queries. Should use parameterized queries or proper escaping.

3. Redundant Path Object Structure

src/store/reducers/network/network.ts:12

{path: {path: databaseFullPath, databaseFullPath}, database}

Both path and databaseFullPath in the nested object have the same value. This appears to be a mistake.

4. Path Construction Logic Issues

src/containers/Tenant/ObjectSummary/SchemaTree/SchemaTree.tsx:115-116

const prefix = databaseFullPath === parentPath ? '' : `${databaseFullPath}/`;
const newPath = `${prefix}${parentPath}/${relativePath}`;

When databaseFullPath === parentPath, this creates paths like /parentPath/relativePath which may not be correct. Needs clarification on expected behavior.

src/containers/Tenant/ObjectSummary/CreateDirectoryDialog/CreateDirectoryDialog.tsx:65

const path = `${parentPath}/${relativePath}`;

Similar issue - when parentPath equals databaseFullPath, this could create duplicate path segments.

5. Type Safety Concern

src/store/reducers/viewSchema/viewSchema.ts:23

path: path.replace(databaseFullPath, '')

Simple string replacement is fragile. If databaseFullPath appears multiple times in path or as a substring, this will produce incorrect results. Should validate that path starts with databaseFullPath before removing the prefix.

✅ Positive Observations

  1. Good TypeScript practices: The new SchemaPathParam type properly encapsulates path-related data
  2. Consistent naming refactor: Migration from tenantName to database improves clarity
  3. Proper i18n usage: All user-facing strings use i18n keys (verified in reviewed files)
  4. Redux patterns: RTK Query endpoints properly wrap window.api calls
  5. Memory management: No Monaco Editor memory leaks detected in changed files

📋 Recommendations

  1. Fix the critical bugs immediately - The missing databaseFullPath parameters will cause runtime failures
  2. Add parameter validation in createPathWhereCondition to prevent SQL injection
  3. Clarify path construction logic with unit tests to ensure edge cases are handled correctly
  4. Use more robust string operations for path manipulation (e.g., check startsWith before replacing)
  5. Add JSDoc comments to the new getSchemaPath method explaining the relative path conversion logic

Summary

This PR successfully implements the relative path feature across the codebase with mostly good patterns. However, the critical bugs in the AccessRights dialogs must be fixed before merging, as they will cause immediate runtime failures for users. The SQL injection risk should also be addressed for security reasons.

The refactoring from tenantName to database naming is well-executed and improves code clarity. The introduction of SchemaPathParam type provides better type safety for path-related operations.

Todo List:

  • Read CLAUDE.md for repo-specific guidelines
  • Review critical AccessRights dialog files for bugs
  • Check new SchemaPathParam type and API changes
  • Verify TypeScript type safety and interface usage
  • Check for hardcoded strings (i18n compliance)
  • Validate Redux patterns and API calls
  • Review performance considerations
  • Provide comprehensive feedback

@Raubzeug Raubzeug added this pull request to the merge queue Sep 15, 2025
Merged via the queue into main with commit 9fc87c4 Sep 15, 2025
7 checks passed
@Raubzeug Raubzeug deleted the relative branch September 15, 2025 06:50
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.

Top tables - broken links in cloud Improve relative schema objects paths

3 participants