-
Notifications
You must be signed in to change notification settings - Fork 77
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[editor] ErrorBoundary Renderer for PromptInput #799
Conversation
}; | ||
|
||
function InputErrorFallback({ input, toggleJSONEditor }: ErrorFallbackProps) { | ||
const { resetBoundary: clearRenderError } = useErrorBoundary(); |
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.
nit: delete resetBoundary
since we don't use it
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 do use it -- this just renames it to clearRenderError
since I found resetBoundary
to be a bit unclear
fallbackRender={() => ( | ||
<InputErrorFallback | ||
input={input} | ||
toggleJSONEditor={() => setIsRawJSON(true)} |
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 confusing because it only gets called during the setIsRawJSON
prop in lines 35-27, but we're immediately setting this back to true?
Can we rename this to make ti more intuitive, or leave a comment in code explaining what's happening
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.
It's implicitly assuming that the current state of isRawJson
is false due to check on L61. Let's either
- Add a comment why we're setting this to
true
so it's obvious - Pass in
!isRawJSON
instead (though I guess this might become weird if code gets rearranged and we're already somehow in raw json format and asking user to switch to raw json). Maybe we should do anassert
statement here between L71 and 72 to ensure thatisRawJson is false
before proceeding?
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'll leave a comment in the code.
This is confusing because it only gets called during the setIsRawJSON prop in lines 35-27, but we're immediately setting this back to true?
It's only ever rendered if the toggle is not showing raw JSON already (and there's a render error), so we explicitly want the fallback toggle to only be able to toggle to showing the raw JSON editor.
Maybe we should do an assert statement here
Generally we shouldn't do asserts in react code since throwing an error will result in the entire react component structure being unmounted by default, unless there's an ErrorBoundary higher up to catch it. Regardless, in this case isRawJson needs to be false because of the ternary
<> | ||
{schema ? ( | ||
<PromptInputSchemaRenderer | ||
input={input} | ||
schema={schema} | ||
onChangeInput={onChangeInput} | ||
/> | ||
) : ( | ||
<IconBraces size="1rem" /> | ||
<PromptInputConfigRenderer | ||
input={input} | ||
onChangeInput={onChangeInput} | ||
/> | ||
)} | ||
</ActionIcon> | ||
</Tooltip> | ||
</Flex> | ||
{rawJSONToggleButton} | ||
</> |
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.
Nit: lots of nested ternary expressions make it a bit hard to follow. Could we move this into it's own helper component like <PromptInputRendererForNonJson>
or something?
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.
Cool, learned about react-error-boundary
today: https://github.com/bvaughn/react-error-boundary
# [editor] ErrorBoundary Renderer for PromptInput With the ability to set arbitrary JSON for the prompt input, if we have a PromptSchema which states what the input should look like, we should be able to do some basic validation that the input at least matches the general type (for now, compare 'string' PromptInput vs object -- in the future, we could do some basic validation against the schema). If there is any error with rendering the schema-based rendering for the input, we can use ErrorBoundary to fallback to an error message informing the user that the format is invalid and to toggle to JSON editor to fix. <img width="1083" alt="Screenshot 2024-01-05 at 11 01 07 PM" src="https://github.com/lastmile-ai/aiconfig/assets/5060851/d0a817f5-b85b-4501-9ca5-28b324061ed4"> Toggling to the JSON editor will clear the error state and try to render the correct Schema renderer when toggling back. Note that in dev mode, these errors are still propagated to a top-level error screen which you can dismiss to show the fallback. In prod, that won't happen and will just show the fallback and an error in the console. We will do the same for the SettingsRenderer in a subsequent PR ## Testing: - Updated `OpenAIChatModelParserPromptSchema` input to match that of `OpenAIChatVisionModelParserPromptSchema` while the input is still string. See the error fallback - Toggle to JSON editor and back, ensure error fallback still shows - Toggle to JSON editor and update to valid input ``` { "attachments": [ { "data": "https://s3.amazonaws.com/files.uploads.lastmileai.com/uploads/cldxsqbel0000qs8owp8mkd0z/2023_12_1_21_23_24/942/Screenshot 2023-11-28 at 11.11.25 AM.png", "mime_type": "image/png" }, { "data": "https://s3.amazonaws.com/files.uploads.lastmileai.com/uploads/cldxsqbel0000qs8owp8mkd0z/2023_12_1_21_23_24/8325/Screenshot 2023-11-28 at 1.51.52 PM.png", "mime_type": "image/png" } ], "data": "What do these images show?" } ``` and see proper schema rendering https://github.com/lastmile-ai/aiconfig/assets/5060851/1343a201-e5eb-46a0-a7f5-a4bdb228d120
Changes from review: added comment, pulled non-JSON renderer to a const for clarity |
[editor] Add Output Error Rendering # [editor] Add Output Error Rendering Adding some basic rendering for Error output types if they're ever added to any configs. Also, propagate server run errors through the client as (client-only) Error outputs to display for the relevant prompt: ## Testing: - Raise an exception in the api/run method on the server and ensure it propagates to the output <img width="992" alt="Screenshot 2024-01-06 at 4 33 55 PM" src="https://github.com/lastmile-ai/aiconfig/assets/5060851/93309f64-4ce5-47dd-b553-c6fb44daaca0"> --- Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/lastmile-ai/aiconfig/pull/803). * __->__ #803 * #802 * #801 * #800 * #799
[editor] ErrorBoundary Renderer for PromptInput
[editor] ErrorBoundary Renderer for PromptInput
With the ability to set arbitrary JSON for the prompt input, if we have a PromptSchema which states what the input should look like, we should be able to do some basic validation that the input at least matches the general type (for now, compare 'string' PromptInput vs object -- in the future, we could do some basic validation against the schema).
If there is any error with rendering the schema-based rendering for the input, we can use ErrorBoundary to fallback to an error message informing the user that the format is invalid and to toggle to JSON editor to fix.
Toggling to the JSON editor will clear the error state and try to render the correct Schema renderer when toggling back.
Note that in dev mode, these errors are still propagated to a top-level error screen which you can dismiss to show the fallback. In prod, that won't happen and will just show the fallback and an error in the console.
We will do the same for the SettingsRenderer in a subsequent PR
Testing:
OpenAIChatModelParserPromptSchema
input to match that ofOpenAIChatVisionModelParserPromptSchema
while the input is still string. See the error fallbackand see proper schema rendering
Screen.Recording.2024-01-05.at.10.56.54.PM.mov
Stack created with Sapling. Best reviewed with ReviewStack.