probe review: please ignore

Thorough review of the cumulative final state at b4779cb (WIP, so no per-commit notes). Compared against merge-base bd3d3701c5 with both UIs talking to the same make run-broker instance. Almost everything matches base behaviour, but there are 2 blockers and several should-fix items.

(Posted as a COMMENT review — this account can't REQUEST_CHANGES on rabbitmq/rabbitmq-server.)

Blockers

1. Page-out-of-range recovery is broken (main.js:1677, not in the PR diff) — check_bad_response still calls Sammy.log(...), but the shim only exposes Application (see inline on router.js), so it throws before update_pages(..., 1). The out-of-range page number stays in localStorage; every refresh hits 400 again and the list never redraws. Verified: after forcing page 7 on exchanges, PR kept page 7; base reset to 1. Hits when a list shrinks under the user (vhost switch, deletes, filters). Fix: console.log/debug, or add log to the shim.

2. Chart y-axes no longer start at 0 — Flot 4's default autoScale: "loose" ignores yaxis.min: 0. A flat backlog of 1000 becomes 1000–1000.01 with five identical "1.0 k" ticks; 100→120 becomes 95–125; rates 0.15/0.2 become 0.14–0.21. autoScale: "none" leaves the axis at 0–1. A fix that restored base ranges in testing: push an invisible zero series in render_chart, e.g. out_data.push({data:[[x0,0]], lines:{show:false}}).

Should-fix

3. Start page loads twice → getFullYear TypeError — when the URL has no # (normal after login / logout→login / reload of /), start_app sets url + '#/' (queues hashchange), then app.run() immediately runs #/ and starts /api/overview; the hashchange runs the same route again, aborts the first request (status 0), and update_status('error') throws because last_successful_connect is still undefined (fmt_date0 → getFullYear at main.js:1504). Sammy skipped re-running the same location; the new router does not. Verified 3/3 PR logins vs 0/3 base; also duplicates /api/overview. Same path for the OAuth #token_type redirect by reading. Fix: remember last routed hash and skip same-hash hashchange; also don't treat aborted requests as "Error", and harden update_status.

4. Dark mode: chart labels nearly invisible — Flot 4 draws axis labels as SVG <text> with black fill on a dark background (base used grey HTML labels). Y-axis label also sits ~8px left of the chart and gets clipped (light mode too). Font 10px → 12px. Likely fix: .flot-tick-label { fill: currentColor } or axis font.color, plus left margin. (Screenshots compared PR vs base locally; happy to share another way if useful.)

5. OAuth2 callback still loads deleted files (js/oidc-oauth/login-callback.html:7-8, not in the PR diff) — still references jquery-3.5.1.min.js and base64.js (both 404). Not fatal on the success path of oauth_completeLogin (doesn't use $), but needs updating to jquery-3.7.1.min.js and dropping the base64 script.

6. Third-party plugins: one unsupported Sammy call kills the whole UI — in-repo plugins are fine (management, stream, top, shovel, federation, tracing: only get/put/post/del with string routes, this.params, this.title). External risk: a regex route throws (path.replace is not a function); this.redirect / partial / before / params.toHash also throw. The throw is inside dispatcher(), so start_app never runs and the entire UI is dead. Wrap each dispatcher_modules[i](this) in try/catch, accept regex routes, and mention the Sammy removal in release notes.

Minor

• Forms with no method: router always sets location.hash = path + '?' + qs. The OAuth oauth2-resource-form has neither method nor action → URL becomes #? (Sammy did nothing). Clash with non-SP-initiated location.href redirect untested (no IdP).
• Idle charts lose the top "1.0" tick (fmt_y_axis max==1 hack).
• #/queues?hasOwnProperty=1 throws and the page doesn't render (base rendered Queues) — use Object.prototype.hasOwnProperty.call.
• Form with unknown method throws at routes.length after preventDefault (harmless).
• Tests: router suite never exercises run() + hashchange (would have caught #3); charts.test.js only checks a config constant (misses #2); nothing greps leftover Sammy.*; js-test isn't in CI and exits 0 when node is missing; engines >=21 but tests pass on Node 20.

Nits

• jQuery.inArray still at global.js:791-793 and formatters.js:475,487 (works; the replace pass is incomplete).
• LICENSE still lists ExplorerCanvas / base64.js / Sammy; their LICENSE-* files remain.
• jquery.flot-4.2.6.js is byte-identical to .min.js.
• .gitignore adds /.playwright-mcp/ (author tooling).
• Router helpers are globals; :param names now include - (Sammy stopped at \w).

Clean / verified OK

• jQuery 3.7.1 byte-identical to code.jquery.com; no removed APIs in core, plugins, or templates; indexOf / .trim() replacements match.
• Flot bundle byte-identical to npm flot@4.2.6 (dist/es5); time plugin included; time labels fine.
• CSP: no eval/inline script; tested with the broker's CSP header.
• Removed files have no remaining callers except finding 5.
• Route params / query decoding match Sammy; queue q a/b#c?d+e%f and vhost v h/#? went through create/open/publish/get/purge/delete same as base.
• Login, failed login, logout, rate-options auto-submit, back/forward, titles, #/top and #/traces redirects match.
• Packaging: test/ not in the plugin package; no build scripts reference removed files.
• make -C deps/rabbitmq_management js-test: 58/58 on Node 20.19.2. Plugin suites (still on old fake Sammy): federation 3/3, shovel 3/3, stream 4/4.

Not checked

Real OAuth2/UAA (no IdP), Firefox/Safari/touch, actual third-party plugins.

Fresh full review of the cumulative state (merge-base bd3d3701c5..c1433b6). I compared against the merge-base UI in headless Chrome, with the API served by a live make run-broker node and with the core, stream, top, shovel, federation and tracing plugins enabled.

No blockers and nothing to fix before merge. Every route, form and plugin page I exercised behaves the same as on the merge-base.

Previous issues: all 7 fixed and verified against merge-base

• The Sammy.log call in check_bad_response is gone. A page that is out of range gets a 400 and falls back to page 1 with no exception.
• The y-axis starts at 0 again: data at 100–122 gives a 0–150 axis, the same as on the merge-base. All overview and queue chart axes and ticks match.
• Logging in sends one page request, with no aborted XHR and no duplicate #/ run. When /api/overview fails before any success, the merge-base throws getFullYear of undefined. The PR shows "Error: could not connect to server. Will retry at …".
• Chart labels in dark mode are readable (#bbb on dark, #666 on light).
• oidc-oauth/login-callback.html loads jquery-3.7.1.min.js. All of its scripts return 200, with no errors.
• A GET form with no fields now goes to the plain action, and filled fields give the same query string as on the merge-base.
• A query key such as hasOwnProperty no longer throws (covered by a test).

What I exercised (merge-base vs PR, identical results)

• About 45 routes, including every plugin route. I used a vhost rv2/v h+x and queue names containing / % + # ? & = and unicode, plus a query string, an unknown route and a malformed %. A malformed % raises the same URIError in both.
• Real tab and link clicks, back/forward, reload on a hash, and login, bad login and logout.
• Every in-repo PUT/POST/DELETE form produces the same API calls: queues, exchanges, bindings, publish/get, purge, users, permissions, policies, limits, cluster name, super streams, federation upstreams, dynamic shovels, traces, and the auto-submitted column/rate options.
• A plugin that calls an unsupported Sammy API keeps the routes it registered before that call, and the other plugins keep working.
• I injected jQuery Migrate and browsed every page. Core, plugin and template code make no deprecated or removed jQuery calls. The only warnings come from Flot 4.2.6 itself ($.isFunction, .unbind, .click()/.dblclick() shorthands). They are harmless on 3.7.1 but will need attention for a future jQuery 4.
• jquery-3.7.1(.min).js and jquery.flot-4.2.6.min.js are byte-identical to the npm releases. Sammy, base64, json2, excanvas, title.js and evil.css all return 404, and nothing references them anymore.
• I found no eval or new Function, and the broker's CSP logged no violations. test/ is not part of the packaged plugin.

Minor (inline comments below)

1. router.js:109: a regex route with the g flag only matches every other time, because exec keeps lastIndex.
2. router.js:125: exceptions thrown by route callbacks now propagate. Sammy caught and logged them.
3. router.js:62-66: use() and helper() silently do nothing.
4. charts.js:257-261: the lines are now 1px instead of 2px, and tickColor no longer sets the grid line colour.
5. charts.js:267-268: an empty series now gets a 0–0.01 axis with five "0.0" labels.

Nits (not on diff lines)

• put_parameter / put_cast_params in main.js: mandatory_keys.indexOf(...) throws when a caller passes undefined, where jQuery.inArray returned -1. All in-repo callers pass arrays, so this only matters for third-party extensions that call these globals.
• The js-test target duplicates the existing npm-test target (rabbit_common/mk/rabbitmq-npm-tests.mk), which already runs as part of make tests. npm-test needs Node ≥ 21 for its glob; js-test works on Node 20.
• The unminified jquery-3.7.1.js still ships but nothing loads it, while the unminified Flot was dropped. LICENSE-MIT-jQuery still carries the 2011 John Resig copyright; upstream now says "OpenJS Foundation and other contributors". rabbitmq_federation_management/LICENSE* still lists Sammy, base64.js and ExplorerCanvas (this predates the PR).
• The router exposes a global named Application, which is quite generic.
• The release note could mention point 2 above.

Tests and CI

• make -C deps/rabbitmq_management js-test on Node 20: 83/83 pass. With Node 22 and node --test 'test/**/*.test.js': management 83/83, federation_management 3/3, shovel_management 3/3, stream_management 4/4.
• CI on c1433b6: everything is green, including "Test Management UI with Selenium for PRs" (basic auth, management prefix, OAuth with Keycloak/UAA).
• Coverage gaps: nothing tests unload(), regex routes with g, or empty series. The router plugin tests cover stream and top, but not shovel, federation or tracing routing.

Not checked

• End-to-end OAuth against a real IdP. I relied on the Selenium job for that.
• Multi-node clusters, and browsers other than Chrome.

All points from the previous review are addressed. Two remaining notes:

• Minor, router.js:63: use() now throws. A third-party extension calling this.use('Title') before its routes (a common Sammy idiom) will lose every route defined after that call, since dispatcher() catches the error (main.js:97-101). Suggest making use() a no-op with a console.warn (keep helper() throwing), or listing use as unsupported in the release notes.
• Nit, charts.js:321-322: getComputedStyle runs on every chart render; the grid colour could be read once per refresh.

Pre-existing on main, not introduced here (main.js:115-126, start_app_login): with OAuth enabled, app.run() is only called when there are no stored credentials. If stored credentials exist but check_login() fails, the login page is re-rendered with the Basic auth form while the router is not bound. Submitting it then falls back to a native GET (browsers don't know method="put"), so the browser goes to /?username=…&password=…#/login and the password lands in server logs and browser history.

Fix: use the same condition as the non-OAuth branch:

if (!has_auth_credentials() || !check_login()) app.run();

Two pre-existing issues on main in $(document).ready (deps/rabbitmq_management/priv/www/js/main.js). These lines aren't in this PR's diff, so they go here.

1. Medium: on the ?error= landing page, the Basic form submits natively and the password ends up in the URL (main.js:24-31)

The if (error) branch renders login_oauth with the warning but never calls start_app_login(), so no router is bound. With OAuth enabled and oauth_disable_basic_auth: false, the Basic form is shown (notAuthorized is false). Submitting <form action="#/login" method="put"> then falls back to a native GET:

/?username=guest&password=guest#/login

The password ends up in broker access logs, browser history, proxy logs and the address bar. This branch is reached through:

• oauth_redirectToLogin(error) in oidc-oauth/helper.js:290-294 (SP-initiated callback errors);
• rabbit_mgmt_login:replyWithError/3 (IdP-initiated POST /login failures);
• any link to /?error=....

I reproduced it in headless Chrome at 3d18f2c, both SP- and IdP-initiated. It's the same class of bug that 1ecf32c fixes in start_app_login.

Suggested fix (tested; afterwards the login goes through login() and no password reaches the URL):

  if (error) {
    clear_pref("oauth-idp-pending");
    clear_pref("oauth-return-to");
    if (oauth.enabled) {
      renderWarningMessageInLoginStatus(oauth, fmt_escape_html(error));
      start_app_login(true);
    }
  } else {

function start_app_login (skip_stored_credentials) {
  // ...
  if (skip_stored_credentials || !has_auth_credentials() || !check_login()) {
    app.run();
  }
}

Optionally, also drop ?error= once it has been read, so it doesn't stay in the URL after a successful login. Otherwise a reload shows the error page again:

history.replaceState(null, '', location.pathname + location.hash);

2. Nit: ?error= with OAuth disabled renders a blank page (main.js:28-30)

Only the oauth.enabled case renders anything, so with OAuth off the page stays empty.

We can merge this. Remaining findings are for issues that already exist in main, or nits. We can fix them here or elsewhere. Merging early ensures we can test it over a long period of time.