Skip to content

Conversation

@artemmufazalov
Copy link
Member

@artemmufazalov artemmufazalov commented Nov 11, 2025

Part of #2892

Separated part of #3054 to ensure PR code readability and to fix different problems one by one - when I update all 30 settings at once, 75 tests fail

To test setting, you need to change width of table columns

Stand with LS only: https://nda.ya.ru/t/9h38FrvI7MnmFj
Stand with external settings: https://nda.ya.ru/t/A6j4w3EB7MnmGz

CI Results

Test Status: ⚠️ FLAKY

📊 Full Report

Total Passed Failed Flaky Skipped
378 375 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: 66.11 MB | Main: 66.09 MB
Diff: +0.02 MB (0.04%)

⚠️ 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.

@Raubzeug
Copy link
Contributor

Raubzeug commented Nov 12, 2025

@greptile-review

Copilot finished reviewing on behalf of Raubzeug November 12, 2025 06:03
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 introduces a meta settings infrastructure to support external storage of user settings and implements it for table column width persistence. It's part of a larger effort to address 30 settings-related issues, with this PR focusing on the foundational infrastructure and table width use case.

Key Changes:

  • Created new useSetting hook with external storage support via meta API
  • Implemented batched settings API with optimistic updates
  • Migrated settings constants to centralized location
  • Added loading states for externally-stored settings

Reviewed Changes

Copilot reviewed 55 out of 56 changed files in this pull request and generated 12 comments.

Show a summary per file
File Description
src/store/reducers/settings/useSetting.ts New hook supporting both localStorage and external meta API settings with debouncing and loading states
src/store/reducers/settings/api.ts RTK Query endpoints for settings with batching and optimistic updates
src/services/api/metaSettings.ts Meta API client with request batching (100ms window)
src/utils/hooks/useTableResize.ts Refactored to use new useSetting hook, returns loading state
src/store/reducers/settings/constants.ts Centralized settings keys and default values (moved from utils/constants.ts)
src/store/reducers/settings/utils.ts Utility functions for settings value parsing and localStorage operations
src/store/reducers/settings/settings.ts Updated to support optional setting names
src/store/reducers/authentication/authentication.ts Added anonymous user ID support for unauthenticated settings
src/types/api/settings.ts Type definitions for settings API
src/types/api/whoami.ts Added UserID field for anonymous users
src/services/api/baseMeta.ts Extracted base class for meta API clients
src/utils/hooks/useSetting.ts Updated to call settingsManager directly (compensates for removal from setSettingValue)
src/components/ResizeableDataTable/ResizeableDataTable.tsx Shows loading skeleton when table widths are loading
src/components/PaginatedTable/ResizeablePaginatedTable.tsx Shows loading skeleton when table widths are loading
/.tsx (30+ files) Updated to use SETTING_KEYS from new location

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.

56 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

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.

56 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@astandrik
Copy link
Collaborator

What will happen if cluster does not support settings backend?

@astandrik
Copy link
Collaborator

Could you describe what will happen if our set setting request to backend fails? We will live on localStorage value till the next page refresh?

(Sorry for asking - I'm sure its in the code but Its pretty hard to observe pr of this size at once)

@artemmufazalov
Copy link
Member Author

Could you describe what will happen if our set setting request to backend fails? We will live on localStorage value till the next page refresh?

(Sorry for asking - I'm sure its in the code but Its pretty hard to observe pr of this size at once)

If meta settings configured, the app won't work normally in case of errors.

If get setting fails, nothing happens, we just use last saved local storage value.

If set settings fails, we revert to previous value (https://github.com/ydb-platform/ydb-embedded-ui/blob/external-table-width/src/store/reducers/settings/api.ts#L64). So if meta is configured, but doesn't work, it will be impossible to change setting - it will be always reverted

@artemmufazalov
Copy link
Member Author

What will happen if cluster does not support settings backend?

Settings backend is meta service, it doesn't depend on cluster. If there is such service in installation meta, it should work everywhere. If not, we just do not configure it in uiFactory and local storage will be used

@astandrik
Copy link
Collaborator

👀

@artemmufazalov artemmufazalov added this pull request to the merge queue Nov 14, 2025
Merged via the queue into main with commit 56f1fc6 Nov 14, 2025
7 checks passed
@artemmufazalov artemmufazalov deleted the external-table-width branch November 14, 2025 13:36
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