MBS-11889, MBS-11521: External links editor refactor #3621

mwiencek · 2025-08-28T04:04:20Z

This was the very first React component written for MBS, all the way back in 2015, and requires significant refactoring in order to follow the Rules of React [1] and work properly with React v19 and new tools like react-compiler.

Since this effort was blocking the React v19 upgrade, I also included that upgrade in this PR to make sure the tests actually pass with React v19. (They did when I tried locally; no more bizarre "Maximum update depth exceeded" errors.)

Fixes MBS-11889
Fixes MBS-11521

[1] https://react.dev/reference/rules

eslint.config.mjs

reosarevok

Leaving some comments for now, but still I need to keep reviewing the main refactor commit and actually test this locally.

root/static/scripts/external-links-editor/components/ExternalLinksEditor.js

reosarevok · 2025-08-28T13:53:56Z

root/release/EditRelationships.js

+    release != null &&
+    release.entityType === 'release' &&
+    !release.isNewEntity,
+  );


Why not use getSourceEntityData for this one?

I pushed some more changes to use getSourceEntityData here. But the previous reason is that this is a server component, and getSourceEntityData used to only work in the browser (because it used to call getCatalystContext, which doesn't work on the server).

reosarevok · 2025-08-28T14:04:02Z

root/static/scripts/edit/URLCleanup.js

        result.push(type[sourceType]);
      }
-      return result.sort();
+      return result;


I think I probably added this because of hitting some test failure? But maybe I'm being an idiot.

That's what the original commit implied, but they did seem to pass fine without it locally. If they fail on CI I'll add it back (but ideally sorting once at the end rather than in a loop).

I couldn’t find the exact reason for this sort call, but here is another hint about it: #2339 (comment)

reosarevok · 2025-08-28T14:20:16Z

root/static/scripts/edit/URLCleanup.js

-          [...allowedType].sort(),
-        ),
+        selectedTypes.length === allowedType.length &&
+        (new Set(selectedTypes)).isSubsetOf(new Set(allowedType)),


I guess there's no "is equal" set property that could be used - "is equally long and a subset of" is a funny way of writing it, but probably not worse than set1.symmetricDifference(set2).size === 0 anyway

Agreed, I'm not sure why there's no builtin method for it. The advantage of isSubsetOf over symmetricDifference is just that it doesn't require constructing a new set. If we find more uses for it, we could just define our own utility.

root/release/edit/information.tt

root/static/scripts/external-links-editor/utility/cleanupUrl.js

mwiencek · 2025-10-22T14:15:56Z

Rebased on master (no changes besides fixing conflicts).

yvanzo

Reviewed up to the commit edff139 for now. It requires rebasing to resolve conflicts.

eslint.config.mjs

root/static/scripts/edit/URLCleanup.js

root/static/scripts/common/utility/catalyst.js

yvanzo · 2025-12-17T15:59:23Z

root/static/scripts/edit/URLCleanup.js

        result.push(type[sourceType]);
      }
-      return result.sort();
+      return result;


I couldn’t find the exact reason for this sort call, but here is another hint about it: #2339 (comment)

yvanzo

Reviewed up to the commit b07e166 for now. Previous comments remain unanswered.

root/static/scripts/event/components/EventEditForm.js

root/static/scripts/edit/MB/edit.js

root/types/releaseeditor.js

reosarevok

Only got to ExternalLinks.js in the big commit, but had a few comments for now.

root/static/scripts/edit/MB/edit.js

root/static/scripts/external-links-editor/state.js

root/static/scripts/external-links-editor/validation.js

root/static/scripts/event/components/EventEditForm.js

root/static/scripts/external-links-editor/components/ExternalLink.js

mwiencek · 2026-02-03T21:18:46Z

I had to force-push twice because I rebased an old version of the branch the first time. The changes in the second push aren't actually new.

yvanzo

I reviewed answers and each commit but the big commit mentioned by @reosarevok as it should probably be more detailed or further split.

reosarevok

A few comments today about ExternalLinkRelationship and related functions only.

