feat: refactor library routes and add section/subsection tabs by rpenido · Pull Request #2039 · openedx/frontend-app-authoring

rpenido · 2025-05-28T21:52:49Z

Description

This PR adds the "Section" and "Subsections" tabs to the library authoring and refactors our library router hook to fix some ambiguities and solve some bugs.

Additional Information:

Related to:
- Library section/subsection tabs #1615
- Implement a selection state that is distinct from focus visibility. #1792 (comment)

Testing Instructions

Create a new section/subsection and navigate to the new tabs
The tabs should not have the "Type"
Check the URL changes while changing tabs and selecting components, collections, units, subsections and sections
Check also that you can navigate back and forth between the states
Check that you can not stay in a state where the selected component is not in context:
- Select a "Unit" and click on the "Components" tab, which should deselect the unit
- Select a "Component" and click on the "Components" tab, which should leave the component selected
Opening a Collection/Unit page should open the Collection/Info sidebar, ignoring prior selected components
Check the component picker
- In the Unit Page, click on "Add Existing Content" and you should only see components
- In the Collection Page, click on"Add Existing Content" and you should see components, units, sections, subsections, but not collections

Private ref: https://tasks.opencraft.com/browse/FAL-4168

openedx-webhooks · 2025-05-28T21:52:53Z

Thanks for the pull request, @rpenido!

This repository is currently maintained by @bradenmacdonald.

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.

Details

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:

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.

rpenido · 2025-05-28T22:06:06Z

-  useEffect(() => {
-    if (!componentPickerMode) {
-      openInfoSidebar(componentId, collectionId, unitId);
-    }
-  }, []);
-


Moved to SidebarContext

rpenido · 2025-05-28T22:06:32Z

-  /*
-
-  <FilterByPublished key={
-                // It is necessary to re-render `FilterByPublished` every time `FilterByBlockType`
-                // appears or disappears, this is because when the menu is opened it is rendered
-                // in a previous state, causing an inconsistency in its position.
-                // By changing the key we can re-render the component.
-                !(insideCollections || insideUnits) ? 'filter-published-1' : 'filter-published-2'
-              }
-  */
-


This seems to be a temporary/not used code commented

rpenido · 2025-05-28T22:07:40Z

