refactor: Reorganize Zustand state retrievals to prevent potential circular dependencies (resolves #264); Add error logging before postPopUp().
#265
Conversation
|
""" WalkthroughThe changes reorganize the order and locality of Zustand global state retrievals ( Changes
Sequence Diagram(s)sequenceDiagram
participant Component
participant StoreA
participant StoreB
Note over Component: Example of reorganized state retrieval
Component->>StoreA: getState() or hook selector (just before use)
Component->>StoreB: getState() or hook selector (just before use)
Component->>Component: Use retrieved state
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm warn config production Use Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
🧠 Learnings (1)📓 Common learnings🧬 Code Graph Analysis (1)src/components/AppController.tsx (2)
🔇 Additional comments (6)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
junhaoliao
changed the title
junhaoliao
changed the title
![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 (5)
src/contexts/AppController.tsx(3 hunks)src/stores/logExportStore.ts(1 hunks)src/stores/logFileStore.ts(2 hunks)src/stores/queryStore/createQueryControllerSlice.ts(1 hunks)src/stores/viewStore.ts(6 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/queryStore/createQueryControllerSlice.tssrc/stores/logExportStore.tssrc/stores/viewStore.tssrc/stores/logFileStore.tssrc/contexts/AppController.tsx
🧬 Code Graph Analysis (3)
src/stores/logExportStore.ts (2)
src/services/LogFileManager/index.ts (3)
numEvents(124-126)fileName(116-118)fileName(200-221)src/utils/config.ts (1)
EXPORT_LOGS_CHUNK_SIZE(181-181)
src/stores/logFileStore.ts (2)
src/services/LogFileManager/index.ts (2)
create(164-189)cursor(492-514)src/typings/worker.ts (2)
FileSrcType(108-108)CursorType(107-107)
src/contexts/AppController.tsx (1)
src/services/LogFileManagerProxy.ts (1)
loadFile(17-42)
🔇 Additional comments (16)
src/stores/queryStore/createQueryControllerSlice.ts (1)
54-55: Good adjustment of state retrieval timingMoving the state retrieval after the early return check is a good optimization. This ensures that
postPopUpandlogFileManagerProxyare only accessed when they're actually needed, avoiding unnecessary state retrieval when the function exits early.src/stores/logExportStore.ts (2)
39-40: Improved order of state retrievalGetting
fileNameandnumEventsfirst is logical since they're needed immediately for creating the LogExportManager. This reorganization improves code locality by grouping related operations together.
47-48: Appropriate state retrieval sequencingDeferring retrieval of
postPopUpandlogFileManagerProxyuntil closer to where they're used in the async function is a good practice. This supports the PR's goal of preventing potential circular dependencies by localizing state access.src/stores/logFileStore.ts (3)
100-101: Valid eslint commentThe eslint-disable comment is appropriate since you're refactoring the code organization without changing the overall function structure. Future work might consider breaking this function into smaller pieces to avoid needing the eslint exception.
108-108: Good separation of state retrievalExtracting only
setLogDataat this point and deferring other state until needed is appropriate since it's used earlier in the function and aligns with the PR's goal of reducing potential circular dependencies.
123-125: Effective reorganization of state accessMoving these state retrievals closer to their usage in the async function follows good code organization principles and helps prevent potential circular dependencies by localizing state access to where it's needed.
src/contexts/AppController.tsx (4)
92-93: Improved React state access patternUsing the selector pattern with
useViewStore((state) => state.updatePageData)is better than callinggetState()inside effects. This follows React best practices by properly subscribing to state changes and including them in dependency arrays.
94-95: Consistent state access patternUsing the selector pattern for
logFileManagerProxymaintains consistency with other state access in the component and follows recommended patterns for Zustand stores in React components.
149-149: Simplified state usageUsing the directly retrieved
updatePageDatafunction is cleaner than calling throughgetState()and avoids potential issues with stale state references in asynchronous contexts.
164-166: Properly updated effect dependenciesAdding
updatePageDatato the dependency array is correct since it's used within the effect. This ensures the effect runs when this dependency changes, following React's rules of hooks.src/stores/viewStore.ts (6)
115-117: Localise state selection in filterLogs
DeclaringupdatePageData,logEventNum,postPopUp, andlogFileManagerProxyimmediately before they're used enhances readability and ensures the values are fresh. This also minimises the surface area for potential circular dependencies by limiting when and where these imports are accessed.
162-162: Guard navigation with UI readiness check
RetrievinguiStatefromuseUiStorebefore proceeding ensures that page navigation only occurs when the interface is in a READY state, reducing the risk of race conditions or conflicting loads.
208-209: Show fast loading state on toggle prettification
InvokingsetUiState(UI_STATE.FAST_LOADING)at the start of the prettification toggle provides immediate visual feedback to the user, aligning the UI behaviour with the underlying asynchronous operation.
213-213: Localise context store access for error handling
DestructuringlogEventNumandpostPopUpright before thecatchblock ensures that these values are captured at the moment of need and helps prevent unintended import order or circular dependency issues.
222-223: Optimise state retrieval for prettification flow
PullingupdatePageDataandlogFileManagerProxyimmediately before the asynchronous call keeps related code together, improving locality and guarding against stale closures or circular import pitfalls.
246-246: Reset UI state after updating page data
MovingsetUiState(UI_STATE.READY)to execute after both the store update and the URL hash change ensures the UI only transitions to the ready state once all side effects have completed.
|
@junhaoliao if possible, let's merge this before we rebase #152, as it fixes some zustand store usage errors. |
# Conflicts: # src/stores/queryStore/createQueryControllerSlice.ts
# Conflicts: # src/stores/queryStore/createQueryControllerSlice.ts
|
i merged on my end instead |
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/components/AppController.tsx(3 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/AppController.tsx
🧬 Code Graph Analysis (1)
src/components/AppController.tsx (3)
src/services/LogFileManager/index.ts (2)
numEvents(124-126)cursor(492-514)src/typings/states.ts (1)
UI_STATE(106-106)src/typings/worker.ts (2)
CursorType(107-107)CURSOR_CODE(100-100)
🔇 Additional comments (5)
src/components/AppController.tsx (5)
87-95: Store access re-organization for improved code structureThe refactoring of the hook usage reorganizes Zustand state imports to prevent circular dependencies. Moving store-specific selectors together makes the code more maintainable and easier to track dependencies.
127-128: Improved input validation orderMoving the clamping logic earlier in the code flow is a good improvement that ensures the event number is valid before proceeding with further operations.
144-144: Improved state management approachUsing the
updatePageDataselector from the store is a better approach than directly mutating state.
146-146: Enhanced error loggingAdding console error logging before displaying the popup provides better debugging information without affecting the user experience.
154-162: Updated effect dependenciesThe effect dependency array has been properly updated to include
updatePageDatasince it's now being used within the effect. This ensures the effect will re-run when any of its dependencies change.
src/stores/logFileStore.ts
Outdated
| (async () => { | ||
| const {logFileManagerProxy} = useLogFileManagerProxyStore.getState(); | ||
| const decoderOptions = getConfig(CONFIG_KEY.DECODER_OPTIONS); | ||
| const {isPrettified} = useViewStore.getState(); |
There was a problem hiding this comment.
Can we move this before const pageData, where it is being used?
…s from code review
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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 (8)
src/services/LogFileManager/index.ts (2)
create(164-189)cursor(492-514)src/typings/worker.ts (2)
FileSrcType(108-108)CursorType(107-107)src/typings/states.ts (1)
UI_STATE(106-106)src/stores/logExportStore.ts (1)
LOG_EXPORT_STORE_DEFAULT(65-65)src/contexts/UrlContextProvider.tsx (1)
updateWindowUrlSearchParams(279-279)src/typings/url.ts (1)
SEARCH_PARAM_NAMES(38-38)src/utils/config.ts (1)
getConfig(182-182)src/typings/config.ts (1)
CONFIG_KEY(44-44)
🔇 Additional comments (5)
src/stores/logFileStore.ts (5)
100-100: LGTM: Added ESLint directive.This directive is appropriate for the function since it exceeds the line limit.
105-105: Good improvement: UI state now set immediately.Moving this UI state update to the beginning of the function ensures users see the loading state right away, improving the perceived responsiveness of the application.
107-119: Good refactoring: State access now grouped by usage.Reorganizing the state access to be closer to where it's used improves code readability and reduces the risk of circular dependencies between stores.
127-129: Good refactoring: Deferred state access until needed.Moving these accesses inside the async function means they're retrieved just before they're needed, which is good practice for preventing circular dependencies.
141-143: Good refactoring: ViewStore state accessed at point of use.Moving these state accesses after setting the file info and right before they're used improves code organization and maintainability.
| setLogData("Loading..."); | ||
|
|
||
| set({fileSrc}); | ||
| if ("string" !== typeof fileSrc) { | ||
| updateWindowUrlSearchParams({[SEARCH_PARAM_NAMES.FILE_PATH]: null}); | ||
| } | ||
|
|
||
| const decoderOptions = getConfig(CONFIG_KEY.DECODER_OPTIONS); | ||
| const {postPopUp} = useContextStore.getState(); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider moving postPopUp inside the async function.
For consistency with the pattern of other state retrievals in this PR, consider moving this line inside the async function, closer to where it's used in the catch block at line 153.
- const {postPopUp} = useContextStore.getState();
(async () => {
const {logFileManagerProxy} = useLogFileManagerProxyStore.getState();
const decoderOptions = getConfig(CONFIG_KEY.DECODER_OPTIONS);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/stores/logFileStore.ts at line 125, the variable postPopUp is retrieved
from useContextStore.getState() outside the async function, but for consistency
with other state retrievals and to keep the code cleaner, move the declaration
of postPopUp inside the async function, closer to its usage in the catch block
around line 153. This will ensure the state is accessed only when needed and
aligns with the pattern used elsewhere in the code.
There was a problem hiding this comment.
if i move it into the async function, don't i need to retrieve postPopUp again in the catch handler?
There was a problem hiding this comment.
@junhaoliao You're absolutely right! If you move postPopUp into the main async function body, you would need to retrieve it again in the catch handler since the scope would be different.
Looking at other changes in this PR, I see a pattern where state retrievals are moved closer to their actual usage points. For postPopUp specifically, a consistent approach would be to move it directly inside the catch handler where it's needed:
(async () => {
const {logFileManagerProxy} = useLogFileManagerProxyStore.getState();
const decoderOptions = getConfig(CONFIG_KEY.DECODER_OPTIONS);
// ...rest of function...
if (0 === decoderOptions.formatString.length && canFormat) {
const {postPopUp} = useContextStore.getState();
postPopUp(FORMAT_POP_UP_MESSAGE);
}
})().catch((e: unknown) => {
console.error(e);
const {postPopUp} = useContextStore.getState();
postPopUp({
level: LOG_LEVEL.ERROR,
message: String(e),
timeoutMillis: DO_NOT_TIMEOUT_VALUE,
title: "Action failed",
});
setUiState(UI_STATE.UNOPENED);
});This approach keeps the state retrieval closest to where it's used, which seems to be the pattern in this refactoring.
✏️ Learnings added
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#265
File: src/stores/logFileStore.ts:125-125
Timestamp: 2025-05-17T05:13:39.921Z
Learning: When refactoring Zustand state imports to prevent circular dependencies in the yscope-log-viewer project, the preferred pattern is to move state retrievals closest to where they're actually used rather than retrieving them at the beginning of the function.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Let's see if @hoophalab has any other comments. Otherwise, I'm good with the changes
for the title, how about
- rephrasing
state imports->state retrievals? - mentioning that we added
console.errorfor all places we callpostPopUp()with errors?
i.e.,
refactor: Reorganize Zustand state retrievals to prevent potential circular dependencies (resolves #264); Add error logging before `postPopUp()`.
Henry8192
changed the title
postPopUp().
Henry8192
changed the title
postPopUp().postPopUp().
Description
Resolves #264.
Checklist
breaking change.
Validation performed
Summary by CodeRabbit