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: Use StarSearchPayload from API #3424

Merged
merged 1 commit into from
May 21, 2024
Merged

feat: Use StarSearchPayload from API #3424

merged 1 commit into from
May 21, 2024

Conversation

jpmcb
Copy link
Member

@jpmcb jpmcb commented May 18, 2024

Description

This is a followup to https://github.com/open-sauced/api/pull/818 which introduces a well defined, strongly typed object for sending in the StarSearch data: stream. This should make parsing the incoming content much, much easier. This method is heavily inspired by ChatGPT and the data object they send back in their stream.

This also introduces the remark github style markdown plugin. I'm not sure if it's making a difference, but maybe worth playing around with.

Related Tickets & Documents

Followup to: https://github.com/open-sauced/api/pull/818

⚠️ The deploy preview will not work until the API changes ^go in.

Mobile & Desktop Screenshots/Recordings

Overall, this seems to fix alot of the messy line breaking that was going on:

Screenshot 2024-05-18 at 4 59 34 PM

Steps to QA

  1. Run the API locally (or wait for API changes to go in).
  2. Run app locally, check the /star-search route

Tier (staff will fill in)

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

@jpmcb jpmcb requested a review from a team May 18, 2024 23:03
Copy link

netlify bot commented May 18, 2024

Deploy Preview for design-insights ready!

Name Link
🔨 Latest commit 0c76073
🔍 Latest deploy log https://app.netlify.com/sites/design-insights/deploys/664cb809b7f9fd0008c70f20
😎 Deploy Preview https://deploy-preview-3424--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.

Copy link

netlify bot commented May 18, 2024

Deploy Preview for oss-insights ready!

Name Link
🔨 Latest commit 0c76073
🔍 Latest deploy log https://app.netlify.com/sites/oss-insights/deploys/664cb8096ea5bd00074b28d8
😎 Deploy Preview https://deploy-preview-3424--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
Contributor

@zeucapua zeucapua left a comment

Choose a reason for hiding this comment

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

Looks good! Just needs a fix on the text break.

pages/star-search/index.tsx Outdated Show resolved Hide resolved
@zeucapua
Copy link
Contributor

The list markdown isn't rendering correctly:

API response
Screenshot 2024-05-20 at 12 40 08

Rendered Response
image

@jpmcb
Copy link
Member Author

jpmcb commented May 20, 2024

Force pushed to set the markdown class as break-word which should now wrap only on long words instead of break-all which was wrapping all words: #3420

@jpmcb
Copy link
Member Author

jpmcb commented May 20, 2024

The list markdown isn't rendering correctly:

