feat: popover to configure editor #62452

bahrmichael · 2024-05-06T13:28:26Z

This PR introduces a new popover to configure the editor.

I had to extend the initial settings to get the current settings, introduce new methods to update settings, and handle the two-step update (because each SettingsEdit can only modify one field). With this solution we get the same look-and-feel like the React app, and the editor icon updates without a page reload.

There's a new updateUserSetting on the root layout, with which this change and other features can update user settings. It fetches the lastID before writing a change, which means it will force the given setting and won't recheck if something changed.

Screen.Recording.2024-05-14.at.13.52.10.mov

Test plan

Manual testing, see video above

bahrmichael · 2024-05-13T14:20:59Z

Update: Since the comment below, I reworked the code to use svelte stores which makes this look nicer. It could still need some improvement, but I'm happier with the code now.

The current setup works, but I'm not very happy with it. The main problem is that editSettings needs the latest settings ID (which comes from the subjects), and can only modify one field at a time.

So to get the lastID when starting out (to make sure that the user's settings haven't been modified in another tab or so), we need to get the lastID that was present when the user open the popover. The lastID changes when we make an edit call, so we have to request it again in between.

I'm hoping there's another way to make this work nicely, but couldn't find one yet. Maybe there's a better Svelte-style approach?

fkling

I left a couple of comments inline. This might be a tricky one because I assume you just want to get this "done", but at the same time its introducing new behavior (updating user settings) for which it makes sense to have a reasonable API, instead of just doing it "ad hoc". Let me know what you think!

client/web-sveltekit/src/lib/repo/open-in-editor/OpenInEditor.svelte

fkling · 2024-05-15T14:28:14Z

client/web-sveltekit/src/routes/+layout.ts

        settings,
+        subjects,


