Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(logs): centralize time range handling in DateTimeSelectionV2 #7175

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(logs): centralize time range handling in DateTimeSelectionV2 #7175

wants to merge 1 commit into from
+24 −74

Conversation

ahmadshaheer
Copy link
Collaborator

@ahmadshaheer ahmadshaheer commented Feb 23, 2025 •
edited
Loading

Summary

Time range selection was not updating correctly when logs explorer query_range returned empty results. This was caused by custom time range handling logic in logs components that bypassed the time range handling in DateTimeSelectionV2 component.

  • Remove custom time range handling logic from logs components
  • Use unified time range selection through DateTimeSelectionV2 component

Related Issues / PR's

close #7163

Screenshots

Before:

2025-02-23.19-00-26.mov

After:

2025-02-23.19-01-04.mov

Affected Areas and Manually Tested Areas

Important

Centralizes time range handling in DateTimeSelectionV2, removing custom logic from logs components.

  • Behavior:
    • Centralizes time range handling in DateTimeSelectionV2.
    • Removes custom time range logic from LogsExplorerViews and useCopyLogLink.
  • Code Changes:
    • Removes onTimeRangeChange from useCopyLogLink and useLogsData.
    • Deletes LogTimeRange type from types.ts.
    • Simplifies URL query handling in DateTimeSelectionV2 by removing conditional logic for logs explorer.
  • Misc:
    • Minor refactoring in DateTimeSelectionV2 to remove unused imports and variables.

This description was created by Ellipsis for f8c2788. It will automatically update as commits are pushed.

@ahmadshaheer
fix(logs): centralize time range handling in DateTimeSelectionV2
f8c2788 
- Remove custom time range handling logic from logs components
- Use unified time range selection through DateTimeSelectionV2 component
@github-actions github-actions bot added the bug Something isn't working label Feb 23, 2025
ellipsis-dev[bot]
ellipsis-dev bot reviewed Feb 23, 2025
View reviewed changes
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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! Reviewed everything up to f8c2788 in 1 minute and 0 seconds

More details
  • Looked at 268 lines of code in 5 files
  • Skipped 0 files when reviewing.
  • Skipped posting 12 drafted comments based on config settings.
1. frontend/src/container/LogsExplorerViews/index.tsx:102
  • Draft comment:
    Good removal of the onTimeRangeChange logic. Ensure that unified time range changes are fully handled in DateTimeSelectionV2 now.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
2. frontend/src/container/TopNav/DateTimeSelectionV2/index.tsx:762
  • Draft comment:
    Avoid inline styles (e.g., using style={{ marginRight: 12 }}). Use CSS classes or styled components to ensure design consistency.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. frontend/src/hooks/logs/types.ts:5
  • Draft comment:
    Removal of pageSize field from LogTimeRange looks intentional. Verify that no dependent code still relies on it.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
4. frontend/src/hooks/logs/useCopyLogLink.ts:44
  • Draft comment:
    onLogCopy implementation is straightforward. Verify that URL query manipulation aligns with new time range centralization.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
5. frontend/src/hooks/useLogsData.ts:151
  • Draft comment:
    Ensure that merging of logs in useEffect([data]) properly handles resetting when a new query is issued.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
6. frontend/src/container/LogsExplorerViews/index.tsx:102
  • Draft comment:
    Removal of the onTimeRangeChange prop centralizes time range updates to DateTimeSelectionV2. Ensure that all side‐effects on time changes are now solely handled by the unified component.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
7. frontend/src/container/TopNav/DateTimeSelectionV2/index.tsx:317
  • Draft comment:
    The conditional branch for non-logs explorer pages was removed to unify URL updates. Verify that this unified handling correctly updates URL parameters for all pages.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
8. frontend/src/container/TopNav/DateTimeSelectionV2/index.tsx:762
  • Draft comment:
    Avoid using inline styles (e.g. marginRight inline here). Move this styling to a CSS class or styled component to maintain design and theming consistency.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. frontend/src/hooks/logs/types.ts:5
  • Draft comment:
    The unused 'pageSize' property has been removed from LogTimeRange. Ensure that all consumers of this type are updated accordingly.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is asking the PR author to ensure that all consumers of the type are updated, which violates the rule against asking the author to ensure things are tested or reviewed. It doesn't provide a specific suggestion or point out a specific issue.
10. frontend/src/container/LogsExplorerViews/index.tsx:459
  • Draft comment:
    Typographical suggestion: In the error message ('Panel limit exceeded for ... Please checkout our paid plans'), consider changing 'checkout' to 'check out' for improved clarity.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. frontend/src/container/TopNav/DateTimeSelectionV2/index.tsx:93
  • Draft comment:
    Typo: The state variable 'isValidteRelativeTime' appears to be misspelled. Consider renaming it to 'isValidRelativeTime' for clarity.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
12. frontend/src/container/TopNav/DateTimeSelectionV2/index.tsx:293
  • Draft comment:
    Typo: The variable 'minutedDiff' is likely a misspelling of 'minutesDiff'. Consider renaming it to improve readability.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_0Jc0QeJ6JG7zegYc

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ellipsis-dev ellipsis-dev[bot] ellipsis-dev[bot] left review comments

@YounixM YounixM Awaiting requested review from YounixM YounixM is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

LOGS[FE]: Timestamp filter change not working.
1 participant
@ahmadshaheer