search

gristlabs / grist-core

Grist is the evolution of spreadsheets.
https://www.getgrist.com
Apache License 2.0
7.27k stars 322 forks source link

Issue 1242 url preview #1247

Open fflorent opened 1 month ago

fflorent commented 1 month ago

Context

When previewing the URL of an instance (say https://docs.getgrist.com/ ) in websites or apps, it simply displays "Loading..." without any further description or images. It's quite confusing.

See #1242 for detailed steps to reproduce.

Proposed solution

Also a technical note: I had to change the way requestUtils deduce the host of the home, so it takes into account the port. Otherwise this test failed with my change, as I now specify the absolute URL for serving static resources (needed for specifying the opengraph icon).

Related issues

Fixes #1242

Has this been tested?

Screenshots / Screencasts

Here is how the preview of the instance link looks like on Signal: preview home page

And how it looks like when previewing a document (:warning: Important: it must be shared publicly to obtain this result): preview doc title

You also may check this website which seems to show the rendering of URL previews on different websites: https://opengraph.dev/

Home page: https://opengraph.dev/panel?url=https%3A%2F%2Fgrist-fflorent-grist-core-issue-1242-url-preview.fly.dev%2F Doc page title: https://opengraph.dev/panel?url=https%3A%2F%2Fgrist-fflorent-grist-core-issue-1242-url-preview.fly.dev%2FinSPY9A8z7X3%2FMy-document-title

github-actions[bot] commented 1 month ago

Deployed commit 64685b3d13870d72f94c27d1c3ba91f97668d367 as https://grist-fflorent-grist-core-issue-1242-url-preview.fly.dev (until 2024-11-03T13:44:16.143Z)

github-actions[bot] commented 1 month ago

Deployed commit aeb4a245d0fc661bfc347c5cb2ba03d9f7eb0d32 as https://grist-fflorent-grist-core-issue-1242-url-preview.fly.dev (until 2024-11-04T09:10:29.627Z)

github-actions[bot] commented 1 month ago

Deployed commit 715b5ebbc4a28a7153efcbe35f43290af2af873a as https://grist-fflorent-grist-core-issue-1242-url-preview.fly.dev (until 2024-11-04T10:05:07.677Z)

github-actions[bot] commented 1 month ago

Deployed commit 9a72269ae801372761fe6706e09ecf82540fafd4 as https://grist-fflorent-grist-core-issue-1242-url-preview.fly.dev (until 2024-11-04T10:46:01.039Z)

github-actions[bot] commented 1 month ago

Deployed commit 3aabb0aea9274dd447cd2fedaa95bfd90edca5f4 as https://grist-fflorent-grist-core-issue-1242-url-preview.fly.dev (until 2024-11-04T11:47:21.488Z)

github-actions[bot] commented 1 month ago

Deployed commit 725aeb9b7a469c3e691e609c9719e26bb0e891e7 as https://grist-fflorent-grist-core-issue-1242-url-preview.fly.dev (until 2024-11-04T12:51:34.330Z)

github-actions[bot] commented 1 month ago

Deployed commit b81bea2d058be8b43abf1f4ad4f73fafd480a7f0 as https://grist-fflorent-grist-core-issue-1242-url-preview.fly.dev (until 2024-11-06T08:18:40.164Z)

github-actions[bot] commented 1 month ago

Deployed commit b2889474b05a2c073e6d9e662bb5783eafcaebbb as https://grist-fflorent-grist-core-issue-1242-url-preview.fly.dev (until 2024-11-06T10:27:53.976Z)

github-actions[bot] commented 1 month ago

Deployed commit d752342ce4ef39596b95f1743d2464bf5c8435b0 as https://grist-fflorent-grist-core-issue-1242-url-preview.fly.dev (until 2024-11-06T10:52:11.687Z)

github-actions[bot] commented 1 month ago

Deployed commit 6ebbee8b8010ac42e56215cfbd0357c810c739e2 as https://grist-fflorent-grist-core-issue-1242-url-preview.fly.dev (until 2024-11-06T12:52:12.090Z)

github-actions[bot] commented 1 month ago

Deployed commit 6ebbee8b8010ac42e56215cfbd0357c810c739e2 as https://grist-fflorent-grist-core-issue-1242-url-preview.fly.dev (until 2024-11-07T07:27:13.258Z)

github-actions[bot] commented 1 month ago

Deployed commit 2464849f54edb4994fa2fff0d738839cab7da287 as https://grist-fflorent-grist-core-issue-1242-url-preview.fly.dev (until 2024-11-07T14:28:51.869Z)

manuhabitela commented 1 month ago

Hey, great work :)

Maybe I'm saying this too late but… how about using a more "marketing" approach for og title/images? :)

1. About the image:

Most social platforms recommend using a 1200x630px image as og:image, that is generally not just a logo, but something that looks more friendly when sharing the webpage.

If you test this with the LinkedIn inspector for example, you see they target a different aspect ratio, as X does, as Facebook does, etc.:

image

Here is quickly made-up example of what we could do instead (not large enough in terms of pixels but with the correct aspect ratio):

