-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Fetch link previews from RQ #5608
Conversation
|
af22cd1
to
1d118c4
Compare
1edfc95
to
6a08e2c
Compare
Co-authored-by: Mary <[email protected]>
Co-authored-by: Mary <[email protected]>
They don't get fancy previews but at least we show something.
This is a bit annoying to port over and seems like a nice-to-have. Might readd in a different place later but I think this also needs a smarter condition.
This reverts commit 35ade48b98ae9aab01fb99eef886a60f3cbc2e45.
}, | ||
err => { | ||
if (err instanceof EmbeddingDisabledError) { | ||
setError(_(msg`This post's author has disabled quote posts.`)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry if this is a stupid question... this error message shown when a quotegate is set doesn't seem to have been added back anywhere that I can see, should it be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup, currently it’ll use the message from the error constructor but i need to fix this up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@haileyok noting this ideally needs to be fixed — probably with a catch in Composer around post call and then rethrow with localized better error message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 thanks for hitting it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't find a way to break it! The only thing I found is that if there's an external card and you paste a bsky.app link, it doesn't show a preview (but does post). However, this is happening on main and also in prod (so predates other composer changes) so I feel fine approving this, we should look into it though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ran a similar set of tests as the pr this is based on, looks solid.
Yea that’s an existing issue — everything is wired up to get this fixed now but we need to change the preview components in the composer to know how do display other embed types (and then return those embed types from RQ instead of just display names). |
* origin/main: (267 commits) Nicer error message for disabled quotes (#5644) Update neue nux date (#5643) Make alt text scrollable on native (#5642) Move remaining composer state into reducer (#5623) Fetch link previews from RQ (#5608) Use composer state as source of truth for embeds/links on publish (#5606) Update Indonesian translation (#5316) [Video] Add dimension info to share intent pt.2 (#5640) [Video] Prevent screen from dimming while in full screen (#5637) [Video] Add dimension info to share intent (#5639) [Video] Revert safari hackfix (#5367) Swipeable to delete chat, custom swipeable (#5614) equal spacing on displayname/handle (#5636) Update Japanese translation (#5374) Update catalana messages.po (#5380) Update and inconsistency fixes to pt-br translation (#5436) Tweak #5522 (#5635) Update Korean localization (#5401) Update Chinese localization (#5433) Add Cantonese Localization (#5479) ...
Stacked on #5606
This makes the composer reducer a source of truth for link previews. Since the composer reducer itself doesn't actually store preview-related information, the preview is kept in RQ. That was the key insight from @mary-ext's work in #4163.
This unlocks a few things:
extLink
/setExtLink
/extGif
/setExtGif
/useExternalLinkFetch
async monstrosity.