Display filteredPool when AI search query is absent#4791
Conversation
ellenmuller
changed the title
joelochlann
left a comment
joelochlann
left a comment
There was a problem hiding this comment.
Nice! Functionality looks great 👌
Only UI thing that surprised me was: I did actually expect the badges (GNM-owned, agency picks) even in the filters-only mode, so that it's literally just the "Best 200 of" popping in when you add a query, i.e.
1,178,998 matches [4,705 GNM-owned] [5,960 agency picks]
to
Best 200 of 1,178,998 matches [4,705 GNM-owned] [5,960 agency picks]
But it doesn't matter much so it's only worth changing if it simplifies code or if someone else agrees with me.
On the code side, no blockers to this PR, but I'd definitely suggest having a go at simplifying the logic for distinguishing "normal AI search" from "filters-only AI search" from "an actual error", starting with #4752 (comment) which I think you'll find satisfying and will simplify some boilerplate in the tests. If anything's unclear about that let me know and we could do a quick half an hour's pairing to align
| image-results-count"> | ||
| <span ng-if="ctrl.isAiSearch">Best {{ctrl.aiResultsShown | toLocaleString}} of {{ctrl.totalResults | toLocaleString}} matches</span> | ||
| <span ng-if="! ctrl.isAiSearch">{{ctrl.totalResults | toLocaleString}} matches</span> | ||
| <span ng-if="ctrl.isAiSearch && !ctrl.filtersOnlyAiSearch">Best {{ctrl.aiResultsShown | toLocaleString}} of {{ctrl.totalResults | toLocaleString}} matches</span> |
There was a problem hiding this comment.
Could be clearer to wrap in a <span ng-if="ctrl.isAiSearch> since they both contain ctrl.isAiSearch condition?
| // prompt the user to add a query. | ||
| case _ => | ||
| emptyAiSearchResponse | ||
| filtersOnlyAiSearchResponse(_searchParams) |
There was a problem hiding this comment.
Not a blocker, but I'm pretty sure there's an opportunity to simplify this, especially if you start by implementing this suggestion: #4752 (comment)
Basically if you make AiQueryParts.from do all the parsing and error handling, it will either give you a Left containing the reason why it can't do the search, or a Right with the parsed stuff ready to do a query. Then you should be able to call filtersOnlyAiSearchResponse in just one place, instead of three.
You could also make the Left be a union type (a sealed trait with some case objects) so you could pattern match on it to distinguish the expected cases you want to handle with your filters-only search, from the ones that are genuine error conditions.
| case Left(_) => | ||
| if (params.aiQueryParts.hasFilterConditions) { | ||
| logger.info(logMarker, s"AI search with only filters and no query to rank by; returning filter pool counts: ${params.aiQueryParts.filterConditions}") | ||
| filtersOnlyAiSearchResponse(params) |
There was a problem hiding this comment.
both these if/else branches contain filtersOnlyAiSearchResponse, maybe best to pull out
2 participants
What does this change?
When AI search mode has no ranking query, the UI previously showed two different empty states — one for "no query at all" and one for "filters but no query". This unifies them into a single state that tells the user how many images are in the pool a query would rank over.