FEATURE: show recent searches in quick search panel (PR #15024)

GitHub

@term contains the search term stripped of special keywords like @user or in:title and the like.

The title of this pull request changed from “FEATURE: show recent searches” to "FEATURE: show recent searches in quick search panel

Did this not work without the A? Usually it’s not required.

Approved. I made some minor comments but none are deal breakers.

Minor but maybe it would be more RESTful to say DELETE at /u/recent-searches?

This is not super important but you might be able to use the hbs helper for a template like this.

If the site setting is enabled this is an extra query per logged in user. That might be OK, but I wonder if we should consider only querying this if they use “search”? The downside is of course that search would show up slower.

Oh yes, that looks much better with the hbs helper.

Yeah, it’s a trade-off, but I’m leaning more towards what you’re suggesting, i.e. making a request when invoking search. I guess we’ll see if the delay in displaying it is a nuisance.

I know we have failed_json since a long time ago but I’ve always found returning a 200 response with an error message a very odd pattern to follow. It’ll be great if we can change this pattern if it isn’t too hard.

I don’t think we need this if we eventually pluck(:term)?

    render json: success_json.merge(recent_searches: results)

The indentation was a little off so I would just inline it.

save can return false so I think we should either handle errors while saving or use save! if we don’t expect it to fail often.

    current_user.user_option.update!(oldest_search_log_date: 1.second.ago)

Minor but we can use update too to reduce the amount of code.

I’m not 100% sure but I don’t think we need to index rows where user_id is null.

Hmm I feel this has the potential to be flaky or inaccurate. Can we perhaps set an initial value for oldest_search_log_date and check that the value changed after the delete request?

    it 'returns the right response for anon' do

Minor but I feel like the controller action is actually still doing something.

      user.user_option.update!(oldest_search_log_date: 10.seconds.ago)

I think it’ll be more descriptive if we assign 5 to a constant.