Add support for bulk adding items to a list#1227
Add support for bulk adding items to a list#1227olioby wants to merge 6 commits intoFuzzyGrim:devfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the user experience by introducing a bulk selection and addition mechanism for media items to custom lists. This feature addresses a common pain point for users migrating data from other services, allowing them to efficiently organize large collections of media. The changes involve both frontend interactivity using Alpine.js and HTMX, and new backend logic to process bulk operations securely and efficiently. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
olioby
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a feature to bulk-add media items to a custom list, utilizing Alpine.js for frontend state management and htmx for server communication. However, a reflected Cross-Site Scripting (XSS) vulnerability was identified in the media_list.html template where URL parameters are rendered directly into an Alpine.js attribute without proper JavaScript escaping. Additionally, suggestions have been made to improve the security and efficiency of the backend view handling item additions, and to optimize a frontend JavaScript function for better performance.
| custom_list_id = request.POST["custom_list_id"] | ||
|
|
||
| custom_list = get_object_or_404(CustomList, id=custom_list_id) | ||
|
|
||
| if custom_list.user_can_edit(request.user): | ||
| CustomListItem.objects.bulk_create( | ||
| [CustomListItem(custom_list=custom_list, item_id=i) for i in item_ids], | ||
| ignore_conflicts=True, | ||
| ) | ||
| logger.info("%d items bulk added to %s.", len(item_ids), custom_list) | ||
|
|
||
| return render(request, "lists/components/bulk_fill_lists_success.html") | ||
|
|
||
| messages.error(request, "You do not have permission to edit this list.") | ||
| return helpers.redirect_back(request) |
There was a problem hiding this comment.
The current implementation for fetching the CustomList and checking for edit permissions can be improved for security and efficiency.
- Accessing
request.POST["custom_list_id"]directly will raise aMultiValueDictKeyErrorif the key is missing. It's safer to userequest.POST.get()and handle the case where it's not provided. - Fetching the object and then checking for permissions separately can leak information about the existence of a list a user doesn't have access to. It's better to incorporate the permission check into the
get_object_or_404query. This also makes the view consistent withlist_item_toggle.
Here's a suggested refactoring that addresses both points and simplifies the code by removing the if/else block for permissions.
| custom_list_id = request.POST["custom_list_id"] | |
| custom_list = get_object_or_404(CustomList, id=custom_list_id) | |
| if custom_list.user_can_edit(request.user): | |
| CustomListItem.objects.bulk_create( | |
| [CustomListItem(custom_list=custom_list, item_id=i) for i in item_ids], | |
| ignore_conflicts=True, | |
| ) | |
| logger.info("%d items bulk added to %s.", len(item_ids), custom_list) | |
| return render(request, "lists/components/bulk_fill_lists_success.html") | |
| messages.error(request, "You do not have permission to edit this list.") | |
| return helpers.redirect_back(request) | |
| custom_list_id = request.POST.get("custom_list_id") | |
| if not custom_list_id: | |
| messages.error(request, "No list was selected.") | |
| return helpers.redirect_back(request) | |
| custom_list = get_object_or_404( | |
| CustomList.objects.filter( | |
| Q(owner=request.user) | Q(collaborators=request.user), id=custom_list_id | |
| ).distinct() | |
| ) | |
| CustomListItem.objects.bulk_create( | |
| [CustomListItem(custom_list=custom_list, item_id=i) for i in item_ids], | |
| ignore_conflicts=True, | |
| ) | |
| logger.info("%d items bulk added to %s.", len(item_ids), custom_list) | |
| return render(request, "lists/components/bulk_fill_lists_success.html") |
| }); | ||
| </script> | ||
|
|
||
| <div x-data="{ sort: '{{ current_sort }}', status: '{{ current_status }}', layout: '{{ current_layout }}', search: '{{ request.GET.search|default:'' }}', bulkListsOpen: false, statusLabels: { |
There was a problem hiding this comment.
The search parameter from the URL, as well as current_sort, current_status, and current_layout, are rendered directly into an Alpine.js x-data attribute. Because these values are enclosed in single quotes and not properly escaped for a JavaScript context, an attacker can provide a malicious value (e.g., in the search query parameter) containing single quotes to break out of the string and execute arbitrary JavaScript (Reflected XSS).
| selectAll() { | ||
| document.querySelectorAll('[data-bulk-id]').forEach(function(el) { | ||
| const id = parseInt(el.dataset.bulkId); | ||
| if (Alpine.store('bulk').ids.indexOf(id) === -1) Alpine.store('bulk').ids.push(id); | ||
| }); | ||
| }, |
There was a problem hiding this comment.
The selectAll function can be implemented more efficiently. The current implementation iterates over all visible items and checks for existence in the ids array one by one, which can be slow if there are many items on the page or many items are already selected.
A more performant approach would be to get all visible IDs, merge them with the already selected IDs using a Set to handle duplicates, and then update the ids array.
selectAll() {
const allVisibleIds = Array.from(document.querySelectorAll('[data-bulk-id]'), el => parseInt(el.dataset.bulkId));
this.ids = Array.from(new Set([...this.ids, ...allVisibleIds]));
},
1 participant
Hey, I took a stab at adding support for adding multiple media items to a custom list in one go. This was a big pain point for me and something that makes it very hard to migrate from other services like imdb or goodreads.
Let me know what you think of the approach - I'm not very well versed in Django/htmx so I'd appreciate a critical look.
Thanks @FuzzyGrim for all your work on this project!
Screenshots