-
Notifications
You must be signed in to change notification settings - Fork 17
feat: save storage type in user settings #3160
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
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.
3 files reviewed, 3 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.
Pull request overview
This PR adds persistence of the storage type selection (groups/nodes view) to user settings, allowing the application to remember the user's preferred storage view across sessions.
Key Changes:
- Added
STORAGE_TYPEsetting key with default value ofSTORAGE_TYPES.groups - Modified
handleStorageTypeChangeto save the selected type to user settings - Added
useSaveStorageTypehook to initialize URL parameter from saved settings on component mount
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/store/reducers/settings/constants.ts | Adds STORAGE_TYPE setting key and default value to user settings configuration |
| src/containers/Storage/useStorageQueryParams.ts | Implements storage type persistence by saving to settings on change and initializing URL from saved settings; fixes missing dependency in useEffect |
| src/containers/Storage/PaginatedStorage.tsx | Integrates the new useSaveStorageType hook to apply saved storage type on component mount |
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.
3 files reviewed, no comments
| if (normalizedStorageType !== queryStorageType) { | ||
| setQueryStorageType(normalizedStorageType); | ||
| } |
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.
Is it possible to get to table with specific storage type in query? With your code in such case storage type from query will always be replaced with the saved one.
I think it should be two ways sync like here: https://github.com/ydb-platform/ydb-embedded-ui/blob/main/src/containers/Tenant/TenantNavigation/useTenantNavigation.tsx#L60
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.
No, normalizedStorageType calculates with storageTypeSchema.parse(queryStorageType ?? savedStorageType). So, queryStorageType has priority and will be taken, if it is presented.
closes #2283
Greptile Overview
Greptile Summary
This PR implements persistent user preference for storage view type (groups vs nodes), resolving issue #2283 where users wanted their storage type selection to be remembered across sessions.
STORAGE_TYPEsetting key with default value ofgroupsto user settingsuseSaveStorageTypehook that syncs URL query params with saved user preferenceshandleStorageTypeChangeto save user preference when storage type is changedConfidence Score: 5/5
useSettinghook and query param management. No breaking changes, backward compatible with proper defaults. Previous typo feedback has been addressed.Important Files Changed
File Analysis
useSaveStorageTypehook to persist storage type selection and modifiedhandleStorageTypeChangeto save user preferenceuseSaveStorageTypehook to initialize storage type from saved settingsSTORAGE_TYPEsetting key with default value ofgroupsto user settings configurationSequence Diagram
sequenceDiagram participant User participant PaginatedStorage participant useSaveStorageType participant useStorageQueryParams participant LocalStorage participant URL User->>PaginatedStorage: Load storage page PaginatedStorage->>useSaveStorageType: Initialize hook useSaveStorageType->>LocalStorage: Read saved storage type (default: "groups") useSaveStorageType->>URL: Check query param "type" alt Query param exists useSaveStorageType->>useSaveStorageType: Use query param value else No query param useSaveStorageType->>URL: Set query param to saved value end User->>User: Change storage type (groups/nodes) User->>useStorageQueryParams: handleStorageTypeChange(newType) useStorageQueryParams->>URL: Update query param useStorageQueryParams->>LocalStorage: Save new preference User->>PaginatedStorage: Return to storage page later PaginatedStorage->>useSaveStorageType: Initialize hook useSaveStorageType->>LocalStorage: Read saved storage type useSaveStorageType->>URL: Set query param to saved preferenceCI Results
Test Status:⚠️ FLAKY
📊 Full Report
Test Changes Summary ⏭️2
⏭️ Skipped Tests (2)
Bundle Size: 🔺
Current: 62.33 MB | Main: 62.33 MB
Diff: +3.37 KB (0.01%)
ℹ️ CI Information