feat(content-answers): Upgrade Content Answers by greg-in-a-box · Pull Request #3658 · box/box-ui-elements

greg-in-a-box · 2024-09-17T00:13:35Z

Updated Content Answer with the latest features

tjiang-box · 2024-09-19T21:12:09Z

+    isCitationsEnabled?: boolean;
+    isMarkdownEnabled?: boolean;
+    isResetChatEnabled?: boolean;
+    onAsk?: () => void;


I think onAsk will always exist here since the upper level ContentAnswers.tsx will always pass down handleAsk() as onAsk, also same as onRequestClose

its separated like that because a customer can submit onAsk and onRequestClose from the root lvl via contentAnswersProps

tjiang-box · 2024-09-19T21:46:33Z

+            isCompleted: true,
+            isLoading: false,
+        };
+        setIsLoading(false);


we don't need this line since setIsLoading(false); will be called after the try catch in handleAsk() function

tjiang-box · 2024-09-19T21:50:16Z

+                created_at: q.created_at,
+            }));
+
+            const nextQuestions = [...(isRetry ? questions.slice(0, -1) : questions)];


should this be more like prevQuestions instead

…ox#3626) * fix(docgen-sidebar): add collapsible component to hide nested tags * fix(docgen-sidebar): nesting of tags in tag tree * fix(docgen-tags): update styles to use blueprint tokens * fix(docgen-tags): replace bdl tokens with blueprint tokens * fix(docgen-tags): convert test from enzyme to rtl * fix(docgen-tags): remove snapshots, reuse blueprint loader * fix(docgen-tags): address code review comments * fix(docgen-tags): remove the sidebar test file * fix(docgen-tags): file with updated name * Update src/elements/content-sidebar/__tests__/DocGenSidebar.test.tsx Co-authored-by: greg-in-a-box <103291617+greg-in-a-box@users.noreply.github.com> * Update src/elements/content-sidebar/__tests__/DocGenSidebar.test.tsx Co-authored-by: greg-in-a-box <103291617+greg-in-a-box@users.noreply.github.com> * fix(docgen-sidebar): address code review comments * fix(docgen-sidebar): address code review comments * fix(docgen-sidebar): remove unused mock --------- Co-authored-by: greg-in-a-box <103291617+greg-in-a-box@users.noreply.github.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

benjamin-shen · 2024-09-24T16:17:03Z

+     * @param dialogueHistory
     * @param {Array<object>} items - Array of items to ask about


suggestion: switch order to match actual params

Suggested change

* @param dialogueHistory

* @param {Array<object>} items - Array of items to ask about

* @param {Array<object>} items - Array of items to ask about

* @param dialogueHistory

benjamin-shen · 2024-09-24T16:18:36Z

+        question: QuestionType,
+        items: Array<BoxItem>,
+        dialogueHistory: Array<QuestionType> = [],
+        options: { include_citations: boolean } = {},


is this supposed to be

Suggested change

options: { include_citations: boolean } = {},

options: { include_citations?: boolean } = {},

? if it's defaulted to {}

benjamin-shen · 2024-09-24T16:41:14Z

+        const errorMessage = error ? error.message || '' : '';
+        const isRateLimitingError =
+            (error && error.response && error.response.status === 429) || rateLimitingRegex.test(errorMessage);


could use optional chaining here if you prefer. non-blocking, totally up to you

Suggested change

const errorMessage = error ? error.message || '' : '';

const isRateLimitingError =

(error && error.response && error.response.status === 429) || rateLimitingRegex.test(errorMessage);

const errorMessage = error?.message || '';

const isRateLimitingError =

(error?.response?.status === 429) || rateLimitingRegex.test(errorMessage);

benjamin-shen · 2024-09-24T16:42:45Z

+    const handleAsk = useCallback(
+        async (question: QuestionType, aiAgent: AgentType, isRetry = false) => {
+            !!onAsk && onAsk();
+            const id = file && file.id;


when can file be falsy? what happens here if file is falsy?

benjamin-shen · 2024-09-24T16:52:38Z

+            submitQuestion={handleAsk}
+            suggestedQuestions={suggestedQuestions || localizedQuestions}
+            warningNotice={spreadsheetNotice}
+            warningNoticeAriaLabel={formatMessage(messages.welcomeMessageSpreadsheetNoticeAriaLabel)}


should this also be conditional on isSpreadsheet?

warningNotice controls if the ariaLabel is used.

benjamin-shen · 2024-09-24T16:54:12Z

+            onModalClose={handleOnRequestClose}
+            open={isOpen}
+            onOpenChange={handleOnRequestClose}
+            onClearAction={handleClearConversation}


idk if you were going for alphabetizing here, but if so, these should be reordered

tjuanitas · 2024-09-24T18:11:25Z

+    );
+};
+
+export default withAPIContext(withCurrentUser(ContentAnswersModal));


