-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Media Picker: Implement paging #20202
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ent with changed data types.
…ces-after-migration
fix bug for Media Picker is slow when you have a large number of images at the root folder umbraco#12364
This reverts commit 4d86734.
Replaces folder view calls to entityResource.getChildren with entityResource.getPagedChildren in MediaPickerController. - Adds orderBy/Direction and increases default pageSize to 200 - Resets pagination when entering folders or clearing search - Updates changePagination to work for both search and folder views - Keeps legacy behaviour for searchMedia (no breaking server changes)
|
Hi there @landlogicit, thank you for this contribution! 👍 While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:
Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution. If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request. Thanks, from your friendly Umbraco GitHub bot 🤖 🙂 |
…aco-CMS into v13/fix-issue-12364
|
Please review this pr, Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks very much for this @landlogicit. I've done some testing having artificially set a low number for the page size, and it all looks to work as expected.
I left a few inline comments below if you wouldn't mind taking a look at please? Some questions as to why certain changes were necessary, and some on the various comments added that feel more like points for the code reviewer and won't be useful or could be confusing once the PR is merged.
...Umbraco.Web.UI.Client/src/views/common/infiniteeditors/mediapicker/mediapicker.controller.js
Outdated
Show resolved
Hide resolved
...Umbraco.Web.UI.Client/src/views/common/infiniteeditors/mediapicker/mediapicker.controller.js
Outdated
Show resolved
Hide resolved
...Umbraco.Web.UI.Client/src/views/common/infiniteeditors/mediapicker/mediapicker.controller.js
Show resolved
Hide resolved
...Umbraco.Web.UI.Client/src/views/common/infiniteeditors/mediapicker/mediapicker.controller.js
Outdated
Show resolved
Hide resolved
...Umbraco.Web.UI.Client/src/views/common/infiniteeditors/mediapicker/mediapicker.controller.js
Outdated
Show resolved
Hide resolved
...Umbraco.Web.UI.Client/src/views/common/infiniteeditors/mediapicker/mediapicker.controller.js
Outdated
Show resolved
Hide resolved
...Umbraco.Web.UI.Client/src/views/common/infiniteeditors/mediapicker/mediapicker.controller.js
Outdated
Show resolved
Hide resolved
…ments - Removed unnecessary helper methods (pickPositive / pickNonNegative) and assign pagination values directly, as backend always provides valid positive/ non-negative numbers. - Renamed or removed review-only comments to keep codebase clean. - Clarified purpose of resetting `vm.searchOptions.filter` to explain why the filter is cleared after loading items. - Adjusted indentation and minor formatting for consistency.
|
Pushed an update that: Removes the pickPositive/pickNonNegative helpers and assigns pagination values directly, since the API guarantees valid numbers. Cleans up or removes review-only comments. Clarifies the purpose of resetting vm.searchOptions.filter when navigating folders. Fixes minor indentation/formatting. This should address all review comments and keep the MediaPickerController cleaner and easier to maintain. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates. The code all looks good now, but I found an issue in testing. Do you think you might be able to find a solution to this please?
- Open media picker
- Navigate into a folder that has more items than the page size.
- Navigate to page 2.
- Close the media picker.
- Open the media picker again. You will be returned to the last folder you were looking at, however now all items are shown rather than just a single page, and there's no paging details displayed.
|
Thanks for the catch! I’ve updated the navigation flow so that run(), ensureWithinStartNode(), gotoStartNode() and gotoFolder() consistently return and chain the same promise, ensuring the paged fetch (getChildren → getPagedChildren) always runs when restoring the last folder. This fixes the issue where reopening the media picker shows all items without paging metadata. |
Ensures the Media Picker always loads a paged result when restoring the last visited folder. - run(), ensureWithinStartNode(), gotoStartNode(), and gotoFolder() now return and chain the same promise flow. - gotoFolder() resolves path → sets current folder → resets pagination → calls getChildren() (which uses getPagedChildren). - Fixed cases where reopening the picker showed all items with no pager. - Kept existing UX: filter is cleared on folder navigation to start unfiltered. - Minor indentation/formatting cleanups.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does seem to work now, but the last commit contains quite a bit more change than I'd have expected. Can I ask if you are getting some AI help here? Not against that, but I'd like to make sure you are clear why the changes you are proposing resolve the issue, and to include only changes that are necessary. I get nervous otherwise! Could you share your method here please?
|
Thanks Andy, I completely understand your concern and I appreciate the careful review. The essential fix is about promise chaining: run(), ensureWithinStartNode(), gotoStartNode(), and gotoFolder() now consistently return and chain the same promise so that the paged fetch (getChildren → getPagedChildren) always runs when restoring the last folder. I can certainly trim or squash the commit to show only those minimal, necessary changes if that helps make the diff clearer and keeps history clean. |
|
Fantastic, thanks very much for the explanation @landlogicit. Don't worry about the git history, that's not a concern as we'll squash the PR when merging anyway. Now you've explained the solution you've come up with that's good for me. |
AndyButland
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have manually tested fix by temporarily configuring a low page size, and verifying top level and lower level folders correctly handle paging.
Thanks very much for the effort on this @landlogicit, and the fix, it's much appreciated.
Summary
This PR improves the Media Picker performance when browsing folders
containing a large number of media items.
Changes
entityResource.getChildrenwithentityResource.getPagedChildrenin MediaPickerController.
orderBy/orderDirectionand sets a default pageSize of 200.changePaginationto handle both folder browsing and search results.Why
Currently the Media Picker loads all children of a folder in one call,
which can be very slow or even cause timeouts when the folder contains
hundreds or thousands of media items.
By using
getPagedChildrenwe only load one page of results at a time,improving performance and memory usage.
Breaking changes
None.
This is a pure client-side change; no modifications to
EntityControlleror server-side APIs are required. Existing external consumers of the
back-office API remain unaffected.
Testing
that items are loaded and displayed correctly.