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

feat: enabled loading an existing conversation in StarSearch #3563

Merged
merged 26 commits into from
Jun 18, 2024

Conversation

nickytonline
Copy link
Member

@nickytonline nickytonline commented Jun 13, 2024

Description

Now you can share an existing thread (conversation in StarSearch). We had a rudimentary version of this previously (see #3324), but it was just sharing the prompt and running it.

These shared conversations are the full conversation that gets replayed and does not hit our AI backend at all.

If you are not logged in, you will only be able to replay it once (unless the page is refreshed in the browser) and any other actions will require you to log in.

One thing to note is if you generate a shared URL, it will generate for beta.app.opensauced.pizza on beta and deploy previews, and one in prod, will be prod links. All that said, you'll just need to replace the beta base URL with the deployment preview URL.

TODO:

  • fix flash of content when loading a conversation
    - fix OG image for a shareable StarSearch conversation
    - fix Server-side request forgery warnings

Note: This PR adds Valibot to the project. It is MIT licensed and a small package (1kb). It's used for some validation in this PR and I plan to use it elsewhere in the application long term.

Related Tickets & Documents

Closes #3551

Mobile & Desktop Screenshots/Recordings

Steps to QA

Here's a few share links you can try:

Associated OG image, https://deploy-preview-3563--oss-insights.netlify.app/og-images/star-search/?share_id=900686cc-8926-47fd-a1d6-0e19da967f48

image

Associated OG image, https://deploy-preview-3563--oss-insights.netlify.app/og-images/star-search/?share_id=ff9b26b7-64e7-460b-9d33-252543bee24d

image

--

Also try and create a new conversation then try to share it.

Tier (staff will fill in)

  • Tier 1
  • Tier 2
  • Tier 3
  • Tier 4

[optional] What gif best describes this PR or how it makes you feel?

Copy link

netlify bot commented Jun 13, 2024

Deploy Preview for oss-insights ready!

Name Link
🔨 Latest commit 5faacee
🔍 Latest deploy log https://app.netlify.com/sites/oss-insights/deploys/6671a45ecd50250008ad44ab
😎 Deploy Preview https://deploy-preview-3563--oss-insights.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Jun 13, 2024

Deploy Preview for design-insights ready!

Name Link
🔨 Latest commit 5faacee
🔍 Latest deploy log https://app.netlify.com/sites/design-insights/deploys/6671a45e8252ba00094b0e81
😎 Deploy Preview https://deploy-preview-3563--design-insights.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

components/StarSearch/StarSearchChat.tsx Fixed Show resolved Hide resolved
components/StarSearch/StarSearchChat.tsx Fixed Show resolved Hide resolved
@BekahHW
Copy link
Member

BekahHW commented Jun 14, 2024

I'm a little bit confused by this:

image

We're making it a two-step process to share and I think that's adding some friction. We already have a note at the bottom of the links that it makes it public. Can we remove this initial message?

One other thing is that when you load the shareable link, we see the starsearch input screen flash first. Could we do a loader or something else? Not sure if that's just bc of having to move between beta and deploy preview.

Screen.Recording.2024-06-14.at.4.45.44.PM.mov

But this is looking great! Super excited to get this shipped.

@nickytonline
Copy link
Member Author

I'm a little bit confused by this:

image

We're making it a two-step process to share and I think that's adding some friction. We already have a note at the bottom of the links that it makes it public. Can we remove this initial message?

One other thing is that when you load the shareable link, we see the starsearch input screen flash first. Could we do a loader or something else? Not sure if that's just bc of having to move between beta and deploy preview.

Screen.Recording.2024-06-14.at.4.45.44.PM.mov
But this is looking great! Super excited to get this shipped.

I'm taking a page from the ChatGPT book here. They have update link, but what they're doing is generating a public link.

CleanShot 2024-06-14 at 16 59 22

This wasn't part of the flow originally when I chatted with @isabensusan a couple days ago. The reason this flow is necessary is we run into issues with browser security if we try to open a link programmatically. iOS at least prevents a site from doing that, so I generate the share link then the share menu rerenders with the list of links.

The text below was there prior to this intermediate step. Definitely open to suggestions.

For the quick flash, I had something in place for that. I must have undone that. I will look into that.

@nickytonline nickytonline force-pushed the nickytonline/update-star-search-endpoints branch from 975c38a to 5818004 Compare June 15, 2024 02:32
@nickytonline nickytonline marked this pull request as ready for review June 15, 2024 02:32
@nickytonline nickytonline force-pushed the nickytonline/update-star-search-endpoints branch 2 times, most recently from 900a982 to 727b597 Compare June 15, 2024 03:20
@nickytonline
Copy link
Member Author

@BekahHW, the flash of the StarSearch header is gone if a shared conversation loads now.

@nickytonline
Copy link
Member Author

As discussed in standup, I'm going to bring back the functionality to load an initial prompt via the prompt querystring key/value pair so pre-threaded history shares still work. The onlu caveat is if you're not logged in, you'll have to log in. A small price considering we don't have many of the current shared prompts floating around.

@nickytonline nickytonline force-pushed the nickytonline/update-star-search-endpoints branch from 528ba91 to cec5899 Compare June 17, 2024 19:25
@nickytonline nickytonline force-pushed the nickytonline/update-star-search-endpoints branch from 9dfbfc3 to 1cbb744 Compare June 17, 2024 20:01
Copy link
Contributor

@brandonroberts brandonroberts left a comment

Choose a reason for hiding this comment

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

The flow works as intended for me 🍕

Copy link
Member

@jpmcb jpmcb left a comment

Choose a reason for hiding this comment

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

Looks great - great work. With this, will we be ready to bring beta API changes up into prod tomorrow? We'll want to coordinate on that launch since we'll be sending some big, breaking changes into the API that could disrupt the client.

@nickytonline
Copy link
Member Author

nickytonline commented Jun 17, 2024

I'm a little bit confused by this:

image

We're making it a two-step process to share and I think that's adding some friction. We already have a note at the bottom of the links that it makes it public. Can we remove this initial message?

One other thing is that when you load the shareable link, we see the starsearch input screen flash first. Could we do a loader or something else? Not sure if that's just bc of having to move between beta and deploy preview.

Screen.Recording.2024-06-14.at.4.45.44.PM.mov

But this is looking great! Super excited to get this shipped.

As mentioned @BekahHW, the flash of the StarSearch header has been taken care of.

For the create link, it's a step that actually makes the thread public. Maybe the button should say make conversation public or singing song those lines?

@BekahHW
Copy link
Member

BekahHW commented Jun 18, 2024

@nickytonline I tried the Brandon example that you shared. It's interesting that it says the query is from me. Do we want that to show the original poster?

image

@nickytonline
Copy link
Member Author

@nickytonline I tried the Brandon example that you shared. It's interesting that it says the query is from me. Do we want that to show the original poster?

image

That is a good point. We currently don't surface that in the thread history, but we could derive it. I think as a first pass, I could just show the StarSearch logo for a shared thread history. wdyt?

@BekahHW
Copy link
Member

BekahHW commented Jun 18, 2024

When you share a thread, the person who opens the thread still has the ability to add a query, but bc it isn't their thread, they get this response and it seems broken. We should put something in place that prevents them from querying.

I think if you don't "own" the thread, then the input box either shouldn't be there or be disabled with a message about why. Something like, "This thread belongs to X. If you would like more information, start your own conversation with StarSearch."

image

@nickytonline
Copy link
Member Author

When you share a thread, the person who opens the thread still has the ability to add a query, but bc it isn't their thread, they get this response and it seems broken. We should put something in place that prevents them from querying.

I think if you don't "own" the thread, then the input box either shouldn't be there or be disabled with a message about why. Something like, "This thread belongs to X. If you would like more information, start your own conversation with StarSearch."

image

Really great catch here @BekahHW! I wouldn't remove the input box from a UX perspective as that might confuse the user, but we could disabled it, and have a message above it explaining why like you mention in you alternative suggestion.

I'm going to go ahead and implement that.

There is an edge case where the currently logged in user might own the thread, so we could punt on that edge case for now. I have some ideas, but I think it's best to get this big chunk of work in first.

@nickytonline
Copy link
Member Author

Just putting in this draft so I can implement some of the feedback as there is already two approvals.

@nickytonline
Copy link
Member Author

Somewhat unrelated, the deploy preview seems to have this weird justified-left bug:

Screenshot 2024-06-18 at 8 43 31 AM Screenshot 2024-06-18 at 8 43 47 AM
I feel like this was fixed in another PR recently ... maybe updating with beta will _just fix it ™️ _?

Getting latest has fixed this.

@nickytonline nickytonline force-pushed the nickytonline/update-star-search-endpoints branch from 6750294 to 769ac12 Compare June 18, 2024 14:53
@nickytonline nickytonline marked this pull request as ready for review June 18, 2024 15:14
@nickytonline
Copy link
Member Author

nickytonline commented Jun 18, 2024

When you share a thread, the person who opens the thread still has the ability to add a query, but bc it isn't their thread, they get this response and it seems broken. We should put something in place that prevents them from querying.

I think if you don't "own" the thread, then the input box either shouldn't be there or be disabled with a message about why. Something like, "This thread belongs to X. If you would like more information, start your own conversation with StarSearch."

image

Fixed by 5faacee @BekahHW

CleanShot 2024-06-18 at 11 17 49

@nickytonline nickytonline merged commit cc34e7b into beta Jun 18, 2024
13 checks passed
@nickytonline nickytonline deleted the nickytonline/update-star-search-endpoints branch June 18, 2024 16:07
open-sauced bot pushed a commit that referenced this pull request Jun 18, 2024
## [2.36.0-beta.6](v2.36.0-beta.5...v2.36.0-beta.6) (2024-06-18)

### 🍕 Features

* enabled loading an existing conversation in StarSearch ([#3563](#3563)) ([cc34e7b](cc34e7b))
open-sauced bot pushed a commit that referenced this pull request Jun 18, 2024
## [2.36.0](v2.35.0...v2.36.0) (2024-06-18)

### 🐛 Bug Fixes

* Contributor Confidence styling ([#3572](#3572)) ([3e7c4ed](3e7c4ed))
* remove GitHub organization permissions request from authentication ([#3570](#3570)) ([673923c](673923c))
* update user search to fallback to GitHub API on error and zero results ([#3569](#3569)) ([a5946d0](a5946d0))

### 🍕 Features

* add contributions filter to contributor profile ([#3556](#3556)) ([f588f9d](f588f9d))
* add workspace layout to user page ([#3537](#3537)) ([2504c34](2504c34))
* enabled loading an existing conversation in StarSearch ([#3563](#3563)) ([cc34e7b](cc34e7b))
* now StarSearch uses the new StarSearch API endpoints for chat creation/updates ([#3565](#3565)) ([0ac0145](0ac0145))
* Repo Pages - move devstats to top of mobile view ([#3574](#3574)) ([658b76a](658b76a))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Update API endpoints in current StarSearch to prepare for Thread History
4 participants