withCurrentUser already has withAPIContext, is it needed here?

yea we need this here for the name and avatar url

i think we talked about this but can you remind me what's happening here? shouldn't the props forwarded from withAPIContext be passed through withCurrentUser to this component?

tjiang-box · 2024-09-25T21:35:41Z

+        expect(modal).toBeInTheDocument();
+
+        const textArea = screen.getByRole('textbox', { name: 'Ask anything about this doc' });
+        fireEvent.change(textArea, { target: { value: prompt } });


the prompt here seems not defined in this file.

And could we try using userEvent.type(textArea, 'question prompt'). Same for ones used in ContentAnswersModal.test.tsx file.
Example https://github.com/box/box-ui-elements/blob/master/src/elements/content-explorer/__tests__/RenameDialog.test.tsx#L59-L62

tjiang-box · 2024-09-25T21:51:27Z


 type Props = {
    file: BoxItem;
 };


should the prop types above changed to interface like below

export interface contentAnswersProps extends ContentAnswersModalExternalProps { show?: boolean; file: BoxItem; }

benjamin-shen · 2024-09-25T21:56:14Z

+            onOpenChange={handleOnRequestClose}
            onModalClose={handleOnRequestClose}


m comes before o in the alphabet!

tjuanitas · 2024-09-26T19:11:37Z

+                    <Notification.Provider>
+                        <Notification.Viewport />
+                        <TooltipProvider>


internal apps will likely have these providers already. any concerns with nested providers?