-    <Routes>
+const LibraryLayout = () => (
+  <Routes>
+    <Route element={<LibraryLayoutWrapper />}>


Added the wrapper using proper route nesting instead of a function

rpenido · 2025-05-28T22:08:42Z

-  componentId: string | undefined;
-  setComponentId: (componentId?: string) => void;


Removed misleading componentId.

rpenido · 2025-05-28T22:09:28Z

  const [collectionId, setCollectionId] = useState(
-    skipUrlUpdate ? undefined : urlCollectionId || (!selectedItemIdIsUnit ? urlSelectedItemId : undefined),


This state is now only for the collection page

rpenido · 2025-05-28T22:09:49Z

-    skipUrlUpdate ? undefined : urlCollectionId || (!selectedItemIdIsUnit ? urlSelectedItemId : undefined),
+    skipUrlUpdate ? undefined : urlCollectionId,
  );
  const [unitId, setUnitId] = useState(


This state is now only for the unit page

codecov · 2025-05-29T02:18:38Z

Codecov Report

Attention: Patch coverage is 97.94521% with 3 lines in your changes missing coverage. Please review.

Project coverage is 94.01%. Comparing base (fffa9e2) to head (3adac7d).
Report is 11 commits behind head on master.

Files with missing lines	Patch %	Lines
src/library-authoring/routes.ts	96.00%	2 Missing ⚠️
...c/library-authoring/collections/CollectionInfo.tsx	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2039      +/-   ##
==========================================
+ Coverage   93.99%   94.01%   +0.02%     
==========================================
  Files        1155     1154       -1     
  Lines       24177    24185       +8     
  Branches     5119     5240     +121     
==========================================
+ Hits        22724    22737      +13     
+ Misses       1385     1371      -14     
- Partials       68       77       +9

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

pomegranited

Hi @rpenido , I found some bugs with the route changes, so haven't finished my review.
I'm wondering if we shouldn't keep these changes smaller to reduce regressions?

pomegranited · 2025-05-30T14:48:12Z

  collectionId?: string,
  contentType?: ContentType,
  unitId?: string,
-  doubleClicked?: boolean,


This PR breaks the "open on double click" behaviour added by #2002 ; they're now opening on single click.

I assume Axim will want Sections and Subsections to open on double click as well (not single click).

Hi @pomegranited!
I didn't remove the double-click behaviour.

Calling navigateTo({ collectionId: "someId" }) now opens the collection page and navigateTo({ selectedItemId: "someId"}) opens the sidebar, so we don't need the double-click anymore.

It was required before because the collectionId could mean "collection open on sidebar" or "collection page open".

Example here:
https://github.com/open-craft/frontend-app-authoring/blob/c6509a92effe6b38d24b868c8ccdf02aa77db405/src/library-authoring/components/ContainerCard.tsx#L205-L219

Can you report to me if it is not working for you?

Ahhh.. I understand now. Yep, double clicking is still working as expected, sorry for the noise!

pomegranited · 2025-05-30T14:53:26Z

+      // Overwrite the params with the provided values.
+      ...((selectedItemId !== undefined) && { selectedItemId }),
+      ...((unitId !== undefined) && { unitId }),
+      ...((collectionId !== undefined) && { collectionId }),


There's a bug when I try to switch back and forth between the All Content and Section / Subsection tabs.
To reproduce:

Start on the main library "All Content" tab

Click on Subsections tab: correct url /subsections ✅

Click on All Content: incorrectly retains the /subsections URL ❌

Same thing happens when switching between "All Content" and "Sections", but not with any of the other tabs.

"All Content" -> "Section" -> "All Content" or

FYI I'm not sure what part of the refactor broke this -- I'm just commenting here.

A different bug when switching between Collections and Subsections / Sections:

Start on the main All Content tab.

Click Subsections: correct url /subsections ✅

Click Collections: incorrect url /collections/subsections ❌

Same thing happens with Sections -> Collections.

Sorry for that @pomegranited! 😞
I missed adding the routes! Fixed here: 950914d

rpenido · 2025-05-30T19:28:30Z

+  useEffect(() => {
+    // Update the active key whenever the route changes. This ensures that the correct tab is selected
+    // when navigating using the browser's back/forward buttons because it does not trigger a re-render.
+    setActiveKey(getActiveKey());
+  }, [location.key, getActiveKey]);


This unreported bug exists on master

pomegranited

👍 The refactoring you did here is really great @rpenido ! Much cleaner now, I think.
And thank you for finding and fixing those bugs with back/forth!

I tested this on my tutor dev stack using the PR test instructions, and everything I could think to do.
I read through the code
I checked for accessibility issues by using my keyboard to navigate
Includes documentation -- code comments
User-facing strings are extracted for translation

ChrisChV · 2025-05-31T00:11:21Z

@rpenido Thanks for this refactor! I found an issue on the component picker on the collection Page:

It is not possible to switch to other tabs.
The same adding library content in a course

https://www.loom.com/share/742a87206c364edbac391640217c158f?sid=d2cef863-f5ca-4cf2-b15a-6f321c4794ac

ChrisChV

@rpenido Great work! Thanks for this refactor! I left some comments

ChrisChV · 2025-05-31T00:08:25Z

-          element={<ComponentPicker extraFilter={['NOT block_type = "unit"']} />}
+          element={(
+            <ComponentPicker
+              extraFilter={['NOT block_type = "unit", "NOT block_type = "section"', 'NOT block_type = "subsection"']}


Also, shouldn't visibleTabs be added like this? I'm not sure where this case is used to see the Component Picker

Also, shouldn't visibleTabs be added like this? I'm not sure where this case is used to see the Component Picker

Sure! That makes sense!
Fixed: a4a6d99

Note: This current code is used on the legacy studio, where an iframe opens the route http://apps.local.edly.io:2001/authoring/component-picker to select components for courses.

ChrisChV · 2025-05-31T00:24:33Z

+
+    // Handle selected item id changes
+    if (selectedItemId) {
+      if (selectedItemId.includes(':unit:')) {


What do you think about using getContainerTypeFromId to check if it is a unit and, in the future, check if it is another container?

Done: 3adac7d

ChrisChV · 2025-05-31T00:34:32Z

+    });
+  });
+
+  it('should navigate to the container if double clicked', async () => {


@rpenido Thanks for these tests!

pomegranited · 2025-06-02T16:19:29Z

It is not possible to switch to other tabs.

Good catch @ChrisChV ! I neglected to test that detail, so maybe @rpenido can add tests for it here?

…rMode

rpenido · 2025-06-02T17:00:43Z

It is not possible to switch to other tabs.

Good catch @ChrisChV ! I neglected to test that detail, so maybe @rpenido can add tests for it here?

Sure! Added test here: 3ccddc8

rpenido · 2025-06-02T17:11:50Z

It is not possible to switch to other tabs.

The same adding library content in a course

Thanks for catching that! Fixed here: 636653d

ChrisChV

Looks good 👍

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label May 28, 2025

openedx-webhooks added this to Contributions May 28, 2025

rpenido marked this pull request as draft May 28, 2025 21:52

github-project-automation Bot moved this to Needs Triage in Contributions May 28, 2025

rpenido force-pushed the rpenido/fal-4168/library-section-subsection-tabs branch from 439fd90 to 10b54ce Compare May 28, 2025 22:05

rpenido commented May 28, 2025

View reviewed changes

rpenido force-pushed the rpenido/fal-4168/library-section-subsection-tabs branch 8 times, most recently from 5159eda to 3656f84 Compare May 29, 2025 02:09

rpenido force-pushed the rpenido/fal-4168/library-section-subsection-tabs branch 8 times, most recently from 07c061d to 8130606 Compare May 29, 2025 15:15

rpenido mentioned this pull request May 29, 2025

Implement a selection state that is distinct from focus visibility. #1792

Closed

rpenido force-pushed the rpenido/fal-4168/library-section-subsection-tabs branch 3 times, most recently from 4603202 to a45d642 Compare May 29, 2025 19:16

rpenido changed the title ~~feat: library section subsection tabs~~ feat: refactor library routes and add section/subsection tabs May 29, 2025

rpenido force-pushed the rpenido/fal-4168/library-section-subsection-tabs branch from a45d642 to acf2077 Compare May 29, 2025 19:28

feat: refactor library routes and add section/subsection tabs

c6509a9

rpenido force-pushed the rpenido/fal-4168/library-section-subsection-tabs branch from acf2077 to c6509a9 Compare May 29, 2025 19:39

rpenido marked this pull request as ready for review May 29, 2025 20:21

rpenido requested a review from pomegranited May 29, 2025 22:39

pomegranited suggested changes May 30, 2025

View reviewed changes

rpenido added 3 commits May 30, 2025 15:32

fix: add section and subsection routes

950914d

fix: navigation on tab change

312ed20

fix: doesnt need to set values to context because it will be re-rendered

9144414

rpenido force-pushed the rpenido/fal-4168/library-section-subsection-tabs branch from 5846373 to 9144414 Compare May 30, 2025 18:39

rpenido commented May 30, 2025

View reviewed changes

pomegranited approved these changes May 30, 2025

View reviewed changes

ChrisChV requested changes May 31, 2025

View reviewed changes

test: add test to check if we can change tabs while on ComponentPicke…

3ccddc8

…rMode

rpenido added 2 commits June 2, 2025 14:09

fix: changing tabs while on component picker

636653d

fix: component picker params for iframe

a4a6d99

fix: check container type

3adac7d

ChrisChV self-requested a review June 2, 2025 18:34

ChrisChV approved these changes Jun 2, 2025

View reviewed changes

ChrisChV merged commit cfb4944 into openedx:master Jun 2, 2025

github-project-automation Bot moved this from Needs Triage to Done in Contributions Jun 2, 2025

ChrisChV deleted the rpenido/fal-4168/library-section-subsection-tabs branch June 2, 2025 19:05

		componentId: string \| undefined;
		setComponentId: (componentId?: string) => void;

		const [collectionId, setCollectionId] = useState(
		skipUrlUpdate ? undefined : urlCollectionId \|\| (!selectedItemIdIsUnit ? urlSelectedItemId : undefined),

Uh oh!

Conversation

rpenido commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Additional Information:

Testing Instructions

Uh oh!

openedx-webhooks commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rpenido May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rpenido May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pomegranited left a comment

Choose a reason for hiding this comment

Uh oh!

pomegranited May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rpenido May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pomegranited left a comment

Choose a reason for hiding this comment

Uh oh!

ChrisChV commented May 31, 2025

Uh oh!

ChrisChV left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pomegranited commented Jun 2, 2025

Uh oh!

rpenido commented Jun 2, 2025

Uh oh!

rpenido commented Jun 2, 2025

Uh oh!

ChrisChV left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

rpenido commented May 28, 2025 •

edited

Loading

openedx-webhooks commented May 28, 2025 •

edited

Loading

rpenido May 28, 2025 •

edited

Loading

rpenido May 28, 2025 •

edited

Loading

codecov Bot commented May 29, 2025 •

edited

Loading

pomegranited May 30, 2025 •

edited

Loading

rpenido May 30, 2025 •

edited

Loading