You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@rocketmq.apache.org by "messere1 (via GitHub)" <gi...@apache.org> on 2026/10/10 10:00:29 UTC
[PR] fix(web): sanitize throughput sparkline samples [rocketmq-dashboard]
messere1 opened a new pull request, #6137:
URL: https://github.com/apache/rocketmq-dashboard/pull/6137
### What problem does this PR solve?
The dashboard throughput sparkline (`MiniBar`) renders invalid samples as traffic:
```ts
const max = Math.max(...data, 1);
height: `${value === 0 ? 0 : Math.max(4, (value / max) * height)}px`
```
- A negative sample (a monotonic broker counter reset between collection windows produces negative deltas) is rendered as a visible 4px traffic bar instead of no traffic.
- A non-finite sample (a failed collection window) poisons `Math.max`, so every bar computes `NaNpx` — an invalid CSS height the browser discards, collapsing the whole sparkline.
Original report: #1680 (issue closed as not planned by the stale sweep, not invalidated as fixed).
### What is changed and how it works?
Samples are normalized to a finite non-negative value once, before the maximum, the rendered heights and the default accessible label are computed:
```ts
const normalizedData = data.map((value) => (Number.isFinite(value) ? Math.max(0, value) : 0));
```
Valid positive samples keep their current relative heights and minimum visibility; negative and non-finite samples read as zero, both in the bars and in the localized `aria-label`.
### Verifies
- [x] `npx vitest run src/components/__tests__/MiniBar.test.tsx` — 7/7 pass; the new regression test fails on the unpatched tip (bars end up with discarded/empty heights for `[-5, NaN, Infinity, 10]`).
- [x] `npx eslint` on the touched files — clean.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(web): share sample sanitization across the chart family [rocketmq-dashboard]
Posted by "messere1 (via GitHub)" <gi...@apache.org>.
messere1 commented on PR #6137:
URL: https://github.com/apache/rocketmq-dashboard/pull/6137#issuecomment-6129510439
Thanks @lizhimins for the deep verification — the `List<Integer>` / no-delta-computation analysis is exactly the correction this needed, and the prescription was straightforward to follow. Reworked in c0232b01, point by point:
1. **Shared helper** — `finiteNumber`/`nonNegativeNumber` now live in `web/src/utils/chartSamples.ts`; `dashboardTrafficInsights.ts` consumes the shared rule instead of its private copy, with its public API unchanged (its existing tests are untouched and green).
2. **Family consistency** — both `MiniBar` and `MiniLine` consume the shared helper, so the identical `Math.max(...data, 1)` pattern is sanitized in both. `MiniLine` gets a new mutation-sensitive test: the path for `[-5, NaN, Infinity, 10]` must equal its zero-clamped reference `[0, 0, 0, 10]` with no `NaN` coordinates — it fails on the unpatched component.
3. **Reflow dropped** — the empty-state `<span>` is back to the original formatting. (For the record: that reflow was almost certainly our pre-commit lint-staged `prettier --write`, not a hand edit; since CI does not gate on prettier, the trunk formatting stands as requested.)
4. **Motivation restated** — the PR description now frames this as defense-in-depth for a shared chart component and explicitly states the only production caller feeds a server-side `List<Integer>` that cannot be NaN/negative; it is family-consistency hardening, not a live bug fix. Title updated to `refactor(web): share sample sanitization across the chart family` to match.
Full check on the reworked tip: 21/21 tests green across MiniBar / MiniLine / dashboardTrafficInsights / the insights page test, eslint clean, `tsc -b` clean.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(web): share sample sanitization across the chart family [rocketmq-dashboard]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #6137: refactor(web): share sample sanitization across the chart family
URL: https://github.com/apache/rocketmq-dashboard/pull/6137
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(web): share sample sanitization across the chart family [rocketmq-dashboard]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #6137:
URL: https://github.com/apache/rocketmq-dashboard/pull/6137#issuecomment-6280291296
This pull request has been automatically closed because there has been no activity for 7 days. If you would like to continue working on this, please reopen the PR with updated changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(web): share sample sanitization across the chart family [rocketmq-dashboard]
Posted by "messere1 (via GitHub)" <gi...@apache.org>.
messere1 commented on PR #6137:
URL: https://github.com/apache/rocketmq-dashboard/pull/6137#issuecomment-6130467944
The Backend Tests (Java 21) failure on this head looks like an infra flake rather than anything this PR can influence:
- the sole error is `ResourceOwnershipGuardTest.migrationConcurrentStartupIsIdempotentTest` — an H2 `ConcurrentModificationException` thrown from inside H2's own `DatabaseMetaLocal.getIndexInfo` while the test drives concurrent schema-migration startup by design; the stack has no frame in `web/` or anywhere this diff touches;
- this PR is frontend-only (`chartSamples.ts` + the two chart components + a re-import in `dashboardTrafficInsights.ts`);
- the same 3523-test suite is green in the same window on our other heads (#6145, #6368, #6448), and the full frontend pipeline (eslint + vitest + `tsc -b`) was verified green locally on the exact pushed tree.
Could you re-run the Backend Tests job (or re-approve the run) when convenient? Happy to do anything else that helps.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org