Skip to content

Conversation

@Raubzeug
Copy link
Contributor

@Raubzeug Raubzeug commented Sep 5, 2025

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.39 MB | Main: 85.39 MB
Diff: +1.22 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
artemmufazalov previously approved these changes Sep 5, 2025
astandrik
astandrik previously approved these changes Sep 5, 2025
@astandrik
Copy link
Collaborator

/claude_review

@claude
Copy link

claude bot commented Sep 5, 2025

Claude finished @astandrik's task —— View job


Claude Code has completed the review

Review Checklist:

  • Analyze API changes in src/services/api/base.ts
  • Review API export changes in src/services/api/index.ts
  • Check scheme API update in src/services/api/scheme.ts
  • Examine store configuration in src/store/configureStore.ts
  • Verify TypeScript type safety and interfaces
  • Check for hardcoded strings that need i18n
  • Assess performance and security implications
  • Review code standards compliance

Summary

This PR introduces a configurable useRelativePath feature for API requests, allowing schema paths to be relative to the database path. The implementation is clean and follows existing patterns.

✅ Approved Changes

1. TypeScript Type Safety

  • Properly typed BaseAPIParams interface with useRelativePath property (src/services/api/base.ts:21)
  • All new properties correctly typed as undefined | boolean
  • Interface follows existing patterns with required explicit passing

2. API Architecture

  • Clean integration into existing BaseYdbAPI class
  • getSchemaPath method properly handles edge cases (src/services/api/base.ts:97-110)
  • Consistent usage across 13+ API endpoints in viewer.ts

3. Code Standards

  • Follows existing patterns for configuration options
  • Proper use of optional chaining and null safety
  • No state mutations detected

📋 Observations

1. No Breaking Changes

  • Default value is false, maintaining backward compatibility (src/services/api/index.ts:53)
  • Only affects behavior when explicitly enabled

2. Implementation Details

The getSchemaPath method correctly handles:

  • Empty database or path (returns original path)
  • Path equals database (returns empty string)
  • Path starting with database prefix (strips the prefix)
  • All other cases (returns original path)

3. Security Considerations

  • No security issues identified
  • Path manipulation is safe and doesn't allow directory traversal
  • No exposed secrets or authentication issues

✅ No Issues Found

  • ✅ All TypeScript types are properly defined
  • ✅ No hardcoded strings requiring i18n
  • ✅ API calls still go through window.api
  • ✅ No direct API calls bypassing the wrapper
  • ✅ Error handling preserved in all modified code
  • ✅ Performance impact is negligible (simple string operations)

💭 Suggestions for Future Improvements

While not required for this PR:

  1. Consider adding JSDoc comments to getSchemaPath method explaining the relative path behavior
  2. Unit tests for the getSchemaPath method would be beneficial to ensure edge cases remain handled

Overall, this is a well-implemented feature that follows all YDB Embedded UI coding standards and best practices. The changes are minimal, focused, and maintain backward compatibility.

@astandrik astandrik self-requested a review September 5, 2025 14:04
@Raubzeug Raubzeug added this pull request to the merge queue Sep 5, 2025
Merged via the queue into main with commit 3069209 Sep 5, 2025
5 checks passed
@Raubzeug Raubzeug deleted the relative-schema-path branch September 5, 2025 14:17
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.

4 participants