refactor: Replace NotificationContext with useNotificationStore for improved state management (fixes #290, resolves #292). #297
Conversation
|
Warning Rate limit exceeded@junhaoliao has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 47 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
## Walkthrough
This change removes the `NotificationContextProvider` and all related context-based notification logic, replacing it with a new Zustand store (`useNotificationStore`) for managing notification pop-ups. All components and stores now access notification state and actions directly via the new store, eliminating the React context layer for notifications.
## Changes
| File(s) | Change Summary |
|-----------------------------------------------------------------------------------------------|---------------|
| src/App.tsx | Removed `NotificationContextProvider` from the component tree and deleted its import. |
| src/components/AppController.tsx | Replaced use of `NotificationContext` and context-based `postPopUp` with `useNotificationStore`; removed synchronization effect for `postPopUp`. |
| src/components/CentralContainer/Sidebar/SidebarTabs/SettingsTabPanel/index.tsx | Switched from context-based notification posting to direct usage of `useNotificationStore`. |
| src/components/PopUps/PopUpMessageBox.tsx<br>src/components/PopUps/index.tsx | Replaced all notification context usage with direct access to notification state/actions from `useNotificationStore`. |
| src/contexts/NotificationContextProvider.tsx | Deleted file; removed the notification context, provider, and related types. |
| src/stores/contextStore.ts | Removed all notification-related state and actions (`postPopUp`, `setPostPopUp`) from the context store. |
| src/stores/logExportStore.ts<br>src/stores/logFileStore.ts<br>src/stores/queryStore/createQueryControllerSlice.ts<br>src/stores/viewStore.ts | Updated error handling to use `handleErrorWithNotification` from `useNotificationStore` instead of the context store. |
| src/stores/notificationStore.ts | Added new Zustand store (`useNotificationStore`) for notification pop-up management, including state, actions, and types. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Component
participant useNotificationStore
participant UI
Component->>useNotificationStore: postPopUp(message)
useNotificationStore-->>useNotificationStore: Add message with unique ID
UI->>useNotificationStore: Subscribe to popUpMessages
useNotificationStore-->>UI: Provide updated popUpMessages
UI->>Component: Display notification pop-up
UI->>useNotificationStore: handlePopUpMessageClose(messageId)
useNotificationStore-->>useNotificationStore: Remove message by ID
useNotificationStore-->>UI: Provide updated popUpMessagesPossibly related issues
Possibly related PRs
Suggested reviewers
|
|
@junhaoliao I observe that you want to push to notification store branch in y-scope. If that is intended, why not just push this little change directly to that branch? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
src/App.tsx(1 hunks)src/components/AppController.tsx(2 hunks)src/components/CentralContainer/Sidebar/SidebarTabs/SettingsTabPanel/index.tsx(3 hunks)src/components/PopUps/PopUpMessageBox.tsx(2 hunks)src/components/PopUps/index.tsx(2 hunks)src/contexts/NotificationContextProvider.tsx(0 hunks)src/stores/contextStore.ts(0 hunks)src/stores/logExportStore.ts(2 hunks)src/stores/logFileStore.ts(2 hunks)src/stores/notificationStore.ts(1 hunks)src/stores/queryStore/createQueryControllerSlice.ts(2 hunks)src/stores/viewStore.ts(4 hunks)
💤 Files with no reviewable changes (2)
- src/stores/contextStore.ts
- src/contexts/NotificationContextProvider.tsx
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
src/stores/queryStore/createQueryControllerSlice.tssrc/components/PopUps/index.tsxsrc/stores/viewStore.tssrc/stores/logExportStore.tssrc/components/AppController.tsxsrc/App.tsxsrc/stores/logFileStore.tssrc/components/PopUps/PopUpMessageBox.tsxsrc/components/CentralContainer/Sidebar/SidebarTabs/SettingsTabPanel/index.tsxsrc/stores/notificationStore.ts
🧬 Code Graph Analysis (1)
src/stores/notificationStore.ts (1)
src/typings/common.ts (1)
WithId(12-12)
🔇 Additional comments (22)
src/stores/logExportStore.ts (2)
10-10: Import updated to use the new notification storeThe import has been correctly updated to use the new Zustand-based notification store instead of the previous context-based approach, which aligns with the PR objective.
53-53: Notification access properly migrated to the new storeThe code now correctly retrieves the
postPopUpfunction fromuseNotificationStoreinstead of the previous context store, maintaining the same error handling capabilities while benefiting from Zustand's improved state management.src/stores/queryStore/createQueryControllerSlice.ts (2)
6-6: Import updated to use the new notification storeThe import has been correctly updated to use the new Zustand-based notification store, which aligns with the PR's goal of replacing context-based notifications with a more accessible store implementation.
57-57: Notification access properly migrated to the new storeThe error handling now correctly retrieves the
postPopUpfunction fromuseNotificationStoreinstead of the previous context store, maintaining the same error notification capability while leveraging Zustand's better performance characteristics.src/stores/viewStore.ts (4)
26-26: Import updated to use the new notification storeThe import has been correctly added to use the new Zustand-based notification store, aligning with the PR's objective of improving state management for notifications.
142-142: Notification access properly migrated in filterLogs methodThe error handling in the
filterLogsmethod now correctly retrieves thepostPopUpfunction fromuseNotificationStore, maintaining the same error notification capability while benefiting from the improved state management approach.
196-196: Notification access properly migrated in loadPageByAction methodThe error handling in the
loadPageByActionmethod now correctly retrieves thepostPopUpfunction fromuseNotificationStore, consistent with the other changes in this PR.
246-246: Notification access properly migrated in updateIsPrettified methodThe error handling in the
updateIsPrettifiedmethod now correctly retrieves thepostPopUpfunction fromuseNotificationStore, completing the consistent migration of all notification access points in this file.src/components/PopUps/index.tsx (2)
6-6: Import updated to use the new notification storeThe import has been correctly updated to use the new Zustand-based notification store, which is a key part of the notification system refactoring.
18-18: Component properly migrated to use the Zustand storeThe component now correctly uses the Zustand store to access
popUpMessagesinstead of retrieving it from context. This simplifies the code and eliminates the need for context consumption, while maintaining the same functionality.src/App.tsx (1)
13-17: Excellent refactoring by removing the NotificationContextProvider!The component tree is now simpler with UrlContextProvider as the only top-level context provider. This change aligns perfectly with the PR's objective of replacing context-based notifications with Zustand stores.
src/stores/logFileStore.ts (2)
27-27: Import for useNotificationStore looks good.This import correctly replaces the previous context-based notification system.
125-125: Successfully migrated to useNotificationStore.The notification handling is now correctly sourced from the Zustand store, maintaining the same functionality while aligning with the new architecture.
src/components/CentralContainer/Sidebar/SidebarTabs/SettingsTabPanel/index.tsx (3)
1-1: Clean import statement.Removed the unnecessary useContext import since it's no longer needed after the migration to Zustand.
15-15: Properly imported useNotificationStore.This import correctly brings in the new Zustand store for notifications.
138-138: Successfully implemented useNotificationStore with selector.This line efficiently extracts just the postPopUp function from the notification store using a selector pattern, which is a good practice with Zustand stores.
src/components/PopUps/PopUpMessageBox.tsx (2)
18-18: Good import with type extraction.The import now includes the PopUpMessage type directly from the notification store, which is a cleaner approach than importing it separately.
42-43: Correctly implemented Zustand selector.The component now properly uses the useNotificationStore with a selector to extract only the handlePopUpMessageClose function, which follows Zustand best practices.
src/components/AppController.tsx (2)
16-16: Clean implementation of the notification store import.This import aligns with the PR objective of replacing context-based notifications with a Zustand store.
90-90: Good replacement of context with store.The refactoring from context to Zustand store simplifies state management while preserving the same functionality.
src/stores/notificationStore.ts (2)
1-52: Well-implemented Zustand store for notification management.The implementation is clean, type-safe, and follows best practices. The store effectively replaces the context-based approach while maintaining the same functionality.
33-47: Good implementation of the postPopUp function.The function correctly handles adding new messages with unique IDs and preserving the existing messages. Placing new messages at the beginning of the array ensures newer notifications appear first.
…in multiple components
Co-authored-by: Henry8192 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/stores/notificationStore.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
src/stores/notificationStore.ts
🧬 Code Graph Analysis (1)
src/stores/notificationStore.ts (3)
src/typings/common.ts (1)
WithId(12-12)src/typings/logs.ts (1)
LOG_LEVEL(56-56)src/typings/notifications.ts (1)
DO_NOT_TIMEOUT_VALUE(36-36)
🔇 Additional comments (5)
src/stores/notificationStore.ts (5)
1-9: Imports are well-structured.External and local imports are clearly separated and relevant types (
WithId,LOG_LEVEL,DO_NOT_TIMEOUT_VALUE,PopUpMessage) are correctly brought in.
11-21: Types and interfaces are clear and concise.The separation of
NotificationValues,NotificationActions, and their composition intoNotificationStatefollows good practices for maintainable store typing.
22-24: Default state initialisation is correct.
NOTIFICATION_STORE_DEFAULTprovides a clear initial value forpopUpMessages.
28-50: Store creation and action logic looks solid.The implementation of
handlePopUpMessageCloseandpostPopUpcorrectly updates state immutably and prepends new messages. Leveragingsetcallbacks maintains consistency.
69-71: Exports are appropriate.The named export for
PopUpMessageand the default export of the store hook align with consumption patterns elsewhere.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/stores/logFileStore.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
src/stores/logFileStore.ts
🧬 Code Graph Analysis (1)
src/stores/logFileStore.ts (1)
src/stores/notificationStore.ts (1)
handleErrorWithNotification(70-70)
🔇 Additional comments (2)
src/stores/logFileStore.ts (2)
26-26: Good choice migrating to the new notification store.The import change aligns with the PR objective of replacing the Context-based notification system with a Zustand store implementation, which should improve performance for frequent updates.
146-146: Clean update to use the new notification store.Correctly updated to retrieve the
postPopUpfunction from the new Zustand store instead of the previous context-based approach. This maintains the same functionality while benefiting from the improved state management architecture.
Description
This PR replaces the React Context-based notification system with a Zustand store implementation. The change simplifies the codebase by:
The implementation maintains all the existing functionality:
The Zustand approach provides better performance characteristics than React Context, especially for frequent updates, and makes the notification system easier to use in components.
Checklist
breaking change.
Validation performed
npm run build; npm run previewand verified that the pop-up box show up when accessing an non-existent file. (see Bug: Error pop-ups don't appear when loading non-existent files #290 for issue reproduction).Summary by CodeRabbit