Use public endpoints for AnswerBot

[Fredx87]

Description

This PR updates the Answer Bot modal, using the public endpoints instead of the internal ones.
The logic for redirecting the user to a new page and showing a notification has been ported in the theme. For this reason, there is a new notifications module that exposes a function to add a notification in the browser session storage, and it is also loaded on each page from the document_head template for showing any eventual notification present in the session storage.

Screenshots

Screen.Recording.2024-04-11.at.16.57.30.mov

Screen.Recording.2024-04-11.at.17.01.13.mov

Checklist

• 📗 all commit messages follow the conventional commits standard
• ⬅️ changes are compatible with RTL direction
• ♿ Changes to the UI are tested for accessibility and compliant with WCAG 2.1.
• 📝 changes are tested in Chrome, Firefox, Safari and Edge
• 📱 changes are responsive and tested in mobile
• 👍 PR is approved by @zendesk/vikings

[luis-almeida]

Do we need this extra bundle to show notifications on the request page?

[Fredx87]

We need this extra bundle to show the notifications on the new page where the user is redirected, not on the request page. This bundle is loaded in the document_head template:

copenhagen_theme/templates/document_head.hbs

Line 17 in 27cf94e

import {renderFlashNotifications} from "notifications-bundle";

I think it make sense to separate it from the request form bundle, otherwise we'll need to load the request form bundle in every page

[luis-almeida]

Nit: unsolveNotificationAndRedirect

[Fredx87]

I renamed it to addUnsolvedNotificationAndRedirect

[luis-almeida]

If we really need the separate notifications bundle, could it also contain this api? It seems like it will be loaded anyway in all pages.

[Fredx87]

This is what rollup emits, it creates a bundle with the code shared between the new-request-form bundle and the notifications bundle, mainly the addFlashNotification function and the things related to theming. I don't know if it is possible to avoid the creation of this bundle

[luis-almeida]

This is what rollup emits, it creates a bundle with the code shared between the new-request-form bundle and the notifications bundle

That's probably because we're importing addFlashNotification with a relative path and not from the already created notifications-bundle. In webpack that can be supported with aliases but then eslint-import-resolver-webpack is needed to "fix" eslint. Rollup might have something similar but tbh I've never set it up and it just feels like going in the wrong direction.

I don't know if it is possible to avoid the creation of this bundle

We just did something similar for the translations. Handling it with manualChunks instead of adding it to input should work:

if (id.includes("notifications")) {
  return "notifications";
}

I'm mostly concerned about two points:

Updatability

The more bundles/chunks we have the harder it will be for us to document how to update the app, and ultimately for the user to do so. Especially since the bundles start to depend more and more on each other.
We could also consider renaming vendor to shared and have notifications be part of it. That would remove the need to separate notifications and we'd have one less bundle.

Performance

notifications is being added on all pages and it depends on vendor which now needs to be loaded as well and it's adding quite a lot of weight. My first thought was to put it within {{#is signed_in}} ... {{/is}} so it's at least not loaded for anonymous users. Can anonymous users see the answer bot? If so then that won't work.
Another option is to only load it if there are indeed notifications to show in the session.

Let me know your thoughts.

[Fredx87]

We just did something similar for the translations. Handling it with manualChunks instead of adding it to input should work:

if (id.includes("notifications")) {
  return "notifications";
}

This doesn't work, because removing the notifications module from the input modules, will not bundle things not used in other modules, like renderFlashNotifications which is used only in the document_head template.

I'm mostly concerned about two points:

Updatability

The more bundles/chunks we have the harder it will be for us to document how to update the app, and ultimately for the user to do so. Especially since the bundles start to depend more and more on each other. We could also consider renaming vendor to shared and have notifications be part of it. That would remove the need to separate notifications and we'd have one less bundle.

I agree with this concern, but on the other hand, to avoid this we are hooking into the bundle setup. This makes our setup more complex and difficult to reason about. We are also potentially doing things against the optimizations already present in the bundler. Maybe we should just aim to find an easy way to update the components in the future, regardless of the bundler output and number of assets.

Including the notifications module into shared has the same problem as above, notifications needs to be an input.

Performance

notifications is being added on all pages and it depends on vendor which now needs to be loaded as well and it's adding quite a lot of weight. My first thought was to put it within {{#is signed_in}} ... {{/is}} so it's at least not loaded for anonymous users. Can anonymous users see the answer bot? If so then that won't work. Another option is to only load it if there are indeed notifications to show in the session.

Let me know your thoughts.

Anonymous users can see the AnswerBot modal.

It is true that in this way we are loading the vendor chunk on all pages, but since we are using ES modules all the bundles are loaded using defer, so after the page has been loaded and displayed.
I did a quick test and we would probably need to add also the async attribute to avoid blocking other scripts in the page, but this could be actually a performance improvement: if we load the vendor bundle on each page, even if it is not needed, without blocking the rest of HC, it will be already cached when needed in the new request form page

[luis-almeida]

I'm approving the PR but my reservations are still very much present.

I do agree that we should look into other ways of providing easier updates in the future but I am worried about the one we will have now.

I also think that forcing all users to always load this much JS when the vast majority of them will never need it is quite suboptimal and very hard to justify on our end. Even if it's non-blocking. But lets move forward and try to improve in a following step 👍🏼