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

ref(replay/feedback): don't suggest empty tag #80353

Merged
merged 1 commit into from
Nov 8, 2024

Conversation

michellewzhang
Copy link
Member

resolves JAVASCRIPT-2WH4

before:

Untitled.mov

after:

Screen.Recording.2024-11-06.at.3.52.50.PM.mov

@michellewzhang michellewzhang requested review from a team as code owners November 6, 2024 23:55
@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Nov 6, 2024
Copy link

codecov bot commented Nov 7, 2024

Codecov Report

Attention: Patch coverage is 0% with 8 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
static/app/components/feedback/feedbackSearch.tsx 0.00% 4 Missing ⚠️
static/app/views/replays/list/replaySearchBar.tsx 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #80353      +/-   ##
==========================================
- Coverage   78.33%   77.33%   -1.01%     
==========================================
  Files        7188     7188              
  Lines      317875   317885      +10     
  Branches    43812    43813       +1     
==========================================
- Hits       249018   245822    -3196     
- Misses      62510    65550    +3040     
- Partials     6347     6513     +166     

@aliu39
Copy link
Member

aliu39 commented Nov 7, 2024

We should look into why we're storing empty tags in the first place 🤔🤔

@michellewzhang
Copy link
Member Author

We should look into why we're storing empty tags in the first place 🤔🤔

it's based on most commonly searched values

Copy link
Member

@billyvg billyvg left a comment

Choose a reason for hiding this comment

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

I think we should leave it in because it's a valid use case to look for untagged events

@aliu39
Copy link
Member

aliu39 commented Nov 7, 2024

@billyvg oh is searching for os.name="" meant to be the same as searching for replays without the os.name tag?

Just did a search and this replay with os.name=MacOS is returned.

@billyvg
Copy link
Member

billyvg commented Nov 7, 2024

@billyvg oh is searching for os.name="" meant to be the same as searching for replays without the os.name tag?

Just did a search and this replay with os.name=MacOS is returned.

No, just literal empty string -- though maybe I am making this up and it's not a valid tag value?

@michellewzhang
Copy link
Member Author

michellewzhang commented Nov 7, 2024

@billyvg oh is searching for os.name="" meant to be the same as searching for replays without the os.name tag?
Just did a search and this replay with os.name=MacOS is returned.

No, just literal empty string -- though maybe I am making this up and it's not a valid tag value?

@aliu39 @billyvg i think my reasoning for removing the empty string suggestion is more like, having an empty tag value isn't a useful suggestion for users most of the time since they probably want to search on an actual name/version - i'd rather save cognitive load and just show suggestions that are actually values. maybe "" is a valid tag to search on but it's not necessarily useful to suggest

@aliu39
Copy link
Member

aliu39 commented Nov 7, 2024

Based on the results of the search I linked above, I don't think the search for "" is valid, so I'm good with removing it. My question is still why is this value returned by the backend - I can make a note for that or open a separate ticket.

We can also, maybe, support an explicit value that indicates "I want to search for replays missing this tag". Similar to what I did for user fields a while back. But if we don't have this functionality already, that's out of scope from this pr too.

@michellewzhang
Copy link
Member Author

Based on the results of the search I linked above, I don't think the search for "" is valid, so I'm good with removing it. My question is still why is this value returned by the backend - I can make a note for that or open a separate ticket.

We can also, maybe, support an explicit value that indicates "I want to search for replays missing this tag". Similar to what I did for user fields a while back. But if we don't have this functionality already, that's out of scope from this pr too.

SCR-20241107-jtin SCR-20241107-jtjr

to me it just looks like these are the commonly searched values

@aliu39
Copy link
Member

aliu39 commented Nov 7, 2024

I worked on this endpoint for replays, these suggestions are from our database and count is the number of times a value appears in the table. Can follow up to look into this, but yeah for now, definitely seems like we shouldn't suggest these

@michellewzhang michellewzhang merged commit 09f75b6 into master Nov 8, 2024
43 of 44 checks passed
@michellewzhang michellewzhang deleted the mz/fb-replay-search-empty-tag branch November 8, 2024 19:01
@github-actions github-actions bot locked and limited conversation to collaborators Nov 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Frontend Automatically applied to PRs that change frontend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants