fix(new-webui): Drop timezone from Dates returned by antd's DatePicker.RangePicker to match timezone (UTC) shown in search results (fixes #1085).#1053
Conversation
WalkthroughThe update modifies the TimeRangeInput component’s DatePicker.RangePicker by removing the Changes
Suggested reviewers
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
⚙️ Source: CodeRabbit Configuration File List of files the instruction was applied to:
🧠 Learnings (2)📓 Common learningscomponents/webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/index.tsx (3)🔇 Additional comments (1)
✨ 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/index.tsx(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>.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
components/webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/index.tsx
🧠 Learnings (2)
📓 Common learnings
Learnt from: gibber9809
PR: y-scope/clp#504
File: components/core/src/clp_s/search/kql/CMakeLists.txt:29-29
Timestamp: 2024-10-22T15:36:04.655Z
Learning: When reviewing pull requests, focus on the changes within the PR and avoid commenting on issues outside the scope of the PR.
components/webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/index.tsx (1)
Learnt from: junhaoliao
PR: y-scope/clp#937
File: components/log-viewer-webui/client/src/AntdApp.tsx:16-24
Timestamp: 2025-05-29T20:33:40.653Z
Learning: In components/log-viewer-webui React codebase: Return type annotations (like `: JSX.Element`) are unnecessary and not preferred for React components in JSX/TSX files.
|
|
||
| // `ShowNow` and `allowClear` options are disabled since they set the timezone | ||
| // of the range picker to the local timezone. | ||
| allowClear={false} | ||
| className={styles["rangePicker"] || ""} | ||
| showNow={true} | ||
| showNow={false} |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Minor wording & redundancy nitpick
- The explanatory comment uses
ShowNow, but the prop isshowNow; updating the casing will avoid confusion for future readers. showNowalready defaults tofalsein antd’sRangePicker, so the explicitshowNow={false}is technically redundant. Removing it would shorten the JSX without altering behaviour.
- // `ShowNow` and `allowClear` options are disabled since they set the timezone
- // of the range picker to the local timezone.
+ // `showNow` and `allowClear` are disabled because enabling either forces
+ // the picker to adopt the local timezone.
allowClear={false}
- className={styles["rangePicker"] || ""}
- showNow={false}
+ className={styles["rangePicker"] || ""}Optional clean-up only; functional change is not required.
📝 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.
| // `ShowNow` and `allowClear` options are disabled since they set the timezone | |
| // of the range picker to the local timezone. | |
| allowClear={false} | |
| className={styles["rangePicker"] || ""} | |
| showNow={true} | |
| showNow={false} | |
| // `showNow` and `allowClear` are disabled because enabling either forces | |
| // the picker to adopt the local timezone. | |
| allowClear={false} | |
| className={styles["rangePicker"] || ""} |
🤖 Prompt for AI Agents
In
components/webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/index.tsx
around lines 71 to 76, update the comment to use the correct prop casing
"showNow" instead of "ShowNow" to avoid confusion, and remove the explicit
showNow={false} prop since it defaults to false in antd's RangePicker, thereby
simplifying the JSX without changing behavior.
How'd you figure that out? |
I tested. If you look at logs from the UI in the console. You will see that the submitted timestamp is 4 hours off (after you clear it). The now option was also using my local time, and not the utc time. |
…meRangeInput/index.tsx Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
|
Entering a timestamp by typing also seems to trigger the issue. Any ideas how we could fix/mitigate that as well? |
This is kind of annoying. Maybe we do a gross manipulation (i think the timeline does something like this) when updating the state ? I think you can use It dosent seem antd provides good support for timezones. Originally i wanted to avoid this, and I thought setting the starting timestamp in utc was elegant fix, but it turns out it dosent work. we will still have to drop "now" feature since it will be local time, but allow clear should be fine |
Sounds reasonable for now. |
|
@kirkrodrigues - i made the fix. It appears to work, overall it is not that messy. |
kirkrodrigues
left a comment
There was a problem hiding this comment.
Could you update the PR description about why we removed showNow as well as the updated validation steps?
…meRangeInput/index.tsx Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
Dates returned by antd's DatePicker.RangePicker to match timezone (UTC) shown in search results (fixes #1085).
Dates returned by antd's DatePicker.RangePicker to match timezone (UTC) shown in search results (fixes #1085).Date returned by antd's DatePicker.RangePicker to match timezone (UTC) shown in search results (fixes #1085).
Date returned by antd's DatePicker.RangePicker to match timezone (UTC) shown in search results (fixes #1085).Dates returned by antd's DatePicker.RangePicker to match timezone (UTC) shown in search results (fixes #1085).
…cker.RangePicker` to match timezone (UTC) shown in search results (fixes y-scope#1085). (y-scope#1053) Co-authored-by: Marco <david.marcovitch@yscope.com> Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
Description
@kirkrodrigues mentioned a bug that the range picker is not accurately selecting dates.
PR fixes this issue by dropping any timezone offset supplied by antd range picker.
PR also drops
showNowoptions since it selects the time in the local timezone. This does not work for our design since the whole in UI is in utc.Checklist
breaking change.
Validation performed
tested the submitted timestamp in utc is the same when (1) using the selector, and (2) when clearing the timestamp first then using the selector.
Summary by CodeRabbit