-
-
Notifications
You must be signed in to change notification settings - Fork 59
devex: Refactor components to use org search context #2880
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
Conversation
24390ad
to
4ebc5eb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good overall! I think this is a nice step towards easier access to search across the app — and if/when we have some sort of backend search service/tool/whatever it'll likely be easy to drop in fetches to that instead of Fuse. Got a few questions about some things, but nothing that necessarily should prevent a merge. Nice work!
} | ||
|
||
const results = this.searchOrg.collections | ||
?.search(this.searchByValue) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small thing, but we might want to put a limit on the results returned from fuse for perf reasons. Conservatively, we could probably do 5 + the number of items that have been selected?
?.search(this.searchByValue) | |
?.search(this.searchByValue, { | |
limit: 5 + this.nameSearchMap.keys.length, | |
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call on limit, addressed the selected items by enabling extended search patterns: 077bb97
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a clever approach, but there are some edge cases that break it. For example, if a collection's name starts with "
then it will be the only search result that ever shows up if it's selected as an auto-add collection for a workflow:
Screen.Recording.2025-10-08.at.6.34.19.PM.mov
I don't know for sure how often this'll come up, but it doesn't seem entirely unfeasible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm yeah I can't think of a user realistically having a collection named " <something>
since it's the space after the double quote that breaks search, not just starting with double quotes. Quotes in the middle of the name break as well. Looking to see if there's a quick fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this has an edge case that breaks the search, see my comment here: #2880 (comment)
Nice, looking good! |
Precedes #2862
Changes
In preparation for #2862, the client-side search index for org-wide collection names has been moved to a new
searchOrgContext
. The context currently only supports fuzzy searching by collection name.Also addresses feedback in #2871
Manual testing
Log in as a crawler and regression test the following:
Follow-ups
Left this implementation generic so that it's fairly simple to incorporate or migrate to https://github.com/webrecorder/browsertrix/tree/frontend-data-fetching-exploration in the future.