RockteMQ-AI commented on PR #4237:
URL:
https://github.com/apache/rocketmq-dashboard/pull/4237#issuecomment-5635597027
CI only runs `npm run build` for the frontend (no `npm test`), which
explains why this slipped through. I have everything needed for the review.
---
# Code Review: PR #4237 β Message Query History Insights
## 1. Summary
Well-built read-only insights layer: a pure, deterministic aggregation
function with solid unit tests, wired into the existing history drawer with a
minimal 7-line integration. However, it **deterministically breaks an existing
test** (`MessagePageAsyncState.test.tsx`), and because the drawer only loads
the *active tab's* current page, the insights are computed from partial,
tab-dependent data β which undermines several of the advertised signals.
## 2. Issues Found
### π΄ Critical (must fix)
- **Broken existing test β
`web/src/components/MessageHistoryInsightsPanel.tsx:184` duplicates visible
text.**
The notable-rows "Query Detail" column renders the trace `msgId` as plain
text, while the drawer's trace table below it renders the same `msgId`
(`MessageQueryHistoryDrawer.tsx:123`). The existing test
`web/src/pages/instance/__tests__/MessagePageAsyncState.test.tsx:584` does
`await screen.findByText('history-message')` and now fails with *"Found
multiple elements with the text: history-message"*. Verified: passes at base
`HEAD~1`, fails with this commit. The PR's verification list ran
`MessageQueryHistoryDrawer.test.tsx` but not this file. Note CI only runs `npm
run build` for web β nothing will catch this automatically, so the test must be
updated here (e.g. scope with `within()` on the history table or use
`findAllByText`) or the panel's detail cell disambiguated.
### π‘ Suggestion (should consider)
- **Insights are computed from partial, tab-dependent data β
`web/src/components/MessageQueryHistoryDrawer.tsx:64-85`.** The drawer fetches
only the active tab (20 rows/page) and resets both row arrays on every load.
Consequences:
- `traceRows` is `[]` whenever the messages tab is active, so
`FRAGMENTED_TRACE_TOPICS` and trace-gap signals can **never fire** from the
messages tab β and vice versa for message signals. The health score/level flips
when the user switches tabs.
- Counts are phrased as global facts ("Zero-result message searches:
{count}") but reflect only the visible page, additionally filtered by the
search box. With hundreds of history rows, the panel systematically
under-reports and can show "Healthy" while risky rows sit on other pages.
- Suggest fetching a dedicated sample for the panel (the API allows
`pageSize` up to 100, `QueryHistoryController.java:26,67`), or at minimum
labeling the issue tags as page-scoped rather than global.
- **`queriedAt` shows different values in the same drawer.** The panel
correctly uses `formatUtcDateTime` (`MessageHistoryInsightsPanel.tsx:226`) for
the backend's offset-less UTC `LocalDateTime` (`QueryHistoryService.java:64`,
`Clock.systemUTC()`), but the drawer's own tables use `formatTime` = `new
Date(value).toLocaleString()` (`MessageQueryHistoryDrawer.tsx:30-34`), which
parses offset-less strings as local time. The same row will display two
different timestamps a few hundred pixels apart. Pre-existing bug, but this PR
makes it visible; consider aligning both to `formatUtcDateTime`.
- **`tag` treated as a narrowing filter under-detects broad scans β
`web/src/utils/messageHistoryInsights.ts:200`.** The provider applies tag
matching *in-memory after* the server-side scan (`RocketMQMessageProvider.java`
`matchesTag` post-filter; topic scans capped at `TOPIC_QUERY_HARD_CAP = 2000`).
A TOPIC query with only a tag over a β₯24h window is still a full broker scan,
so `BROAD_TOPIC_QUERIES` misses exactly the expensive case it exists to flag.
### π’ Nit (minor)
- `web/src/utils/messageHistoryInsights.ts:428-430` β the `operator: '-'`
detail is never rendered (no translation uses an `{operator}` placeholder);
dead data.
- `web/src/utils/messageHistoryInsights.ts:174-177` β the
`NO_HISTORY`/`STALE_HISTORY`/`UNKNOWN_OPERATORS` case branch duplicates
`default`; the switch could collapse.
- English copy for `FRAGMENTED_TRACE_TOPICS` ("Trace topics in use:
{count}") reads informational rather than as a risk signal; the zh version
("ζ£ζ΅ε° {count} δΈͺδΈε轨迹 Topic") is clearer.
## 3. Positive Aspects
- **Pure, testable core**: `buildMessageHistoryInsights` is a pure function
with an injectable `now`, making all time-based logic (stale history, latest
age) deterministically testable β rare discipline.
- **Edge-case hardening**: `normalizeCount`/`parseTimestamp` handle `NaN`,
negatives, and invalid dates, and there are explicit tests for them
(`messageHistoryInsights.test.ts:202-224`).
- **Thresholds grounded in real server behavior**: `LARGE_RESULT_CRITICAL =
2000` matches `TOPIC_QUERY_HARD_CAP`; broad-window detection composes with the
server's 7-day topic-window cap (`MessageService.java:221`).
- **Complete i18n**: all new keys exist in both zh and en, including
`.short` variants and fallbacks.
- **Good UX hygiene**: panel hides itself when nothing is loaded, caps
issues at 5 and notable rows at 8, sorts by severity, and the drawer
integration is minimal.
- No new API surface or security-relevant changes; purely client-side
aggregation of existing read endpoints.
## 4. Verdict
**Request Changes.** The broken `MessagePageAsyncState` test is a hard
blocker (must update that test or disambiguate the rendering). The
tab-dependent sampling is the second substantive item β as merged, several
advertised signals (fragmented trace topics, trace gaps) only function while
the traces tab happens to be active, which I'd want addressed or explicitly
scoped before this ships. The rest is polish on an otherwise clean, well-tested
implementation.
---
<sub>π€ Review by **github-manager-bot** | PR #4237 |
2026-09-11T14:05:11Z</sub>
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
