fix: a11y and performance issues with library authoring navigation [FC-0076]

[pomegranited]

Description

This PR fixes some a11y and performance issues for Library Authors introduced by #1575.

Accessibility

1. When navigating between the various Library Authoring pages, the search keyword box should not be auto-focused, so focus will follow the user's click/tab events and keep that continuity.
   The course content search modal behavior is unchanged -- loading this modal auto-focuses the search input box as one would expect.
   Current behavior: see Shareable URLs for library components and searches #1499 (comment)
   Fixed behavior: https://github.com/user-attachments/assets/539aad1b-13ae-48da-a5f1-6f15cb6f2e4e

Performance

1. Navigating between the various Library Authoring pages was causing a full re-mount of the <LibraryLayout/> component -- this has been fixed by e92eeb6.
2. React query's staleTime option is used to control how long data is considered fresh in both queries and routes. By default, the staleTime for queries is set to 0 milliseconds, meaning that the data will always be considered stale and will be refetched whenever the query is mounted.
   244cfc6 fixes this by setting the QueryClient's default staleTime to 1 hour.

Supporting information

See #1499 (comment)
Private-ref: FAL-3984

Testing instructions

1. View a course, and click the "Search" icon to load the search modal
   Note that the search input box still has the initial focus, so you can start typing keywords immediately.
2. View a library with multiple components and/or collections in it
3. Click through the different components/collection cards.
   Note that the search input box does not capture focus any more.
4. Open the page Inspector's Network tab while navigating around your library.
   Note that the API calls to studio are not re-fetched once they're loaded (except for the component iframe, which is loaded every time you load a component, e.g http://studio.local.openedx.io:8001/xblocks/v2/lb:OpenCraft:FAL-3984:html:19b3a1f4-0d98-41d9-82d9-ee5096cb9bf5/embed/student_view/).
5. Edit a component or collection -- changing title, content, tags, and collection membership.
   Ensure that these changes appear in the UI after save.

[openedx-webhooks]

Thanks for the pull request, @pomegranited!

This repository is currently maintained by @openedx/2u-tnl.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

• If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
  • This process (including the steps you'll need to take) is documented here.
• If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

• Dependencies

  This PR must be merged before / after / at the same time as ...

• Blockers

  This PR is waiting for OEP-1234 to be accepted.

• Timeline information

  This PR must be merged by XX date because ...

• Partner information

  This is for a course on edx.org.

• Supporting documentation
• Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

---

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

• Overview of Review Process for Community Contributions
• Pull Request Status Guide
• Making changes to your pull request

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

• The size and impact of the changes that it introduces
• The need for product review
• Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

[navinkarkera]

@pomegranited It seems like the real problem is with the page refresh(fast-page reload via react routes) on clicking any component/collection/library-info that updates the browser url. If you click on any of these objects, all the data like search results, library info etc. is fetched again from server. I am not sure how we can fix that and still have sharable urls. 🥲

[pomegranited]

@navinkarkera

If you click on any of these objects, all the data like search results, library info etc. is fetched again from server. I am not sure how we can fix that and still have sharable urls.

Oh ya, that's pretty bad :/

I got all excited when I saw the search-manager's useQuery calls pass in refetchOnWindowFocus: false and sometimes refetchOnMount: false, but when I tried using these parameters in the library-authoring apiHooks, it didn't reduce the queries on navigate at all :(

@rpenido do you know more about these options and how/whether we could use them here?

[rpenido]

@rpenido do you know more about these options and how/whether we could use them here?

The main issue is that we unmount and mount a new LibraryLayout every time we change the location.pathname. You can see that (on this PR) when we click on a card, the "select retangle" around the card vanishes right away because the whole page is being unmounted and mounted again (we don't get a flicker because react does its magic).

I did some (not exhaustive) tests and think we don't need to do the unmount/mount after changing location.pathname. This solves most (all?) unnecessary fetches (open-craft#79). If any component doesn't work well after this change, we can try to fix it at the component level (ping me if you need more eyes!).

Alternatively, to make the refetchOnX work, I think we need to set the staleTime as we did on useContentSearchConnection (the default is 0).

Also, setting and maintaining this behavior on all hooks will require a lot of work. If we think this is necessary, it will probably be better to set this behavior for the whole app, setting it on the QueryClient instance.

Edit: I found an issue (bug?): If we click a second time on an already selected collection, we are redirected to the collection page. I'm unsure if that was intended, so I "fixed" it on the PR. Feel free to ignore the second commit on the PR if it was intentional, or fix it using another approach (I left some comments on the PR)!

[bradenmacdonald]

@rpenido do you know more about these options and how/whether we could use them here?

@pomegranited It's well worth reading the official docs: https://tanstack.com/query/v4/docs/framework/react/reference/useQuery I agree with @rpenido that setting staleTime should fix it, but the bigger issue is that everything is being unmounted and mounted. His fix in open-craft#79 looks right to me.

[pomegranited]

Thank you for your help here @rpenido !

I used your <LibraryLayout /> fix -- that explained a lot :) I also set the default staleTime to 1h at the QueryClient level, after finding that making this change across the library-authoring, content-tag-drawer, and taxonomy apiHooks got rid of the extra API calls.

Edit: I found an issue (bug?): If we click a second time on an already selected collection, we are redirected to the collection page. I'm unsure if that was intended, so I "fixed" it on the PR. Feel free to ignore the second commit on the PR if it was intentional, or fix it using another approach (I left some comments on the PR)!

I chose not to include this change though, because I like that "double click card" behaviour -- but will flag it with Product and make sure they agree. and so does @sdaitzman (cf Slack).

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.24%. Comparing base (98fbcff) to head (693da9c).
Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1593      +/-   ##
==========================================
+ Coverage   93.23%   93.24%   +0.01%     
==========================================
  Files        1098     1100       +2     
  Lines       21709    21795      +86     
  Branches     4593     4703     +110     
==========================================
+ Hits        20240    20323      +83     
+ Misses       1404     1400       -4     
- Partials       65       72       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[navinkarkera]

@rpenido @pomegranited Nice work! The issue with re-mounting library page is fixed but now opening collection is not working consistently, the first collection page that you open is opened everytime after that even if you click on another collection. And sometimes it also crashes.

I also tested with additional changes that @rpenido has done in open-craft#79

[pomegranited]

@navinkarkera

The issue with re-mounting library page is fixed but now opening collection is not working consistently, the first collection page that you open is opened everytime after that even if you click on another collection. And sometimes it also crashes.

Ah ok -- I ended up re-instating the key={collectionId} code we had before PR#1575, see 693da9c.

[navinkarkera]

@pomegranited Perfect 👍

• I tested this: (Tested all library tabs and collections pages)
• I read through the code
• I checked for accessibility issues
• Includes documentation