Skip to content

Add removal of items in multi-selection for collections #6800

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

Closed
wants to merge 9 commits into from

Conversation

mektem
Copy link

@mektem mektem commented Apr 24, 2025

Changes:
Implement Remove from collections for multiselect in the collections screen. (issue #6772)
image

Issues:
N/A

Changes:
Implement Remove from collections for multiselect in the collections screen. (issue jellyfin#6772)

Issues:
N/A
@mektem mektem requested a review from a team as a code owner April 24, 2025 13:49
@jellyfin-bot
Copy link
Collaborator

jellyfin-bot commented Apr 24, 2025

Cloudflare Pages deployment

Latest commit 1b9d997
Status ✅ Deployed!
Preview URL https://49ee886d.jellyfin-web.pages.dev
Type 🔀 Preview

View build logs

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

ESLint doesn't pass. Please fix all ESLint issues.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

ESLint doesn't pass. Please fix all ESLint issues.

@mektem mektem marked this pull request as ready for review April 24, 2025 14:34
Copy link
Member

@viown viown left a comment

Choose a reason for hiding this comment

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

It looks like you have a race condition in your code that can sometimes prevent the "Remove from collection" menu item from showing.

If this promise resolves before this promise, collectionItem will be null and the menu item will not be added.

Instead, you'll need to re-structure your promises to follow an A -> B -> C structure. You can refactor your collection fetch call:

const collectionPromise = collectionId ? apiClient.getItem(user.Id, collectionId) : Promise.resolve(null)

Then wrap the previous promise with:

collectionPromise.then(collectionItem => {
    ...
})

@mektem mektem closed this Apr 27, 2025
@mektem mektem deleted the collection-fixes branch April 27, 2025 15:31
@mektem
Copy link
Author

mektem commented Apr 27, 2025

@viown New PR here with your above suggestion, and did some cleanup: #6808

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