Add basic text search of conversations

[mattlindsey]

Basic search returns conversations with the query text in either the conversation title or message text.

I just need to clean up the UI a bit, and maybe try to add the automatic search after pause of more than X00 milliseconds, as requested in #56.

[mattlindsey]

I think this PR might be ready.

P.S. Fixed styling issues.

[krschacht]

This is a great feature, thanks for taking this on! I tried it out and noticed some subtle interaction things:

• When I click in the input, type a search, and then press enter it executes the search but the cursor moves back to the beginning of the line. I think the cursor should stay at the end.
• After executing a search, if I want to get my conversations back I click the "X" and the search clears but my conversations don't reappear. I have to press return within the empty search box in order to get all my conversations. That's a little unintuitive. I think it's okay for search to only execute when I press return, however I think when I click the "X" or when I clear all input manually (with just backspace) then the instant the search input is blank it should immediately re-execute the blank search without needing to press enter.
• When you have text input entered into the search and you click elsewhere on the page to unfocus the search input, the "X" disappears from the search box. I think it should stay in there so long as a search query is present.
• Similarly, if I enter a search input into the box and don't press return but click elsewhere on the page to unfocus, I think we should execute the search as if you just pressed return. That way you never end up in a state where you come back to the page, see a search input yet that search filter is not actually in effect.
• Let's add a cursor-pointer on the "X" icon so that you get a nice visual indicator that it's clickable.
• The search input is stealing focus. Try creating a brand new conversation. As soon as you do, the cursor autofocuses into the chat input. Type your question and press return and the page refreshes with your response. But after this page refresh it's supposed to keep the focus within the chat input so you can keep asking follow up questions without having to explicitly click to refocus the chat input. This has been working but now that we have the conversation search, it's stealing the focus so you have to re-click the chat input to ask your next question.

That's it on the interaction / usability stuff. The one styling thing that would be nice is to mimic the ChatGPT iPad app for visual design where it's in the top left. If this is hard we don't have to do it, but it would be nice:

[mattlindsey]

• When I click in the input, type a search, and then press enter it executes the search but the cursor moves back to the beginning of the line. I think the cursor should stay at the end.
• After executing a search, if I want to get my conversations back I click the "X" and the search clears but my conversations don't reappear. I have to press return within the empty search box in order to get all my conversations. That's a little unintuitive. I think it's okay for search to only execute when I press return, however I think when I click the "X" or when I clear all input manually (with just backspace) then the instant the search input is blank it should immediately re-execute the blank search without needing to press enter.
• When you have text input entered into the search and you click elsewhere on the page to unfocus the search input, the "X" disappears from the search box. I think it should stay in there so long as a search query is present.
• Similarly, if I enter a search input into the box and don't press return but click elsewhere on the page to unfocus, I think we should execute the search as if you just pressed return. That way you never end up in a state where you come back to the page, see a search input yet that search filter is not actually in effect.
• Let's add a cursor-pointer on the "X" icon so that you get a nice visual indicator that it's clickable.
• The search input is stealing focus. Try creating a brand new conversation. As soon as you do, the cursor autofocuses into the chat input. Type your question and press return and the page refreshes with your response. But after this page refresh it's supposed to keep the focus within the chat input so you can keep asking follow up questions without having to explicitly click to refocus the chat input. This has been working but now that we have the conversation search, it's stealing the focus so you have to re-click the chat input to ask your next question.
• ADDED: Auto execute search after 1000 ms. Adding this also completes a couple if the issues above.

P.S. I'm not sure how to do the 3 issues that talk about the X in the search box. I'm not sure how to access the icon.

[krschacht]

@mattlindsey Ah, I didn’t realize that was the browser control. Change the input type from.search_field to .text_field and theX will disappear. Annoyingly, this is one of the cross-browser differences so we need to introduce our own control.

You can add your own control by wrapping the text input in a div with the relative class, then inside that div you can will be the text input followed by an additional icon (the <%= icon … helper). Position the icon with absolute right-0 and it should then appear on top of the text input. Then you can add a right padding on the text input to ensure the text ends before the positioning of the X icon. That will ensure that long text does not appear underneath the icon.

