Skip to content
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

Jump to bottom

[editor] ErrorBoundary Renderer for PromptInput #799

Merged
merged 1 commit into from
Jan 8, 2024
Merged

[editor] ErrorBoundary Renderer for PromptInput #799

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions python/src/aiconfig/editor/client/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
"oboe": "^2.1.5",
"react": "^18",
"react-dom": "^18",
"react-error-boundary": "^4.0.12",
"react-markdown": "^8.0.6",
"react-scripts": "5.0.1",
"remark-gfm": "^4.0.0",
Expand Down
20 changes: 20 additions & 0 deletions python/src/aiconfig/editor/client/src/components/JSONEditorToggleButton.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import { ActionIcon, Tooltip } from "@mantine/core";
import { IconBraces, IconBracesOff } from "@tabler/icons-react";

type Props = {
isRawJSON: boolean;
setIsRawJSON: (value: boolean) => void;
};

export default function JSONEditorToggleButton({
isRawJSON,
setIsRawJSON,
}: Props) {
return (
<Tooltip label="Toggle JSON editor" withArrow>
<ActionIcon onClick={() => setIsRawJSON(!isRawJSON)}>
{isRawJSON ? <IconBracesOff size="1rem" /> : <IconBraces size="1rem" />}
</ActionIcon>
</Tooltip>
);
}
87 changes: 70 additions & 17 deletions python/src/aiconfig/editor/client/src/components/prompt/prompt_input/PromptInputRenderer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,27 +3,63 @@ import { memo, useState } from "react";
import { PromptInputSchema } from "../../../utils/promptUtils";
import PromptInputSchemaRenderer from "./schema_renderer/PromptInputSchemaRenderer";
import PromptInputConfigRenderer from "./PromptInputConfigRenderer";
import { ActionIcon, Flex, Tooltip } from "@mantine/core";
import { IconBraces, IconBracesOff } from "@tabler/icons-react";
import { Flex } from "@mantine/core";
import PromptInputJSONRenderer from "./PromptInputJSONRenderer";
import { ErrorBoundary, useErrorBoundary } from "react-error-boundary";
import { Text } from "@mantine/core";
import JSONRenderer from "../../JSONRenderer";
import JSONEditorToggleButton from "../../JSONEditorToggleButton";

type Props = {
input: PromptInput;
schema?: PromptInputSchema;
onChangeInput: (value: PromptInput) => void;
};

type ErrorFallbackProps = {
input: PromptInput;
toggleJSONEditor: () => void;
};

function InputErrorFallback({ input, toggleJSONEditor }: ErrorFallbackProps) {
const { resetBoundary: clearRenderError } = useErrorBoundary();
Copy link
Contributor

@rossdanlm rossdanlm Jan 7, 2024 •
edited
Loading

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

Copy link
Contributor Author

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

return (
<Flex direction="column">
<Text color="red" size="sm">
Invalid input format for model. Toggle JSON editor to update
</Text>
<JSONRenderer content={input} />
<Flex justify="flex-end">
<JSONEditorToggleButton
isRawJSON={false}
setIsRawJSON={() => {
clearRenderError();
toggleJSONEditor();
}}
/>
</Flex>
</Flex>
);
}

export default memo(function PromptInputRenderer({
input,
schema,
onChangeInput,
}: Props) {
const [isRawJSON, setIsRawJSON] = useState(false);
return (
const rawJSONToggleButton = (
<Flex justify="flex-end">
<JSONEditorToggleButton
isRawJSON={isRawJSON}
setIsRawJSON={setIsRawJSON}
/>
</Flex>
);

const nonJSONRenderer = (
<>
{isRawJSON ? (
<PromptInputJSONRenderer input={input} onChangeInput={onChangeInput} />
) : schema ? (
{schema ? (
<PromptInputSchemaRenderer
input={input}
schema={schema}
Expand All @@ -35,17 +71,34 @@ export default memo(function PromptInputRenderer({
onChangeInput={onChangeInput}
/>
)}
<Flex justify="flex-end">
<Tooltip label="Toggle JSON editor" withArrow>
<ActionIcon onClick={() => setIsRawJSON((curr) => !curr)}>
{isRawJSON ? (
<IconBracesOff size="1rem" />
) : (
<IconBraces size="1rem" />
)}
</ActionIcon>
</Tooltip>
</Flex>
{rawJSONToggleButton}
</>
);

return (
<>
{isRawJSON ? (
<>
<PromptInputJSONRenderer
input={input}
onChangeInput={onChangeInput}
/>
{rawJSONToggleButton}
</>
) : (
<ErrorBoundary
fallbackRender={() => (
<InputErrorFallback
input={input}
// Fallback is only shown when an error occurs in non-JSON renderer
// so toggle must be to JSON editor
toggleJSONEditor={() => setIsRawJSON(true)}
Copy link
Contributor

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

Copy link
Contributor

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

  1. Add a comment why we're setting this to true so it's obvious
  2. 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 an assert statement here between L71 and 72 to ensure that isRawJson is false before proceeding?

Copy link
Contributor Author

@rholinshead rholinshead Jan 8, 2024 •
edited
Loading

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

/>
)}
>
{nonJSONRenderer}
</ErrorBoundary>
)}
</>
);
});
3 changes: 1 addition & 2 deletions ...r/client/src/components/prompt/prompt_input/schema_renderer/PromptInputSchemaRenderer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,7 @@ function SchemaRenderer({ input, schema, onChangeInput }: SchemaRendererProps) {
} = schema.properties;

if (typeof input === "string") {
return null;
// TODO: Add ErrorBoundary handling and throw error here
throw new Error("Expected input type object but got string");
}

const { data, attachments, ..._restData } = input;
Expand Down
7 changes: 7 additions & 0 deletions python/src/aiconfig/editor/client/yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9594,6 +9594,13 @@ react-dropzone@14.2.3:
file-selector "^0.6.0"
prop-types "^15.8.1"

react-error-boundary@^4.0.12:
version "4.0.12"
resolved "https://registry.yarnpkg.com/react-error-boundary/-/react-error-boundary-4.0.12.tgz#59f8f1dbc53bbbb34fc384c8db7cf4082cb63e2c"
integrity sha512-kJdxdEYlb7CPC1A0SeUY38cHpjuu6UkvzKiAmqmOFL21VRfMhOcWxTCBgLVCO0VEMh9JhFNcVaXlV4/BTpiwOA==
dependencies:
"@babel/runtime" "^7.12.5"

react-error-overlay@^6.0.11:
version "6.0.11"
resolved "https://registry.yarnpkg.com/react-error-overlay/-/react-error-overlay-6.0.11.tgz#92835de5841c5cf08ba00ddd2d677b6d17ff9adb"
Expand Down