feat: add user-level parameter autofill #11731

ammario · 2024-01-21T19:59:14Z

TODO:

Add backend endpoint
Get frontend working
Write frontend tests
Update docs

Implementation

This PR solves #10478 by auto-filling previously used template values in create and update workspace flows.

I decided against explicit user values in settings for these reasons:

Autofill is far easier to implement
Users benefit from autofill by default — we don't need to teach them new concepts
If we decide that autofill creates more harm than good, we can remove it without breaking compatibility

ammario · 2024-01-21T23:09:45Z

@BrunoQuaresma there are some minor testing issues I have to fix, but curious for your review on the core of the implementation.

BrunoQuaresma

FE looks good!

mtojek

First round done, I focused mostly on the backend side and the database layer. I see that you use Graphite, so do you mind splitting it into chunks? Once the backend side is done, we can focus on FE logic.

I'd like to hear more voices around the API design, so adding @spikecurtis and @mafredri to evaluate user experience.

coderd/coderd.go

coderd/database/queries/workspacebuildparameters.sql

codersdk/users.go

mtojek · 2024-01-22T13:44:55Z

site/src/utils/richParameters.ts

@@ -4,25 +4,43 @@ import {
 } from "api/typesGenerated";
 import * as Yup from "yup";

+export type AutofillSource = "user_history" | "url" | "active_build";


I haven't reviewed the frontend side thoroughly yet, but I'd rather pull the source information from the backend side, either the previous build or the current one. URL will of course override the auto-fill.

That would involve a wider refactor that I think is out of scope for this PR.

Oh, I see. I assumed that it would be mostly a modification of the database query you'd like to introduce.

spikecurtis

API semantics seem fine to me. I could imagine us adding additional query parameters in the future to select a specific named parameter and/or return multiple previous values for multiple choice.

coderd/database/dbauthz/dbauthz.go

coderd/users.go

coderd/users_test.go

spikecurtis · 2024-01-23T05:17:23Z

coderd/database/dbmem/dbmem.go

