Copilot commented on code in PR #4222:
URL: https://github.com/apache/hertzbeat/pull/4222#discussion_r3623470013
##########
hertzbeat-manager/src/main/java/org/apache/hertzbeat/manager/service/impl/StatusPageServiceImpl.java:
##########
@@ -242,7 +241,8 @@ private StatusPageHistory
combineOneDayStatusPageHistory(List<StatusPageHistory>
}
@Override
- public ComponentStatus queryComponentStatus(long id) {
+ public ComponentStatus queryComponentStatus(long id, int days) {
+ int historySpanDays = days - 1;
StatusPageComponent component = statusPageComponentDao.findById(id)
Review Comment:
`days` is not validated before computing `historySpanDays = days - 1`, so
invalid values (<=0) or very large values can produce incorrect windows and/or
expensive processing. Validate `days` (e.g., 1..90) before continuing.
##########
hertzbeat-manager/src/main/java/org/apache/hertzbeat/manager/service/impl/StatusPageServiceImpl.java:
##########
@@ -126,7 +124,8 @@ public StatusPageComponentInfo
queryStatusPageComponent(long id) {
}
@Override
- public List<ComponentStatus> queryComponentsStatus() {
+ public List<ComponentStatus> queryComponentsStatus(int days) {
+ int historySpanDays = days - 1;
List<StatusPageComponent> components =
statusPageComponentDao.findAll();
Review Comment:
`days` is used to derive `historySpanDays = days - 1` without any bounds
checking. Because this endpoint is public-facing, an extremely large (or
negative) `days` value can trigger very heavy queries/loops and unexpected time
windows. Add input validation (e.g., enforce 1..90) before proceeding.
##########
web-app/src/app/routes/status-public/status-public.component.ts:
##########
@@ -106,7 +137,7 @@ export class StatusPublicComponent implements OnInit {
showComponent() {
this.showMode = 'component';
- this.loadStatusPageOrg();
+ this.loadComponentStatus();
}
Review Comment:
`showComponent()` used to reload the org data (and set the page title)
before showing components. After this change it only reloads component status,
which can leave the org banner/title stale after spending time on the incident
view.
##########
web-app/src/app/routes/status-public/status-public.component.html:
##########
@@ -119,7 +128,7 @@ <h3 style="margin: 0; padding: 0; font-size: 16px">{{
statusOrg.name }}</h3>
<span nz-icon style="margin-left: 5px" nzType="stop"
nzTheme="outline"></span>
</div>
</div>
- <div style="margin-left: 10px; margin-top: 10px; margin-bottom: 10px;
display: flex">
+ <div style="margin-left: 10px; margin-top: 10px; margin-bottom: 10px;
display: flex; gap: 1%">
Review Comment:
Using a percentage-based `gap: 1%` scales with the number of history blocks;
at 90 days the gaps alone can consume most of the row width, making blocks
extremely small/unusable. Prefer a fixed gap (px/rem) for consistent layout
across different day counts.
##########
hertzbeat-manager/src/test/java/org/apache/hertzbeat/manager/controller/StatusPagePublicControllerTest.java:
##########
@@ -88,7 +88,7 @@ public void testQueryStatusPageOrgNotFound() throws Exception
{
public void testQueryStatusPageComponent() throws Exception {
List<ComponentStatus> componentStatusList =
Collections.singletonList(new ComponentStatus());
-
when(statusPageService.queryComponentsStatus()).thenReturn(componentStatusList);
+
when(statusPageService.queryComponentsStatus(30)).thenReturn(componentStatusList);
Review Comment:
The new `days` query parameter behavior is only tested for the default (no
param) path. Add a test case that passes a non-default `days` value (e.g.
`?days=7`) so regressions in request parameter binding/defaulting are caught.
##########
hertzbeat-manager/src/main/java/org/apache/hertzbeat/manager/service/impl/StatusPageServiceImpl.java:
##########
@@ -154,7 +153,7 @@ public List<ComponentStatus> queryComponentsStatus() {
long preTimestamp = now
Review Comment:
The comment immediately above this block still says "30d" even though the
history span is now driven by the `days` parameter, which is misleading. Update
the comment to reflect the configurable span.
##########
hertzbeat-manager/src/test/java/org/apache/hertzbeat/manager/service/impl/StatusPageServiceImplTimeTest.java:
##########
@@ -99,7 +99,7 @@ void testMidnightBoundary() {
when(historyDao.findStatusPageHistoriesByComponentIdAndTimestampBetween(anyLong(),
anyLong(), anyLong()))
.thenReturn(List.of(before, after));
- List<ComponentStatus> result = service.queryComponentsStatus();
+ List<ComponentStatus> result = service.queryComponentsStatus(30);
assertEquals(30, result.get(0).getHistory().size());
}
Review Comment:
Service tests still only exercise `queryComponentsStatus(30)`. Since `days`
is now configurable, add assertions for at least one additional supported value
(e.g. 1 and/or 90) so the new branching/loop bounds are covered.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