root/static/scripts/external-links-editor/validation.js

root/static/scripts/external-links-editor/components/ExternalLinkRelationship.js

root/static/scripts/external-links-editor/validation.js

root/static/scripts/external-links-editor/components/ExternalLinkRelationship.js

reosarevok

A few more comments, will continue with URLInputPopover tomorrow.

root/static/scripts/external-links-editor/components/ExternalLinksEditor.js

reosarevok · 2026-02-09T16:20:58Z

root/static/scripts/external-links-editor/components/ExternalLinksEditor.js

  let hiddenInputsContainer = document.getElementById(
    'external-links-editor-submission',


When does this already exist? Is it if submission was attempted and failed?

It shouldn't exist, since the submissionInProgress check should now guarantee this only runs once, though that didn't exist in the previous code. I'll add a comment, though I think it's fine to leave prepareExternalLinksHtmlFormSubmission how it is.

reosarevok · 2026-02-09T16:28:01Z

root/static/scripts/external-links-editor/components/ExternalLinksEditor.js

+      if (state.links.size && hasSessionStorage) {
+        sessionStorageWrapper.set(
+          getSubmittedLinksKey(source),
+          JSON.stringify(compactEntityJson(state.links)),
+        );


This is what keeps the state in case of an error, right? In which case do we not hasSessionStorage, and what happens on error in that case?

Yes, if sessionStorage isn't available, then no links (and no changes to existing links) will be restored.

The hasSessionStorage export references https://bugzilla.mozilla.org/show_bug.cgi?id=365772 in a comment, which is now fixed. You can also toggle dom.storage.enabled in Firefox, though I don't think it's common for anybody to do that.

Should we then drop that comment? So, basically, it should mostly always be available nowadays unless someone actively went out of their way to make it not available, right? Guess that's fine then.

The comment is still useful for explaining why the code exists, but it could be updated if someone wants to research all of the current scenarios in which those statements could throw (I'm not volunteering for this at the moment :)).

reosarevok · 2026-02-09T16:36:35Z

root/static/scripts/external-links-editor/components/ExternalLinksEditor.js

+    if (elementToFocus) {
+      elementToFocus.focus();
+    }
+    dispatch({focus: '', type: 'set-focus'});


Does this then cause the same function to run again, but return immediately because of !selector?

Correct, it will cause the component to re-render, and any effects whose dependencies have changed will run again.

Is that actually wanted/useful in any way, or just a side-effect of us storing focus in state like this? (not that I mind if that's the most effective way otherwise, I was just a bit surprised because AFAICT we don't do focus in a similar way elsewhere).

It's not desirable for the component to re-render in this case, but not a big deal either. Being able to sync focus changes with specific state changes is a lot more useful than not.

root/static/scripts/external-links-editor/components/ExternalLinksEditor.js

reosarevok · 2026-02-09T16:44:36Z

root/static/scripts/external-links-editor/components/ExternalLinksEditor.js

+    }
+  }, [links]);
+
+  const submissionInProgress = React.useRef(false);


useRef here is so that we don't try to recalculate this in future renderings, and it stays false or true as pre-set, right? (I'm more used to refs used for passing around)

Yes, false is just the initial value. Whatever value you set is preserved between renders. That's only used to store data that doesn't affect the component return value. In this case it just prevents prepareExternalLinksHtmlFormSubmission from running multiple times.

reosarevok · 2026-02-09T16:48:18Z

root/static/scripts/external-links-editor/components/ExternalLinksEditor.js

+  }, [state]);
+
+  const linkElements = [];
+  let linkIndex = 0;


While here we don't useRef because we want it to start from 0 in every re-render rather than from whatever ++linkIndex below left it as, right?

Correct, linkIndex is being used to calculate isLastLink, so it has to start from 0.

root/static/scripts/external-links-editor/components/ExternalLinksEditorFieldset.js

reosarevok

Now checked up to state.js (rechecked some previous bits that have changed too).

root/static/scripts/external-links-editor/components/ExternalLinkRelationship.js

