Don't register mediawiki.router routes that are catch-alls
Closed, ResolvedPublic
Actions

Description

Currently there are a few routes registered which are catch-alls -- a regexp that matches any string. They're being used as a "navigated away" faux-event.

This is bad, because only one route will ever be executed, so the instant we have multiple registered on the same pageload that cleanup-path will be completely broken.

From another ticket there was discussion of alternatives, but we should just clean these up first with the existing "routing is happening" event since they're broken now.

In T376391#10268858, @Esanders wrote:

There is some discussion on https://gerrit.wikimedia.org/r/c/mediawiki/extensions/MultimediaViewer/+/1080061 on the best way to register functions for closing apps, i.e. when the path no longer matches your app, or is empty.

@Jdlrobson has proposed a second callback:
router.addRoute( Config.ROUTE_REGEXP, this.route.bind( this ), destroyMMV );

although I think this not quite flexible enough, you may have two paths within one app and exiting either would not destroy the app, but exiting both would.

@Jdlrobson's second proposal is:
router.addDestroyRoute( / list of routes not to apply to / [ Config.ROUTE_REGEXP ] () => {} )

I think this is more promising as syntactic sugar around the general approach in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/MultimediaViewer/+/1080061.

In general it might be a good idea to move Router away from being a registry, as I don't see much value from being abe to silently overwrite a path registration - it is far more likely we would have multiple apps listening to the same path.

To do

TypeAheadSearch
MultimediaViewer

[x ] NearbyPages

Details

Related Changes in Gerrit:

Subject	Repo	Branch	Lines +/-
Don't register mediawiki.router routes that are catch-alls	mediawiki/extensions/NearbyPages	master	+9 -8
mediawiki.router: add a teardown callback to addRoute	mediawiki/core	master	+16 -3
Routing: Don't register a path for closing MMV	mediawiki/extensions/MultimediaViewer	master	+56 -39
Typeaheadsearch: don't add an empty route	mediawiki/core	master	+3 -2

Customize query in gerrit

Related Objects

Mentioned In: T401067: Back button no longer work on my mobile device for search
T376391: Can't close a map after opening an image
Mentioned Here: T376391: Can't close a map after opening an image

Event Timeline

DLynch created this task.Aug 6 2025, 4:51 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 6 2025, 4:51 PM

https://codesearch.wmcloud.org/search/?q=addRoute%5C%28&files=.%2B%5C.%28%3F%3Ajs%7Cvue%29%24&excludeFiles=.%2B%2Fnode_modules%2F.%2B&repos= suggests that there's three easily-findable examples left: TypeAheadSearch, MultimediaViewer, and NearbyPages.

Change #1175547 had a related patch set uploaded (by DLynch; author: DLynch):

[mediawiki/core@master] Typeaheadsearch: don't add an empty route

https://gerrit.wikimedia.org/r/1175547

gerritbot added a project: Patch-For-Review.Aug 6 2025, 5:07 PM

Change #1175547 merged by jenkins-bot:

[mediawiki/core@master] Typeaheadsearch: don't add an empty route

https://gerrit.wikimedia.org/r/1175547

ReleaseTaggerBot added a project: MW-1.45-notes (1.45.0-wmf.14; 2025-08-12).Aug 6 2025, 6:00 PM

Maintenance_bot removed a project: Patch-For-Review.Aug 6 2025, 6:30 PM

Change #1080061 had a related patch set uploaded (by DLynch; author: Esanders):

[mediawiki/extensions/MultimediaViewer@master] Routing: Don't register a path for closing MMV

https://gerrit.wikimedia.org/r/1080061

gerritbot added a project: Patch-For-Review.Aug 7 2025, 8:40 PM

matmarex subscribed.Aug 11 2025, 8:08 PM

Change #1080061 merged by jenkins-bot:

[mediawiki/extensions/MultimediaViewer@master] Routing: Don't register a path for closing MMV

https://gerrit.wikimedia.org/r/1080061

ReleaseTaggerBot edited projects, added MW-1.45-notes (1.45.0-wmf.15; 2025-08-19); removed MW-1.45-notes (1.45.0-wmf.14; 2025-08-12).Aug 12 2025, 1:00 AM

DLynch mentioned this in T376391: Can't close a map after opening an image.Aug 12 2025, 1:30 AM

Maintenance_bot removed a project: Patch-For-Review.Aug 12 2025, 1:30 AM

To protect against this in the future, addRoute should log a warning or throw when trying to register a path that matches empty string or /.

matmarex added projects: MediaWiki-User-Interface (autocomplete search), MediaViewer, NearbyPages.Aug 12 2025, 2:41 PM

matmarex updated the task description. (Show Details)

Jdlrobson-WMF subscribed.Aug 15 2025, 5:48 PM

To protect against this in the future, addRoute should log a warning or throw when trying to register a path that matches empty string or /.

That seems sensible. I do think given the widespread usage we should consider creating router.addDestroyRoute or router.addCatchAll to make this easier to understand. We can also make the error point to this method.

FWIW I recently used Vue router which does it using the following route:

{
       path: '/:catchAll(.*)',
       component
   }

FWIW I can review a Nearby patch but I'm still getting up to speed on what we're doing here.

Change #1179216 had a related patch set uploaded (by DLynch; author: DLynch):

[mediawiki/core@master] mediawiki.router: add a teardown callback to addRoute

https://gerrit.wikimedia.org/r/1179216

gerritbot added a project: Patch-For-Review.Aug 15 2025, 7:28 PM

I think I like the second argument to addRoute the most, so here's a patch implementing it, plus the warning on an empty route.

Jdlrobson-WMF mentioned this in T401067: Back button no longer work on my mobile device for search.Aug 15 2025, 10:06 PM

Change #1180629 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/extensions/NearbyPages@master] Don't register mediawiki.router routes that are catch-alls

https://gerrit.wikimedia.org/r/1180629

Jdlrobson-WMF triaged this task as Low priority.Aug 20 2025, 9:55 PM

Jdlrobson-WMF added a project: Readers Essential Work.

Jdlrobson-WMF moved this task from For triage to Freezer on the Readers Essential Work board.Aug 27 2025, 3:00 PM

Jdlrobson-WMF added a project: Reader Growth Team.Aug 29 2025, 5:08 PM

Jdlrobson-WMF moved this task from Incoming/Inbox to Needs Refinement on the Reader Growth Team board.

Change #1179216 merged by jenkins-bot:

[mediawiki/core@master] mediawiki.router: add a teardown callback to addRoute

https://gerrit.wikimedia.org/r/1179216

ReleaseTaggerBot edited projects, added MW-1.45-notes (1.45.0-wmf.17; 2025-09-02); removed MW-1.45-notes (1.45.0-wmf.15; 2025-08-19).Aug 29 2025, 6:00 PM

Change #1180629 merged by jenkins-bot:

[mediawiki/extensions/NearbyPages@master] Don't register mediawiki.router routes that are catch-alls

https://gerrit.wikimedia.org/r/1180629

Maintenance_bot removed a project: Patch-For-Review.Sep 2 2025, 2:32 PM

ReleaseTaggerBot edited projects, added MW-1.45-notes (1.45.0-wmf.18; 2025-09-09); removed MW-1.45-notes (1.45.0-wmf.17; 2025-09-02).Sep 2 2025, 3:00 PM

As we went with the teardown argument, I'll note that using that syntax you could work around the limitation I described in the description above, e.g.

const noop = () => {};
router.addRoute( /subfeature1\/.*, loadSubFeature1 );
router.addRoute( /subfeature2\/.*, loadSubFeature2 );
router.addRoute( /subfeature1|subfeature2\/.*, noop, closeApp );

this would allow you to switch between two different routes of an app, and only call closeApp when a route matching neither is detected.

Jdlrobson-WMF closed this task as Resolved.Sep 2 2025, 10:13 PM

Jdlrobson-WMF claimed this task.

Jdlrobson-WMF updated the task description. (Show Details)

Jdlrobson-WMF edited projects, added Unplanned-Sprint-Work, Reader Growth Team (Sprint 4 ); removed Reader Growth Team.

Don't register mediawiki.router routes that are catch-allsClosed, ResolvedPublicActions