Skip to content
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

fix(queries): Ignore duplicate recent queries COMPASS-2237 #3171

Merged
merged 3 commits into from Jun 9, 2022

Conversation

lerouxb
Copy link
Contributor

@lerouxb lerouxb commented Jun 7, 2022

This won't save a query that already exists in the recents again. It also moves a bunch of tests that were in test/renderer and therefore ignored to the right places so they run again.

@github-actions github-actions bot added the fix label Jun 7, 2022
Copy link
Member

@Anemy Anemy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does solve the duplicate entries pain point, however I think as a user I'd expect/like if the recent query I use goes to the top of the recents list when it's used.
Is it possible to update the _lastExecuted entry for the matching recent when a query is performed?

@@ -25,7 +24,9 @@ const configureStore = (options = {}) => {
* Filter attributes that aren't query fields or have default/empty values.
* @param {object} attributes
*/
_filterDefaults(attributes) {
_filterDefaults(_attributes) {
// don't mutate the passed in parameter
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lost some time here. My test that added the same query twice actually never executed the code that would check if it is a duplicate because this stripped ns from the query the first time, causing it to early return for a different reason.

I think it is just better style to not implicitly modify things that came in from the outside.

return isDeepStrictEqual(_item.filter, query.filter);
const existingQuery = this.state.items
.find((item) => {
return _.isEqual(comparableQuery(item), query);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found a second copy of this logic / a third place that could benefit from the new function.

Copy link
Member

@Anemy Anemy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@lerouxb lerouxb merged commit ef43bdc into main Jun 9, 2022
@lerouxb lerouxb deleted the save-query-once branch June 9, 2022 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants