feat(content-answers): add inline error #3444

TaoJiang98 · 2023-10-13T21:40:02Z

greg-in-a-box

left some comments

greg-in-a-box · 2023-10-16T17:10:30Z

src/features/content-answers/Answer.tsx

            {answer && <ContentAnswersGridCard>{answer}</ContentAnswersGridCard>}
            {!answer && isLoading && <LoadingElement />}


We need to prevent these were rendering if there is an error, definitely the first line but the second line is debatable depending on how isLoading is handle but I think its safe to say both should not render if there is an error

greg-in-a-box · 2023-10-16T17:11:35Z

src/features/content-answers/ContentAnswersModal.tsx

    prompt: string;
    answer?: string;
    createdAt?: string;
+    error?: string;


I didnt notice this before but abc

greg-in-a-box · 2023-10-16T17:14:17Z

src/features/content-answers/ContentAnswersModal.tsx

        }
+
+        setQuestions([...updatedQuestions, lastQuestion]);


should we still update state if there isnt a lastQuestion?

It should be guaranteed that there is always a lastQuestion in the question array. The reason is that whenever we submit a question, the new question object with the prompt only will be appended into the question array.

greg-in-a-box · 2023-10-16T17:41:43Z

src/features/content-answers/ContentAnswersModal.tsx

@@ -53,11 +54,15 @@ const ContentAnswersModal = ({ api, currentUser, file, isOpen, onRequestClose }:
        setQuestions([...updatedQuestions, lastQuestion]);
    }, []);

