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

Still visible in the sidebar but nothing appears in the tracklist (ISRC, comments, AcoustID) any more #596

Closed
4 tasks done
Tracked by #38
jesus2099 opened this issue Jun 20, 2021 · 31 comments
Closed
4 tasks done
Tracked by #38
Labels
Milestone

Comments

@jesus2099
Copy link
Owner

jesus2099 commented Jun 20, 2021

#38 – NG with release with more than 10 mediums after MBS-3841

Workaround is to make this script near or at the end of your userscript list.


We cannot see recording comments since some change on beta, that will probably occur on MBS to, soon.

Spotted by @Lotheric: #445 (comment)

Related to

@jesus2099
Copy link
Owner Author

jesus2099 commented Jun 20, 2021

There is still problems with the other tools (recording open edits, relate/merge tools and Edit rec. buttons).
It seems to be because of react-hydrate nightmare redrawing the last column. 😨
But beta release table last 2 columns are not stable yet, see how ratings are not clickable: https://tickets.metabrainz.org/browse/MBS-11735

@tigerman325
Copy link

tigerman325 commented Jun 29, 2021

Mass Merge Recordings & Set Comments for Recordings for a release have just stopped working for me. They show that they are loaded, but the button for Set Comments for Recordings no longer shows up and when I add a release into the field for remote release box, it never loads them.

@jesus2099
Copy link
Owner Author

It must be because of https://tickets.metabrainz.org/browse/MBS-11703 that may have introduced react-hydrate (page redraw) stuff.

@tigerman325
Copy link

The release comment script is back working. So at least that was short lived.

@jesus2099
Copy link
Owner Author

jesus2099 commented Jun 29, 2021

The release comment script was not updated in 10 month but will hopefully be fixed by its author @mwiencek soon. ;)
As it's himself, bitmap, who changed release page recently in MBS-11703.

Maybe it works for you because it is running enough late (behind lots of other scripts), after react has done its deferred page redraw, or something.

As I don't understand react, I will be able to learn from his fix.
I am in search of a clean stable reliable solution for these react-hydrate-deferred-page-redraws rather than my delays and other tricks.

@jesus2099 jesus2099 changed the title Stopped working (on beta) Stopped working since tracks are displayed with React (MBS-11703) Jun 29, 2021
@jesus2099 jesus2099 removed the blocked label Jun 29, 2021
@jesus2099
Copy link
Owner Author

In fact I found the react hydrate stuff in:

@jesus2099 jesus2099 changed the title Stopped working since tracks are displayed with React (MBS-11703) Stopped working since tracks are displayed with React (MBS-9921) Jun 29, 2021
@mwiencek
Copy link
Contributor

Sorry this causes so much trouble. :\ The main issue I see is that React will remove any elements you inject whenever it re-renders the parent. I'm not sure of the best way to handle this. MutationObserver could work somehow, but might be slow and tedious.

I could also add some callbacks in the MBS components that userscripts could hook into and modify stuff, though this probably still requires some minor knowledge of React. It might be the most reliable option though.

Could you make a list of the different elements on the release page you need to modify/inject into?

@jesus2099
Copy link
Owner Author

Thanks for chiming in, @mwiencek. :)

The main issue I see is that React will remove any elements you inject whenever it re-renders the parent.

Exatcly!

I thought there were some React functions that I could call to tell React this and this had to be kept at next page redraw,
There is no such thing as a kind of react.register(myElements) function?

I don't think MBS code should go as far as to include specific callbacks for user scripts, though.
It would be too much.

If there is no clean react way, I try to make some clean new code based on setInterval or setTimeout functions, like I do for more and more scripts (GitHub user scripts required that, the first).
I remember I tried MutationObserver but there were some problems I don't remember.
I only remember it was simpler and more reliable to use dumb setInterval or setTimeout functions.

@mwiencek
Copy link
Contributor

I thought there were some React functions that I could call to tell React this and this had to be kept at next page redraw,
There is no such thing as a kind of react.register(myElements) function?

Sadly no. setInterval is probably your best bet, then -- if the elements you insert have unique IDs, getElementById will be a very fast way to check if they were removed, allowing a smaller interval.

@jesus2099
Copy link
Owner Author

Thanks! 😁

@jesus2099
Copy link
Owner Author

jesus2099 commented Jul 6, 2021

Hi @mwiencek, did the div.loading-message disappear from loading expand/collapse mediums?
It seems there is now only:

<tr><td colspan="4" style="padding: 1em;">Loading...</td></tr>

Which makes it difficult with multilingual MBS.

Before that, I used to have tr > td > div.loading-message.


Update

Indeed:

Before

metabrainz/musicbrainz-server@34fbf7d#diff-b4fa79da081c94745153af98e38b774364465df255d3dccd9e254b8ec302e11aL60-L63

<tr>
	<td>
		<div class="loading-message">
			{l('Loading...')}
		</div>
	</td>
</tr>

After

metabrainz/musicbrainz-server@34fbf7d#diff-a94cec52e8d4657c88b7e1066cd4276083e045038928cf5c871611392d94fe34R315

<tr>
	<td>
		{l('Load all tracks...')}
	</td>
</tr>

Do you think this kind of stuff could be put back?

@reosarevok
Copy link

I'm pretty sure it could. I'll look into making the change.

@reosarevok
Copy link

metabrainz/musicbrainz-server#2166 - does that look like it would work? (it'd need a small change at least since there's no longer a div, but)

@jesus2099
Copy link
Owner Author

Good release example

To test this out: https://musicbrainz.org/release/16caa039-aba9-45e8-9ff1-8cca8635ff52

@jesus2099 jesus2099 added this to the 2021 milestone Aug 21, 2021
@jesus2099 jesus2099 added the major label Oct 2, 2021
@jesus2099
Copy link
Owner Author

Work amount is also broken, I now see only Tracks: https://musicbrainz.org/release/5ebe09a7-2451-47aa-91eb-9a9ce5973ef3

@jesus2099
Copy link
Owner Author

I will fix this by finally writing a new version that handles all kinds paginated releases (#38), like recently done for COLLECTION HIGHLIGHTER.

It will solve this issue at the same time!

@jesus2099
Copy link
Owner Author

This bug will soon trigger all the time, like on beta: see examples in #682.

@redactedscribe
Copy link

This bug will soon trigger all the time, like on beta: see examples in #682.

Now triggering on non-beta site too.

@jesus2099 jesus2099 changed the title Stopped working since tracks are displayed with React (MBS-9921) Script is visible in the sidebar but nothing appears in the tracklist (ISRC, comments, AcoustID) Jun 13, 2022
@jesus2099 jesus2099 changed the title Script is visible in the sidebar but nothing appears in the tracklist (ISRC, comments, AcoustID) Still visible in the sidebar but nothing appears in the tracklist (ISRC, comments, AcoustID) any more Jun 13, 2022
@ROpdebee
Copy link
Contributor

@jesus2099 wrapping your INLINE STUFF script into a window load event handler seems to be a viable workaround on my end.

window.addEventListener('load', function() {
  // rest of the script
});

I don't think it's truly a reliable workaround, but the React hydration errors disappeared and INLINE STUFF seems to do its job. Alternatively, @run-at document-idle seems to solve it as well.

The reason it's not a reliable workaround is that it's also waiting for all images to finish loading. So, if the Internet Archive/CAA is having a particularly bad day, this workaround could make it so your script will only execute after the sidebar cover art is loaded, which is of course not ideal.

MutationObserver, as recommended by bitmap previously, won't work for INLINE STUFF's hydration errors (nor will it work for my inline track recordings script, ROpdebee/mb-userscripts#484) since there's no client-side rendering going on (unless you trigger the hydration errors, in which case React will fall back to client-side rendering).

What I think could be a decent solution would be a custom event that gets dispatched whenever hydration of a component has finished. Userscripts could then listen for that event and postpone their modifications until it arrives. Ideally the event would bubble and be dispatched on the hydrated component's root, so we can listen to it anywhere in the parent chain but still figure out which element exactly was hydrated. That also wouldn't require any tricky hooks made specifically for userscripts, and no deep React knowledge required for the userscript authors. Similar events dispatched in the client-side rendering hooks (componentDidMount and componentDidUpdate) would be even more amazing, then we could get notified of when userscript elements need to be reinserted instead of having to observe for that. @mwiencek does this sound feasible to you (primarily the hydration part)?

@jesus2099
Copy link
Owner Author

jesus2099 commented Jun 13, 2022

Thank you very much.
Anyway I will surely do like I did for COLLECTION HIGHLIGHTER (setInterval: 463183e).
I need to support all tracks, even those that are loaded later, with all kinds of collapsed/paginated releases.

But it might be good to put most of my scripts in document-idle.

But at the moment I am on several other stuff, that I need more.
And the workaround works for me.
Maybe I will do a quick fix as you suggest, for others.

@jesus2099
Copy link
Owner Author

jesus2099 commented Jun 13, 2022

In my past react-hydrate new hint: custom mb-hydration event 12 fixes "researches," I think it was not possible to wish for such post-hydrate events.
I think I always came to the conclusion that setInterval was actually the best solution, and not so dirty, after all.

jesus2099 added a commit that referenced this issue Jun 13, 2022
Give more time before real fix
@jesus2099 jesus2099 reopened this Jun 13, 2022
@jesus2099
Copy link
Owner Author

@redactedscribe @Alfge, tell me if my quick fix did the trick for you.

@a23bed
Copy link

a23bed commented Jun 14, 2022

last update works fine now. Thanks

@redactedscribe
Copy link

redactedscribe commented Jun 14, 2022

tell me if my quick fix did the trick for you.

Yep, works, thanks (on beta too)!

@Alfge
Copy link

Alfge commented Jun 14, 2022

Tried the "Workaround is to make this script near or at the end of your userscript list."
but doesn't help for me :-(

@jesus2099
Copy link
Owner Author

jesus2099 commented Jun 14, 2022

@Alfge did you update to version 2022.6.14, already?
This is the version with document-idle quick-fix, suggested by @ROpdebee, working for most people.

@mwiencek
Copy link
Contributor

@mwiencek does this sound feasible to you (primarily the hydration part)?

Yes, I think custom events is a good idea.

@Alfge
Copy link

Alfge commented Jun 14, 2022

Just updated to version 2022.6.14 and now it works for me too.
Thank you!

mwiencek added a commit to mwiencek/musicbrainz-server that referenced this issue Jun 14, 2022
Currently it's difficult for userscripts to determine when hydration is
completed so that it's safe to modify the page.  This adds a custom
mb-hydration event that scripts can listen for.

Based on a suggestion by @ROpdebee in
jesus2099/konami-command#596 (comment)
mwiencek added a commit to mwiencek/musicbrainz-server that referenced this issue Jun 14, 2022
Currently it's difficult for userscripts to determine when hydration is
completed so that it's safe to modify the page.  This adds a custom
mb-hydration event that scripts can listen for.

Based on a suggestion by @ROpdebee in
jesus2099/konami-command#596 (comment)
mwiencek added a commit to metabrainz/musicbrainz-server that referenced this issue Jun 14, 2022
Currently it's difficult for userscripts to determine when hydration is
completed so that it's safe to modify the page.  This adds a custom
mb-hydration event that scripts can listen for.

Based on a suggestion by @ROpdebee in
jesus2099/konami-command#596 (comment)
@jesus2099
Copy link
Owner Author

https://musicbrainz.org/release/16caa039-aba9-45e8-9ff1-8cca8635ff52

For this release, still no comments, no ISRC, no AcoustID.

@jesus2099
Copy link
Owner Author

Today, that release recording comments show up (on mobile).
It's very random until I fix.

@jesus2099
Copy link
Owner Author

jesus2099 commented Jul 31, 2023

I added that same old lame react hydrate workaround (delay): e540156?w=1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants