Skip to content

Conversation

@artemmufazalov
Copy link
Member

@artemmufazalov artemmufazalov commented Aug 29, 2025

Part of #2725

New meta version decodes some fields, while our UI expects them to be strings

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.38 MB | Main: 85.38 MB
Diff: +0.46 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.

@artemmufazalov
Copy link
Member Author

/claude_review

@claude
Copy link

claude bot commented Aug 29, 2025

Claude finished @artemmufazalov's task —— View job


Code Review: Handle Meta Version Field Decoding

Summary

This PR addresses a backward compatibility issue where new meta versions decode certain fields as objects while the UI expects strings. The implementation provides robust type-safe parsing with proper error handling.

✅ Strengths

  1. Excellent Type Safety (src/types/api/meta.ts:47-51)

    • Union types string | Type properly handle both stringified and object formats
    • Types are prefixed with 'T' following project conventions
    • Clear separation between base and extended cluster interfaces
  2. Robust Parsing Implementation (src/store/reducers/cluster/parseFields.ts)

    • Early returns for already-decoded objects avoid unnecessary processing
    • Zod schemas provide runtime validation
    • Proper error handling with console.error for debugging
    • Each parser handles undefined/empty inputs gracefully
  3. Comprehensive Test Coverage (src/store/reducers/cluster/__test__/parseFields.test.ts)

    • Tests cover all edge cases: valid JSON, objects, invalid JSON, undefined
    • Clear test descriptions following testing patterns
    • Proper test structure with describe/test blocks
  4. Redux Pattern Compliance (src/store/reducers/cluster/cluster.ts:167-169)

    • Follows RTK Query patterns correctly
    • Uses selectors appropriately with createSelector
    • Integrates parsing functions cleanly in useClusterBaseInfo hook
  5. i18n Compliance (src/containers/Cluster/ClusterInfo/utils/useClusterLinks.ts:15,22,29)

    • All user-facing strings use i18n keys
    • No hardcoded strings

🔍 Areas for Improvement

  1. Missing trace_check Parser

    • Type MetaClusterTraceCheck is defined in src/types/api/meta.ts:96-98
    • Field is included in union type at line 51
    • But no parser function is implemented for it
    • Consider if this field needs parsing support
  2. Performance Optimization

    // Current (src/store/reducers/cluster/parseFields.ts:17)
    if (traceView && typeof traceView === 'object') {
        return traceView;
    }
    
    // Consider more explicit check
    if (traceView && typeof traceView === 'object' && !Array.isArray(traceView)) {
        return traceView as MetaClusterTraceView;
    }
  3. Error Message Constants
    Consider extracting error messages to constants for consistency:

    const PARSE_ERRORS = {
      TRACE_VIEW: 'Error parsing trace_view field:',
      CORES: 'Error parsing cores field:',
      LOGGING: 'Error parsing logging field:'
    } as const;
  4. Monitoring Data Parsing (src/utils/monitoring.ts:86-98)

    • Good that it follows the same pattern as new parse functions
    • Consider moving all parse functions to a central location for consistency

📋 Minor Observations

  1. Deleted Files

    • Removed parseMetaCluster.ts and prepareLinks.test.ts suggests good cleanup
    • trace.ts removal aligns with inline type definitions
  2. Function Naming

    • parseTraceField could be parseTraceView for consistency with field name
    • All other parsers match their field names
  3. Type Inference

    • Return types are explicitly defined which is good
    • Consider using satisfies operator for schema definitions in future TypeScript versions

✅ Security & Best Practices

  • ✅ No hardcoded secrets or sensitive data
  • ✅ Proper error handling prevents runtime crashes
  • ✅ Input validation through Zod schemas
  • ✅ No direct API calls (uses window.api pattern)
  • ✅ No state mutations in RTK Query

Overall Assessment

This is a well-implemented solution that properly handles backward compatibility. The code is type-safe, well-tested, and follows all project conventions. The approach of checking types at runtime and parsing only when needed is correct for this scenario.

Recommendation: Approved with minor suggestions. Consider implementing the trace_check parser if that field is used in the UI.

@ydb-platform ydb-platform deleted a comment from claude bot Aug 29, 2025
@artemmufazalov artemmufazalov marked this pull request as ready for review August 29, 2025 15:39
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 fixes parsing of meta cluster fields to handle newer API versions that return already-parsed objects instead of stringified JSON. The UI was expecting strings and attempting to parse them, which caused issues when the API started returning valid objects directly.

  • Updates type definitions to handle both string and object formats for meta fields (solomon, cores, logging, trace_view)
  • Refactors parsing functions to check if data is already an object before attempting JSON.parse
  • Consolidates parsing logic into dedicated utility functions with proper error handling

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/types/api/meta.ts Updates cluster info types to support both string and object formats for meta fields
src/types/api/trace.ts Removes trace view schema (moved to parseFields)
src/store/reducers/cluster/parseFields.ts New utility module with parsing functions for meta fields
src/store/reducers/cluster/cluster.ts Updates useClusterBaseInfo to use new parsing functions
src/store/reducers/cluster/test/parseFields.test.ts Test coverage for new parsing functions
src/utils/monitoring.ts Updates monitoring functions to handle object/string union types
src/utils/logs.ts Updates logging interface to use proper type
src/services/parsers/parseMetaCluster.ts Removes old parsing logic (replaced by parseFields)
src/containers/Cluster/ClusterInfo/utils/useClusterLinks.ts Simplifies link preparation using parsed objects
src/containers/Cluster/ClusterInfo/utils/tests/prepareLinks.test.ts Removes tests for old parsing functions

@artemmufazalov artemmufazalov added this pull request to the merge queue Sep 1, 2025
Merged via the queue into main with commit 4cfdcf6 Sep 1, 2025
7 checks passed
@artemmufazalov artemmufazalov deleted the meta-decoded-fields branch September 1, 2025 08:04
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.

3 participants