8000 feat: setup url autofill for dynamic parameters by jaaydenh · Pull Request #17739 · coder/coder · GitHub
[go: up one dir, main page]
Skip to content

feat: setup url autofill for dynamic parameters #17739

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
May 16, 2025
Merged

feat: setup url autofill for dynamic parameters #17739

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
4209d7d
feat: setup autofill for dynamic parameters
jaaydenh May 7, 2025
1d798ea
chore: cleanup
jaaydenh May 8, 2025
9d10195
feat: set initial param values from autofill
jaaydenh May 9, 2025
ec35a7f
fix: setup ParameterLabel to use htmlfor
jaaydenh May 12, 2025
04c7f83
chore: change autofill to boolean
jaaydenh May 12, 2025
03dde8a
chore: updates for PR review comments
jaaydenh May 12, 2025
afdab85
chore: move logic for sending initial parameters to websocket
jaaydenh May 12, 2025
a76159f
fix: improvements to dynamic parameter handling
jaaydenh May 13, 2025
6711e28
fix: updates for PR review
jaaydenh May 15, 2025
7dc96bb
fix: update logic for onMessage
jaaydenh May 15, 2025
eb413fb
fix: format
jaaydenh May 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
feat: setup autofill for dynamic parameters
  • Loading branch information
@jaaydenh
jaaydenh committed May 15, 2025
commit 4209d7dcb6596ecd42b562d38254589adc53992f
91 changes: 77 additions & 14 deletions site/src/modules/workspaces/DynamicParameter/DynamicParameter.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,23 +32,27 @@ import {
TooltipProvider,
TooltipTrigger,
} from "components/Tooltip/Tooltip";
import { Info, Settings, TriangleAlert } from "lucide-react";
import { Info, Link, Settings, TriangleAlert } from "lucide-react";
import { type FC, useEffect, useId, useState } from "react";
import type { AutofillBuildParameter } from "utils/richParameters";
import * as Yup from "yup";

export interface DynamicParameterProps {
parameter: PreviewParameter;
value?: string;
onChange: (value: string) => void;
disabled?: boolean;
isPreset?: boolean;
autofill?: AutofillBuildParameter;
}