reosarevok · 2026-02-10T09:47:53Z

root/static/scripts/external-links-editor/components/ExternalLinkRelationship.js

+         * If there are no applicable types, show types that aren't
+         * restricted to specific sites.


If there are no applicable autoselected types, show all types that aren't restricted to autoselected sites only?

Don't we only say autoselected when there's only one possible type? For example, Apple Music links are type restricted but not autoselected.

I guess that's fair. My main concern was that we could specify that "applicable" here means "guessed / restricted", so maybe If there are no restricted types for this URL, show all types that aren't restricted by URL?

Updated the comment

root/static/scripts/external-links-editor/components/ExternalLinkRelationship.js

reosarevok · 2026-02-10T10:28:04Z

root/static/scripts/external-links-editor/components/ExternalLinkRelationship.js

          />
-        ) : null}
+          ) : null}
        <ExternalLinkAttributeDialog


I do wonder if this should be renamed - the attribute "video" is edited outside of the dialog, and entity credits are not attributes. Why not URLRelationshipPopover, or ExternalLinkRelationshipPopover? Since it's a popover and we use URLInputPopover for the other one.

Renamed to ExternalLinkRelationshipPopover

root/static/scripts/external-links-editor/state.js

reosarevok · 2026-02-10T14:07:00Z

root/static/scripts/external-links-editor/state.js

