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

Fetch link previews from RQ #5608

Merged
merged 31 commits into from
Oct 8, 2024
Merged

Fetch link previews from RQ #5608

merged 31 commits into from
Oct 8, 2024

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Oct 4, 2024

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:

  • We can kill the extLink / setExtLink / extGif / setExtGif / useExternalLinkFetch async monstrosity.
  • We no longer prevent you from attaching media when a link card exists. (Media will just take priority.)
  • We no longer force you to wait for the link card to upload. You can just press Post and it'll do the right thing.

Copy link

github-actions bot commented Oct 4, 2024

Old size New size Diff
7.92 MB 7.92 MB 0 B (0.00%)

@gaearon gaearon force-pushed the embed-7 branch 2 times, most recently from af22cd1 to 1d118c4 Compare October 5, 2024 13:51
@surfdude29
Copy link
Contributor

surfdude29 commented Oct 5, 2024

Stacked on #5608

Little typo here as this is #5608, I think it should be #5606

@gaearon gaearon force-pushed the embed-8 branch 2 times, most recently from 1edfc95 to 6a08e2c Compare October 6, 2024 03:46
@gaearon gaearon marked this pull request as ready for review October 6, 2024 04:03
gaearon added 11 commits October 6, 2024 14:44
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.`))
Copy link
Contributor

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?

Copy link
Collaborator Author

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

Copy link
Collaborator Author

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 thanks for hitting it

Copy link
Member

@mozzius mozzius left a 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

Copy link
Contributor

@haileyok haileyok left a 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.

Base automatically changed from embed-7 to main October 7, 2024 23:58
@arcalinea arcalinea temporarily deployed to embed-8 - social-app PR #5608 October 8, 2024 00:02 — with Render Destroyed
@haileyok haileyok merged commit c06040c into main Oct 8, 2024
5 of 6 checks passed
@haileyok haileyok deleted the embed-8 branch October 8, 2024 00:03
@gaearon
Copy link
Collaborator Author

gaearon commented Oct 8, 2024

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

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).

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 8, 2024

#5647

estrattonbailey added a commit that referenced this pull request Oct 8, 2024
* 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)
  ...
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.

5 participants