Display images in markdown with support for lakefs:// URIs #5449

eladlachmi · 2023-03-12T12:11:25Z

Resolves #5366

This PR adds support for lakefs:// URIs in markdown images and resolving them to pre-signed URLs for display in the browser. This works for all markdown files, not README.md exclusively. The initial markup (incl. lakefs:// URIs) is immediately displayed and is replaced once pre-signed URLs are generated. This means the process doesn't hold up the rendering of the repository view and also doesn't affect markdown files that don't include images with lakefs:// URIs.

This PR also replaces jest with vitest for easier integration with Vite.

itaiad200

Very cool! couple of questions/comments.

itaiad200 · 2023-03-12T16:00:39Z

webui/src/lib/remark-plugins/imageUriReplacer.ts

+  for (const image of images) {
+    const [repo, branch, ...path] = image.url.split("/").slice(2);
+    const promise = objects
+      .getPresignedUrl(repo, branch, path.join("/"))


What if presigned URLs is not enabled for the installation?

Like you can still read the images even if presign urls are not available, right?

@itaiad200 This assumes pre-signed URLs are enabled. If not, I could add an alternative to download the file and convert it to a base64 data URL. Do you think that's needed?

How can it assumes presigned is enabled if some installations (local, mem) don't support that at all and in other (Azure) it depends on the configuration?
We have a way to check a installation/repo for presigned support (as it's part of the settings). We can add the images support for repos without presigned urls later, as long as we handle these repos nicely for now, i.e. don't try to embed images for now.

I see... I did not consider that... 🤔
I'll see if I can construct data URLs to support installations without presigned URLs.
We also need to know if presigned URLs are available on the client
Maybe it's time to add a "supported features" or "configuration" endpoint so the client is aware of what's available/supported/enabled/disabled/etc.

Replaced presigned URLs with "regular" URLs. This also simplifies several other things.

itaiad200 · 2023-03-12T16:03:37Z

webui/src/lib/remark-plugins/imageUriReplacer.ts

+const imageUriReplacer: Plugin<[], Root> = () => async (tree) => {
+  const images: Array<Node & { url: string }> = [];
+  visit(tree, "image", (node: Node & { url: string }) => {
+    if (node.url.startsWith("lakefs://")) {


What about relative paths? I think that's the more common case. Our readme for example:

<img src="docs/assets/img/logo_large.png"/>

I'll add relative URL support, but I must make some assumptions.
When the URL starts with lakefs://, the format is lakefs://{repo}/{branch}/...{path}
Regarding relative URLs, I'll assume it's relative to the current repo and branch. WDYT?

Added relative URL support + relevant tests

I think your assumptions are spot on.

Just ref instead of branch 🙈

itaiad200 · 2023-03-12T16:03:50Z

webui/src/lib/remark-plugins/imageUriReplacer.ts

+
+const imageUriReplacer: Plugin<[], Root> = () => async (tree) => {
+  const images: Array<Node & { url: string }> = [];
+  visit(tree, "image", (node: Node & { url: string }) => {


Isn't img a valid image label too?

In markdown, an image node is ![alt](URL). Not sure if this AST matches HTML <img /> tags as well. I'll need to check.

@itaiad200 It looks like it does not match HTML within MD. This means I'd need to parse HTML nodes manually, and I would rather not since it could lead to many edge cases, which this AST was supposed to spare us.

I think it's legit to say that we support MD image tags for automated resolution, at least for now.
WDYT?

webui/src/lib/api/index.js

webui/src/lib/remark-plugins/imageUriReplacer.spec.ts

itaiad200

Getting closer 💪 See my comments...

itaiad200 · 2023-03-13T08:18:58Z

webui/src/lib/remark-plugins/imageUriReplacer.spec.ts

+});
+
+describe("imageUriReplacer", async () => {
+  let mockObjects: any;


Notice the warning here

Yes, I'm aware. Since it's a spec file and figuring out the TS support of vitest mocks was taking an unproportionate amount of time, I left it this way (at least for now)

I honestly don't know what that means and I understand it has 0 impact on anything important. If I remember this linter behavior, it will now present this warning for any PR (even non-UI related) until it's fixed, right?

I removed it, so no more warning

itaiad200 · 2023-03-13T08:25:20Z

webui/src/lib/remark-plugins/imageUriReplacer.ts

+const imageUriReplacer: Plugin<[], Root> = () => async (tree) => {
+  const images: Array<Node & { url: string }> = [];
+  visit(tree, "image", (node: Node & { url: string }) => {
+    if (node.url.startsWith("lakefs://")) {


I think your assumptions are spot on.

itaiad200 · 2023-03-13T08:31:21Z

webui/src/lib/remark-plugins/imageUriReplacer.ts

+  for (const image of images) {
+    const [repo, branch, ...path] = image.url.split("/").slice(2);
+    const promise = objects
+      .getPresignedUrl(repo, branch, path.join("/"))


How can it assumes presigned is enabled if some installations (local, mem) don't support that at all and in other (Azure) it depends on the configuration?
We have a way to check a installation/repo for presigned support (as it's part of the settings). We can add the images support for repos without presigned urls later, as long as we handle these repos nicely for now, i.e. don't try to embed images for now.

itaiad200 · 2023-03-13T08:32:21Z

webui/src/lib/remark-plugins/imageUriReplacer.ts

+        if (node.url.startsWith("/")) {
+          node.url = node.url.slice(1);
+        }
+        node.url = `lakefs://${options.repo}/${options.branch}/${node.url}`;


I think options.ref is better. It can also be a commit or a tag.

itaiad200

Yay!

ozkatz · 2023-03-15T15:56:38Z

webui/package.json

    "remark-gfm": "^3.0.1",
    "remark-html": "^15.0.1",
+    "unist": "^8.0.11",


@eladlachmi this line fails npm install for me. Couldn't find it on NPM manually either.

Additionally, I think this PR also introduces a package-lock.json file at the root of the repository (i.e. not under webui/) by mistake..

* Display images in markdown with support for lakefs:// URIs

* Display images in markdown with support for lakefs:// URIs (#5449) * Display images in markdown with support for lakefs:// URIs * change dockerfile to include delta diff * change Dockerfile to include the Delta Lake diff plugin * fix docker-compose files * add a stage with both lakefs and plugins and separate it from a lakefs-only stage * PR fixes * Delta Lake Diff: Enable feature (#5508) * fix hover issue when hovering over a tree-entry-row * enable Delta Lake diff feature * remove experimental delta diff button * PR fixes * change docker image publish target to lakefs-plugins * fix bug hunt bugs (#5534) * remove empty line * Delta Lake: Update Dockerfile base images to Alpine (#5546) * change the Dockerfile back to Alpine and build the Delta plugin dynamically * remove alpine-sdk from dockerfile * add alpine-sdk * add comment about the 'RUSTFLAGS=-Ctarget-feature=-crt-static' flag * change back from bullseye to alpine --------- Co-authored-by: eladlachmi <110764839+eladlachmi@users.noreply.github.com>

* change default behavior of plugin loading to scan in the default plugins directory for known plugins * get delta lake diff plugin from default path if available * add homedir.Expand call to custom plugin path * remove empty line Co-authored-by: Barak Amar <barak.amar@treeverse.io> * change version from a pointer to an int * Delta Lake Diff: Dockerfile (#5496) * Display images in markdown with support for lakefs:// URIs (#5449) * Display images in markdown with support for lakefs:// URIs * change dockerfile to include delta diff * change Dockerfile to include the Delta Lake diff plugin * fix docker-compose files * add a stage with both lakefs and plugins and separate it from a lakefs-only stage * PR fixes * Delta Lake Diff: Enable feature (#5508) * fix hover issue when hovering over a tree-entry-row * enable Delta Lake diff feature * remove experimental delta diff button * PR fixes * change docker image publish target to lakefs-plugins * fix bug hunt bugs (#5534) * remove empty line * Delta Lake: Update Dockerfile base images to Alpine (#5546) * change the Dockerfile back to Alpine and build the Delta plugin dynamically * remove alpine-sdk from dockerfile * add alpine-sdk * add comment about the 'RUSTFLAGS=-Ctarget-feature=-crt-static' flag * change back from bullseye to alpine --------- Co-authored-by: eladlachmi <110764839+eladlachmi@users.noreply.github.com> --------- Co-authored-by: Barak Amar <barak.amar@treeverse.io> Co-authored-by: eladlachmi <110764839+eladlachmi@users.noreply.github.com>

* add makefile target to build delta diff plugin * test goreleaser with rust * remove cache step * add activate on push to this branch * change working-directory * remove working-directory * remove target from args * add protoc for mac and name steps * add version to protoc action * switch from Arduino * use correct version * change back to arduino * change version to 3.x * add protoc to unix and windows * change version to 3.x * change to release, upload artifact * downloading artifacts * add needs to releaser * add build * move --release to arguments * fix artifact location * fix naming * add cache * add .exe suffix to windows build target * add id to the download artifact step * add registry and index to cache * add renaming of artifacts * move download-artifacts to the goreleaser job * change cache support to use custom action * run goreleaser without truly releasing * fix goreleaser * change delta artifact location * fix releaser yaml * change releaser to be production ready * change from 3 jobs definitions to a single job definition with a more complex matrix * fix makefile * Delta Lake Diff: Change default plugin loading (#5495) * change default behavior of plugin loading to scan in the default plugins directory for known plugins * get delta lake diff plugin from default path if available * add homedir.Expand call to custom plugin path * remove empty line Co-authored-by: Barak Amar <barak.amar@treeverse.io> * change version from a pointer to an int * Delta Lake Diff: Dockerfile (#5496) * Display images in markdown with support for lakefs:// URIs (#5449) * Display images in markdown with support for lakefs:// URIs * change dockerfile to include delta diff * change Dockerfile to include the Delta Lake diff plugin * fix docker-compose files * add a stage with both lakefs and plugins and separate it from a lakefs-only stage * PR fixes * Delta Lake Diff: Enable feature (#5508) * fix hover issue when hovering over a tree-entry-row * enable Delta Lake diff feature * remove experimental delta diff button * PR fixes * change docker image publish target to lakefs-plugins * fix bug hunt bugs (#5534) * remove empty line * Delta Lake: Update Dockerfile base images to Alpine (#5546) * change the Dockerfile back to Alpine and build the Delta plugin dynamically * remove alpine-sdk from dockerfile * add alpine-sdk * add comment about the 'RUSTFLAGS=-Ctarget-feature=-crt-static' flag * change back from bullseye to alpine --------- Co-authored-by: eladlachmi <110764839+eladlachmi@users.noreply.github.com> --------- Co-authored-by: Barak Amar <barak.amar@treeverse.io> Co-authored-by: eladlachmi <110764839+eladlachmi@users.noreply.github.com> --------- Co-authored-by: Barak Amar <barak.amar@treeverse.io> Co-authored-by: eladlachmi <110764839+eladlachmi@users.noreply.github.com>

eladlachmi added include-changelog PR description should be included in next release changelog team/cloud-native Team cloud native labels Mar 12, 2023

eladlachmi requested review from nopcoder and itaiad200 March 12, 2023 12:11

eladlachmi self-assigned this Mar 12, 2023

itaiad200 requested changes Mar 12, 2023

View reviewed changes

eladlachmi requested a review from itaiad200 March 12, 2023 18:07

itaiad200 requested changes Mar 13, 2023

View reviewed changes

eladlachmi requested a review from itaiad200 March 13, 2023 08:35

eladlachmi added 10 commits March 13, 2023 15:43

Display images in markdown with support for lakefs:// URIs

c536353

fix failing tests

1154515

Remove/replace deprecated packages

986321e

CR fixes

a0228b4

CR fixes

d60158b

CR fixes

fe2fd1d

Replace presigned URLs with regular lakeFS URLs

5d4204c

CR fixes

2b2b138

CR fixes

138dbf8

update lock file after rebase

7514033

eladlachmi force-pushed the 5366-display-images-in-markdown branch from f35a593 to 7514033 Compare March 13, 2023 13:45

itaiad200 approved these changes Mar 14, 2023

View reviewed changes

eladlachmi merged commit 34c243b into master Mar 14, 2023

eladlachmi deleted the 5366-display-images-in-markdown branch March 14, 2023 08:38

ozkatz reviewed Mar 15, 2023

View reviewed changes

Jonathan-Rosenberg pushed a commit that referenced this pull request Mar 15, 2023

Display images in markdown with support for lakefs:// URIs (#5449)

749a974

* Display images in markdown with support for lakefs:// URIs

nopcoder pushed a commit that referenced this pull request Mar 20, 2023

Display images in markdown with support for lakefs:// URIs (#5449)

0f7d91c

* Display images in markdown with support for lakefs:// URIs

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display images in markdown with support for lakefs:// URIs #5449

Display images in markdown with support for lakefs:// URIs #5449

eladlachmi commented Mar 12, 2023

itaiad200 left a comment

itaiad200 Mar 12, 2023

itaiad200 Mar 12, 2023

eladlachmi Mar 12, 2023

itaiad200 Mar 13, 2023

eladlachmi Mar 13, 2023

eladlachmi Mar 13, 2023

itaiad200 Mar 12, 2023

eladlachmi Mar 12, 2023

eladlachmi Mar 12, 2023

itaiad200 Mar 13, 2023 •

edited

Loading

itaiad200 Mar 13, 2023

eladlachmi Mar 13, 2023

itaiad200 Mar 12, 2023

eladlachmi Mar 12, 2023

eladlachmi Mar 12, 2023 •

edited

Loading

itaiad200 left a comment

itaiad200 Mar 13, 2023

eladlachmi Mar 13, 2023

itaiad200 Mar 13, 2023

eladlachmi Mar 13, 2023

itaiad200 Mar 13, 2023 •

edited

Loading

itaiad200 Mar 13, 2023

itaiad200 Mar 13, 2023

eladlachmi Mar 13, 2023

itaiad200 left a comment

ozkatz Mar 15, 2023

Display images in markdown with support for lakefs:// URIs #5449

Display images in markdown with support for lakefs:// URIs #5449

Conversation

eladlachmi commented Mar 12, 2023

itaiad200 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itaiad200 Mar 13, 2023 • edited Loading

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

eladlachmi Mar 12, 2023 • edited Loading

Choose a reason for hiding this comment

itaiad200 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

itaiad200 Mar 13, 2023 • edited Loading

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

itaiad200 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itaiad200 Mar 13, 2023 •

edited

Loading

eladlachmi Mar 12, 2023 •

edited

Loading

itaiad200 Mar 13, 2023 •

edited

Loading