lizhimins commented on PR #4603: URL: https://github.com/apache/rocketmq-dashboard/pull/4603#issuecomment-5761159996
Thanks — this is a well-scoped PR and the implementation holds up. I want to be clear that the reason for closing is a product decision, not a code review finding. **What we verified.** The behaviour you describe is accurate on trunk: `audit.tsx` writes `summary` only in the `.then`, so the `.catch` leaves it null after showing the toast, and `AuditSummaryCards` substitutes an all-zero object for a null summary — "匹配记录 0", "成功率 0%", "失败 / 部分成功 0 / 0" — while the record table on the same screen still lists matching rows. Your scoping is also correct: we checked the other consumer of `summary`, and `AuditRiskInsights` already degrades to the loaded records via `auditRiskInsightModel`'s `?? safeRecords...` fallbacks, so it cannot infer `NO_MATCHING_RECORDS` from a null summary. The cards were the only surface that needed the change. The reset path is right too (`setSummaryLoading(true)` before each fetch, and the `loading` skeleton branch precedes the new `failed` branch, so a retry does not flash the failure state), clearing `summary` rather than keeping the previous aggregate is correct given the filter has chang ed, the new test lands in the existing `AuditPage.test.tsx` and is mutation-sensitive, and using antd `Alert` for a semantic failure state matches the project convention that reserves `InfoBanner` for neutral persistent copy. Your point that the toast auto-dismisses while the zero cards stay on screen is fair. **The decision.** We are keeping the zeros. This is the same call we made on #4386 and wrote up in #4361: when a read fails, the surface keeps its numeric default rather than gaining a second representation of "no data". #4361 was scoped to the consumer-group page, but the reasoning is the one we are applying here — a per-page availability vocabulary means every page that shows derived numbers grows its own `failed`/`unavailable` flag, its own copy string and its own renderer branch, and they have to be kept consistent by hand. We would rather pay the cost of an occasionally misleading zero than ship N bespoke versions of "unknown", and the failure is already announced by the toast plus the fact that the record table below is populated. The only thing the review flagged at all was cosmetic and non-blocking: the new `failed` prop sits next to the component's existing `data.failed` (the count of failed records), which reads confusingly — `summaryFailed` would have been clearer. That is not why this is closing. **If you want to revisit it**, the shape we would engage with is the one #4361 left open: a single repository-wide unavailable-data contract — one resolver, one VO/field convention, one renderer and export policy — covering the consumer-group, audit and any other aggregate surfaces together, rather than one page at a time. That is a reasonable proposal and we would review it on its own merits. -- 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]
