feat: Support switching timezone of log event timestamp (resolves #207). #251
Conversation
WalkthroughThis change introduces timezone selection support for log timestamp formatting. It adds a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TimezoneSelect
participant ViewStore
participant LogFileManagerProxy
participant LogFileManager
participant Decoder
User->>TimezoneSelect: Selects a timezone
TimezoneSelect->>ViewStore: updateLogTimezone(newTimezone)
ViewStore->>LogFileManagerProxy: loadPage(cursor, isPrettified, newTimezone)
LogFileManagerProxy->>LogFileManager: loadPage(cursor, isPrettified, newTimezone)
LogFileManager->>Decoder: decodeRange(beginIdx, endIdx, useFilter, newTimezone)
Decoder-->>LogFileManager: Decoded log events (formatted with newTimezone)
LogFileManager-->>LogFileManagerProxy: PageData
LogFileManagerProxy-->>ViewStore: PageData
ViewStore-->>TimezoneSelect: Updated logs displayed
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (10)
src/components/StatusBar/TimezoneSelect/index.css (1)
13-23: Consider reducing reliance on!importantflags.While these overrides work, using multiple
!importantflags can lead to maintainability issues in the long run. Consider using more specific selectors or modifying the base Joy UI components if possible.src/services/decoders/ClpIrDecoder/index.ts (2)
99-105: Good timezone handling logic with fallback.The implementation correctly handles both the case when a timezone is provided and when it's not. However, there's a TODO comment with a developer name that should be addressed.
Consider replacing the developer-specific TODO comment with a more descriptive task identifier:
- // TODO: ZZX, replace with the original log timezone + // TODO: Replace with the original log timezone from the log file
176-177: Add a more specific timeline for implementing timezone support for JSON logs.The TODO comment indicates that timezone support for structured (JSON) log events is planned but not implemented yet. Consider adding more details about when this will be addressed or creating a tracking issue.
- // TODO: Support specifying timezone for JSON log events. + // TODO: Support specifying timezone for JSON log events. (See issue #XXX)src/contexts/StateContextProvider.tsx (2)
190-196: Function parameter documentation is inconsistent with existing patterns.The newly added parameter documentation uses "format the timezone" while the existing parameter uses "is pretty-printing". Consider maintaining a consistent style across parameter documentation.
- * @param logTimezone format the timezone of the log timestamp + * @param logTimezone timezone to use for formatting log timestamps
493-514: Consider extracting duplicate page reloading logic.This useEffect for isPrettified contains logic that's nearly identical to the new useEffect for logTimezone below. Both create similar cursors, set the UI to loading state, and call loadPageByCursor with current parameters.
You could extract this pattern into a helper function:
+ // Helper function to reload the current page with updated parameters + const reloadCurrentPage = useCallback(() => { + if (null === mainWorkerRef.current) { + return; + } + + const cursor: CursorType = { + code: CURSOR_CODE.EVENT_NUM, + args: {eventNum: logEventNumRef.current ?? 1}, + }; + + setUiState(UI_STATE.FAST_LOADING); + loadPageByCursor( + mainWorkerRef.current, + cursor, + isPrettifiedRef.current, + logTimezoneRef.current + ); + }, []); // Then use it in both effects useEffect(() => { isPrettifiedRef.current = isPrettified ?? false; - if (null === mainWorkerRef.current) { - return; - } - - const cursor: CursorType = { - code: CURSOR_CODE.EVENT_NUM, - args: {eventNum: logEventNumRef.current ?? 1}, - }; - - setUiState(UI_STATE.FAST_LOADING); - loadPageByCursor( - mainWorkerRef.current, - cursor, - isPrettified ?? false, - logTimezoneRef.current - ); + reloadCurrentPage(); }, [isPrettified, reloadCurrentPage]);src/components/StatusBar/TimezoneSelect/index.tsx (5)
34-52: Consider making timezone list configurable.The hardcoded list of common timezones may not cover all user needs or regional preferences. Consider making this configurable or loading it from a configuration file.
-const COMMON_TIMEZONES = [ - "America/New_York", - "Europe/London", - // ... other timezones -]; +// Import from a configuration file or context +import { COMMON_TIMEZONES } from "../../../config/timezones";
74-81: Improve selected timezone value rendering.The component uses a nested Chip component that displays "Timezone" and the selected value. Consider adding an aria-label for improved accessibility.
const handleRenderValue = (selected: SelectValue<SelectOption<string>, false>) => ( - <Box className={"timezone-select-render-value-box"}> + <Box className={"timezone-select-render-value-box"} aria-label="Selected timezone"> <Chip className={"timezone-select-render-value-box-label"}>Timezone</Chip> <Chip> {selected?.label} </Chip> </Box> );
104-118: Simplify conditional rendering logic.The conditional rendering for the timezone options uses separate blocks with repeated ListItemContent. This can be simplified for better maintainability.
- {LOGGER_TIMEZONE !== value && - <ListItemContent> - ( - {getLongOffsetOfTimezone(value)} - ) - {" "} - {label} - {" "} - {suffix ?? ""} - </ListItemContent>} - - {LOGGER_TIMEZONE === value && - <ListItemContent> - {LOGGER_TIMEZONE} - </ListItemContent>} + <ListItemContent> + {LOGGER_TIMEZONE === value + ? LOGGER_TIMEZONE + : <>({getLongOffsetOfTimezone(value)}) {label} {suffix ?? ""}</> + } + </ListItemContent>
141-144: Consider handling browser timezone changes.The component fetches the browser timezone only once on mount. If the user changes their system timezone while the app is open, this won't be reflected until a refresh.
You could add a periodic check or handle the
Intl.DateTimeFormat().resolvedOptions().timeZonechanges, though this is a fairly edge case.
154-164: Improve logic for updating URL hash parameters.The conditional statement for updating URL hash could be more explicit about its purpose. Consider adding a comment explaining the logic.
useEffect(() => { if (selectedTimezone !== logTimezoneRef.current) { + // If Logger Timezone is selected, remove the parameter from URL + // Otherwise set it to the selected timezone const updatedTimezone = (LOGGER_TIMEZONE === selectedTimezone) ? null : selectedTimezone; updateWindowUrlHashParams({ [HASH_PARAM_NAMES.LOG_TIMEZONE]: updatedTimezone, }); } }, [selectedTimezone]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
docs/src/dev-guide/contributing-validation.md(1 hunks)src/components/StatusBar/TimezoneSelect/index.css(1 hunks)src/components/StatusBar/TimezoneSelect/index.tsx(1 hunks)src/components/StatusBar/index.tsx(2 hunks)src/contexts/StateContextProvider.tsx(7 hunks)src/contexts/UrlContextProvider.tsx(2 hunks)src/services/LogFileManager/index.ts(5 hunks)src/services/MainWorker.ts(3 hunks)src/services/decoders/ClpIrDecoder/index.ts(3 hunks)src/services/decoders/JsonlDecoder/index.ts(1 hunks)src/typings/decoders.ts(1 hunks)src/typings/states.ts(5 hunks)src/typings/url.ts(2 hunks)src/typings/worker.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/components/StatusBar/index.tsxsrc/services/MainWorker.tssrc/contexts/UrlContextProvider.tsxsrc/services/LogFileManager/index.tssrc/typings/states.tssrc/services/decoders/ClpIrDecoder/index.tssrc/typings/url.tssrc/typings/decoders.tssrc/services/decoders/JsonlDecoder/index.tssrc/typings/worker.tssrc/components/StatusBar/TimezoneSelect/index.tsxsrc/contexts/StateContextProvider.tsx
🧬 Code Graph Analysis (2)
src/contexts/UrlContextProvider.tsx (1)
src/typings/url.ts (1)
HASH_PARAM_NAMES(39-39)
src/services/decoders/ClpIrDecoder/index.ts (2)
src/typings/decoders.ts (1)
DecodeResult(108-108)src/services/decoders/JsonlDecoder/utils.ts (1)
convertToDayjsTimestamp(81-81)
🔇 Additional comments (35)
src/components/StatusBar/index.tsx (2)
25-25: Import of TimezoneSelect component looks good.The import is correctly placed and follows the same import pattern used for other components in this file.
69-69: TimezoneSelect component integration is well-positioned.The TimezoneSelect component is properly integrated into the StatusBar JSX structure, positioned appropriately above the "Copy link to clipboard" section. This placement is intuitive and aligns with the PR objectives to allow users to select different timezone options for log event timestamps.
docs/src/dev-guide/contributing-validation.md (1)
43-43: Documentation updated appropriately for manual testing.The new testing instruction clearly communicates how to validate the timezone selection feature through both the UI dropdown and URL parameter, which aligns with the PR objectives.
src/typings/states.ts (2)
42-42: New UI element for timezone selection looks good.The TIMEZONE_SETTER enum entry is properly added to the UI_ELEMENT enum to represent the new timezone selection component.
69-69: UI state configuration for timezone selector is appropriate.The TIMEZONE_SETTER UI element is properly configured across all UI states:
- Disabled when no file is opened (UNOPENED state)
- Disabled during file loading (FILE_LOADING state)
- Enabled during fast loading (FAST_LOADING state)
- Enabled when the file is loaded (READY state)
This configuration aligns with how other similar UI elements are handled and matches the expected behavior described in the PR objectives.
Also applies to: 81-81, 93-93, 105-105
src/typings/url.ts (2)
10-11: Hash parameter for timezone selection looks good.The LOG_TIMEZONE parameter is properly added to the HASH_PARAM_NAMES enum, maintaining alphabetical ordering. This supports the URL parameter requirement mentioned in the PR objectives.
21-21: Interface updated correctly for timezone parameter.The UrlHashParams interface is properly extended to include the LOG_TIMEZONE parameter as a string type, which is consistent with the parameter implementation pattern in this file.
src/services/MainWorker.ts (3)
99-99: Timezone parameter correctly propagatedThe
args.logTimezoneparameter is now passed to theloadPagemethod, enabling timestamp timezone transformation when loading a file.
109-109: Timezone parameter correctly propagatedThe
args.logTimezoneparameter is now passed to theloadPagemethod, enabling timestamp timezone transformation when navigating between pages.
120-120: Timezone parameter correctly propagatedThe
args.logTimezoneparameter is now passed to theloadPagemethod, ensuring that timezone transformations persist when filters are applied.src/typings/decoders.ts (2)
93-94: JSDoc comment is appropriate and clearThe JSDoc accurately describes the purpose of the new timezone parameter.
97-102: Interface method signature properly updatedThe
decodeRangemethod signature is properly updated to include the new timezone parameter, ensuring consistent implementation across all decoder classes.src/typings/worker.ts (3)
106-106: Type definition correctly updated for LOAD_FILEThe
logTimezoneparameter is properly added to the LOAD_FILE request type with the correct nullable string type.
111-111: Type definition correctly updated for LOAD_PAGEThe
logTimezoneparameter is properly added to the LOAD_PAGE request type with the correct nullable string type.
117-117: Type definition correctly updated for SET_FILTERThe
logTimezoneparameter is properly added to the SET_FILTER request type with the correct nullable string type.src/components/StatusBar/TimezoneSelect/index.css (2)
1-3: Well-documented CSS explanation.This comment clearly explains the reason for the custom styling, which helps future developers understand the purpose of these overrides.
29-31: Excellent use of CSS variables for dynamic sizing.Using CSS variables for calculating the dropdown height ensures the component adapts correctly to different viewport sizes and respects the layout constraints.
src/contexts/UrlContextProvider.tsx (2)
35-35: LGTM: Consistent implementation of new URL parameter.The addition of
LOG_TIMEZONEto the default URL hash parameters follows the existing pattern and initializes it tonullas expected.
222-225: LGTM: Proper extraction of timezone parameter from URL hash.The implementation correctly extracts the timezone parameter from the URL hash and assigns it to the hash parameters object when present. This follows the same pattern as other parameters like
logEventNumandisPrettified.src/services/LogFileManager/index.ts (4)
250-259: LGTM: Proper signature extension for timezone support.The
loadPagemethod's signature has been correctly updated to include thelogTimezoneparameter. The JSDoc is updated to reflect this change, which is good practice.
278-278: LGTM: Propagating timezone parameter to decoder.The timezone parameter is correctly passed to the decoder's
decodeRangemethod, enabling timezone-aware formatting of timestamps.
226-227: Explicit null timezone parameter in non-UI code paths.Explicitly passing
nullas the timezone parameter in methods likeexportChunkAndScheduleNextand#queryChunkAndScheduleNextensures consistent behavior when these paths are executed, which is a good defensive programming approach.Also applies to: 396-397
282-282: Simplified error message is more concise.The error message is now more concise, which is generally good practice. However, it still provides the essential information for debugging.
src/services/decoders/ClpIrDecoder/index.ts (3)
89-92: LGTM: Proper method signature extension for timezone support.The
#formatUnstructuredResultsmethod signature has been correctly updated to include thelogTimezoneparameter, enabling timezone-aware formatting.
154-155: LGTM: Updated decodeRange method with timezone parameter.The
decodeRangemethod signature has been properly updated to include the timezone parameter, which enables the propagation of timezone information from UI to the decoder.
173-173: LGTM: Correct timezone parameter passing to formatter.The timezone parameter is correctly passed to the
#formatUnstructuredResultsmethod when formatting unstructured logs.src/contexts/StateContextProvider.tsx (7)
254-254: LGTM: Properly extracting logTimezone from context.The change correctly pulls the logTimezone from the UrlContext along with other existing parameters.
278-278: LGTM: Correctly initialized logTimezone reference.The ref is properly initialized with the value from context.
431-431: LGTM: Properly propagating logTimezone to worker request.The timezone reference is correctly passed to the worker request when loading a file.
472-477: LGTM: Correctly passing logTimezone to loadPageByCursor.The change properly includes the logTimezone reference when navigating between pages.
489-489: LGTM: Propagating logTimezone when filtering logs.The change correctly includes the timezone reference when applying log level filters.
521-537: LGTM: Effect properly synchronizes logTimezone state.The effect correctly updates the logTimezoneRef and reloads the page with the updated timezone when it changes. This ensures consistent timezone formatting across the application.
583-589: LGTM: Correctly passing logTimezone when loading pages by event number.The logTimezoneRef is properly included when changing pages based on event number changes.
src/components/StatusBar/TimezoneSelect/index.tsx (2)
146-152: LGTM: Properly synchronizes with URL context.The effect correctly updates the selected timezone when the logTimezone from URL context changes or when the component becomes enabled.
172-226: LGTM: Well-implemented timezone selector component.The component is properly integrated with application state, uses appropriate UI elements, and includes clear visual indicators for the selected timezone. The dropdown with system timezone and common timezones provides good user experience.
| // eslint-disable-next-line no-warning-comments | ||
| // TODO: Support specifying timezone for JSON log events. | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| _: string | null | ||
| ): Nullable<DecodeResult[]> { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider implementing timezone support for JsonlDecoder
The method correctly implements the required interface by accepting the timezone parameter, but it's currently unused (marked with underscore). The TODO comment indicates future plans to support timezones for JSON log events.
While this satisfies the interface contract, consider implementing actual timezone support to provide consistent behavior across all decoder types.
🏁 Script executed:
#!/bin/bash
# Check if other decoders implement timezone functionality
echo "Checking how other decoders implement timezone support:"
rg -A 20 -B 5 "decodeRange.*timezone" --glob "src/services/decoders/**/*.ts" | grep -v "JsonlDecoder"Length of output: 222
Implement timezone support for JsonlDecoder
The timezone argument in JsonlDecoder.decodeRange (src/services/decoders/JsonlDecoder/index.ts lines 103–107) is accepted but currently ignored (marked as _), and no other decoders in src/services/decoders provide timezone handling. To ensure consistent timestamp parsing across all decoders:
- Replace the
_placeholder with logic to parse and apply thetimezoneparameter (e.g., use Luxon or similar to parse/convert JSON log timestamps). - Factor out shared timezone‐handling utilities for reuse if other decoders adopt timezone support later.
- Add unit tests covering various timezone values to verify correct normalization of log timestamps.
| const getLongOffsetOfTimezone = (tz: string): string => { | ||
| return new Intl.DateTimeFormat("default", { | ||
| timeZone: tz, | ||
| timeZoneName: "longOffset", | ||
| }).formatToParts() | ||
| .find((p) => "timeZoneName" === p.type)?.value ?? "Unknown timezone"; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for invalid timezone strings.
The function doesn't handle invalid timezone strings gracefully. While there's a fallback to "Unknown timezone", it would be better to validate the timezone before attempting to use it.
const getLongOffsetOfTimezone = (tz: string): string => {
+ // Validate timezone before using it
+ try {
+ // Check if timezone is valid by attempting to use it
+ Intl.DateTimeFormat("default", { timeZone: tz });
+
return new Intl.DateTimeFormat("default", {
timeZone: tz,
timeZoneName: "longOffset",
}).formatToParts()
.find((p) => "timeZoneName" === p.type)?.value ?? "Unknown timezone";
+ } catch (error) {
+ console.warn(`Invalid timezone: ${tz}`);
+ return "Invalid timezone";
+ }
};📝 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.
| const getLongOffsetOfTimezone = (tz: string): string => { | |
| return new Intl.DateTimeFormat("default", { | |
| timeZone: tz, | |
| timeZoneName: "longOffset", | |
| }).formatToParts() | |
| .find((p) => "timeZoneName" === p.type)?.value ?? "Unknown timezone"; | |
| }; | |
| const getLongOffsetOfTimezone = (tz: string): string => { | |
| // Validate timezone before using it | |
| try { | |
| // Check if timezone is valid by attempting to use it | |
| Intl.DateTimeFormat("default", { timeZone: tz }); | |
| return new Intl.DateTimeFormat("default", { | |
| timeZone: tz, | |
| timeZoneName: "longOffset", | |
| }).formatToParts() | |
| .find((p) => "timeZoneName" === p.type)?.value ?? "Unknown timezone"; | |
| } catch (error) { | |
| console.warn(`Invalid timezone: ${tz}`); | |
| return "Invalid timezone"; | |
| } | |
| }; |
|
hey @zzxthehappiest , thanks a lot for your contribution again! Please allow me some another week to go through this - We are conducting a major refactoring to clean up the global states in #224 and we want to merge that PR first to avoid causing more conflicts. I'll let you know once we're done with the refactoring. Feel free to share if you have any question! |
|
Convert the PR to draft, I will work on applying my code based on the refactored codebase. |
# Conflicts: # src/contexts/StateContextProvider.tsx # src/services/LogFileManager/index.ts # src/services/MainWorker.ts # src/typings/worker.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/services/decoders/JsonlDecoder/index.ts (1)
99-103: Implement timezone support for JsonlDecoder.The method signature has been updated to include a timezone parameter, but it's currently unused (marked with underscore). The TODO comment indicates future plans to support timezone for JSON log events.
#!/bin/bash # Check implementation details of timezone handling in other decoders echo "Checking how other decoders implement timezone support:" rg -A 10 "decodeRange.*timezone" --glob "src/services/decoders/**/*.ts" | grep -v "JsonlDecoder/index.ts"src/components/StatusBar/TimezoneSelect/index.tsx (1)
59-65: 🛠️ Refactor suggestionAdd error handling for invalid timezone strings.
The function doesn't handle invalid timezone strings gracefully. While there's a fallback to "Unknown timezone", it would be better to validate the timezone before attempting to use it.
const getLongOffsetOfTimezone = (tz: string): string => { + // Validate timezone before using it + try { + // Check if timezone is valid by attempting to use it + Intl.DateTimeFormat("default", { timeZone: tz }); + return new Intl.DateTimeFormat("default", { timeZone: tz, timeZoneName: "longOffset", }).formatToParts() .find((p) => "timeZoneName" === p.type)?.value ?? "Unknown timezone"; + } catch (error) { + console.warn(`Invalid timezone: ${tz}`); + return "Invalid timezone"; + } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
src/components/StatusBar/TimezoneSelect/index.tsx(1 hunks)src/components/StatusBar/index.tsx(2 hunks)src/contexts/AppController.tsx(4 hunks)src/contexts/UrlContextProvider.tsx(2 hunks)src/services/LogFileManager/index.ts(4 hunks)src/services/LogFileManagerProxy.ts(1 hunks)src/services/decoders/ClpIrDecoder/index.ts(3 hunks)src/services/decoders/JsonlDecoder/index.ts(1 hunks)src/stores/logFileStore.ts(2 hunks)src/stores/viewStore.ts(7 hunks)src/typings/states.ts(5 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/StatusBar/index.tsxsrc/typings/states.tssrc/contexts/UrlContextProvider.tsxsrc/services/LogFileManagerProxy.tssrc/services/decoders/ClpIrDecoder/index.tssrc/contexts/AppController.tsxsrc/stores/logFileStore.tssrc/stores/viewStore.tssrc/components/StatusBar/TimezoneSelect/index.tsxsrc/services/decoders/JsonlDecoder/index.tssrc/services/LogFileManager/index.ts
🧬 Code Graph Analysis (5)
src/contexts/UrlContextProvider.tsx (1)
src/typings/url.ts (1)
HASH_PARAM_NAMES(39-39)
src/services/decoders/ClpIrDecoder/index.ts (2)
src/typings/decoders.ts (1)
DecodeResult(108-108)src/services/decoders/JsonlDecoder/utils.ts (1)
convertToDayjsTimestamp(81-81)
src/contexts/AppController.tsx (2)
src/contexts/UrlContextProvider.tsx (1)
UrlContext(288-288)src/services/LogFileManager/index.ts (1)
cursor(498-520)
src/stores/viewStore.ts (4)
src/services/LogFileManager/index.ts (1)
cursor(498-520)src/typings/notifications.ts (1)
DO_NOT_TIMEOUT_VALUE(36-36)src/typings/worker.ts (2)
CursorType(107-107)CURSOR_CODE(100-100)src/stores/contextStore.ts (1)
CONTEXT_STORE_DEFAULT(35-35)
src/components/StatusBar/TimezoneSelect/index.tsx (3)
src/contexts/UrlContextProvider.tsx (2)
UrlContext(288-288)updateWindowUrlHashParams(284-284)src/typings/states.ts (1)
UI_ELEMENT(110-110)src/typings/url.ts (1)
HASH_PARAM_NAMES(39-39)
🪛 GitHub Actions: lint
src/contexts/AppController.tsx
[error] 85-85: ESLint: Arrow function has too many statements (21). Maximum allowed is 20. (max-statements)
🔇 Additional comments (38)
src/components/StatusBar/index.tsx (2)
26-26: Clean import of the new TimezoneSelect component.The import of the TimezoneSelect component is well-placed among the other component imports.
71-71: Well-positioned TimezoneSelect UI component.The TimezoneSelect component is appropriately placed within the StatusBar layout, providing users with convenient access to timezone selection functionality.
src/stores/logFileStore.ts (2)
109-109: Properly destructured logTimezone from viewStore.The logTimezone property is correctly extracted from viewStore.getState() alongside other properties needed for file loading.
134-134: Successfully propagated logTimezone to the loadPage call.The logTimezone parameter is now properly passed to the logFileManagerProxy.loadPage method, allowing timezone-aware log timestamp formatting.
src/typings/states.ts (3)
42-42: Added TIMEZONE_SETTER to UI_ELEMENT enum.The new TIMEZONE_SETTER UI element is properly added to the UI_ELEMENT enum, enabling state control for the timezone selection feature.
69-69: Appropriate UI_STATE_GRID configuration for TIMEZONE_SETTER.The TIMEZONE_SETTER is correctly:
- Disabled in UNOPENED, FILE_LOADING, and FAST_LOADING states
- Enabled only in READY state when a log file is loaded and can be viewed
This configuration aligns with the expected behavior for the timezone selection feature.
Also applies to: 81-81, 93-93, 105-105
90-90: Changed PRETTIFY_BUTTON to disabled in FAST_LOADING state.Disabling the PRETTIFY_BUTTON during FAST_LOADING prevents interaction with this button during loading operations, which is a good UX improvement to avoid potential race conditions.
src/contexts/UrlContextProvider.tsx (2)
35-35: Good addition of LOG_TIMEZONE parameterAdding this parameter to the default URL hash parameters properly enables timezone selection persistence in the application URL, aligning with the PR objectives.
227-230: Well-implemented timezone parameter extractionThe implementation follows the same pattern as other hash parameters, maintaining code consistency. Good use of the null check pattern with
null !== logTimezonewhich aligns with the coding guideline of preferringnull !== expressionover negation.src/contexts/AppController.tsx (4)
87-87: Proper integration of logTimezone from URL contextCorrectly extended the destructured context to include the new logTimezone parameter from the UrlContext.
96-96: Well-structured state management for timezoneAppropriately added the ViewStore timezone updater and a ref to track the timezone value, consistent with how other parameters are managed in this component.
Also applies to: 103-103
125-132: Good synchronization pattern for timezoneThe effect hook correctly synchronizes the logTimezoneRef with the URL context value and updates the view store, following the established pattern used for other parameters.
158-162: Properly propagated timezone to loadPage callThe timezone value is correctly passed to the loadPage method, enabling timezone-aware log display.
src/services/LogFileManager/index.ts (4)
285-289: Good method signature update for timezone supportThe loadPage method signature is properly extended with the logTimezone parameter, and the JSDoc is updated accordingly to document the new parameter's purpose.
304-309: Correctly propagates timezone to decoderThe timezone parameter is correctly passed to the decoder's decodeRange method, enabling timezone-aware timestamp formatting.
312-312: Improved error message clarityThe error message for decode failures is now more concise and clear, making debugging easier.
256-257: Consistent null timezone handling in utility methodsThe exportChunkAndScheduleNext and queryChunkAndScheduleNext methods now correctly pass null for the timezone parameter, ensuring consistent behavior across the codebase.
Also applies to: 425-427
src/services/LogFileManagerProxy.ts (2)
44-48: Well-implemented timezone support in loadPageThe loadPage method signature is properly updated to include the logTimezone parameter, and it correctly forwards this parameter to the underlying LogFileManager.
Also applies to: 50-50
56-58: Properly extended setFilter to support timezoneThe setFilter method signature is appropriately updated to include the logTimezone parameter, ensuring that timezone settings persist when filters change.
Also applies to: 62-62
src/stores/viewStore.ts (7)
34-34: Interface addition aligns with feature requirements.Adding
logTimezonefield toViewStoreValuesinterface properly establishes the state management needed for the timezone switching functionality.
48-48: Action method interface addition is well-defined.This method signature appropriately accepts a nullable string, allowing users to select specific timezones or reset to the default logger timezone.
58-58: Default state is appropriately initialized.Setting the default
logTimezonetonullis appropriate as it indicates that the default logger timezone should be used when no explicit timezone is selected.
130-132: Updated filter parameter passing is correct.Passing the current
logTimezoneto thesetFiltermethod ensures that any filtering operations maintain the user's selected timezone setting.
147-147: Propagation of timezone in loadPageByAction is correct.The
logTimezonestate value is correctly passed to theloadPagecall, ensuring that navigation actions maintain the user's selected timezone.Also applies to: 184-184
227-231: Updated prettify state change correctly handles timezone.The
updateIsPrettifiedmethod appropriately forwards the current timezone setting when reloading the page with the new prettify state.
243-278: Well-implemented timezone update method.The
updateLogTimezonemethod follows the same pattern as other state update methods in this file, which is good for consistency. It properly:
- Checks for redundant updates
- Updates the state
- Sets the loading UI state
- Determines the appropriate cursor
- Loads the page with the new timezone
- Handles errors appropriately
src/services/decoders/ClpIrDecoder/index.ts (5)
86-92: Method signature correctly extended with timezone parameter.The
#formatUnstructuredResultsmethod has been appropriately updated to accept a nullable timezone parameter, enabling timezone-aware formatting.
99-105: Timezone application logic is well-implemented.The conditional logic properly applies the timezone when formatting timestamps if a timezone is provided. The fallback to the default format when timezone is null is appropriate.
The TODO comment clearly indicates a future enhancement to use the original log timezone as the fallback.
154-156: Method signature extension maintains parameter order consistency.The
decodeRangesignature has been correctly updated to include the timezone parameter while maintaining the logical order of parameters.
173-173: Timezone parameter correctly passed to formatting function.The code properly passes the timezone parameter to the
#formatUnstructuredResultsmethod when formatting unstructured logs.
176-177: TODO comment provides clarity on future implementation.The comment clearly indicates that timezone support for JSON log events is planned for future implementation, which is good documentation.
src/components/StatusBar/TimezoneSelect/index.tsx (7)
34-51: Well-organized timezone constants.The default "Logger Timezone" constant and the comprehensive list of common timezones provide good options for users. The list includes major timezone regions across different continents.
73-80: Clean render value implementation with semantic structure.The
handleRenderValuefunction creates a clean UI for the selected value, using appropriate semantic elements (Box and Chip components) with proper class naming.
91-119: Well-structured timezone option rendering.The
renderTimezoneOptionfunction properly handles different rendering cases:
- Special rendering for the Logger Timezone
- Including GMT offset for all other timezones
- Optional suffix support (used for browser timezone)
This provides clear information to users about each timezone option.
129-152: State management is well-implemented.The component correctly:
- Retrieves the current UI state and URL context
- Initializes state for browser and selected timezones
- Uses a ref to track URL timezone changes
- Respects the UI disabled state
- Initializes the browser timezone on mount
- Updates selected timezone when URL or disabled state changes
This ensures proper synchronization between component state and application state.
153-163: URL synchronization is correctly implemented.The effect correctly updates the URL hash parameters when the selected timezone changes, and handles the special case of resetting to null when the default Logger Timezone is selected.
171-225: Select component implementation is comprehensive and well-structured.The Select component:
- Uses appropriate styling and size
- Correctly handles disabled state
- Provides clear visual indicator
- Uses custom rendering for selected value
- Properly structures the options with a separator
- Handles selection changes correctly
This provides a good user experience for timezone selection.
165-169: 🧹 Nitpick (assertive)Consider using TypeScript's type assertion instead of type coercion.
When accessing
dataset.value, a type assertion would be clearer than using nullish coalescing with a default.const handleOptionClick = useCallback((ev: React.MouseEvent) => { const currentTarget = ev.currentTarget as HTMLElement; - const value = currentTarget.dataset.value ?? LOGGER_TIMEZONE; + const value = currentTarget.dataset.value as string || LOGGER_TIMEZONE; setSelectedTimezone(value); }, []);Likely an incorrect or invalid review comment.
| @@ -82,9 +82,9 @@ | |||
| * @param props.children | |||
| * @return | |||
| */ | |||
| const AppController = ({children}: StateContextProviderProps) => { | |||
There was a problem hiding this comment.
Fix the linting error for function size
The function now exceeds the maximum allowed statements (21 vs limit of 20) according to ESLint configuration.
Consider refactoring by extracting some functionality into separate helper functions to reduce the statement count within the main component function.
🧰 Tools
🪛 GitHub Check: lint-check
[failure] 85-85:
Arrow function has too many statements (21). Maximum allowed is 20
🪛 ESLint
[error] 85-214: Arrow function has too many statements (21). Maximum allowed is 20.
(max-statements)
🪛 GitHub Actions: lint
[error] 85-85: ESLint: Arrow function has too many statements (21). Maximum allowed is 20. (max-statements)
|
The following lint check cannot be passed. I checked the |
Thanks for updating this PR! We will continue to migrate states from the UrlContextProvider (and potentially the NotificationProvider) to Zustand, which should further reduce the size of AppController. If you may, please put this PR on hold while we figure out how to proceed with the UrlContextProvider refactoring, hopefully in one or two days. |
|
Convert to draft. |
Description
This PR is to resolve #207
Checklist
breaking change.
Validation performed
Summary by CodeRabbit
New Features
Documentation
Style