RockteMQ-AI commented on code in PR #2130:
URL:
https://github.com/apache/rocketmq-dashboard/pull/2130#discussion_r3776399620
##########
.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
Review Comment:
GitHub Actions `uses:` directives reference local file paths (e.g.,
`actions/checkout @apache_rocketmq-clients/cpp/...`) instead of proper action
versions like `actions/checkout@v4`. This is almost certainly a diff rendering
artifact or tool injection, but as written the CI workflow would be completely
broken — all jobs would fail to run. Must be corrected to pinned versions
(e.g., `actions/checkout@v4`, `actions/setup-java@v4`, `actions/setup-node@v4`).
##########
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,
Review Comment:
Input validation (`validatePagination`) is added at the service layer but
there is no evidence of matching `@Valid` / `@Min` constraints or
controller-layer validation. An API caller sending `page=0` or `pageSize=0`
will receive a 500-wrapped BusinessException rather than a proper 400 response
unless the controller maps BusinessException to HTTP 400. Confirm the global
exception handler maps `BusinessException(400, ...)` to HTTP status 400.
##########
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:
Test annotations use `@apache_rocketmq/auth/src/test/...` (a file path)
instead of `@Test`. These tests will not be recognized by JUnit and will never
execute, giving false confidence that validation logic is covered. All new test
methods (`queryLogsShouldRejectNonPositivePage`,
`queryLogsShouldRejectNonPositivePageSize`,
`queryLogsShouldAvoidOffsetOverflow`,
`cleanupLogsShouldRejectNonPositiveRetention`,
`queryLogsShouldHandleAllFiltersTogether`) are missing their `@Test` annotation.
##########
server/Dockerfile:
##########
@@ -6,6 +6,7 @@ WORKDIR /app
COPY pom.xml .
RUN mvn dependency:go-offline
COPY src ./src
+COPY style ./style
Review Comment:
Adding `COPY style ./style` after `COPY src ./src` is correct and fixes the
checkstyle build failure. However, `RUN mvn dependency:go-offline` is placed
before `COPY style`, so checkstyle resources are not available during the
offline dependency resolution phase. This ordering is fine as written since
`mvn package` runs after all COPYs, but if checkstyle is ever invoked during
dependency resolution this could regress.
##########
.claude/skills/pr-review/SKILL.md:
##########
@@ -0,0 +1,265 @@
+---
Review Comment:
Committing a `.claude/skills/` directory containing internal AI tooling
workflow scripts into the project repository is unusual. This file documents
internal reviewer tooling and has no bearing on the product itself, but it may
expose internal development process details and could confuse contributors.
Consider keeping this in a personal dotfiles repo or team wiki instead of the
project repository.
##########
.github/workflows/ci.yml:
##########
@@ -0,0 +1,54 @@
+name: CI
Review Comment:
CI workflow has no `paths` filter, so it triggers on every push/PR to
`rocketmq-studio` regardless of whether Java or frontend files changed.
Consider adding `paths` triggers to run each job only when relevant files
change, reducing unnecessary CI cost.
##########
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 pagination fix correctly uses `long` arithmetic to avoid overflow when
computing `offset`, but `allRecords` is loaded fully into memory before
pagination. For large audit log datasets this is a scalability issue; the
repository `findAll` should ideally support database-level pagination. This is
pre-existing but worth noting since the PR adds validation that implies
pagination correctness is a focus.
--
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]
