New Pull Request for Corrected Commit withtout corrupt files issue #2792#3083
New Pull Request for Corrected Commit withtout corrupt files issue #2792#3083MomoWorlData wants to merge 2 commits intobluewave-labs:developfrom
Conversation
MomoWorlData
requested review from
Br0wnHammer,
Owaiseimdad,
ajhollid and
karenvicent
as code owners
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
API Routing Configuration server/src/config/routes.js |
Imports and mounts the new SummaryRoutes module to extend the V1 API routing. |
Summary Endpoint Implementation server/src/routes/v1/summaryRoute.js |
New route module defining a GET /api/v1/summary endpoint that concurrently queries Monitor and Incident models to compute uptime, infrastructure, and incident counts. Includes error handling with HTTP 500 response on failure. |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
- Review focus areas:
- Verify the filtering logic for uptime (non-hardware, active) vs. infrastructure (hardware, active) matches intended business rules
- Confirm concurrent database queries are properly awaited and error handling captures both query failures
- Validate that the query filters and field selections align with the Monitor and Incident data model structure
Poem
🐰 A widget is born, swift and bright,
Dashboard summaries taking flight,
Uptime, infrastructure, incidents too,
Summary counts for me and you!
Homepage rejoices, the status is gleam. ✨
Pre-merge checks and finishing touches
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Title check | The title references issue #2792 and previous PR #3081 but is unclear about the primary change; it mentions 'corrupt files' which is not evident in the actual code changes. | Change the title to clearly describe the main change: 'Add homepage widget API endpoint for summary metrics' or similar. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description check | ✅ Passed | The description adequately explains the changes, includes the issue number (#2792), identifies the files modified/added, and provides testing instructions. |
| Linked Issues check | ✅ Passed | The code changes successfully implement the homepage widget requirements: the summaryRoute.js exposes uptime, infrastructure, and incidents counts as specified in issue #2792. |
| Out of Scope Changes check | ✅ Passed | All changes directly support the homepage widget feature; no unrelated modifications detected in the route configuration or new endpoint implementation. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
server/src/routes/v1/summaryRoute.js (1)
7-19: Consider following the established controller pattern.This route handler is defined inline, but all other routes in this codebase follow a class-based controller pattern (see routes.js lines 21-32 where controllers are instantiated with
new AuthRoutes(controller), etc.).Consider refactoring to follow the established pattern for consistency and maintainability.
server/src/config/routes.js (1)
46-46: Pattern inconsistency: SummaryRoutes doesn't follow the controller class pattern.All other routes in this file are instantiated as controller classes (lines 21-32, e.g.,
new AuthRoutes(controllers.authController)), butSummaryRoutesis imported and used directly. This breaks the established architectural pattern.Consider refactoring
summaryRoute.jsto follow the same controller-based pattern for consistency and maintainability.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/src/config/routes.js(2 hunks)server/src/routes/v1/summaryRoute.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
server/src/routes/v1/summaryRoute.js (2)
server/src/db/v1/modules/monitorModuleQueries.js (1)
req(1079-1079)server/src/db/v1/models/Incident.js (1)
Incident(74-74)
🔇 Additional comments (1)
server/src/config/routes.js (1)
15-15: LGTM: Import statement is correct.The import is properly structured and includes a helpful comment.
| app.use("/api/v1/notifications", verifyJWT, notificationRoutes.getRouter()); | ||
| app.use("/api/v1/diagnostic", verifyJWT, diagnosticRoutes.getRouter()); | ||
| app.use("/api/v1/incidents", verifyJWT, incidentRoutes.getRouter()); | ||
| app.use(SummaryRoutes); //used for Homepage widget |
There was a problem hiding this comment.
Critical: Add authentication middleware to protect internal metrics.
The SummaryRoutes is mounted without authentication, making internal system metrics (monitor counts, incident counts) publicly accessible. All other non-public routes in this file use verifyJWT middleware for protection (see lines 35-45).
Exposing these metrics without authentication is a security risk as it reveals:
- The number and status of monitored services
- Infrastructure details (hardware monitor counts)
- Active incident counts
This information could be valuable to attackers for reconnaissance.
Apply this diff to add authentication:
- app.use(SummaryRoutes); //used for Homepage widget
+ app.use("/api/v1", verifyJWT, SummaryRoutes); //used for Homepage widgetNote: You'll also need to update the route definition in summaryRoute.js line 7 from /api/v1/summary to just /summary since the /api/v1 prefix will be handled by the mount point.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In server/src/config/routes.js around line 46, SummaryRoutes is mounted without
authentication; update the mount to use the existing verifyJWT middleware (same
pattern as lines 35-45) so SummaryRoutes is protected (e.g., app.use('/api/v1',
verifyJWT, SummaryRoutes) or app.use(verifyJWT, SummaryRoutes) consistent with
surrounding mounts). Also update server/src/routes/summaryRoute.js at line 7 to
change the route path from '/api/v1/summary' to '/summary' so it relies on the
mount prefix for '/api/v1' and does not duplicate the prefix.
| router.get("/api/v1/summary", async (req, res) => { | ||
| try { | ||
| const [uptime, infrastructure, incidents] = await Promise.all([ | ||
| Monitor.countDocuments({ status: true, type: { $ne: "hardware" } }), | ||
| Monitor.countDocuments({ status: true, type: "hardware" }), | ||
| Incident.countDocuments({ status: true }) | ||
| ]); | ||
| res.json({ uptime, infrastructure, incidents }); | ||
| } catch (error) { | ||
| console.error("Erreur /api/v1/summary :", error); | ||
| res.status(500).json({ error: "Erreur interne" }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Critical: Add authentication middleware and verify field semantics.
This endpoint has several critical issues:
-
Authentication missing: The endpoint will be mounted without authentication (see routes.js line 46), exposing internal system metrics publicly. All other non-public routes use
verifyJWTmiddleware for protection. -
Field semantics unclear: The query uses
status: true, but the PR objectives state "services that are currently up". Verify thatstatus: trueactually means "up/active" in your Monitor and Incident schemas, not just "enabled". Ifstatusmeans "enabled" rather than "currently up", this query will return incorrect counts. -
French error messages: Lines 16-17 contain French text ("Erreur /api/v1/summary", "Erreur interne"), which violates English-first conventions and the i18n requirements mentioned in the PR checklist.
Apply this diff to fix the error messages:
} catch (error) {
- console.error("Erreur /api/v1/summary :", error);
- res.status(500).json({ error: "Erreur interne" });
+ console.error("Error in /api/v1/summary:", error);
+ res.status(500).json({ error: "Internal server error" });
}For authentication, see the comment on routes.js line 46. For field semantics, run the following verification:
#!/bin/bash
# Verify the schema definition for Monitor and Incident models to confirm field semantics
echo "=== Checking Monitor model schema ==="
rg -A 10 "status.*:" server/src/db/v1/models/Monitor.js
echo "=== Checking Incident model schema ==="
rg -A 10 "status.*:" server/src/db/v1/models/Incident.js
echo "=== Checking if 'isActive' or similar field exists ==="
rg -n "isActive|isUp|active" server/src/db/v1/models/🤖 Prompt for AI Agents
In server/src/routes/v1/summaryRoute.js around lines 7-19, add the verifyJWT
middleware to the route so it requires authentication, replace the console.error
French messages with English (e.g., "Error /api/v1/summary" and "Internal server
error") and use the central logger instead of console where available; then
verify the Monitor and Incident model semantics (run the provided rg checks) and
if the models use a different field for current up-state (e.g., isUp, isActive)
change the query predicates from status: true to the correct field (or combine
checks like { status: true, isUp: true } if both are required) so the counts
reflect services currently up rather than merely enabled.
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Code Review by LlamaPReview
🎯 TL;DR & Recommendation
Recommendation: Request Changes
This PR introduces a new API endpoint for Homepage widget integration but has critical security vulnerabilities and bugs that could expose sensitive data and provide incorrect metrics.
📄 Documentation Diagram
This diagram documents the new summary API endpoint workflow for Homepage widget integration.
sequenceDiagram
participant Client as Homepage Widget
participant Router as Express Router
participant MongoDB as MongoDB
Client->>Router: GET /api/v1/summary
activate Router
note over Router: PR #35;3083 added this new endpoint
par Concurrent Queries
Router->>MongoDB: Query Monitor (status=true, type≠hardware)
MongoDB-->>Router: uptime count
and
Router->>MongoDB: Query Monitor (status=true, type=hardware)
MongoDB-->>Router: infrastructure count
and
Router->>MongoDB: Query Incident (status logic)
MongoDB-->>Router: incidents count
end
Router-->>Client: 200 { uptime, infrastructure, incidents }
deactivate Router
🌟 Strengths
- Adds a useful new endpoint to support Homepage widget functionality as requested in issue #2792.
- Implements concurrent database queries for efficiency.
| Priority | File | Category | Impact Summary (≤12 words) | Anchors |
|---|---|---|---|---|
| P1 | server/src/config/routes.js | Architecture | Bypasses authentication, exposing metrics without security. | path:server/src/config/routes.js, search:app.use |
| P1 | server/src/routes/v1/summaryRoute.js | Bug | Incorrectly counts incidents, breaking core business logic. | path:server/src/routes/v1/summaryRoute.js, search:Incident.countDocuments |
| P2 | server/src/routes/v1/summaryRoute.js | Architecture | Route path duplication causes confusion and potential routing issues. | path:server/src/routes/v1/summaryRoute.js, method:get |
| P2 | server/src/routes/v1/summaryRoute.js | Maintainability | French error messages create inconsistency in the codebase. | path:server/src/routes/v1/summaryRoute.js, search:Erreur |
| P2 | server/src/config/routes.js | Architecture | Route registration order inconsistent, harming maintainability. | path:server/src/config/routes.js, search:SummaryRoutes |
| P2 | server/src/routes/v1/summaryRoute.js | Testing | Lacks comprehensive error handling and validation patterns. | path:server/src/routes/v1/summaryRoute.js, method:get, search:Promise.all |
🔍 Notable Themes
- Security and Authentication: The endpoint is mounted without the standard JWT middleware, violating the application's security model and exposing system metrics.
- Error Handling and Consistency: Multiple issues with error messages, route definitions, and code organization suggest a need for better adherence to existing patterns to improve maintainability.
📈 Risk Diagram
This diagram illustrates the authentication bypass risk and incorrect incident counting in the new summary endpoint.
sequenceDiagram
participant Client as Homepage Widget
participant ExpressApp as Express App
participant Auth as Auth Middleware
participant SummaryRouter as Summary Router
participant MongoDB as MongoDB
Client->>ExpressApp: Request to /api/v1/summary
note over ExpressApp, Auth: R1(P1): Authentication bypass - verifyJWT middleware not applied
ExpressApp->>SummaryRouter: Route directly (app.use without path)
SummaryRouter->>MongoDB: Query incidents with {status: true}
note over SummaryRouter: R2(P1): Incorrect query - should check for open incidents
MongoDB-->>SummaryRouter: Return count
SummaryRouter-->>Client: Response with potentially incorrect data
💡 Have feedback? We'd love to hear it in our GitHub Discussions.
✨ This review was generated by LlamaPReview Advanced, which is free for all open-source projects. Learn more.
| import NotificationRoutes from "../routes/v1/notificationRoute.js"; | ||
| import SummaryRoutes from "../routes/v1/summaryRoute.js"; //for Homepage widget | ||
|
|
There was a problem hiding this comment.
P1 | Confidence: High
The route is mounted incorrectly, causing it to bypass all existing route configuration and middleware. The app.use(SummaryRoutes) without a path prefix means the router's internal path (/api/v1/summary) is mounted at the root, creating a route at //api/v1/summary (double slash) which is malformed. More critically, it bypasses the verifyJWT middleware that protects all other /api/v1/* routes, making this endpoint publicly accessible without authentication. This violates the established security pattern of the application where all /api/v1/ endpoints (except public ones like /api/v1/status-page) require authentication. This creates a security vulnerability by exposing potentially sensitive system metrics without authentication, and breaks architectural consistency.
- Speculative: The route registration is inserted between
/api/v1/diagnosticand/api/v1/incidentsroutes without following the established alphabetical or logical grouping pattern. Other routes are imported and registered in a consistent order. While not functionally incorrect, this inconsistency makes the code harder to maintain and scan. Following the established pattern improves code readability and maintainability.
| import NotificationRoutes from "../routes/v1/notificationRoute.js"; | |
| import SummaryRoutes from "../routes/v1/summaryRoute.js"; //for Homepage widget | |
| app.use("/api/v1/summary", verifyJWT, SummaryRoutes); | |
| // Also ensure the import is placed in alphabetical order with other imports. |
Evidence: path:server/src/config/routes.js, path:server/src/routes/v1/summaryRoute.js, search:app.use, search:SummaryRoutes
| const [uptime, infrastructure, incidents] = await Promise.all([ | ||
| Monitor.countDocuments({ status: true, type: { $ne: "hardware" } }), | ||
| Monitor.countDocuments({ status: true, type: "hardware" }), | ||
| Incident.countDocuments({ status: true }) | ||
| ]); |
There was a problem hiding this comment.
P1 | Confidence: High
The incident query uses { status: true } which assumes incidents have a boolean status field. Based on the issue requirement for "open incidents," and common incident tracking patterns, this is likely incorrect. Most incident tracking systems use statuses like "open", "resolved", "investigating" etc., not booleans. This will either return 0 incidents (if the field is string-based) or incorrectly count resolved incidents (if boolean-based). This breaks the core business logic requirement of counting only "open" incidents as specified in the issue #2792.
Code Suggestion:
Incident.countDocuments({ endTime: null }) // Assuming open incidents have no endTime
// OR
Incident.countDocuments({ status: "open" }) // If using string statusEvidence: path:server/src/routes/v1/summaryRoute.js, search:Incident.countDocuments
| } catch (error) { | ||
| console.error("Erreur /api/v1/summary :", error); | ||
| res.status(500).json({ error: "Erreur interne" }); | ||
| } |
There was a problem hiding this comment.
P2 | Confidence: High
The error message is in French ("Erreur interne") while the rest of the codebase appears to use English. This creates inconsistency in the codebase and provides a confusing error message to users/developers who expect English messages. Consistent language usage is important for maintainability and developer experience.
| } catch (error) { | |
| console.error("Erreur /api/v1/summary :", error); | |
| res.status(500).json({ error: "Erreur interne" }); | |
| } | |
| console.error("Error in /api/v1/summary:", error); | |
| res.status(500).json({ error: "Internal server error" }); |
Evidence: path:server/src/routes/v1/summaryRoute.js, search:Erreur
| const router = express.Router(); | ||
|
|
||
| router.get("/api/v1/summary", async (req, res) => { |
There was a problem hiding this comment.
P2 | Confidence: Medium
The route definition contains the full /api/v1/summary path within the router. In Express.js, routers should define relative paths, and the full path is composed when mounting the router via app.use(). This creates path duplication and confusion about the actual endpoint location. The current pattern in the codebase (as seen in other route files) is to define relative paths in routers (e.g., router.get("/", ...)) and mount them with full paths in routes.js. The current approach will cause issues when combined with the improper mounting in routes.js.
| const router = express.Router(); | |
| router.get("/api/v1/summary", async (req, res) => { | |
| router.get("/", async (req, res) => { // Changed from "/api/v1/summary" to "/" |
Evidence: path:server/src/routes/v1/summaryRoute.js, method:get
|
|
||
| const router = express.Router(); | ||
|
|
||
| router.get("/api/v1/summary", async (req, res) => { |
There was a problem hiding this comment.
P2 | Confidence: Medium
Speculative: The endpoint lacks comprehensive error handling and validation. While basic error handling exists, there's no validation of query parameters (if any are added later), no input sanitization, and no handling of edge cases like database connection failures beyond the general catch block. The API also doesn't document expected behavior or error responses. While the current implementation is minimal, establishing proper error patterns early prevents technical debt as the endpoint evolves.
1 participant
(Please remove this line only before submitting your PR. Ensure that all relevant items are checked before submission.)
Describe your changes
This come to response to closed past pull request #3081 , the problem was solved and there are now only one file changed and one added a summaryRoute.js in server/route/v1 file to create an API we wall call in Homepage and updated server/config/routes.js adding app.use(SummaryRoutes). To test the API juste run localhost:52345/api/v1/metrics on your browser
Write your issue number after "Fixes "
Fixes #2792
Please ensure all items are checked off before requesting a review. "Checked off" means you need to add an "x" character between brackets so they turn into checkmarks.
<div>Add</div>, use):npm run formatin server and client directories, which automatically formats your code.Summary by CodeRabbit
/api/v1/summaryendpoint providing aggregated counts for uptime monitoring, infrastructure status, and incident tracking.✏️ Tip: You can customize this high-level summary in your review settings.