Yeah, I was hoping the github flavored markdown plugin would fix this :( but it's till messed up. Any ideas?

pages/star-search/index.tsx Outdated Show resolved Hide resolved
@jpmcb jpmcb force-pushed the use-starsearch-payload branch 2 times, most recently from f66ba64 to 672c4a5 Compare May 20, 2024 20:52
@jpmcb jpmcb requested review from a team, brandonroberts and zeucapua May 20, 2024 20:52
@jpmcb
Copy link
Member Author

jpmcb commented May 20, 2024

prose seemed to be the silver bullet we needed to get these to render nicely - huge shoutout to @zeucapua for discovering that!!! 👏🏼

Screenshot 2024-05-20 at 2 53 10 PM

Also added some additional comments and a check that result.content.parts has some actual stuff.

*/

const matched = v.match(/data:\s?(?<result>.*)/);
const matched = v.match(/data:\s(?<result>.*)/);
Copy link
Member Author

Choose a reason for hiding this comment

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

I made the regex just abit more strict: now, there has to be a space between data: and the json object flowing in. We can always expect the same result now from the API: the data key, a :, a space , and then the json object. So, for example, data: { id: "abc123" } will get captured. But data: or data:{ ... } will be rejected.

Copy link
Member

Choose a reason for hiding this comment

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

always one space or can it be \s+? I'd assume just \s since the API is handling the formatting?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, correct, the payload from the API should always be data: { ... } with one space and we should reject anything that is malformed. It's probably a moot point to make the regex more strict, but it at least makes the client have to do alot less heavy lifting since the API is doing it now

pages/star-search/index.tsx Outdated Show resolved Hide resolved
@jpmcb jpmcb force-pushed the use-starsearch-payload branch 3 times, most recently from 91d545e to e055e39 Compare May 20, 2024 21:19
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.

Nice work! 🤝

@jpmcb
Copy link
Member Author

jpmcb commented May 21, 2024

Looking good with the chat bubbles!! cc @nickytonline

Screenshot 2024-05-21 at 8 29 21 AM

would love to get this in a release today so we can play around with everything before thursday!

Copy link
Member

@nickytonline nickytonline left a comment

Choose a reason for hiding this comment

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

Nothing blocking, just some comments. Great work @jpmcb!

🚢

pages/star-search/index.tsx Show resolved Hide resolved
next-types.d.ts Outdated Show resolved Hide resolved
*/

const matched = v.match(/data:\s?(?<result>.*)/);
const matched = v.match(/data:\s(?<result>.*)/);
Copy link
Member

Choose a reason for hiding this comment

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

always one space or can it be \s+? I'd assume just \s since the API is handling the formatting?

Makes the Markdown class use "break-word" and "prose" classes for styling

Co-authored-by: John McBride <john@opensauced.pizza>
Co-authored-by: Zeu Capua <zeu@opensauced.pizza>
Signed-off-by: John McBride <john@opensauced.pizza>
Comment on lines +372 to +375
if (!payload || !payload.content || payload.content.parts.length === 0) {
Sentry.captureException(
new Error(`Parsed and rejected malformed JSON for StarSearch. JSON payload: ${v}`)
);
Copy link
Member Author

Choose a reason for hiding this comment

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

Sends Sentry an error if we get some weird malformed JSON for some reason.

@nickytonline
Copy link
Member

Looking good with the chat bubbles!! cc @nickytonline

Screenshot 2024-05-21 at 8 29 21 AM

would love to get this in a release today so we can play around with everything before thursday!

Not sure why you're loader is blue. 🤔 Here it is on beta.

CleanShot 2024-05-21 at 11 40 54

@nickytonline
Copy link
Member

deploy-preview-3424--oss-insights.netlify.app

Not sure what's going on your local, but on your deploy preview the loader animation is orange. 🙃

@jpmcb jpmcb merged commit a8b01f7 into beta May 21, 2024
13 checks passed
@jpmcb jpmcb deleted the use-starsearch-payload branch May 21, 2024 15:52
open-sauced bot pushed a commit that referenced this pull request May 21, 2024
## [2.30.0-beta.2](v2.30.0-beta.1...v2.30.0-beta.2) (2024-05-21)

### 🍕 Features

* Use StarSearchPayload from API ([#3424](#3424)) ([a8b01f7](a8b01f7))
open-sauced bot pushed a commit that referenced this pull request May 21, 2024
## [2.30.0](v2.29.0...v2.30.0) (2024-05-21)

### 🍕 Features

* add workspaces intro video to welcome modal ([#3432](#3432)) ([d4bce9a](d4bce9a))
* added a loader for StarSearch responses ([#3422](#3422)) ([8972760](8972760))
* open up access to StarSearch and show modal/drawer for login ([#3442](#3442)) ([d228949](d228949))
* Small update to copy for suggested starsearch queries ([#3438](#3438)) ([012db59](012db59))
* update to lunch week card ([#3439](#3439)) ([d41f8fa](d41f8fa))
* Use StarSearchPayload from API ([#3424](#3424)) ([a8b01f7](a8b01f7))

### 🐛 Bug Fixes

* add mobile drawer for `InsightUpgradeModal` ([#3429](#3429)) ([d081980](d081980))
* add support for clipboard copy in Safari and use utility function ([#3433](#3433)) ([2d1b74f](2d1b74f))
* disable search for workspace dropdown if not logged in ([#3414](#3414)) ([b242e92](b242e92))
* fix to avoid StarSearch UI breaking when long words are outputted ([#3420](#3420)) ([4d6bb23](4d6bb23))
* fixed rendering of lottery chart when less than 4 contributors ([#3415](#3415)) ([3888ecb](3888ecb))
* fixed scrolling issues with StarSearch ([#3444](#3444)) ([2c46494](2c46494))
* history chart using correct data for `StarsChart` ([#3443](#3443)) ([f85f1fd](f85f1fd))
* now sauced-orange colour defaults to 1 for opacity ([#3437](#3437)) ([aa9f67f](aa9f67f))
* remove reactions from issues table ([#3435](#3435)) ([e15c6b6](e15c6b6))
* swap out css module prompt with lottery factor ([#3413](#3413)) ([2c9def0](2c9def0))
* updated StarSearch copy ([#3440](#3440)) ([abe33f1](abe33f1))
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.

None yet

4 participants