Issue 1242 url preview #1247

fflorent · 2024-10-04T13:38:29Z

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

the description defaults to "Grist is the evolution of spreadsheets." and is translatable
the icon defaults to statics/img/opengraph-preview-image.png (introduced in this PR)
the title in meta descriptions defaults to Grist, the evolution of spreadsheet, and to [doc name] - Grist if a document is consulted.

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?

👍 yes, I added tests to the test suite
💭 no, because this PR is a draft and still needs work
🙅 no, because this is not relevant here
🙋 no, because I need help

Screenshots / Screencasts

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

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

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 · 2024-10-04T13:46:55Z

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

github-actions · 2024-10-05T09:13:37Z

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

github-actions · 2024-10-05T10:08:20Z

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

github-actions · 2024-10-05T10:47:32Z

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

github-actions · 2024-10-05T11:48:48Z

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

github-actions · 2024-10-05T12:53:10Z

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

github-actions · 2024-10-07T08:20:26Z

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

fflorent · 2024-10-07T08:22:35Z

app/server/lib/sendAppPage.ts

@@ -174,8 +174,8 @@ export function makeSendAppPage({ server, staticDir, tag, testLogin, baseDomain
    ).join('\n');
    const content = fileContent
      .replace("<!-- INSERT WARNING -->", warning)
-      .replace("<!-- INSERT TITLE -->", getPageTitle(req, config))
-      .replace("<!-- INSERT META -->", getPageMetadataHtmlSnippet(config))
+      .replace("<!-- INSERT TITLE -->", getPageTitle(config) ?? (translate(req, 'Loading') + "..."))


I moved this line of code. I am a bit worried, because this seems to me a bad practice to concatenate a translated string with a punctuation.

I would rather move the ellipsis in the translation. Does it make sense to you?

Yes at least for right to left languages.

github-actions · 2024-10-07T10:30:19Z

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

github-actions · 2024-10-07T10:54:43Z

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

github-actions · 2024-10-07T12:53:49Z

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

github-actions · 2024-10-08T07:28:50Z

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

github-actions · 2024-10-08T14:30:47Z

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

hexaltation

LGTM

hexaltation · 2024-10-07T10:18:29Z

static/img/icon-grist.png

May be use https://github.com/gristlabs/grist-core/blob/main/static/icons/grist.svg instead.

manuhabitela · 2024-10-16T13:58:27Z

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

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

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 · 2024-10-16T15:59:55Z

@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 · 2024-10-16T16:01:28Z

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

github-actions · 2024-10-16T16:04:46Z

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

github-actions · 2024-11-18T15:26:17Z

Deployed commit e8b3d06e463df68034a2f6f395b553d28b014970 as https://grist-fflorent-grist-core-issue-1242-url-preview.fly.dev (until 2024-12-18T15:24:39.800Z)

github-actions · 2024-11-18T15:43:52Z

Deployed commit b1e145dfc6e6a85ac52b59d6bb392d5b6748c470 as https://grist-fflorent-grist-core-issue-1242-url-preview.fly.dev (until 2024-12-18T15:42:12.403Z)

github-actions · 2024-11-18T15:51:42Z

Deployed commit 6263797d2dfcba428b4ad97280773b9b4f7050c9 as https://grist-fflorent-grist-core-issue-1242-url-preview.fly.dev (until 2024-12-18T15:49:47.475Z)

github-actions · 2024-11-18T20:19:11Z

Deployed commit c353d8996f360a165118b9c9693b84410f53b822 as https://grist-fflorent-grist-core-issue-1242-url-preview.fly.dev (until 2024-12-18T20:18:02.730Z)

github-actions · 2024-11-19T08:55:27Z

Deployed commit 61239994dac4389d21299f4fb947c6d72ad2a145 as https://grist-fflorent-grist-core-issue-1242-url-preview.fly.dev (until 2024-12-19T08:53:36.335Z)

github-actions · 2024-11-20T13:41:41Z

Deployed commit 85aa365bbcd1ff7f26d1ad2b25dc1cff414edaed as https://grist-fflorent-grist-core-issue-1242-url-preview.fly.dev (until 2024-12-20T13:40:08.735Z)

paulfitz · 2024-11-21T14:41:07Z

app/server/lib/sendAppPage.ts

@@ -24,9 +25,14 @@ import * as fse from 'fs-extra';
 import * as handlebars from 'handlebars';
 import jsesc from 'jsesc';
 import * as path from 'path';
-import difference = require('lodash/difference');
+import { difference, trimEnd } from 'lodash';


Some years ago we did a round of optimization on app startup. Turns out, lodash is big! And loading it unqualified takes enough time to matter. Since then, we pivoted from being a desktop app to being a web app, and startup time mattered less. But this will be why you don't see many imports of plain 'lodash' in Grist (though they have crept in) since if a reviewer is around who remembers this, they'll push back on it. But I count 7 other plain 'lodash' imports now, so I'm ok giving up and accepting this. Just explaining why lodash/difference was there.

paulfitz · 2024-11-21T14:53:22Z

app/server/lib/requestUtils.ts

@@ -40,7 +40,7 @@ export function adaptServerUrl(url: URL, req: RequestWithOrg): void {
  const reqBaseDomain = parseSubdomain(req.hostname).base;

  if (process.env.GRIST_SERVE_SAME_ORIGIN === 'true' || req.isCustomHost) {
-    url.hostname = req.hostname;
+    url.host = req.headers.host || req.hostname;


This is kind of a scary change but it seems logical to use the host rather than hostname. And I remember express being weird about req.host which it feels like would be the logical thing to use if it were sane? I'm just not confident I know what impact this would have under proxy forwarding, if there could be a subtle difference in support for X-Forwarded-For.

paulfitz · 2024-11-21T15:00:29Z

app/server/lib/sendAppPage.ts

-const translate = (req: express.Request, key: string, args?: any) => req.t(`sendAppPage.${key}`, args);
+const { escapeExpression } = handlebars.Utils;
+
+const translateEscaped = (req: express.Request, key: string, args?: any) => {


Any reason not to write it as just

function translateEscaped(req, key, args) { ... }

It would be helpful to add a docstring with the motivation for this method and the problem that it solves.

Tiny thing: often in Grist codebase the pair of variables req and res are used together as express.Request and express.Response. Maybe use a different name than res here?

paulfitz · 2024-11-21T15:02:39Z

app/server/lib/sendAppPage.ts

-    const staticBaseUrl = `${staticOrigin}/v/${staticTag}/`;
+    // APP_STATIC_URL or APP_HOME_URL.
+    const staticOrigin = staticTag === 'boot' ? '' : (process.env.APP_STATIC_URL || config.homeUrl || '');
+    const staticBaseUrl = `${trimEnd(staticOrigin, '/')}/v/${staticTag}/`;


Another scary change. The motivation was as I now specify the absolute URL for serving static resources (needed for specifying the opengraph icon)? Do you actually still need this?

I removed the changes that looked scary to you, with the last changes it was indeed not worth the risk, you're right.

github-actions · 2024-11-26T15:36:06Z

Deployed commit 332f060526f9c30fc6e4b1eed2cd629f9b42e69f as https://grist-fflorent-grist-core-issue-1242-url-preview.fly.dev (until 2024-12-26T15:34:25.840Z)

…ristlabs#1242 When pasting a URL in some app or website that allows previewing the link, for other resources than documents, you were offered an irrelevant description. This patches aims to give a generic description of what is Grist. Co-authored-by: Emmanuel Pelletier <[email protected]>

github-actions · 2024-11-26T15:53:12Z

Deployed commit 0e1727d66717b4862a41c63fd0e11f6e1f467307 as https://grist-fflorent-grist-core-issue-1242-url-preview.fly.dev (until 2024-12-26T15:51:28.945Z)

paulfitz

Thanks for whittling out some parts that were hard to process. Have one more question, it may be fine.

paulfitz · 2024-11-26T18:01:33Z

app/server/lib/sendAppPage.ts

+ * @param key The key of the translation (which will be prefixed by `sendAppPage`)
+ * @param args The args to pass to the translation string (optional)
+ */
+function translateEscaped(req: express.Request, key: string, args?: any) {


This helper is giving me pause. I guess anything it is used to translate won't be picked up by https://github.com/gristlabs/grist-core/blob/main/buildtools/generate_translation_keys.js - not necessarily a problem, since you can add manually - but makes the eventual garbage collection problem harder of figuring out what keys are no longer used as code changes so they can be dropped.

The escaping, the threat model here is that the translations may be tainted, we can't trust them? This does make sense given how our weblate project is run, it is pretty open. Just going to page @berhalak here for some advice on whether there might be a more systematic way to handle this threat.

We can leave this helper for now. I will remove it later and prepare some cleaning mechanism at the build time. Same issue is for client, when someone uses it insecurely (like el.innerHTML = t(....)).
I'm surprised I didn't think of it myself :)

@berhalak Right, yeah. Thanks for your feedback, do you want me to create a separate issue for that?

Hi @fflorent, Yes please. I'll try to do it asap.

Hi @berhalak

Reported here as a bug: #1350

hexaltation · 2024-11-26T23:01:46Z

app/common/gristUrls.ts

@@ -107,6 +107,7 @@ export const commonUrls = {
  formulaSheet: 'https://support.getgrist.com/formula-cheat-sheet',
  formulas: 'https://support.getgrist.com/formulas',
  forms: 'https://www.getgrist.com/forms/?utm_source=grist-forms&utm_medium=grist-forms&utm_campaign=forms-footer',
+  openGraphPreviewImage: 'https://grist-static.com/icons/opengraph-preview-image.png',


I'm a little concerned about GDPR for European self-hosters opening their instances publicly.
They should have a mechanism to a least offer user deactivation of such url sending data to a third party.
I'm realizing that there is a lot of urls in DINUM and ANCT instances sending IPs data to a third party in GDRP terms.
May be it could be a good Idea to open an issue about that.

fflorent added the preview Launch preview deployment of this PR label Oct 4, 2024

fflorent force-pushed the issue-1242-url-preview branch from 64685b3 to aeb4a24 Compare October 5, 2024 09:05

fflorent force-pushed the issue-1242-url-preview branch from aeb4a24 to 715b5eb Compare October 5, 2024 10:00

fflorent force-pushed the issue-1242-url-preview branch from 715b5eb to 99283f7 Compare October 5, 2024 10:40

fflorent force-pushed the issue-1242-url-preview branch from 9a72269 to 3aabb0a Compare October 5, 2024 11:42

fflorent marked this pull request as ready for review October 5, 2024 11:50

fflorent requested a review from hexaltation October 5, 2024 11:50

fflorent force-pushed the issue-1242-url-preview branch from 3aabb0a to 725aeb9 Compare October 5, 2024 12:46

fflorent force-pushed the issue-1242-url-preview branch from 725aeb9 to b81bea2 Compare October 7, 2024 08:13

fflorent commented Oct 7, 2024

View reviewed changes

hexaltation added the anct label Oct 8, 2024

fflorent removed the anct label Oct 9, 2024

fflorent requested a review from manuhabitela October 10, 2024 08:36

hexaltation approved these changes Oct 16, 2024

View reviewed changes

fflorent force-pushed the issue-1242-url-preview branch 2 times, most recently from c353d89 to 7fe7d10 Compare November 18, 2024 20:13

fflorent requested a review from berhalak November 18, 2024 21:22

fflorent force-pushed the issue-1242-url-preview branch from 7fe7d10 to 6123999 Compare November 19, 2024 08:48

fflorent force-pushed the issue-1242-url-preview branch from 6123999 to 85aa365 Compare November 20, 2024 13:35

paulfitz reviewed Nov 21, 2024

View reviewed changes

paulfitz mentioned this pull request Nov 22, 2024

ADAPT_DOMAIN does not take into account the specified port #1319

Open

2 tasks

fflorent and others added 8 commits November 26, 2024 16:45

PR remarks

b76e3ba

Use commonUrls for the image preview

a5a6a38

Remove unused param

64cbdee

Wrongly removed use of maybeDoc?.options?.icon

c372eb8

Escape translations and keep key for Loading

3c7b862

Document the translation function

9fcf510

Remove scary changes

0e1727d

fflorent force-pushed the issue-1242-url-preview branch from 332f060 to 0e1727d Compare November 26, 2024 15:46

fflorent requested review from paulfitz, hexaltation and manuhabitela November 26, 2024 15:47

paulfitz reviewed Nov 26, 2024

View reviewed changes

hexaltation reviewed Nov 26, 2024

View reviewed changes

fflorent mentioned this pull request Dec 20, 2024

Sanitize localization function #1350

Open

2 tasks

Issue 1242 url preview #1247

Are you sure you want to change the base?

Issue 1242 url preview #1247

Conversation

fflorent commented Oct 4, 2024 • edited Loading

Context

Proposed solution

Related issues

Has this been tested?

Screenshots / Screencasts

github-actions bot commented Oct 4, 2024

github-actions bot commented Oct 5, 2024

github-actions bot commented Oct 5, 2024

github-actions bot commented Oct 5, 2024

github-actions bot commented Oct 5, 2024

github-actions bot commented Oct 5, 2024

github-actions bot commented Oct 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 7, 2024

github-actions bot commented Oct 7, 2024

github-actions bot commented Oct 7, 2024

github-actions bot commented Oct 8, 2024

github-actions bot commented Oct 8, 2024

hexaltation left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuhabitela commented Oct 16, 2024

fflorent commented Oct 16, 2024

manuhabitela commented Oct 16, 2024

github-actions bot commented Oct 16, 2024

github-actions bot commented Nov 18, 2024

github-actions bot commented Nov 18, 2024

github-actions bot commented Nov 18, 2024

github-actions bot commented Nov 18, 2024

github-actions bot commented Nov 19, 2024

github-actions bot commented Nov 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 26, 2024

github-actions bot commented Nov 26, 2024

paulfitz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fflorent commented Oct 4, 2024 •

edited

Loading