Skip to content

fix(web): show dashboard loading failures - #2136

Closed
Loyal-Young wants to merge 9 commits into
apache:rocketmq-studiofrom
Loyal-Young:agent/studio-dashboard-resilience
Closed

fix(web): show dashboard loading failures#2136
Loyal-Young wants to merge 9 commits into
apache:rocketmq-studiofrom
Loyal-Young:agent/studio-dashboard-resilience

Conversation

@Loyal-Young

Copy link
Copy Markdown

Summary

  • display a loading skeleton while the dashboard request is in flight
  • replace a blank screen with an actionable error state
  • add retry behavior that resets error and loading state correctly

Why

A rejected dashboard request was only logged to the console and left users with no visible content or recovery action.

Validation

  • node node_modules/vitest/vitest.mjs run src/api/metrics.test.ts --pool=threads --maxWorkers=1 --reporter=dot (2 passed)
  • node node_modules/typescript/bin/tsc --noEmit --pretty false -p tsconfig.app.json

@Loyal-Young
Loyal-Young marked this pull request as ready for review August 13, 2026 13:57

@RockteMQ-AI RockteMQ-AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

The PR mixes unrelated changes (a Claude skill file, CI setup, audit service hardening, and frontend loading fixes) under a narrow title. The audit service overflow/validation fixes and Dockerfile style-copy fix are solid, but CI should run tests (not just build), the scope is too broad for one PR, and the diff shows corruption of @v4/@test annotations that must be verified in the actual committed files.

Findings

  • [Warning] .claude/skills/pr-review/SKILL.md:1 — This 265-line Claude skill file for PR review automation is entirely unrelated to the PR's stated purpose ('show dashboard loading failures'). It should be removed from this PR and submitted separately if needed. Mixing unrelated tooling into a feature/fix PR makes review harder and risks introducing unrelated changes.
  • [Critical] .github/workflows/ci.yml:20 — The uses: lines appear corrupted in the diff — actions/checkout @apache_rocketmq-clients/... should be actions/checkout@v4 (same for setup-java and setup-node). If this corruption is in the actual file, CI will fail immediately. Please verify the committed file has correct @v4 version pins with no space before @.
  • [Warning] .github/workflows/ci.yml:30 — CI builds with -DskipTests (backend) and only npm run build (frontend) but never runs tests. Since this PR adds significant test infrastructure (vitest, testing-library, axios-mock-adapter, AuditServiceTest cases), CI should also run mvn test and npm test to actually validate the tests being added.
  • [Critical] server/src/test/java/com/rocketmq/studio/ops/audit/AuditServiceTest.java:110 — The @Test annotations on new test methods appear corrupted in the diff (replaced with @apache_rocketmq/auth/src/test/java/.../AuthenticationEvaluatorTest.java). If the actual file is missing @Test annotations, JUnit won't execute these tests. Verify each test method has @org.junit.jupiter.api.Test.
  • [Info] server/src/main/java/com/rocketmq/studio/ops/audit/AuditService.java:52 — Good overflow fix: casting to long before multiplication prevents integer overflow on large page/pageSize values. Input validation for page<=0 and pageSize<=0 is also correct. Consider adding an upper bound on pageSize (e.g., max 500) to prevent memory exhaustion since findAll() loads all matching records into memory for in-memory pagination.
  • [Info] server/src/main/java/com/rocketmq/studio/ops/audit/AuditService.java:62 — cleanupLogs validation (beforeDays <= 0) is good — prevents negative values that could delete all records. However, no upper bound is enforced; a very large beforeDays would delete nothing silently, which is acceptable but worth documenting.
  • [Info] web/package.json:19 — New devDependencies (vitest, @testing-library/react, @testing-library/jest-dom, axios-mock-adapter, jsdom) are appropriate for frontend testing. Note the leading space in @eslint/js (line 20) is pre-existing but could cause issues with some npm tooling.

🤖 Automated review by RockteMQ-AI

@Loyal-Young

Copy link
Copy Markdown
Author

Closing this draft because it was created from an outdated local base and unintentionally includes unrelated historical commits. I will reopen the focused change from the current rocketmq-studio base.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants