Fix : View Retention in Users Tab After Navigation and Logout #9485
Conversation
WalkthroughThis pull request introduces a new utility module Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Routers/routes/UserRoutes.tsx (1)
14-16: Consider adding type safety for view parameter.While the implementation is clean, consider adding runtime validation for the view parameter to prevent potential routing issues with invalid values.
- "/users": () => <Redirect to={`/users/${getDefaultUsersView()}`} />, + "/users": () => { + const view = getDefaultUsersView(); + if (view !== "list" && view !== "card") { + console.error(`Invalid view type: ${view}`); + return <Redirect to="/users/card" />; + } + return <Redirect to={`/users/${view}`} />; + },src/components/Users/ManageUsers.tsx (1)
108-113: Consider adding error handling for navigation failures.The handleTabChange function looks good but should handle potential navigation failures.
const handleTabChange = (tab: number) => { setActiveTab(tab); const newView = tab === 1 ? "list" : "card"; localStorage.setItem("usersDefaultView", newView); - navigate(`/users/${newView}`); + navigate(`/users/${newView}`).catch((error) => { + console.error("Navigation failed:", error); + // Optionally show a user-friendly error notification + }); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Routers/routes/UserRoutes.tsx(1 hunks)src/components/Users/ManageUsers.tsx(4 hunks)
🔇 Additional comments (3)
src/Routers/routes/UserRoutes.tsx (1)
10-11: LGTM! Clean implementation of view preference retrieval.
The function is well-focused and provides a good fallback mechanism.
src/components/Users/ManageUsers.tsx (2)
31-31: LGTM! Well-structured component initialization.
The component properly handles the defaultView prop and initializes the state accordingly.
Also applies to: 50-50
121-123: Verify the integration with UserListView component.
Ensure that the UserListView component properly handles the tab change callback.
✅ Verification successful
The UserListView component properly handles the tab change callback
The verification shows that:
- The
UserListViewcomponent inUserListAndCard.tsxcorrectly defines theonTabChangeprop in its interface asonTabChange: (tab: number) => void - The component properly receives and uses the
onTabChangeprop to handle tab changes - The implementation in
ManageUsers.tsxcorrectly passes thehandleTabChangecallback to theUserListViewcomponent
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for UserListView component implementation to verify onTabChange prop handling
ast-grep --pattern 'interface $interface {
$$$
onTabChange: $type
$$$
}'
# Search for actual usage of onTabChange in the component
ast-grep --pattern 'function UserListView($props) {
$$$
onTabChange($$$)
$$$
}'
Length of output: 168
Script:
#!/bin/bash
# Let's try a broader search for UserListView component and its props
rg -A 10 "UserListView" --type typescript
# Also search for any file containing UserListView in its name
fd UserListView
# Search for tab change handling patterns
rg "onTabChange|handleTabChange" --type typescript
Length of output: 182
Script:
#!/bin/bash
# Search for UserListView component and its props without type filter
rg -A 10 "UserListView"
# Search for any file containing UserListView in its name
fd UserListView
# Search for tab change handling patterns
rg "onTabChange|handleTabChange"
Length of output: 6358
|
I'd recommend creating a util. function that handles the logic to create an abstraction between the storage, since we have similar use cases in other places. |
|
Also, let's not have a dedicated route defined for it. |
Oh true, resources and shifting if I recall. |
|
And appointments page in DO Branch, (although the list view is not ready there yet) |
For Users i have done. |
| "/shifting": () => ( | ||
| <Redirect | ||
| to={`/shifting/${getDefaultView("defaultResourceView", "board")}`} | ||
| /> | ||
| ), | ||
| "/shifting/board": () => <BoardView />, | ||
| "/shifting/list": () => <ListView />, |
There was a problem hiding this comment.
@Jacobjeevan @rithviknishad Shifting and Resources tabs have two dedicated components for each list and board view and dedicated routes for them.
Should i call them in a single file and conditionally handle the views or let it be same
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/Routers/routes/ShiftingRoutes.tsx (1)
Line range hint
1-1: Consider centralizing localStorage keysTo improve maintainability and prevent key conflicts, consider creating a central constants file for localStorage keys:
// src/constants/storage.ts export const STORAGE_KEYS = { VIEWS: { USERS: 'usersDefaultView', RESOURCES: 'resourcesDefaultView', SHIFTING: 'shiftingDefaultView' } } as const;This would provide:
- Type safety for keys
- Easy discovery of existing keys
- Prevention of duplicate keys
- Single source of truth for storage keys
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Routers/routes/ResourceRoutes.tsx(1 hunks)src/Routers/routes/ShiftingRoutes.tsx(1 hunks)src/Routers/routes/UserRoutes.tsx(1 hunks)src/Utils/viewStorageUtils.ts(1 hunks)src/components/Users/ManageUsers.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Routers/routes/UserRoutes.tsx
- src/components/Users/ManageUsers.tsx
| export const getDefaultView = (key: string, defaultValue: string): string => { | ||
| return localStorage.getItem(key) || defaultValue; | ||
| }; | ||
|
|
||
| export const setDefaultView = (key: string, value: string): void => { | ||
| localStorage.setItem(key, value); | ||
| }; |
There was a problem hiding this comment.
I meant something like:
const [view, setView] = useView("shifting");
| "/shifting": () => ( | ||
| <Redirect | ||
| to={`/shifting/${getDefaultView("defaultShiftingView", "board")}`} | ||
| /> | ||
| ), | ||
| "/shifting/board": () => <BoardView />, | ||
| "/shifting/list": () => <ListView />, |
There was a problem hiding this comment.
yup, let's remove these routes too and instead we could do:
(which uses that hook behind the scenes)
"/shifting": () => <View
name="shifting"
board={BoardView}
list={ListView}
/>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/components/Shifting/ShiftingList.tsx (2)
39-39: Consider calling setDefaultView before navigationFor better user experience, consider storing the view preference before navigating to prevent any potential race conditions.
const onBoardViewBtnClick = () => { + setDefaultView("defaultShiftView", "board"); navigate("/shifting/board", { query: qParams }); - setDefaultView("defaultShiftView", "board"); };
22-22: Consider using constants for view types and storage keysTo maintain consistency and prevent typos, consider extracting the view types and storage keys into constants.
// Add to a constants file export const VIEW_TYPES = { BOARD: 'board', LIST: 'list' } as const; export const STORAGE_KEYS = { SHIFT_VIEW: 'defaultShiftView', RESOURCE_VIEW: 'defaultResourceView', USER_VIEW: 'defaultUserView' } as const;Also applies to: 39-39
src/components/Resource/ResourceBoard.tsx (1)
51-51: Apply consistent view management patternSimilar to the ShiftingList component, consider:
- Moving setDefaultView before navigation
- Using constants for view types and storage keys
const onListViewBtnClick = () => { + setDefaultView(STORAGE_KEYS.RESOURCE_VIEW, VIEW_TYPES.LIST); navigate("/resource/list", { query: qParams }); - setDefaultView("defaultResourceView", "list"); };src/components/Shifting/ShiftingBoard.tsx (2)
82-82: Apply consistent view management patternSimilar to other components, consider:
- Moving setDefaultView before navigation
- Using constants for view types and storage keys
const onListViewBtnClick = () => { + setDefaultView(STORAGE_KEYS.SHIFT_VIEW, VIEW_TYPES.LIST); navigate("/shifting/list", { query: qParams }); - setDefaultView("defaultShiftView", "list"); };
33-33: Consider creating a reusable view switching hookThere's a pattern of duplicated view switching logic across components. Consider creating a custom hook to encapsulate this behavior.
// src/hooks/useViewSwitcher.ts import { navigate } from "raviger"; import { setDefaultView } from "@/Utils/viewStorageUtils"; import { VIEW_TYPES, STORAGE_KEYS } from "@/common/constants"; type ViewConfig = { storageKey: typeof STORAGE_KEYS[keyof typeof STORAGE_KEYS]; listPath: string; boardPath: string; }; export const useViewSwitcher = (config: ViewConfig) => { const switchToListView = (qParams: Record<string, any>) => { setDefaultView(config.storageKey, VIEW_TYPES.LIST); navigate(config.listPath, { query: qParams }); }; const switchToBoardView = (qParams: Record<string, any>) => { setDefaultView(config.storageKey, VIEW_TYPES.BOARD); navigate(config.boardPath, { query: qParams }); }; return { switchToListView, switchToBoardView }; }; // Usage in components: const { switchToListView } = useViewSwitcher({ storageKey: STORAGE_KEYS.SHIFT_VIEW, listPath: "/shifting/list", boardPath: "/shifting/board" }); // Replace onListViewBtnClick with: const onListViewBtnClick = () => switchToListView(qParams);This approach would:
- Reduce code duplication
- Ensure consistent implementation across components
- Make it easier to modify view switching behavior globally
- Improve maintainability
Also applies to: 82-82
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/Routers/routes/ShiftingRoutes.tsx(1 hunks)src/components/Resource/ResourceBoard.tsx(2 hunks)src/components/Resource/ResourceList.tsx(2 hunks)src/components/Shifting/ShiftingBoard.tsx(2 hunks)src/components/Shifting/ShiftingList.tsx(2 hunks)src/components/Users/ManageUsers.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Routers/routes/ShiftingRoutes.tsx
🔇 Additional comments (3)
src/components/Resource/ResourceList.tsx (1)
25-25: LGTM: View persistence implementation looks good!
The implementation correctly uses the setDefaultView utility to persist the view preference when switching to board view.
Also applies to: 41-41
src/components/Users/ManageUsers.tsx (2)
51-53: LGTM: Proper initialization of view preference!
The implementation correctly retrieves the stored view preference with a sensible default of "card" view.
111-115: LGTM: Clean implementation of view persistence!
The handleTabChange function properly manages both the local state and persistent storage of the view preference. The implementation is concise and follows the single responsibility principle.
Also applies to: 124-124
Proposed Changes
Recording.2024-12-18.173555.mp4
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Chores