+        link,
+        (linkCtx) => {
+          const relationshipsCtx = linkCtx.get('relationships');
+          // Move focus to the next <select>.


This only moves the focus if it's a new rel, though, right? So it should be I guess:

// If a new relationship, drop it and move focus to the next <select>

And then below over toggleRemoveMatchingRelationships something like // If preexisting, toggle its removed value

If preexisting, toggle its removed value

That code is also run for new relationships (in which case toggleRemoveMatchingRelationships deletes the relationship).

Reworded the first comment

reosarevok · 2026-02-10T14:11:39Z

root/static/scripts/external-links-editor/state.js

+          ctx,
+          link,
+          linkCtx => {
+            const cleanUrl = linkCtx.read().url.trim();


Calling this cleanUrl is a bit confusing since it hasn't gone through a cleanup, right? Why not trimmedUrl?

url is always cleaned-up, so cleanUrl makes sense to me. I'm not sure why the trim is needed (it should have been trimmed in handle-url-change), but might have been copied from the previous code somehow.

Oh, so this is not a raw URL but the cleaned one. I was confused because we set it as rawUrl below, but that's I guess making the clean URL the new raw URL. I'm still not sure if we want it to remember the originally entered URL until the edit is submitted or not. I think it would make sense, but I guess both options have their benefits (as I commented elsewhere, rn we do it one way for the popover and the other for the main editor, which does not seem intentional).

I am also not sure why we would need to trim it then.

reosarevok · 2026-02-10T14:14:05Z

root/static/scripts/external-links-editor/state.js

+      updateLink(ctx, link, linkCtx => {
+        const pendingLink = linkCtx.read().urlPopoverLinkState ?? link;
+        const updatedPendingLink = mutate(pendingLink)
+          .set('rawUrl', rawUrl)


I'm confused by one thing here when testing. When I paste https://open.spotify.com/artist/1m9WPOccw8sizsVYUhSVjZ?333 (so, a link with whatever random param), it gets cleaned up automatically and the rawUrl is set to the cleaned up version (the parameter is dropped). But if I edit the link manually from the popover, and change it to https://open.spotify.com/artist/1m9WPOccw8sizsVYUhSVjZ?333 there, it still shows cleaned up, but the rawUrl does include the parameter. Is this intentional? I'm not sure which option is best but I'd kinda expect both to work the same way.

At the very least, we should probably trim it?

That seems like a bug (a benign one at least). I changed accept-url-input-popover to update the rawUrl too.

reosarevok

Looked through the last bits, still need to test it properly locally.

reosarevok · 2026-02-11T09:15:27Z

root/static/scripts/external-links-editor/types.js

+   *
+   * If the relationship has no changes, then `x.originalState === x`.
+   * If this is a new relationship, `originalState` is `null`.
+   */


Maybe a stupid question, but how is x.originalState === x if x contains originalState but x.originalState does not? From what I can tell in state.js, during initialization we do linkRelationship.originalState = linkRelationship which I expected means the originalState is equivalent to linkRelationship before this assignment?

Maybe a stupid question, but how is x.originalState === x if x contains originalState but x.originalState does not?

It's a circular reference, so x.originalState.originalState === x is also true.

From what I can tell in state.js, during initialization we do linkRelationship.originalState = linkRelationship which I expected means the originalState is equivalent to linkRelationship before this assignment?

Before the assignment, originalState is null (not sure if I got what you meant here).

I did not realize that it was a reference in that way. Ok, I guess it works then (I expect it's not actually storing the data recursively and it's reference magic).

Right, we don't make a copy of the object or anything, we just assign a reference to itself on an own property.

root/static/scripts/external-links-editor/types.js

root/static/scripts/external-links-editor/validation.js

reosarevok · 2026-02-11T10:35:47Z

root/static/scripts/series/edit.js

 import ko from 'knockout';

 import '../common/entity.js';
+import '../external-links-editor/components/StandaloneExternalLinksEditor.js';


For clarification, this needs to be added for series but not other entities because series uses SeriesRelationshipEditor.js, right? (I expect that's something we expect to consolidate eventually since that file still uses jquery).

Indirectly I think, since this is normally included via root/forms/relationship-editor.tt -> relationship-editor.js.

reosarevok · 2026-02-11T13:38:21Z

root/static/scripts/external-links-editor/components/ExternalLinkRelationship.js

+         * If there are no applicable types, show types that aren't
+         * restricted to specific sites.
+         */
+        : !RESTRICTED_LINK_TYPES.includes(option.data.gid)


While testing I noticed that this still shows for example "cover art" for releases, but that immediately errors with "This relationship type is deprecated and should not be used." - can we add an extra check to skip deprecated types?

It filters out deprecated types now.

This causes the deprecated type test in t/selenium/External_Links_Editor.json5 to fail since it's no longer selectable. Do you think I should remove that test entirely?

It's only used by the external links editor.

https://flow.org/en/docs/react/component-syntax/ Diff best viewed with `-w`.

https://github.com/mwiencek/weight-balanced-tree/blob/master/CHANGELOG.md

* Merges `getSourceEntityDataForRelationshipEditor` into `getSourceEntityData`. * Caches the return value by `$c`, so that the same entity object is returned no matter how many times it's called per request. This is important for component/hook memoization (`React.memo`/`useMemo`). * Requires passing `$c` to `getSourceEntityData`. In some cases we already have a `$c` from the React context, which is better to rely on than the window global. See the comment above `getCatalystContext`. This also allows for passing a `CatalystContextT` on the server, where `getCatalystContext` doesn't work at all. * Allows passing an optional `entityType` parameter after `$c` which refines the expected entity type for you. For example, passing 'release' will return a `ReleaseT` rather than a generic `RelatableEntityT`.

Returning `object | false` is a bit confusing. JavaScript already has a type for indicating no value: null.

Luckily, these arrays were very small. It was added for the tests in e1151dc, but they seem to pass fine without it.

All this did was call `filterApplicableTypes` and return null if the list was empty. We can just cache the possible types on the checker object to begin with.

`EntityTypes` sounds like a list of entity types, but based on the usage of `EntityTypesMap`, it's actually relationship types associated with an entity. We already have a type alias named `RelationshipTypeT` for that.

https://react.dev/reference/react/useReducer#avoiding-recreating-the-initial-state The previous code invokes `createInitialState` on every single render.

This used to be used in the knockout-based relationship editor a long time ago. Nowadays it's only used to generate external link edits in the release editor. The single call to `edit.relationshipEdit` that remains doesn't even pass the third `relationship` parameter. You can deduce that this code does nothing without a `relationship`, other than make an identical copy of `args.attributes`.

Tried my best to derive these from lib/MusicBrainz/Server/Controller/ReleaseEditor.pm.

Prior to this commit, seeded release data was embedded in a script tag which invoked `releaseEditor.init`. However, this creates a dependency where the external links editor can't be hydrated until after the release editor is initialized. This commit removes that dependency by allowing seeded URLs to be pulled from the stash freely.

This is a leftover from when the release editor used to load the initial release data asynchronously. Nowadays, `releaseEditor.loadRelease` is only used to load releases from the duplicates tab. A callback is always specified there, so a fallback isn't needed.

…ntity` property `source_entity` is currently undef here on /release/add. The `isNewEntity` flag is used by `getSourceEntityData` in root/static/scripts/common/utility/catalyst.js to create a new placeholder object using one of the functions from root/static/scripts/common/entity2.js. The release editor doesn't require this yet, but the new external links editor code will, as it calls `getSourceEntityData`, which expects `source_entity` to be defined.

`FormRowText` isn't memoized (currently at least), but UrlRelationshipCreditFieldset shouldn't be concerned with that.

mwiencek · 2026-02-12T19:06:36Z

I reviewed answers and each commit but the big commit mentioned by @reosarevok as it should probably be more detailed or further split.

I split another couple of small commits from it, though I mostly spent time adding comments to the type definitions in root/static/scripts/external-links-editor/types.js.

This was the very first React component written for MBS, all the way back in 2015, and requires significant refactoring in order to follow the Rules of React [1] and work properly with React v19 and new tools like react-compiler. Fixes MBS-11889 Fixes MBS-11521 [1] https://react.dev/reference/rules

…-links-editor/

https://react.dev/blog/2024/04/25/react-19 https://react.dev/blog/2024/04/25/react-19-upgrade-guide It seems that a new version of the react-dom libdefs have not been published to flow-typed yet. I simply moved the existing v18 file and removed all removed functions. I didn't add any of the new functions (we don't use them yet).

mwiencek · 2026-02-12T19:36:33Z

Bit of a convoluted issue, but [...]

I've changed it so that the type removal buttons are shown if there's an error, which I think should resolve this generally.

mwiencek commented Aug 28, 2025

View reviewed changes

eslint.config.mjs Outdated Show resolved Hide resolved

reosarevok reviewed Aug 28, 2025

View reviewed changes

mwiencek force-pushed the external-links-editor-refactor branch 3 times, most recently from 3ebeda6 to 0bec1f3 Compare August 29, 2025 02:30

mwiencek mentioned this pull request Aug 29, 2025

MBS-13716: Upgrade React to v19 #3345

Closed

mwiencek force-pushed the external-links-editor-refactor branch 2 times, most recently from 573e899 to ae0ba12 Compare September 2, 2025 16:33

mwiencek force-pushed the external-links-editor-refactor branch 3 times, most recently from ddcc760 to 78f0175 Compare October 8, 2025 14:12

mwiencek changed the title ~~External links editor refactor~~ MBS-11889, MBS-11521: External links editor refactor Oct 22, 2025

mwiencek force-pushed the external-links-editor-refactor branch from 78f0175 to 4e2bd7a Compare October 22, 2025 14:15

yvanzo reviewed Dec 17, 2025

View reviewed changes

yvanzo reviewed Feb 3, 2026

View reviewed changes

root/static/scripts/event/components/EventEditForm.js Show resolved Hide resolved

root/static/scripts/edit/MB/edit.js Show resolved Hide resolved

root/types/releaseeditor.js Show resolved Hide resolved

reosarevok reviewed Feb 3, 2026

View reviewed changes

mwiencek force-pushed the external-links-editor-refactor branch 2 times, most recently from 54c27c4 to 28e1089 Compare February 3, 2026 21:13

yvanzo reviewed Feb 4, 2026

View reviewed changes

reosarevok added React PRs directly related with React conversion Refactoring Refactoring-only PRs (eslint fixes etc) labels Feb 6, 2026

reosarevok reviewed Feb 6, 2026

View reviewed changes

reosarevok reviewed Feb 9, 2026

View reviewed changes

mwiencek force-pushed the external-links-editor-refactor branch 2 times, most recently from c5b8938 to a02829b Compare February 9, 2026 23:35

reosarevok reviewed Feb 10, 2026

View reviewed changes

reosarevok reviewed Feb 11, 2026

View reviewed changes

mwiencek added 21 commits February 12, 2026 13:02

Move URLInputPopover from edit/ to external-links-editor/

1da517a

It's only used by the external links editor.

Move ExternalLinkAttributeDialog and UrlRelationshipCreditFieldset

94b370c

Convert ExternalLink.js to Flow component syntax

e56028a

https://flow.org/en/docs/react/component-syntax/ Diff best viewed with `-w`.

Convert ExternalLinkRelationship.js to Flow component syntax

32015ee

https://flow.org/en/docs/react/component-syntax/ Diff best viewed with `-w`.

Move getFormData out of the ExternalLinksEditor class

527f703

Update weight-balanced-tree to v0.16.0

735741d

https://github.com/mwiencek/weight-balanced-tree/blob/master/CHANGELOG.md

Clarify some refinements in URLCleanup.js

ee826a9

Change weird boolean types in URLCleanup.js

29d5acb

Returning `object | false` is a bit confusing. JavaScript already has a type for indicating no value: null.

Remove repeated sorting from within a loop in filterApplicableTypes

d408c0b

Luckily, these arrays were very small. It was added for the tests in e1151dc, but they seem to pass fine without it.

Remove getPossibleTypes

3ed759e

All this did was call `filterApplicableTypes` and return null if the list was empty. We can just cache the possible types on the checker object to begin with.

Remove confusing/redundant type

4f9e3c4

`EntityTypes` sounds like a list of entity types, but based on the usage of `EntityTypesMap`, it's actually relationship types associated with an entity. We already have a type alias named `RelationshipTypeT` for that.

Replace arraysEqual call with set comparison

8d7dc16

Use the three-argument form of useReducer

fc0c484

https://react.dev/reference/react/useReducer#avoiding-recreating-the-initial-state The previous code invokes `createInitialState` on every single render.

Add advanceUniqueId

a0c5761

Define types for seeded release data

ece2878

Tried my best to derive these from lib/MusicBrainz/Server/Controller/ReleaseEditor.pm.

Fix react/jsx-no-bind in UrlRelationshipCreditFieldset

b9a81fc

`FormRowText` isn't memoized (currently at least), but UrlRelationshipCreditFieldset shouldn't be concerned with that.

mwiencek force-pushed the external-links-editor-refactor branch from a02829b to 8915fb3 Compare February 12, 2026 19:04

mwiencek added 5 commits February 12, 2026 13:31

Remove unused ESLint rule exclusions for root/static/scripts/external…

8b39a5d

…-links-editor/

Handle .eu in isMusicBrainz link utility

eaac2d3

Add deepFreezeInDevelopment

621b7f6

mwiencek force-pushed the external-links-editor-refactor branch from 8915fb3 to 621b7f6 Compare February 12, 2026 19:35

		let hiddenInputsContainer = document.getElementById(
		'external-links-editor-submission',

		* If there are no applicable types, show types that aren't
		* restricted to specific sites.

Uh oh!

MBS-11889, MBS-11521: External links editor refactor #3621

Are you sure you want to change the base?

MBS-11889, MBS-11521: External links editor refactor #3621

Conversation

mwiencek commented Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

reosarevok left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mwiencek commented Oct 22, 2025

Uh oh!

yvanzo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yvanzo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

reosarevok left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mwiencek commented Feb 3, 2026

Uh oh!

yvanzo left a comment

Choose a reason for hiding this comment

Uh oh!

reosarevok left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

reosarevok left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mwiencek commented Aug 28, 2025 •

edited

Loading

mwiencek Feb 12, 2026 •

edited

Loading