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: stream translation titles in feed and post page #4098

Open
wants to merge 13 commits into
base: feat-realtime-translation
Choose a base branch
from

Conversation

omBratteng
Copy link
Member

@omBratteng omBratteng commented Jan 23, 2025

Changes

Events

Did you introduce any new tracking events?

Experiment

Did you introduce any new experiments?

Manual Testing

Caution

Please make sure existing components are not breaking/affected by this PR

AS-925

Preview domain

https://as-925-kvasir-stream.preview.app.daily.dev

Copy link

vercel bot commented Jan 23, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
daily-webapp ✅ Ready (Inspect) Visit Preview Jan 24, 2025 2:39pm
1 Skipped Deployment
Name Status Preview Updated (UTC)
storybook ⬜️ Ignored (Inspect) Jan 24, 2025 2:39pm

@omBratteng omBratteng changed the base branch from main to feat-realtime-translation January 23, 2025 10:13
@omBratteng omBratteng force-pushed the AS-925-kvasir-stream branch from 218ee51 to 6343527 Compare January 23, 2025 10:16
@omBratteng omBratteng changed the title feat: stream translation titles in feed feat: stream translation titles in feed and post page Jan 23, 2025
@omBratteng omBratteng marked this pull request as ready for review January 23, 2025 10:17
@omBratteng omBratteng requested a review from a team as a code owner January 23, 2025 10:17
@omBratteng omBratteng requested review from rebelchris, ilasw, capJavert, sshanzel, nensidosari and AmarTrebinjac and removed request for a team January 23, 2025 10:17
Copy link
Contributor

@capJavert capJavert left a comment

Choose a reason for hiding this comment

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

Some initial comments, overall logic looks nice. Did not spend time yet to test it out.

@@ -24,6 +24,7 @@
"date-fns": "^2.25.0",
"date-fns-tz": "1.2.2",
"dompurify": "^2.5.4",
"fetch-event-stream": "^0.1.5",
Copy link
Contributor

Choose a reason for hiding this comment

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

You are using this because native EventSource does not allow headers? I would rather use the query param from AI search and native EventSource, wasteful to add dependencies for this.

Copy link
Member Author

@omBratteng omBratteng Jan 23, 2025

Choose a reason for hiding this comment

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

Yeah, it's because it doesn't allow headers, think it's a neater solutions than token (don't really like auth tokens in params, even short lived ones 🙈) and language as query param. Package is 730B gzipped, so negligible small imo.

Copy link
Member Author

@omBratteng omBratteng Jan 23, 2025

Choose a reason for hiding this comment

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

Can shave it down to 608B gzipped by using events and not their stream wrapper which just adds two headers and checks if response is OK before returning events 🤪

Copy link
Member Author

@omBratteng omBratteng Jan 23, 2025

Choose a reason for hiding this comment

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

If anything, I think we should move AI search over to use this as well. Especially now when we're starting to send smart prompts, we might encounter issues with max length of URLs in browsers.

Also because EventSource is kinda "deprecated", no browsers is going to remove it, but it's not getting any new features.

So the package is just for parsing the stream, fetch natively supports streaming.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering can we just use fetch then?

Copy link
Member Author

Choose a reason for hiding this comment

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

So the package name is a bit misleading, it's more a parser than fetch. It exposes events which is the stream parser (608B) then stream (122B) which just wraps around fetch and returns the events parsed stream.

packages/shared/src/hooks/translation/useTranslation.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@rebelchris rebelchris 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, but do check question.

packages/shared/src/hooks/useFeed.ts Outdated Show resolved Hide resolved
Comment on lines +39 to +41
updatedPost.sharedPost.title = translation.title;
updatedPost.sharedPost.translation = { title: !!translation.title };
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this set it wrong in cache?
eg some case the title might not render but we need it for some other field?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, don't think so

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.

3 participants