fix: Make log exporting feature asynchronous, preventing UI freeze in the front-end (fixes #92). #203
Conversation
WalkthroughThis pull request refactors the log export functionality by renaming constants and updating related code references. The constants tracking log export progress have been renamed from Changes
Sequence Diagram(s)sequenceDiagram
participant UI as ExportLogsButton
participant SC as StateContextProvider
participant MW as MainWorker
participant LF as LogFileManager
UI->>SC: User initiates log export
SC->>MW: Send EXPORT_LOGS request
MW->>LF: Invoke exportChunkAndScheduleNext()
LF-->>MW: Return log data (string)
MW->>SC: Respond with log data
SC->>UI: Update export progress display
Assessment against linked issues
Possibly related PRs
Suggested reviewers
📜 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 `!`.
🧬 Code Definitions (1)src/services/LogFileManager/index.ts (3)
🔇 Additional comments (6)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/services/MainWorker.ts (1)
61-61: Removed redundant ESLint rule.Keeping only
max-lines-per-functionis sufficient as it effectively covers the same concerns as the removedmax-statementsrule.🧰 Tools
🪛 Biome (1.9.4)
[error] 61-62: A global variable should not be reassigned.
Assigning to a global variable can override essential functionality.
(lint/suspicious/noGlobalAssign)
src/contexts/StateContextProvider.tsx (1)
287-294: Key fix: Added continuous log chunk processing.This change is the core improvement that addresses the service worker clogging issue. Now the worker will continue to request more log chunks until the process is complete, rather than potentially stalling.
However, consider adding an additional null check instead of relying on the non-null assertion:
} else { - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - workerPostReq(mainWorkerRef.current!, WORKER_REQ_CODE.EXPORT_LOGS, { + if (mainWorkerRef.current) { + workerPostReq(mainWorkerRef.current, WORKER_REQ_CODE.EXPORT_LOGS, { decodedEventIdx: args.nextDecodedEventIdx, - }); + }); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/components/MenuBar/ExportLogsButton.tsx(2 hunks)src/contexts/StateContextProvider.tsx(3 hunks)src/services/LogExportManager.ts(3 hunks)src/services/LogFileManager/index.ts(2 hunks)src/services/MainWorker.ts(1 hunks)src/typings/states.ts(1 hunks)src/typings/worker.ts(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/services/LogExportManager.ts
- src/components/MenuBar/ExportLogsButton.tsx
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ...
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
src/typings/states.tssrc/services/MainWorker.tssrc/services/LogFileManager/index.tssrc/typings/worker.tssrc/contexts/StateContextProvider.tsx
🪛 Biome (1.9.4)
src/services/MainWorker.ts
[error] 61-62: A global variable should not be reassigned.
Assigning to a global variable can override essential functionality.
(lint/suspicious/noGlobalAssign)
🔇 Additional comments (11)
src/typings/states.ts (1)
97-97: Enabling navigation during slow loading is a good improvement.This change allows users to access the navigation bar during slow loading operations, which is particularly important during log exports. Users can now navigate away from the page while an export is in progress, preventing the UI from becoming unresponsive.
src/services/LogFileManager/index.ts (2)
198-198: Simplified method signature improves clarity.The return type was changed from an object to a string, which better represents the actual value being returned and simplifies the API.
214-214: Direct string return matches the updated method signature.This change improves code clarity by directly returning the concatenated string instead of wrapping it in an object.
src/typings/worker.ts (3)
82-82: Consistent naming with 'EXPORT_LOGS' instead of 'EXPORT_LOG'.The plural form is more accurate since the function exports multiple logs, and this matches the naming pattern used elsewhere in the code.
99-101: Added necessary parameter for export logs request.The request now properly includes the
decodedEventIdxparameter needed to track export progress.
125-125: Added next event index to improve export flow control.Including
nextDecodedEventIdxin the response allows for proper continuation of the export process without having to recalculate this value on the client side.src/services/MainWorker.ts (2)
68-68: Updated case statement to match renamed request code.This change ensures consistency with the updated enum in worker.ts.
73-80: Simplified log export response handling.The implementation has been streamlined by:
- Directly accessing the
decodedEventIdxfrom args- Using a single
postRespcall instead of a loop- Including the next index position for proper progress tracking
This refactoring likely addresses the service worker clogging issue by making the export process more efficient and responsive.
src/contexts/StateContextProvider.tsx (3)
14-15: LGTM: Consistent renaming of constants to plural form.The constants have been renamed from
EXPORT_LOG_PROGRESS_VALUE_MAX/MINtoEXPORT_LOGS_PROGRESS_VALUE_MAX/MINwhich brings better consistency with the rest of the codebase, such as the existingEXPORT_LOGS_CHUNK_SIZEconstant.
393-393: LGTM: Updated to use the renamed constant.The export progress is now correctly set using the renamed constant
EXPORT_LOGS_PROGRESS_VALUE_MIN.
400-402: LGTM: Worker request updated to new export logs API.The export request has been properly updated to use the new
EXPORT_LOGSworker request code (replacingEXPORT_LOG) and includes the necessary startingdecodedEventIdxparameter.
|
Took a look at a high level, had a couple questions. I think it overall idea looks good
|
Good point. Let me try that.
AFAIK this is not a problem. The major time cost is internal exporting process, and if a user decides to open a new file, that worker will be dropped. You can verify that. |
I see Anyways still consider moving to ready state, since in fast loading we can't switch pages |
It can. It just freezes the query input box. But anyway we should use the ready state. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/contexts/StateContextProvider.tsx(3 hunks)src/services/MainWorker.ts(2 hunks)src/typings/worker.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/typings/worker.ts
- src/contexts/StateContextProvider.tsx
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ...
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
src/services/MainWorker.ts
🪛 Biome (1.9.4)
src/services/MainWorker.ts
[error] 85-86: A global variable should not be reassigned.
Assigning to a global variable can override essential functionality.
(lint/suspicious/noGlobalAssign)
🪛 GitHub Check: lint-check
src/services/MainWorker.ts
[failure] 60-60:
Missing JSDoc @throws declaration
🪛 GitHub Actions: lint
src/services/MainWorker.ts
[warning] 60-60: ESLint found too many warnings (maximum: 0). Missing JSDoc @throws declaration (jsdoc/require-throws)
🔇 Additional comments (5)
src/services/MainWorker.ts (5)
15-15: Good addition of the defer utility.Adding the defer utility from the time utils is a solid choice here. This will help prevent the service worker from blocking during log exports, which directly addresses the issue described in the PR.
66-68: Correct null check pattern.Good job using
null === LOG_FILE_MANAGERinstead of!LOG_FILE_MANAGER. This follows the coding guidelines that preferfalse == <expression>rather than!<expression>.
78-80: Smart use of defer for asynchronous progression.Using defer to schedule the next export chunk is excellent. This non-blocking approach ensures the service worker remains responsive during large exports, preventing UI freeze and allowing users to navigate between pages as mentioned in the PR testing steps.
85-85: ESLint directive improvement.Good change to only disable the max-lines-per-function rule while enabling other rules. This helps maintain better code quality while acknowledging that this function needs to be longer than usual.
🧰 Tools
🪛 Biome (1.9.4)
[error] 85-86: A global variable should not be reassigned.
Assigning to a global variable can override essential functionality.
(lint/suspicious/noGlobalAssign)
92-94: Successfully updated export logs implementation.The case statement has been properly renamed from EXPORT_LOG to EXPORT_LOGS, matching updates in the worker typings. The implementation has been simplified by delegating to the new helper function, which should resolve the service worker clogging issue.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/services/LogFileManager/index.ts (1)
216-216: JSDoc update needed for exportChunkAndScheduleNext methodThe JSDoc comment still references "loads log events" rather than accurately describing that it exports logs.
- * Loads log events in the range and sends them to the renderer. The range is defined as + * Exports log events in the range and sends them to the renderer. The range is defined as
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/services/LogFileManager/index.ts(8 hunks)src/services/MainWorker.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/services/LogFileManager/index.tssrc/services/MainWorker.ts
🧬 Code Definitions (1)
src/services/MainWorker.ts (1)
src/typings/worker.ts (3) (3)
WORKER_RESP_CODE(188:188)MainWorkerReqMessage(195:195)WORKER_REQ_CODE(187:187)
🪛 Biome (1.9.4)
src/services/MainWorker.ts
[error] 69-70: A global variable should not be reassigned.
Assigning to a global variable can override essential functionality.
(lint/suspicious/noGlobalAssign)
🔇 Additional comments (16)
src/services/MainWorker.ts (5)
50-57: Well-structured callback function for exporting logsThis new callback function is well designed for handling log exports. It encapsulates the responsibility of posting responses with exported logs, following the single responsibility principle.
69-69: ESLint directive update improves code quality controlThe ESLint directive has been updated to only disable
max-lines-per-functionwhile removing themax-statementsrule. This is a good practice as it maintains better control over the function complexity.🧰 Tools
🪛 Biome (1.9.4)
[error] 69-70: A global variable should not be reassigned.
Assigning to a global variable can override essential functionality.
(lint/suspicious/noGlobalAssign)
76-76: Request code properly renamed to match type definitionThe request code has been updated from
EXPORT_LOGtoEXPORT_LOGS, which aligns with the changes in the worker type definitions shown in the relevant code snippets.
80-80: Improved log export implementationThe code now delegates the log export functionality to the LogFileManager, which enhances modularity and separation of concerns. This should resolve the service worker clogging issue mentioned in the PR objectives.
84-90: Constructor parameters updated with onExportChunk callbackThe LogFileManager creation now includes the onExportChunk callback, which completes the implementation of the new export functionality. The object parameter style makes the code more maintainable for future parameter additions.
src/services/LogFileManager/index.ts (11)
40-47: Well-structured interface for constructor parametersThis new interface clearly defines the parameters needed for the LogFileManager constructor, making the code more maintainable and self-documenting. The inclusion of the
onExportChunkcallback parameter supports the new export functionality.
49-55: Clean interface for create method parametersThis interface nicely separates the parameters for the static create method from the constructor parameters, following good object-oriented design principles. The parameters are well organized and documented.
73-73: Private member for export chunk callbackThe addition of this private member variable properly encapsulates the callback function for exporting log chunks, maintaining good encapsulation and information hiding.
93-100: Updated constructor with proper parameter destructuringThe constructor now uses object destructuring for its parameters, which makes the code more readable and maintainable. The parameter list matches the defined interface.
105-105: Initialization of onExportChunk callbackThis line properly initializes the private onExportChunk member with the callback provided in the constructor parameters.
142-148: Static create method with updated parametersThe create method uses object destructuring for its parameters, consistent with the constructor. The addition of onExportChunk parameter allows for proper callback functionality.
158-158: Proper callback passing to constructorThe onExportChunk callback is correctly passed to the constructor, ensuring the export functionality works correctly throughout the component lifecycle.
223-223: Method renamed to better reflect its functionalityThe method name has been updated from
loadChunktoexportChunkAndScheduleNext, which better describes its purpose and behavior. This improved naming makes the code more self-documenting.
238-238: Improved handling of exported logsInstead of returning an object with logs, the method now directly calls the onExportChunk callback with the joined log messages. This is more efficient and aligns with the callback pattern used throughout the codebase.
240-244: Non-blocking chunk export with deferThis implementation uses defer to schedule the next chunk export without blocking the main thread. This is a key improvement that should resolve the service worker clogging issue mentioned in the PR objectives.
413-450: Well-structured helper method for query processingThe #processQueryDecodedEvents method has been extracted to improve code organization. This method encapsulates the logic for processing decoded events and populating query results, following the single responsibility principle.
Henry8192
changed the title
Description
Fixes #92.
Checklist
breaking change.
Validation performed
Summary by CodeRabbit