Looking at the heroicons.com page, I think the “mini” x-circle is the standard one that apps use.

[mattlindsey]

@krschacht Cool. I added the icon after the text box, which looks good. I think everthing works and looks good now, unless you see anything else?

[mattlindsey]

@krschacht I added a few more minor fixes, so I think this is ready now. Also the other google_search tool PR should be ready too.

[krschacht]

@mattlindsey I merged in the google_search and pulled this one down to give it another run through. I'm still getting some interaction issues so I dug into the code a bit more. I see that you're using inline-edit stimulus controller, but we shouldn't use that one. That's a very specific use case where static text gets swapped out for a text input, and then it swaps back to the static text. This is used, for example, when you edit a conversation's title. But I think we're overloading that controller by trying to use it here. Let's create a new stimulus controller for this search input so each can stay focused on a specific feature.

The specific issues I'm running into:

• When I start a new conversation it steals focus. I'm pressing Cmd+J to trigger a new conversation and expecting the focus to be in the chat message composer.
• When I search for something and that search is executed (either b/c of 1 second delay or b/c I press return), we should keep the focus within the text input so I can continue modifying the search. For example, I may type "the" and the search executes, I see too many results, so I just want to add "theater" to narrow the results down more.
• I tweaked the styling so the clear "X" is inside the text input, but I think it should only appear when text is entered and if I click it to clear the text (or backspace to manually clear all text) then it should disappear.
• Also, when I click the "X" to clear the input, we should auto-focus inside the text input. I tested a few other search input clearings they all behaved that way, so I think that's probably the norm and what people will expect (e.g. google)
• On mobile (portrait) when the search executes the page jumps and the nav column auto closes itself. It could be that fixing the above issues automatically fixes this, or this might be a different issue, I'm not sure. Here is a video so you can see what I mean:

ScreenRecording_12-06-2024.22-15-18_1.MP4

Sorry for all this feedback! I hope this doesn't come across as too nitpicky and discourage you. Getting the UX really polished and working well always has a long tail of small issues. But it's always worth it in the end and helps the app really feel smooth to everyone using it.

[mattlindsey]

Hey. No problem, but the autofocus: @query.nil? ? false : true, line you took out clears up most of those problems. Is there a reason you took it out?

[krschacht]

Hey. No problem, but the autofocus: @query.nil? ? false : true, line you took out clears up most of those problems. Is there a reason you took it out?

Ohhh, I didn’t necessarily mean to take that out. Sorry about that. While I was testing I tried changing it to autofocus: false and I tried removing it completely to see if that fixed it, but in both those cases it still auto focused when I created a new conversation. That made me think the line wasn’t doing anything at all, but I overlooked the fact that it was addressing some of those other situations. :) Feel free to add that back in.

[mattlindsey]

@krschacht I think that I fixed all of the issues except the last one in your video. I don't understand exactly how that nav column works. Could it be something to do with Current.user.preferences[:nav_closed]?

[krschacht]

@krschacht I think that I fixed all of the issues except the last one in your video. I don't understand exactly how that nav column works. Could it be something to do with Current.user.preferences[:nav_closed]?

I tried it out again and the improvements are nice. Few things:

1. There is something funny with the "X". If you start a new conversation (Cmd+J) and submit a chat message, when the page refreshes the cursor properly stays focused in the chat input but the "X" appears within the search input even though there is no text inside of it and no interaction has been had with it.

2. The other thing that's odd is when I'm viewing a conversation and I type something into the search, my conversation disappears. It looks like it's automatically changing the full URL back to the assistant/new route.

I'm not sure about the 1st issue, but I think the 2nd issue is related to the nav column on mobile. I didn't realize you were doing a full page submit and when that happens, that's what's causing the nav to close. You can ignore the Current.user.preferences[:nav_closed] as that is related to permanently collapsing the nav on desktop. Instead, on mobile portrait, when you tap the hamburger menu button the mobile-nav-overlay is made to appear with CSS. But since a whole page refresh is happening it's losing that bit of state.