I was hoping we can get away without having to expose individual subjects, but I guess not. However, if we have to expose both then I think we should expose viewerSettings directly (subjects by itself isn't very descriptive). That would require updating all places where we access data.settings of course (of which there seem only two atm).

fkling · 2024-05-15T14:30:05Z

client/web-sveltekit/src/lib/repo/open-in-editor/OpenInEditor.svelte

    $: start = position ?? range?.start
+
+    $: lastId = writable<number>(subjects.at(-1)?.latestSettings?.id);


I don't know whether we can assume that the last subject is the user one. Looking at the React code, they are extracting the subject with type 'user' which seems safer.

fkling · 2024-05-15T14:58:00Z

client/web-sveltekit/src/routes/[...repo=reporev]/(validrev)/(code)/-/blob/[...path]/+page.ts

+        updateEditor: async (subject: string, lastID: number, edit: SettingsEdit) => {
+            const mutationResult1 = await client.mutation(
+                EditSettings,
+                {
+                    lastID,
+                    subject,
+                    edit,
+                },
+                { requestPolicy: 'network-only', fetch }
+            )
+            if (!mutationResult1.data || mutationResult1.error) {
+                throw new Error(`Failed to update editor path: ${mutationResult1.error}`)
+            }
+            const settingsResult = await client.query(LatestSettingsQuery, {})
+            if (!settingsResult.data || settingsResult.error) {
+                throw new Error(`Failed to fetch latest settings during editor update: ${settingsResult.error}`)
+            }
+            const newLastId = settingsResult.data.viewerSettings.subjects.at(-1)?.latestSettings?.id
+            if (!newLastId) {
+                throw new Error('Failed to get new last ID from settings result')
+            }
+            return newLastId
+        },


The function is called updateEditor but it seems possible to update arbitrary settings with this function.
I'd suggest to create a generic updateSettings function in the root layout data loader.
Looking at the React version it also seems they are retrying the mutation when there was a version error, so the "current" settings ID doesn't seem necessary. Relevant code: https://sourcegraph.sourcegraph.com/github.com/sourcegraph/sourcegraph@cb03009ca0fb1c504f3903bf48e7f5531b6d1cb4/-/blob/client/web/src/platform/context.ts?L51-85

If I can just refresh the IDs it makes things a lot easier! I'll push a new version where I was able to remove a lot of code, and pull the lastID before sending a request.

fkling · 2024-05-15T15:08:28Z

client/web-sveltekit/src/lib/repo/open-in-editor/OpenInEditor.svelte

+    $: handleEditorUpdate = async (): Promise<void> => {
+        if (!$selectedEditorId || !$defaultProjectPath) {
+            return;
+        }
+        isSaving = true;
+        const newLastId1 = await updateEditor($subjectId, $lastId, {
+            value: $defaultProjectPath,
+            keyPath: [{property: 'openInEditor'}, {property: 'projectPaths.default'}],
+        });
+        lastId.set(newLastId1);
+        const newLastId2 = await updateEditor($subjectId, $lastId, {
+            value: [$selectedEditorId],
+            keyPath: [{property: 'openInEditor'}, {property: 'editorIds'}],
+        });
+        lastId.set(newLastId2);
+        isSaving = false;
+
+        openInEditor = {
+            editorIds: [$selectedEditorId],
+            'projectPaths.default': $defaultProjectPath,
+        }
+    }


I think it would make sense to abstract some of details of updating settings. E.g. we could provide an API that actually takes an arbitrary number of fields so that the details of the lastId stuff is hidden.
For reference, here is the React version: https://sourcegraph.sourcegraph.com/github.com/sourcegraph/sourcegraph@cb03009ca0fb1c504f3903bf48e7f5531b6d1cb4/-/blob/client/web/src/open-in-editor/OpenInEditorActionItem.tsx?L80-95

I pushed a new version that hides the lastID.

bahrmichael · 2024-05-17T12:33:36Z

I left a couple of comments inline. This might be a tricky one because I assume you just want to get this "done", but at the same time its introducing new behavior (updating user settings) for which it makes sense to have a reasonable API, instead of just doing it "ad hoc". Let me know what you think!

I'm happy to take the hard route. This was my attempt to get something done, so that we can iterate together on making it nice :)

…svelte Co-authored-by: Felix Kling <felix@felix-kling.de>

graphite-app · 2024-05-17T15:09:31Z

(Notifying @sourcegraph/source of a change that affects gitserver)

…nction and move it into root

bahrmichael · 2024-05-17T15:52:08Z

@fkling I've done another pass, and it looks much cleaner now. The main change was to drop the initial subjects fetch, assuming that we can fetch the lastID before submitting the SettingsEdit.

I'm happy to merge this if you agree.

fkling

lgtm, just some nits inside.

client/web-sveltekit/src/lib/repo/open-in-editor/OpenInEditor.svelte

…svelte Co-authored-by: Felix Kling <felix@felix-kling.de>

wip: popover with styling; refactoring and mutate call tbd

4828232

cla-bot bot added the cla-signed label May 6, 2024

bahrmichael and others added 2 commits May 13, 2024 12:46

it works, but it ain't pretty

779c2e6

Merge branch 'main' into bahrmichael/62292

2b85283

bahrmichael added 5 commits May 14, 2024 13:57

chore: rework settings calls into svelte stores

aedc1ef

chore: format imports

2a903f0

chore: format imports

d1c07b1

chore: clean up imports

ef1039e

Merge branch 'main' into bahrmichael/62292

70aac19

bahrmichael marked this pull request as ready for review May 14, 2024 12:14

bahrmichael requested a review from a team May 14, 2024 12:18

fix: resolve type issues

3d65574

fkling reviewed May 15, 2024

View reviewed changes

Update client/web-sveltekit/src/lib/repo/open-in-editor/OpenInEditor.…

565e790

…svelte Co-authored-by: Felix Kling <felix@felix-kling.de>

graphite-app bot added the team/source Tickets under the purview of Source - the one Source to graph it all label May 17, 2024

bahrmichael force-pushed the bahrmichael/62292 branch from eede6e6 to 565e790 Compare May 17, 2024 15:12

bahrmichael and others added 4 commits May 17, 2024 17:13

refactor: drop init-loading, refetch ID on update, refactor update fu…

9f81003

…nction and move it into root

Merge branch 'main' into bahrmichael/62292

9933553

chore: cleanup

8dfdaad

chore: cleanup

3ae8123

bahrmichael requested a review from fkling May 17, 2024 15:52

bahrmichael added 2 commits May 21, 2024 16:24

Merge branch 'main' into bahrmichael/62292

43fba81

Merge branch 'main' into bahrmichael/62292

90a046d

fkling approved these changes May 22, 2024

View reviewed changes

bahrmichael and others added 2 commits May 23, 2024 14:01

Update client/web-sveltekit/src/lib/repo/open-in-editor/OpenInEditor.…

a9154ce

…svelte Co-authored-by: Felix Kling <felix@felix-kling.de>

feat: system aware examples, and labels instead of paragraphs

807556d

bahrmichael added 2 commits May 23, 2024 16:12

fix: resolve sveltecheck issues

cc56993

fix: sg lint

7caab3c

bahrmichael merged commit d8284e3 into main May 23, 2024
8 checks passed

bahrmichael deleted the bahrmichael/62292 branch May 23, 2024 14:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: popover to configure editor #62452

feat: popover to configure editor #62452

bahrmichael commented May 6, 2024 •

edited

bahrmichael commented May 13, 2024 •

edited

fkling left a comment

fkling May 15, 2024

fkling May 15, 2024

fkling May 15, 2024

bahrmichael May 17, 2024

fkling May 15, 2024

bahrmichael May 17, 2024 •

edited

bahrmichael commented May 17, 2024

graphite-app bot commented May 17, 2024

bahrmichael commented May 17, 2024

fkling left a comment

		$: start = position ?? range?.start

		$: lastId = writable<number>(subjects.at(-1)?.latestSettings?.id);

feat: popover to configure editor #62452

feat: popover to configure editor #62452

Conversation

bahrmichael commented May 6, 2024 • edited

Test plan

bahrmichael commented May 13, 2024 • edited

fkling left a comment

Choose a reason for hiding this comment

fkling May 15, 2024

Choose a reason for hiding this comment

fkling May 15, 2024

Choose a reason for hiding this comment

fkling May 15, 2024

Choose a reason for hiding this comment

bahrmichael May 17, 2024

Choose a reason for hiding this comment

fkling May 15, 2024

Choose a reason for hiding this comment

bahrmichael May 17, 2024 • edited

Choose a reason for hiding this comment

bahrmichael commented May 17, 2024

graphite-app bot commented May 17, 2024

bahrmichael commented May 17, 2024

fkling left a comment

Choose a reason for hiding this comment

bahrmichael commented May 6, 2024 •

edited

bahrmichael commented May 13, 2024 •

edited

bahrmichael May 17, 2024 •

edited