-
Notifications
You must be signed in to change notification settings - Fork 17
fix(TenantNavigation): use query for tenant page #3094
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.
Pull Request Overview
This PR refactors the tenant page state management to use URL query parameters via the useTenantPage hook instead of storing the state directly in the Redux store. This change aligns with the goal of connecting the tenant page to the useSetting hook pattern while maintaining the state in the URL.
Key changes:
- Introduced a new
useTenantPagehook that manages tenant page state through URL query params and local settings - Removed
tenantPagefrom Redux state, reducer actions, and URL state mapping - Updated all components to use
handleTenantPageChangefromuseTenantPageinstead of dispatching Redux actions
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/store/state-url-mapping.ts | Removed tenantPage entries from URL mapping configuration since it's now managed by use-query-params |
| src/store/reducers/tenant/types.ts | Removed tenantPage from TenantState interface |
| src/store/reducers/tenant/tenant.ts | Removed setTenantPage action and related initialization logic from Redux slice |
| src/containers/Tenant/utils/schemaActions.tsx | Added setTenantPage to interface and updated calls to use parameter instead of Redux dispatch |
| src/containers/Tenant/utils/controls.tsx | Added setTenantPage to interface and updated calls to use parameter instead of Redux dispatch |
| src/containers/Tenant/TenantPages.tsx | Added tenantPage to AdditionalQueryParams type |
| src/containers/Tenant/TenantNavigation/useTenantNavigation.tsx | Implemented new useTenantPage hook with URL query param and settings management |
| src/containers/Tenant/ObjectSummary/SchemaTree/SchemaTree.tsx | Added handleTenantPageChange from useTenantPage hook and passed to action helpers |
| src/containers/Tenant/ObjectSummary/SchemaActions.tsx | Updated to use useTenantPage hook instead of Redux selector |
| src/containers/Tenant/ObjectSummary/ObjectSummary.tsx | Added handleTenantPageChange from useTenantPage and passed to controls |
| src/containers/Tenant/ObjectGeneral/ObjectGeneral.tsx | Updated to get tenantPage from useTenantPage hook instead of Redux selector |
| src/containers/Tenant/Diagnostics/TenantOverview/TenantOverview.tsx | Added handleTenantPageChange from useTenantPage for navigation handler |
Comments suppressed due to low confidence (1)
src/containers/Tenant/ObjectSummary/SchemaTree/SchemaTree.tsx:184
- The
handleTenantPageChangedependency is missing from theuseMemodependency array on line 170. SincehandleTenantPageChangeis used in the memoized function (line 155), it must be included in the dependencies to ensure the memoized value updates correctly when the callback changes.
}, [
actionsSchemaData,
createDirectoryFeatureAvailable,
dispatch,
input,
isActionsDataFetching,
isDirty,
isStreamingInfoFetching,
onActivePathUpdate,
streamingSysData,
database,
databaseFullPath,
controlPlane,
clusterMonitoring,
]);
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.
12 files reviewed, no comments
Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format
8710161 to
2903bd1
Compare
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
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
|
@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.
12 files reviewed, no comments
Part of #2892
Stand: https://nda.ya.ru/t/BEIjuuLI7N8Qd7
Goal: connect tenant page to
useSettinghook instead of direct LS access in storeCI Results
Test Status:⚠️ FLAKY
📊 Full Report
Test Changes Summary ⏭️2
⏭️ Skipped Tests (2)
Bundle Size: ✅
Current: 66.13 MB | Main: 66.12 MB
Diff: +2.75 KB (0.00%)
✅ Bundle size unchanged.
ℹ️ CI Information
Greptile Overview
Greptile Summary
Refactored tenant page state management from Redux to URL query parameters with persistent settings storage. The changes move
tenantPagestate out of Redux into URL query params usinguse-query-paramslibrary, while maintaining user preferences through theuseSettinghook. The newuseTenantPagehook manages synchronization between URL state and saved settings, providing a consistent source of truth across the application.Key changes:
useTenantPagehook combining URL query params with settings persistencetenantPagefrom Redux state andsetTenantPageactionhandleTenantPageChangecallback instead of dispatching Redux actionsstate-url-mappingsystemConfidence Score: 4/5
use-query-paramsover legacy Redux state), consistent application of patterns across all files, proper validation with Zod schemas and fallbacks. Tests are passing with only expected skips. The implementation aligns with the codebase's migration toward query param management.Important Files Changed
File Analysis
useSettinghook, addeduseTenantPagecustom hook for managing tenant page statetenantPagefrom Redux state and deletedsetTenantPageaction, simplified state managementuseTenantPagehook instead of Redux selectorsetTenantPagecallback instead of dispatching Redux actionsetTenantPagecallback instead of dispatching Redux actionSequence Diagram
sequenceDiagram participant User participant Navigation as TenantNavigation participant Hook as useTenantPage participant Query as useQueryParams participant Settings as useSetting participant LS as LocalStorage User->>Navigation: Click navigation item Navigation->>Hook: handleTenantPageChange(pageId) Hook->>Query: setQueryParams({tenantPage: pageId}) Query->>Query: Update URL query parameter Hook->>Settings: setInitialTenantPage(pageId) Settings->>LS: Store in YDB settings service Note over Hook,Query: On mount / URL change Hook->>Query: Read tenantPageFromQuery Hook->>Settings: Read initialTenantPage Hook->>Hook: Parse with tenantPageSchema.catch(fallback) alt Valid query param Hook->>Hook: Use query param as tenantPage Hook->>Settings: Sync to settings if differs else Invalid query param Hook->>Hook: Use saved setting as tenantPage Hook->>Query: Replace URL param with saved value end Hook-->>Navigation: Return {tenantPage, handleTenantPageChange} Navigation-->>User: Display active page