Use release title for release page master search - #1054
Conversation
Instead searching for link title: “See all versions”
|
I don't know why this auto test is failing, as |
|
It's failing because of the missing formatting. You can run I haven't looked at the code yet. |
|
I typed |
I'm honestly not sure about this error. It seems this might be happening if your NodeJS version is below 24. Anyway, you can just run the formatter script and it should normalise everything. |
Also I don't know if this command is going to work, but if pnpm is installed on your system, there's no need to run it with |
arsinclair
left a comment
There was a problem hiding this comment.
I ran an AI code checker against the changes here and manually validated all the findings.
-
all MB links disappear when the Discogs API fails
insertMBLinks()now runs only frominsertMBSection()after a successful API request (discogs_importer.user.js:74, 99–101, 603–605). A rate limit, network error, API outage, or parsing exception therefore prevents every MusicBrainz link from appearing on release pages—not just the release-group search link. Previously these links were independent of the import API request. -
arbitrary release titles aren’t URL/HTML encoded
Lines 313–324 now pass the API title intoMBImport.searchUrlFor(), which applies Lucene escaping but notencodeURIComponent(). For example,Rock & Rollproduces:?query=Rock & Roll&type=release_group&indexed=1The browser interprets the query as
Rockplus an extraRollparameter. Double quotes in a title can also corrupt the interpolated HTML attribute.
A safer approach is to retain the original early insertMBLinks() call, then update only the sidebar search link’s href once the release title arrives. The URL should be constructed using encoded query parameters or assigned through the DOM rather than HTML interpolation.
|
It may be a pre-existing bug because I don't think I don't have much time, at the moment. Let's keep this aside for a while... IMO, 1 is not that important, and the MB links should be better fixed anyway, as they regularly don't appear, because of some late Discogs DOM changes, already. Similar to how MBS React hydrate redraws DOM and wipes out all userscripts. The best fix for 1, that I initially tried to add was: Because the RG search for See all versions doesn't make sense and is of no use, even temporarily. But the code is not mine and I don't get it totally (I'm not used to jQuery and such). I had not enough time to spend on this, and went to a simpler change that fixed it for me. I didn't think those MB links could still be useful when the import is failing (API down). But I guess you're right. |
|
I'm marking this as draft. I'll keep this version for my own usage, until I have time to write a better version for this repo. Or until someone else does it, I don't mind. |
|
If you don't have time, I can probably fix them some time before the end of the year. The requested changes are not that big of a deal, but also I don't work with the discogs script that much and its logic is already too convoluted, so it will take me a while to wrap my head around it. Regarding problem #2 it seems it is a tiny fix, we already have a function that correctly escapes unescaped characters for lucene. Regarding #1: if MB API is responding with errors (which happens every day now btw) it still may be helpful if the script renders the icons that are cached in the local storage, but with this change they would disappear.
I agree, I saw that bug and never really got time to pin it down. It should definitely be fixed too, as it's very annoying. The author tag is not filled here, but from the commit history alone I can see that @murdos is the author of the script, so he might be better equipped reviewing this and evaluating whether the problems outlined are actually important enough, or can be fixed later on their own. |
|
I wrote this script a long time ago (more than 15 years), and I don't use it since more that 10 years, so I really don't have any idea. |
Current userscript searches for link title: “See all versions”.
I made this change, so that the master link MB release group search uses current Discogs release page title, instead.
I have ran:
With no errors, before submitting this merge request (PR).