refactor: Replace StateContextProvider with Zustand for state management (resolves #168, resolves #211). #224
Conversation
## Walkthrough
This change refactors the application's state management from React context to multiple Zustand stores, replacing the `StateContextProvider` with a new `AppController` and introducing dedicated stores for log files, UI, queries, view state, and log export. It also replaces manual web worker message handling with a Comlink-based proxy architecture.
## Changes
| File(s) | Change Summary |
|-----------------------------------------------------------------------------------------|---------------|
| `package.json` | Added dependencies: `comlink` and `zustand`. |
| `src/components/CentralContainer/Sidebar/SidebarTabs/FileInfoTabPanel.tsx`<br>`src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/QueryInputBox.tsx`<br>`src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx`<br>`src/components/CentralContainer/Sidebar/SidebarTabs/SettingsTabPanel/index.tsx`<br>`src/components/CentralContainer/Sidebar/SidebarTabs/index.tsx`<br>`src/components/CentralContainer/Sidebar/index.tsx`<br>`src/components/DropFileContainer/index.tsx`<br>`src/components/Editor/index.tsx`<br>`src/components/MenuBar/ExportLogsButton.tsx`<br>`src/components/MenuBar/NavigationBar.tsx`<br>`src/components/MenuBar/PageNumInput.tsx`<br>`src/components/MenuBar/index.tsx`<br>`src/components/StatusBar/LogLevelSelect/index.tsx`<br>`src/components/StatusBar/index.tsx` | Refactored components to use Zustand stores instead of React context for state management. |
| `src/contexts/StateContextProvider.tsx` | Removed all internal state, logic, and side effects; now provides a null context and delegates all state to external stores. |
| `src/contexts/AppController.tsx` | Introduced new `AppController` component for URL synchronization and high-level state management using Zustand stores. |
| `src/App.tsx` | Replaced `StateContextProvider` with `AppController` in the component tree. |
| `src/services/MainWorker.ts` | Replaced manual message/event handling with Comlink's `expose` for a `LogFileManagerProxy` instance. |
| `src/services/LogFileManagerProxy.ts` | Added new `LogFileManagerProxy` class to encapsulate log file operations and expose them via Comlink. |
| `src/services/LogFileManager/index.ts` | Added `FILE_TYPE` enum and `fileType` getter; updated `loadPage` method signature. |
| `src/services/decoders/ClpIrDecoder/index.ts`<br>`src/services/decoders/JsonlDecoder/index.ts` | Removed `postFormatPopup` side effect; added `irStreamType` getter to `ClpIrDecoder`. |
| `src/typings/worker.ts` | Removed detailed worker protocol types/enums; added minimal `LogFileInfo` and `PageData` types. |
| `src/stores/contextStore.ts` | Added new Zustand store for context-related state (log event number, popup posting). |
| `src/stores/logExportStore.ts` | Added new Zustand store for log export state and actions. |
| `src/stores/logFileManagerProxyStore.ts` | Added new Zustand store to manage a Comlink remote proxy to the log file manager. |
| `src/stores/logFileStore.ts` | Added new Zustand store for log file metadata, loading, and related actions. |
| `src/stores/queryStore/createQueryConfigSlice.ts`<br>`src/stores/queryStore/createQueryControllerSlice.ts`<br>`src/stores/queryStore/createQueryResultsSlice.ts`<br>`src/stores/queryStore/index.ts`<br>`src/stores/queryStore/types.ts` | Added Zustand slices and store for query configuration, control, and results. |
| `src/stores/uiStore.ts` | Added new Zustand store for UI state (active tab, UI status). |
| `src/stores/viewStore.ts` | Added new Zustand store for view/page state and navigation. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant AppController
participant ZustandStores
participant LogFileManagerProxy (Comlink)
participant MainWorker
AppController->>ZustandStores: Update/query state (UI, file, view, query)
AppController->>LogFileManagerProxy: loadFile(), loadPage(), etc. (via Comlink)
LogFileManagerProxy->>MainWorker: Method call via Comlink
MainWorker->>LogFileManagerProxy: Response via Comlink
LogFileManagerProxy->>ZustandStores: Update state via callback (export chunk, query results)
ZustandStores->>AppController: State change triggers UI updateAssessment against linked issues
Possibly related PRs
Suggested reviewers
Follow coding guidelines for boolean comparisons Per your coding guidelines, prefer - if ("string" !== typeof fileSrc) {
+ if ("string" == typeof fileSrc) {
+ // Code for string file source if needed
+ } else {
updateWindowUrlSearchParams({[SEARCH_PARAM_NAMES.FILE_PATH]: null});
+ }
Follow coding guidelines for boolean comparisons Per your coding guidelines, prefer - if (QUERY_PROGRESS_VALUE_MIN === progress) {
+ if (QUERY_PROGRESS_VALUE_MIN == progress) {
Follow coding guidelines for boolean comparisons Per your coding guidelines, prefer - if (0 === decoderOptions.formatString.length && canFormat) {
+ if (0 == decoderOptions.formatString.length && canFormat) {
postPopUp(FORMAT_POP_UP_MESSAGE);
}
✨ 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 (
|
|
adding @hoophalab for awareness |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Henry8192
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
src/services/LogFileManager/index.ts
Outdated
| // fall through to unreachable error. | ||
| } | ||
| } | ||
| throw new Error("Unexpected decoder type when determining file type."); |
There was a problem hiding this comment.
to fix the linter
| throw new Error("Unexpected decoder type when determining file type."); | |
| throw new Error("Unexpected decoder type when determining file type."); |
Co-authored-by: Junhao Liao <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/services/LogFileManager/index.ts (1)
1-1: Consider refactoring rather than increasing ESLint limits.Increasing the max-lines limit from 500 to 600 suggests the file could benefit from further modularization. Consider splitting functionality into separate modules to improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/services/LogFileManager/index.ts(7 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/services/LogFileManager/index.ts
🔇 Additional comments (3)
src/services/LogFileManager/index.ts (3)
44-48: Good addition of a typed enum for file types.Creating a proper enum for file types improves type safety and makes the code more maintainable. This is a good practice when working with a fixed set of values.
283-283: Good simplification of the return type.Replacing the
WorkerResp<WORKER_RESP_CODE.PAGE_DATA>type with the directPageDatatype simplifies the API and aligns with the refactoring to Zustand stores.
517-517: Good exposure of theFILE_TYPEenum.Exporting the
FILE_TYPEenum makes it available for use in other modules, which is necessary for proper typing in the refactored state management.
|
|
||
| // fall through to unreachable error. | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Fix trailing whitespace on this line.
Remove the trailing whitespace to comply with linting rules.
- // fall through to unreachable error.
- }
+ // fall through to unreachable error.
+ }Committable suggestion skipped: line range outside the PR's diff.
| /** | ||
| * Returns the type of file based on the decoder in use. | ||
| * | ||
| * @return The detected file type. | ||
| * @throws {Error} If the decoder type is unknown. | ||
| */ | ||
| get fileType (): FILE_TYPE { | ||
| const decoder = this.#decoder; | ||
| if (decoder instanceof JsonlDecoder) { | ||
| return FILE_TYPE.JSONL; | ||
| } else if (decoder instanceof ClpIrDecoder) { | ||
| switch (decoder.irStreamType) { | ||
| case CLP_IR_STREAM_TYPE.STRUCTURED: | ||
| return FILE_TYPE.CLP_KV_IR; | ||
| case CLP_IR_STREAM_TYPE.UNSTRUCTURED: | ||
| return FILE_TYPE.CLP_TEXT_IR; | ||
| default: | ||
|
|
||
| // fall through to unreachable error. | ||
| } | ||
| } | ||
| throw new Error("Unexpected decoder type when determining file type."); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make fileType implementation more future-proof and remove class checks.
Using instanceof ties this method to the current decoder list and will fail silently if a decoder is proxied across workers (where prototype chains are lost) or if a new decoder class is added.
A safer pattern:
- get fileType (): FILE_TYPE {
- const decoder = this.#decoder;
- if (decoder instanceof JsonlDecoder) {
- return FILE_TYPE.JSONL;
- } else if (decoder instanceof ClpIrDecoder) {
- switch (decoder.irStreamType) {
- case CLP_IR_STREAM_TYPE.STRUCTURED:
- return FILE_TYPE.CLP_KV_IR;
- case CLP_IR_STREAM_TYPE.UNSTRUCTURED:
- return FILE_TYPE.CLP_TEXT_IR;
- default:
-
- // fall through to unreachable error.
- }
- }
- throw new Error("Unexpected decoder type when determining file type.");
- }
+ get fileType (): FILE_TYPE {
+ if ("isJsonlDecoder" in this.#decoder) {
+ return FILE_TYPE.JSONL;
+ } else if ("irStreamType" in this.#decoder) {
+ switch (this.#decoder.irStreamType) {
+ case CLP_IR_STREAM_TYPE.STRUCTURED:
+ return FILE_TYPE.CLP_KV_IR;
+ case CLP_IR_STREAM_TYPE.UNSTRUCTURED:
+ return FILE_TYPE.CLP_TEXT_IR;
+ default:
+ // fall through to error
+ }
+ }
+ throw new Error("Unexpected decoder type when determining file type.");
+ }This approach uses feature detection rather than type checking, making it more resilient to changes in the codebase.
Committable suggestion skipped: line range outside the PR's diff.
Co-authored-by: Junhao Liao <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/stores/logFileStore.ts (3)
111-117: ResetnumEventsduring file loadingWhen loading a new file,
fileNameandonDiskFileSizeInBytesare reset to defaults, butnumEventsis not, which could lead to stale data briefly appearing in the UI.setFileName("Loading..."); setOnDiskFileSizeInBytes(LOG_FILE_STORE_DEFAULT.onDiskFileSizeInBytes); + setNumEvents(LOG_FILE_STORE_DEFAULT.numEvents); setExportProgress(LOG_EXPORT_STORE_DEFAULT.exportProgress); clearQuery(); setUiState(UI_STATE.FILE_LOADING); setLogData("Loading...");
134-136:set(fileInfo)leaks extra keys into the Zustand statePassing the whole
fileInfoobject toset()might add properties beyond those defined inLogFileValues, potentially causing type inconsistencies or overwriting unrelated state in future refactors.- set(fileInfo); + const { + fileName, + numEvents, + onDiskFileSizeInBytes, + fileType + } = fileInfo; + set({fileName, numEvents, onDiskFileSizeInBytes, fileType});
102-154:loadFilearrow function is 50+ lines – consider extracting helpersThe function combines UI resets, proxy calls, error handling, and pop-up logic, making it harder to read, test, and lint (exceeding the 60-line limit mentioned in PR comments).
Consider extracting the following helper functions:
resetStoreState()- Lines 111-117handleSuccessfulLoad(fileInfo, pageData)- Lines 134-143handleLoadError(e)- Lines 144-153
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/stores/logFileStore.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/logFileStore.ts
🧬 Code Graph Analysis (1)
src/stores/logFileStore.ts (12)
src/typings/common.ts (1)
Nullable(10-10)src/typings/worker.ts (2)
FileSrcType(108-108)CursorType(107-107)src/services/LogFileManager/index.ts (2)
cursor(492-514)FILE_TYPE(517-517)src/typings/logs.ts (1)
LOG_LEVEL(56-56)src/typings/notifications.ts (2)
LONG_AUTO_DISMISS_TIMEOUT_MILLIS(37-37)DO_NOT_TIMEOUT_VALUE(36-36)src/typings/query.ts (2)
QueryResults(23-23)QUERY_PROGRESS_VALUE_MIN(28-28)src/stores/logExportStore.ts (1)
LOG_EXPORT_STORE_DEFAULT(63-63)src/typings/states.ts (1)
UI_STATE(106-106)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)
🪛 GitHub Check: lint-check
src/stores/logFileStore.ts
[failure] 30-30:
'VIEW_STORE_DEFAULT' is defined but never used
🪛 ESLint
src/stores/logFileStore.ts
[error] 30-30: 'VIEW_STORE_DEFAULT' is defined but never used.
(@typescript-eslint/no-unused-vars)
🪛 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 (3)
src/stores/logFileStore.ts (3)
76-76: 🧹 Nitpick (assertive)Prefer
false == <expression>over!== nullAccording to your coding guidelines, boolean expressions should use the form
false == <expression>rather than!<expression>.- if (null !== logExportManager) { + if (false == (null === logExportManager)) {Likely an incorrect or invalid review comment.
92-92: 🧹 Nitpick (assertive)Prefer
false == <expression>over===According to your coding guidelines, boolean expressions should use the form
false == <expression>rather than!<expression>or===.- if (QUERY_PROGRESS_VALUE_MIN === progress) { + if (false == (QUERY_PROGRESS_VALUE_MIN !== progress)) {Likely an incorrect or invalid review comment.
141-141: 🧹 Nitpick (assertive)Prefer
false == <expression>over===According to your coding guidelines, boolean expressions should use the form
false == <expression>rather than=== 0.- if (0 === decoderOptions.formatString.length && canFormat) { + if (false == (0 !== decoderOptions.formatString.length) && canFormat) {Likely an incorrect or invalid review comment.
| interface LogFileValues { | ||
| fileName: string; | ||
| fileSrc: Nullable<FileSrcType>; | ||
| numEvents: number; | ||
| onDiskFileSizeInBytes: number; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding fileType to LogFileValues interface
The fileType property is used in lines 138-139, but it's not included in the LogFileValues interface, suggesting it might be one of the "leaked" properties from fileInfo.
interface LogFileValues {
fileName: string;
fileSrc: Nullable<FileSrcType>;
numEvents: number;
onDiskFileSizeInBytes: number;
+ fileType?: string;
}📝 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.
| interface LogFileValues { | |
| fileName: string; | |
| fileSrc: Nullable<FileSrcType>; | |
| numEvents: number; | |
| onDiskFileSizeInBytes: number; | |
| } | |
| interface LogFileValues { | |
| fileName: string; | |
| fileSrc: Nullable<FileSrcType>; | |
| numEvents: number; | |
| onDiskFileSizeInBytes: number; | |
| fileType?: string; | |
| } |
| setFileName: (newFileName) => { | ||
| set({fileName: newFileName}); | ||
| }, | ||
| setNumEvents: (newNumEvents) => { | ||
| set({numEvents: newNumEvents}); | ||
| }, | ||
| setOnDiskFileSizeInBytes: (newSize) => { | ||
| set({onDiskFileSizeInBytes: newSize}); | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add JSDoc comments for setter functions
The setter functions lack JSDoc comments, unlike other functions in this file which are well-documented.
+ /**
+ * Sets the file name in the store
+ * @param newFileName - The new file name to set
+ */
setFileName: (newFileName) => {
set({fileName: newFileName});
},
+ /**
+ * Sets the number of events in the store
+ * @param newNumEvents - The new number of events to set
+ */
setNumEvents: (newNumEvents) => {
set({numEvents: newNumEvents});
},
+ /**
+ * Sets the on-disk file size in bytes in the store
+ * @param newSize - The new file size to set
+ */
setOnDiskFileSizeInBytes: (newSize) => {
set({onDiskFileSizeInBytes: newSize});
},📝 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.
| setFileName: (newFileName) => { | |
| set({fileName: newFileName}); | |
| }, | |
| setNumEvents: (newNumEvents) => { | |
| set({numEvents: newNumEvents}); | |
| }, | |
| setOnDiskFileSizeInBytes: (newSize) => { | |
| set({onDiskFileSizeInBytes: newSize}); | |
| }, | |
| /** | |
| * Sets the file name in the store | |
| * @param newFileName - The new file name to set | |
| */ | |
| setFileName: (newFileName) => { | |
| set({fileName: newFileName}); | |
| }, | |
| /** | |
| * Sets the number of events in the store | |
| * @param newNumEvents - The new number of events to set | |
| */ | |
| setNumEvents: (newNumEvents) => { | |
| set({numEvents: newNumEvents}); | |
| }, | |
| /** | |
| * Sets the on-disk file size in bytes in the store | |
| * @param newSize - The new file size to set | |
| */ | |
| setOnDiskFileSizeInBytes: (newSize) => { | |
| set({onDiskFileSizeInBytes: newSize}); | |
| }, |
| const {postPopUp} = useContextStore.getState(); | ||
|
|
||
| if (QUERY_CONFIG_DEFAULT.queryString === queryString) { | ||
| return; |
There was a problem hiding this comment.
I believe we need to clearResults() before early returning. Otherwise, any results from previous queries will still be rendered in the panel even if the query string is empty. e.g.,
@coderabbitai please create an issue to track this
There was a problem hiding this comment.
🧩 Analysis chain
@junhaoliao I've created an issue to track the bug where query results aren't cleared when the query string is empty. The issue describes the problem in the startQuery() function where we should call clearQueryResults() before the early return to ensure previous search results are cleared when a user empties the search box.
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue to track the bug related to not clearing query results
gh issue create \
--title "Bug: Query results not cleared when query string is empty" \
--body "## Description
When the query string is empty, the previous search results still remain displayed in the panel. This happens because in \`startQuery()\` function, we return early when the query string equals the default empty string, without clearing the previous results first.
## Current Behavior
Currently in \`src/stores/queryStore/createQueryControllerSlice.ts\`:
\`\`\`javascript
startQuery: () => {
const {
clearQueryResults,
queryString,
queryIsCaseSensitive,
queryIsRegex,
} = get();
const {logFileManagerProxy} = useLogFileManagerStore.getState();
const {postPopUp} = useContextStore.getState();
if (QUERY_CONFIG_DEFAULT.queryString === queryString) {
return;
}
clearQueryResults();
// ...
}
\`\`\`
When the query string is empty or matches the default value, the function returns early without clearing previous results, causing them to remain visible.
## Expected Behavior
The \`clearQueryResults()\` function should be called before the early return to ensure that previous results are cleared when the query string is empty:
\`\`\`javascript
startQuery: () => {
const {
clearQueryResults,
queryString,
queryIsCaseSensitive,
queryIsRegex,
} = get();
const {logFileManagerProxy} = useLogFileManagerStore.getState();
const {postPopUp} = useContextStore.getState();
clearQueryResults(); // Clear results regardless of query string value
if (QUERY_CONFIG_DEFAULT.queryString === queryString) {
return;
}
// ...
}
\`\`\`
## References
- Identified in PR: #224
- Comment: https://github.com/y-scope/yscope-log-viewer/pull/224#discussion_r2085584901
## Screenshots
" \
--assignee junhaoliao
Length of output: 1974
@junhaoliao I've created an issue to track this bug: #266. The issue details the problem where query results aren't cleared when the query string is empty, along with the proposed fix to call clearQueryResults() before the early return.
🧬 Code Graph Analysis Results
Relevant snippets for understanding src/stores/queryStore/createQueryControllerSlice.ts and the user comment:
From src/stores/queryStore/createQueryResultsSlice.ts (line 52):
QUERY_RESULTS_DEFAULT- This constant is imported and used in
clearQuery()to reset query results state. - Likely defines the default state for query results, important for understanding what "clearing results" entails.
From src/stores/queryStore/queryConfigSlice.ts (line 39):
QUERY_CONFIG_DEFAULT- This constant is imported and used in
clearQuery()and in thestartQuery()method to compare the current query string with the default. - Represents the default query configuration, including the default query string (likely empty).
- Relevant for understanding the early return condition in
startQuery().
From src/typings/logs.ts (line 56):
LOG_LEVEL- Enum or constants defining log levels, used in error handling in
startQuery()when posting a popup on failure.
From src/typings/notifications.ts (line 36):
DO_NOT_TIMEOUT_VALUE- Constant used in
startQuery()error handling to specify a popup notification that does not timeout.
From src/stores/queryStore/types.ts (lines 46-50):
QueryControllerSlice
QueryControllerValues
QueryState- Types defining the shape of the slice, values, and overall query state.
- Important for understanding the structure of the state managed by this slice.
Summary of createQueryControllerSlice (lines 10-56 in src/stores/queryStore/createQueryControllerSlice.ts):
- Defines a Zustand slice managing query state and actions.
clearQuery()resets query config, controller values, and results to their defaults.setQueryProgress(newProgress)updates query progress.startQuery():- Early returns if the current query string equals the default query string (likely empty).
- Calls
clearQueryResults()before starting a new query. - Starts the query asynchronously via
logFileManagerProxy. - Catches errors and posts a popup with error details.
These snippets and summaries provide the necessary context to understand the state management and behavior of query clearing and starting, which is directly related to the user comment about clearing results before early returning in startQuery().
| } from "./UrlContextProvider"; | ||
|
|
||
|
|
||
| const StateContext = createContext<null>(null); |
There was a problem hiding this comment.
Sorry for missing this in the previous review - we should no longer need to create a context here. And we should remove the unused export {StateContext}. instead, we can simply
return <div>
{children}
</div>
in AppController.
Also, we should rename StateContextProviderProps -> AppController.
@coderabbitai create an issue to track this
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Create an issue to track the needed cleanup of AppController.tsx
gh issue create \
--title "cleanup: Remove unused StateContext from AppController.tsx" \
--body "## Description
As noted in PR #224, we should clean up the AppController component since we no longer need the StateContext:
- Remove the creation of StateContext since it's no longer needed
- Remove the unused export of StateContext
- Replace the \`<StateContext.Provider value={null}>\` wrapper with a simple \`<div>\` wrapper in the AppController component
- Rename the interface \`StateContextProviderProps\` to \`AppControllerProps\`
This is a cleanup task after migrating from React Context to Zustand for state management.
## References
- Discussion: https://github.com/y-scope/yscope-log-viewer/pull/224#discussion_r2093320308
- Requested by: @junhaoliao"
Length of output: 813
@junhaoliao I've created an issue to track this cleanup task. You can find it here: Issue #280.
🧬 Code Graph Analysis Results
Relevant snippets for understanding src/contexts/AppController.tsx and the user comment:
From src/contexts/AppController.tsx (lines 1-132)
The entire file content is relevant as it defines the AppController component, the StateContext context, and the StateContextProviderProps interface. Key points:
StateContextis created but only used internally and exported unused.StateContextProviderPropsis an interface with a singlechildrenprop.AppControllercomponent uses multiple stores and contexts (NotificationContext,UrlContext, various Zustand stores).- It manages synchronization of URL parameters and application state, including loading files and updating UI state.
- The component returns a
StateContext.Providerwrappingchildren. - The user comment suggests removing the context creation and export, simplifying the component to just return
<div>{children}</div>. - Also suggests renaming
StateContextProviderPropstoAppController.
From src/contexts/NotificationContextProvider.tsx (line 84)
NotificationContext- This is imported and used in
AppControllerto getpostPopUpfunction. - Relevant because
postPopUpis used in error handling and effect hooks.
From src/contexts/UrlContextProvider.tsx (lines 278-282)
updateWindowUrlHashParams
UrlContext
URL_HASH_PARAMS_DEFAULT
URL_SEARCH_PARAMS_DEFAULT- These are imported and used in
AppController. updateWindowUrlHashParamsis used to update URL hash parameters.UrlContextprovidesfilePath,isPrettified, andlogEventNum.URL_HASH_PARAMS_DEFAULTandURL_SEARCH_PARAMS_DEFAULTprovide default values for URL parameters.
From src/utils/data.ts (lines 190, 193)
findNearestLessThanOrEqualElement
isWithinBounds- Utility functions used in
updateUrlIfEventOnPageto check bounds and find nearest element. - Important for URL update logic in
AppController.
From src/typings/worker.ts (lines 100, 107)
CURSOR_CODE
CursorType- Used to define cursor objects for loading pages or events.
- Used in
AppControllerto create cursor objects for loading log file pages.
From src/typings/logs.ts (line 56)
LOG_LEVEL- Used for logging levels in notifications, e.g., error pop-ups.
From src/typings/notifications.ts (line 36)
DO_NOT_TIMEOUT_VALUE- Used as a timeout value for notifications in error handling.
From src/typings/states.ts (line 106)
UI_STATE- Used to set UI state, e.g.,
UI_STATE.FAST_LOADINGinAppController.
Summary
The following are relevant to understanding and potentially refactoring AppController.tsx as per the user comment:
- The entire
AppController.tsxfile (lines 1-132). NotificationContextfromNotificationContextProvider.tsx.updateWindowUrlHashParams,UrlContext,URL_HASH_PARAMS_DEFAULT,URL_SEARCH_PARAMS_DEFAULTfromUrlContextProvider.tsx.- Utility functions
findNearestLessThanOrEqualElementandisWithinBoundsfromutils/data.ts. - Types and constants
CURSOR_CODE,CursorTypefromtypings/worker.ts. LOG_LEVELfromtypings/logs.ts.DO_NOT_TIMEOUT_VALUEfromtypings/notifications.ts.UI_STATEfromtypings/states.ts.
These snippets provide the context for the state management, URL synchronization, notification handling, and cursor logic used in AppController. They are essential for understanding the current implementation and the implications of removing the context and simplifying the component as suggested.
| useEffect(() => { | ||
| setPostPopUp(postPopUp); | ||
| }, [ | ||
| postPopUp, | ||
| setPostPopUp, | ||
| ]); |
There was a problem hiding this comment.
We should setPostPopUp() to update the reference in contextStore before loading a file, or any error occured during file loading would not show a UI pop-up. e.g., accessing this should show a 404 error pop-up but nothing was shown: https://y-scope.github.io/yscope-log-viewer/?filePath=does_not_exist
@coderabbitai create an issue to track this
There was a problem hiding this comment.
🧩 Analysis chain
@junhaoliao I've created an issue to track this bug. The issue documents that error pop-ups don't appear when loading non-existent files because setPostPopUp() is called after file loading in the current implementation. The issue includes reproduction steps, root cause analysis, and a proposed solution to move the setPostPopUp() call before the file loading logic.
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue to track the bug where error pop-ups don't appear during file loading
gh issue create \
--title "Bug: Error pop-ups don't appear when loading non-existent files" \
--body "## Description
When attempting to load a non-existent file, the application should display an error pop-up, but currently nothing is shown.
## Reproduction Steps
1. Visit https://y-scope.github.io/yscope-log-viewer/?filePath=does_not_exist
2. Notice that no error pop-up appears, even though a 404 error should be displayed
## Root Cause
In the AppController component, \`setPostPopUp()\` is called in a useEffect hook, but this happens after the file loading attempt. Any errors that occur during file loading won't show a UI pop-up because the pop-up function reference hasn't been updated in the contextStore yet.
## Proposed Solution
Move the \`setPostPopUp()\` call before loading a file to ensure that error pop-ups can be displayed for any errors during the file loading process.
## References
- Referenced in PR: #224
- Comment: https://github.com/y-scope/yscope-log-viewer/pull/224#discussion_r2101376855
cc @junhaoliao" \
--assignee junhaoliao
Length of output: 1117
@junhaoliao I've created an issue to track this bug: #290. The issue describes the problem where error pop-ups don't appear when loading non-existent files because setPostPopUp() is called after file loading in the current implementation.
🧬 Code Graph Analysis Results
Relevant snippets for understanding src/contexts/AppController.tsx and the user comment:
From src/contexts/NotificationContextProvider.tsx (line 84)
NotificationContext
- This is the React context providing the
postPopUpfunction used inAppControllerto show UI pop-ups for notifications/errors.
From src/contexts/UrlContextProvider.tsx (lines 278-282)
updateWindowUrlHashParams
UrlContext
URL_HASH_PARAMS_DEFAULT
URL_SEARCH_PARAMS_DEFAULT
updateWindowUrlHashParams: Function used inAppControllerto update URL hash parameters, e.g., updatinglogEventNum.UrlContext: React context providing URL parameters likefilePath,isPrettified, andlogEventNumused inAppController.URL_HASH_PARAMS_DEFAULTandURL_SEARCH_PARAMS_DEFAULT: Default values for URL hash and search parameters, used for comparison inAppController.
From src/typings/logs.ts (line 56)
LOG_LEVEL
- Enum or constants defining log levels (e.g., ERROR) used in
postPopUpcalls inAppController.
From src/typings/notifications.ts (line 36)
DO_NOT_TIMEOUT_VALUE
- Constant used as
timeoutMillisinpostPopUpto indicate the notification should not timeout automatically.
From src/typings/worker.ts (lines 100-107)
CURSOR_CODE
CursorType
CURSOR_CODE: Enum or constants defining cursor codes likeEVENT_NUM,LAST_EVENT.CursorType: Type describing the cursor object used to load pages or files inAppController.
From src/utils/data.ts (lines 190-193)
findNearestLessThanOrEqualElement
isWithinBounds
- Utility functions used in
updateUrlIfEventOnPageto check if a log event number is within bounds and to find the nearest event number on the current page.
From src/services/LogFileManager/index.ts (lines 124-126, 492-514)
get numEvents () {
return this.#numEvents;
}
#getCursorData (cursor: CursorType, numActiveEvents: number): CursorData {
const {code, args} = cursor;
switch (code) {
case CURSOR_CODE.PAGE_NUM:
return getPageNumCursorData(
args.pageNum,
args.eventPositionOnPage,
numActiveEvents,
this.#pageSize,
);
case CURSOR_CODE.LAST_EVENT:
return getLastEventCursorData(numActiveEvents, this.#pageSize);
case CURSOR_CODE.EVENT_NUM:
return getEventNumCursorData(
args.eventNum,
numActiveEvents,
this.#pageSize,
this.#decoder.getFilteredLogEventMap(),
);
default:
throw new Error(`Unsupported cursor type: ${code}`);
}
}
numEventsgetter: Returns the number of events in the log file, used inAppControllerto clamp event numbers.#getCursorData: Internal method to get cursor data based on cursor type, relevant to howAppControllerconstructs cursors for loading pages/files.
These snippets provide context on:
- How notifications (
postPopUp) are triggered and managed. - How URL parameters are read and updated.
- How cursor types and codes are used to load specific pages or events.
- How event numbers are validated and clamped.
- How errors during file loading might be surfaced via notifications.
This context is directly relevant to the user comment about ensuring setPostPopUp() updates the reference before loading a file to properly show error pop-ups on file load failure.
✏️ Learnings added
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#224
File: src/contexts/AppController.tsx:185-190
Timestamp: 2025-05-22T00:03:20.535Z
Learning: In the AppController component, `setPostPopUp()` should be called before loading a file to ensure error pop-ups appear when file loading fails, such as with non-existent files.
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#224
File: src/contexts/AppController.tsx:185-190
Timestamp: 2025-05-22T00:03:20.535Z
Learning: In the AppController component, `setPostPopUp()` should be called before loading a file to ensure error pop-ups appear when file loading fails, such as with non-existent files.
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.
Description
Resolves #168; Resolves #211.
Checklist
breaking change.
Validation performed
logExportStore Test
exportProgress. After the download finishes, the download button becomes green.queryStore Test
uiStore Test
uiStatemakes most buttons unavailable. After loading a file, theuiStategets updated and makes the unavailable buttons available.mainWorkerStore Test
destroy()andinit()works.Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores