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.

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

fix: fix the issue of saved view overriding query for logs and traces #6678

Merged
merged 8 commits into from
Dec 24, 2024

Conversation

ahmadshaheer
Copy link
Collaborator

@ahmadshaheer ahmadshaheer commented Dec 19, 2024

Summary

  • Added isDefaultQuery function to determine if the current query matches the default query.
  • Integrated isDefaultQuery into ExplorerOptions to conditionally handle applying the recently used saved view based on the query type.
  • Updated types to include IsDefaultQueryProps for better type safety.

This enhancement improves the query handling logic, ensuring that only default queries are processed for recently used saved view.

Related Issues / PR's

close https://signoz-team.slack.com/archives/C02TJ466H8U/p1734874203586999
close https://github.com/SigNoz/engineering-pod/issues/2067

Previous PR: #6612

Screenshots

Before:

2024-12-11.11-26-02.mov

After:

2024-12-11.11-27-29.mov

Affected Areas

  • Logs explorer
  • Traces explorer

Manually Tested Areas

  • Visiting logs explorer from service details page (View logs) -> should respect the query and avoid applying the recently used saved view
  • Visiting traces explorer from service details page (View traces) -> should respect the query and avoid applying the recently used saved view
  • Visiting logs explorer from timeline table of alert history page (View logs) -> should respect the query and avoid applying the recently used saved view
  • Visiting logs explorer from other pages -> should apply the recently used saved view
  • Visiting traces explorer from other pages -> should apply the recently used saved view

Important

Introduces isDefaultQuery function to conditionally apply saved views in ExplorerOptions based on query type, with updated types for safety.

  • Behavior:
    • Added isDefaultQuery function in QueryBuilder.tsx to check if a query is default.
    • Integrated isDefaultQuery in ExplorerOptions.tsx to conditionally apply saved views based on query type.
  • Types:
    • Updated types in queryBuilder.ts to include IsDefaultQueryProps for type safety.

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

@github-actions github-actions bot added the bug Something isn't working label Dec 19, 2024
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 c8d349c in 2 minutes and 24 seconds

More details
  • Looked at 200 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. frontend/src/container/ExplorerOptions/ExplorerOptions.tsx:483
  • Draft comment:
    The useEffect hook is missing onMenuItemSelectHandler in its dependency array. This could lead to stale closures if onMenuItemSelectHandler changes.
  • Reason this comment was not posted:
    Comment was on unchanged code.
2. frontend/src/container/ExplorerOptions/ExplorerOptions.tsx:489
  • Draft comment:
    Consider adding a try-catch block around JSON.parse to handle potential JSON parsing errors gracefully. This is also applicable at lines 232 and 283.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_r5YGZHZVsFJO1V2Z


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

YounixM
YounixM previously approved these changes Dec 20, 2024
SagarRajput-7
SagarRajput-7 previously approved these changes Dec 21, 2024
@ahmadshaheer ahmadshaheer force-pushed the fix/saved-view-overrides-query branch from c8d349c to 33c3f28 Compare December 22, 2024 12:29
@ahmadshaheer ahmadshaheer dismissed stale reviews from YounixM and SagarRajput-7 via 7670b93 December 22, 2024 13:03
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! Incremental review on 7670b93 in 47 seconds

More details
  • Looked at 25 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. frontend/src/pages/LogsExplorer/__tests__/LogsExplorer.test.tsx:158
  • Draft comment:
    The isDefaultQuery function is mocked to always return false. Consider testing scenarios where it returns true to ensure comprehensive test coverage. This applies to TracesExplorer/__test__/testUtils.ts as well.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment suggests adding more test coverage, which could be valid. However, it's making assumptions about what needs to be tested without understanding the full context. We don't know if isDefaultQuery=true is an important case that needs testing. The comment also references another file (TracesExplorer) which violates the cross-file issues rule.
    I might be too harsh - test coverage suggestions can be valuable. The suggestion to test both true/false cases is a common testing best practice.
    While test coverage is important, we don't have enough context to know if isDefaultQuery=true is a meaningful case that needs testing. The comment also violates the rule about cross-file issues.
    Delete the comment. While suggesting test coverage is good, we don't have enough context to know if this specific case needs testing, and the comment references other files which violates our rules.
2. frontend/src/pages/LogsExplorer/__tests__/LogsExplorer.test.tsx:155
  • Draft comment:
    Avoid using inline styles in React components. Use external stylesheets, CSS classes, or styled components instead. This is also applicable in other parts of the code where inline styles are used.
  • Reason this comment was not posted:
    Comment was not on a valid diff hunk.
3. frontend/src/pages/TracesExplorer/__test__/testUtils.ts:198
  • Draft comment:
    Avoid using inline styles in React components. Use external stylesheets, CSS classes, or styled components instead. This is also applicable in other parts of the code where inline styles are used.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_a2dzBwuZrB9pL3pB


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

@ahmadshaheer ahmadshaheer enabled auto-merge (squash) December 24, 2024 15:17
@ahmadshaheer ahmadshaheer merged commit d2aa1cf into main Dec 24, 2024
15 of 16 checks passed
@ahmadshaheer ahmadshaheer deleted the fix/saved-view-overrides-query branch December 24, 2024 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants