- Notifications
You must be signed in to change notification settings - Fork1k
feat: select template version for tasks#20146
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
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 see a lot of types coming from the backend here with PascalCase. this isn't the first time, and I don't want it to become a habit. I think we need to nip these here and now.
| useEffect(()=>{ | ||
| if(presetPrompt){ | ||
| setPrompt(presetPrompt); | ||
| } | ||
| },[presetPrompt]); |
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 derived state. no need for auseEffect here.
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 sure I follow. Theprompt value is the controlled state for the textarea, and we want to sync it with the preset prompt (when one exists) as needed. 🤔
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.
value={prompt ?? presetPrompt} or something like that
Effects are for synchronizing with the outside world. this value is already in React, so there's no more need for an effect.
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 still not quite following. When there’s a preset prompt, I want theprompt value to be set to it; otherwise I’d have to usepresetPrompt || prompt and re-check it again on submit, which doesn’t feel cleaner. Also, when new presets load, I need to sync other state (e.g., set a new default for the selected preset). 🤔
If you’re available, I’d appreciate pairing on this so I can learn the approach.
| useEffect(()=>{ | ||
| setSelectedPresetId(defaultPreset?.ID); | ||
| },[defaultPreset?.ID]); | ||
| // Extract AI prompt from selected preset | ||
| constdefaultPreset=presets?.find((p)=>p.Default); | ||
| setSelectedPresetId(defaultPreset?.ID??presets?.[0]?.ID); | ||
| },[presets]); |
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 also derived state.selectedPresetId can already be undefined, just sayconst presetId = selectedPresetId ?? defaultPresetId somewhere.
and as a related semantics nit: there's nothing "selected" about a default. defaults are the absence of a more meaningful selected 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.
I’m also not sure I follow here. We haveselectedPresetId, which is the state used by the presets select. When the presets change—because the user selected a different template or version—it should be reset. I’m not sure how we could achieve that with derived state only. Could you show me an example?
@asilac I left a few comments to make sure I fully understand how your observations could be turned into derived state. Regarding the BE attributes: I’ve discussed this with the team before, and it would introduce a breaking change (request body and potentially the response). If we want to move forward, I suggest we first start a broader discussion in the dev channel, and then open a separate PR dedicated to those API changes. What do you think? |
840afb2 intomainUh oh!
There was an error while loading.Please reload this page.
Allows users with
updateTemplatespermission to select a specific template version when creating a task.One of the biggest changes was moving
TaskPromptintomodules/taskand adding a Storybook entry for it. I also moved some stories fromTasksPageto simplify its story.Closes#19986