refactor: Move logEventNum update handling from AppController to viewStore; Split viewStore into slices.#316
Merged
Henry8192 merged 14 commits intoy-scope:mainfrom Jun 23, 2025
Merged
Conversation
# Conflicts: # src/components/AppController.tsx
… now be reflected in the editor
## Walkthrough
This change refactors view state management by introducing a modular Zustand store architecture with distinct slices for events, pagination, filtering, and formatting. It removes URL and page synchronization logic from `AppController`, delegating log event navigation and related side effects to the new store. Associated components are updated to use the new store API.
## Changes
| File(s) | Change Summary |
|------------------------------------------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| src/components/AppController.tsx | Removed all logic syncing `logEventNum` state with URL and page loading; simplified URL param update functions; removed `logEventNum` state subscription; now only initializes state from URL/hash on load and hash changes. |
| src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/Result.tsx<br>src/components/Editor/index.tsx | Updated to use `updateLogEventNum` (from Zustand store) instead of `setLogEventNum` for log event navigation. |
| src/stores/viewStore/index.ts | Introduced and exported a new Zustand store `useViewStore`, combining four view state slices: event, page, filter, and formatting. |
| src/stores/viewStore/types.ts | Added TypeScript interfaces/types for view state slices: pagination, event, formatting, filtering, and the combined `ViewState`. |
| src/stores/viewStore/createViewEventSlice.ts | New Zustand slice: manages `logEventNum` state, updates URL, loads new pages if needed, and handles errors. Exports `createViewEventSlice` and default state. |
| src/stores/viewStore/createViewPageSlice.ts | New Zustand slice: manages pagination state, provides actions for page navigation/loading, updates URL and global state, and handles errors. |
| src/stores/viewStore/createViewFilterSlice.ts | New Zustand slice: provides `filterLogs` method for log level filtering, updates UI state, fetches filtered data, and handles errors. |
| src/stores/viewStore/createViewFormattingSlice.ts | New Zustand slice: manages prettification state, updates UI and page data on change, and handles errors. |
| src/stores/logFileStore.ts | Removed explicit setting of log data to `"Loading..."` during file load; replaced with resetting structured page data. No other logic changed. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant UI_Component as UI Component (e.g., Editor, Search Result)
participant ViewStore as useViewStore (Zustand)
participant LogFileManager as logFileManagerProxy
participant URL as URL Hash
UI_Component->>ViewStore: updateLogEventNum(newLogEventNum)
alt event is on current page
ViewStore->>URL: Update hash param with new logEventNum
else event not on current page
ViewStore->>UI_Component: Set UI to FAST_LOADING
ViewStore->>LogFileManager: loadPage({eventNum: newLogEventNum})
LogFileManager-->>ViewStore: Return page data
ViewStore->>ViewStore: updatePageData(pageData)
ViewStore->>URL: Update hash param with new logEventNum
endPossibly related PRs
Suggested reviewers
|
junhaoliao
requested changes
Jun 14, 2025
Member
junhaoliao
left a comment
junhaoliao
left a comment
There was a problem hiding this comment.
for the PR title, how about:
refactor: Move logEventNum update handling from AppController to viewStore.
Henry8192
changed the title
updateLogEventNum
junhaoliao
reviewed
Jun 16, 2025
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (1)
src/stores/viewStore/createViewUtilitySlice.ts (1)
113-129: Avoid hard-crashing on “reload” pre-conditionsThrowing an Error here will bubble up past the store call-site and blank the React tree. Prefer returning early with a warning, or surface the issue via the notification store for a controlled UX.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
src/components/AppController.tsx(2 hunks)src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/Result.tsx(1 hunks)src/components/Editor/index.tsx(1 hunks)src/stores/viewStore/createViewUpdateSlice.ts(1 hunks)src/stores/viewStore/createViewUtilitySlice.ts(4 hunks)src/stores/viewStore/index.ts(1 hunks)src/stores/viewStore/types.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/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/Result.tsxsrc/stores/viewStore/index.tssrc/components/Editor/index.tsxsrc/stores/viewStore/types.tssrc/components/AppController.tsxsrc/stores/viewStore/createViewUtilitySlice.tssrc/stores/viewStore/createViewUpdateSlice.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: zzxthehappiest
PR: y-scope/yscope-log-viewer#286
File: src/components/Editor/index.tsx:138-139
Timestamp: 2025-06-01T13:40:12.222Z
Learning: In the yscope-log-viewer codebase, when using Zustand stores in React components, the preferred pattern is to use `getState()` for static setters that never change (like `setLogEventNum`) to avoid unnecessary subscriptions, while using hooks for actions that do more than just setting values. All store state variables should be declared at the beginning of the component for consistency and clear dependency overview.
🔇 Additional comments (2)
src/components/AppController.tsx (2)
26-33: Potential hash-change feedback loop
updateLogEventNumtriggers its ownupdateWindowUrlHashParams. When invoked from ahashchangelistener this could rewrite the hash again, causing an extra (though usually identical)hashchangeevent on some browsers. Consider short-circuiting inupdateLogEventNumwhen the incoming value already matches the hash.
70-73: Double processing of view parameters
handleHashChangecallsupdateViewHashParams()which in turn callsupdateLogEventNum, leading again to the feedback loop risk noted above. Confirm no infinite recursion occurs when the store writes back an unchanged hash.
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/Result.tsx
Show resolved
Hide resolved
Comment on lines
264
to
267
| updateWindowUrlHashParams({logEventNum: newLogEventNum}); | ||
| const {setLogEventNum} = useViewStore.getState(); | ||
| setLogEventNum(newLogEventNum); | ||
| const {updateLogEventNum} = useViewStore.getState(); | ||
| updateLogEventNum(newLogEventNum); | ||
| }, []); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same redundancy as in Result.tsx
handleCursorExplicitPosChange updates the hash, then hands the same value to updateLogEventNum, which updates it again. This can bloat the browser history and cause an extra render.
Apply the same one-liner fix as suggested for Result.tsx.
🤖 Prompt for AI Agents
In src/components/Editor/index.tsx around lines 264 to 267, the function updates
the URL hash and then redundantly updates the same value in the state, causing
extra renders and bloating browser history. Refactor the code to update the
state first and then update the URL hash in a single step or combine the updates
into one call to avoid duplication and unnecessary renders, following the
one-liner fix applied in Result.tsx.
Comment on lines
163
to
166
| const newLogEventNum = pageData.logEventNum; | ||
| updateWindowUrlHashParams({logEventNum: newLogEventNum}); | ||
| const {updateLogEventNum} = get(); | ||
| updateLogEventNum(newLogEventNum); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Drop the second updateLogEventNum call
updatePageData already updates the URL hash; calling updateLogEventNum immediately afterwards sets the same state again and triggers another hash update, generating an extra render and history entry.
🤖 Prompt for AI Agents
In src/stores/viewStore/createViewUpdateSlice.ts around lines 163 to 166, remove
the second call to updateLogEventNum(newLogEventNum) because updatePageData
already updates the URL hash and state. This redundant call causes an extra
render and history entry. Keep only the updateWindowUrlHashParams call and the
initial assignment to newLogEventNum.
Comment on lines
131
to
134
| const clampedLogEventNum = clamp(newLogEventNum, 1, numEvents); | ||
| const {beginLineNumToLogEventNum} = get(); | ||
| const logEventNumsOnPage: number [] = Array.from(beginLineNumToLogEventNum.values()); | ||
| set({logEventNum: clampedLogEventNum}); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure logEventNumsOnPage is ordered before bounds checks
Array.from(beginLineNumToLogEventNum.values()) relies on insertion order. If the map is ever populated out of order, isWithinBounds and nearest-element lookup will misbehave.
-const logEventNumsOnPage: number [] = Array.from(beginLineNumToLogEventNum.values());
+const logEventNumsOnPage = [...beginLineNumToLogEventNum.values()].sort((a, b) => a - b);📝 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.
Suggested change
| const clampedLogEventNum = clamp(newLogEventNum, 1, numEvents); | |
| const {beginLineNumToLogEventNum} = get(); | |
| const logEventNumsOnPage: number [] = Array.from(beginLineNumToLogEventNum.values()); | |
| set({logEventNum: clampedLogEventNum}); | |
| const clampedLogEventNum = clamp(newLogEventNum, 1, numEvents); | |
| const {beginLineNumToLogEventNum} = get(); | |
| - const logEventNumsOnPage: number [] = Array.from(beginLineNumToLogEventNum.values()); | |
| + const logEventNumsOnPage = [...beginLineNumToLogEventNum.values()].sort((a, b) => a - b); | |
| set({logEventNum: clampedLogEventNum}); |
🤖 Prompt for AI Agents
In src/stores/viewStore/createViewUpdateSlice.ts around lines 131 to 134, the
array logEventNumsOnPage is created from a Map's values without sorting, which
can cause incorrect behavior if the Map is populated out of order. To fix this,
sort the logEventNumsOnPage array numerically after creating it from the Map
values to ensure it is always ordered before any bounds checks or
nearest-element lookups.
junhaoliao
requested changes
Jun 18, 2025
Member
There was a problem hiding this comment.
-
For the PR title, how about:
refactor: Move logEventNum update handling from AppController to viewStore; Split viewStore into slices. -
setNumPages/setPageNum/setBeginLineNumToLogEventNumare never used, right? Can we remove them? -
Following (2.), I believe
setLogDatashould be removed as well, sincelogDatacan be set byupdatePageData. We do usesetLogDatainloadFileto initializelogData, but I think it's better we also initialize other states, so it happens that we can just useupdatePageData. -
I think we discussed that we want to create slices based on features in the very initial Zustand refactoring PR. can we slice the store this way instead:
createViewPageSliceState:
- pageNum
- numPages
- logData
- beginLineNumToLogEventNum
Actions:
- loadPageByAction
- updatePageData
createViewEventSlice
State:
- logEventNum
Actions:
- updateLogEventNum
createViewFormattingSlice
State:
- isPrettified
Actions:
- updateIsPrettified
createViewFilterSlice
State:
- (none)
Actions:
- filterLogs
junhaoliao
requested changes
Jun 18, 2025
Henry8192
changed the title
Co-authored-by: Junhao Liao <junhao@junhao.ca>
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
src/stores/logFileStore.ts(0 hunks)src/stores/viewStore/createViewEventSlice.ts(1 hunks)src/stores/viewStore/createViewFilterSlice.ts(1 hunks)src/stores/viewStore/createViewFormattingSlice.ts(1 hunks)src/stores/viewStore/createViewPageSlice.ts(1 hunks)src/stores/viewStore/index.ts(1 hunks)src/stores/viewStore/types.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/stores/logFileStore.ts
🧰 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/viewStore/index.tssrc/stores/viewStore/createViewFormattingSlice.tssrc/stores/viewStore/types.tssrc/stores/viewStore/createViewPageSlice.tssrc/stores/viewStore/createViewFilterSlice.tssrc/stores/viewStore/createViewEventSlice.ts
🔇 Additional comments (11)
src/stores/viewStore/index.ts (1)
10-15: LGTM: Clean store composition patternThe store composition using Zustand slices follows a clear and maintainable pattern. The slice combination is straightforward and the type system ensures compile-time safety.
src/stores/viewStore/createViewFormattingSlice.ts (1)
34-38: LGTM: Efficient early return optimizationGood optimization to avoid unnecessary work when the prettification state hasn't actually changed.
src/stores/viewStore/createViewFilterSlice.ts (1)
26-49: LGTM: Well-structured async filtering operationThe filtering logic properly coordinates UI state, async operations, and error handling. The sequence of setting loading state, filtering logs, updating page data, and restarting queries is correct.
src/stores/viewStore/types.ts (2)
1-6: LGTM: Clean import structure without file extensionsThe imports correctly omit TypeScript file extensions, maintaining tool-agnostic compatibility. The past review concern about .ts extensions appears to have been addressed.
27-44: Method naming is now more consistentThe action methods now consistently use descriptive prefixes that better reflect their behaviour:
updateLogEventNum,updateIsPrettified,updatePageDatafor state changes with side effects, andloadPageByAction,filterLogsfor specific operations. This addresses the past review concern about naming consistency.src/stores/viewStore/createViewPageSlice.ts (3)
37-75: LGTM: Robust navigation cursor logicThe
getPageNumCursorfunction properly handles all navigation actions with appropriate page clamping and event positioning. The switch statement covers all cases and returns null for unknown actions, providing good error handling.
110-126: LGTM: Comprehensive RELOAD validation logicThe RELOAD action handling includes proper validation of file source and log event number states, with descriptive error messages for debugging.
128-134: Good UI state validation prevents concurrent operationsProper check for UI_STATE.READY prevents navigation during ongoing operations, with helpful console warning for debugging.
src/stores/viewStore/createViewEventSlice.ts (3)
37-37: LGTM! Coding guideline compliance.Good adherence to the coding guideline by using
false === isWithinBounds(...)instead of!isWithinBounds(...).
80-80: LGTM! Coding guideline compliance.Good adherence to the coding guideline by using
0 === numEventsinstead of!numEvents.
52-54: Excellent type safety documentation.The detailed comments explaining why the type casting is safe are exemplary. This makes the code much more maintainable and demonstrates thorough understanding of the logic constraints.
Comment on lines
+54
to
+61
| (async () => { | ||
| const {logFileManagerProxy} = useLogFileManagerStore.getState(); | ||
| const pageData = await logFileManagerProxy.loadPage(cursor, newIsPrettified); | ||
|
|
||
| const {updatePageData} = get(); | ||
| updatePageData(pageData); | ||
| })().catch(handleErrorWithNotification); | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding race condition protection
The async operation could potentially race with other state updates or be called multiple times rapidly. Consider adding a loading flag or debouncing mechanism to prevent concurrent executions.
updateIsPrettified: (newIsPrettified: boolean) => {
const {isPrettified} = get();
if (newIsPrettified === isPrettified) {
return;
}
+
+ // Prevent concurrent updates
+ const currentState = get();
+ if (currentState.isUpdatingPrettification) {
+ return;
+ }
+ set({isUpdatingPrettification: true});
const {setUiState} = useUiStore.getState();
setUiState(UI_STATE.FAST_LOADING);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/stores/viewStore/createViewFormattingSlice.ts around lines 54 to 61, the
async function that loads page data can be triggered multiple times rapidly,
causing race conditions. To fix this, add a loading flag in the state to track
if the operation is in progress and prevent re-entry until it completes, or
implement a debounce mechanism to limit how often the function can be called.
Update the state accordingly before and after the async call to ensure only one
execution runs at a time.
Comment on lines
+29
to
+48
| (async () => { | ||
| const {logFileManagerProxy} = useLogFileManagerStore.getState(); | ||
| const {isPrettified, logEventNum} = get(); | ||
| const pageData = await logFileManagerProxy.setFilter( | ||
| { | ||
| code: CURSOR_CODE.EVENT_NUM, | ||
| args: { | ||
| eventNum: logEventNum, | ||
| }, | ||
| }, | ||
| isPrettified, | ||
| filter | ||
| ); | ||
|
|
||
| const {updatePageData} = get(); | ||
| updatePageData(pageData); | ||
|
|
||
| const {startQuery} = useQueryStore.getState(); | ||
| startQuery(); | ||
| })().catch(handleErrorWithNotification); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding concurrent operation protection
Similar to the formatting slice, this async operation could benefit from protection against concurrent executions to prevent race conditions.
🤖 Prompt for AI Agents
In src/stores/viewStore/createViewFilterSlice.ts around lines 29 to 48, the
async IIFE performing filter setting and page data updating lacks protection
against concurrent executions, risking race conditions. Introduce a concurrency
control mechanism such as a flag or a mutex to ensure only one instance of this
async operation runs at a time, preventing overlapping executions and
inconsistent state updates.
Contributor
Author
Do you think |
Co-authored-by: Junhao Liao <junhao@junhao.ca>
Member
ah, |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/stores/viewStore/index.ts (1)
10-15: Guard against key collisions when merging slicesSpreading multiple slice objects means duplicate keys are silently overwritten. Consider asserting uniqueness during dev builds (e.g., compare
Object.keys) or giving each slice its own namespace.src/stores/viewStore/createViewEventSlice.ts (2)
84-91: Consider performance optimization for frequent calls.Creating a new array from
beginLineNumToLogEventNum.values()on every call could be inefficient ifupdateLogEventNumis called frequently during user interactions. Consider caching or memoizing the array creation, or modifyupdateUrlIfEventOnPageto accept the Map directly.
94-107: Ensure UI state is reset on page loading failure.The async page loading logic is well-structured, but if page loading fails, the UI state might remain in
FAST_LOADINGstate sincehandleErrorWithNotificationonly handles error notification. Consider wrapping the async logic in try-catch-finally to properly reset the UI state in all cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/stores/viewStore/createViewEventSlice.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>.
src/stores/viewStore/index.tssrc/stores/viewStore/createViewEventSlice.ts
🔇 Additional comments (1)
src/stores/viewStore/createViewEventSlice.ts (1)
33-61: LGTM! Proper use of boolean comparison and comprehensive bounds checking.The helper function correctly uses
false === isWithinBounds()following the coding guidelines and handles edge cases well with detailed comments explaining the safety of type casting.
junhaoliao
requested changes
Jun 23, 2025
Member
junhaoliao
left a comment
junhaoliao
left a comment
There was a problem hiding this comment.
sorry for missing a few things in the last review
junhaoliao
approved these changes
Jun 23, 2025
3 tasks
This was referenced Aug 12, 2025
Merged
3 tasks
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Instead of subscribing
logEventNumchanges in AppControlleruseEffect,setLogEventNumis changed toupdateLogEventNum, which handleslogEventNumchange and triggers a page load if needed.viewStoreis also refactored using zustand slices.Checklist
breaking change.
Validation performed
Summary by CodeRabbit
New Features
Improvements
Refactor