Fix : View(list/card/board) retention for encounters/resource/users pages #10039
Conversation
WalkthroughThis pull request introduces a comprehensive view management system across multiple components. The changes focus on implementing persistent view state retention using a new custom hook Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Finishing Touches
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: 2
🧹 Nitpick comments (7)
src/Utils/ViewCache.ts (2)
1-3: Add namespace prefix to localStorage keys.The
getKeyfunction should include a namespace prefix to prevent potential naming conflicts with other localStorage keys used in the application.const getKey = (name: string) => { - return `${name}`; + return `view_preference_${name}`; };
15-18: Use TypeScript enum or const for view keys.Consider using a TypeScript enum or const object to maintain type safety and centralize the view keys configuration.
export const VIEW_TYPES = { USERS: 'users', RESOURCE: 'resource', APPOINTMENTS: 'appointments' } as const; const invalidateAll = () => { Object.values(VIEW_TYPES).forEach(invalidate); };src/components/Common/View.tsx (2)
14-14: Extract default view as a prop.The default view ("board") is hardcoded in the useView hook call. Consider making it configurable through props.
+interface ViewProps { + name: typeof VIEW_TYPES[keyof typeof VIEW_TYPES]; + board: ComponentType; + list: ComponentType; + defaultView?: "board" | "list"; +} -const [view] = useView(name, "board"); +const [view] = useView(name, props.defaultView ?? "board");
21-21: Improve type safety for view selection.The type assertion in
view as keyof typeof viewscould be unsafe. Consider adding runtime validation.-const SelectedView = views[view as keyof typeof views] || board; +const isValidView = (v: string): v is keyof typeof views => + v === "board" || v === "list"; +const SelectedView = isValidView(view) ? views[view] : board;src/Utils/useView.ts (1)
10-13: Add error handling for localStorage operations.The localStorage operations should be wrapped in try-catch blocks to handle potential errors (e.g., when storage is full or disabled).
const updateView = (newView: string) => { - localStorage.setItem(name, newView); - setView(newView); + try { + localStorage.setItem(name, newView); + setView(newView); + } catch (error) { + console.error(`Failed to update view preference: ${error}`); + setView(newView); // Still update state even if storage fails + } };src/components/Facility/FacilityUsers.tsx (1)
126-126: Remove debugging console.log statement.This console.log statement appears to be a debugging artifact and should be removed.
- console.log("activeTab", activeTab);src/components/Auth/Login.tsx (1)
33-33: Move cache invalidation after validation.The cache invalidation should occur only after successful validation to avoid unnecessary cache clearing.
const handleSubmit = async (e: React.FormEvent) => { e.preventDefault(); - ViewCache.invalidateAll(); const validated = validateData(); if (!validated) return; + ViewCache.invalidateAll(); staffLoginMutation.mutate(validated); };Also applies to: 246-246
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/Routers/routes/ResourceRoutes.tsx(1 hunks)src/Utils/ViewCache.ts(1 hunks)src/Utils/useView.ts(1 hunks)src/components/Auth/Login.tsx(2 hunks)src/components/Common/View.tsx(1 hunks)src/components/Facility/FacilityUsers.tsx(3 hunks)src/components/Resource/ResourceBoard.tsx(3 hunks)src/components/Resource/ResourceList.tsx(3 hunks)src/pages/Appointments/AppointmentsPage.tsx(4 hunks)
🔇 Additional comments (8)
src/Routers/routes/ResourceRoutes.tsx (1)
10-10: LGTM! Clean implementation of view routing.The routing implementation is simplified and well-structured, properly utilizing the new View component.
src/components/Resource/ResourceBoard.tsx (1)
27-27: LGTM! View state management has been simplified.The changes effectively centralize view state management using the
useViewhook, removing the need for explicit navigation and local storage manipulation.Also applies to: 40-40, 97-97
src/components/Facility/FacilityUsers.tsx (1)
19-19: LGTM! View state management has been updated consistently.The changes align with the PR objective by implementing view retention using the
useViewhook.Also applies to: 124-124
src/components/Resource/ResourceList.tsx (1)
23-23: LGTM! View state management has been simplified consistently.The changes align with the broader refactor by:
- Using the
useViewhook for centralized view state management.- Removing navigation-based view switching.
- Simplifying the control flow.
Also applies to: 28-28, 223-223
src/pages/Appointments/AppointmentsPage.tsx (4)
68-68: LGTM! Clean import of the view retention hook.The import of
useViewhook aligns with the PR objective of implementing view retention.
332-333: LGTM! Proper implementation of the Tabs component.The Tabs component is correctly configured with the new
activeTabstate and includes proper type casting in theonValueChangehandler.
641-641: LGTM! Clean conditional rendering implementation.The ternary operator provides a clean way to toggle between board and list views based on the
activeTabstate.
242-242: Implementation of view retention looks good.The
useViewhook is properly configured with "appointments" namespace and "board" as the default view.Let's verify the view persistence across navigation:
✅ Verification successful
View persistence implementation verified successfully
The
useViewhook implementation properly handles view persistence using localStorage with cross-tab synchronization. The single usage with "appointments" namespace prevents any state conflicts.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent usage of useView hook across the codebase # Expect: No conflicting implementations using the same namespace # Search for other usages of the appointments namespace rg "useView\(['\"]appointments['\"]" -A 2Length of output: 293
Script:
#!/bin/bash # Search for the useView hook implementation ast-grep --pattern 'const useView = $_' # Also search for any exports of useView rg "export.*useView"Length of output: 110
Script:
#!/bin/bash # Get the implementation of useView cat src/Utils/useView.tsLength of output: 709
| useEffect(() => { | ||
| const interval = setInterval(() => { | ||
| const storedView = localStorage.getItem(name); | ||
| if (storedView !== view) { | ||
| setView(storedView || defaultView); | ||
| } | ||
| }, 100); | ||
| return () => { | ||
| clearInterval(interval); | ||
| }; | ||
| }, [name, view]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace polling with storage event listener.
The current implementation uses polling with a 100ms interval, which is inefficient. Consider using the storage event listener instead.
useEffect(() => {
- const interval = setInterval(() => {
+ const handleStorageChange = (e: StorageEvent) => {
+ if (e.key === name) {
+ const storedView = e.newValue;
+ if (storedView !== view) {
+ setView(storedView || defaultView);
+ }
+ }
+ };
+
+ window.addEventListener('storage', handleStorageChange);
+ return () => {
+ window.removeEventListener('storage', handleStorageChange);
+ };
- const storedView = localStorage.getItem(name);
- if (storedView !== view) {
- setView(storedView || defaultView);
- }
- }, 100);
- return () => {
- clearInterval(interval);
- };
}, [name, view]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| const interval = setInterval(() => { | |
| const storedView = localStorage.getItem(name); | |
| if (storedView !== view) { | |
| setView(storedView || defaultView); | |
| } | |
| }, 100); | |
| return () => { | |
| clearInterval(interval); | |
| }; | |
| }, [name, view]); | |
| useEffect(() => { | |
| const handleStorageChange = (e: StorageEvent) => { | |
| if (e.key === name) { | |
| const storedView = e.newValue; | |
| if (storedView !== view) { | |
| setView(storedView || defaultView); | |
| } | |
| } | |
| }; | |
| window.addEventListener('storage', handleStorageChange); | |
| return () => { | |
| window.removeEventListener('storage', handleStorageChange); | |
| }; | |
| }, [name, view]); |
There was a problem hiding this comment.
This wont be needed once #9768 gets merged
Proposed Changes
Fixes View retention in appoinments, resource and users page #10006
View retention added for appointments
View retention added for facility users
View retention added for resource page
view.mp4
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Chores