Conversation
funkyhippo
left a comment
funkyhippo
left a comment
There was a problem hiding this comment.
Thanks for the contribution, reviewing this (+ JS rather than TS code) was a blast-to-the-past and the equivalent to rawdogging digital junk-food.
I left a couple comments.
| if (event.key === "Enter" && event.target.value) { | ||
| let query = { | ||
| title: event.target.value, | ||
| includedTags: [], |
There was a problem hiding this comment.
I'm not sure whether this is safe to include as a default; I think it's up to the source to implement the search logic, and it's possible that the inclusion of includedTags would trigger a different search path.
I only spot-checked a single source, but the version of the NHentai source has code that checks for tags, which would result in undefined in the query because of the optional chaining. It doesn't seem like it actually breaks the searching though.
Were there other sources that had buggy implementations?
There was a problem hiding this comment.
Hey, thanks for the comments, I agree that having it included by default probably isn't a great idea.
I don't know if it's an issue on my end or not but I haven't been able to get the NH search to return anything unless the undefined was removed from the query. i.e. /api/galleries/search?query=test%20undefined&page=1 returns an empty list but /api/galleries/search?query=test&page=1 had actual results.
Scanning through the other sources I didn't see any that had issues with includedTags the same way in their search implementations. Would having an extra check in runSearchQuery to only add the empty tags array to the NH source be a better solution?
Ex:
runSearchQuery = (query) => {
Object.entries(this.props.sources).forEach(([sourceName, source]) => {
const modifiedQuery = (sourceName === "NHentai") ? { ...query, includedTags: [] } : { ...query };
const queryTask = {
source,
query: modifiedQuery,
metadata: undefined,
};
this.sourceQueryHelper(sourceName, source, queryTask);
});
};| if(this.state.itemLength + LOAD_BATCH_COUNT >= this.state.childrenLength && | ||
| this.props.onReachEnd && | ||
| !this.state.loadingMore) { |
There was a problem hiding this comment.
nit: formatting (though apologies, this repo wasn't opinionated in having the right checks in place)
| Promise.resolve(this.props.onReachEnd()).finally(() => { | ||
| this.setState({ loadingMore: false }); | ||
| }); |
There was a problem hiding this comment.
Does this behave correctly if the carousel is expanded? The component is somewhat poorly named, since it can be a magazine-like component if expanded.
| onReachEnd={ | ||
| hasMoreInSource ? async () => await this.loadMoreFromSource(source) : undefined | ||
| } |
There was a problem hiding this comment.
If we opt to implement this, we may need to update the lines above to somehow signal the number of results that are actually available:
cubari-proxy/src/containers/Search.js
Line 149 in dc146bb
Otherwise it might be misleading to the user for the number of results.
2 participants
These changes do the following two things:
1. Fix search for NH source.
adding
includedTags: []to the query object in Search.js prevents the proxy url from having %20undefined appended to it, letting the query return successfully.2. Implement TODO from Search.js.
Changes behavior in Search.js and ScrollableCarousel.js so that only 1 search is done per source, and subsequent searches are done as the user scrolls the carousel near the end.