grist-meta-example

2. About the title:

I'd argue "Welcome - Grist" is also not that friendly as the main title for sharing purposes and would say having the tagline in it might be better. When showing the website preview card, some platforms show the description, some do not.

So I'd say having Grist, the evolution of spreadsheet as the title, and changing the description to prevent too much duplication, for example with the tagline visible on getgrist.com I guess, A modern, open source spreadsheet that goes beyond the grid) might fit better?

Sidenote: when testing this with this tool: https://www.bannerbear.com/tools/twitter-card-preview-tool, Loading… is still shown instead of Welcome…. Not an official thing by any mean but maybe it shows a remaining issue?

fflorent commented 1 month ago

@manuhabitela Thanks for your feedback. As discussed, I take a look of what I can, but I feel like it should not block the review as it is better than what already exist. Still this should be a nice follow-up.

Sidenote: when testing this with this tool: https://www.bannerbear.com/tools/twitter-card-preview-tool, Loading… is still shown instead of Welcome…. Not an official thing by any mean but maybe it shows a remaining issue?

I don't know how this tool works. The only thing I notice is that the preview is not rendered on Twitter, at the contrary of say https://duckduckgo.com

I am trying something to fix this.

manuhabitela commented 1 month ago

You're totally right, my mistake for reporting that late :)

github-actions[bot] commented 1 month ago

Deployed commit 3206c227534b46c5fccc6d16808f32b8c65edd60 as https://grist-fflorent-grist-core-issue-1242-url-preview.fly.dev (until 2024-11-15T16:02:49.258Z)

fflorent commented 1 month ago

@manuhabitela You don't have to apologize, remarks even late are welcome :)

github-actions[bot] commented 1 month ago

Deployed commit c20b19826785ed4cbabefd148810e17d33a95198 as https://grist-fflorent-grist-core-issue-1242-url-preview.fly.dev (until 2024-11-16T13:46:12.020Z)

github-actions[bot] commented 1 month ago

Deployed commit e38a25e0d0f3dff39a61b2545aa110ef3ddd468c as https://grist-fflorent-grist-core-issue-1242-url-preview.fly.dev (until 2024-11-16T14:10:16.697Z)

github-actions[bot] commented 1 month ago

Deployed commit 2b173b6165716446e5928d709d1f96493078f326 as https://grist-fflorent-grist-core-issue-1242-url-preview.fly.dev (until 2024-11-17T14:39:27.200Z)

github-actions[bot] commented 1 month ago

Deployed commit c0daff55541c174bb48d8b95d2c132ebec8c78a8 as https://grist-fflorent-grist-core-issue-1242-url-preview.fly.dev (until 2024-11-17T14:43:52.291Z)

anaisconce commented 3 weeks ago

This is strictly an improvement, thank you! For the OG image, just the logo and the name Grist, no tagline, may be best.

For example, here's what Airtable and Notion look like.

image image

@nbush Any edits to the meta description? Okay to stick with it this for now if there's nothing obviously better. :)

nbush commented 3 weeks ago

This is a great improvement that I'm so glad you were able to implement. It looks quite finicky...

The description text of "A modern, open source spreadsheet that goes beyond the grid" is perfect, and I've attached a properly-sized OpenGraph card for use.

grist-opengraph

fflorent commented 3 weeks ago

