[Meru][Assistant Builder] Improved description suggestion #4559
Conversation
|
I think I would remove the confirmation when clicking on sparkles with a description already there. No big deal if we override that description no? |
| const descriptionSuggestions = await getDescriptionSuggestions({ | ||
| owner, | ||
| instructions: builderState.instructions || "", | ||
| name: builderState.handle || "", | ||
| }); | ||
| if (descriptionSuggestions.isOk()) { | ||
| setDescriptionSuggestions(descriptionSuggestions.value); | ||
| const suggestion = | ||
| descriptionSuggestions.value.status === "ok" && |
There was a problem hiding this comment.
This return type fields weird, the http code should be enough to say if the suggestion calls worked.
There was a problem hiding this comment.
Good point. This is debatable, since there are cases where user input is correct, the API call works, the dust app is run, and the LLM decides it's not time to create suggestions (e.g. because of the content of the instructions). So it's not exactly an error, the API call worked and is saying "ok, I looked, with this data it's not appropriate to give you suggestions" (but the request body was correct, with valid arguments, and no way to tell beforehand)
WDYT?
There was a problem hiding this comment.
From a FE standpoint, we don't need to know all this conditional logic around the LLM deciding whether or not it's time to generate a description.
I'd suggest to go with an empty array in those scenarios.
There was a problem hiding this comment.
Well, depending on the reason for the empty array, the front end will need to react differently. Agree that's debatable, boils down to deciding what are valid answers from the endpoint and what are "errors" and I agree the duplicate status is indeed weird. But changing API scope is not for this PR. We can discuss it further for another PR if you beleive it's warranted
| }); | ||
| } | ||
| } else { | ||
| sendNotification({ |
There was a problem hiding this comment.
Should we display notification in all cases? Only on user intent?
| builderState.instructions?.trim() && | ||
| !generatingDescription | ||
| ) { | ||
| suggestDescription().catch((error) => { |
There was a problem hiding this comment.
Shouldn't we have the catch directly in suggestDescription to centralize the error handling?
There was a problem hiding this comment.
IMHO the catch is not necessary (so I would not wrap the whole suggestDescription in a try / catch) -- but IIRC we decided we should not void promises? glad to void it if I remembered wrong
There was a problem hiding this comment.
Seen IRL, we should wrap the fetch call with a catch and return Err if needed / or throw for unexpected errors. Down the line we can only check isErr.
There was a problem hiding this comment.
👍 as per irl we will void in the useEffect and await in the onclick
| <Page.SectionHeader title="Description" /> | ||
| <div className="flex gap-1"> | ||
| <Page.SectionHeader title="Description" /> | ||
| {generatingDescription && <Spinner size="sm" />} |
Not sure about that; my assumption is that some users won't pay attention and click (curiosity, misclick, etc.). If they had taken the time to write a description, they'll be frustrated. Note that the confirmation is asked only if it's user-written, there is no confirmation to replace generated description |
| import { isDevelopmentOrDustWorkspace } from "@app/lib/development"; | ||
| import { debounce } from "@app/lib/utils/debounce"; | ||
| import { as } from "fp-ts/lib/Option"; |
| }); | ||
| } | ||
| return new Ok(await res.json()); | ||
| return await fetchWithErr( |
There was a problem hiding this comment.
| return await fetchWithErr( | |
| return fetchWithErr( |
* update description suggestion app * clean dust app + don't use cache * improve description suggestions behaviour * review * clean
Description
Note: the heads up is a simple
confirm, an immediately following PR will make a lib for alert/confirm with our standard modals and use themRisk
none, gated
Deploy Plan
deploy front