feat: Open all add external IDs links #150
Conversation
|
maybe I missed it, but does this have a buffer of any kind in between link opens? if a user opens all the links for a 40-track release at once, for example, they'll likely violate musicbrainz rate-limiting. as a high volume editor, my number one issue is always MB rate-limiting. |
|
Does MusicBrainz limit the web server requests? I've never encountered that, usually only API is rate-limited. |
I'm not sure whether I agree with this. Stylistically it kinda makes sense, but it has to be semantically clear that it is not a link. Maybe we should at least mention that it will open X new tabs? P.S. I will comment on the rest of the PR once I find a bit more time. |
Here's an example of how that could look 🙂 
Good point. Right now, that has not been added. But I can add some delay between when the tabs are opened. 👍 Update: For reference, I can do something similar like in the userscript linked in the issue and delay ever opened tab by 300ms. for (let i = 0; i < links.length; i++) {
setTimeout(() => {
const newWindow = window.open(links[i].href, '_blank');
if (!newWindow || newWindow.closed || typeof newWindow.closed === 'undefined') {
console.warn("Popup was blocked for link:", links[i].href);
}
}, i * 300);
} |
I think all the requests from your IP are counted in rate limiting. None of the link opens will be dropped or show an obvious problem, but if there is are API request happening elsewhere at the same time, those will start to fail if too many requests are sent via web at the same time. |
|
We can handily preview how it would look as a button, because something very similar is implemented in the "HarmonyOpenAllRecordings" userscript: 
I honestly don't think I have a preference. The text saying how many tabs will be opened seems the most important. A also did a lazy preview of a h3 header + button: 
I don't really like the button next to the header, but a header could be useful if this section keeps getting busier. This is quite a long way to say that I don't really mind the current implementation in this ticket :D |
|
I want to iterate on Aerozol's stylistic suggestion and recommend to dim the blue colour of the button at least by half (it's visually distracting), and also maybe to make it shorter the text could say Thank you for the great work and can't wait for this to get merged! And yes, to double kellnerd's point, a button is a button, we shouldn't make it pretend to be a link (because some old-school users like myself will click on it using a mouse-wheel button and wil be frustrated that nothing is happening 😁). |
|
Alright, there are some different opinions. It seems a visual button is the most popular option. 🙂
I get your point. Buuut... We could also just bump Preact to get Another thought, it could be an idea to show a confirmation box before opening a lot of tabs. Taking inspiration from 
And here's a new design with the buttons looking like buttons. I've also implemented the delay between the tabs opening. The color of the buttons are a bit darker. First I looked at using the link color or label color, but looking at the accessibility stats then the contrast was too low. So, making them darker improved that. They are however based on the link color, just darker. And I kept the icon, either way I'd like to keep the button and the links aligned. I've kept things in separate commits for now if we want to go back. But if things looks good, I can squash them. 
Update (2025-10-08): I committed the unit test I created (as mentioned in the description) and used dependency injection of |
|
Thank you @gustavkj, I'm happy with the design of it :) |
There was a problem hiding this comment.
Thank you for the improvements and your patience.
And sorry for the initial rant in advance, once I've got that out of the way I mostly have comments about code style rather than functionality 😊
I find it extremely annoying to open multiple recording edit tabs at once, no matter if it is done by hand or via the button, with or without delays.
Without delays, there are occasionally tabs with repeatedly failing ws/js requests, which is a rate-limit problem (as well as an MB server bug IMO).
And with delays it takes ages for dozens of tabs to open, with each new tab stealing the focus from the previous one, making it impossible to use the browser until the last tab has been opened (at least in my Firefox configuration).
I really hope that there will be better support in MB for this kind of editing in the future, but until then we should try to find the least bad solution.
This is probably the version with the delays, which at least doesn't hammer the MB servers too much.
One thing I am unsure about is the additional confirmation dialog if there are 10+ links. It will quickly annoy regular users with one unnecessary extra click.
Maybe the button tooltip/title is sufficient as a warning? In case we want to make this warning more prominent, we could have the button label say "Open all ... links in new tabs". Or additional inline text in brackets after the button, as in aerozol's mockup.
And a huge thank you for adding a few test cases for the MB edit link generation, these are really appreciated. IIRC we already had a regression in this area once which was only caught by a user.
| import { flatten } from 'utils/object/flatten.js'; | ||
| import { type EntityWithUrlRels, getMusicBrainzEditLink } from '@/utils/mbLink.ts'; | ||
| import type { ResolvableEntity } from '@/harmonizer/types.ts'; | ||
| import { OpenAllLinks } from '@/server/islands/OpenAllLinks.tsx'; |
There was a problem hiding this comment.
Can you group the island import with the other component imports at the top? I would like to keep JSX/TSX imports separated from the rest.
| entities: ResolvableEntity[]; | ||
| entityType: EntityType; | ||
| sourceEntityUrl: URL; | ||
| sourceEntityUrl?: URL; |
There was a problem hiding this comment.
| sourceEntityUrl?: URL; | |
| sourceEntityUrl: URL; |
I don't want to make a required property optional just because it makes it easier to handle in TS, even if that requires a bit more code where the component is used.
| const entitiesWithMbEditLinks = entities.map((entity) => ({ | ||
| entity, | ||
| mbEditLink: getMusicBrainzEditLink({ entity, entityType, sourceEntityUrl, entityCache, providers }), | ||
| })) | ||
| .filter(isEntityWithMbEditLink); |
There was a problem hiding this comment.
We can avoid having to define the one-off type EntityWithMbEditLink and its type guard function:
| const entitiesWithMbEditLinks = entities.map((entity) => ({ | |
| entity, | |
| mbEditLink: getMusicBrainzEditLink({ entity, entityType, sourceEntityUrl, entityCache, providers }), | |
| })) | |
| .filter(isEntityWithMbEditLink); | |
| const entitiesWithMbEditLinks = entities.map((entity) => { | |
| const mbEditLink = getMusicBrainzEditLink({ entity, entityType, sourceEntityUrl, entityCache, providers }); | |
| if (mbEditLink) { | |
| return { entity, mbEditLink }; | |
| } | |
| }).filter(isDefined); |
| ); | ||
| } | ||
|
|
||
| function LinkWithMusicBrainzEntry({ mbEditLink, entity, entityType }: { |
There was a problem hiding this comment.
| function LinkWithMusicBrainzEntry({ mbEditLink, entity, entityType }: { | |
| function LinkWithMusicBrainzAction({ mbEditLink, entity, entityType }: { |
P.S. Continuing this thought, we could probably also introduce a new CSS class action which shares the same styles as a message? I was just lazy when I initially designed the release actions page 😅
Alternatively we could keep message and have an additional semantic class action to aid userscripts/userstyles.
| return ( | ||
| <div class={classList(hasMultipleEntities && 'message-group')}> | ||
| {hasMultipleEntities && ( | ||
| <OpenAllLinks | ||
| mbEditLinks={entitiesWithMbEditLinks.map(({ mbEditLink }) => mbEditLink.href)} | ||
| entityType={entityType} | ||
| /> | ||
| )} | ||
| {entitiesWithMbEditLinks.map(({ entity, mbEditLink }) => ( | ||
| <LinkWithMusicBrainzEntry | ||
| mbEditLink={mbEditLink} | ||
| entity={entity} | ||
| entityType={entityType} | ||
| /> | ||
| ))} | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
I only wanted to omit the unnecessary wrapper div for a single link.... which forced me to refactor the entire thing:
| return ( | |
| <div class={classList(hasMultipleEntities && 'message-group')}> | |
| {hasMultipleEntities && ( | |
| <OpenAllLinks | |
| mbEditLinks={entitiesWithMbEditLinks.map(({ mbEditLink }) => mbEditLink.href)} | |
| entityType={entityType} | |
| /> | |
| )} | |
| {entitiesWithMbEditLinks.map(({ entity, mbEditLink }) => ( | |
| <LinkWithMusicBrainzEntry | |
| mbEditLink={mbEditLink} | |
| entity={entity} | |
| entityType={entityType} | |
| /> | |
| ))} | |
| </div> | |
| ); | |
| const actions = entitiesWithMbEditLinks.map(({ entity, mbEditLink }) => ( | |
| <LinkWithMusicBrainzAction | |
| mbEditLink={mbEditLink} | |
| entity={entity} | |
| entityType={entityType} | |
| /> | |
| )); | |
| if (actions.length > 1) { | |
| return ( | |
| <div class='message-group'> | |
| <OpenAllLinks | |
| mbEditLinks={entitiesWithMbEditLinks.map(({ mbEditLink }) => mbEditLink.href)} | |
| entityType={entityType} | |
| /> | |
| {actions} | |
| </div> | |
| ); | |
| } else { | |
| return actions[0]; | |
| } |
| export function OpenAllLinks({ mbEditLinks, entityType }: { | ||
| mbEditLinks: string[]; | ||
| entityType: EntityType; |
There was a problem hiding this comment.
I wonder whether these properties should have more generic names to make reuse of this component more intuitive:
| export function OpenAllLinks({ mbEditLinks, entityType }: { | |
| mbEditLinks: string[]; | |
| entityType: EntityType; | |
| export function OpenAllLinks({ links, entityType }: { | |
| links: string[]; | |
| entityType: EntityType; |
Maybe even rename entityType to linkType, then we could simply relax its type to string if needed in the future.
server/routes/release/actions.tsx
Outdated
| <LinkWithMusicBrainz | ||
| entities={release.labels} | ||
| entityType='label' | ||
| sourceEntityUrl={releaseUrl!} |
There was a problem hiding this comment.
This type assertion is safe because release is only defined when releaseUrl is defined as well.
There was a problem hiding this comment.
Please move this module (and its tests) to musicbrainz/edit_link.ts where other MB-specific code lives. utils is just my dump folder for everything which currently doesn't fit elsewhere.
| }>; | ||
| } | ||
|
|
||
| export function getMusicBrainzEditLink( |
There was a problem hiding this comment.
This function needs a description and/or a better name, since it generates an MB edit link to add external linls specifically. Maybe something like getEditUrlToAddExternalLinks or constructUrlToSeedExternalLinks?
| button.open-all-links { | ||
| display: inline-flex; | ||
| align-items: center; | ||
| padding: 0.4em; | ||
| border-radius: 0.2em; | ||
| font-size: 0.8em; | ||
| line-height: 0.8em; | ||
| white-space: nowrap; | ||
| background-color: var(--button-accent-fill); | ||
| border: none; | ||
| color: var(--button-accent-text); | ||
| font-size: inherit; | ||
| font-family: inherit; | ||
| } | ||
|
|
||
| button.open-all-links:focus, button.open-all-links:hover { | ||
| background-color: var(--button-accent-focus); | ||
| cursor: pointer; | ||
| } |
There was a problem hiding this comment.
Some of these properties seem to be leftovers from restyling links as buttons or have no visible effect and can be removed: cursor, display, align-items, white-space
(font-size is defined twice, the first one should be removed.)
Other styles such as font settings and maybe border could probably applied to buttons in general.
Existing buttons already have a different border-radius, for example.
P.S. I always appreciate UI design advice, the current button style was the best thing I could come up within a reasonable amount of time.
|
Thanks for the feedback! I'm a bit short on time right now, but I'll take a closer look towards next weekend. 😊 |
I have two implementations to deal with this, one more complicated and one more simple. Without my "MB Link Opener" running, I have a toast that pops up and says "You're about to open N links in batches of Y" and i can cancel that if it's too many. I do batches of 10 and open with 400ms in between each link and then 2000ms in between batches. Yes that is slower but it is more stable. Any link open is backgrounded so I can focus on the work I'm doing. When I have my "link opener" userscript running, it controls the buffering by looking at in-flight tabs, last link opened and next link needed and it queues most of my other activities to try to reduce hammering MB. I no longer use the batching or pause between batches and instead try to use states to add more control. This is also more effective because I can use dynamic timing based on certain conditions, e.g. more aggressive opening when I can save a call by checking my MB mirror instead of checking the production server. I'm currently looking at a node-red implementation that would be better but that's obviously more architecture than simple javascript. |
|
I've seen a similar implementation on one of the forum sites I'm using where they allow opening all unread notifications in new tabs at once (they limit it by 20 tabs at once). I've never encountered any problem with it or a delay. So I just went there and reverse engineered how they are doing it. They use a zero timeout for the first tab and then for every new tab the timeout increases a bit, so it's not constant. Also, in order to avoid focus stealing, we can mimic the Ctrl+Click behavior, which would open the tabs in the background. Anyway, do we have any preview environment? I'd like to quickly give it a test in my browser. |
@ligeia Where can we find that userscript and is it possible to reproduce more sophisticated tab/window management functionality in a regular website? As far as I am aware that only works with browser extensions (userscript extensions make use of the same browser APIs).
@arsinclair Do you have reference code or any other link? Currently we have such a timeout loop, but we use the
Nope, there is no public server running a preview version of Harmony that I'm aware of. |
Unfortunately, I cannot share it, since it's a private forum. But I can explain how it works. They first grab the actual links from the page using jQuery based on a certain class name, that is characteristic to only those links. Then in a loop they set the target attribute of the link to It looks like this: 
Regarding Ctrl+Click I found this: https://stackoverflow.com/questions/10812628/open-a-new-tab-in-the-background
Okay, noted. |
There is SOME control we can do in browser but, e.g., methods in Firefox and Chrome are different. Yes, most of the control with that script right now depends on GM-related functions. I will see if I can make some suggestions but the API for Harmony that I'm working on is a higher priority for me at the moment. |
5 participants
Closes: #96
Adds a button to open all links to add external IDs for artists, labels, and recordings. Since opening multiple tabs/windows is blocked by default in most browsers, a warning is shown if the tabs were blocked informing the user that they need to allow pop-ups for the site.
The button to open all links is only shown when more than one link is available. Some margin around the group of links is added then there are multiple links and the button is shown.
While I've styled the button as a link, it is semantically a button. But at least in from my point of view, I think it fits in better by following the same format as all the other sections on the release actions page.
Side note: I wrote a test for
getMusicBrainzEditLink(which is stashed locally for now) but it is failing due to write permission always being required for thesnaps.dbfile. It would be possible to refactor the function further and use dependency injection to get around this. But thought I'd bring it up here before doing so. Thought on that, or suggestions around mocking/stubbing in Deno test?Side note 2: I'm new to the islands architecture and Fresh/Preact, so feel free to give feedback if there are anything that should be done differently. Initially, I ran into an issue related to this where I was trying to send the URL objects to the island which did not work since it is not serializable.
Button is available for both artist and recording links (as well as for labels) (
/release/actions?release_mbid=53bbe568-4b33-4e93-b93c-f3adc4e15b2d)Message informing the user after having clicked the button if the browser blocked tabs from opening
No change when there is only one link (
/release/actions?release_mbid=7b49ac48-6c32-4831-a7dd-aba21b075299)