RockteMQ-AI commented on code in PR #2938:
URL:
https://github.com/apache/rocketmq-dashboard/pull/2938#discussion_r3909139252
##########
web/src/services/consumerService.ts:
##########
@@ -138,9 +138,15 @@ export async function listAllConsumerGroups(
page,
pageSize: EXPORT_PAGE_SIZE,
});
- groups.push(...result.items);
- const total = result.total ?? groups.length;
- if (result.items.length === 0 || groups.length >= total) {
+ // A success envelope may still carry a null or item-less payload; treat
it as an
+ // empty page instead of crashing the whole export walk.
+ const items = Array.isArray(result?.items) ? result.items : [];
+ groups.push(...items);
+ const total =
Review Comment:
When `result.total` is not a finite number (e.g. `Infinity`, `NaN`, or a
string) but `result.items` contains entries, `total` falls back to
`groups.length`. The exit guard `groups.length >= total` then evaluates to
`true` immediately, silently truncating the export after the first page.
Consider logging a warning when `total` is discarded, or treating a
missing/invalid `total` as a signal to continue until an empty page rather than
stopping.
##########
web/src/services/consumerService.test.ts:
##########
@@ -15,7 +15,7 @@
* limitations under the License.
*/
-import { describe, expect, it, vi } from 'vitest';
+import { beforeEach, describe, expect, it, vi } from 'vitest';
Review Comment:
`afterEach` is used on line 51 but is not imported from `vitest`. The import
on line 18 adds `beforeEach` but omits `afterEach`. This will throw a
`ReferenceError` at test time, causing the entire `describe` block to fail.
Fix: add `afterEach` to the import list.
##########
web/src/api/settings.ts:
##########
@@ -129,9 +129,15 @@ export async function listAllDataSources(params: {
search?: string; type?: strin
page,
pageSize: DATA_SOURCE_EXPORT_PAGE_SIZE,
});
- allDataSources.push(...result.items);
- const total = result.total ?? allDataSources.length;
- if (result.items.length === 0 || allDataSources.length >= total) {
+ // A success envelope may still carry a null or item-less payload; treat
it as an
+ // empty page instead of crashing the whole export walk.
+ const items = Array.isArray(result?.items) ? result.items : [];
+ allDataSources.push(...items);
+ const total =
Review Comment:
Same truncation risk: a non-finite `result.total` with valid items causes
the walk to exit after one page because `total` collapses to
`allDataSources.length`. Data-source exports would silently lose entries beyond
the first page.
##########
web/src/services/topicService.ts:
##########
@@ -95,9 +95,15 @@ export const listAllTopics = async (params: TopicQuery =
{}): Promise<Topic[]> =
while (page <= MAX_EXPORT_PAGES) {
const result = await listTopicsPage({ ...params, page, pageSize:
EXPORT_PAGE_SIZE });
- topics.push(...result.items);
- const total = result.total ?? topics.length;
- if (result.items.length === 0 || topics.length >= total) return topics;
+ // A success envelope may still carry a null or item-less payload; treat
it as an
+ // empty page instead of crashing the whole export walk.
+ const items = Array.isArray(result?.items) ? result.items : [];
+ topics.push(...items);
+ const total =
Review Comment:
Same truncation risk as consumerService: if the server returns items but a
non-finite `total`, the fallback `total = topics.length` causes immediate exit
after one page. This silently drops data for large exports where the backend
sends a malformed total. A defensive log or a 'continue until empty page'
strategy would be safer.
##########
web/src/services/topicService.export.test.ts:
##########
@@ -0,0 +1,55 @@
+import { afterEach, describe, expect, it, vi } from 'vitest';
Review Comment:
This new test file is missing the Apache 2.0 license header that the
surrounding test files (e.g. `consumerService.test.ts`) include. This may cause
CI license-check failures.
##########
web/src/api/studioUsers.ts:
##########
@@ -63,9 +63,15 @@ export const listAllStudioUsers = async (
page,
pageSize: STUDIO_USER_EXPORT_PAGE_SIZE,
});
- allUsers.push(...result.items);
- const total = result.total ?? allUsers.length;
- if (result.items.length === 0 || allUsers.length >= total) return allUsers;
+ // A success envelope may still carry a null or item-less payload; treat
it as an
+ // empty page instead of crashing the whole export walk.
+ const items = Array.isArray(result?.items) ? result.items : [];
+ allUsers.push(...items);
+ const total =
Review Comment:
Same truncation risk: a non-finite `result.total` with valid items causes
immediate exit. Studio-user exports would silently drop users beyond the first
page.
##########
web/src/services/topicService.ts:
##########
@@ -95,9 +95,15 @@ export const listAllTopics = async (params: TopicQuery =
{}): Promise<Topic[]> =
while (page <= MAX_EXPORT_PAGES) {
const result = await listTopicsPage({ ...params, page, pageSize:
EXPORT_PAGE_SIZE });
- topics.push(...result.items);
- const total = result.total ?? topics.length;
- if (result.items.length === 0 || topics.length >= total) return topics;
+ // A success envelope may still carry a null or item-less payload; treat
it as an
+ // empty page instead of crashing the whole export walk.
+ const items = Array.isArray(result?.items) ? result.items : [];
Review Comment:
The null-safe pagination logic (items fallback, total validation, exit
guard) is duplicated identically across four files (`topicService.ts`,
`consumerService.ts`, `settings.ts`, `studioUsers.ts`). Consider extracting a
shared helper like `walkPages<T>(fetchPage, pageSize, maxPages)` to centralize
the tolerance logic and avoid drift as the pattern evolves.
--
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]
