-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Timezone basic functionality #6492
Timezone basic functionality #6492
Conversation
…and timezone preferences
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
1 similar comment
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested. Reviewed everything up to 676e32e in 44 seconds
More details
- Looked at
396
lines of code in8
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. frontend/src/providers/Timezone.tsx:76
- Draft comment:
Storing the timezone as a JSON string in localStorage is inconsistent with how it's retrieved. Consider storing it as a plain string instead. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_gm33mh7sTACsIzlq
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Incremental review on e012f10 in 45 seconds
More details
- Looked at
77
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. frontend/src/providers/Timezone.tsx:34
- Draft comment:
EnsuregetTimezoneObjectByTimezoneString
does not returnnull
or handle the case where it might returnnull
to avoid runtime errors. - Reason this comment was not posted:
Comment did not seem useful.
2. frontend/src/components/CustomTimePicker/timezoneUtils.ts:136
- Draft comment:
EnsuregetTimezoneObjectByTimezoneString
does not returnnull
or handle the case where it might returnnull
to avoid runtime errors. - Reason this comment was not posted:
Marked as duplicate.
3. frontend/src/providers/Timezone.tsx:27
- Draft comment:
Avoid using thecomponent/index.tsx
file structure approach, as it makes it difficult to debug and find components using global search tools like VS Code. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is not relevant to the changes made in this diff. The file structure mentioned in the comment does not apply to the file being reviewed. Therefore, the comment should be deleted.
I might be missing some context about the overall project structure, but based on the file path and name, the comment does not seem applicable.
The file path and name provide enough context to determine that the comment is not relevant to this specific file.
The comment is not about a change made in this diff and should be deleted.
Workflow ID: wflow_DFcP49jSYbkqMl2K
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
const browserTimezone = useMemo(() => getBrowserTimezone(), []); | ||
|
||
const [timezone, setTimezone] = useState<Timezone | undefined>( | ||
getStoredTimezoneValue() ?? browserTimezone, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: Consider introducing a configurable fallback timezone for cases where both localStorage and browserTimezone fail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SagarRajput-7, here the browserTimezone will never fail, IMO, as dayjs can always get browser's timezone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Incremental review on 5048115 in 22 seconds
More details
- Looked at
13
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. frontend/src/providers/Timezone.tsx:57
- Draft comment:
The check fortimezone.value
is good for preventing errors with undefined values. Ensuretimezone
always has avalue
property when valid. - Reason this comment was not posted:
Confidence changes required:20%
TheupdateTimezone
function correctly checks for a validtimezone.value
before proceeding. This prevents potential errors from undefined values.
Workflow ID: wflow_9hosx07IsU101fJn
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
Summary
Related Issues / PR's
Part of https://github.com/SigNoz/engineering-pod/issues/2005
Screenshots
NA
Affected Areas and Manually Tested Areas
Important
Adds timezone management with context provider, browser fallback, and local storage, updating components to reflect timezone changes.
TimezoneProvider
inproviders/Timezone.tsx
to manage timezone context.LOCALSTORAGE.PREFERRED_TIMEZONE
).CustomTimePicker.tsx
,CustomTimePickerPopoverContent.tsx
, andTimezonePicker.tsx
now useuseTimezone
hook to access timezone context.TimezoneAdaptation.tsx
allows clearing timezone override, reverting to browser timezone.CustomTimePicker
andCustomTimePickerPopoverContent
.TimezoneAdaptation
when timezone is overridden.This description was created by for 5048115. It will automatically update as commits are pushed.