export const DynamicParameter: FC<DynamicParameterProps> = ({
parameter,
value,
onChange,
disabled,
isPreset,
autofill,
}) => {
const id = useId();

Expand All @@ -57,13 +61,18 @@ export const DynamicParameter: FC<DynamicParameterProps> = ({
className="flex flex-col gap-2"
data-testid={`parameter-field-${parameter.name}`}
>
<ParameterLabel parameter={parameter} isPreset={isPreset} />
<ParameterLabel
parameter={parameter}
isPreset={isPreset}
autofill={autofill}
/>
<div className="max-w-lg">
<ParameterField
id={id}
parameter={parameter}
value={value}
onChange={onChange}
disabled={disabled}
id={id}
/>
</div>
{parameter.diagnostics.length > 0 && (
Expand All @@ -76,9 +85,14 @@ export const DynamicParameter: FC<DynamicParameterProps> = ({
interface ParameterLabelProps {
parameter: PreviewParameter;
isPreset?: boolean;
autofill?: AutofillBuildParameter;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is only being used for a truthy check to influence the conditional rendering, can we replace this with an autofill boolean?

}

const ParameterLabel: FC<ParameterLabelProps> = ({ parameter, isPreset }) => {
const ParameterLabel: FC<ParameterLabelProps> = ({
parameter,
isPreset,
autofill,
}) => {
const hasDescription = parameter.description && parameter.description !== "";
const displayName = parameter.display_name
? parameter.display_name
Expand Down Expand Up @@ -137,6 +151,23 @@ const ParameterLabel: FC<ParameterLabelProps> = ({ parameter, isPreset }) => {
</Tooltip>
</TooltipProvider>
)}
{autofill && (
<TooltipProvider delayDuration={100}>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something I've been wondering about: when do you think it makes sense to define a global TooltipProvider to act as a default context, making it so that people only need to add a provider specifically to override the duration?

Defining the providers every single time still works, but there's more risk of the durations getting out of sync, which can make the UX feel inconsistent across the product

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default delay duration is too slow, I do think the delayDuration should be set globally but haven't had a chance to do this yet. Already there are inconsistencies between the TooltipProvider and the legacy MUI tooltips.

<Tooltip>
<TooltipTrigger asChild>
<span className="flex items-center">
<Badge size="sm">
<Link />
URL Autofill
</Badge>
</span>
</TooltipTrigger>
<TooltipContent className="max-w-xs">
Autofilled from the URL
</TooltipContent>
</Tooltip>
</TooltipProvider>
)}
</Label>

{hasDescription && (
Expand All @@ -153,22 +184,27 @@ const ParameterLabel: FC<ParameterLabelProps> = ({ parameter, isPreset }) => {

interface ParameterFieldProps {
parameter: PreviewParameter;
value?: string;
onChange: (value: string) => void;
disabled?: boolean;
id: string;
}

const ParameterField: FC<ParameterFieldProps> = ({
parameter,
value,
onChange,
disabled,
id,
}) => {
const value = validValue(parameter.value);
const [localValue, setLocalValue] = useState(value);
const initialValue =
value !== undefined ? value : validValue(parameter.value);
const [localValue, setLocalValue] = useState(initialValue);

useEffect(() => {
setLocalValue(value);
if (value !== undefined) {
setLocalValue(value);
}
}, [value]);
Copy link
Member
@Parkreiner Parkreiner May 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For something like this, you don't want to sync state via useEffect, because that means the UI completes a full render with the wrong data (including painting the screen), the state sync happens, and then you have to redo the whole render (possibly introducing screen flickering)

This is how I'd do it, and the React Docs recommend it, too:

const [localValue, setLocalValue] = useState(
  value !== undefined ? value : validValue(parameter.value),
);
if (value !== undefined && value !== localValue) {
  setLocalValue(value);
}

Setting state mid-render is valid, as long as you eventually hit a case where you stop calling the state setter. Inside an event handler, the state setter will bail out of re-renders if you dispatch a state value that's equal to the value currently in state. That protection is removed during a render to make sure the user doesn't call a state setter unconditionally

This approach limits the "scope of the redo", because what happens is:

  1. Let's say Component A is defined in terms of Component B and Component C
  2. Component A starts rendering
  3. The state changes mid-render
  4. The render for Component A finishes. Any effects and event handlers that were defined inside the component are created as monadic functions, and the JSX object output is returned, which includes component references for Component B and Component C
  5. React sees that the state changed, and knows that the result it produced is invalid
  6. It throws away the effects, event handlers, and JSX objects. At this point, Component B and Component C have not been allowed to start rendering
  7. React redoes the render for Component A
  8. The state stays stable this time around
  9. React knows that the output is fine this time, so it proceeds to use the JSX object output to render Component B and Component C – for the very first time in this specific render cycle
  10. The whole subtree finishes rendering, and then paints the whole updated output to the screen

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though (lol), after writing all that out, I think the better option would be to remove the state entirely, and then pass this to the value prop: value ?? validValue(parameter.value)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How much churn has this component had? I see a bunch of undefined checks for localValue, even though it's guaranteed to always be a string


switch (parameter.form_type) {
Expand Down Expand Up @@ -469,14 +505,14 @@ export const getInitialParameterValues = (
({ name }) => name === parameter.name,
);

const useAutofill =
autofillParam &&
isValidParameterOption(parameter, autofillParam) &&
autofillParam.value;

return {
name: parameter.name,
value:
autofillParam &&
isValidParameterOption(parameter, autofillParam) &&
autofillParam.value
? autofillParam.value
: validValue(parameter.value),
value: useAutofill ? autofillParam.value : validValue(parameter.value),
};
});
};
Expand All @@ -489,14 +525,41 @@ const isValidParameterOption = (
previewParam: PreviewParameter,
buildParam: WorkspaceBuildParameter,
) => {
if (previewParam.form_type === "multi-select") {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this also could have been if (previewParam.type === "list(string) && param.options.length > 0)
but it feels like checking the form_type is more specific and the only case that needs this logic

console.log("buildParam.value", buildParam.value);
let values: string[] = [];
try {
const parsed = JSON.parse(buildParam.value);
if (Array.isArray(parsed)) {
values = parsed;
}
} catch (e) {
console.error(
"Error parsing parameter value with form_type multi-select",
e,
);
return false;
}

// If options exist, validate each value
if (previewParam.options.length > 0) {
const validValues = previewParam.options.map(
(option) => option.value.value,
);
return values.every((value) => validValues.includes(value));
}
return false;
}
// For parameters with options (dropdown, radio, etc.)
if (previewParam.options.length > 0) {
const validValues = previewParam.options.map(
(option) => option.value.value,
);
return validValues.includes(buildParam.value);
}

return false;
// For parameters without options (input, textarea, etc.)
return true;
};

export const useValidationSchemaForDynamicParameters = (
Expand Down
14 changes: 14 additions & 0 deletions site/src/pages/CreateWorkspacePage/CreateWorkspacePageViewExperimental.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import {
useContext,
useEffect,
useId,
useMemo,
useRef,
useState,
} from "react";
Expand Down Expand Up @@ -141,6 +142,14 @@ export const CreateWorkspacePageViewExperimental: FC<
},
});

const autofillByName = useMemo(
() =>
Object.fromEntries(
autofillParameters.map((param) => [param.name, param]),
),
[autofillParameters],
);

useEffect(() => {
if (error) {
window.scrollTo(0, 0);
Expand Down Expand Up @@ -509,6 +518,9 @@ export const CreateWorkspacePageViewExperimental: FC<
return null;
}

const formValue =
form.values?.rich_parameter_values?.[index]?.value || "";

return (
<DynamicParameter
key={parameter.name}
Expand All @@ -518,6 +530,8 @@ export const CreateWorkspacePageViewExperimental: FC<
}
disabled={isDisabled}
isPreset={isPresetParameter}
autofill={autofillByName[parameter.name]}
value={formValue}
/>
);
})}
Expand Down
0