I think the right way to fix this is a bit of restructuring. The key thing that needs to happen is the left nav needs to be able to be updated independently of a full page refresh. We need to wrap the left nav in a frame and replace the contents of the frame when a search is executed. The easiest way to do this is to have a dedicated endpoint. Here is what I would suggest:

• Create a conversation#index action which just returns a simple view consisting of the the search box and the conversation list beneath the search box.
• Get this view fully working so that when you hit http://localhost:3000/conversations and see it, typing into the search box submits back to /conversations?query=...
• Now you can refactor the two views which show the conversation list. Those are http://localhost:3000/assistants/*/messages/new (I think this is messages#new) and http://localhost:3000/conversations/*/messages (I think this is messages#index). This would probably be a good use of lazy-loading turbo frame, so these views will load with just a <turbo-frame ... in the left nav. You would actually remove any setting of @nav_conversations for this messages#new messages#index. Right after the views render they'll immediately make a lazy-loading request to conversations#index. Then we just need to make sure that when a user submits the form within here that the submit simply replaces the turbo-frame, this should be the default behavior. But, of course, we want clicking the conversation title in the left nav to still open in the main area but it looks like that's already the case. I just checked _conversation and I see it linking to data: { turbo_frame: "_top" } in order to break out of the turbo frame.

One consequence of this is that the user will never see ?query= at the top-level URL since that URL will always be within the turbo-frame, but I think that's okay. It means a user would not be able to bookmark or direct link someone to a conversation search result list, but I think that's okay. That does not feel like a use case that matters within the context of this app.

Let me know if this approach sounds reasonable. I'm not certain about this, I just came up with it while poking around your code and thinking about the remaining UX challenges. There may be some downsides to this that I'm not anticipating, but I think/hope it would actually solve things in an elegant way. Some times this new Rails Turbo way of thinking is a bit of a brain teaser. But I think this refactor would basically solve issue (1), (2), and the remaining mobile nav closing (I'll call that 3).

[mattlindsey]

@krschacht That approach sounds good.
Or I could keep the full page refresh, stay on the show route instead of new, and perform the search on the current conversation hi-lighting the first occurrence. I might try that quick, then go to your approach if that seems clunky or not useful. I'm slow, so your approach will take a little while. LOL.

[mattlindsey]

I fixed issue 2 above by setting the form URL conditionally here:

hostedgpt/app/views/messages/_nav_column.html.erb

Line 32 in bc13528

<% if @conversation.present? %>

But that's not the desired solution since it still does a full page refresh. I can work on the Rails Turbo version next week.

[mattlindsey]

@krschacht I think this PR is ready now.
Should I add the test below, or is it better as a system test? It passes if you add it at the end of conversation_test.rb:

  test "#grouped_by_increasing_time_interval_for_user with a query" do
    user = users(:keith)
    query = "test"

    grouped_conversations = Conversation.grouped_by_increasing_time_interval_for_user(user, query)

    # Total conversations from all prior tests. 2 with matching titles plus 1 other with a matching message.
    assert_equal 3, grouped_conversations.values.flatten.count
  end

P.S. It seems to always pass, but needs to run last and shouldn't order be irrelevant? I don't see an 'after others' option in minitest.

[krschacht]

@krschacht I think this PR is ready now. Should I add the test below, or is it better as a system test? It passes if you add it at the end of conversation_test.rb:

  test "#grouped_by_increasing_time_interval_for_user with a query" do
    user = users(:keith)
    query = "test"

    grouped_conversations = Conversation.grouped_by_increasing_time_interval_for_user(user, query)

    # Total conversations from all prior tests. 2 with matching titles plus 1 other with a matching message.
    assert_equal 3, grouped_conversations.values.flatten.count
  end

P.S. It seems to always pass, but needs to run last and shouldn't order be irrelevant? I don't see an 'after others' option in minitest.

Yes, I think it's worth having this test. In fact, maybe text the 3 possible cases:

A) A search query only matches a message of a conversation
B) A search query only matches a title of a conversation
C) A search query matches the message of one conversation and the title of another conversation — both conversations are returned