tjuanitas · 2024-09-30T18:34:07Z


    const currentExtension = getProp(file, 'extension');
    return (
        <div className="bdl-ContentAnswers">


this class name is no longer used i think. or should be changed to be-ContentAnswers

tjuanitas · 2024-09-30T19:19:50Z

+| sharedLink           | string |  | *See the [developer docs](https://developer.box.com/docs/box-content-preview#section-options).*                                                                                     |
+| sharedLinkPassword   | string |  | *See the [developer docs](https://developer.box.com/docs/box-content-preview#section-options).*                                                                                     |
+| showAnnotations      | boolean | `true` | *See the [developer docs](https://developer.box.com/docs/box-content-preview#section-options).*                                                                                     |
+| shouldProvide        | boolean | `true` | Decide if it should wrap the element with the `Notification` and `Tooltip` Provider. Set this to false if you already wrap your app with both of these providers.                       |


if this is a boolean then maybe hasProviders?

alternate approach:
or what about an array/object that specifies providers? that way it's more configurable depending on the providers that the consumer already has

for example:

// in consuming app const providers = ['date-picker', 'notification', 'tooltip']; // Blueprint DatePicker has a provider we'll eventually need to include <ContentPreview providers={providers} ... /> // in new providers component const ElementProviders = ({ children, providers = ['notification', 'tooltip'] }) => { let element = children; providers.forEach(provider => { switch (provider) { case 'date-picker': ... break; case 'notification': element = <Notification.Provider>...</Notification.Provider>; break; case 'tooltip': element = <TooltipProvider>{element}</TooltipProvider>; break; } }); return element; };

tjuanitas · 2024-09-30T19:36:44Z

+    const [questions, setQuestions] = useState<QuestionType[]>([]);
+    let localizedQuestions: SuggestedQuestionType[] = [];
+
+    if (!suggestedQuestions) {


fyi an empty array for suggestedQuestions is truthy so this only enters the if statement for undefined. is that okay?

my guess is that it could render empty space but that seems out of scope for this component and more of something that should be handled by the shared-feature

tjuanitas · 2024-09-30T19:38:17Z

+    let localizedQuestions: SuggestedQuestionType[] = [];
+
+    if (!suggestedQuestions) {
+        localizedQuestions = DOCUMENT_SUGGESTED_QUESTIONS.map(question => ({


is this something the webapp is doing for all file extensions?

tjuanitas · 2024-09-30T19:48:59Z

i feel like uses of useCallback in this file are unnecessary:
https://react.dev/reference/react/useCallback#should-you-add-usecallback-everywhere

tjuanitas · 2024-09-30T19:59:58Z

+    );
+};
+
+export default withAPIContext(withCurrentUser(ContentAnswersModal));


i think we talked about this but can you remind me what's happening here? shouldn't the props forwarded from withAPIContext be passed through withCurrentUser to this component?

tjuanitas · 2024-10-01T00:11:19Z

+    // Note, you can still specify the more
+    // specific options listed in the section above


what does "section above" refer to?

it looks like this file/object is used once, what about adding viewport property directly to preview.tsx?

tjuanitas · 2024-10-01T00:18:35Z

nit: Provide sounds a little odd to me as a component because of the verb. Providers sounds a little clearer

related to conversation about granular providers, we might need to consider how customers can enable/disable providers like Theme, Router (react-router upgrade)

yes for theme, but router no since that is more universal, unless the backwards compat guide says we need one

tjuanitas · 2024-10-01T00:22:26Z

+import { render } from '../../../test-utils/testing-library';
+import Provide from '../Provide';
+
+describe('Provide component', () => {


should be path to component

tjuanitas · 2024-10-01T00:27:11Z

+| sharedLink           | string |  | *See the [developer docs](https://developer.box.com/docs/box-content-preview#section-options).*                                                                                     |
+| sharedLinkPassword   | string |  | *See the [developer docs](https://developer.box.com/docs/box-content-preview#section-options).*                                                                                     |
+| showAnnotations      | boolean | `true` | *See the [developer docs](https://developer.box.com/docs/box-content-preview#section-options).*                                                                                     |
+| hasProviders         | boolean | `true` | Decide if it should wrap the element with the `Notification` and `Tooltip` Provider. Set this to false if you already wrap your app with both of these providers.                       |


tjuanitas · 2024-10-01T00:27:51Z

    responseInterceptor?: Function,
    sharedLink?: string,
    sharedLinkPassword?: string,
+    hasProviders?: boolean,


tjuanitas · 2024-10-01T00:42:20Z

+        expect(screen.getByRole('region', { name: 'Notifications (F8)' })).toBeInTheDocument();
+    });
+
+    test('renders only children when shouldProvide is false', () => {


Suggested change

test('renders only children when shouldProvide is false', () => {

test('renders only children when hasProviders is false', () => {

tjuanitas · 2024-10-01T00:42:31Z

+        expect(screen.queryByRole('region', { name: 'Notifications (F8)' })).not.toBeInTheDocument();
+    });
+
+    test('throws an error if more than one child is provided when shouldProvide is false', () => {


Suggested change

test('throws an error if more than one child is provided when shouldProvide is false', () => {

test('throws an error if more than one child is provided when hasProviders is false', () => {

tjuanitas · 2024-10-01T00:42:44Z

+        ).toThrow();
+    });
+
+    test('renders children within Notification and TooltipProvider by default when shouldProvide is not provided', () => {


Suggested change

test('renders children within Notification and TooltipProvider by default when shouldProvide is not provided', () => {

test('renders children within Notification and TooltipProvider by default when hasProviders is not provided', () => {

tjuanitas · 2024-10-01T00:45:48Z

+        renderComponent({ fileExtension: 'invalid', onClick });
+        expect(onClick).toBeCalledTimes(0);
+        const button = screen.getByRole('button');
+        await userEvent.click(button, { pointerEventsCheck: 0 });


what's pointerEventsCheck for? is this optimization?

it throws an error like this Error: Unable to perform pointer interaction as the element has pointer-events: none if that flag isnt there, the button from blueprint set pointer-events to none

tjuanitas · 2024-10-01T00:46:21Z

+    test('should display not allowed tooltip', async () => {
+        renderComponent({ fileExtension: 'invalid' });
+        const button = screen.getByRole('button', { name: 'Box AI' });
+        await userEvent.hover(button, { pointerEventsCheck: 0 });


is hover using pointer events?

same as above

tjuanitas

lgtm

benjamin-shen · 2024-10-02T15:31:41Z

+     * @param {QuestionType} question - Object should at least contain the prompt, which is the question to ask
     * @param {Array<object>} items - Array of items to ask about
-     * @param dialogueHistory
-     * @param options
+     * @param {Array<QuestionType>} dialogueHistory - Array of previous questions object that already have answers


naming QuestionType with a -Type suffix instead of Question is like naming something FooInterface instead of Foo, right? is that an existing naming convention in this repo? if not, can we rename QuestionType to Question?

is it possible to make these types stronger, eg. define a QuestionWithAnswer type that is Question & { answer... }

that type is coming from the shared-feature, its the name they are using

benjamin-shen · 2024-10-02T15:56:32Z

        expect(answer).toBeInTheDocument();

-        expect(modal.getByText('Based on:')).toBeInTheDocument();
+        expect(modal.queryByText('Based on:')).not.toBeInTheDocument();


where is the text 'Based on:' actually rendered from? does it come from the api response or is it hardcoded in the repo?

part of the citation feature that appears in the answer if the citations are present via the shared-features

* fix(docgen-sidebar): add collapsible component to hide nested tags (box#3626) * fix(docgen-sidebar): add collapsible component to hide nested tags * fix(docgen-sidebar): nesting of tags in tag tree * fix(docgen-tags): update styles to use blueprint tokens * fix(docgen-tags): replace bdl tokens with blueprint tokens * fix(docgen-tags): convert test from enzyme to rtl * fix(docgen-tags): remove snapshots, reuse blueprint loader * fix(docgen-tags): address code review comments * fix(docgen-tags): remove the sidebar test file * fix(docgen-tags): file with updated name * Update src/elements/content-sidebar/__tests__/DocGenSidebar.test.tsx Co-authored-by: greg-in-a-box <103291617+greg-in-a-box@users.noreply.github.com> * Update src/elements/content-sidebar/__tests__/DocGenSidebar.test.tsx Co-authored-by: greg-in-a-box <103291617+greg-in-a-box@users.noreply.github.com> * fix(docgen-sidebar): address code review comments * fix(docgen-sidebar): address code review comments * fix(docgen-sidebar): remove unused mock --------- Co-authored-by: greg-in-a-box <103291617+greg-in-a-box@users.noreply.github.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> * chore(storybook): Added global types for storybook * feat(content-answers): Upgrade Content Answers * feat(content-answers): fix tests * feat(content-answers): Enabled some new features * feat(content-answers): Feedback * chore(content-answers): feedback * feat(content-answers): Feedback * feat(content-answers): Feedback * feat(content-answers): Feedback * feat(content-answers): vrt width * feat(content-answers): Feedback * feat(content-answers): Feedback * feat(content-answers): Providers * feat(content-answers): Providers * feat(content-answers): Feedback * feat(content-answers): Feedback * feat(content-answers): tests * feat(content-answers): Feedback * feat(content-answers): Feedback --------- Co-authored-by: rustam-e <6816931+rustam-e@users.noreply.github.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

greg-in-a-box changed the title ~~Qa modal swap~~ feat(content-answers): Upgrade Content Answers Sep 17, 2024

greg-in-a-box force-pushed the qa-modal-swap branch 4 times, most recently from a6cb441 to fc6826c Compare September 19, 2024 20:31

greg-in-a-box marked this pull request as ready for review September 19, 2024 20:35

greg-in-a-box requested review from a team as code owners September 19, 2024 20:35

tjiang-box reviewed Sep 19, 2024

View reviewed changes

greg-in-a-box force-pushed the qa-modal-swap branch 2 times, most recently from 7eff2e4 to f216c82 Compare September 20, 2024 19:58

rustam-e and others added 6 commits September 20, 2024 16:05

chore(storybook): Added global types for storybook

9231ab9

feat(content-answers): Upgrade Content Answers

62a4054

feat(content-answers): fix tests

a547df8

feat(content-answers): Enabled some new features

ef2d698

feat(content-answers): Feedback

de0375f

greg-in-a-box force-pushed the qa-modal-swap branch from f216c82 to de0375f Compare September 20, 2024 20:11

benjamin-shen reviewed Sep 24, 2024

View reviewed changes

tjuanitas reviewed Sep 24, 2024

View reviewed changes

greg-in-a-box force-pushed the qa-modal-swap branch 2 times, most recently from 279f2bc to b4a3884 Compare September 25, 2024 19:46

chore(content-answers): feedback

773c509

greg-in-a-box force-pushed the qa-modal-swap branch from b4a3884 to 773c509 Compare September 25, 2024 19:50

feat(content-answers): Feedback

e52ad9f

tjiang-box requested changes Sep 25, 2024

View reviewed changes

benjamin-shen reviewed Sep 25, 2024

View reviewed changes

greg-in-a-box force-pushed the qa-modal-swap branch 2 times, most recently from 4fc491f to e04c240 Compare September 25, 2024 22:56

greg-in-a-box force-pushed the qa-modal-swap branch from e71ae24 to 57f2a49 Compare September 25, 2024 23:40