@nbush @anaisconce Thanks for your feedback (glad you're enthusiast!), I have integrated @nbush's suggestion, you will be able to use this URL to see how it renders now:

Any other suggestions come to your mind?

github-actions[bot] commented 3 weeks ago

Deployed commit 20cb7e142419dd0f2c765bd7b173e58fcdbed0b9 as https://grist-fflorent-grist-core-issue-1242-url-preview.fly.dev (until 2024-11-23T06:53:58.393Z)

nbush commented 3 weeks ago

@fflorent I think that looks good, apart from the main site meta description which should be "Grist, the evolution of spreadsheets", but is currently missing the "s" at the end.

fflorent commented 3 weeks ago

Fixed, thanks @nbush!

github-actions[bot] commented 3 weeks ago

Deployed commit 23824631a7e03a198c72eca9f646f299fd8ce710 as https://grist-fflorent-grist-core-issue-1242-url-preview.fly.dev (until 2024-11-23T14:41:44.562Z)

manuhabitela commented 3 weeks ago

The final visual is way better than my quick-and-dirty example, thanks :sweat_smile:

paulfitz commented 2 weeks ago

Fixed, thanks @nbush!

Neat! Looks like there's a test that needs updating after the text change @fflorent ?

github-actions[bot] commented 2 weeks ago

Deployed commit 0d4bb3ae44f33317f72149de672100bd91c5c4ad as https://grist-fflorent-grist-core-issue-1242-url-preview.fly.dev (until 2024-11-29T16:58:49.633Z)

github-actions[bot] commented 2 weeks ago

Deployed commit 9f22629c603e309d8c506f4a01fb4bec6d5c6da1 as https://grist-fflorent-grist-core-issue-1242-url-preview.fly.dev (until 2024-11-29T17:08:20.568Z)

fflorent commented 2 weeks ago

@paulfitz Thanks for notifying me about that. Done!

berhalak commented 1 week ago

I have one question (maybe it was discussed previously). The URL for the thumbnail looks like this: https://host/v/unknown/img/opengraph-preview-image.png, the v/unknown part bothers me here.

@paulfitz Am I right thinking here. This is a random and temporary doc worker id. So it is possible that someone will post link to the document on some platform, where our meta info will be cached and stored in some thread, and it will soon be broken once doc worker flips.

If I'm thinking straight here, I'd prefer externalizing this image. Putting it in commonUrls property, and by default storing it on our marketing site getgrist.com.

paulfitz commented 1 week ago

I have one question (maybe it was discussed previously). The URL for the thumbnail looks like this: https://host/v/unknown/img/opengraph-preview-image.png, the v/unknown part bothers me here.

@paulfitz Am I right thinking here. This is a random and temporary doc worker id. So it is possible that someone will post link to the document on some platform, where our meta info will be cached and stored in some thread, and it will soon be broken once doc worker flips.

If I'm thinking straight here, I'd prefer externalizing this image. Putting it in commonUrls property, and by default storing it on our marketing site getgrist.com.

That is a good point @berhalak. The v/<tag> may not be very long lived. For our SaaS, it is a deployment hash, rather than a worker id. To avoid clients having inconsistent assets in their browser cache, after a redeployment, we have unique asset paths per deployment. Old deployment assets eventually go away. They are a lot more durable than worker ids (for us, months rather than days) but by no means permanent.

Using commonUrls and an external image is a solid idea, but feels maybe a little intrusive for self installers, if posting links to their material pulls from our marketing site. Would it be practical to add a specific https://host/img/opengraph-preview-image.png URL for this purpose? Either redirecting or serving the asset directly? Serving the asset from node isn't maybe the best solution since we lose CDN caching.

fflorent commented 1 week ago

Hmm, then I need some context, there seems to exist some logic that is proper to the SAAS version.

This is a random and temporary doc worker id.

I thought that part was specified in /dw/{workerId}. At least, in grist-core, I understand that the tags are git commit tags, as suggests this variable: https://github.com/gristlabs/grist-core/blob/b633dd7de92a5caf0e84d8897778db6a30c51b62/stubs/app/common/version.ts#L5

Which seems to be used to build the tag: https://github.com/gristlabs/grist-core/blob/b633dd7de92a5caf0e84d8897778db6a30c51b62/app/server/lib/FlexServer.ts#L661-L664

Taking a look at the TagChecker, these lines raised my attention: https://github.com/gristlabs/grist-core/blob/b633dd7de92a5caf0e84d8897778db6a30c51b62/app/server/lib/TagChecker.ts#L45-L52

Also the comment is very interesting. I wonder as of today on what instance this TagChecker is useful? It looks like you have transitionned to a CDN (grist-static), which motivated you to ignore the tag. And self-hosters use the unknown tag anyways.

But when I get some asset fetched on docs.getgrist.com, like this one (the link will probably expire in the future), the tag is present in the URL and the asset is accessible. But if I alter the tag in the URL, I get an error with this message: The specified key does not exist. This error is not present in grist-core, so I deduce only the CDN checks the tags.

To summarize my questionings:

paulfitz commented 1 week ago

When using a CDN, I expect that the APP_STATIC_URL is set, inhibates the TagChecker, am I right?

This is how it looks on our SaaS:

Screenshot from 2024-11-06 12-23-49

All server assets are being served from a prefix on our CDN that is unique to the particular deployment (yes, a commit hash).

So, for example, https://grist-static.com/v/01fef3138/icons/favicon.png exists, but /icons/favicon.png probably doesn't exist. Nothing on grist-static.com gets routed to an actual Grist server. The assets are pushed there via a deployment step.

The comment you found sounds like a check that could and probably should be turned back on.

paulfitz commented 1 week ago

The deployment step, for concreteness:

  GITCOMMIT=$(node -p "require('./$WORK_DIR/grist/app/common/version').gitcommit")
  S3_URL="s3://grist-static/v/$GITCOMMIT/"
  echo "Deploying assets to $S3_URL"
  aws s3 cp --recursive --follow-symlinks --quiet $WORK_DIR/grist/static $S3_URL
  aws s3 cp --recursive --follow-symlinks --quiet $WORK_DIR/grist/bower_components $S3_URL
dsagal commented 1 week ago

I went ahead and created a more permanent copy of the image in https://grist-static.com/icons/opengraph-preview-image.png. Jarek's idea of adding this URL to commonUrls. For SaaS, this seems the best solution. For self-hosters, it doesn't seem very intrusive to me, but a little, yes. How do you @fflorent feel about it? Would you change it in your instance? If not, I'd go with the simple solution, and not bother with a new environment variable, until someone needs it.

fflorent commented 1 week ago

How do you @fflorent feel about it? Would you change it in your instance?

I think it's a possibility, to customize the color and the name. @vviers What do you think?