[krschacht]

Yay, nice job restructuring this! The code really ended up pretty clean. And it looks like it helped resolve most of the issues. I did a bunch of testing and I notice two small UX bugs, but I think I was able to fix both of those.

1. Open an existing conversation, then type some text into the search box, then go back to the composer in your existing conversation and submit a message. After that chat goes through, the search box steals focus back even though you were chatting with the model.

2. Enter text into the search box, select one of the conversations from the newly filtered list, then type submit a chat message on this conversation. After that chat goes through, the search area resets losing your search and the filtered list.

The root of the problem is that we basically want this whole search area to be a persistent element and not reload in any case except when the form is submitted, but as the page morphs it's being morphed. We can solve both these issues and simplify some javascript by just making this thing persist. I'll add inline comments so you see what I mean.

[mattlindsey]

Yay, nice job restructuring this! The code really ended up pretty clean. And it looks like it helped resolve most of the issues. I did a bunch of testing and I notice two small UX bugs, but I think I was able to fix both of those.

@krschacht Thanks! I made all of those changes and think it looks good.

P.S. Actually I just tried it again and it looks like our most recent changes is making the sidebar refresh before and after sending each message. Let me look at that. I thought the morphing thing fixed that.
Yeah... Looks like the turbo-permanent isn't quite working yet. This was working with the refresh="morph" so I'm not sure how to fix it and still get the behavior you want (submitting a message on existing conversation AND retain the search results).

[krschacht]

I needed to change this from connect() to inputTargetConnected() but according to stimulus docs that should not make a difference. I suspect we may be working around a stimulus bug. I filed a ticket here: hotwired/turbo#1351

[krschacht]

P.S. Actually I just tried it again and it looks like our most recent changes is making the sidebar refresh before and after sending each message. Let me look at that. I thought the morphing thing fixed that. Yeah... Looks like the turbo-permanent isn't quite working yet. This was working with the refresh="morph" so I'm not sure how to fix it and still get the behavior you want (submitting a message on existing conversation AND retain the search results).

@mattlindsey this was because you put a permanent on the reload of the turbo-frame but not on the initial load. I added it here: 6ed2255 (#576)

After more playing with things I was getting one more subtle bug: sometimes the X was not properly disappearing after you cleared the input. I fixed this by calling setSearchClearIcon() any time the search input changed but then I noticed an occasional javascript exception in console which really confused me. I eventually fixed it by changing connect() to inputTargetConnected(). It's in this commit: 2e73c04 (#576)

I did a couple other really tiny commits to clean up some code. I think your editor is not configured to use the .editorconfig file in the root of the project. It's worth installing an IDE plugin for this and then we'll never have subtle differences in whitespace.

I'll go ahead and click merge now. This JS stuff is really hard to get right. I'm really undecided on what I think of this whole turbo/stimulus pattern that's part of rails now. Most of the time it works pretty well, but working through the various UX issues on this PR is a reminder that when it fails to do exactly what we want it's often quite difficult to figure out how to make it right.

[mattlindsey]

@krschacht Yeah. I'm having a hard time with the new stuff!

The only problem I noticed with making the Search box and conversation list 'perminant' now is it no longer adds a brand new conversation (and generated title) to the list right away like it did before. So maybe add an Issue to fix that?

[krschacht]

Oh shoot! Good catch. I just pushed a quick hotfix to main to unbreak that. I guess I should have just accepted the constraint that: if you've conducted a search in the left column but then you engage in a conversation, the search will clear.

But ugh, now it's doing that flickering after each chat message. This is what you were referring to with the morph. Do we need to add that back too?

Now I'm worried I made things worse rather than better. Main is unbroken, but maybe we do need a new PR. Ugh... Turbo/Stimulus! Just when I think I wrap my head around it, there is some new subtle interaction.