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.

Sign up for GitHub

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

Jump to bottom

fix(server): use fqdn for og:image meta tag value #11082

Merged
merged 11 commits into from
Jul 29, 2024
Merged

fix(server): use fqdn for og:image meta tag value #11082

merged 11 commits into from
Jul 29, 2024

Conversation

eleith
Copy link
Contributor

@eleith eleith commented Jul 14, 2024 •
edited
Loading

opengraph image specifies that the url contains http or https, thus implying a fqdn.

this change uses the external domain from the server config to attempt to make the og:image have both the existing path to the thumbnail along with the desired domain

please note, some og implementations do work with relative paths, so not all og image checkers may still pass, but not all implementations have this fallback and thus will not find the image otherwise

fix #11081

jrasm91
jrasm91 previously requested changes Jul 14, 2024
View reviewed changes
Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

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

This is a bit pointless since the change isn't server side rendered. You should take a look at api.service.ts in the server repository instead. There is also an e2e test for shared links that checks this specific functionality.

@eleith eleith requested a review from danieldietzler as a code owner July 14, 2024 05:20
@eleith eleith requested a review from jrasm91 July 14, 2024 05:43
@github-actions github-actions bot added documentation Improvements or additions to documentation 📱mobile labels Jul 14, 2024
@eleith eleith force-pushed the eleith-og-image-external-url branch 2 times, most recently from 8cc8670 to ab26cf6 Compare July 14, 2024 15:26
@eleith eleith changed the title attempt to use fqdn for og:image fix(server): use fqdn for og:image meta tag value Jul 14, 2024
@eleith
Copy link
Contributor Author

eleith commented Jul 14, 2024

i've addressed the comments, lints and tests. there is likely some opportunity to improve the approach (SSR vs web) to improve future DX concerns, but they could be decoupled from this PR as well. i am open to either.

thanks for the feedback so far.

@eleith eleith force-pushed the eleith-og-image-external-url branch from 000c46c to 07dc23a Compare July 16, 2024 15:25
jrasm91
jrasm91 reviewed Jul 16, 2024
View reviewed changes
Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

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

Looks a lot better.

server/src/services/api.service.ts Outdated Show resolved Hide resolved
@jrasm91 jrasm91 dismissed their stale review July 16, 2024 16:38

Addressed

@eleith eleith force-pushed the eleith-og-image-external-url branch from 25ce639 to 0cae9d1 Compare July 17, 2024 05:45
jrasm91
jrasm91 approved these changes Jul 17, 2024
View reviewed changes
Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

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

This looks good, the only issue iM thinking about now is if the extension domain has a trailing slash or not? Did you test both situations? Alternatively new URL(path, domain).href would probably fix the issue as well

@eleith
Copy link
Contributor Author

eleith commented Jul 17, 2024

This looks good, the only issue iM thinking about now is if the extension domain has a trailing slash or not? Did you test both situations? Alternatively new URL(path, domain).href would probably fix the issue as well

i didn't test that!

i just dove in assuming externalDomain was a trimmed and validated host and lazily joined them.

i've pushed updates now to better assemble the host and pathname/query params and have successfully tested with trailing slashes.

@eleith eleith force-pushed the eleith-og-image-external-url branch from 8abfaa6 to c6ea55a Compare July 17, 2024 15:26
@jrasm91
Copy link
Contributor

jrasm91 commented Jul 18, 2024

I mean, we probably should validate/normalize it lol. Maybe in a follow up PR we'll fix it all?

@eleith
Copy link
Contributor Author

eleith commented Jul 18, 2024

I'll fix the tests

eleith added 6 commits July 17, 2024 21:57
@eleith
attempt to use fqdn for og:image
dd823d2 
opengraph image specifies that the url contains http or https, thus
implying a fqdn.

this change uses the external domain from the server config to attempt
to make the og:image have both the existing path to the thumbnail along
with the desired domain

if the server setting is empty, the old behavior will persist

please note, some og implementations do work with relative paths, so not
all og image checkers may still pass, but not all implementations have
this fallback and thus will not find the image otherwise
@eleith
tests and ssr for og:image value as fqdn
07fce1d
@eleith
formatting
8eb934c
@eleith
fix test
25b133c
@eleith
formatting
fce1e8f
@eleith
formatting
2af2345
eleith and others added 3 commits July 17, 2024 21:57
@eleith
fix tests
71d08c4 
getConfig was requiring authentication. using already initiated global stores instead
@eleith
load config in shared link service itself
2d0873c
@eleith
join host and pathname/params safely
d0a0e52
@eleith eleith force-pushed the eleith-og-image-external-url branch from c6ea55a to d0a0e52 Compare July 18, 2024 04:57
use origin instead of host for full domain string
640ec61 
also fixes lint and address the imageURL type which is optional
@eleith
Copy link
Contributor Author

eleith commented Jul 18, 2024

locally, lint, e2e and svelte check now all pass for me

@eleith
Copy link
Contributor Author

eleith commented Jul 20, 2024

@jrasm91 I assume I don't have to keep this PR updated until further notice.

let me know if there are additional requirements or actions I can take.

@jrasm91
Copy link
Contributor

jrasm91 commented Jul 20, 2024

@jrasm91 I assume I don't have to keep this PR updated until further notice.

let me know if there are additional requirements or actions I can take.

Correct. It has just been a busy week, but I will try to get it merged in next week.

@eleith
Copy link
Contributor Author

eleith commented Jul 20, 2024

thanks for all the feedback and i appreciate understanding the next steps.

I mostly wanted clarification that I don't need to monitor the PR anymore.

all sounds good to me, it will land when it lands!

@jrasm91 jrasm91 enabled auto-merge (squash) July 29, 2024 21:33
@jrasm91 jrasm91 merged commit a70cd36 into immich-app:main Jul 29, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mertalev mertalev mertalev approved these changes

@danieldietzler danieldietzler danieldietzler approved these changes

@jrasm91 jrasm91 jrasm91 approved these changes

Assignees
No one assigned
Labels
documentation Improvements or additions to documentation 📱mobile 🗄️server 🖥️web
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

open graph image urls are relative and may not work for non-relaxed implementations
4 participants
@eleith @jrasm91 @danieldietzler @mertalev