Skip to content

feat(routes): report the matched route pattern across nested routers - #1258

Open
damiengillesscality wants to merge 8 commits into
development/1.0from
improvement/route-pattern-tracking
Open

damiengillesscality wants to merge 8 commits into
development/1.0from
improvement/route-pattern-tracking

Conversation

@damiengillesscality

@damiengillesscality damiengillesscality commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

TL;DR — Adds a drop-in Routes wrapper, a RoutePatternProvider and a useCurrentRoutePattern() hook so a host app can tell which page pattern is shown (e.g. /platform/nodes/:name/overview) even though the page is assembled from several nested routers: the shell's, each federated micro-app's, and Tabs. No page needs new code and no list of patterns is kept anywhere — each matched route reports its full match chain to the closest provider and the deepest one wins.

flowchart TD
  subgraph Shell["Host app (e.g. shell-ui)"]
    BR[BrowserRouter] --> P["RoutePatternProvider"]:::new
    P --> SR["Routes /platform/*"]
    Hook["useCurrentRoutePattern()"]:::new
  end
  subgraph Remote["Federated micro-app (maybe another core-ui version)"]
    AR["Routes nodes/*"] --> NR["Routes :name/*"]
  end
  subgraph CoreUI["core-ui"]
    Tabs["Tabs → Routes overview"]:::changed
    Wrap["Routes wrapper: RoutePatternReporter beside every route element"]:::new
  end
  SR --> AR
  NR --> Tabs
  SR -. register .-> P
  AR -. register .-> P
  NR -. register .-> P
  Tabs -. register .-> P
  P -- "deepest match chain" --> Hook
  classDef new fill:#d4f7d4,stroke:#2b8a3e;
  classDef changed fill:#fff3bf,stroke:#e67700;
Loading
  • Each route rendered by the wrapper re-provides the pattern context with its own pattern prefix, and its reporter registers its pattern; nothing reads react-router internals. Parent splats are dropped when joining, so /platform/* + nodes/* + :name/* + overview gives /platform/nodes/:name/overview. Absolute paths resolve against the enclosing <Routes>, as react-router does (Tabs relies on it).
  • Patterns are collected in a plain React context: the deepest mounted reporter wins (latest registered on a tie). Reporters register when the route actually renders, so lazy-loaded remotes and 404 routes are reflected correctly.
  • Only the context object is created on globalThis (Symbol.for('@scality/core-ui/route-pattern-context/v1')), because a micro-app may bundle a different core-ui version than the host, and the host's provider and routes must reach its routes.
  • Without a provider, routes render as usual and report nothing, so a micro-app running standalone needs no setup.
  • When a nested <Routes> matches nothing, the parent's pattern is reported (e.g. /platform/nodes/*).
  • Consumers adopt it by importing Routes from @scality/core-ui instead of react-router. Every router between the host and the page must use it: a level rendered by react-router's Routes is missing from the pattern (e.g. /about instead of /platform/about).

Add a drop-in Routes wrapper that renders a reporter beside every
route element. Each reporter registers the full pattern of its match
chain in a store shared through globalThis, and useCurrentRoutePattern
returns the deepest one, so a host app can tell which page is shown
(e.g. /platform/nodes/:name/overview) across its own router, federated
micro-apps and Tabs, without listing the patterns anywhere.

Tabs now uses the wrapper so tab routes are part of the pattern.
@bert-e

bert-e commented Oct 14, 2026

Copy link
Copy Markdown
Contributor

Hello damiengillesscality,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Available options
name description privileged authored
/after_pull_request Wait for the given pull request id to be merged before continuing with the current one.
/bypass_author_approval Bypass the pull request author's approval ⭐
/bypass_build_status Bypass the build and test status ⭐
/bypass_commit_size Bypass the check on the size of the changeset TBA ⭐
/bypass_incompatible_branch Bypass the check on the source branch prefix ⭐
/bypass_jira_check Bypass the Jira issue check ⭐
/bypass_peer_approval Bypass the pull request peers' approval ⭐
/bypass_leader_approval Bypass the pull request leaders' approval ⭐
/bypass_source_branch_lineage Bypass the cross-branch contamination check ⭐
/approve Instruct Bert-E that the author has approved the pull request. ✍️
/create_pull_requests Allow the creation of integration pull requests.
/create_integration_branches Allow the creation of integration branches.
/no_octopus Prevent Wall-E from doing any octopus merge and use multiple consecutive merge instead
/unanimity Change review acceptance criteria from one reviewer at least to all reviewers
/wait Instruct Bert-E not to run until further notice.
Available commands
name description privileged
/help Print Bert-E's manual in the pull request.
/status Print Bert-E's current status in the pull request.
/clear Remove all comments from Bert-E from the history TBA
/retry Re-start a fresh build TBA
/build Re-start a fresh build TBA
/force_reset Delete integration branches & pull requests, and restart merge process from the beginning.
/reset Try to remove integration branches unless there are commits on them which do not appear on the source branch.

Status report is not available.

@bert-e

bert-e commented Oct 14, 2026

Copy link
Copy Markdown
Contributor

Waiting for approval

The following approvals are needed before I can proceed with the merge:

  • the author

  • one peer

Peer approvals must include at least 1 approval from the following list:

Comment thread src/lib/components/routes/Routes.component.tsx Outdated
Comment thread src/lib/components/routes/Routes.component.tsx Outdated
… key

Render Component before element and an Outlet when element is falsy, so
layout routes with element={null} keep rendering their children. Version
the globalThis key, since core-ui copies of different versions share it.
Comment thread src/lib/index.ts Outdated
@bert-e

bert-e commented Oct 14, 2026

Copy link
Copy Markdown
Contributor

Waiting for approval

The following approvals are needed before I can proceed with the merge:

  • the author

  • one peer

Peer approvals must include at least 1 approval from the following list:

@eve-scality

eve-scality commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

🔴 Request changes

Findings

  • ⚠️ Add a test that checks the pattern immediately after navigation, while both old and new…

  • ⚠️ joinRoutePath is untested for path='/' — document that it behaves like an index route,…

This review is advisory and does not block the merge.

Full review

Findings

  • ⚠️ Add a test that checks the pattern immediately after navigation, while both old and new…
    src/lib/components/routes/Routes.test.tsx:122
    The 'forgets the pattern of routes that unmount' test (line 122) asserts the settled state after a full navigation: the old reporters have cleaned up and the new ones are registered. It does not cover the transient window where both are in the Map simultaneously — which happens whenever a navigation causes new reporters to register before the outgoing route's useLayoutEffect cleanup fires. In that window, deepestPattern (implementation lines 51-63) selects between two entries at the same depth by comparing their order counter. The later-registered entry (higher order) wins, which is the intended behaviour, but this tie-breaking path is never exercised by any test. A regression that reversed the comparison — preferring the lower-order entry — would leave every existing test green, because the tests read only the final settled state where only one entry remains. To protect this path, add a test that navigates between two sibling routes and asserts the pattern immediately after each click, not just after React has fully settled.
    Suggestion: Render two sibling routes ('/platform/alpha' and '/platform/beta') under a single RoutePatternProvider. Click to navigate from alpha to beta, then assert the pattern is '/platform/beta' after the click. Then click back and assert '/platform/alpha'. This forces deepestPattern to resolve a tie between overlapping registrations, not just the single-entry settled state.
  • ⚠️ joinRoutePath is untested for path='/' — document that it behaves like an index route,…
    src/lib/components/routes/Routes.test.tsx:211
    The joinRoutePath unit tests (lines 211-220) cover three shapes: a relative path appended to a prefix, a prefix with no second argument (layout/index route), and an empty prefix with no second argument. They do not test path='/' explicitly. The implementation (line 46-47) filters all empty strings after splitting on '/', so path='/' produces the same result as path=undefined: joinRoutePath('', '/') yields '/' and joinRoutePath('/platform', '/') yields '/platform'. This means a route with path='/' at the top of the tree is reported identically to a no-match, with no test that would fail if the function ever changed that behaviour. The risk model lists 'Route with path="/"' as an untested edge case for exactly this reason.
    Suggestion: Add two assertions to the joinRoutePath describe block: expect(joinRoutePath('', '/')).toBe('/') and expect(joinRoutePath('/platform', '/')).toBe('/platform'). This documents that an explicit slash path is treated as an index route and protects the filter(Boolean) logic that produces this behaviour.

Its argument type comes from react-router's UNSAFE_RouteContext, so
exporting it would tie the stable surface to a react-router internal.
Comment thread src/lib/components/routes/Routes.component.tsx Outdated
Replace the globalThis store with a RoutePatternProvider the host renders
around its routes. Reporters register with the closest provider and
useCurrentRoutePattern reads it from context, so routes rendered without
a provider, such as a micro-app running standalone, report nothing.

Only the context object stays on globalThis, since micro-apps may bundle
a different core-ui version than the host.
@bert-e

bert-e commented Oct 15, 2026

Copy link
Copy Markdown
Contributor

Waiting for approval

The following approvals are needed before I can proceed with the merge:

  • the author

  • one peer

Peer approvals must include at least 1 approval from the following list:

Comment thread src/lib/components/routes/Routes.component.tsx Outdated
…eContext

Each route rendered by core-ui Routes passes its pattern down through a
shared scope context, and its reporter registers that pattern, so the
pattern no longer depends on react-router's UNSAFE_RouteContext.

The trade-off: every router between the host and the page must use
core-ui Routes, since a level rendered by react-router's Routes is now
missing from the pattern. Absolute paths resolve against the enclosing
Routes, as react-router does, which keeps Tabs' paths right.
Each route re-provides the same context with its own depth and prefix,
keeping the provider's register and current pattern, so a second context
is not needed.
Cover when to use core-ui Routes instead of react-router's, and show that
routes rendered by react-router's Routes are missing from the pattern.
@bert-e

bert-e commented Oct 15, 2026

Copy link
Copy Markdown
Contributor

Waiting for approval

The following approvals are needed before I can proceed with the merge:

  • the author

  • one peer

Peer approvals must include at least 1 approval from the following list:

Prettier reflows a template literal inside an MDX Source block and broke the
sample's indentation.
@damiengillesscality
damiengillesscality marked this pull request as ready for review October 15, 2026 14:01

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants