lizhimins commented on PR #5720: URL: https://github.com/apache/rocketmq-dashboard/pull/5720#issuecomment-6093317997
> Thanks — the helper design (dedupe by key, explicit `SKIPPED_AUTH`, cursor-based worker pool with an injectable clock) and its 11 tests are solid, and the concurrency / dedupe / skip assertions are genuinely mutation-sensitive. Five things to address before this can land: > > 1. Two user-visible strings bypass i18n: `dataSourceConnectivityBatch.ts:64` (`'Requires credentials entered in the editor'`) and `:98` (`'Connection request failed'`). They end up in the result table and in the exported CSV, so zh-CN users see English. Please thread translations in from the component (inject a label/message function) or map status → i18n key at render time; `settings.dataSourceAuthTestHint` already exists for the skipped case. > 2. `dataSourceNeedsRuntimeSecret` (`dataSourceConnectivityBatch.ts:42`) duplicates `authNeedsSecret` in `pages/settings/DataSourceTab.tsx:94` with identical semantics. Please extract one shared helper (e.g. under `web/src/utils/`) and have both call sites use i t, so the two cannot drift when a new auth mode is added. > 3. The new 245-line drawer has no test. Please add a component test (open → run → success/failed/skipped rows, skipped-count hint, status filter) plus a case in `DataSourceTab.test.tsx` for the new entry button (opens the drawer; disabled when `total === 0`). > 4. `CONTRIBUTING.md` asks page-level neutral notes to use the shared `InfoBanner` component; the resident description at `DataSourceConnectivityDrawer.tsx:168-175` uses `<Alert type="info">`. The `type="warning"` skipped-auth notice at `:199` is a semantic alert and can stay. > 5. Table widths: all seven columns declare a fixed `width`, so surplus width in a wide drawer is left blank. Per the list-table rule in `CONTRIBUTING.md`, give the primary text column (`name`, `:96`) `minWidth` so it absorbs the surplus. --- **Decision**: we are closing this rather than requesting the changes above. A 613-line browser-side bulk-probe capability is new functionality, and new features need an agreed design first - an issue or RFC that maintainers sign off on - rather than arriving as a finished pull request; #3210 was filed by an account whose contributions we no longer review, so there is no agreed design behind this one. Independently of that, the 245-line drawer has no tests, two user-facing strings bypass i18n, and `authNeedsSecret` is re-implemented instead of reusing the existing helper. If you want to pursue it: open an issue describing the operator workflow and the intended UX, and we will settle the design there. A much smaller PR that reuses the existing helper and covers the drawer would then be welcome. -- 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]
