- Notifications
You must be signed in to change notification settings - Fork906
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
@@ -489,14 +525,41 @@ const isValidParameterOption = ( | |||
previewParam: PreviewParameter, | |||
buildParam: WorkspaceBuildParameter, | |||
) => { | |||
// multi-select is the only list(string) type with options | |||
if (previewParam.form_type === "multi-select") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this also could have beenif (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
// This ensures the backend has the complete initial state of the form, | ||
// which is vital for correctly rendering dynamic UI elements where parameter visibility | ||
// or options might depend on the initial values of other parameters. | ||
const hasInitializedWebsocket = useRef(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
An alternative to avoid the boolean ref and the dependency array could be to add refs for the dependencies
const currentParameters = initialParametersRef.current;const currentRichParams = initialRichParameterValuesRef.current;const currentSendMessage = sendMessageRef.current;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Have a few changes I think we should make. I'm happy to pair on Monday
const initialValue = | ||
value !== undefined ? value : validValue(parameter.value); | ||
const [localValue, setLocalValue] = useState(initialValue); | ||
useEffect(() => { | ||
setLocalValue(value); | ||
if (value !== undefined) { | ||
setLocalValue(value); | ||
} | ||
}, [value]); |
ParkreinerMay 9, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
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 viauseEffect
, 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:
- Let's say Component A is defined in terms of Component B and Component C
- Component A starts rendering
- The state changes mid-render
- 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
- React sees that the state changed, and knows that the result it produced is invalid
- 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
- React redoes the render for Component A
- The state stays stable this time around
- 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
- The whole subtree finishes rendering, and then paints the whole updated output to the screen
There was a problem hiding this comment.
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 thevalue
prop:value ?? validValue(parameter.value)
There was a problem hiding this comment.
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 forlocalValue
, even though it's guaranteed to always be a string
site/src/modules/workspaces/DynamicParameter/DynamicParameter.tsx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
@@ -76,9 +85,14 @@ export const DynamicParameter: FC<DynamicParameterProps> = ({ | |||
interface ParameterLabelProps { | |||
parameter: PreviewParameter; | |||
isPreset?: boolean; | |||
autofill?: AutofillBuildParameter; |
There was a problem hiding this comment.
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 anautofill
boolean?
site/src/modules/workspaces/DynamicParameter/DynamicParameter.tsx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
site/src/modules/workspaces/DynamicParameter/DynamicParameter.tsx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
site/src/modules/workspaces/DynamicParameter/DynamicParameter.tsx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
site/src/modules/workspaces/DynamicParameter/DynamicParameter.tsx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
site/src/pages/CreateWorkspacePage/CreateWorkspacePageViewExperimental.tsx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
site/src/pages/CreateWorkspacePage/CreateWorkspacePageViewExperimental.tsx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
site/src/pages/CreateWorkspacePage/CreateWorkspacePageViewExperimental.tsx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Parkreiner left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We're getting there, but there are still definitely bugs or things waiting to be bugs in this PR
I'm approving for now, just because I don't have the bandwidth to do another review cycle. I tried to make suggestions, but I haven't actually tried any of them
Uh oh!
There was an error while loading.Please reload this page.
value?: string; | ||
onChange: (value: string) => Promise<void>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do we want to make only one of these required? I would think it'd be better to have both be optional, so that users can use the component as controlled/uncontrolled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Especially since some of the variants don't receive anonChange
at all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Also, I'm just now realizing: I think we can remove thePromise
part of the return type. I don't see any awaits in this file, and that reduces the function coloring problem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I just checked again and it seems like every form control variant does use onChange
@@ -137,9 +169,26 @@ const ParameterLabel: FC<ParameterLabelProps> = ({ parameter, isPreset }) => { | |||
</Tooltip> | |||
</TooltipProvider> | |||
)} | |||
{autofill && ( | |||
<TooltipProvider delayDuration={100}> |
There was a problem hiding this comment.
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 globalTooltipProvider
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
There was a problem hiding this comment.
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.
const prevDebouncedValueRef = useRef<string | undefined>(); | ||
useEffect(() => { | ||
setLocalValue(value); | ||
}, [value]); | ||
if (prevDebouncedValueRef.current !== undefined) { | ||
onChangeEvent(debouncedLocalValue); | ||
} | ||
prevDebouncedValueRef.current = debouncedLocalValue; | ||
}, [debouncedLocalValue, onChangeEvent]); |
ParkreinerMay 15, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This actually won't work as intended.localValue
is guaranteed to be of type string on initialization, butprevDebouncedValueRef
is being initialized withundefined
, so the comparison willalways evaluate tofalse
on the initial render, and theonChangeEvent
branch will never get hit
One easy fix is this, which also removes the need for the type parameter:
constprevDebouncedValueRef=useRef(localValue);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
My goal for this was to preventonChangeEvent
getting triggered until user makes a change in the form field. Did you have a different idea for what the intention is?
onInput={(e) => { | ||
const target = e.currentTarget; | ||
target.style.maxHeight = "700px"; | ||
target.style.height = `${target.scrollHeight}px`; | ||
}} |
ParkreinerMay 15, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
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 toonChange
(in fact, React made the historical mistake of breaking from the HTML spec, and patchingonChange
to be more likeonInput
). 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 theonChange
handler.
But if the max height is being statically set to700px
, maybe we could make it so that this change triggers in response to the firstonFocus
event?
There was a problem hiding this comment.
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.
site/src/pages/CreateWorkspacePage/CreateWorkspacePageExperimental.tsx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
if (ws.current && ws.current.readyState === WebSocket.OPEN) { | ||
ws.current.send(JSON.stringify(request)); | ||
return prevId + 1; | ||
} | ||
returnresponse; | ||
returnprevId; |
ParkreinerMay 15, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
State setter callbacks have to be 100% pure. I want to say React Strict Mode will also double-call a state dispatch update to make sure of that. As in, it does the first dispatch, throws away the result, and then only flushes the second one to state
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
But also, I'm seeing that we're never using the ID in the render logic, so assuming we don't need it for any future work, we can safely replace it with a useRef call
setCurrentResponse((prev) => { | ||
if (prev?.id === response.id) { | ||
return prev; | ||
} | ||
if (!initialParamsSentRef.current && response.parameters.length > 0) { | ||
sendInitialParameters([...response.parameters]); | ||
} | ||
return response; | ||
}); | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is another case where we want to remove the side effect from the state update. I'm actuallymuch more concerned about this actually turning into a bug
I want to say useEffectEvent should help here. As in, since the function isn't ever being passed down as props, it should be safe to turn this into:
constonMessage=useEffectEvent((response:DynamicParametersResponse)=>{if(currentResponse?.id===response.id){return;}if(!initialParamsSentRef.current&&response.parameters.length>0){sendInitialParameters([...response.parameters]);}setCurrentReswponse(response);});
79c36c8
to7dc96bb
Compared6cb9b4
intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
resolvescoder/preview#80
Parameter autofill allows setting parameters from the url using the format param.[param name]=["purple","green"]
Example:
http://localhost:8080/templates/coder/scratch/workspace?param.list=%5b%22purple%22%2c%22green%22%5d%0a
The goal is to maintain feature parity of for autofill with dynamic parameters.
Note: user history autofill is no longer being used and is being removed.