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

Add Open Graph preview functionality #1653

Open
wants to merge 4 commits into
base: stable
Choose a base branch
from

Conversation

MMDJafari
Copy link
Contributor

This PR adds a new OpenGraphPreview component for previewing pages with Open Graph metadata.

  • The OpenGraphPreview component fetches the page HTML and parses meta tags for Open Graph data. It uses DOMParser and meta selectors to simplify this.
  • It renders a basic preview UI with the Open Graph image, title, description, and source website name.
  • The LinkPreview component was updated to check for Open Graph data if no other specific preview type matches first.
  • If Open Graph data is found, the OpenGraphPreview is shown. This provides a fallback preview.
  • Some styling was added for the OpenGraphPreview in Post.scss file.

This allows us to preview many more pages that implement proper Open Graph metadata tags. It provides a better preview than just rendering the naked URL if we don't support the site yet.

Screenshot 1402-08-04 at 4 02 38 in the afternoon

Let me know if any changes are needed!

Create OpenGraphPreview component to fetch and parse metadata. Integrate into LinkPreview to show if no other preview available. Implement polished styling for clean, readable design.
Copy link
Contributor

@indeyets indeyets left a comment

Choose a reason for hiding this comment

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

We tried to apply your changes, but didn't manage to get a working result. Do you have any additional instructions?

  1. We get "cannot access isTrigger before initialisation" error on react level
  2. (the most important part) we can't figure out how this is supposed to work even theoretically as CORS restrictions block requests to arbitrary site on the internet

@MMDJafari
Copy link
Contributor Author

We tried to apply your changes, but didn't manage to get a working result. Do you have any additional instructions?

  1. We get "cannot access isTrigger before initialisation" error on react level
  2. (the most important part) we can't figure out how this is supposed to work even theoretically as CORS restrictions block requests to arbitrary site on the internet

Hi there,

Thanks for the feedback. Let me clarify a few things:

  1. Regarding isTrigger, if I remember correctly and am not mistaken, it was used for the mentioning feature and should not exist in this opengraph branch. It belonged to the mention branch and may also exist in the main branch.

  2. To test locally, you'll need to bypass CORS restrictions which can easily be done using a Chrome extension like CORS Unblock: https://chrome.google.com/webstore/detail/cors-unblock/lfhmikememgdcahcdlaciloancbhjino
    Alternatively, you can test this branch live on Vercel here: https://ferferme-git-og-test-ferferme.vercel.app/
    Simply enter a URL with OpenGraph tags, like this one I used in the screenshot: https://www.theguardian.com/world/2023/oct/25/russia-simulates-nuclear-strike-after-opting-out-of-treaty, and you should see the preview.

The CORS Unblock extension allows cross-origin requests during local development, otherwise the browser's security will block API requests to different domains.

Let me know if this helps explain things! I'm happy to clarify or provide any other information needed to get this running smoothly. Just let me know.

@indeyets
Copy link
Contributor

Are you sure it works for you when this extension is disabled? Our tests show that theguardian doesn't support cors requests (and it shouldn't, that would be unsafe)

@MMDJafari
Copy link
Contributor Author

Are you sure it works for you when this extension is disabled? Our tests show that theguardian doesn't support cors requests (and it shouldn't, that would be unsafe)

Thanks for following up. To clarify the CORS Unblock extension, you originally mentioned "when this extension is disabled" which I should have asked about for specificity. There are a couple possibilities I should have covered better:

  1. If you hadn't enabled the extension at all yet, it would need to be installed and enabled first in order to allow the cross-origin requests for local testing. Please let me know if you need any help with that.
  2. If you meant you enabled the extension, tested that the preview loaded, then disabled it and found the requests failed - that makes total sense and is a great validation that the native CORS headers differ from the extension overrides. I should have explained the steps more clearly.

Regarding testing on server, the feature does function properly in a deployed environment as evidenced by the live Vercel URL I previously provided. But I'm happy to demonstrate or explain anything needed to give you full confidence it will work on server as intended.

Please let me know if I have the right understanding of the scenarios here or if you have any other questions! I appreciate you taking the time to be thorough and help me improve my technical communication - it is very valuable for me.

@indeyets
Copy link
Contributor

I don't do testing on localhost, neither my colleagues. These are stats for browsing 10 pages of real content with your patch. Most of the links give CORS-errors (including links t theguardian website) and the ones that gave 200 didn't have necessary metadata anyway.

so, my theory is, that you have that Chrome extension enabled which disables CORS for you even for non-local websites.

Monosnap Developer Tools 2023-10-27 10-13-11

@MMDJafari
Copy link
Contributor Author

I don't do testing on localhost, neither my colleagues. These are stats for browsing 10 pages of real content with your patch. Most of the links give CORS-errors (including links t theguardian website) and the ones that gave 200 didn't have necessary metadata anyway.

so, my theory is, that you have that Chrome extension enabled which disables CORS for you even for non-local websites.

You're absolutely right, I made an incorrect assumption by validating with the CORS Unblock extension enabled globally. Thank you for catching this gap in my testing methodology. I clearly should have tested with the extension fully disabled. Please accept my apologies for the improper validation. Moving forward, I will work on the code to address the CORS issues without relying on the extension override so that the cross-origin preview functions properly.

@MMDJafari MMDJafari force-pushed the opengraph branch 2 times, most recently from ab0f851 to 4e1930c Compare October 27, 2023 12:41
Exclude internal URLs and subpaths (i.e., URLs that start with the same origin as the current window location) from being checked for Open Graph tags
…cess

This change will allow us to display link previews for websites that we were not able to display them for before
@MMDJafari
Copy link
Contributor Author

Added a CORS proxy to allow us to fetch Open Graph tags for websites that we couldn't before. The CORS proxy bypasses the restriction that prevents us from directly fetching Open Graph tags from some websites.

@indeyets
Copy link
Contributor

indeyets commented Nov 2, 2023

@MMDJafari right. But we try not to rely on external services. So the next logical step is to make our own CORS-proxy as part of the backend. And then we need to protect it from the third parties somehow.

So this was the plan for quite a long time actually, except that we didn't have a good solution for the protection part. You could come to talk to us before doing implementation :-)

We can limit CORS-proxy to authorised users — that should be good enough.

Also, there's minor chance that someone might want to block our IP. But probably not, as we're quite small.

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.

2 participants