Skip to content

Conversation

@astandrik
Copy link
Collaborator

@astandrik astandrik commented Dec 19, 2025

Greptile Summary

This PR refactors how the settings backend endpoint is resolved by moving the configuration lookup from API construction time to runtime evaluation in the getPath() method.

Key changes:

  • Removed setBaseUrlOverride() method and baseUrlOverride field from MetaSettingsAPI
  • Moved uiFactory import from src/services/api/index.ts to src/services/api/metaSettings.ts
  • Settings backend endpoint and user ID are now resolved dynamically on each getPath() call instead of being cached during initialization

Impact:

  • Makes the settings endpoint configuration more flexible and allows it to change at runtime
  • Aligns with the existing pattern used in src/store/reducers/settings/api.ts where resolveRemoteSettingsClientAndUser() also checks uiFactory dynamically
  • Minimal performance impact since getPath() is not called frequently enough to cause concerns

Confidence Score: 5/5

  • This PR is safe to merge with no risk
  • The refactoring is straightforward and improves code architecture by removing unnecessary state. The dynamic endpoint resolution aligns with existing patterns in the codebase and adds flexibility without introducing bugs or breaking changes.
  • No files require special attention

Important Files Changed

Filename Overview
src/services/api/index.ts Removed uiFactory import and base URL override logic from constructor, simplifying initialization
src/services/api/metaSettings.ts Moved settings backend resolution to getPath() method, evaluating configuration dynamically on each call

Sequence Diagram

sequenceDiagram
    participant App as Application
    participant API as YdbEmbeddedAPI
    participant MSA as MetaSettingsAPI
    participant UIF as uiFactory
    participant Backend as Settings Backend
    
    Note over App,Backend: Before (Old Implementation)
    App->>API: new YdbEmbeddedAPI(useMetaSettings: true)
    API->>UIF: settingsBackend?.getEndpoint()
    API->>UIF: settingsBackend?.getUserId()
    API->>MSA: new MetaSettingsAPI()
    API->>MSA: setBaseUrlOverride(endpoint)
    Note over MSA: Stores endpoint in baseUrlOverride field
    
    App->>MSA: getSingleSetting({name, user})
    MSA->>MSA: getPath(path)
    Note over MSA: Uses cached baseUrlOverride
    MSA->>Backend: HTTP Request to endpoint
    Backend-->>MSA: Response
    MSA-->>App: Setting value
    
    Note over App,Backend: After (New Implementation)
    App->>API: new YdbEmbeddedAPI(useMetaSettings: true)
    API->>MSA: new MetaSettingsAPI()
    Note over MSA: No endpoint cached
    
    App->>MSA: getSingleSetting({name, user})
    MSA->>MSA: getPath(path)
    MSA->>UIF: settingsBackend?.getEndpoint()
    MSA->>UIF: settingsBackend?.getUserId()
    Note over MSA: Resolves endpoint dynamically
    MSA->>Backend: HTTP Request to endpoint
    Backend-->>MSA: Response
    MSA-->>App: Setting value
Loading

CI Results

Test Status: ⚠️ FLAKY

📊 Full Report

Total Passed Failed Flaky Skipped
384 381 0 1 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: 62.61 MB | Main: 62.61 MB
Diff: 0.65 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.

Copilot AI review requested due to automatic review settings December 19, 2025 11:49
@astandrik astandrik requested a review from adameat as a code owner December 19, 2025 11:49
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 breaking change refactors how the settings endpoint is determined for the MetaSettingsAPI. Instead of configuring the endpoint at API construction time via setBaseUrlOverride(), the logic now evaluates uiFactory.settingsBackend at runtime within the getPath() method. This provides more flexibility since the settings backend can be configured after API initialization.

Key changes:

  • Removes initialization-time endpoint configuration from YdbEmbeddedAPI constructor
  • Implements runtime endpoint resolution in MetaSettingsAPI.getPath()
  • Cleans up unused baseUrlOverride state and setter method

Reviewed changes

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

File Description
src/services/api/index.ts Removes initialization logic that configured settings endpoint at construction time; removes unused uiFactory import
src/services/api/metaSettings.ts Adds runtime endpoint resolution in getPath() by checking uiFactory.settingsBackend directly; removes baseUrlOverride state management

getPath(path: string, clusterName?: string) {
if (this.baseUrlOverride) {
return joinBaseUrlAndPath(this.baseUrlOverride, path);
const settingsUserId = uiFactory.settingsBackend?.getUserId?.();
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

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

The variable settingsUserId is retrieved but never used. Looking at the pattern in src/store/reducers/settings/api.ts:124-140, both getUserId() and getEndpoint() are typically checked together to determine if the custom settings backend should be used. However, here only metaSettingsBaseUrl affects the control flow. Consider removing this unused variable retrieval to improve code clarity, or if the userId is needed for some validation or future use, document why it's being checked.

Copilot uses AI. Check for mistakes.
@astandrik astandrik added this pull request to the merge queue Dec 19, 2025
Merged via the queue into main with commit 9b6921a Dec 19, 2025
13 of 15 checks passed
@astandrik astandrik deleted the astandrik.fix-endpoint branch December 19, 2025 12:05
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