-
Notifications
You must be signed in to change notification settings - Fork 17
feat: configure meta settings, use for table columns width #3068
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
3115c35 to
0c2b19b
Compare
|
@greptile-review |
There was a problem hiding this 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
useSettinghook 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 |
There was a problem hiding this 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
There was a problem hiding this 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
|
What will happen if cluster does not support settings backend? |
|
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 |
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 |
|
👀 |
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
Test Changes Summary ⏭️2
⏭️ Skipped Tests (2)
Bundle Size: 🔺
Current: 66.11 MB | Main: 66.09 MB
Diff: +0.02 MB (0.04%)
ℹ️ CI Information