Skip to content
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

metadata update function catch case fixes #6559 #6571

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jagadam97
Copy link

@jagadam97 jagadam97 commented Feb 23, 2025

Fix: catch block for metadataupdate function to fix infinite loader when update api fails fixes #6559

Changes

  1. as described in Endlessly spin when adding people without selecting type #6559 there is a infinite loader in update metadata when the api fails. This can be avoided by using try catch method. as such the same has been implemented in this PR.

Issues
fixes #6559

Proof of testing
https://youtu.be/3YHXEthmzoY

@jagadam97 jagadam97 requested a review from a team as a code owner February 23, 2025 03:56
@jellyfin-bot
Copy link
Collaborator

jellyfin-bot commented Feb 23, 2025

Cloudflare Pages deployment

Latest commit ae4e64d
Status ✅ Deployed!
Preview URL https://2efcf138.jellyfin-web.pages.dev
Type 🔀 Preview

View build logs

@@ -85,7 +85,7 @@
bottom: 0;
left: 0;
right: 0;
background: rgba(0, 0, 0, 0.7);
background: none;
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated changes.

Copy link
Member

Choose a reason for hiding this comment

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

I would split this fix into a separate PR targeting the release-10.10.z branch.

function afterContentTypeUpdated() {
toast(globalize.translate('MessageItemSaved'));
async function submitUpdatedItem(form, item) {
const apiClient = getApiClient();
Copy link
Contributor

Choose a reason for hiding this comment

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

I would move it back to where it was to make fewer changes.

apiClient.ajax({
try {
await apiClient.updateItem(item);
const newContentType = form.querySelector('#selectContentType')?.value || '';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why check the existence of an element on your own static form?
I'm just not a fan of overly secure programming (just checking for the sake of checking). 😅


url: apiClient.getUrl('Items/' + item.Id + '/ContentType', {
if ((metadataEditorInfo?.ContentType || '') !== newContentType) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably shouldn't even start doing anything if metadataEditorInfo is undefined/null. Maybe just throw an exception at the beginning of the function instead of optional chaining?

Copy link
Author

Choose a reason for hiding this comment

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

seeing that this check is to just update the items content type when there is change in content type and ignore if otherwise. we can directly ignore the check and lets do the update content api even if its redundant.

@jagadam97 jagadam97 force-pushed the metadataupdater-catch-case branch from cbcad15 to ae4e64d Compare February 23, 2025 14:00
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.

Endlessly spin when adding people without selecting type
4 participants