refactor: Optimize React hook dependencies by using getState() for immutable Zustand store actions (fixes #299); Remove unused setters from logFileStore.
#334
Conversation
## Walkthrough
This change refactors multiple components to access Zustand store actions using `useStore.getState()` directly at invocation time, rather than subscribing to actions via hooks. Several setter methods are removed from the store interface, and store updates now use direct partial state updates. Memoization with `useCallback` is applied to relevant handlers.
## Changes
| File(s) | Change Summary |
|---------------------------------------------------------------------------------------------|------------------------------------------------------------------------------------------------------------------|
| src/components/CentralContainer/Sidebar/SidebarTabs/index.tsx, src/components/CentralContainer/Sidebar/index.tsx | Refactored to access store setters via `useUiStore.getState()` within callbacks; memoized handlers with `useCallback`. |
| src/components/DropFileContainer/index.tsx, src/components/MenuBar/index.tsx | Store actions accessed via `useLogFileStore.getState()`; handlers wrapped with `useCallback` for memoization. |
| src/components/MenuBar/ExportLogsButton.tsx | Store action accessed via `useLogExportStore.getState()` in a memoized callback. |
| src/components/PopUps/PopUpMessageBox.tsx | Store action accessed via `useNotificationStore.getState()` in memoized and effect callbacks. |
| src/stores/logFileStore.ts | Removed explicit setter methods from interface and implementation; store updates use direct partial state updates.|
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant UIComponent
participant ZustandStore
UIComponent->>ZustandStore: getState().action()
Note right of ZustandStore: Action is invoked directly from the current state
ZustandStore-->>UIComponent: Updates state (no subscription for action)Assessment against linked issues
Possibly related PRs
Suggested reviewers
Learnt from: zzxthehappiest Learnt from: junhaoliao Learnt from: junhaoliao Learnt from: junhaoliao Learnt from: junhaoliao Learnt from: Henry8192 Learnt from: junhaoliao Learnt from: junhaoliao Learnt from: junhaoliao Learnt from: junhaoliao Learnt from: junhaoliao Learnt from: junhaoliao Learnt from: zzxthehappiest Learnt from: junhaoliao Learnt from: junhaoliao Learnt from: junhaoliao Learnt from: davemarco Learnt from: davemarco Learnt from: junhaoliao Learnt from: junhaoliao Learnt from: davemarco Learnt from: davemarco |
![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 (8)
src/components/CentralContainer/Sidebar/SidebarTabs/index.tsx(3 hunks)src/components/CentralContainer/Sidebar/index.tsx(1 hunks)src/components/DropFileContainer/index.tsx(4 hunks)src/components/MenuBar/ExportLogsButton.tsx(1 hunks)src/components/MenuBar/index.tsx(2 hunks)src/components/PopUps/PopUpMessageBox.tsx(3 hunks)src/stores/logFileStore.ts(1 hunks)src/stores/viewStore/index.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>.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
src/components/CentralContainer/Sidebar/index.tsxsrc/components/MenuBar/index.tsxsrc/components/CentralContainer/Sidebar/SidebarTabs/index.tsxsrc/components/DropFileContainer/index.tsxsrc/components/MenuBar/ExportLogsButton.tsxsrc/components/PopUps/PopUpMessageBox.tsxsrc/stores/viewStore/index.tssrc/stores/logFileStore.ts
🧠 Learnings (1)
src/components/CentralContainer/Sidebar/index.tsx (1)
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#94
File: new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx:99-118
Timestamp: 2024-10-19T03:33:29.578Z
Learning: In `new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx`, when using `useEffect` to register window resize event handlers, it's acceptable to have an empty dependency array because the variables and functions used within the effect (`getPanelWidth`, `PANEL_CLIP_THRESHOLD_IN_PIXELS`, and `tabListRef`) are constants or refs declared outside the functional component and do not change.
🔇 Additional comments (17)
src/stores/logFileStore.ts (2)
101-101: LGTM: Store creation signature simplified appropriately.Removing the unused
getparameter from the store creation function is a good cleanup that aligns with the refactoring to use directset()calls instead of individual setter methods.
107-109: LGTM: Direct state updates improve performance.The refactor to use direct
set()calls with partial state updates instead of individual setter methods is more efficient and follows zustand best practices. This eliminates the overhead of maintaining separate setter functions.src/components/CentralContainer/Sidebar/index.tsx (1)
52-55: LGTM: Imperative store access prevents stale closures.The refactor to access
setActiveTabNamedirectly fromuseUiStore.getState()inside the callback is correct. This ensures the latest store method is always used and eliminates the need to include it in the dependency array, making the callback more stable.src/components/MenuBar/ExportLogsButton.tsx (1)
39-39: LGTM: Direct store method access optimizes performance.Accessing
exportLogsdirectly viauseLogExportStore.getState().exportLogsin the onClick handler is correct. This avoids reactive subscriptions to the method and ensures the latest store method is used when the button is clicked.src/components/MenuBar/index.tsx (1)
35-40: LGTM: Memoization with imperative store access is well-implemented.The combination of
useCallbackwith direct store access viauseLogFileStore.getState().loadFileis excellent. The empty dependency array ensures the callback remains stable while the imperative access guarantees the latest store method is used when opening files.src/components/CentralContainer/Sidebar/SidebarTabs/index.tsx (1)
54-67: LGTM: Correct memoization with appropriate dependencies.The
useCallbackimplementation with[activeTabName]dependency is correct since the callback uses this value in its toggle logic. AccessingsetActiveTabNamedirectly fromuseUiStore.getState()ensures the latest store method is used while maintaining proper memoization.src/components/DropFileContainer/index.tsx (3)
1-4: LGTM: Import addition is appropriate.Adding
useCallbackto the React imports aligns with the memoization pattern applied to the event handlers below.
31-52: LGTM: Proper memoization with appropriate dependencies.The
handleDragcallback is correctly memoized with an empty dependency array since it only usessetIsFileHovering(which is stable fromuseState) and event properties.
54-71: Excellent refactoring: Direct store access improves performance.The refactoring from hook-based subscription to direct
getState()access is well-implemented:
useCallbackcorrectly includesdisabledas a dependencyloadFileis accessed only when needed, reducing unnecessary re-renders- The pattern aligns with the PR's objective of optimizing zustand action calls
src/components/PopUps/PopUpMessageBox.tsx (3)
2-2: LGTM: Import addition supports callback memoization.Adding
useCallbackto support the memoization pattern applied below.
46-49: Excellent optimization: Direct store access in memoized callback.The refactoring properly:
- Uses
useCallbackwith correctiddependency- Accesses
handlePopUpMessageClosedirectly viagetState()when needed- Eliminates unnecessary component re-renders from store subscriptions
69-69: Consistent pattern: Direct store access in effect.Good consistency applying the same direct access pattern within the
useEffect, maintaining the optimization benefits throughout the component.src/stores/viewStore/index.ts (5)
2-5: LGTM: Appropriate imports for persistence functionality.The imports correctly bring in the necessary zustand persistence middleware components.
7-7: LGTM: Error handling import for async operations.Importing error handling for the async rehydration operation is a good practice.
12-16: LGTM: Well-defined types for URL persistence.The type definitions properly constrain the persisted state to the relevant URL hash parameters.
Also applies to: 19-19
42-59: LGTM: Proper store configuration with persistence.The store configuration correctly:
- Uses the persist middleware wrapper
- Properly partializes state to only persist relevant fields
- Applies the custom hash storage adapter
- Maintains the original store structure
22-40: Verify URL manipulation security and consider edge cases.The custom storage implementation looks functional but has potential concerns:
- URL Length Limits: Large
logEventNumvalues could exceed URL length limits- Invalid Hash Handling: Consider what happens with malformed hash parameters
- Race Conditions: Multiple rapid hash changes might cause issues
#!/bin/bash # Check if there are any URL validation utilities or constants in the codebase rg -A 5 -B 5 "URL.*valid|hash.*valid|URLSearchParams.*valid" # Look for existing URL parameter handling patterns rg -A 10 "URLSearchParams|location\.hash" # Check for any URL length or validation constants rg -A 5 "URL.*LENGTH|MAX.*URL|hash.*length"
src/stores/viewStore/index.ts
Outdated
| window.addEventListener("hashchange", () => { | ||
| (async () => { | ||
| await useViewStore.persist.rehydrate(); | ||
| })().catch(handleErrorWithNotification); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider cleanup and error boundary for event listener.
The hashchange listener implementation is functional but could be improved:
- Memory Leak Prevention: The event listener is never removed
- Component Lifecycle: Consider if this should be tied to component mounting/unmounting
Consider wrapping this in a cleanup mechanism:
+let isListenerAttached = false;
+
+export const attachHashListener = () => {
+ if (isListenerAttached) return;
+
+ const handleHashChange = () => {
+ (async () => {
+ await useViewStore.persist.rehydrate();
+ })().catch(handleErrorWithNotification);
+ };
+
+ window.addEventListener("hashchange", handleHashChange);
+ isListenerAttached = true;
+
+ return () => {
+ window.removeEventListener("hashchange", handleHashChange);
+ isListenerAttached = false;
+ };
+};
-window.addEventListener("hashchange", () => {
- (async () => {
- await useViewStore.persist.rehydrate();
- })().catch(handleErrorWithNotification);
-});🤖 Prompt for AI Agents
In src/stores/viewStore/index.ts around lines 61 to 65, the hashchange event
listener is added but never removed, risking memory leaks. Refactor the code to
add the event listener when the component or module initializes and remove it
during cleanup or unmounting. Implement a cleanup function that removes the
event listener to prevent memory leaks and ensure proper lifecycle management.
junhaoliao
left a comment
junhaoliao
left a comment
There was a problem hiding this comment.
for the PR title, how about:
Optimize React hook dependencies by using getState() for immutable Zustand store actions (fixes #299).
Henry8192
changed the title
useState for calling zustand actions (fixes #299).useState for calling zustand actions (fixes #299); Remove unused setters from logFileStore.
Co-authored-by: Junhao Liao <[email protected]>
Henry8192
changed the title
useState for calling zustand actions (fixes #299); Remove unused setters from logFileStore.
junhaoliao
changed the title
getState() for immutable Zustand store actions (fixes #299); Remove unused setters from logFileStore.
2 participants
Description
Fixes #299.
Checklist
breaking change.
(Zustand docs will be added in docs: Add Zustand development conventions to coding guidelines (resolves #289). #332 )
Validation performed
Summary by CodeRabbit
Refactor
Chores