Skip to content

refactor: Reorganize Zustand state retrievals to prevent potential circular dependencies (resolves #264); Add error logging before postPopUp(). #265

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 16 commits into from
May 20, 2025
Merged

refactor: Reorganize Zustand state retrievals to prevent potential circular dependencies (resolves #264); Add error logging before postPopUp(). #265

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
ddb5889
use getState() JIT
Henry8192 May 12, 2025
53149f7
useStore in a reactive way
Henry8192 May 12, 2025
2ba737b
fix lint
Henry8192 May 12, 2025
0cef615
minor fix
Henry8192 May 14, 2025
b419218
Merge branch 'main' into zustand-refactor
Henry8192 May 15, 2025
56f48a6
Merge branch 'main' into zustand-refactor
Henry8192 May 15, 2025
22e3732
Merge branch 'main' into zustand-refactor
junhaoliao May 16, 2025
6d2d5d9
fix(AppController, viewStore): Add error logging before calling postP…
junhaoliao May 16, 2025
3468220
refactor(AppController): Re-oreder calling sequence in logEventNum us…
junhaoliao May 16, 2025
10697e9
refactor(query, logExport, viewStore): reorder
junhaoliao May 16, 2025
b07cf05
refactor(AppController): reorder state variable
junhaoliao May 16, 2025
43ebb53
Use clampedLogEventNum when request for a new page - Apply suggestion…
junhaoliao May 16, 2025
6695ad0
reorder
junhaoliao May 16, 2025
d4732b4
reorder
junhaoliao May 17, 2025
84643e4
Merge branch 'main' into zustand-refactor
Henry8192 May 20, 2025
b29cd52
address change from code review
Henry8192 May 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 15 additions & 14 deletions src/components/AppController.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,14 +84,15 @@ const AppController = ({children}: AppControllerProps) => {
const {filePath, isPrettified, logEventNum} = useContext(UrlContext);

// States
const beginLineNumToLogEventNum = useViewStore((state) => state.beginLineNumToLogEventNum);
const setIsPrettified = useViewStore((state) => state.updateIsPrettified);
const setLogEventNum = useContextStore((state) => state.setLogEventNum);
const setPostPopUp = useContextStore((state) => state.setPostPopUp);
const logFileManagerProxy = useLogFileManagerStore((state) => state.logFileManagerProxy);
const loadFile = useLogFileStore((state) => state.loadFile);
const {logFileManagerProxy} = useLogFileManagerStore.getState();
const numEvents = useLogFileStore((state) => state.numEvents);
const setLogEventNum = useContextStore((state) => state.setLogEventNum);
const beginLineNumToLogEventNum = useViewStore((state) => state.beginLineNumToLogEventNum);
const setIsPrettified = useViewStore((state) => state.updateIsPrettified);
const updatePageData = useViewStore((state) => state.updatePageData);
const setUiState = useUiStore((state) => state.setUiState);
const setPostPopUp = useContextStore((state) => state.setPostPopUp);

// Refs
const isPrettifiedRef = useRef<boolean>(isPrettified ?? false);
Expand Down Expand Up @@ -123,27 +124,26 @@ const AppController = ({children}: AppControllerProps) => {
return;
}

const clampedLogEventNum = clamp(logEventNum, 1, numEvents);
const logEventNumsOnPage: number [] =
Array.from(beginLineNumToLogEventNum.values());

const clampedLogEventNum = clamp(logEventNum, 1, numEvents);

if (updateUrlIfEventOnPage(clampedLogEventNum, logEventNumsOnPage)) {
// No need to request a new page since the log event is on the current page.
return;
}

const cursor: CursorType = {
code: CURSOR_CODE.EVENT_NUM,
args: {eventNum: logEventNum},
};

setUiState(UI_STATE.FAST_LOADING);

(async () => {
const cursor: CursorType = {
code: CURSOR_CODE.EVENT_NUM,
args: {eventNum: clampedLogEventNum},
};
const pageData = await logFileManagerProxy.loadPage(cursor, isPrettifiedRef.current);
useViewStore.getState().updatePageData(pageData);
updatePageData(pageData);
})().catch((e: unknown) => {
console.error(e);
postPopUp({
level: LOG_LEVEL.ERROR,
message: String(e),
Expand All @@ -156,8 +156,9 @@ const AppController = ({children}: AppControllerProps) => {
logEventNum,
logFileManagerProxy,
numEvents,
setUiState,
postPopUp,
setUiState,
updatePageData,
]);

// On `filePath` update, load file.
Expand Down
10 changes: 6 additions & 4 deletions src/stores/logExportStore.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,21 +34,23 @@ const useLogExportStore = create<LogExportState>((set) => ({
set({exportProgress: newProgress});
},
exportLogs: () => {
const {postPopUp} = useContextStore.getState();
const {logFileManagerProxy} = useLogFileManagerProxyStore.getState();
const {fileName, numEvents} = useLogFileStore.getState();

set({exportProgress: EXPORT_LOGS_PROGRESS_VALUE_MIN});

const {fileName, numEvents} = useLogFileStore.getState();
const logExportManager = new LogExportManager(
Math.ceil(numEvents / EXPORT_LOGS_CHUNK_SIZE),
fileName
);

set({logExportManager});

(async () => {
const {logFileManagerProxy} = useLogFileManagerProxyStore.getState();
await logFileManagerProxy.exportLogs();
})().catch((e: unknown) => {
console.error(e);

const {postPopUp} = useContextStore.getState();
postPopUp({
level: LOG_LEVEL.ERROR,
message: String(e),
Expand Down
25 changes: 16 additions & 9 deletions src/stores/logFileStore.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,31 +97,35 @@ const handleQueryResults = (progress: number, results: QueryResults) => {
mergeQueryResults(results);
};

// eslint-disable-next-line max-lines-per-function
const useLogFileStore = create<LogFileState>((set, get) => ({
...LOG_FILE_STORE_DEFAULT,
loadFile: (fileSrc: FileSrcType, cursor: CursorType) => {
const {setFileName, setOnDiskFileSizeInBytes} = get();
const {postPopUp} = useContextStore.getState();
const {setExportProgress} = useLogExportStore.getState();
const {logFileManagerProxy} = useLogFileManagerProxyStore.getState();
const {clearQuery} = useQueryStore.getState();
const {setUiState} = useUiStore.getState();
const {isPrettified, setLogData, updatePageData} = useViewStore.getState();
setUiState(UI_STATE.FILE_LOADING);

const {setFileName, setOnDiskFileSizeInBytes} = get();
setFileName("Loading...");
setOnDiskFileSizeInBytes(LOG_FILE_STORE_DEFAULT.onDiskFileSizeInBytes);

const {setExportProgress} = useLogExportStore.getState();
setExportProgress(LOG_EXPORT_STORE_DEFAULT.exportProgress);

const {clearQuery} = useQueryStore.getState();
clearQuery();
setUiState(UI_STATE.FILE_LOADING);

const {setLogData} = useViewStore.getState();
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();
Copy link

@coderabbitai coderabbitai bot May 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if i move it into the async function, don't i need to retrieve postPopUp again in the catch handler?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

(async () => {
const {logFileManagerProxy} = useLogFileManagerProxyStore.getState();
const decoderOptions = getConfig(CONFIG_KEY.DECODER_OPTIONS);
const fileInfo = await logFileManagerProxy.loadFile(
{
decoderOptions: decoderOptions,
Expand All @@ -131,8 +135,11 @@ const useLogFileStore = create<LogFileState>((set, get) => ({
Comlink.proxy(handleExportChunk),
Comlink.proxy(handleQueryResults)
);
const pageData = await logFileManagerProxy.loadPage(cursor, isPrettified);

set(fileInfo);

const {isPrettified, updatePageData} = useViewStore.getState();
const pageData = await logFileManagerProxy.loadPage(cursor, isPrettified);
updatePageData(pageData);

const canFormat = fileInfo.fileType === FILE_TYPE.CLP_KV_IR ||
Expand Down
19 changes: 10 additions & 9 deletions src/stores/queryStore/createQueryControllerSlice.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,21 +39,22 @@ const createQueryControllerSlice: StateCreator<
set({queryProgress: newProgress});
},
startQuery: () => {
const {
clearQueryResults,
queryString,
queryIsCaseSensitive,
queryIsRegex,
} = get();
const {logFileManagerProxy} = useLogFileManagerStore.getState();
const {postPopUp} = useContextStore.getState();

const {clearQueryResults} = get();
clearQueryResults();

(async () => {
const {logFileManagerProxy} = useLogFileManagerStore.getState();
const {
queryString,
queryIsCaseSensitive,
queryIsRegex,
} = get();

await logFileManagerProxy.startQuery(queryString, queryIsRegex, queryIsCaseSensitive);
})().catch((e: unknown) => {
console.error(e);

const {postPopUp} = useContextStore.getState();
postPopUp({
level: LOG_LEVEL.ERROR,
message: String(e),
Expand Down
60 changes: 40 additions & 20 deletions src/stores/viewStore.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,43 +109,48 @@ const getPageNumCursor = (
const useViewStore = create<ViewState>((set, get) => ({
...VIEW_STORE_DEFAULT,
filterLogs: (filter: LogLevelFilter) => {
const {updatePageData} = get();
const {logEventNum, postPopUp} = useContextStore.getState();
const {logFileManagerProxy} = useLogFileManagerStore.getState();
const {setUiState} = useUiStore.getState();
setUiState(UI_STATE.FAST_LOADING);

const {clearQuery} = useQueryStore.getState();
clearQuery();
Comment on lines +115 to +116
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see the last comment


(async () => {
const {logFileManagerProxy} = useLogFileManagerStore.getState();
const {logEventNum} = useContextStore.getState();
const {isPrettified} = get();
const pageData = await logFileManagerProxy.setFilter(
{
code: CURSOR_CODE.EVENT_NUM,
args: {
eventNum: logEventNum,
},
},
get().isPrettified,
isPrettified,
filter
);

const {updatePageData} = get();
updatePageData(pageData);

const {startQuery} = useQueryStore.getState();
startQuery();
})().catch((e: unknown) => {
console.error(e);

const {postPopUp} = useContextStore.getState();
postPopUp({
Comment on lines +141 to 142
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the last comment

level: LOG_LEVEL.ERROR,
message: String(e),
timeoutMillis: DO_NOT_TIMEOUT_VALUE,
title: "Action failed",
});
});
const {startQuery} = useQueryStore.getState();
startQuery();
},
loadPageByAction: (navAction: NavigationAction) => {
const {isPrettified, numPages, pageNum, updatePageData} = get();
const {logEventNum, postPopUp} = useContextStore.getState();
const {logFileManagerProxy} = useLogFileManagerStore.getState();
const {fileSrc, loadFile} = useLogFileStore.getState();
const {uiState, setUiState} = useUiStore.getState();
if (navAction.code === ACTION_NAME.RELOAD) {
const {fileSrc, loadFile} = useLogFileStore.getState();
const {logEventNum} = useContextStore.getState();
if (null === fileSrc || CONTEXT_STORE_DEFAULT.logEventNum === logEventNum) {
throw new Error(
`Unexpected fileSrc=${JSON.stringify(
Expand All @@ -161,25 +166,33 @@ const useViewStore = create<ViewState>((set, get) => ({
return;
}

const {uiState, setUiState} = useUiStore.getState();
if (UI_STATE.READY !== uiState) {
console.warn("Skipping navigation: page load in progress.");

return;
}
setUiState(UI_STATE.FAST_LOADING);

const {numPages, pageNum} = get();
const cursor = getPageNumCursor(navAction, pageNum, numPages);
if (null === cursor) {
console.error(`Error with nav action ${navAction.code}.`);

return;
}

setUiState(UI_STATE.FAST_LOADING);

(async () => {
const {logFileManagerProxy} = useLogFileManagerStore.getState();
const {isPrettified} = get();
const pageData = await logFileManagerProxy.loadPage(cursor, isPrettified);

const {updatePageData} = get();
updatePageData(pageData);
})().catch((e: unknown) => {
console.error(e);

const {postPopUp} = useContextStore.getState();
postPopUp({
level: LOG_LEVEL.ERROR,
message: String(e),
Expand All @@ -201,16 +214,17 @@ const useViewStore = create<ViewState>((set, get) => ({
set({pageNum: newPageNum});
},
updateIsPrettified: (newIsPrettified: boolean) => {
const {updatePageData} = get();
const {logEventNum, postPopUp} = useContextStore.getState();
const {logFileManagerProxy} = useLogFileManagerStore.getState();
const {setUiState} = useUiStore.getState();
if (newIsPrettified === get().isPrettified) {
const {isPrettified} = get();
if (newIsPrettified === isPrettified) {
return;
}
set({isPrettified: newIsPrettified});

const {setUiState} = useUiStore.getState();
setUiState(UI_STATE.FAST_LOADING);
Comment on lines +222 to 223
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the last comment


set({isPrettified: newIsPrettified});

const {logEventNum} = useContextStore.getState();
let cursor: CursorType = {code: CURSOR_CODE.LAST_EVENT, args: null};
if (CONTEXT_STORE_DEFAULT.logEventNum !== logEventNum) {
cursor = {
Expand All @@ -220,9 +234,15 @@ const useViewStore = create<ViewState>((set, get) => ({
}

(async () => {
const {logFileManagerProxy} = useLogFileManagerStore.getState();
const pageData = await logFileManagerProxy.loadPage(cursor, newIsPrettified);

const {updatePageData} = get();
updatePageData(pageData);
})().catch((e: unknown) => {
console.error(e);

const {postPopUp} = useContextStore.getState();
postPopUp({
level: LOG_LEVEL.ERROR,
message: String(e),
Expand All @@ -232,7 +252,6 @@ const useViewStore = create<ViewState>((set, get) => ({
});
},
updatePageData: (pageData: PageData) => {
const {setUiState} = useUiStore.getState();
set({
logData: pageData.logs,
numPages: pageData.numPages,
Expand All @@ -242,6 +261,7 @@ const useViewStore = create<ViewState>((set, get) => ({
updateWindowUrlHashParams({
logEventNum: pageData.logEventNum,
});
const {setUiState} = useUiStore.getState();
setUiState(UI_STATE.READY);
Comment on lines +264 to 265
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const {setUiState} = useUiStore.getState();
setUiState(UI_STATE.READY);
useUiStore.getState().setUiState(UI_STATE.READY);

I personally prefer calling state functions this way, but feel free to use whatever style looks best for you.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point raised. we previously discussed both approaches and we ended up preferring the destructuring syntax, to avoid chaining the calls

},
}));
Expand Down
Loading