feat: Migrate utilities from UrlContextProvider to Zustand stores (resolves #269; fixes #294; resolves #306).#286
Conversation
## Walkthrough
This change removes the React `UrlContextProvider` and its context-based URL parameter management, migrating all URL parameter handling to Zustand stores and a new `utils/url` module. Components and stores are refactored to use direct store access and utility functions for synchronizing URL state, and related context/store modules are deleted.
## Changes
| File(s) | Change Summary |
|------------------------------------------------------------------------------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| src/App.tsx | Removed `UrlContextProvider` import and usage; now directly nests `AppController` and `Layout`. |
| src/components/AppController.tsx | Removed all React context and context store usage for URL parameters; added direct Zustand store access and new utility functions for URL hash/query param updates; added hashchange event listener; streamlined state sync logic. |
| src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/Result.tsx | Switched import of `updateWindowUrlHashParams` to `utils/url`; now uses `useViewStore` for state update; memoized click handler. |
| src/components/Editor/index.tsx | Removed `UrlContext` usage; now uses `useViewStore` for state; refactored `handleEditorCustomAction` as a standalone function; unified state and URL sync logic. |
| src/components/StatusBar/index.tsx | Replaced `UrlContext` with Zustand store hooks for prettify state; updated toggle logic to use new store and URL utilities. |
| src/contexts/UrlContextProvider.tsx | Entire file deleted; removed all exported context, provider, constants, and utility functions for URL parameter management. |
| src/stores/contextStore.ts | Entire file deleted; removed Zustand store for `logEventNum` and all related types and exports. |
| src/stores/logFileStore.ts | Changed import of `updateWindowUrlSearchParams` to `utils/url`; now calls `startQuery` after loading page data; added ESLint directive. |
| src/stores/viewStore.ts | Added `logEventNum` state and setter to store; removed dependency on context store; centralized `logEventNum` handling and URL sync in view store; updated related methods. |
| src/utils/url.ts | Added comprehensive URL parameter parsing, updating, and clipboard utilities; exports new constants and functions for search/hash param management. |
| src/typings/worker.ts | Changed `PageData.logEventNum` from nullable to non-nullable number; updated default constant; removed unused import. |
| src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx | Changed import of `copyPermalinkToClipboard` to `utils/url`; now uses store's current state for query values and setters; removed dependencies array from share button callback. |
| src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/QueryInputBox.tsx | Refactored event handlers to use `useCallback`; handlers update URL hash, then store state, then trigger query; unified store access pattern. |
| src/typings/url.ts | Removed exported type alias `UrlParamsType`; no other changes to types or exports. |
| src/typings/common.ts | Removed type alias `NullableProperties<T>` and its export; no other changes. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Window
participant AppController
participant ZustandStores
participant UrlUtils
Window->>AppController: hashchange event
AppController->>UrlUtils: parseWindowUrlHashParams()
UrlUtils-->>AppController: hash params object
AppController->>ZustandStores: update view/query state from hash params
AppController->>ZustandStores: trigger query if query params changed
AppController->>UrlUtils: updateWindowUrlHashParams() (if needed)
UrlUtils-->>Window: update browser URL hashPossibly related issues
Possibly related PRs
Suggested reviewers
|
|
@zzxthehappiest feel free to use #293 as a reference, where we replaced the NotificationConextProvider with a Zustand store |
|
For lint I added this in And modified the limitation in Because I found I couldn't subscribe multiple states stored in Zustandard within in one statement and each one needs to be a independent statement. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
src/stores/viewStore.ts (1)
22-22: 🧹 Nitpick (assertive)Remove the
.tsextension from the import statement.-import {updateWindowUrlHashParams} from "../utils/url.ts"; +import {updateWindowUrlHashParams} from "../utils/url";src/components/AppController.tsx (1)
26-32: 🧹 Nitpick (assertive)Remove the
.tsextension from the import statement.-} from "../utils/url.ts"; +} from "../utils/url";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
src/App.tsx(1 hunks)src/components/AppController.tsx(8 hunks)src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/Result.tsx(2 hunks)src/components/Editor/index.tsx(6 hunks)src/components/StatusBar/index.tsx(4 hunks)src/contexts/UrlContextProvider.tsx(0 hunks)src/stores/contextStore.ts(0 hunks)src/stores/logFileStore.ts(4 hunks)src/stores/viewStore.ts(8 hunks)src/utils/url.ts(3 hunks)
💤 Files with no reviewable changes (2)
- src/stores/contextStore.ts
- src/contexts/UrlContextProvider.tsx
🧰 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/App.tsxsrc/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/Result.tsxsrc/stores/logFileStore.tssrc/components/StatusBar/index.tsxsrc/components/Editor/index.tsxsrc/stores/viewStore.tssrc/components/AppController.tsxsrc/utils/url.ts
🧠 Learnings (1)
src/stores/viewStore.ts (2)
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#286
File: src/stores/viewStore.ts:46-46
Timestamp: 2025-05-21T20:08:19.249Z
Learning: For Zustand store actions, use `setX` for functions that simply change the value of a state, and use `updateX` for functions that do more than just set a value (e.g., perform additional logic, make API calls, update multiple states).
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#286
File: src/stores/viewStore.ts:46-46
Timestamp: 2025-05-21T20:08:19.249Z
Learning: For Zustand store actions, use `setX` for functions that simply change the value of a state, and use `updateX` for functions that do more than just set a value (e.g., perform additional logic, make API calls, update multiple states).
🧬 Code Graph Analysis (4)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/Result.tsx (1)
src/utils/url.ts (1)
updateWindowUrlHashParams(284-284)
src/components/Editor/index.tsx (5)
src/typings/worker.ts (1)
BeginLineNumToLogEventNumMap(105-105)src/typings/common.ts (1)
Nullable(10-10)src/utils/actions.ts (1)
ACTION_NAME(107-107)src/utils/url.ts (1)
updateWindowUrlHashParams(284-284)src/typings/url.ts (1)
HASH_PARAM_NAMES(37-37)
src/stores/viewStore.ts (3)
src/typings/worker.ts (1)
BeginLineNumToLogEventNumMap(105-105)src/utils/actions.ts (1)
ACTION_NAME(107-107)src/utils/url.ts (1)
updateWindowUrlHashParams(284-284)
src/utils/url.ts (2)
src/typings/url.ts (6)
SEARCH_PARAM_NAMES(38-38)HASH_PARAM_NAMES(37-37)UrlSearchParamUpdatesType(45-45)UrlHashParamUpdatesType(42-42)UrlHashParams(41-41)UrlSearchParams(44-44)src/typings/common.ts (1)
NullableProperties(11-11)
🔇 Additional comments (21)
src/App.tsx (1)
14-16: LGTM! Clean removal of UrlContextProvider wrapper.The component tree simplification correctly reflects the migration from UrlContextProvider to Zustand stores. The nesting structure remains logical with NotificationContextProvider directly wrapping AppController and Layout.
src/stores/logFileStore.ts (2)
25-25: LGTM! Correct import path migration.The import path change from the deleted UrlContextProvider to the new utils/url.ts module correctly reflects the migration of URL parameter utilities.
43-43: LGTM! Clean addition of setFileSrc setter.The new setter method follows standard Zustand patterns and provides the necessary functionality for managing fileSrc state in the migration from context-based state management.
Also applies to: 160-162
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/Result.tsx (2)
6-7: LGTM! Correct import migration to Zustand stores.The imports correctly reflect the migration from UrlContextProvider to useViewStore and the new utils/url.ts module for URL parameter utilities.
40-40: LGTM! Proper state synchronization between store and URL.The implementation correctly updates both the URL hash parameters and the view store state, ensuring proper synchronization between URL and internal state management.
Also applies to: 43-43
src/components/StatusBar/index.tsx (2)
13-13: LGTM! Correct migration to view store and URL utilities.The imports correctly reflect the migration from context-based state management to Zustand view store and the new URL utility module.
Also applies to: 18-21
43-45: LGTM! Proper state management migration.The component correctly obtains state and setters from the view store and maintains synchronization between URL parameters and store state.
Also applies to: 55-55
src/components/Editor/index.tsx (3)
138-144: LGTM! State management correctly migrated to Zustand stores.The migration from context to
useViewStoreforisPrettifiedandlogEventNumstate management aligns perfectly with the PR objectives.
182-189: LGTM! Prettify toggle correctly updates both URL and store.The implementation properly synchronizes the URL hash parameters with the store state, which is the correct pattern for this migration.
264-266: LGTM! Cursor position change correctly syncs URL and store.The implementation properly updates both the URL hash parameters and the store state when the cursor position changes.
src/stores/viewStore.ts (4)
34-34: LGTM! logEventNum state properly added to the store.The state property, setter method, and default value are correctly defined following the store structure.
Also applies to: 42-42, 58-58
201-203: LGTM! Setter correctly follows naming conventions.The
setLogEventNumimplementation correctly uses the 'set' prefix for a simple state setter, following the established Zustand naming conventions mentioned in the learnings.
115-117: LGTM! State access correctly migrated.The method now properly retrieves
logEventNumfrom the local store state while maintainingpostPopUpfrom the context store, which appears to be intentional.
242-256: LGTM! Page data update correctly synchronizes state.The implementation properly extracts
logEventNumfrom the page data and synchronizes it with both the URL hash parameters and the store state, with appropriate null checking.src/components/AppController.tsx (4)
46-49: LGTM! Enhanced return type improves clarity.The updated return type provides more detailed information about the operation result, making the function's behavior more explicit.
Also applies to: 70-70
89-102: LGTM! State management correctly migrated to stores.All state and actions are properly migrated from context to Zustand stores, maintaining consistency with the PR objectives.
111-141: Clarify the search params update logic in the hashchange handler.The comment at lines 122-125 suggests uncertainty about updating search params when hash params change. This mixing of concerns could lead to unexpected behavior.
Consider separating the hash and search parameter handling into distinct event handlers or clarifying why they need to be coupled. The current implementation might cause unintended side effects when only hash parameters change.
169-176: LGTM! Properly uses the enhanced return value.The code correctly destructures and uses the enhanced return value from
updateUrlIfEventOnPage.src/utils/url.ts (3)
41-84: Well-implemented complex URL parameter handling.The
getUpdatedSearchParamsfunction demonstrates excellent attention to detail:
- Correct parameter ordering: Ensures
filePathis always last to handle URLs within URLs- Proper null handling: Removes parameters when values are null
- Ambiguous character detection: Prevents URL parsing issues with smart decoding
- Clear documentation: Well-commented complex logic
The implementation correctly handles edge cases like remote URLs containing their own parameters.
184-203: Robust parameter parsing with proper type safety.The hash parameter parsing correctly:
- Uses NullableProperties type: Properly handles the case where parameters might not be present
- Safe number parsing: Checks for NaN and returns null for invalid numbers
- Boolean parsing: Correctly converts string "true" to boolean
- Defensive defaults: Uses structuredClone to avoid mutation of default values
277-288: 🧹 Nitpick (assertive)Consider organizing exports alphabetically.
For better maintainability, consider organizing the exports alphabetically, especially as this utility module grows.
export { copyPermalinkToClipboard, getAbsoluteUrl, getBasenameFromUrlOrDefault, getWindowUrlHashParams, getWindowUrlSearchParams, openInNewTab, updateWindowUrlHashParams, updateWindowUrlSearchParams, URL_HASH_PARAMS_DEFAULT, URL_SEARCH_PARAMS_DEFAULT, };The exports are already well-organized, but completely alphabetical ordering would be:
export { + copyPermalinkToClipboard, + getAbsoluteUrl, + getBasenameFromUrlOrDefault, + getWindowUrlHashParams, + getWindowUrlSearchParams, + openInNewTab, + updateWindowUrlHashParams, + updateWindowUrlSearchParams, + URL_HASH_PARAMS_DEFAULT, + URL_SEARCH_PARAMS_DEFAULT, - copyPermalinkToClipboard, - getAbsoluteUrl, - getBasenameFromUrlOrDefault, - getWindowUrlHashParams, - getWindowUrlSearchParams, - openInNewTab, - updateWindowUrlHashParams, - updateWindowUrlSearchParams, - URL_HASH_PARAMS_DEFAULT, - URL_SEARCH_PARAMS_DEFAULT, };Likely an incorrect or invalid review comment.
|
I think if we add unit tests for |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
src/components/StatusBar/index.tsx (1)
55-55: Follow coding guideline for boolean expressions.The code uses
!isPrettifiedwhich violates the coding guideline that prefersfalse == <expression>rather than!<expression>.Apply this diff to align with the coding guideline:
- setIsPrettified(!isPrettified); + setIsPrettified(false === isPrettified);src/components/Editor/index.tsx (1)
35-35: Remove the.tsextension from the import statement.TypeScript imports typically don't include file extensions. This is also inconsistent with other imports in the file.
-import {updateWindowUrlHashParams} from "../../utils/url.ts"; +import {updateWindowUrlHashParams} from "../../utils/url";src/utils/url.ts (2)
246-250: 🧹 Nitpick (assertive)Clarify the comment about Zustand store updates.
The comment should be clearer about the separation of responsibilities between URL updates and state synchronization.
- * Note: This function only updates the URL. Callers are responsible for updating corresponding - * Zustand store state to maintain synchronization. + * Note: This function only updates the URL. Callers are responsible for updating + * corresponding Zustand store state to maintain synchronization.
267-271: 🧹 Nitpick (assertive)Clarify the comment about Zustand store updates.
Same issue as the hash params function - the comment should be clearer.
- * Note: This function only updates the URL. Callers are responsible for updating corresponding - * Zustand store state to maintain synchronization. + * Note: This function only updates the URL. Callers are responsible for updating + * corresponding Zustand store state to maintain synchronization.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
src/App.tsx(1 hunks)src/components/AppController.tsx(8 hunks)src/components/Editor/index.tsx(6 hunks)src/components/StatusBar/index.tsx(3 hunks)src/contexts/UrlContextProvider.tsx(0 hunks)src/stores/contextStore.ts(0 hunks)src/stores/logFileStore.ts(4 hunks)src/stores/viewStore.ts(8 hunks)src/utils/url.ts(3 hunks)
💤 Files with no reviewable changes (2)
- src/contexts/UrlContextProvider.tsx
- src/stores/contextStore.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/App.tsxsrc/stores/logFileStore.tssrc/components/StatusBar/index.tsxsrc/components/Editor/index.tsxsrc/components/AppController.tsxsrc/utils/url.tssrc/stores/viewStore.ts
🧠 Learnings (2)
src/utils/url.ts (1)
Learnt from: zzxthehappiest
PR: y-scope/yscope-log-viewer#286
File: src/utils/url.ts:46-46
Timestamp: 2025-05-25T02:45:44.343Z
Learning: In TypeScript, when comparing enum values with strings (like keys from Object.entries()), the type assertion `as string` is necessary to avoid the @typescript-eslint/no-unsafe-enum-comparison lint error. This is a valid use of type assertion for enum-to-string comparison.
src/stores/viewStore.ts (2)
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#286
File: src/stores/viewStore.ts:46-46
Timestamp: 2025-05-21T20:08:19.249Z
Learning: For Zustand store actions, use `setX` for functions that simply change the value of a state, and use `updateX` for functions that do more than just set a value (e.g., perform additional logic, make API calls, update multiple states).
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#286
File: src/stores/viewStore.ts:46-46
Timestamp: 2025-05-21T20:08:19.249Z
Learning: For Zustand store actions, use `setX` for functions that simply change the value of a state, and use `updateX` for functions that do more than just set a value (e.g., perform additional logic, make API calls, update multiple states).
🧬 Code Graph Analysis (3)
src/stores/logFileStore.ts (2)
src/typings/common.ts (1)
Nullable(10-10)src/typings/worker.ts (1)
FileSrcType(108-108)
src/components/Editor/index.tsx (5)
src/typings/worker.ts (1)
BeginLineNumToLogEventNumMap(105-105)src/typings/common.ts (1)
Nullable(10-10)src/utils/actions.ts (1)
ACTION_NAME(107-107)src/utils/url.ts (1)
updateWindowUrlHashParams(288-288)src/typings/url.ts (1)
HASH_PARAM_NAMES(37-37)
src/utils/url.ts (2)
src/typings/url.ts (6)
SEARCH_PARAM_NAMES(38-38)HASH_PARAM_NAMES(37-37)UrlSearchParamUpdatesType(45-45)UrlHashParamUpdatesType(42-42)UrlHashParams(41-41)UrlSearchParams(44-44)src/typings/common.ts (1)
NullableProperties(11-11)
🔇 Additional comments (6)
src/App.tsx (1)
12-14: LGTM! Clean removal of context provider wrapper.The removal of
UrlContextProviderwrapper correctly reflects the migration to Zustand stores for URL parameter management. The simplified structure whereAppControllerdirectly containsLayoutis appropriate.src/stores/logFileStore.ts (1)
42-42: LGTM! Proper addition of setFileSrc method.The new
setFileSrcmethod is correctly typed in the interface and properly implemented in the store. This supports the migration by allowing direct store-based updates of the file source state.Also applies to: 159-161
src/components/StatusBar/index.tsx (1)
43-45: LGTM! Proper migration to Zustand store.The component correctly migrates from React context to
useViewStorehooks for managingisPrettifiedandlogEventNumstate. The store-based approach provides better performance and cleaner state management.src/components/Editor/index.tsx (1)
138-144: LGTM! Excellent migration to Zustand store architecture.The component successfully migrates from React context to
useViewStorehooks for state management. The integration maintains all existing functionality while providing better performance and cleaner separation of concerns. The callback dependencies are correctly updated to reflect the new store methods.Also applies to: 196-197, 265-266
src/stores/viewStore.ts (1)
232-237: Good implementation of URL and state synchronization.The order of operations (updating URL first, then store state) ensures proper synchronization between the URL parameters and the internal store state.
src/components/AppController.tsx (1)
39-67: Good improvement to the return type.Returning both
isUpdatedandnearestLogEventNumprovides more useful information to the caller, allowing for better decision-making in the consuming code.
junhaoliao
left a comment
junhaoliao
left a comment
There was a problem hiding this comment.
Thanks for quickly putting up the changes.
Could you also help integrating the changes in junhaoliao@b972113 into the current PR? Those will fix #294
(my fix is to align the existing parameters parsing logic. in my changes, we might also need to make sure the parsed values are of string type before calling new URLSearchParams(). That said, maybe the code should be completely rewritten - the for-loops can go through the currentParams objects to insert values into an empty new URLSearchParams(). if you think there's a better way to fix #294, feel free to make your own changes
I modified the |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/utils/js.ts(2 hunks)src/utils/url.ts(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/utils/js.tssrc/utils/url.ts
🧠 Learnings (1)
src/utils/url.ts (1)
Learnt from: zzxthehappiest
PR: y-scope/yscope-log-viewer#286
File: src/utils/url.ts:46-46
Timestamp: 2025-05-25T02:45:44.362Z
Learning: In TypeScript, when comparing enum values with strings (like keys from Object.entries()), the type assertion `as string` is necessary to avoid the @typescript-eslint/no-unsafe-enum-comparison lint error. This is a valid use of type assertion for enum-to-string comparison.
🔇 Additional comments (5)
src/utils/url.ts (5)
16-32: Well-structured constants with proper immutability.Good use of
Object.freezeto ensure the default values remain immutable, and the regex pattern correctly handles ambiguous URL characters.
59-78: Robust parameter parsing with proper type handling.The function correctly handles type conversions and edge cases, including NaN checks for numeric values and proper boolean parsing.
85-109: Appropriate handling of special filePath parameter.The implementation correctly handles the case where
filePathmay contain its own URL parameters by using string manipulation instead ofURLSearchParams.get(). Good error handling for URL resolution.
119-163: Well-designed parameter update logic with clear documentation.The implementation correctly handles the special requirements for
filePathparameter ordering and includes appropriate type assertions for enum comparisons (as per established patterns). The comments clearly explain the rationale for keepingfilePathas the last parameter.
198-212: Clean implementation with proper error handling.Good use of the Clipboard API with appropriate error handling. The function correctly creates a modified URL without affecting the current window location.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/utils/url.ts(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/utils/url.ts
🧠 Learnings (1)
src/utils/url.ts (1)
Learnt from: zzxthehappiest
PR: y-scope/yscope-log-viewer#286
File: src/utils/url.ts:46-46
Timestamp: 2025-05-25T02:45:44.362Z
Learning: In TypeScript, when comparing enum values with strings (like keys from Object.entries()), the type assertion `as string` is necessary to avoid the @typescript-eslint/no-unsafe-enum-comparison lint error. This is a valid use of type assertion for enum-to-string comparison.
🔇 Additional comments (5)
src/utils/url.ts (5)
14-28: Well-structured constants with proper immutability.Good use of
Object.freezeto ensure the default values remain immutable. The constants are properly typed and provide clear default values.
86-110: Robust implementation for extracting filePath parameter.Good implementation that correctly handles the filePath parameter extraction and URL resolution. The error handling for invalid URLs is appropriate.
120-164: Excellent implementation with clear documentation.The function properly handles the special case of
filePathparameter, ensuring it remains last for URL readability and parameter handling. The URL decoding logic is clever and well-thought-out.
259-269: Well-optimized implementation with clear documentation.Good optimization to prevent unnecessary history entries by checking if the hash actually changed. The comment clearly explains the caller's responsibility for maintaining store synchronization.
280-289: Consistent implementation with hash parameter updates.Good consistency with
updateWindowUrlHashParamsincluding the optimization check and clear documentation about store synchronization responsibility.
|
@junhaoliao Sorry for the delay. I am on a business since last week and get home late recently. I fixed most of comments except this one. I found a behaviour that when clicking the "Prettify" button, there will be a I will definitely finish all things by tomorrow. I need to take a look of the changed code again to ensure all of them apply the comments. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (18)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx (2)
20-20: Remove the .ts extension from the import path.This was previously flagged and agreed upon but still remains unaddressed.
-import {copyPermalinkToClipboard} from "../../../../../utils/url.ts"; +import {copyPermalinkToClipboard} from "../../../../../utils/url";
53-55: Remove redundant state setter calls.These setter calls are redundant as they're setting the same values that are already in the store. This was previously identified but not addressed.
- setQueryIsCaseSensitive(queryIsCaseSensitive); - setQueryIsRegex(queryIsRegex); - setQueryString(queryString); - copyPermalinkToClipboard({}, {src/components/StatusBar/index.tsx (5)
1-1: LGTM! React import added as requested.
15-15: LGTM! Store import updated correctly.
20-23: LGTM! URL utility imports updated correctly.
55-57: Good adherence to coding guidelines and proper state synchronization.The code correctly uses
false === isPrettifiedto follow the coding guideline and properly callsupdateIsPrettifiedto synchronize state.
95-95: Clean implementation of boolean expressions.The props correctly use the coding guideline-compliant boolean expressions with
false === isPrettified.Also applies to: 100-102
src/components/Editor/index.tsx (2)
127-177: Excellent extraction of editor action handler.The standalone
handleEditorCustomActionfunction is well-structured and properly usesgetState()to access store state directly. This follows the suggested pattern from past reviews and improves code organization.
267-268: Optimal use of getState() for static setter.Correctly uses
getState()to accesssetLogEventNumas it's a static setter that never changes, avoiding unnecessary subscriptions. This follows the established pattern from the learnings.src/stores/viewStore.ts (2)
18-18: Proper import of URL utility function.The import correctly references the new URL utilities module.
185-187: Correct naming convention for simple setter.The
setLogEventNumfunction properly follows the established naming convention for simple setters that only change state values.src/components/AppController.tsx (5)
74-141: Excellent separation of concerns with helper functions.The three helper functions (
updateViewHashParams,updateQueryHashParams, andhandleHashChange) provide clean separation of concerns and make the URL parameter handling much more maintainable. The design properly handles query restarts when parameters change and returns parsed parameters for further use.
156-156: Clean state access pattern.Only importing the necessary
logEventNumstate follows the suggested pattern from past reviews, reducing unnecessary subscriptions.
159-159: Well-structured initialization and event handling.The use of
isInitializedref properly handles React StrictMode double execution, and the main effect correctly registers the hash change handler while handling initial page load. The cleanup function properly removes the event listener.Also applies to: 162-190
194-222: Optimal state retrieval pattern.The effect correctly retrieves Zustand states just before usage to avoid circular dependencies, and the dependency array properly includes only
logEventNumas suggested in past reviews. The logic for clamping and page switching is clear and correct.
15-15: 🧹 Nitpick (assertive)Remove the .ts extension from the import path.
The explicit
.tsextension should be removed to follow TypeScript conventions.-} from "../typings/common.ts"; +} from "../typings/common";src/utils/url.ts (2)
166-166: Type assertion is correctly maintained for enum comparison.Based on the retrieved learning, the type assertion
as stringis necessary to avoid the@typescript-eslint/no-unsafe-enum-comparisonlint error when comparing enum values with strings fromObject.entries().
241-255: 🧹 Nitpick (assertive)Consider using notification system for user feedback.
The console logging approach for clipboard operations may not provide adequate user feedback in a production environment.
Since this PR migrates to Zustand stores, consider integrating with a notification store for better user experience:
const copyPermalinkToClipboard = ( searchParamUpdates: UrlSearchParamUpdatesType, hashParamsUpdates: UrlHashParamUpdatesType, ) => { const newUrl = new URL(window.location.href); newUrl.search = getUpdatedSearchParams(searchParamUpdates); newUrl.hash = getUpdatedHashParams(hashParamsUpdates); navigator.clipboard.writeText(newUrl.toString()) .then(() => { - console.log("URL copied to clipboard."); + // TODO: Integrate with notification store for user feedback + console.log("URL copied to clipboard."); }) .catch((error: unknown) => { - console.error("Failed to copy URL to clipboard:", error); + // TODO: Integrate with notification store for error feedback + console.error("Failed to copy URL to clipboard:", error); }); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
src/components/AppController.tsx(4 hunks)src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/QueryInputBox.tsx(3 hunks)src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/Result.tsx(2 hunks)src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx(2 hunks)src/components/Editor/index.tsx(4 hunks)src/components/StatusBar/index.tsx(4 hunks)src/stores/logFileStore.ts(3 hunks)src/stores/viewStore.ts(8 hunks)src/typings/url.ts(0 hunks)src/utils/url.ts(3 hunks)
💤 Files with no reviewable changes (1)
- src/typings/url.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/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/QueryInputBox.tsxsrc/components/StatusBar/index.tsxsrc/stores/viewStore.tssrc/components/AppController.tsxsrc/utils/url.tssrc/stores/logFileStore.tssrc/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/Result.tsxsrc/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsxsrc/components/Editor/index.tsx
🧠 Learnings (5)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/QueryInputBox.tsx (1)
Learnt from: zzxthehappiest
PR: y-scope/yscope-log-viewer#286
File: src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/QueryInputBox.tsx:37-40
Timestamp: 2025-06-01T13:41:12.938Z
Learning: The `updateWindowUrlHashParams` function in `src/utils/url.ts` doesn't throw errors, so error handling is not needed when calling this function.
src/stores/viewStore.ts (2)
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#286
File: src/stores/viewStore.ts:46-46
Timestamp: 2025-05-21T20:08:19.249Z
Learning: For Zustand store actions, use `setX` for functions that simply change the value of a state, and use `updateX` for functions that do more than just set a value (e.g., perform additional logic, make API calls, update multiple states).
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#286
File: src/stores/viewStore.ts:46-46
Timestamp: 2025-05-21T20:08:19.249Z
Learning: For Zustand store actions, use `setX` for functions that simply change the value of a state, and use `updateX` for functions that do more than just set a value (e.g., perform additional logic, make API calls, update multiple states).
src/components/AppController.tsx (3)
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.
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#265
File: src/stores/logFileStore.ts:125-125
Timestamp: 2025-05-17T05:13:39.947Z
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.
Learnt from: zzxthehappiest
PR: y-scope/yscope-log-viewer#286
File: src/components/AppController.tsx:40-43
Timestamp: 2025-06-01T13:44:05.278Z
Learning: In the y-scope/yscope-log-viewer codebase, when functions return invalid or default log event numbers, they should use 0 as this aligns with URL_HASH_PARAMS_DEFAULT.LOG_EVENT_NUM which is set to 0. This maintains consistency with the established default hash parameter pattern.
src/utils/url.ts (1)
Learnt from: zzxthehappiest
PR: y-scope/yscope-log-viewer#286
File: src/utils/url.ts:46-46
Timestamp: 2025-05-25T02:45:44.362Z
Learning: In TypeScript, when comparing enum values with strings (like keys from Object.entries()), the type assertion `as string` is necessary to avoid the @typescript-eslint/no-unsafe-enum-comparison lint error. This is a valid use of type assertion for enum-to-string comparison.
src/components/Editor/index.tsx (2)
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#265
File: src/stores/logFileStore.ts:125-125
Timestamp: 2025-05-17T05:13:39.947Z
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.
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.
🧬 Code Graph Analysis (6)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/QueryInputBox.tsx (3)
src/utils/url.ts (1)
updateWindowUrlHashParams(334-334)src/services/LogFileManagerProxy.ts (1)
startQuery(65-68)src/services/LogFileManager/index.ts (1)
startQuery(356-386)
src/stores/viewStore.ts (3)
src/typings/worker.ts (3)
BeginLineNumToLogEventNumMap(104-104)CursorType(106-106)CURSOR_CODE(99-99)src/services/LogFileManager/index.ts (1)
cursor(492-514)src/utils/url.ts (1)
updateWindowUrlHashParams(334-334)
src/utils/url.ts (1)
src/typings/url.ts (6)
SEARCH_PARAM_NAMES(38-38)HASH_PARAM_NAMES(37-37)UrlSearchParams(43-43)UrlHashParams(41-41)UrlSearchParamUpdatesType(44-44)UrlHashParamUpdatesType(42-42)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/Result.tsx (1)
src/utils/url.ts (1)
updateWindowUrlHashParams(334-334)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx (1)
src/utils/url.ts (1)
copyPermalinkToClipboard(328-328)
src/components/Editor/index.tsx (3)
src/utils/actions.ts (1)
ACTION_NAME(107-107)src/utils/url.ts (1)
updateWindowUrlHashParams(334-334)src/typings/url.ts (1)
HASH_PARAM_NAMES(37-37)
🔇 Additional comments (22)
src/stores/logFileStore.ts (3)
1-1: ESLint directive appropriately added for migration.The increased line limit accommodates the Zustand subscription patterns that require more statements compared to the previous context-based approach.
24-24: Import successfully migrated from context to utility module.The migration from
UrlContextProviderto the newutils/urlmodule aligns with the overall refactoring strategy.
143-144: Query restart implementation addresses past feedback.The explicit
startQuery()call ensures queries are actively restarted once a file is loaded, directly implementing the suggestion from the previous review.src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/QueryInputBox.tsx (4)
1-1: useCallback import appropriately added.The addition supports the memoization pattern recommended in previous reviews.
32-38: Excellent implementation of the suggested callback pattern.The handler correctly follows the recommended approach: uses
useCallbackfor memoization, updates URL hash parameters, retrieves store actions viagetState(), and triggers the query.
40-46: Consistent implementation of the callback pattern.The case sensitivity handler follows the same recommended pattern as the query input handler.
48-54: Proper implementation of the regex toggle handler.The handler maintains consistency with the established pattern across all three event handlers.
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/Result.tsx (2)
1-1: Proper imports added for the migration pattern.The addition of
useCallback,useViewStore, andupdateWindowUrlHashParamsimports supports the consistent implementation of the new state management approach.Also applies to: 8-9
43-47: Correct implementation of the callback pattern.The handler properly uses
useCallbackwith the appropriate dependency array and retrieves the store action viagetState(), following the established migration pattern.src/components/StatusBar/index.tsx (1)
45-47: Excellent migration to Zustand stores.The component now properly accesses
isPrettified,logEventNum, andupdateIsPrettifiedfrom the view store, replacing the previous context-based approach.src/components/Editor/index.tsx (2)
33-33: Clean import without file extension.The import follows TypeScript conventions correctly.
191-191: Proper store access pattern.Correctly accesses
logEventNumfrom the view store using the hook pattern for reactive state.src/stores/viewStore.ts (2)
30-30: Excellent consolidation of logEventNum state.The integration of
logEventNuminto the view store properly consolidates state management and follows the migration from the removed context store.Also applies to: 38-38, 54-54
229-234: Well-implemented URL synchronization.The
updatePageDatafunction properly extractslogEventNumfrom page data, updates URL parameters, and synchronizes store state, maintaining consistency between URL and application state.src/utils/url.ts (8)
1-9: LGTM: Clean imports and ESLint configuration.The ESLint max-lines directive and import statements are well-organized and appropriate for the file's scope.
15-28: LGTM: Well-structured default parameter constants.The default parameter objects are properly frozen and use appropriate default values. The structure supports the migration from context-based to Zustand-based state management.
94-118: LGTM: Comprehensive hash parameter parsing.The hash parameter parsing logic correctly handles all expected parameter types with appropriate type conversions and fallbacks.
125-138: LGTM: Clean getter implementations using composition.The getter functions effectively combine default values with parsed parameters using the spread operator, providing a clean API for accessing URL parameters.
146-150: LGTM: Useful utility function for parameter validation.The
isEmptyOrFalsyhelper function provides a centralized way to check for empty or falsy values, promoting code reuse across the module.
214-229: LGTM: Consistent hash parameter update logic.The hash parameter update function follows the same pattern as search parameters with appropriate type handling for the URLSearchParams constructor.
298-325: LGTM: Efficient URL update implementations with change detection.Both update functions correctly implement change detection to prevent unnecessary history entries, which is an important optimization for browser history management.
327-338: LGTM: Comprehensive export interface.The exports provide a clean public API that covers all necessary URL parameter management functionality for the Zustand store migration.
| /** | ||
| * Parses the URL search parameters from the current window's URL. | ||
| * | ||
| * @return An object containing the parsed search parameters. | ||
| */ | ||
| const parseWindowUrlSearchParams = () : Partial<UrlSearchParams> => { | ||
| const parsedSearchParams : Partial<UrlSearchParams> = {}; | ||
| const searchParams = new URLSearchParams(window.location.search.substring(1)); | ||
|
|
||
| searchParams.forEach((value, key) => { | ||
| parsedSearchParams[key as keyof UrlSearchParams] = value; | ||
| }); | ||
|
|
||
| if (searchParams.has(SEARCH_PARAM_NAMES.FILE_PATH)) { | ||
| // Extract filePath value by finding the parameter and taking everything after it | ||
| const filePathIndex = window.location.search.indexOf("filePath="); | ||
| if (-1 !== filePathIndex) { | ||
| const filePath = window.location.search.substring(filePathIndex + "filePath=".length); | ||
| if (0 !== filePath.length) { | ||
| let resolvedFilePath = filePath; | ||
| try { | ||
| resolvedFilePath = getAbsoluteUrl(filePath); | ||
| } catch (e) { | ||
| console.error("Unable to get absolute URL from filePath:", e); | ||
| } | ||
| parsedSearchParams[SEARCH_PARAM_NAMES.FILE_PATH] = resolvedFilePath; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return parsedSearchParams; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider edge case handling in filePath extraction.
The filePath extraction logic could be more robust. If window.location.search contains multiple occurrences of "filePath=", the current implementation will use the first occurrence, which may not be the intended behavior.
Consider adding validation or using a more precise extraction method:
const parseWindowUrlSearchParams = () : Partial<UrlSearchParams> => {
const parsedSearchParams : Partial<UrlSearchParams> = {};
- const searchParams = new URLSearchParams(window.location.search.substring(1));
+ const searchParams = new URLSearchParams(window.location.search);
searchParams.forEach((value, key) => {
parsedSearchParams[key as keyof UrlSearchParams] = value;
});
if (searchParams.has(SEARCH_PARAM_NAMES.FILE_PATH)) {
- // Extract filePath value by finding the parameter and taking everything after it
- const filePathIndex = window.location.search.indexOf("filePath=");
- if (-1 !== filePathIndex) {
- const filePath = window.location.search.substring(filePathIndex + "filePath=".length);
- if (0 !== filePath.length) {
+ // Use URLSearchParams for more reliable parameter extraction
+ const filePath = searchParams.get(SEARCH_PARAM_NAMES.FILE_PATH);
+ if (filePath && 0 !== filePath.length) {
let resolvedFilePath = filePath;
try {
resolvedFilePath = getAbsoluteUrl(filePath);
} catch (e) {
console.error("Unable to get absolute URL from filePath:", e);
}
parsedSearchParams[SEARCH_PARAM_NAMES.FILE_PATH] = resolvedFilePath;
- }
- }
+ }
}📝 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.
| /** | |
| * Parses the URL search parameters from the current window's URL. | |
| * | |
| * @return An object containing the parsed search parameters. | |
| */ | |
| const parseWindowUrlSearchParams = () : Partial<UrlSearchParams> => { | |
| const parsedSearchParams : Partial<UrlSearchParams> = {}; | |
| const searchParams = new URLSearchParams(window.location.search.substring(1)); | |
| searchParams.forEach((value, key) => { | |
| parsedSearchParams[key as keyof UrlSearchParams] = value; | |
| }); | |
| if (searchParams.has(SEARCH_PARAM_NAMES.FILE_PATH)) { | |
| // Extract filePath value by finding the parameter and taking everything after it | |
| const filePathIndex = window.location.search.indexOf("filePath="); | |
| if (-1 !== filePathIndex) { | |
| const filePath = window.location.search.substring(filePathIndex + "filePath=".length); | |
| if (0 !== filePath.length) { | |
| let resolvedFilePath = filePath; | |
| try { | |
| resolvedFilePath = getAbsoluteUrl(filePath); | |
| } catch (e) { | |
| console.error("Unable to get absolute URL from filePath:", e); | |
| } | |
| parsedSearchParams[SEARCH_PARAM_NAMES.FILE_PATH] = resolvedFilePath; | |
| } | |
| } | |
| } | |
| return parsedSearchParams; | |
| }; | |
| /** | |
| * Parses the URL search parameters from the current window's URL. | |
| * | |
| * @return An object containing the parsed search parameters. | |
| */ | |
| const parseWindowUrlSearchParams = (): Partial<UrlSearchParams> => { | |
| const parsedSearchParams: Partial<UrlSearchParams> = {}; | |
| const searchParams = new URLSearchParams(window.location.search); | |
| searchParams.forEach((value, key) => { | |
| parsedSearchParams[key as keyof UrlSearchParams] = value; | |
| }); | |
| if (searchParams.has(SEARCH_PARAM_NAMES.FILE_PATH)) { | |
| // Use URLSearchParams for more reliable parameter extraction | |
| const filePath = searchParams.get(SEARCH_PARAM_NAMES.FILE_PATH); | |
| if (filePath && 0 !== filePath.length) { | |
| let resolvedFilePath = filePath; | |
| try { | |
| resolvedFilePath = getAbsoluteUrl(filePath); | |
| } catch (e) { | |
| console.error("Unable to get absolute URL from filePath:", e); | |
| } | |
| parsedSearchParams[SEARCH_PARAM_NAMES.FILE_PATH] = resolvedFilePath; | |
| } | |
| } | |
| return parsedSearchParams; | |
| }; |
🤖 Prompt for AI Agents
In src/utils/url.ts between lines 56 and 87, the filePath extraction currently
uses indexOf to find the first occurrence of "filePath=" in
window.location.search, which can cause issues if multiple filePath parameters
exist. To fix this, replace the manual substring extraction with a more precise
method by using URLSearchParams.get or iterating over all "filePath" parameters
to handle multiple occurrences correctly. This ensures the correct filePath
value is extracted and processed robustly.
| const getUpdatedSearchParams = (updates: UrlSearchParamUpdatesType) => { | ||
| const currentSearchParams = parseWindowUrlSearchParams(); | ||
| const newSearchParams = new URLSearchParams(currentSearchParams); | ||
| const {filePath: newFilePath} = updates; | ||
|
|
||
| for (const [key, value] of Object.entries(updates)) { | ||
| if (SEARCH_PARAM_NAMES.FILE_PATH as string === key) { | ||
| // Updates to `filePath` should be handled last. | ||
| continue; | ||
| } | ||
| if (isEmptyOrFalsy(value)) { | ||
| newSearchParams.delete(key); | ||
| } else { | ||
| newSearchParams.set(key, String(value)); | ||
| } | ||
| } | ||
|
|
||
| // `filePath` should always be the last search parameter so that: | ||
| // 1. Users can specify a remote filePath (a URL) that itself contains URL parameters without | ||
| // encoding them. E.g. "<log-viewer-url>/?filePath=https://example.com/log/?p1=v1&p2=v2" | ||
| // 2. Users can easily modify it in the URL | ||
| // | ||
| // NOTE: We're relying on URLSearchParams.set() and URLSearchParams.toString() to store and | ||
| // serialize the parameters in the order that they were set. | ||
| const originalFilePath = newSearchParams.get(SEARCH_PARAM_NAMES.FILE_PATH); | ||
| newSearchParams.delete(SEARCH_PARAM_NAMES.FILE_PATH); | ||
| if ("undefined" === typeof newFilePath && null !== originalFilePath) { | ||
| // If no change in `filePath` is specified, put the original `filePath` back. | ||
| newSearchParams.set(SEARCH_PARAM_NAMES.FILE_PATH, originalFilePath); | ||
| } else if ("undefined" !== typeof newFilePath && null !== newFilePath) { | ||
| // If the new `filePath` has a non-null value, set the value. | ||
| newSearchParams.set(SEARCH_PARAM_NAMES.FILE_PATH, newFilePath); | ||
| } | ||
|
|
||
| // If the stringified search params doesn't contain characters that would make the URL ambiguous | ||
| // to parse, URL-decode it so that the `filePath` remains human-readable. E.g. | ||
| // "filePath=https://example.com/log/?s1=1&s2=2#h1=0" would make the final URL ambiguous to | ||
| // parse since `filePath` itself contains URL parameters. | ||
| let searchString = newSearchParams.toString(); | ||
| if (false === AMBIGUOUS_URL_CHARS_REGEX.test(searchString)) { | ||
| searchString = decodeURIComponent(searchString); | ||
| } | ||
|
|
||
| return searchString; | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify filePath handling logic for edge cases.
The special handling of filePath as the last parameter is well-documented, but the logic has some complexity that could benefit from verification.
The conditions on lines 186-192 handle different scenarios for filePath updates. Let me verify this logic works correctly for all cases:
🏁 Script executed:
#!/bin/bash
# Verify that filePath handling covers all expected scenarios
echo "Testing filePath parameter handling scenarios:"
# Check for any existing tests that validate this logic
fd -e test.ts -e spec.ts | xargs rg -l "filePath" -A 3 -B 3
# Look for any documentation about filePath parameter handling
rg -i "filepath.*parameter" -A 5 -B 5Length of output: 2449
Add comprehensive tests for filePath handling in getUpdatedSearchParams
We didn’t find any existing tests covering the complex ordering and encoding logic for filePath. Please add test cases targeting these scenarios in src/utils/url.ts (lines 160–204):
- No update:
newFilePathisundefined⇒ originalfilePathis preserved - Removal:
newFilePathis explicitlynull⇒filePathparameter is deleted - Update:
newFilePathis a new URL (including its own query params) ⇒ it’s appended last and remains human-readable - Ambiguous characters present (e.g.
?,&,#) ⇒ verify thatdecodeURIComponentis applied only when safe
Cover both browser-style URLs and remote URLs with embedded parameters to ensure correct ordering, deletion and decoding.
🤖 Prompt for AI Agents
In src/utils/url.ts around lines 160 to 204, add comprehensive unit tests for
the getUpdatedSearchParams function focusing on the special handling of the
filePath parameter. Create test cases for scenarios where newFilePath is
undefined (original filePath preserved), explicitly null (filePath removed), and
updated to a new URL with embedded query parameters (filePath appended last and
human-readable). Also, test that decodeURIComponent is applied only when the
search string does not contain ambiguous URL characters. Ensure tests cover both
local and remote URLs with embedded parameters to verify correct ordering,
deletion, and decoding behavior.
Also I am not sure if it is because my network issue... I can't download your code, but I think it's fine. There is only one function that I write it myself |
no worries! there's a holiday in the past few days, and thanks for getting back at least one day earlier than I expected :)
i missed that in my validations. thanks for testing thoroughly and finding the case!
sounds good. I believe this is a better behavior. please proceed with the changes
no worries. i'll do a final check before approving
sounds good. I agree parseWindowUrlHashParams is consistent and thus better |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/components/AppController.tsx (2)
102-109: 🛠️ Refactor suggestionUpdate query parameters conditionally to match the past review suggestion.
The current implementation sets all parameters unconditionally, which could trigger unnecessary state updates. The past review suggested checking against defaults before updating.
Apply this diff to align with the suggested implementation:
let isQueryModified = false; - isQueryModified ||= queryIsCaseSensitive !== currentQueryIsCaseSensitive; - setQueryIsCaseSensitive(queryIsCaseSensitive); + if (URL_HASH_PARAMS_DEFAULT.queryIsCaseSensitive !== queryIsCaseSensitive) { + isQueryModified ||= queryIsCaseSensitive !== currentQueryIsCaseSensitive; + setQueryIsCaseSensitive(queryIsCaseSensitive); + } - isQueryModified ||= queryIsRegex !== currentQueryIsRegex; - setQueryIsRegex(queryIsRegex); + if (URL_HASH_PARAMS_DEFAULT.queryIsRegex !== queryIsRegex) { + isQueryModified ||= queryIsRegex !== currentQueryIsRegex; + setQueryIsRegex(queryIsRegex); + } - isQueryModified ||= queryString !== currentQueryString; - setQueryString(queryString); + if (URL_HASH_PARAMS_DEFAULT.queryString !== queryString) { + isQueryModified ||= queryString !== currentQueryString; + setQueryString(queryString); + }
80-81: 🛠️ Refactor suggestionAdd conditional checks to prevent unnecessary state updates.
The current implementation updates state unconditionally, which could trigger unnecessary re-renders. Consider checking against defaults before updating, as suggested in the past review.
Apply this diff to update only when values differ from defaults:
- updateIsPrettified(isPrettified); - setLogEventNum(logEventNum); + if (URL_HASH_PARAMS_DEFAULT.isPrettified !== isPrettified) { + updateIsPrettified(isPrettified); + } + + if (URL_HASH_PARAMS_DEFAULT.logEventNum !== logEventNum) { + setLogEventNum(logEventNum); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/components/AppController.tsx(4 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
🧠 Learnings (1)
src/components/AppController.tsx (3)
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.
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#265
File: src/stores/logFileStore.ts:125-125
Timestamp: 2025-05-17T05:13:39.947Z
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.
Learnt from: zzxthehappiest
PR: y-scope/yscope-log-viewer#286
File: src/components/AppController.tsx:40-43
Timestamp: 2025-06-01T13:44:05.278Z
Learning: In the y-scope/yscope-log-viewer codebase, when functions return invalid or default log event numbers, they should use 0 as this aligns with URL_HASH_PARAMS_DEFAULT.LOG_EVENT_NUM which is set to 0. This maintains consistency with the established default hash parameter pattern.
🧬 Code Graph Analysis (1)
src/components/AppController.tsx (8)
src/typings/url.ts (1)
UrlHashParams(41-41)src/typings/common.ts (1)
Nullable(6-6)src/utils/url.ts (5)
getWindowUrlHashParams(331-331)updateWindowUrlHashParams(336-336)getWindowUrlSearchParams(332-332)URL_SEARCH_PARAMS_DEFAULT(339-339)URL_HASH_PARAMS_DEFAULT(338-338)src/services/LogFileManagerProxy.ts (2)
startQuery(65-68)loadFile(17-42)src/services/LogFileManager/index.ts (2)
cursor(492-514)numEvents(124-126)src/typings/worker.ts (2)
CursorType(106-106)CURSOR_CODE(99-99)src/typings/states.ts (1)
UI_STATE(106-106)src/stores/notificationStore.ts (1)
handleErrorWithNotification(76-76)
🔇 Additional comments (1)
src/components/AppController.tsx (1)
156-187: Well-structured initialization logic!The implementation correctly follows the past review suggestions:
- Uses
isInitializedref to prevent re-initialization in StrictMode- Retrieves states close to usage to avoid circular dependencies
- Handles both hash and search parameters on initial load
- Good comment explaining why
logEventNum0 is skipped
junhaoliao
left a comment
junhaoliao
left a comment
There was a problem hiding this comment.
thanks for cleaning things up and catching the other bugs! let's address the suggestions below before merging the PR
the PR title is good
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/stores/logFileStore.ts (1)
140-141: 🧹 Nitpick (assertive)Consider the placement of
startQuery().The
startQuery()call is placed afterupdatePageData(). Based on the past review comment, it was suggested to place this before thecanFormatcheck to ensure queries are actively restarted once a file is loaded. The current placement works but might delay query execution slightly.Consider moving
startQuery()before thecanFormatcheck as suggested in the past review:const pageData = await logFileManagerProxy.loadPage(cursor, isPrettified); updatePageData(pageData); const {startQuery} = useQueryStore.getState(); startQuery(); + const canFormat = fileInfo.fileType === FILE_TYPE.CLP_KV_IR || fileInfo.fileType === FILE_TYPE.JSONL;src/components/Editor/index.tsx (1)
189-189: 🧹 Nitpick (assertive)Consider removing refs as suggested in past reviews.
The code still uses
beginLineNumToLogEventNumRef(lines 192-194, 269-272) which could be replaced with direct store access viagetState()as suggested in past review comments. This would simplify the code and remove the need for synchronization via useEffect.Replace ref usage with direct store access:
- const beginLineNumToLogEventNumRef = useRef<BeginLineNumToLogEventNumMap>( - beginLineNumToLogEventNum - ); const handleCursorExplicitPosChange = useCallback(( ev: monaco.editor.ICursorPositionChangedEvent ) => { + const {beginLineNumToLogEventNum} = useViewStore.getState(); const newLogEventNum = getMapValueWithNearestLessThanOrEqualKey( - beginLineNumToLogEventNumRef.current, + beginLineNumToLogEventNum, ev.position.lineNumber );And remove the synchronization useEffect (lines 269-272).
Also applies to: 192-194, 269-272
src/components/AppController.tsx (1)
130-133:⚠️ Potential issueMisleading comment about removing empty/falsy parameters.
The code spreads
hashParamswithout any filtering, making this operation redundant since it updates the URL with the same values that were just read from it.Remove the redundant URL update:
- // Remove empty or falsy parameters. - updateWindowUrlHashParams({ - ...hashParams, - });src/utils/url.ts (2)
115-115: Type assertion is correctly used.Based on the retrieved learnings, the
as stringtype assertion is necessary here to avoid the@typescript-eslint/no-unsafe-enum-comparisonlint error when comparing enum values with strings fromObject.entries().
248-254: 🧹 Nitpick (assertive)Consider using a notification system for user feedback.
The console logging for clipboard operations provides developer feedback but may not be visible to end users. Consider integrating with a user-facing notification system if one exists in the application.
Would you like me to help implement user-facing notifications using the existing notification infrastructure?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
src/components/AppController.tsx(4 hunks)src/components/Editor/index.tsx(4 hunks)src/stores/logFileStore.ts(3 hunks)src/utils/url.ts(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/stores/logFileStore.tssrc/components/Editor/index.tsxsrc/components/AppController.tsxsrc/utils/url.ts
🧠 Learnings (3)
src/components/Editor/index.tsx (2)
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#265
File: src/stores/logFileStore.ts:125-125
Timestamp: 2025-05-17T05:13:39.947Z
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.
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.
src/components/AppController.tsx (3)
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.
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#265
File: src/stores/logFileStore.ts:125-125
Timestamp: 2025-05-17T05:13:39.947Z
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.
Learnt from: zzxthehappiest
PR: y-scope/yscope-log-viewer#286
File: src/components/AppController.tsx:40-43
Timestamp: 2025-06-01T13:44:05.278Z
Learning: In the y-scope/yscope-log-viewer codebase, when functions return invalid or default log event numbers, they should use 0 as this aligns with URL_HASH_PARAMS_DEFAULT.LOG_EVENT_NUM which is set to 0. This maintains consistency with the established default hash parameter pattern.
src/utils/url.ts (1)
Learnt from: zzxthehappiest
PR: y-scope/yscope-log-viewer#286
File: src/utils/url.ts:46-46
Timestamp: 2025-05-25T02:45:44.362Z
Learning: In TypeScript, when comparing enum values with strings (like keys from Object.entries()), the type assertion `as string` is necessary to avoid the @typescript-eslint/no-unsafe-enum-comparison lint error. This is a valid use of type assertion for enum-to-string comparison.
🔇 Additional comments (17)
src/stores/logFileStore.ts (2)
1-1: Consider if the 70-line limit is appropriate for this store.While limiting function length improves maintainability, the 70-line limit seems reasonable for the
loadFilemethod which handles multiple async operations, error handling, and state updates.
24-24: Import path correctly updated.The import has been properly migrated from the removed
UrlContextProviderto the newutils/urlmodule.src/components/Editor/index.tsx (3)
33-33: Import path correctly updated.The import has been properly migrated from the removed
UrlContextProviderto the newutils/urlmodule, and the.tsextension has been correctly removed as per past review comments.
127-178: Well-structured extraction ofhandleEditorCustomAction.Moving this function to the global scope is a good refactoring that:
- Improves code organization and readability
- Reduces the component's complexity
- Properly uses
getState()to access store state and actions at the point of use- Correctly handles the prettify toggle with URL synchronization
The implementation follows the established patterns for Zustand store access.
264-267: Correct use ofgetState()for static setter.The change properly uses
getState()to retrievesetLogEventNumat the point of use, which aligns with the codebase pattern of usinggetState()for static setters that never change. This avoids unnecessary subscriptions.src/components/AppController.tsx (6)
24-30: Import correctly updated to use new URL utilities.The import has been properly migrated from the removed
UrlContextProviderto the newutils/urlmodule, with the.tsextension correctly removed as addressed in past reviews.
71-82: Clean implementation ofupdateViewHashParams.The function properly updates view-related parameters from URL hash, using
getState()to retrieve store actions. The implementation is straightforward and follows the established patterns.
84-112:updateQueryHashParamscorrectly tracks modifications.The function properly:
- Compares current and new query parameters to detect changes
- Updates all query-related state
- Returns whether any modifications occurred for triggering query restart
Good implementation that enables proper query management.
114-136: Well-structuredhandleHashChangeimplementation.The function properly:
- Parses hash parameters
- Updates view and query parameters
- Triggers query restart when needed during hash change events
- Returns parsed parameters for use in initialization
Good separation of concerns between initialization and hash change events.
156-185: Initialization logic properly handles both hash and search parameters.The useEffect:
- Correctly uses
isInitializedref to prevent re-initialization in StrictMode- Registers the hash change handler
- Handles initial URL parameters appropriately
- Loads file with correct cursor based on presence of logEventNum
The implementation follows the suggestions from past reviews.
187-217: Page switching logic correctly uses direct store access.The useEffect properly:
- Retrieves state via
getState()at point of use to avoid circular dependencies- Only depends on
logEventNumas suggested in past reviews- Clamps the event number and handles page switching logic correctly
Good implementation following the established patterns.
src/utils/url.ts (6)
15-17: Consider the default filePath value based on past feedback.The past review comments suggested making parameter values non-nullable and setting
filePathto an empty string as the default. This implementation aligns with that feedback.
62-66: Helper function implementation is solid.The
isEmptyOrFalsyfunction correctly handles the three expected falsy cases:null,false, and empty strings. This follows the coding guideline preference for explicit boolean comparisons.
147-150: URL decoding logic is well-implemented.The conditional decoding based on the absence of ambiguous URL characters is a clever solution to maintain human-readable URLs while avoiding parsing ambiguity. This handles the edge case mentioned in the comments effectively.
174-190: Hash parameter parsing handles all expected types correctly.The function properly converts string values to their appropriate types (boolean, number) with safe fallbacks. The explicit handling of each parameter type ensures type safety.
299-303: Optimization prevents unnecessary history entries.Both update functions correctly check if parameters have actually changed before calling
pushState, preventing unnecessary browser history entries. This is good UX practice.Also applies to: 317-321
327-340: Export organization follows alphabetical order.The exports are well-organized and include all the necessary functions and constants for external consumption. The alphabetical ordering is maintained by the linter.
| if (searchParams.has(SEARCH_PARAM_NAMES.FILE_PATH)) { | ||
| // Extract filePath value by finding the parameter and taking everything after it | ||
| const filePathIndex = window.location.search.indexOf("filePath="); | ||
| if (-1 !== filePathIndex) { | ||
| const filePath = window.location.search.substring(filePathIndex + "filePath=".length); | ||
| if (0 !== filePath.length) { | ||
| let resolvedFilePath = filePath; | ||
| try { | ||
| resolvedFilePath = getAbsoluteUrl(filePath); | ||
| } catch (e) { | ||
| console.error("Unable to get absolute URL from filePath:", e); | ||
| } | ||
| parsedSearchParams[SEARCH_PARAM_NAMES.FILE_PATH] = resolvedFilePath; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Review the filePath extraction approach for potential issues.
The current implementation uses indexOf and substring for filePath extraction, which could be fragile if multiple "filePath=" occurrences exist in the URL. However, based on past discussions, this approach was chosen to handle special cases where filePath contains its own URL parameters.
Consider adding a comment explaining why this manual approach is used instead of URLSearchParams.get() to clarify the design decision for future maintainers.
🤖 Prompt for AI Agents
In src/utils/url.ts around lines 81 to 96, the filePath extraction uses indexOf
and substring instead of URLSearchParams.get() to handle cases where filePath
contains its own URL parameters. Add a clear comment above this code block
explaining this design choice to clarify why the manual extraction approach is
necessary and prevent confusion for future maintainers.
|
@zzxthehappiest Again thanks for your contribution! I'm merging this now |
…egressed by y-scope#286; fixes y-scope#329).
4 participants
Description
#269
Checklist
breaking change.
Validation performed
isPrettified,logEventNum, query-related,filePath).logEventNumbecause its default value is 0, which is not a valid one).Summary by CodeRabbit
Summary by CodeRabbit
Refactor
Bug Fixes
New Features
Style
Documentation