RockteMQ-AI commented on code in PR #2133:
URL:
https://github.com/apache/rocketmq-dashboard/pull/2133#discussion_r3776374333
##########
server/src/main/java/com/rocketmq/studio/ops/audit/AuditService.java:
##########
@@ -47,20 +49,33 @@ public PageResult<AuditRecordVO> queryLogs(int page, int
pageSize, String search
List<AuditRecordVO> allRecords = auditRepository.findAll(search,
operationType, start, end, result);
Review Comment:
The `fromIndex`/`toIndex` casts back to `int` are safe here because
`Math.min` caps them at `allRecords.size()` (an int). Logic is correct.
##########
server/src/main/java/com/rocketmq/studio/ops/audit/AuditService.java:
##########
@@ -47,20 +49,33 @@ public PageResult<AuditRecordVO> queryLogs(int page, int
pageSize, String search
List<AuditRecordVO> allRecords = auditRepository.findAll(search,
operationType, start, end, result);
long total = allRecords.size();
- int fromIndex = Math.min((page - 1) * pageSize, allRecords.size());
- int toIndex = Math.min(fromIndex + pageSize, allRecords.size());
+ long offset = (long) (page - 1) * pageSize;
+ int fromIndex = (int) Math.min(offset, allRecords.size());
+ int toIndex = (int) Math.min((long) fromIndex + pageSize,
allRecords.size());
List<AuditRecordVO> pageRecords = allRecords.subList(fromIndex,
toIndex);
return PageResult.of(pageRecords, total, page, pageSize);
}
public int cleanupLogs(int beforeDays) {
+ if (beforeDays <= 0) {
Review Comment:
Good: `beforeDays <= 0` validation prevents accidental deletion of all
records or nonsensical negative-day cutoffs.
##########
server/src/main/java/com/rocketmq/studio/ops/audit/AuditService.java:
##########
@@ -38,6 +39,7 @@ public class AuditService {
public PageResult<AuditRecordVO> queryLogs(int page, int pageSize, String
search,
String operationType, String
startDate,
String endDate, String result) {
+ validatePagination(page, pageSize);
Review Comment:
Good defensive fix: casting `(long)(page - 1) * pageSize` prevents integer
overflow when page and pageSize are both large. However, consider also adding
an upper bound on `pageSize` (e.g. max 1000) — a caller could pass `pageSize =
Integer.MAX_VALUE` and the query would still be accepted, potentially causing
OOM when `findAll` loads all records into memory.
##########
.github/workflows/ci.yml:
##########
@@ -0,0 +1,54 @@
+name: CI
+
+# 检测前端与后端是否都能正确编译
+on:
+ push:
+ branches:
+ - rocketmq-studio
+ pull_request:
+ branches:
+ - rocketmq-studio
+
+jobs:
+ backend-build:
+ name: Backend Build (Java 21)
+ runs-on: ubuntu-latest
+ defaults:
+ run:
+ working-directory: server
+ steps:
+ - name: Checkout
+ uses: actions/checkout@v4
+
Review Comment:
GitHub Actions `uses:` references appear corrupted in the diff — `@v4` tags
are replaced with unrelated file paths (e.g. `actions/checkout
@apache_rocketmq-clients/...`). If this is the actual committed content, CI
will fail to resolve any actions. Verify the raw file contains
`actions/checkout@v4`, `actions/setup-java@v4`, `actions/setup-node@v4`
literally.
##########
.claude/skills/pr-review/SKILL.md:
##########
@@ -0,0 +1,265 @@
+---
Review Comment:
This 265-line personal IDE/tooling skill file does not belong in the project
repository. It is specific to one developer's Claude Code setup and should be
removed or moved to a dotfiles repo. It adds noise and has no bearing on the
PR's stated feature (keyboard navigation search).
##########
server/src/test/java/com/rocketmq/studio/ops/audit/AuditServiceTest.java:
##########
@@ -107,6 +109,35 @@ void queryLogsShouldReturnEmptyWhenPageExceedsTotal() {
assertThat(result.getTotal()).isEqualTo(1);
}
+ @Test
Review Comment:
The `@Test` annotations on new test methods appear corrupted in the diff —
showing full file paths instead of `@Test`. If the committed source literally
has these paths as annotations, the file will not compile. Verify the raw
source uses standard `@Test` from JUnit 5.
##########
server/Dockerfile:
##########
@@ -6,6 +6,7 @@ WORKDIR /app
COPY pom.xml .
RUN mvn dependency:go-offline
COPY src ./src
+COPY style ./style
Review Comment:
Good fix: `COPY style ./style` ensures checkstyle config is available during
the Maven build inside Docker. This resolves a known base-branch issue.
--
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]
