Search Results #446
Search Results #446
Conversation
|
[diff-counting] Significant lines: 1993. This diff might be too big! Developer leads are invited to review the code. |
There was a problem hiding this comment.
Everything looks great and it's super helpful how you were able to fix so many bugs with the current search algorithm! I also think adding the new results endpoint that is separate from the search endpoint really improves the efficiency and clarity of the search algorithm so good job with that!
For the course results endpoint maybe you can also log the error in the console log as well to make it easier to understand the errors. You could also process the search query string in sanitizeQuery with query.replace(/+/g, ' ').toLowerCase() instead in case there are multiple occurrences not just one. Something else to maybe consider to make it more efficient is parallelizing searchCoursesByProfessor and searchCourses rather than running them sequentially.
There was a problem hiding this comment.
Jacqueline, this is great work, and a really challenging task you've been given! I wasn't able to fully understand all of the changes to the search algorithm at work, but I think that your edits for processing speed by cutting down on the number of returned results and also fixing the parallelism issue is great. I left some comments and questions, but amazing job!
| @@ -21,27 +20,28 @@ const fullCourseSearch = async ({ search }: SearchQueryType) => { | |||
| const query = search.getQuery(); | |||
| let fullSearch = new Set(); // set to ensure no duplicate courses | |||
|
|
|||
| if (query !== undefined && query !== '') { | |||
| // checks query after at least 2 characters to speed up search | |||
There was a problem hiding this comment.
I think this is a smart solution, since there isn't much information to search for with just one character. It can help improve speed without actually affecting the user experience
| return sorted.slice(0, 5); | ||
| } | ||
|
|
||
| if (sorted && searchType === "results" && sorted.length > 200) { |
There was a problem hiding this comment.
What are "results" search types being used for if they provide much more information than we can display?
| } catch (e) { | ||
| return null; | ||
| } | ||
| }; | ||
|
|
||
| export const searchCoursesByProfessor = async ({ search }: SearchQueryType) => { | ||
| export const searchCoursesByProfessor = async ({ search }: SearchQueryType, searchType: string) => { |
There was a problem hiding this comment.
Where you add searchType here, which can either be "search" for 5 results or "results" for <200, would it be easier to just migrate to all "search"?
There was a problem hiding this comment.
Awesome work!!!! Glad we were able to fix most of the issues regarding the search bar.
Hopefully we can potentially work a little bit of the search results page and other minor bugs in a future PR.
Note: Most of issues were discussed in person - documented at https://www.notion.so/Search-Production-Bug-f05d0f1e00714b909ed75aed02dac09c?pvs=4
Summary
This PR works on the Search/Results feature.
PR Type
Mobile + Desktop Screenshots & Recordings
Fixed bug for queries with dashes:
QA - Test Plan
Running
Search.test.tsBreaking Changes & Notes
Added to documentation?
What GIF represents this PR?
gif