feat: add button to open a file in the editor for the webapp rewrite

[bahrmichael]

For #61629

This is the first iteration of a button to open the currently viewed file in your IDE. I reduced the scope of this task, and will follow up with more PRs. See #61629 (comment).

How it looks when you've configured an editor:

How it looks when you did not configure an editor:

Some utility files and icons have been copied from the old webapp.

Test plan

My plan is to get feedback on this work, review the scoping and what is important to implement now, and then write tests depending on how much coverage we need. There are no automated tests (beyond what was copied) yet, but it manual testing was good.

[sourcegraph-bot]

📖 Storybook live preview

[fkling]

I won't have time to give this a thorough review today, so just a couple of high level points:

• Do we need to copy the business logic? It seems to me we could just import it. Similar with the URL builders which are actually already importing (see src/lib/web.ts I think).
• I don't think the images will work this way in production because the static files are not copied to where the img element is trying to access them. We either need the correct image path prefix or reference the images differently. But I'd have to take a closer look to give suggestions for that.

[fkling]

editor instead of e would be more readable (no Go conventions here ;) )

[bahrmichael]

:D

[fkling]

Does this have to be a string? I think we have GraphQL types for those, or not?

[bahrmichael]

Nope, I'm adding the type from GraphQL 👍

[fkling]

We can still use the function from client/web/. If you are concerned about the link to /users/settings: Clicking it will simply load the React app, so that's not a problem.

[bahrmichael]

My main issue was with the markdown that the client/web function produces. We don't have markdown rendering in our tooltips, so it looked a bit ugly. When I added markdown support, I noticed that the styling of the tooltip got a bit wonky and more importantly I noticed that the Tooltip doesn't stay open when I hover from the button to the tooltip, rendering its link useless. Instead of digging deeper here, I thought it's a cleaner solution to give the button the href action.

[fkling]

Ah, got it. Yes, the tooltip really is a tooltip that doesn't expect to have any interactive contents inside... 🤔 You could use a popover as alternative maybe? Like we use for the experimental badge.

[bahrmichael]

I gave the Popover component a quick spin, and it looks like I'd have to do a significant amount of CSS to align the look and feel to the other actions. In favor of consistent behavior I would stick with the current version of the Tooltip. It may also feel odd that some tooltips of the actions would close on hover and others don't. If you don't have a strong opinion here I would keep the current approach.

In the ticket I noted that I'd scope out the dialog about setting up the editor. That may be a good place to revisit the Popover approach.

Screen.Recording.2024-04-15.at.14.27.48.mov

[fkling]

Forgot this earlier: We should somehow fallback to a default icon if we don't have one for the configured editor, otherwise nothing will be displayed when the actions are in "compact" mode.

[bahrmichael]

Looking at fallbacks and error handling I feel like a script based approach is the way to go. Here's what I came up with:

<script lang="ts">
    import type {Editor} from '$lib/web'
    import DefaultEditorIcon from './DefaultEditorIcon.svelte';
    import type {SvelteComponent} from 'svelte';

    export let editorId: Editor['id']

    let Icon: typeof SvelteComponent | typeof DefaultEditorIcon;
    $: import(`./editors/${editorId}.svelte`)
        .then(i => Icon = i.default)
        .catch((err) => {
            console.error(err);
            Icon = DefaultEditorIcon
        })
</script>

If we were to do it in the component body, it would look something like the version below. That version has an error because error is unhandled, and I don't feel like adding a console.error (or a telemetry call) into the component body would be the right way to go.

{#await import(`./editors/${editorId}.svelte`) then {default: Icon}}
    <Icon />
{:catch error}
    <DefaultEditorIcon />
{/await}

What do you think? Is there another way to do this within the Svelte body?

[fkling]

Just having {:catch} (without variable name) is valid too.

[fkling]

Alternatively, we do have a list of known editor IDs, right? We could test against that list before we try to fetch the icon. Maybe we could even generate that list from the list of files at build time.

[bahrmichael]

Just having {:catch} (without variable name) is valid too.

My IDE was complaining that this was invalid syntax, but I see that svelte check doesn't complain. I'm leaning towards the svelte component body approach then.

[camdencheek]

FYI, even though the UI is confusing and suggests that your branch needs to be up to date with main to merge, you don't actually. Merge will be blocked if there are conflicts, but right now the only reason it's blocked is it doesn't have an approving review (which I'll leave to Felix)

[fkling]

Some minor comments inside, overall looks good!

[fkling]

I don't think this is necessary though I also acknowledge that it's not obvious why this can't error. If parent fails to resolves we will likely render an error elsewhere.

Regardless rendering the alert here probably doesn't look good either way. As a precaution we could instead catch any error in the loader and log it in the console (see below).

Eventually we will collect such errors with sentry (I think).

[bahrmichael]

Applied your suggestion so that we get those errors through sentry. That means I can reduce this part to just an await ... then ... and an inner if. Looks clean to me.

[fkling]

What would happen if someone clicks the button while the data is not available yet? Can that even happen?

[bahrmichael]

Not applicable anymore after #61782 (comment)

[fkling]

FWIW, here is an example of how images can be used by importing as "URL"s. That would also be an acceptable approach instead of converting them all to Svelte files, especially if you can use the existing files.

https://sourcegraph.sourcegraph.com/github.com/sourcegraph/sourcegraph@756a245cee6a1ee8b89f6a7df946cc9e28cb2e8e/-/blob/client/web-sveltekit/src/lib/images.ts?L1-3

[bahrmichael]

FWIW, here is an example of how images can be used by importing as "URL"s. That would also be an acceptable approach instead of converting them all to Svelte files, especially if you can use the existing files.

https://sourcegraph.sourcegraph.com/github.com/sourcegraph/sourcegraph@756a245cee6a1ee8b89f6a7df946cc9e28cb2e8e/-/blob/client/web-sveltekit/src/lib/images.ts?L1-3

That's a nice approach!

However, since we're rebuilding the blob page (and the old OpenInEditorAction is only used there), I'm pushing for duplicating the images so we can later more easily remove the old ones. As far as I see the editor images are not used anywhere else. This keeps cross-client dependencies lower.

[fkling]

Thank you for the tests!

[bahrmichael]

There were some weird UI Review differences that I decided to accept even they look a bit wrong. I have no idea what could cause those.