-
-
Notifications
You must be signed in to change notification settings - Fork 4
feat: song search #59
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
base: develop
Are you sure you want to change the base?
Conversation
…ponding tests - Added getFeaturedSongs method to SongService to retrieve featured songs across various timespans. - Implemented tests for getFeaturedSongs to ensure correct functionality and handling of empty results. - Removed SongBrowser module and related files as part of the refactor to streamline the service structure. - Introduced new PageDto class for pagination handling in the database module.
- Implemented various query modes in getSongList method to support fetching featured, recent, category-based, and random songs. - Added error handling for invalid query parameters and improved response types using PageDto. - Introduced searchSongs method for keyword-based song searches with pagination. - Updated Swagger documentation to reflect new query parameters and response structures.
- Introduced for improved query handling, allowing filtering by title, category, uploader, and sorting by various criteria. - Updated method in to support new query parameters and sorting logic. - Added new endpoints for fetching featured songs and available categories. - Refactored frontend components to align with updated API endpoints and enhance song search functionality. - Improved error handling and loading states in the frontend for better user experience.
- Introduced a new SearchButton component for searching songs. - Integrated the SearchButton into the Header component for improved accessibility. - Implemented search query handling and routing to the search results page.
…ld into feature/song-search
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.
Awesome work! I'm so excited to ship this feature 🤩
I've left a few considerations in the review comments; not all of them may be actionable atm.
A few points we've yet to work on:
- I'll work on touching up the frontend presentation so it's more consistent with our current design.
- Recent songs seem not to be loading locally for me
- Are the tests still supposed to be failing?
query.page, | ||
query.limit, | ||
); | ||
return new PageDto<SongPreviewDto>({ |
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.
Could this part of the code be extracted outside of the if
clause? It's repeated exactly for each query mode :)
EDIT: I mean the return new PageDto({...})
part. For some reason I highlighted completely wrong lines.
try { | ||
const response = await axiosInstance.get<FeaturedSongsDto>( | ||
'/song-browser/featured', | ||
'/song/featured', |
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.
Isn't this route supposed to clash with the GET /song/:id
endpoint? Is this currently the case, or is there some precedence order going on to prevent this from happening?
Our song IDs are 10 characters long, spanning A-Za-z0-9
, but were they 8 characters long, featured
could technically be a valid song ID.
try { | ||
const response = await axiosInstance.get<Record<string, number>>( | ||
'/song-browser/categories', | ||
'/song/categories', |
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.
oop, there we go - categories
, a perfectly valid song id! xD
Though I believe the chance of this happening is quite unlikely... 1 in 62^10 or something?
Pictures of the current frontend (courtesy of @tomast1337!): ![]() ![]() |
After exploring the search functionality extensively, I've been considering making a few changes to the search process: Server-side renderingOnce we've entered the search page, a loading screen is still shown as the search results are fetched locally after page load: ![]() Ideally, the process would be smoother: just like a loading bar appears at the top when switching to a different page from the header, the same should happen when performing the initial search, with the user only navigating to the search page after it has been fully loaded. This allows the user to continue viewing the content they're already seeing until the page has been built and is ready to be rendered. It's similar to what services like YouTube or Google do. ab097be67425aeabc5f5d8b164b5d6af.mp4This will likely involve adding server-side rendering for the initial search, which could be a bit involved. Advanced filteringI'd also like the search page to act as an 'advanced filtering' page: a second search bar appears in the song search page, as well as some dropdowns or categories allowing the user to filter the results in more interesting ways (think Jira filters): ![]() This search bar, on the other hand, would update results locally and re-render them on the client side. Right now, filtering wouldn't serve much purpose because the results are very narrow (it's the best we can do with self-managed MongoDB, and no external search engine -- see the last suggestion below). For responsiveness, we could also explore implementing real-time updates as the user types the search query, something already done in this community website: https://nbw.flwc.cc/ Lastly, the nuqs library can be implemented to make it easier to sync the query parameters with the URL. I'll explore these options and let you know how it goes! Fuzzy searchThis may be too complex/costly to implement now, but as the text search options for non-Atlas MongoDB are quite limited, we could move to a proper search engine (e.g. [Elasticsearch](https://www.elastic.co/elasticsearch, OpenSearch), with fuzzy matching support. I haven't yet assessed how close we could get to the features they're offering without incorporating another service provider. As the latter two improvements are somewhat complex, they may be implemented incrementally in future PRs. @tomast1337 Look forward to your feedback on these suggestions :) |
No description provided.