late async search results added to the mixer last#12600
Conversation
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR changes SearchMixer so late async results are inserted at the low-priority edge instead of the high-priority edge, with an updated regression test for the basic late-result ordering case.
Concerns
- The new prepend path changes the order fed into
dedupe_score, which can move already-visible duplicate results whenDedupeStrategy::HighestScoreis enabled.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
|
||
| self.results.extend(late_results); | ||
| let mut existing_results = std::mem::take(&mut self.results); | ||
| self.results = late_results; |
There was a problem hiding this comment.
dedupe_score: with HighestScore, a lower-scored late duplicate is seen first, then replaced by the already-visible higher-scored result in that first slot, moving the existing result to the low-priority edge. Preserve existing result positions when deduping late duplicates.
moirahuang
left a comment
moirahuang
left a comment
There was a problem hiding this comment.
really appreciate you making this fix!
2 participants
Description
This PR fixes an issue where command search (ctrl-r in the terminal input) usually doesn't show any command history items. Here is the scenario:
https://www.loom.com/share/e40d832305d542d8b93eba152072237c
To summarize the video, searching a command the appears a lot in my history, e.g.
oz-dev, should turn up a lot of history items in the results. Instead, I only see notebooks.I've traced this back to the behavior added in #warpdotdev/warp-internal#22987 which AFAICT is not working as intended. The intention is that "late" async results are appended to the end in order to avoid a re-shuffle of the results.
I have confirmed that the notebooks are returning "late" (after the timeout) in the video. However, they are actually added to the beginning, not the end as intended. You can see it in this trimmed recording. The correct results flicker very quickly and then get replaced by the notebooks
show-flicker.mov
The problem is that the mixer results is stored in ascending order (highest score last). This is true whether the results are top-down or bottom-up (a separate field controls that). Underlying ordering is ascending. This means that appending to the end is actually showing the late items first, i.e. as the highest scoring items. This results in a total replacement of the results.
I have fixed this by prepending (inserting at the lowest-scoring edge) instead of appending late results.
Linked Issue
Fixes #11787
Testing
After