+
+	userWorkspaceBuildParameters := make([]database.GetUserWorkspaceBuildParametersRow, 0)
+	for _, wbp := range q.workspaceBuildParameters {
+		userWorkspaceBuildParameters = append(userWorkspaceBuildParameters, database.GetUserWorkspaceBuildParametersRow{


this returns every build parameter for every user, and also includes a named parameter multiple times if there are multiple builds. You need to match the join and filtering behavior of the SQL query

I second this comment. SQL query and dbmem.go diverged. It should be relatively easy to catch these nit-picks with unit tests involving multiple workspaces, users, and ephemeral parameters (ephemeral = false).

Built out the unit test a little bit in the interest of getting this wrapped up today.

coderd/users_test.go

codersdk/users.go

coderd/users_test.go

mtojek

Third round, and... I have reviewed site.

coderd/users.go

mtojek · 2024-01-23T09:57:49Z

coderd/database/dbmem/dbmem.go

+
+	userWorkspaceBuildParameters := make([]database.GetUserWorkspaceBuildParametersRow, 0)
+	for _, wbp := range q.workspaceBuildParameters {
+		userWorkspaceBuildParameters = append(userWorkspaceBuildParameters, database.GetUserWorkspaceBuildParametersRow{


I second this comment. SQL query and dbmem.go diverged. It should be relatively easy to catch these nit-picks with unit tests involving multiple workspaces, users, and ephemeral parameters (ephemeral = false).

mtojek · 2024-01-23T10:03:41Z

coderd/database/queries/workspacebuildparameters.sql

+        wbp.name,
+        wbp.value,
+        wb.created_at,
+        ROW_NUMBER() OVER (PARTITION BY wbp.name ORDER BY wb.created_at DESC) as rn


Thinking loud - maybe we should extend ORDER BY to sort by value too. I can a case where we have 2 workspace builds:

name = "marcin"

name = "john"

depending on database behavior, the engine can mix order, which is usually the source of flakiness in our tests.

This needs to select all data, instead we could remove the partition and simply do SELECT DISTINCT ON (wbp.name) wbp.name, ... ORDER BY wb.created_at DESC, ... this lets Postgres eliminate rows early.

Not going to address right now in the interest of haste. It's unclear how useful this feature will be in its current form.

I suspect that @mafredri suggested a simpler, more performant query.

site/e2e/constants.ts

mtojek · 2024-01-23T10:36:57Z

site/src/components/RichParameterInput/RichParameterInput.tsx

+            {
+              {
+                ["url"]: "value supplied by URL.",
+                ["user_history"]: "recently used value.",
+              }[autofillSource]
+            }


nit: you can extract this block to a separate const for readability purposes

{autofillSource && autofillSource !== "active_build" && ( <div css={{ marginTop: 4, fontSize: 12 }}> 🪄 Autofilled:{" "} {autofillSourceLabel[autofillSource]} </div> )} const autofillSourceLabel = { url: "value supplied by URL.", user_history: "recently used value.", };

hmm I think it's more followable to keep everything in the same JSX block

I guess it depends on which syntax you like more.

site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx

mtojek · 2024-01-23T10:52:39Z

site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.stories.tsx

+      {
+        name: "first_parameter",
+        value: "It works!",
+        source: "user_history",


nit: maybe we should highlight that source is enum?

for example:

enum AutofillSource { UserHistory = "user_history", ActiveBuild = "active_build", Foobar = "foobar", }

Why would that be better?

{ name: "first_parameter", value: "It works!", source: "user_history", }

Again, readability. When you see a block like the above one, it can be misleading that the source can take any string.

mtojek · 2024-01-23T10:59:44Z

site/src/utils/richParameters.ts

+      };
+    }
+
+    const autofillParam = autofillParams?.find(


I think you can make it shorter:

const autofillParam = autofillParams?.find(({ name }) => name === parameter.name); // Use a ternary operator for concise conditional logic return { name: parameter.name, value: autofillParam && isValidValue(parameter, autofillParam) ? autofillParam.value : parameter.default_value, };

mafredri · 2024-01-23T13:33:04Z

coderd/database/dbmem/dbmem.go

@@ -3758,6 +3758,32 @@ func (q *FakeQuerier) GetUserLinksByUserID(_ context.Context, userID uuid.UUID)
 	return uls, nil
 }

+func (q *FakeQuerier) GetUserWorkspaceBuildParameters(_ context.Context, params database.GetUserWorkspaceBuildParametersParams) ([]database.GetUserWorkspaceBuildParametersRow, error) {


Params validation is missing here.

what do you expect to see?

Validation in dbmem.go?

Copied from above. Code-gen should’ve added this 🤔

if err := validateDatabaseType(params); err != nil { return database.UserLink{}, err }

mafredri · 2024-01-23T13:39:06Z

coderd/database/queries/workspacebuildparameters.sql

+        wbp.name,
+        wbp.value,
+        wb.created_at,
+        ROW_NUMBER() OVER (PARTITION BY wbp.name ORDER BY wb.created_at DESC) as rn


This needs to select all data, instead we could remove the partition and simply do SELECT DISTINCT ON (wbp.name) wbp.name, ... ORDER BY wb.created_at DESC, ... this lets Postgres eliminate rows early.

coderd/users.go

mafredri · 2024-01-23T13:44:36Z

docs/templates/parameters.md

+
+## Create Autofill
+
+When the template doesn't specify default values, Coder may still autofill


As a user, I might want the default value to be overridden, even when one is specified. Just a thought.

Yeah, I'm a bit fuzzy on how these different concepts should override each other. I think we leave it as is and let users complain either way.

mafredri · 2024-01-23T13:49:59Z

site/src/components/TemplateParameters/TemplateParameters.tsx


 export type TemplateParametersSectionProps = {
  templateParameters: TemplateVersionParameter[];
+  autofillSources?: Record<string, AutofillSource>;


Suggested change

autofillSources?: Record<string, AutofillSource>;

autofillSources?: Record<string, AutofillSource?>;

I imagine this change is more truthful, unless Records somehow override this (been a while since I wrote TS types).

Hmm, there is already the concept of non-existence built into the record type, and I don't the type to allow for an autofill without a specified source.

ammario · 2024-01-23T20:24:47Z

I'm a bit crunched for time so I'm going to address the important issues and then re-request @mtojek review.

github-actions · 2024-01-23T20:49:49Z

✔️ PR 11731 Updated successfully.
🚀 Access the credentials here.

cc: @ammario

bpmct · 2024-01-23T22:00:51Z

After trying this, I think the result will be jarring to users. The majority of forms in web apps do not autofill all of a user's previous values. The address field in a billing checkout form is an exception, but I think this is often handled by the browser or a custom component/database that stores a user's billing info. (For example, Amazon.com stores a user's shipping addresses).

For infrastructure provisioning platforms (e.g. GCP, Terraform Enterprise), service catalogs, or other products such as GitHub/Vercel, there is no autofill of last values. However, unlike Coder, there are no massively repetitive inputs.

Turning to how other CDE products solve this problem, options such as "dotfiles URI" and "region" are managed in the user settings, not per template. Because of Coder's flexibility, this is more difficult. For example, different templates have different definitions of "region," if any, and there is no guarantee that every template will apply a user's dotfiles URI.

For that reason, I think auto-filling specific parameters per template is an acceptable solution. However, I think that some enhancements could make this better-received:

This introduces a significant change to the default property for coder_parameter. I think we should change the provider docs to reflect how that value will only be the default for the first workspace a user creates in the web UI
For folks using lots of Coder parameters in their templates (e.g. they are treating it kind of like a service catalog), this behavior feels quite unnatural. I would be surprised if I logged into GCP to create an instance and it had my previous values filled out. For this reason, I suggest we either make autofill parameters opt-in or opt-out. My preference is opt-in as I can think of significantly fewer parameters that benefit from autocomplete vs ones where it's not necessary/common.

As you mentioned in the PR description, we can revert if it creates more harm than good. Plus, all we can do is speculate. I'm fine shipping as-is, but wanted to share my concerns about this UX. I'm genuinely not sure if this will be ultimately more convenient (such as how VS Code opens up to your last project by default), but I am skeptical since I don't (remember) seeing this behavior in other software.

I do see opportunities for us to reduce confusion (and manual input) around both of these common parameters:

region:
I see our users often getting confused by the difference between workspace proxies and regions within a template. I think there is an opportunity for us to simplify these concepts for developers, even though it is, under the hood, different and complex. @kylecarbs has mentioned using lat/long coordinates and a magic parameter to match folks to the closet region for a template

dotfiles:
dotfiles is a power user feature. I like the idea of some parameters (e.g. dotfiles, group_id) having memory across multiple templates down the road but not showing up in the core "create workspace" form or showing that they are autofilled from user settings and can be overridden, kind of like an address in checkout form

mtojek

As you mentioned in the PR description, we can revert if it creates more harm than good. Plus, all we can do is speculate. I'm fine shipping as-is, but wanted to share my concerns about this UX.

Yeah, this is purely speculation, and I'm also concerned about changing the default behavior. It should be relatively easy to revert, although we will break the API unfortunately. This is something we have to acknowledge, @bpmct.

Anyway, it looks like the most important comments are addressed so I'm not blocking this. I'm curious to know if we must apply this one from @mafredri too.

Otherwise, LGTM 👍

mtojek · 2024-01-24T08:25:11Z

site/src/components/RichParameterInput/RichParameterInput.tsx

+            {
+              {
+                ["url"]: "value supplied by URL.",
+                ["user_history"]: "recently used value.",
+              }[autofillSource]
+            }


I guess it depends on which syntax you like more.

mtojek · 2024-01-24T08:26:45Z

site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.stories.tsx

+      {
+        name: "first_parameter",
+        value: "It works!",
+        source: "user_history",


{ name: "first_parameter", value: "It works!", source: "user_history", }

Again, readability. When you see a block like the above one, it can be misleading that the source can take any string.

mtojek · 2024-01-24T08:31:08Z

site/src/utils/richParameters.ts

@@ -4,25 +4,43 @@ import {
 } from "api/typesGenerated";
 import * as Yup from "yup";

+export type AutofillSource = "user_history" | "url" | "active_build";


Oh, I see. I assumed that it would be mostly a modification of the database query you'd like to introduce.

mtojek · 2024-01-24T08:33:09Z

coderd/database/queries/workspacebuildparameters.sql

+        wbp.name,
+        wbp.value,
+        wb.created_at,
+        ROW_NUMBER() OVER (PARTITION BY wbp.name ORDER BY wb.created_at DESC) as rn


I suspect that @mafredri suggested a simpler, more performant query.

mtojek · 2024-01-24T08:33:56Z

coderd/database/dbmem/dbmem.go

@@ -3758,6 +3758,32 @@ func (q *FakeQuerier) GetUserLinksByUserID(_ context.Context, userID uuid.UUID)
 	return uls, nil
 }

+func (q *FakeQuerier) GetUserWorkspaceBuildParameters(_ context.Context, params database.GetUserWorkspaceBuildParametersParams) ([]database.GetUserWorkspaceBuildParametersRow, error) {


Validation in dbmem.go?

mafredri · 2024-01-24T09:10:50Z

coderd/users.go

+		return
+	}
+
+	var sdkParams []codersdk.UserParameter


Suggested change

var sdkParams []codersdk.UserParameter

sdkParams := []codersdk.UserParameter{}

To handle the no rows case with empty array vs null, unless httpapi Write already handles it.

ammario added 2 commits January 21, 2024 13:43

feat: add user-level parameter auto-fill

efeb85e

Get coderd tests passing

71bc50d

github-actions bot assigned ammario Jan 21, 2024

This comment was marked as off-topic.

Sign in to view

ammario marked this pull request as ready for review January 21, 2024 20:03

FE works!

73594f9

ammario force-pushed the user-params branch from b07530b to 73594f9 Compare January 21, 2024 20:42

ammario added 8 commits January 21, 2024 14:50

Fix lint and tests

9bbc251

Fix FE tests

d11fb58

Fix swagger ID

1c70e36

make gen & docs

2b0783f

make fmt

1a28ef2

Slight docs edit

1569223

Fix go test

2d544d8

site: use distinct coderd pprof port

93deee5

ammario force-pushed the user-params branch from d53a2f4 to 93deee5 Compare January 21, 2024 22:16

Minor fixes

51cc138

ammario requested a review from BrunoQuaresma January 21, 2024 23:08

ammario changed the title ~~feat: add user-level parameter auto-fill~~ feat: add user-level parameter autofill Jan 21, 2024

Fix one of the e2e tests

e61491b

Fmt and fix e2e test

1a1d0fa

ammario force-pushed the user-params branch from 460eb87 to 1a1d0fa Compare January 22, 2024 03:26

Fix ephemeral parameters

6cb0e29

ammario requested a review from mtojek January 22, 2024 04:05

Minor fixes

76e0018

ammario force-pushed the user-params branch from 3833244 to 76e0018 Compare January 22, 2024 04:06

BrunoQuaresma approved these changes Jan 22, 2024

View reviewed changes

mtojek reviewed Jan 22, 2024

View reviewed changes

mtojek requested a review from spikecurtis January 22, 2024 13:49

ammario force-pushed the user-params branch from d3c276e to 4b00f45 Compare January 22, 2024 19:32

ammario requested a review from mtojek January 22, 2024 19:32

spikecurtis reviewed Jan 23, 2024

View reviewed changes

johnstcn reviewed Jan 23, 2024

View reviewed changes

coderd/users_test.go Outdated Show resolved Hide resolved

mtojek reviewed Jan 23, 2024

View reviewed changes

mafredri reviewed Jan 23, 2024

View reviewed changes

ammario force-pushed the user-params branch 4 times, most recently from 027d4f3 to e37022a Compare January 23, 2024 20:18

ammario force-pushed the user-params branch 2 times, most recently from bdd9f7f to 2c1cbbb Compare January 23, 2024 20:33

Address spike + mathias + cian comments

69ed7c0

ammario force-pushed the user-params branch from 2c1cbbb to 69ed7c0 Compare January 23, 2024 20:40

Merge remote-tracking branch 'origin/main' into user-params

5417e09

ammario requested a review from mtojek January 23, 2024 20:47

ammario force-pushed the user-params branch from 2d4f260 to 392aff9 Compare January 23, 2024 20:59

Fix tests

72f1e88

ammario force-pushed the user-params branch from 392aff9 to 72f1e88 Compare January 23, 2024 21:09

mtojek approved these changes Jan 24, 2024

View reviewed changes

mafredri reviewed Jan 24, 2024

View reviewed changes

ammario added 3 commits January 30, 2024 14:24

Optimize SQL per Mathias suggestion

b0eece8

Merge remote-tracking branch 'origin/main' into user-params

ba18f20

make gen

a6db979

ammario merged commit adbb025 into main Jan 30, 2024

ammario deleted the user-params branch January 30, 2024 22:02

github-actions bot locked and limited conversation to collaborators Jan 30, 2024


		## Create Autofill

		When the template doesn't specify default values, Coder may still autofill

	autofillSources?: Record<string, AutofillSource>;
	autofillSources?: Record<string, AutofillSource?>;

	var sdkParams []codersdk.UserParameter
	sdkParams := []codersdk.UserParameter{}

feat: add user-level parameter autofill #11731

feat: add user-level parameter autofill #11731

Conversation

ammario commented Jan 21, 2024 • edited Loading

Implementation

This comment was marked as off-topic.

ammario commented Jan 21, 2024

BrunoQuaresma left a comment

Choose a reason for hiding this comment

mtojek 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

spikecurtis 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

mtojek left a comment • 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

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

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

ammario commented Jan 23, 2024

github-actions bot commented Jan 23, 2024 • edited Loading

✔️ PR 11731 Updated successfully. 🚀 Access the credentials here.

bpmct commented Jan 23, 2024 • edited Loading

mtojek 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

ammario commented Jan 21, 2024 •

edited

Loading

mtojek left a comment •

edited

Loading

github-actions bot commented Jan 23, 2024 •

edited

Loading

✔️ PR 11731 Updated successfully.
🚀 Access the credentials here.

bpmct commented Jan 23, 2024 •

edited

Loading