Add Dashboard page as first per-agency sidebar entry - #44
Merged
Merged
Conversation
Removes the unused body/#mainDiv/#title/#subtitle/.choicesList/#dataTable rules from general.css and migrates apiCalls/index.jsp and vertStopsScheduleApiParams.jsp off those legacy classes onto Tailwind utilities, matching the rest of the redesigned pages.
Combines the Active Blocks summary, Server Status monitor grid, and the five largest DB tables into a single overview page. The summary strip and monitor grid are extracted into shared tags so the original Active Blocks and Server Status pages render the same markup; the active-blocks Stimulus controller short-circuits its route fetch when accordion targets aren't present so the dashboard can mount the controller for the summary alone.
📝 WalkthroughWalkthroughIntroduces a new dashboard feature for transit agencies that displays server monitoring status and top disk-consuming database tables. Includes a new dashboard JSP page, supporting JSP tag components for rendering active blocks summaries and server status grids, a database API for querying top tables, i18n support, CSS cleanup, and refactors existing status views to use reusable tag components. Changes
Sequence DiagramsequenceDiagram
actor User
participant Browser
participant DashboardJSP as Dashboard JSP
participant ServerStatusFactory as ServerStatusFactory
participant RMICore as RMI Core
participant DbDiskSpace as DbDiskSpaceQuery
participant Database as Database
User->>Browser: GET /dashboard?a=agencyId
Browser->>DashboardJSP: Request dashboard page
DashboardJSP->>ServerStatusFactory: getServerStatus()
ServerStatusFactory->>RMICore: Fetch monitoring results
RMICore-->>ServerStatusFactory: monitorResults + stats
ServerStatusFactory-->>DashboardJSP: Server status data
DashboardJSP->>DbDiskSpace: getTopTables(agencyId, 5)
DbDiskSpace->>Database: Execute TOP_TABLES_SQL query
Database-->>DbDiskSpace: Table size rows
DbDiskSpace-->>DashboardJSP: List<TableSize> records
DashboardJSP->>Browser: Render dashboard with status grid & disk usage table
Browser-->>User: Display dashboard UI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 2/3 reviews remaining, refill in 20 minutes. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
transitclockWebapp/src/main/java/org/transitclock/reports/DbDiskSpaceQuery.java (1)
105-109: Consider returning an immutable list fromrun().
q.rowsis mutable and exposed directly to callers. Returning an immutable copy avoids accidental downstream mutation.♻️ Proposed hardening
static List<TableSize> run(String agencyId, String sql) throws SQLException { TopTablesQuery q = new TopTablesQuery(agencyId); q.doQuery(sql); - return q.rows; + return List.copyOf(q.rows); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transitclockWebapp/src/main/java/org/transitclock/reports/DbDiskSpaceQuery.java` around lines 105 - 109, The run method currently returns the mutable q.rows list from TopTablesQuery, exposing internal state; modify DbDiskSpaceQuery.run(String,String) to return an immutable copy (e.g., use List.copyOf(q.rows) or Collections.unmodifiableList(new ArrayList<>(q.rows))) so callers cannot mutate the internal list while still preserving contents; locate the run method and replace the direct return of q.rows with the immutable wrapper or copy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@transitclockWebapp/src/main/webapp/dashboard/index.jsp`:
- Around line 81-83: The JSP is inserting unescaped request/DB values directly
into attributes (e.g., agencyId in the anchor href and t.tableName used
elsewhere), which can lead to XSS; fix by building hrefs via JSTL c:url with
param (use c:url
value="${pageContext.request.contextPath}/status/activeBlocks.jsp" and <c:param
name="a" value="${agencyId}"/>) so agencyId is URL-encoded, and escape any
attribute/text output from DB like t.tableName with JSTL fn:escapeXml (or the
equivalent server-side escaping utility) before inserting into attributes or
text nodes; apply the same changes to the other occurrences referenced (lines
~100-103, 115-118, 136) so all dynamic attributes are URL-encoded or
XML-escaped.
- Line 140: The width expression can divide by zero when maxBytes == 0; update
the JSP expression that builds the style width (the inline style using
${(t.bytes * 100.0) / maxBytes} ) to guard against zero by computing the
percentage only when maxBytes > 0 (otherwise use 0%), e.g. replace the raw
division with a conditional expression that yields (t.bytes * 100.0) / maxBytes
if maxBytes > 0, else 0, so the resulting style attribute always contains a
valid width value.
- Around line 19-21: WebAgency.getCachedWebAgency(agencyId) may return null and
the code currently dereferences it in pageContext.setAttribute(...,
WebAgency.getCachedWebAgency(agencyId).getAgencyName()), causing a 500; update
the JSP to null-check the result of WebAgency.getCachedWebAgency(agencyId) (or
store it in a local variable) before calling getAgencyName(), and set a safe
fallback (e.g., empty string or "Unknown Agency") into pageContext via
pageContext.setAttribute("agencyName", ...) when the cached agency is null so
the page does not error on invalid agencyId (refer to
WebAgency.getCachedWebAgency, getAgencyName, and agencyId).
In `@transitclockWebapp/src/main/webapp/WEB-INF/tags/activeBlocksSummary.tag`:
- Line 13: Replace the hard-coded English label in activeBlocksSummary.tag (the
<span class="text-xs text-gray-500 font-medium">total</span>) with a
localization lookup (use the same message key naming used elsewhere in this
component), and add a matching "div.total" entry to each locale bundle so Polish
and other locales render correctly; ensure the tag uses the same resource bundle
reference pattern as other labels in activeBlocksSummary.tag.
---
Nitpick comments:
In
`@transitclockWebapp/src/main/java/org/transitclock/reports/DbDiskSpaceQuery.java`:
- Around line 105-109: The run method currently returns the mutable q.rows list
from TopTablesQuery, exposing internal state; modify
DbDiskSpaceQuery.run(String,String) to return an immutable copy (e.g., use
List.copyOf(q.rows) or Collections.unmodifiableList(new ArrayList<>(q.rows))) so
callers cannot mutate the internal list while still preserving contents; locate
the run method and replace the direct return of q.rows with the immutable
wrapper or copy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 444fa892-23ec-4b6c-a26d-c770ee7a7f36
📒 Files selected for processing (13)
transitclockWebapp/src/main/java/org/transitclock/reports/DbDiskSpaceQuery.javatransitclockWebapp/src/main/resources/org/transitclock/i18n/text.propertiestransitclockWebapp/src/main/resources/org/transitclock/i18n/text_pl.propertiestransitclockWebapp/src/main/webapp/WEB-INF/tags/activeBlocksSummary.tagtransitclockWebapp/src/main/webapp/WEB-INF/tags/layout.tagtransitclockWebapp/src/main/webapp/WEB-INF/tags/serverStatusGrid.tagtransitclockWebapp/src/main/webapp/css/general.csstransitclockWebapp/src/main/webapp/dashboard/index.jsptransitclockWebapp/src/main/webapp/javascript/controllers/active_blocks_controller.jstransitclockWebapp/src/main/webapp/reports/apiCalls/index.jsptransitclockWebapp/src/main/webapp/reports/apiCalls/vertStopsScheduleApiParams.jsptransitclockWebapp/src/main/webapp/status/activeBlocks.jsptransitclockWebapp/src/main/webapp/status/serverStatus.jsp
💤 Files with no reviewable changes (1)
- transitclockWebapp/src/main/webapp/css/general.css
Comment on lines
+19
to
+21
| pageContext.setAttribute("agencyId", agencyId); | ||
| pageContext.setAttribute("agencyName", WebAgency.getCachedWebAgency(agencyId).getAgencyName()); | ||
| pageContext.setAttribute("now", Time.timeStrNoTimeZone(new Date())); |
There was a problem hiding this comment.
Guard unknown agencies before dereferencing cached metadata.
WebAgency.getCachedWebAgency(agencyId) can be null for invalid a values, which will 500 the page when calling .getAgencyName().
🛠️ Proposed fix
<%@ page import="org.transitclock.db.webstructs.WebAgency" %>
@@
String agencyId = request.getParameter("a");
if (agencyId == null || agencyId.isEmpty()) {
response.getWriter().write("You must specify agency in query string (e.g. ?a=mbta)");
return;
}
+WebAgency agency = WebAgency.getCachedWebAgency(agencyId);
+if (agency == null) {
+ response.setStatus(404);
+ response.getWriter().write("Unknown agency: " + agencyId);
+ return;
+}
@@
-pageContext.setAttribute("agencyName", WebAgency.getCachedWebAgency(agencyId).getAgencyName());
+pageContext.setAttribute("agencyName", agency.getAgencyName());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@transitclockWebapp/src/main/webapp/dashboard/index.jsp` around lines 19 - 21,
WebAgency.getCachedWebAgency(agencyId) may return null and the code currently
dereferences it in pageContext.setAttribute(...,
WebAgency.getCachedWebAgency(agencyId).getAgencyName()), causing a 500; update
the JSP to null-check the result of WebAgency.getCachedWebAgency(agencyId) (or
store it in a local variable) before calling getAgencyName(), and set a safe
fallback (e.g., empty string or "Unknown Agency") into pageContext via
pageContext.setAttribute("agencyName", ...) when the cached agency is null so
the page does not error on invalid agencyId (refer to
WebAgency.getCachedWebAgency, getAgencyName, and agencyId).
Comment on lines
+81
to
+83
| <a href="${pageContext.request.contextPath}/status/activeBlocks.jsp?a=${agencyId}" | ||
| class="inline-flex items-center gap-1.5 px-3 py-1.5 rounded-md text-xs font-medium text-gray-700 bg-white border border-gray-300 hover:bg-gray-50"> | ||
| View details → |
There was a problem hiding this comment.
Avoid raw attribute interpolation for request/DB values.
agencyId (request-derived) and t.tableName (DB-derived) are inserted directly into HTML attributes. Encode URLs and escape attribute text to prevent attribute-break/XSS paths.
🔒 Proposed fix
+<c:url var="activeBlocksUrl" value="/status/activeBlocks.jsp">
+ <c:param name="a" value="${agencyId}" />
+</c:url>
+<c:url var="serverStatusUrl" value="/status/serverStatus.jsp">
+ <c:param name="a" value="${agencyId}" />
+</c:url>
+<c:url var="dbDiskSpaceUrl" value="/status/dbDiskSpace.jsp">
+ <c:param name="a" value="${agencyId}" />
+</c:url>
@@
- <a href="${pageContext.request.contextPath}/status/activeBlocks.jsp?a=${agencyId}"
+ <a href="${pageContext.request.contextPath}${activeBlocksUrl}"
@@
- <a href="${pageContext.request.contextPath}/status/serverStatus.jsp?a=${agencyId}"
+ <a href="${pageContext.request.contextPath}${serverStatusUrl}"
@@
- <a href="${pageContext.request.contextPath}/status/dbDiskSpace.jsp?a=${agencyId}"
+ <a href="${pageContext.request.contextPath}${dbDiskSpaceUrl}"
@@
- <span class="font-mono text-gray-900 truncate" title="${t.tableName}"><c:out value="${t.tableName}"/></span>
+ <span class="font-mono text-gray-900 truncate"><c:out value="${t.tableName}"/></span>Also applies to: 100-103, 115-118, 136-136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@transitclockWebapp/src/main/webapp/dashboard/index.jsp` around lines 81 - 83,
The JSP is inserting unescaped request/DB values directly into attributes (e.g.,
agencyId in the anchor href and t.tableName used elsewhere), which can lead to
XSS; fix by building hrefs via JSTL c:url with param (use c:url
value="${pageContext.request.contextPath}/status/activeBlocks.jsp" and <c:param
name="a" value="${agencyId}"/>) so agencyId is URL-encoded, and escape any
attribute/text output from DB like t.tableName with JSTL fn:escapeXml (or the
equivalent server-side escaping utility) before inserting into attributes or
text nodes; apply the same changes to the other occurrences referenced (lines
~100-103, 115-118, 136) so all dynamic attributes are URL-encoded or
XML-escaped.
| <span class="text-gray-500 font-mono tabular-nums shrink-0"><c:out value="${t.prettySize}"/></span> | ||
| </div> | ||
| <div class="h-2 rounded-full bg-gray-100 overflow-hidden"> | ||
| <div class="h-full bg-brand-accent rounded-full" style="width: ${(t.bytes * 100.0) / maxBytes}%"></div> |
There was a problem hiding this comment.
Protect the width calculation against zero maxBytes.
If all rows resolve to 0 bytes, this computes a divide-by-zero and produces invalid width output.
🧮 Proposed fix
- <div class="h-full bg-brand-accent rounded-full" style="width: ${(t.bytes * 100.0) / maxBytes}%"></div>
+ <div class="h-full bg-brand-accent rounded-full"
+ style="width: ${maxBytes gt 0 ? (t.bytes * 100.0) / maxBytes : 0}%"></div>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <div class="h-full bg-brand-accent rounded-full" style="width: ${(t.bytes * 100.0) / maxBytes}%"></div> | |
| <div class="h-full bg-brand-accent rounded-full" | |
| style="width: ${maxBytes gt 0 ? (t.bytes * 100.0) / maxBytes : 0}%"></div> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@transitclockWebapp/src/main/webapp/dashboard/index.jsp` at line 140, The
width expression can divide by zero when maxBytes == 0; update the JSP
expression that builds the style width (the inline style using ${(t.bytes *
100.0) / maxBytes} ) to guard against zero by computing the percentage only when
maxBytes > 0 (otherwise use 0%), e.g. replace the raw division with a
conditional expression that yields (t.bytes * 100.0) / maxBytes if maxBytes > 0,
else 0, so the resulting style attribute always contains a valid width value.
| <div class="text-[11px] font-semibold uppercase tracking-wider text-gray-500"><fmt:message key="div.blocks"/></div> | ||
| <div class="flex items-baseline gap-1.5 mt-0.5"> | ||
| <span data-field="total-blocks" class="text-[22px] font-bold tabular-nums text-gray-900">—</span> | ||
| <span class="text-xs text-gray-500 font-medium">total</span> |
There was a problem hiding this comment.
Localize the remaining hard-coded label.
total is the only visible label in this shared component that is still English, so Polish and future locales will render it inconsistently.
Suggested fix
- <span class="text-xs text-gray-500 font-medium">total</span>
+ <span class="text-xs text-gray-500 font-medium"><fmt:message key="div.total"/></span>Add the matching div.total entry to the locale bundles as well.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <span class="text-xs text-gray-500 font-medium">total</span> | |
| <span class="text-xs text-gray-500 font-medium"><fmt:message key="div.total"/></span> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@transitclockWebapp/src/main/webapp/WEB-INF/tags/activeBlocksSummary.tag` at
line 13, Replace the hard-coded English label in activeBlocksSummary.tag (the
<span class="text-xs text-gray-500 font-medium">total</span>) with a
localization lookup (use the same message key naming used elsewhere in this
component), and add a matching "div.total" entry to each locale bundle so Polish
and other locales render correctly; ensure the tag uses the same resource bundle
reference pattern as other labels in activeBlocksSummary.tag.
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/dashboard/index.jsp) that combines the Active Blocks summary, Server Status monitor grid, and the five largest DB tables (horizontal-bar chart + table) onto one overview page. Wired as the first link under each agency in the sidebar.<t:>tags (activeBlocksSummary.tag,serverStatusGrid.tag) so the original/status/activeBlocks.jspand/status/serverStatus.jsppages render the same markup as the dashboard.DbDiskSpaceQuery.getTopTables(agencyId, limit)returning a typedList<TableSize>. The dashboard renders bars/table directly from these rows; no new charting dependency.active-blocksStimulus controller short-circuit route fetching when the accordion/route-template targets are absent so the dashboard can mount the controller for the summary strip alone.Test plan
mvn -pl transitclockWebapp -am package -DskipTestsbuilds clean.mvn -pl transitclockWebapp test) — theDbDiskSpaceQueryTestand JSP audit tests are green. (One pre-existingDbDiskSpaceResourceTestfailure onNoClassDefFound com/fasterxml/jackson/databind/ObjectMapperis unrelated to this PR; it was introduced inb972cee3against the webapp's deliberately-Jackson-excluded runtime — flagged below.)/dashboard/index.jsp?a=1renders the three sections with live data (verified in browser via Playwright; zero console errors)./status/activeBlocks.jsp?a=1regression check — full page including the route accordion still renders and the Stimulus controller fetches both summary and per-route data with no console errors./status/serverStatus.jsp?a=1regression check — monitor grid renders identically./status/dbDiskSpace.jsp?a=1regression check — Google Charts chart and tables still render.Review notes (non-blocking, for reviewer judgment)
/status/serverStatus.jsp. The sharedserverStatusGrid.tagemits<h3>for the alert and per-monitor headings. On the dashboard the page's own<h1>and section<h2>make this correct, but onserverStatus.jsp(which has only an<h1>) it skips an h2. JSP EL can't expand inside element names, so making this a tag attribute would mean a<c:choose>per heading. Leaving as-is for now; happy to add the choose if reviewer prefers strict heading hierarchy.getTopTables. The existingDbDiskSpaceQueryTestcoversgetTotalsJson/getDetailsJson. A unit test pinning the LIMIT clamp at the bounds and theaddRowmapping intoTableSizewould catch future drift; leaving as a follow-up.TableSize.prettySizefield couples the API surface to Postgrespg_size_prettyformatting. Fine for the current single consumer but worth revisiting if a second consumer ever wants different formatting/locale; aBytes.humanize(long)helper plus dropping the field would be the cleanup.GenericQuerystatic-Connection.org.transitclock.db.GenericQuerykeeps astatic Connectionreassigned by every constructor and never closed. The dashboard adds another concurrent caller (getTopTablesalongside the existinggetTotalsJson), making the latent thread-safety/leak concern a little more reachable. Out of scope for this PR.DbDiskSpaceResourceTest.happyPath_wrapsBothJsonsInEnvelopefails withNoClassDefFound com/fasterxml/jackson/databind/ObjectMapper. The webapppom.xmldeliberately excludesjersey-media-json-jackson(per its own comment) to keep Hibernate's Jackson chain at 2.8.9. Worth a separate fix to either provide Jackson at test scope only or remove theObjectMapper.readTreeline.Summary by CodeRabbit
Release Notes
New Features
Localization
Refactor