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 search params being set in wrong place #8214

Merged
merged 13 commits into from
Apr 17, 2024

Conversation

ccschmitz
Copy link
Contributor

@ccschmitz ccschmitz commented Apr 10, 2024

Summary

We have a bug where interacting with UI in the related resource panel will manipulate the main page's search query rather than the query used for the related resource in the panel. e.g. if you are on the traces page and click on "Apply as filter" on a log attribute from the logs' related resource panel it will apply the filter on the traces' query.

My workaround for this is to move some of the search state/logic into a SearchContext that can be added around search components, then use the context (via useSearchContext) to access a setter that will update the query just for that instance of the search.

Also makes a few small tweaks to fix some issues I noticed while click testing.

  • Centers the reset ("X") button to the right of the search filters
  • Sorts the root spans so we always show the longest one first if we have two that start around the same time (temporary fix improvement until we fix our timing issues)

How did you test this change?

  • Go to the traces page
  • Open a trace
  • Click "View logs" on the trace panel
  • Expand a log and click "Apply as filter" on one of the attributes
    • Make sure this doesn't update the traces' search query

Are there any deployment considerations?

Should only fix a client-side bug. No backend changes.

Does this work require review from our design team?

N/A

Copy link

linear bot commented Apr 10, 2024

Copy link

changeset-bot bot commented Apr 10, 2024

⚠️ No Changeset found

Latest commit: a59967e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

💥 An error occurred when fetching the changed packages and changesets in this PR
Some errors occurred when validating the changesets config:
The package or glob expression "rrdom" is specified in the `ignore` option but it is not found in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.
The package or glob expression "rrdom-nodejs" is specified in the `ignore` option but it is not found in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.
The package or glob expression "rrweb" is specified in the `ignore` option but it is not found in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.
The package or glob expression "rrweb-player" is specified in the `ignore` option but it is not found in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.
The package or glob expression "rrweb-snapshot" is specified in the `ignore` option but it is not found in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.
The package or glob expression "@rrweb/types" is specified in the `ignore` option but it is not found in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.
The package or glob expression "@rrweb/web-extension" is specified in the `ignore` option but it is not found in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.
The package or glob expression "rrvideo" is specified in the `ignore` option but it is not found in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.

Copy link
Contributor

@SpennyNDaJets SpennyNDaJets left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed a lot of the log attributes were missing the filter by button. Not sure if this was expected, but only looks to be the case on the related-resource view

Screenshot 2024-04-16 at 4 36 13 PM

@ccschmitz
Copy link
Contributor Author

ccschmitz commented Apr 17, 2024

@SpennyNDaJets thanks for catching that! In that context I think we want to disable that feature entirely since we don't want people interacting with that search box. I just fixed that up and merged in your latest changes + handled conflicts. Should be ready for another look whenever you have time!

Copy link
Contributor

@SpennyNDaJets SpennyNDaJets left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Click test looks good to me!

@ccschmitz ccschmitz merged commit d18cb62 into main Apr 17, 2024
13 checks passed
@ccschmitz ccschmitz deleted the hig-4470-search-form-on-related-logs-panel-broken branch April 17, 2024 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants