refactor: Replace NotificationContext with useNotificationStore for improved state management (fixes #290, resolves #292). #293
Conversation
## Walkthrough
This change removes the `NotificationContextProvider` and all related React context usage for notification pop-ups, replacing it with a new Zustand store (`useNotificationStore`). All components and stores previously using the context now interact directly with the Zustand store for notification state and actions. The context provider and its types are deleted.
## Changes
| File(s) | Change Summary |
|--------------------------------------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| src/App.tsx | Removed `NotificationContextProvider` from the component tree and its import. The app now uses `UrlContextProvider` as the outermost provider. |
| src/components/AppController.tsx<br>src/components/CentralContainer/Sidebar/SidebarTabs/SettingsTabPanel/index.tsx<br>src/components/PopUps/PopUpMessageBox.tsx<br>src/components/PopUps/index.tsx | Replaced usage of `NotificationContext` and `useContext` with the new `useNotificationStore` for notification pop-up state and actions. Removed all context-related imports and logic. |
| src/contexts/NotificationContextProvider.tsx | Deleted the file implementing the notification context provider, its types, and default values. |
| src/stores/contextStore.ts | Removed all properties and methods related to notification pop-ups (`postPopUp`, `setPostPopUp`) from the context store and its types. |
| src/stores/notificationStore.ts | Added a new Zustand store (`useNotificationStore`) to manage notification pop-up messages, including actions for posting and closing notifications. Exported the `PopUpMessage` type. |
| src/stores/logExportStore.ts<br>src/stores/logFileStore.ts<br>src/stores/queryStore/createQueryControllerSlice.ts<br>src/stores/viewStore.ts | Changed notification handling in error cases to use `useNotificationStore` instead of the context store. In `logFileStore.ts`, also resets file name and log data to defaults on load failure. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Component
participant useNotificationStore
Component->>useNotificationStore: postPopUp(message)
useNotificationStore-->>useNotificationStore: Add message with unique ID to popUpMessages
Component->>useNotificationStore: handlePopUpMessageClose(messageId)
useNotificationStore-->>useNotificationStore: Remove message by ID from popUpMessages
Component->>useNotificationStore: Read popUpMessages for renderingPossibly related issues
Possibly related PRs
Suggested reviewers
|
![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(3 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/logExportStore.tssrc/stores/queryStore/createQueryControllerSlice.tssrc/components/PopUps/PopUpMessageBox.tsxsrc/App.tsxsrc/components/AppController.tsxsrc/components/CentralContainer/Sidebar/SidebarTabs/SettingsTabPanel/index.tsxsrc/stores/logFileStore.tssrc/stores/viewStore.tssrc/components/PopUps/index.tsxsrc/stores/notificationStore.ts
🧬 Code Graph Analysis (2)
src/stores/logFileStore.ts (1)
src/stores/viewStore.ts (1)
VIEW_STORE_DEFAULT(271-271)
src/stores/notificationStore.ts (1)
src/typings/common.ts (1)
WithId(12-12)
🔇 Additional comments (25)
src/stores/logExportStore.ts (2)
10-10: Import change looks good!The replacement of the context store import with the new Zustand-based notification store aligns perfectly with the PR's objective.
53-53: State access properly updated!The change correctly retrieves the postPopUp function from the new Zustand notification store while maintaining the same error handling logic.
src/stores/queryStore/createQueryControllerSlice.ts (2)
6-6: Import change looks good!The replacement of the context store import with the new Zustand-based notification store aligns with the PR's objective of improving state management.
57-57: State access properly updated!The change correctly retrieves the postPopUp function from the new Zustand notification store while maintaining the same error handling logic.
src/App.tsx (1)
13-17: Component tree simplified!The removal of the NotificationContextProvider wrapper successfully simplifies the component tree while maintaining functionality. This change is a key part of the refactoring effort and aligns with the PR objective of eliminating the need for a context provider wrapper component.
src/components/PopUps/PopUpMessageBox.tsx (2)
18-18: Import update looks good!The import change correctly brings in both the useNotificationStore hook and the PopUpMessage type from the new Zustand store.
42-43: State access properly updated!The change correctly replaces context consumption with a direct subscription to the Zustand store. The selector pattern used here is efficient and will prevent unnecessary re-renders.
src/stores/viewStore.ts (4)
26-26: LGTM: Added import for the new notification store.The addition of the import for useNotificationStore aligns with the PR objective of replacing context-based notifications with Zustand store.
142-142: LGTM: Migrated error notification handling to useNotificationStore.This change correctly replaces the context-based approach with the new Zustand store implementation, maintaining the same error notification behavior.
196-196: LGTM: Consistent implementation of notification store usage.The change maintains consistency with other error handlers in the file by using the same pattern to access the notification store.
246-246: LGTM: Successfully migrated the last error handler to useNotificationStore.This completes the migration of all error handlers in this file from context to Zustand store.
src/components/CentralContainer/Sidebar/SidebarTabs/SettingsTabPanel/index.tsx (3)
1-1: LGTM: Simplified import by removing useContext.The change correctly removes the no-longer-needed useContext import, which aligns with the PR objective.
15-15: LGTM: Added import for useNotificationStore.This change properly adds the import for the new notification store that will replace the context-based approach.
138-138: LGTM: Successfully migrated to Zustand-based notification store.The component now correctly uses the Zustand store selector pattern to access the postPopUp function, which simplifies the code and improves maintainability.
src/components/AppController.tsx (2)
16-16: LGTM: Added import for notification store.This change correctly imports the new notification store to replace the context-based approach.
90-90: LGTM: Switched to store-based notifications.The component now properly uses the Zustand store selector pattern to access the postPopUp function, simplifying state management.
src/components/PopUps/index.tsx (2)
6-6: LGTM: Added import for notification store.This change correctly imports the useNotificationStore hook that will replace the context-based notification system.
18-18: LGTM: Migrated to store-based message retrieval.The component now correctly uses the Zustand store selector pattern to access the popUpMessages array, maintaining the same functionality while simplifying the state management approach.
src/stores/logFileStore.ts (4)
27-27: LGTM! Good replacement of context with Zustand store.The import change from context to Zustand store aligns with the PR objective to improve state management.
30-30: LGTM! Necessary import for error handling.Adding VIEW_STORE_DEFAULT import enables proper reset of log data on error.
125-125: LGTM! Updated notification source.Successfully migrated from context-based notification to the new Zustand store implementation.
160-161: LGTM! Improved error handling.Enhanced error recovery by explicitly resetting file name and log data to default values. This ensures the application state is clean after an error occurs.
src/stores/notificationStore.ts (3)
1-51: LGTM! Well-implemented Zustand store replacing context.This implementation successfully replaces the React Context notification system with a clean, well-typed Zustand store. The store structure clearly separates state and actions while maintaining the same functionality.
7-20: LGTM! Clear type definitions and default state.The TypeScript interfaces provide excellent type safety for the store. The use of WithId type to add IDs to messages is a good pattern for maintaining unique identifiers.
27-31: LGTM! Good immutable state update patterns.Both actions follow immutable update patterns, creating new state objects rather than mutating existing ones. This is the recommended approach for Zustand stores and helps prevent unexpected side effects.
Also applies to: 41-47
| popUpMessages: [], | ||
| }; | ||
|
|
||
| let nextPopUpMessageId = 0; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider persisting the ID counter.
The simple counter approach works well for generating unique IDs, but it resets on page reload. While likely acceptable for this use case, you might consider persisting it if maintaining unique IDs across sessions becomes important.
Also applies to: 34-40
🤖 Prompt for AI Agents
In src/stores/notificationStore.ts at lines 22 and 34-40, the nextPopUpMessageId
counter resets on page reload, losing uniqueness across sessions. To fix this,
persist the counter value in localStorage or another persistent storage,
initializing nextPopUpMessageId from there on load and updating the stored value
whenever the counter increments. This ensures IDs remain unique even after page
reloads.
There was a problem hiding this comment.
The IDs are not expected to be persisted across page reloads.
There was a problem hiding this comment.
@junhaoliao Got it, thanks for clarifying. That makes sense for this use case since notifications are typically transient UI elements that would be cleared on reload anyway.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/stores/logFileStore.ts (1)
151-160: Consider resetting fileName and logData on errorWhen a file loading error occurs, the UI state is reset to
UNOPENED, but the filename and log data aren't reset to their default values, which could lead to inconsistent UI state.I see this issue was already identified in a previous review and is being tracked separately in issue #296. This approach of separating concerns helps maintain the focus of the current PR on the notification system refactoring.
📜 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
🪛 GitHub Check: lint-check
src/stores/logFileStore.ts
[failure] 30-30:
'VIEW_STORE_DEFAULT' is defined but never used
🪛 GitHub Actions: lint
src/stores/logFileStore.ts
[error] 30-30: ESLint: 'VIEW_STORE_DEFAULT' is defined but never used. (@typescript-eslint/no-unused-vars)
🔇 Additional comments (1)
src/stores/logFileStore.ts (1)
125-125: LGTM! Properly migrated from context to storeThe notification system has been successfully migrated from React Context to Zustand store, maintaining the same functionality while making the state management more consistent.
| import useQueryStore from "./queryStore"; | ||
| import useUiStore from "./uiStore"; | ||
| import useViewStore from "./viewStore"; | ||
| import useViewStore, {VIEW_STORE_DEFAULT} from "./viewStore"; |
There was a problem hiding this comment.
Remove unused import
The VIEW_STORE_DEFAULT import is defined but never used in this file, which is causing linting errors in the CI pipeline.
-import useViewStore, {VIEW_STORE_DEFAULT} from "./viewStore";
+import useViewStore from "./viewStore";📝 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.
| import useViewStore, {VIEW_STORE_DEFAULT} from "./viewStore"; | |
| -import useViewStore, {VIEW_STORE_DEFAULT} from "./viewStore"; | |
| +import useViewStore from "./viewStore"; |
🧰 Tools
🪛 GitHub Check: lint-check
[failure] 30-30:
'VIEW_STORE_DEFAULT' is defined but never used
🪛 GitHub Actions: lint
[error] 30-30: ESLint: 'VIEW_STORE_DEFAULT' is defined but never used. (@typescript-eslint/no-unused-vars)
🤖 Prompt for AI Agents
In src/stores/logFileStore.ts at line 30, the import VIEW_STORE_DEFAULT is
unused and causing lint errors. Remove VIEW_STORE_DEFAULT from the import
statement to fix the issue.
|
This was created from a wrong branch and was replaced by #297 |
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