-    const handleErrorCallback = useCallback((error: ElementsXhrError): void => {
+    const handleErrorCallback = useCallback((error: ElementsXhrError, updatedQuestions): void => {


since we're already adding the error type, can we add the updatedQuestions type?

src/features/content-answers/__tests__/InlineError.test.tsx

greg-in-a-box · 2023-10-16T17:54:47Z

src/features/content-answers/InlineError.tsx

+    error: ElementsXhrError;
+};
+
+const InlineError = ({ error }: Props) => {


do we have to pass in the error? you're already controlling when to render component in the parent component

removed error prop, but in the future we could possibly pass error and make the inlineError component show different error messages based on the error prop

greg-in-a-box · 2023-10-16T17:56:56Z

src/features/content-answers/__tests__/Answer.test.tsx


        const loadingElement = screen.getByTestId('LoadingElement');
        expect(loadingElement).toBeInTheDocument();
    });
+
+    test('should render inline error', () => {


I think we should check if the other two have rendered or not

greg-in-a-box · 2023-10-16T17:57:30Z

src/features/content-answers/__tests__/Answer.test.tsx


        const loadingElement = screen.getByTestId('LoadingElement');
        expect(loadingElement).toBeInTheDocument();
    });
+
+    test('should render inline error', () => {
+        renderComponent({ answer: '', error: new Error('error'), isLoading: true });


I think you need to do something like a test.each to check all the possibilities and then verify the expected behavior in each of them

since each check has special case such as checking only renders LoadingElement would have to check the length of testId 'content-answers-grid-card' to be exact 1 so that I separate those three test cases

greg-in-a-box · 2023-10-17T19:59:14Z

src/features/content-answers/ContentAnswersModalContent.tsx

@@ -24,11 +24,11 @@ const ContentAnswersModalContent = ({ currentUser, fileName, isLoading, question
            <WelcomeMessage fileName={fileName} />
            <ul>
                {questions &&
-                    questions.map(({ prompt, answer = '' }, index) => {
+                    questions.map(({ prompt, answer = '', error }, index) => {


defaulted props always come at the end of the args

greg-in-a-box · 2023-10-17T21:32:59Z

src/features/content-answers/Answer.tsx

    isLoading: boolean;
 };

-const Answer = ({ answer, isLoading }: Props) => {
+const Answer = ({ answer, error, isLoading }: Props) => {
    return (
        <div className="bdl-Answer">
            {answer && <ContentAnswersGridCard>{answer}</ContentAnswersGridCard>}


I dont think we should give the opportunity to provide an answer and an error at the same time where it renders both

do you suggest that only pass answer prop here and if the error is not undefined, we display InlineError directly in ContentAnswersModalContent component? so that Answer component won't have the answer and error prop appeared at the same time

src/features/content-answers/__mocks__/mocks.ts

greg-in-a-box · 2023-10-19T20:27:13Z

src/features/content-answers/__tests__/Answer.test.tsx

        renderComponent({ answer: '', isLoading: true });

        const loadingElement = screen.getByTestId('LoadingElement');
        expect(loadingElement).toBeInTheDocument();
+        expect(screen.queryAllByTestId('content-answers-grid-card')).toHaveLength(1);


I think this should assert if the answer is there not the card, its easier to understand in terms of readability. noone is going to know that the card is actually under the LoadingElement too unless they check in that component too

since the answer prop here is given as ''. I think it does not make sense to check if text '' not in the document. Instead, I checked length of children list under Answer component is 1 which should be loading element.

greg-in-a-box · 2023-10-19T20:28:35Z

src/features/content-answers/ContentAnswersModal.tsx

+        const lastQuestion = updatedQuestions.pop();
+        if (lastQuestion && lastQuestion.prompt) {
+            lastQuestion.error = error;
+            setQuestions([...updatedQuestions, lastQuestion]);


I think you need a test to handle this logic

ghost · 2023-10-23T20:52:45Z

src/features/content-answers/messages.ts

@@ -32,6 +32,11 @@ const messages = defineMessages({
            'Disabled tooltip message for Content Answers entry point button when the file type is not supported',
        id: 'boxui.contentAnswers.disabledTooltipFileNotCompatible',
    },
+    inlineErrorText: {
+        defaultMessage: 'The Box AI service was unavailable.',


Should it say, is instead of was?

When the error comes out, there would be a retry button for user to click. And I think it would make sense saying that Box AI service was unavailable and it is worth to retry.

tjuanitas · 2023-10-23T22:51:03Z

src/features/content-answers/InlineError.tsx

+                </div>
+            </Media.Figure>
+            <Media.Body>
+                <div className="bdl-InlineError-AlertIcon">


the descendant element should be camelcase:

Suggested change

<div className="bdl-InlineError-AlertIcon">

<div className="bdl-InlineError-alertIcon">

tjuanitas · 2023-10-23T22:53:49Z

src/features/content-answers/InlineError.scss

+.bdl-InlineError {
+    margin-top: $bdl-grid-unit * 4;
+
+    &-icon {


in the past we've tried avoiding using & for descendant elements and modifiers so it doesn't hinder searching

e.g. searching bdl-InlineError-icon in GitHub or in an IDE would not show results because it's written &-icon

tjuanitas · 2023-10-23T23:03:25Z

src/features/content-answers/__tests__/ContentAnswersModalContent.test.tsx

@@ -59,4 +60,11 @@ describe('features/content-answers/ContentAnswersModalContent', () => {
        const loadingElement = screen.queryAllByTestId('LoadingElement');
        expect(loadingElement.length).toEqual(1);
    });
+
+    test('should render only the inline error which not loading and there is an error', () => {


when instead of which?

tjuanitas · 2023-10-23T23:19:10Z

src/features/content-answers/__tests__/InlineError.test.tsx

+    const IntlWrapper = ({ children }: { children?: React.ReactNode }) => {
+        return <IntlProvider locale="en">{children}</IntlProvider>;
+    };


does this work:

jest.mock('react-intl', () => ({ ...jest.requireActual('react-intl'), FormattedMessage: ({ defaultMessage }: { defaultMessage: string }) => <span>{defaultMessage}</span>, }));

or does it still need the whole wrapper? when does the console error occur? why does a test like ContentAnalyticsErrorState.test.tsx not need it?

yeah I think this works. The console error occurs when I only mocks 'react-intl'. The
test ContentAnalyticsErrorState.test.tsx did not need it because it was testing the element instead of the text.

tjuanitas · 2023-10-23T23:30:31Z

src/features/content-answers/ContentAnswersModal.tsx

-        if (error) {
-            // TODO: render error component
+
+        const lastQuestion = updatedQuestions.pop();


we should probably avoid object mutation here since the reference to updatedQuestions is used in multiple places which could cause unexpected issues

maybe a slice() or length solution:

const [lastQuestion] = updatedQuestions.slice(-1); // or const lastQuestion = updatedQuestions[updatedQuestions.length - 1];

tjuanitas · 2023-10-23T23:34:40Z

src/features/content-answers/ContentAnswersModal.tsx

    }, []);

-    const handleErrorCallback = useCallback((error: ElementsXhrError): void => {
+    const handleErrorCallback = useCallback((error: ElementsXhrError, updatedQuestions: QuestionType[]): void => {
        setIsLoading(false);


why is setIsLoading called at the beginning of the function instead of the end of the logic or at the end of handleOnAsk?

tjuanitas · 2023-10-23T23:41:50Z

src/features/content-answers/ContentAnswersModalContent.tsx

                        return (
                            <li key={index}>
                                <Question currentUser={currentUser} prompt={prompt} />
-                                <Answer answer={answer} isLoading={isLoading} />
+                                {!hasError && <Answer answer={answer} isLoading={isLoading} />}
+                                {hasError && !isLoading && <InlineError />}


do we want to remove the isLoading check for InlineError? wouldn't this affect all "error answers"?

Stage 1

Q: summarize this document

Stage 2

Q: summarize this document
A: ERROR

Stage 3

Q: summarize this document
A: ERROR
Q: summarize this document in 200 words

Stage 4

Q: summarize this document
A: LOADING
Q: summarize this document in 200 words
A: LOADING

Stage 5

Q: summarize this document
A: ERROR
Q: summarize this document in 200 words
A: blah blah blah

{hasError ? <InlineError /> : <Answer answer={answer} isLoading={isLoading} />}

tjuanitas · 2023-10-24T19:20:38Z

src/features/content-answers/ContentAnswersModal.tsx

@@ -76,8 +75,9 @@ const ContentAnswersModal = ({ api, currentUser, file, isOpen, onRequestClose }:
                const response = await api.getIntelligenceAPI(true).ask(prompt, items);
                handleSuccessCallback(response, updatedQuestions);
            } catch (e) {
-                handleErrorCallback(e);
+                handleErrorCallback(e, updatedQuestions);


sorry it might be out of scope but I didn't realize this before: are these "callbacks" necessary?

why not something like:

const handleOnAsk = useCallback( ... let nextQuestion = { prompt }; setQuestions([...questions, nextQuestion]); setIsLoading(true); try { const response = await api.getIntelligenceAPI(true).ask(prompt, items); nextQuestion = { answer: response.data.answer, createdAt: response.data.created_at, prompt, }; } catch (error) { nextQuestion = { error, prompt, }; } setQuestions([...questions, nextQuestion]); setIsLoading(false);

that way you don't need handleSuccessCallback and handleErrorCallback and avoids the object mutation

This is a very great point! I think previously it is written as handleSuccessCallback and handleErrorCallback to better address which part need to work on for following small tasks.

When bring everything back to try catch, it still needs to update the last question either in place or pop up and then append it back since we need to append the question with prompt into the array at the very beginning.

removed the object mutation logic

greg-in-a-box · 2023-10-24T21:10:24Z

src/features/content-answers/__tests__/ContentAnswersModal.test.tsx


    test('should render the header icon', () => {
-        renderComponent();
+        renderComponent(mockApi);


cant mockApi be defaulted since you have to use it more than once?

TaoJiang98 requested a review from a team as a code owner October 13, 2023 21:40

TaoJiang98 marked this pull request as draft October 13, 2023 21:46

TaoJiang98 marked this pull request as ready for review October 16, 2023 15:28

greg-in-a-box reviewed Oct 16, 2023

View reviewed changes

greg-in-a-box requested changes Oct 17, 2023

View reviewed changes

greg-in-a-box requested changes Oct 19, 2023

View reviewed changes

TaoJiang98 requested a review from a team as a code owner October 23, 2023 19:42

ghost reviewed Oct 23, 2023

View reviewed changes

tjuanitas reviewed Oct 23, 2023

View reviewed changes

tjiang09 added 6 commits October 24, 2023 10:06

feat(content-answers): add inline error

11bbcfe

feat(content-answers): change error as optional prop

49a07b1

feat(content-answers): update prop and tests

d659f8b

feat(content-answers): remove error from answers component

4dc5283

feat(content-answers): fix tests

173f6d9

feat(content-answers): update test styles and comments

1528b62

TaoJiang98 force-pushed the inline-error branch from f0da546 to 1528b62 Compare October 24, 2023 17:08

tjuanitas reviewed Oct 24, 2023

View reviewed changes

tjiang09 added 2 commits October 24, 2023 13:30

feat(content-answers): removed object mutation logic

a24ea35

feat(content-answers): fix console.err

2e7ee8d

greg-in-a-box requested changes Oct 24, 2023

View reviewed changes

feat(content-answers): feedback

d5d0237

tjuanitas approved these changes Oct 24, 2023

View reviewed changes

greg-in-a-box approved these changes Oct 24, 2023

View reviewed changes

ghost approved these changes Oct 24, 2023

View reviewed changes

TaoJiang98 added the ready-to-merge label Oct 24, 2023

mergify bot merged commit 405cdb4 into master Oct 24, 2023
6 checks passed

mergify bot deleted the inline-error branch October 24, 2023 22:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(content-answers): add inline error #3444

feat(content-answers): add inline error #3444

TaoJiang98 commented Oct 13, 2023

greg-in-a-box left a comment

greg-in-a-box Oct 16, 2023

greg-in-a-box Oct 16, 2023

greg-in-a-box Oct 16, 2023

TaoJiang98 Oct 17, 2023

greg-in-a-box Oct 16, 2023

greg-in-a-box Oct 16, 2023

TaoJiang98 Oct 17, 2023

greg-in-a-box Oct 16, 2023

greg-in-a-box Oct 16, 2023

TaoJiang98 Oct 17, 2023

greg-in-a-box Oct 17, 2023

greg-in-a-box Oct 17, 2023

TaoJiang98 Oct 17, 2023 •

edited

greg-in-a-box Oct 19, 2023

TaoJiang98 Oct 20, 2023

greg-in-a-box Oct 19, 2023

ghost Oct 23, 2023

TaoJiang98 Oct 23, 2023

tjuanitas Oct 23, 2023

tjuanitas Oct 23, 2023

tjuanitas Oct 23, 2023

tjuanitas Oct 23, 2023

TaoJiang98 Oct 24, 2023

tjuanitas Oct 23, 2023

tjuanitas Oct 23, 2023

tjuanitas Oct 23, 2023

tjuanitas Oct 23, 2023

tjuanitas Oct 24, 2023

TaoJiang98 Oct 24, 2023

TaoJiang98 Oct 24, 2023

greg-in-a-box Oct 24, 2023

		{answer && <ContentAnswersGridCard>{answer}</ContentAnswersGridCard>}
		{!answer && isLoading && <LoadingElement />}

	<div className="bdl-InlineError-AlertIcon">
	<div className="bdl-InlineError-alertIcon">

feat(content-answers): add inline error #3444

feat(content-answers): add inline error #3444

Conversation

TaoJiang98 commented Oct 13, 2023

greg-in-a-box left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TaoJiang98 Oct 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TaoJiang98 Oct 17, 2023 •

edited