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
Prev Previous commit
Next Next commit
fix: improvements to dynamic parameter handling
  • Loading branch information
@jaaydenh
jaaydenh committed May 15, 2025
commit a76159f21fd86449e67231ebe46d1f977baa0649
237 changes: 145 additions & 92 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,15 +32,17 @@ import {
TooltipProvider,
TooltipTrigger,
} from "components/Tooltip/Tooltip";
import { useDebouncedValue } from "hooks/debounce";
import { useEffectEvent } from "hooks/hookPolyfills";
import { Info, LinkIcon, Settings, TriangleAlert } from "lucide-react";
import { type FC, useId, useState } from "react";
import { type FC, useEffect, useId, useRef, 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;
onChange: (value: string) => Promise<void>;
disabled?: boolean;
isPreset?: boolean;
autofill: boolean;
Expand Down Expand Up @@ -68,13 +70,24 @@ export const DynamicParameter: FC<DynamicParameterProps> = ({
autofill={autofill}
/>
<div className="max-w-lg">
<ParameterField
id={id}
parameter={parameter}
value={value}
onChange={onChange}
disabled={disabled}
/>
{parameter.form_type === "input" ||
parameter.form_type === "textarea" ? (
<DebouncedParameterField
id={id}
parameter={parameter}
value={value}
onChange={onChange}
disabled={disabled}
/>
) : (
<ParameterField
id={id}
parameter={parameter}
value={value}
onChange={onChange}
disabled={disabled}
/>
)}
</div>
{parameter.diagnostics.length > 0 && (
<ParameterDiagnostics diagnostics={parameter.diagnostics} />
Expand Down Expand Up @@ -187,15 +200,15 @@ const ParameterLabel: FC<ParameterLabelProps> = ({
);
};

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

const ParameterField: FC<ParameterFieldProps> = ({
const DebouncedParameterField: FC<DebouncedParameterFieldProps> = ({
parameter,
value,
onChange,
Expand All @@ -205,16 +218,96 @@ const ParameterField: FC<ParameterFieldProps> = ({
const [localValue, setLocalValue] = useState(
value !== undefined ? value : validValue(parameter.value),
);
if (value !== undefined && value !== localValue) {
setLocalValue(value);
const debouncedLocalValue = useDebouncedValue(localValue, 500);
const onChangeEvent = useEffectEvent(onChange);
// prevDebouncedValueRef is to prevent calling the onChangeEvent on the initial render
const prevDebouncedValueRef = useRef<string | undefined>();

useEffect(() => {
if (prevDebouncedValueRef.current !== undefined) {
onChangeEvent(debouncedLocalValue);
}

prevDebouncedValueRef.current = debouncedLocalValue;
}, [debouncedLocalValue, onChangeEvent]);

switch (parameter.form_type) {
case "textarea":
return (
<Textarea
id={id}
className="max-w-2xl"
value={localValue}
onChange={(e) => {
setLocalValue(e.target.value);
}}
onInput={(e) => {
const target = e.currentTarget;
target.style.maxHeight = "700px";
target.style.height = `${target.scrollHeight}px`;
}}
Copy link
Member

@Parkreiner Parkreiner May 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not 100% clear on why this is set up this way, or the nuances of what it's trying to do. onInput is mostly equivalent to onChange (in fact, React made the historical mistake of breaking from the HTML spec, and patching onChange to be more like onInput). I don't think most React components ever need both

I'm guessing that at the very least, the handler is meant to make it so that the textarea only grows when we know for a fact that the user wants to interact with it. In which case, all of this logic can be moved into the onChange handler.

But if the max height is being statically set to 700px, maybe we could make it so that this change triggers in response to the first onFocus event?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved the code to onChange, onFocus does not work as expected.

disabled={disabled}
placeholder={parameter.styling?.placeholder}
required={parameter.required}
/>
);

case "input": {
const inputType = parameter.type === "number" ? "number" : "text";
const inputProps: Record<string, unknown> = {};

if (parameter.type === "number") {
const validations = parameter.validations[0] || {};
const { validation_min, validation_max } = validations;

if (validation_min !== null) {
inputProps.min = validation_min;
}

if (validation_max !== null) {
inputProps.max = validation_max;
}
}

return (
<Input
id={id}
type={inputType}
value={localValue}
onChange={(e) => {
setLocalValue(e.target.value);
}}
disabled={disabled}
required={parameter.required}
placeholder={parameter.styling?.placeholder}
{...inputProps}
/>
);
}
}
};

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

const ParameterField: FC<ParameterFieldProps> = ({
parameter,
value,
onChange,
disabled,
id,
}) => {
switch (parameter.form_type) {
case "dropdown":
return (
<Select
onValueChange={onChange}
value={localValue}
value={value}
disabled={disabled}
required={parameter.required}
>
Expand All @@ -234,7 +327,15 @@ const ParameterField: FC<ParameterFieldProps> = ({
);

case "multi-select": {
const values = parseStringArrayValue(localValue ?? "");
const parsedValues = parseStringArrayValue(value ?? "");

if (parsedValues.error) {
return (
<p className="text-sm text-content-destructive">
{parsedValues.error}
</p>
);
}

// Map parameter options to MultiSelectCombobox options format
const options: Option[] = parameter.options.map((opt) => ({
Expand All @@ -247,7 +348,7 @@ const ParameterField: FC<ParameterFieldProps> = ({
parameter.options.map((opt) => [opt.value.value, opt.name]),
);

const selectedOptions: Option[] = values.map((val) => {
const selectedOptions: Option[] = parsedValues.values.map((val) => {
return {
value: val,
label: optionMap.get(val) || val,
Expand Down Expand Up @@ -279,13 +380,21 @@ const ParameterField: FC<ParameterFieldProps> = ({
}

case "tag-select": {
const values = parseStringArrayValue(localValue ?? "");
const parsedValues = parseStringArrayValue(value ?? "");

if (parsedValues.error) {
return (
<p className="text-sm text-content-destructive">
{parsedValues.error}
</p>
);
}

return (
<TagInput
id={id}
label={parameter.display_name || parameter.name}
values={values}
values={parsedValues.values}
onChange={(values) => {
onChange(JSON.stringify(values));
}}
Expand All @@ -297,7 +406,7 @@ const ParameterField: FC<ParameterFieldProps> = ({
return (
<Switch
id={id}
checked={localValue === "true"}
checked={value === "true"}
onCheckedChange={(checked) => {
onChange(checked ? "true" : "false");
}}
Expand All @@ -307,11 +416,7 @@ const ParameterField: FC<ParameterFieldProps> = ({

case "radio":
return (
<RadioGroup
onValueChange={onChange}
disabled={disabled}
value={localValue}
>
<RadioGroup onValueChange={onChange} disabled={disabled} value={value}>
{parameter.options.map((option) => (
<div
key={option.value.value}
Expand All @@ -337,7 +442,7 @@ const ParameterField: FC<ParameterFieldProps> = ({
<div className="flex items-center space-x-2">
<Checkbox
id={id}
checked={localValue === "true"}
checked={value === "true"}
onCheckedChange={(checked) => {
onChange(checked ? "true" : "false");
}}
Expand All @@ -353,9 +458,8 @@ const ParameterField: FC<ParameterFieldProps> = ({
<Slider
id={id}
className="mt-2"
value={[Number(localValue)]}
value={[Number(value)]}
onValueChange={([value]) => {
setLocalValue(value.toString());
onChange(value.toString());
}}
min={parameter.validations[0]?.validation_min ?? 0}
Expand All @@ -365,79 +469,32 @@ const ParameterField: FC<ParameterFieldProps> = ({
<span className="w-4 font-medium">{parameter.value.value}</span>
</div>
);

case "textarea":
return (
<Textarea
id={id}
className="max-w-2xl"
value={localValue}
onChange={(e) => {
setLocalValue(e.target.value);
onChange(e.target.value);
}}
onInput={(e) => {
const target = e.currentTarget;
target.style.maxHeight = "700px";
target.style.height = `${target.scrollHeight}px`;
}}
disabled={disabled}
placeholder={parameter.styling?.placeholder}
required={parameter.required}
/>
);

case "input": {
const inputType = parameter.type === "number" ? "number" : "text";
const inputProps: Record<string, unknown> = {};

if (parameter.type === "number") {
const validations = parameter.validations[0] || {};
const { validation_min, validation_max } = validations;

if (validation_min !== null) {
inputProps.min = validation_min;
}

if (validation_max !== null) {
inputProps.max = validation_max;
}
}

return (
<Input
id={id}
type={inputType}
value={localValue}
onChange={(e) => {
setLocalValue(e.target.value);
onChange(e.target.value);
}}
disabled={disabled}
required={parameter.required}
placeholder={parameter.styling?.placeholder}
{...inputProps}
/>
);
}
}
};

const parseStringArrayValue = (value: string): string[] => {
let values: string[] = [];
type ParsedValues = {
values: string[];
error: string;
};

const parseStringArrayValue = (value: string): ParsedValues => {
const parsedValues: ParsedValues = {
values: [],
error: "",
};

if (value) {
try {
const parsed = JSON.parse(value);
if (Array.isArray(parsed)) {
values = parsed;
parsedValues.values = parsed;
}
} catch (e) {
console.error("Error parsing parameter of type list(string)", e);
parsedValues.error = `Error parsing parameter of type list(string), ${e}`;
}
}

return values;
return parsedValues;
};

interface OptionDisplayProps {
Expand Down Expand Up @@ -542,10 +599,6 @@ const isValidParameterOption = (
values = parsed;
}
} catch (e) {
console.error(
"Error parsing parameter value with form_type multi-select",
e,
);
return false;
}

Expand Down
Loading