Skip to content

Conversation

@bra-i-am
Copy link

@bra-i-am bra-i-am commented Jan 9, 2026

This pull request refactors and optimizes how collections and their components are indexed and filtered in the library authoring UI. The main improvements are the introduction of a centralized collection indexing context for O(1) lookups, frontend filtering for the Collections tab to enable more flexible workflows, and associated updates to component interfaces and context providers. These changes improve performance and maintainability, especially when working with large sets of collections and components.

Collection indexing and context improvements:

  • Introduced CollectionIndexProvider and related hooks/types in ComponentPickerContext.tsx to precompute and share collection/component relationships and sizes across the UI, avoiding repeated computation and improving performance. This includes new interfaces and a context for accessing the precomputed data. [1] [2]
  • Updated imports and wrapped relevant page components (LibraryAuthoringPage and LibraryCollectionPage) with CollectionIndexProvider to ensure all children have access to the precomputed collection index data. [1] [2] [3] [4] [5] [6]

Filtering and content display logic:

  • Moved filtering of collections in the Collections tab from the backend to the frontend in LibraryContent.tsx, ensuring all components remain available in the search hits for selection workflows. Adjusted the filtering logic in LibraryAuthoringPage.tsx accordingly. [1] [2] [3]
  • Updated LibraryCollectionComponents.tsx to use the default LibraryContent (with frontend filtering) instead of passing a collections-specific content type. [1] [2]

Type and interface enhancements:

  • Extended selection-related interfaces to support collections, including new types for selected collections, collection status, and events that handle both components and collections. [1] [2] [3] [4]

Test data updates:

  • Added block_id fields to mock collection data in library-search.json to better reflect the updated data structures and support new indexing logic. [1] [2] [3] [4] [5]

How to test

Make sure you have a collection in your Libraries ( http://apps.local.openedx.io:2001/authoring/libraries ) with questions in it

  1. Have a Teak environment
  2. Mount this MFE and switch to this branch
  3. Install https://github.com/eduNEXT-collab/xblock-exam-question-bank in the CMS
  4. Go to the Authoring MFE ( http://apps.local.openedx.io:2001/authoring/ ), create a new course or use any existing one, go to Settings > Advanced Settings and add "examquestionbank" to the list of Advanced module list
  5. Go to the content outline and access a unit
  6. Click Advanced in the component selector and add an Exam Question Bank
  7. Click Add Problems and select a collection; it should be selected and deselected properly, and once you click Add Selected Components, they should be displayed in the xblock as will be shown in the following screencast

Screencast

Screencast.from.09-01-26.14.40.43.webm

@bra-i-am bra-i-am marked this pull request as ready for review January 14, 2026 13:14
@MaferMazu MaferMazu self-requested a review January 15, 2026 15:55
Copy link

@MaferMazu MaferMazu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The functionality works fine, thanks. My only concern is the efficiency of selecting elements from the collections (because the number of problems and collections will increase). We can handle documentation that explains the client's limitations, but if we can find a way to make it more efficient, that would be great.


// Select another component
fireEvent.click(screen.queryAllByRole('button', { name: 'Select' })[1]);
fireEvent.click(screen.queryAllByRole('button', { name: 'Select' })[7]);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add more information on why we need this change? Something simple.
I won't argue about the test change, but it could be valuable to track the why.

Copy link
Author

@bra-i-am bra-i-am Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MaferMazu, this PR allows selecting collections too, due to it, once in the test it is queried screen.queryAllByRole('button', { name: 'Select' }) which returns an array bigger than before (before contained 3 items, now it includes 9 items counting the collections) and taking into account that in the library-search.json the collections are before the components (and rendered before the components), the index of the components changed from 1 to 7 (with 6 collections in between)

I could change the library-search.json items order and preserve the test with index 1, what do you think?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you can change the JSON order, or in this case, leave it as it is; at least we have it documented in some way. Another thing you could do is use a specific id instead of the index, or filter only the components (but not necessary for this PR).

@bra-i-am bra-i-am force-pushed the bc/allow-selecting-collections-ceibal branch from c9c43bd to d232be1 Compare January 19, 2026 21:53
@bra-i-am bra-i-am force-pushed the bc/allow-selecting-collections-ceibal branch from d232be1 to b6908d1 Compare January 20, 2026 14:08
@bra-i-am bra-i-am requested review from MaferMazu and Copilot January 20, 2026 14:10

This comment was marked as duplicate.

This comment was marked as resolved.

@eduNEXT eduNEXT deleted a comment from Copilot AI Jan 20, 2026
@eduNEXT eduNEXT deleted a comment from Copilot AI Jan 20, 2026
@eduNEXT eduNEXT deleted a comment from Copilot AI Jan 20, 2026
@eduNEXT eduNEXT deleted a comment from Copilot AI Jan 20, 2026
@eduNEXT eduNEXT deleted a comment from Copilot AI Jan 20, 2026
@eduNEXT eduNEXT deleted a comment from Copilot AI Jan 20, 2026
@eduNEXT eduNEXT deleted a comment from Copilot AI Jan 20, 2026
Copy link

@MaferMazu MaferMazu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bra-i-am thanks a lot for this PR, it worked as expected!

@MaferMazu MaferMazu self-requested a review January 22, 2026 22:49
@MaferMazu
Copy link

@bra-i-am I just realized that with big numbers, we have an issue when selecting elements.
It is because we don't show all the components. If a collection has, for example, 22 elements, selecting the collection adds only the visible ones (fewer than 22).

Can you guide me on how to fix that, or, if there isn't much time, help me fix it?

Screencast.from.2026-01-22.17-57-12.webm

@bra-i-am bra-i-am force-pushed the bc/allow-selecting-collections-ceibal branch from 9fd69a0 to 7fc902e Compare January 23, 2026 21:43
@bra-i-am bra-i-am force-pushed the bc/allow-selecting-collections-ceibal branch from 7fc902e to d0cb659 Compare January 23, 2026 21:44
Copy link

@MaferMazu MaferMazu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all the changes, @bra-i-am. I'll use it on stage to finish testing.
I noticed we had a test comment regarding nextOffset and getNextPageParam. If we are not going to solve it, can we leave a comment in the PR explaining why, or can we solve it?

@MaferMazu
Copy link

MaferMazu commented Jan 26, 2026

To add information to this PR, I asked @bra-i-am to increase the number of components shown per page in libraries from 20 to 100. This is intended to reduce the time spent searching between pages when bringing all the elements from the collections. But it is because, in our case, for Ceibal, we expect to use that amount of components or more.

That change, for now, is not part of the upstream change openedx#2824. I added to the Ceibal product documentation that it would be desirable to add an upstream change that allows configuring that limit.

@MaferMazu
Copy link

@bra-i-am I tested in stage, and it worked as expected. Thanks a lot for solving the issues. After solving the nextOffset issue, or at least having a comment regarding that, we can merge this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants