Skip to content

router: fixes and improvements for the router inspector #349

Closed

Description

@erkamyaman

Context

Fixes and improvements from a code audit of the router inspector, done in October 2026. Roadmap work lives in #340, #341, #342 and #343. Items marked (to confirm) were not reproduced; check them before fixing.

Problems

Medium

  • instrument and resolve-lazy ignore actions.router: ROUTER_WRITE_ACTIONS lists only navigate, abort, replay and probe. instrument rewrites guard and resolver entries in the live config, and resolve-lazy runs the app's loadChildren import with its side effects, so both still run with router writes off, while probe (which may also load lazy chunks) is blocked. Decide whether they count as writes and gate them, or document why they are safe. Splitting a read-only probe tool from the write navigate tool would let ACTION_TOOLS.router list the write one (packages/devtools/src/config.ts:404, packages/devtools/src/devframe.ts:2323).
  • links on parents are missed: linksOf only queries a, button, [routerlink], [routerLink]. It misses <li routerLinkActive><a routerLink>, where RouterLinkActive sits on a parent, and property-bound [routerLink] on other elements, which leave no attribute. The link-aria-current lint and the link checks in inspect-route skip these links. Also query [routerlinkactive], or find the directives through ng.getDirectives on hosts the components collector knows, and report a truncated count when MAX_LINKS (100) is hit (packages/devtools/src/router-links.ts:164).
  • router attached only once: the overlay finds the Router once and never checks it again. After an HMR re-bootstrap, a late micro-frontend or Angular Elements, the old router stays watched and routerCleanup never runs. After 3 pushes that find a component but no router, it stops looking for good (routerMisses < 3). Check on a backoff that the router is still reachable from a live root; when it is gone, run routerCleanup, reset state and attach again. Replace the 3-miss cap with createScanBackoff (packages/devtools/src/overlay.ts:408-432).

Low

  • shared module state across routers: pendingCaller, pendingRedirect, configGeneration and knownSecrets are module globals, so with several routers a redirect or caller from one can attach to another's NavigationStart, and a DevTools navigate() racing an app navigation can be mislabelled. Move them into a per-router context returned by watchRouter (packages/devtools/src/router.ts:165-172).
  • title set after the push: nav.title is written in a microtask after onChange already ran, so no push is scheduled and the title only shows up on a later push. Call onChange or scheduleRouterPush after setting it, or read the title lazily on the next push (packages/devtools/src/router.ts:1042-1045).
  • instrumentation described as opt-in: storedInstrumented() returns true unless sessionStorage holds '0', so it is on by default, but the JSDoc says "Opt-in" and the explain-navigation header always tells agents to "Turn instrumentation on". Fix the JSDoc and give the instruction only when page.instrumented is false (packages/devtools/src/router-actions.ts:91-96, 541-545; packages/devtools/src/rpc/router-tools.ts:813).
  • explain-navigation capped at 50: limit is clamped to the hard-coded MAX_NAVIGATIONS = 50, while the hub keeps up to limits.navigations per page. Pass limits.navigations into explainNavigationText (packages/devtools/src/rpc/router-tools.ts:51, 797; packages/devtools/src/devframe.ts:501).
  • empty-redirect-prefix label is wrong: the finding says angular: 'throws'. Angular throws only when pathMatch is missing, and then the app fails before it reports its config. The rule can only fire for an explicit pathMatch: 'prefix', where Angular stays silent. Set angular: 'silent' and say that an explicit 'prefix' on an empty-path redirect matches every URL (packages/devtools/src/rpc/router-config-tools.ts:384-395).
  • param-shadows-literal false positives: a :param route with children and no empty-path child does not shadow a later literal sibling, because the router backtracks when the children fail to match. Flag only leaf param routes or ones with an empty-path child, or reuse matchUrl to confirm (packages/devtools/src/rpc/router-config-tools.ts:367-383).
  • probe uses magic numbers and waits on a sync throw: probe() compares bare event types (4, 2, 1, 3, 16) and cancel code 0 instead of EventType and CANCEL_CODES. When navigateByUrl throws synchronously, promise is null and the probe waits the full 10s. Use the constants and call finish({ error }) at once when promise is null (packages/devtools/src/router-actions.ts:703-711, 746-754).
  • panel copies server types by hand: app/src/pages/router-types.ts redefines NavigationRecord and RouteNode (missing dataKeys, inputs, classGuards, runGuardsAndResolvers), isReplayableUrl copies isSafeUrl, and guardResult already differs ('running' vs 'still running'). Export the types and pure helpers from a shared types-only entry point, or add a contract test (app/src/pages/router-types.ts:81-97, 207; app/src/pages/route-timeline.ts:696; packages/devtools/src/rpc/router-tools.ts:602-607; packages/devtools/src/rpc/router-loops.ts:345).
  • timeline phase colours are hard-coded: #60a5fa, #f59e0b, #2dd4bf and #34d399 are not theme tokens. Amber and teal on var(--surface-3) in the light theme probably fall below the 3:1 non-text contrast of WCAG 1.4.11. Add --phase-* tokens per theme following ui-guidelines.md and check both themes (app/src/pages/route-timeline.ts:20-25) (to confirm).
  • route nesting is visual only: the active-routes table shows depth only through padding-left, so screen readers do not hear the hierarchy. Add visually hidden depth text ("level 2, child of /users") or use a treegrid with aria-level (app/src/pages/route-current.ts:97).
  • in-place config edits not detected: configSignature counts only routes and loaded routes, so a path or guard changed with the same count does not bump the generation, and the cached config, instrumentation and lint stay stale. Add a cheap hash of path, redirectTo and guard counts per node, still capped at MAX_NODES (packages/devtools/src/router-config.ts:214-236).
  • events-only wording contradicts the data: mode is events-only only when ɵgetInjectorProviders is missing, but pushRouter still walks and sends the config whenever a router and ng exist. The messages say the config cannot be read. Tie the wording to what is actually missing (setup detection, preloader and scroller lookup), or set the mode from whether the config could be walked (packages/devtools/src/router-setup.ts:241; packages/devtools/src/rpc/router-config-tools.ts:234, 580, 614).
  • skipped navigations marked imperative: a NavigationSkipped without a NavigationStart is always recorded with trigger 'imperative', even for popstate or hashchange, so the loop and burst heuristics treat it as code-started. Use 'unknown' or the trigger from currentNavigation, and leave these out of isAutomatic (packages/devtools/src/router.ts:965-977).

Improvements

  • One "what happens if I go to X" result: the matched chain, the effective guards per level from the protection audit, the render mode from explain-render-mode and the source file in one card (packages/devtools/src/rpc/router-config-tools.ts, app/src/pages/route-*.ts).
  • A "diff two navigations" view on top of replay: which guard verdicts, resolvers, params and phase timings changed (packages/devtools/src/router-actions.ts, app/src/pages/route-timeline.ts).
  • Turn the redirect-from-query lint into a live check: when instrumentation sees a guard return a UrlTree built from a query param, flag a possible open redirect with the guard name (packages/devtools/src/router-actions.ts, packages/devtools/src/rpc/router-config-tools.ts).
  • Take the per-navigation HTTP request list from the HTTP collector instead of PerformanceResourceTiming, for status codes and sizes without unrelated concurrent requests (packages/devtools/src/router.ts, packages/devtools/src/rpc/router-tools.ts).
  • An MCP resource or tool that streams new navigations, so agents can follow a login and redirect flow without polling explain-navigation (packages/devtools/src/devframe.ts, packages/devtools/src/config.ts).
  • Record whether withNavigationErrorHandler or the platform Navigation API is in use, and show the browser Navigation API entries next to router history to explain urlDrift (packages/devtools/src/router-setup.ts, app/src/pages/route-current.ts).
  • Lint rules for runGuardsAndResolvers: 'always' on busy parents, resolvers on routes whose components fetch the same data, and loadComponent routes whose component is already in the eager bundle (packages/devtools/src/rpc/router-config-tools.ts).

Already tracked elsewhere


Generated by Claude Code

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething is broken or shows wrong datafeatureA feature request or a pull request that adds one

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions