3120 remove URL's from data when they are supposed to be disabled #3121
Conversation
web-engineer
requested review from
Br0wnHammer,
Owaiseimdad,
ajhollid and
karenvicent
as code owners
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Schema & Validation server/src/db/v1/models/AppSettings.js, server/src/validation/joi.js, client/src/Validation/validation.js |
Add optional boolean showURL to AppSettings schema (default false) and mirror as optional boolean in server and client Joi validations. |
Status Page Logic server/src/db/v1/modules/statusPageModule.js |
Constructor now accepts AppSettings; getStatusPageByUrl fetches AppSettings and uses showURL to conditionally strip url, port, secret, notifications from monitors while still normalizing checks. |
Service Wiring server/src/config/services.js |
Pass AppSettings into StatusPageModule instantiation (constructor signature changed). |
Frontend Settings UI client/src/Pages/v1/Settings/index.jsx, client/src/Hooks/v1/settingsHooks.js |
Normalize showURL input to boolean, remove special-case early return, relocate validation to save handler, dispatch setShowURL(newValue) immediately; hooks formatting tweak. |
Minor Formatting server/src/service/v1/system/settingsService.js |
Whitespace-only change (blank line added). |
Sequence Diagram(s)
sequenceDiagram
actor Client
participant Frontend as Settings/Status UI
participant API as Server API
participant Module as StatusPageModule
participant DB as AppSettings Model
participant Normalizer as NormalizeData
Client->>Frontend: Request status page
Frontend->>API: GET /status/:page
API->>Module: getStatusPageByUrl(page)
Module->>DB: fetch AppSettings (singleton)
DB-->>Module: AppSettings (showURL)
Module->>Normalizer: normalize monitor.checks
Normalizer-->>Module: normalized checks
alt showURL == true
Module-->>API: monitors including url/port/secret/notifications
else showURL == false
Module-->>API: monitors with url/port/secret/notifications removed
end
API-->>Frontend: status page response
Frontend-->>Client: render status page
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
🐰 I nibbled a setting, small and bright,
A toggle tucked to hide the URL light,
Checks stayed tidy, secrets kept snug,
A hop, a patch, a little debug,
Thump-thump — the page sleeps tight. 🥕
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check | The PR description provided addresses the issue, explains what changed and why, but does not follow the required template structure with mandatory checkboxes and checklist items. | Use the repository's PR description template, complete all required checklist items (local deployment, self-review/testing, issue linkage), and add the issue number after 'Fixes'. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly describes the main change: conditionally removing URLs from public status page data based on a disabled setting. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing touches
- 📝 Generate docstrings
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.
|
Tested in our cluster and verified this looks to be working as expected - had to change the docker/prod/server.Dockerfile entrypoint to build the image then I could build and push to our registry, manually patched the live service to use the new image and verified the status pages worked and that the URL's were omitted from the data. Hope this works for you guys - had to rush this out as we want to use this status page for our live services but didn't want to expose our endpoints. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @server/src/db/v1/modules/statusPageModule.js:
- Line 220: The projection incorrectly uses a conditional that references
monitors.url and statusPage.showMonitorUrl; replace it so the status page's own
URL is always included by mapping url to "$statusPage.url" (remove the $cond),
and ensure any conditional hiding applies only to monitor URLs using the global
setting appSettings.showURL (e.g., keep conditional logic for monitors.url using
appSettings.showURL if needed); update the projection in statusPageModule.js
(the pipeline that builds statusPage.url and monitors.url) so statusPage.url is
unconditionally present and monitor URLs respect appSettings.showURL.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/src/config/services.jsserver/src/db/v1/modules/statusPageModule.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-24T17:52:55.506Z
Learnt from: Jesulayomy
Repo: bluewave-labs/Checkmate PR: 2664
File: client/src/Pages/Uptime/Create/index.jsx:92-96
Timestamp: 2025-07-24T17:52:55.506Z
Learning: In the Uptime monitor components, the `formatAndSet` function was deprecated due to rendering issues caused by state mutations. The current approach stores intervals internally in milliseconds (API format) but converts for display using `const displayInterval = monitor?.interval / MS_PER_MINUTE || 1;` and converts user input back to milliseconds using `value = value * MS_PER_MINUTE` in the onChange handler.
Applied to files:
server/src/db/v1/modules/statusPageModule.js
🔇 Additional comments (3)
server/src/db/v1/modules/statusPageModule.js (2)
8-12: LGTM! Dependency injection implemented correctly.The AppSettings dependency is properly injected and stored, following the existing pattern used for other dependencies in this module.
263-273: Monitor URL filtering logic is correct.The approach properly:
- Fetches global AppSettings to determine visibility
- Conditionally removes monitor.url fields when showURL is disabled
- Preserves all other monitor data including normalized checks
The post-processing approach (rather than conditional aggregation projection) is necessary since AppSettings must be fetched after the initial query.
server/src/config/services.js (1)
86-86: LGTM! AppSettings properly wired into StatusPageModule.The dependency injection is correctly implemented, passing the AppSettings model to enable the status page module to access global settings.
![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 fixes a data leak by hiding monitor URLs based on settings, but introduces a critical inconsistency in the filtering logic that could lead to unpredictable behavior and performance regressions.
📄 Documentation Diagram
This diagram documents the modified data flow for public status page responses with conditional URL display based on application settings.
sequenceDiagram
participant C as Client
participant A as API
participant S as StatusPageModule
participant DB as Database
participant AS as AppSettings
C->>A: GET /status-page
A->>S: getStatusPageByUrl()
S->>AS: fetch AppSettings (new)
AS-->>S: showURL flag
S->>DB: aggregate status page data
DB-->>S: data with conditional url field
S->>S: normalize monitors (strip url if !showURL)
S-->>A: response
A-->>C: status page data
note over S: PR #35;3121 adds AppSettings check and conditional logic
🌟 Strengths
- Addresses a security issue by ensuring monitor URLs are hidden when the global setting is disabled.
- Maintains existing functionality when the setting is enabled.
Findings Summary
| Priority | File | Category | Impact Summary | Anchors |
|---|---|---|---|---|
| P1 | server/.../statusPageModule.js | Architecture | Dual-layer filtering causes inconsistency and resource waste | |
| P2 | server/.../statusPageModule.js | Performance | Singleton DB query per request increases latency | |
| P2 | server/.../statusPageModule.js | Maintainability | Logic duplication may lead to maintenance errors | |
| P2 | server/.../services.js | Architecture | Direct model injection bypasses service encapsulation | |
| P2 | server/.../statusPageModule.js | Maintainability | Import removal could cause runtime error if used |
🔍 Notable Themes
- Inconsistent Data Handling: The implementation uses both aggregation-level and application-level conditions for URL filtering, creating a risk of misalignment and inefficiency.
- Architectural Pattern Deviation: Injecting the raw
AppSettingsmodel instead of a service module may lead to tighter coupling and missed optimization opportunities like caching.
📈 Risk Diagram
This diagram illustrates the risk of inconsistent URL filtering due to dual condition checks in the aggregation and application logic.
sequenceDiagram
participant S as StatusPageModule
participant DB as Database
participant AS as AppSettings
S->>DB: aggregate with $cond on statusPage.showMonitorUrl
DB-->>S: data with url field based on page setting
S->>AS: fetch AppSettings.showURL
AS-->>S: global setting
alt global showURL is false
S->>S: strip url field
else global showURL is true
S->>S: keep url field
end
note over S,DB: R1(P1): Inconsistency between page and global settings may lead to wasted data fetch
💡 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.
| timezone: 1, | ||
| showAdminLoginLink: 1, | ||
| url: 1, | ||
| url: { $cond: [{ $eq: ["$statusPage.showMonitorUrl", true] }, "$monitors.url", "$$REMOVE"] }, |
There was a problem hiding this comment.
P1 | Confidence: High
This change introduces a data-driven conditional field in a MongoDB aggregation pipeline using $statusPage.showMonitorUrl as the condition. However, the business logic immediately after the aggregation (changed lines below) uses a different source of truth (AppSettings.showURL). This creates a critical inconsistency: the aggregation may already filter out the url field based on a status-page-specific setting, while the application-level logic filters based on a global setting. This dual-layer filtering is architecturally confusing and could lead to unpredictable behavior (e.g., a status page's showMonitorUrl might be true, but the global showURL is false, causing the URL to be fetched from the DB only to be immediately stripped in JavaScript, wasting bandwidth and processing). The single source of truth for this feature should be clearly defined and applied in one place.
Code Suggestion:
Choose one approach:
1. **Use Global Setting Only:** Remove the aggregation condition and rely solely on the application logic.
```javascript
url: 1,
```
2. **Use Status Page Setting Only:** Remove the `AppSettings` dependency and logic, and ensure `statusPage.showMonitorUrl` is the authoritative setting.
```javascript
// In the constructor, remove AppSettings.
// Remove the appSettings fetch and showURL check in getStatusPageByUrl.
// Keep the aggregation condition as is.
```| const appSettings = await this.AppSettings.findOne({ singleton: true }).lean(); | ||
| const showURL = appSettings?.showURL === true; |
There was a problem hiding this comment.
P2 | Confidence: High
This change adds a database query (AppSettings.findOne) to a public-facing API endpoint (getStatusPageByUrl). This query will execute on every request to the public status page, significantly increasing database load and latency. The AppSettings (a likely singleton configuration) should be cached. The related_context shows services.js injects dependencies into modules; a caching mechanism (e.g., in a SettingsService or SettingsModule) should be used here instead of a direct DB call. Without caching, this introduces a performance regression and a potential scalability bottleneck.
Code Suggestion:
// In services.js, inject settingsModule (which already uses AppSettings)
const statusPageModule = new StatusPageModule({ ..., settingsModule });
// In statusPageModule.js constructor
this.settingsModule = settingsModule;
// In getStatusPageByUrl
const showURL = this.settingsModule.getAppSettings()?.showURL === true;| if (showURL !== true) { | ||
| const { url, ...rest } = monitor; | ||
| return { ...rest, checks: normalizedChecks }; | ||
| } |
There was a problem hiding this comment.
P2 | Confidence: High
The logic for stripping the url field is applied in the post-processing step after the aggregation pipeline. This is fragile because it assumes the url field's presence/absence hasn't already been handled by the aggregation's $cond (see Finding 1). Furthermore, this imperative deletion duplicates the intent already expressed (confusingly) in the aggregation stage. The logic for field visibility should be consolidated into a single, declarative location (ideally within the aggregation pipeline for efficiency and clarity) to prevent future maintenance errors.
Code Suggestion:
const normalizeMonitorForPublicView = (monitor, showURL) => {
const normalizedChecks = this.NormalizeData(monitor.checks, 10, 100);
if (!showURL) {
const { url, ...rest } = monitor;
return { ...rest, checks: normalizedChecks };
}
return { ...monitor, checks: normalizedChecks };
};
// Then in map:
const normalizedMonitors = monitors.map(monitor => normalizeMonitorForPublicView(monitor, showURL));| // import ServiceRegistry from "../../../service/system/serviceRegistry.js"; | ||
| // import StringService from "../../../service/system/stringService.js"; |
There was a problem hiding this comment.
P2 | Confidence: High
The PR removes these import statements (as shown in the import_removed diff category). While likely intentional cleanup, it must be verified that ServiceRegistry and StringService are not used elsewhere in the file. The related_context shows stringService is still used (this.stringService.statusPageNotFound), so its import must remain. Removing a used import will cause a runtime error. This appears to be an error in the diff presentation or a mistake.
Code Suggestion:
// import ServiceRegistry from "../../../service/system/serviceRegistry.js"; // REMOVE ONLY IF UNUSED
import StringService from "../../../service/system/stringService.js"; // KEEP| const checkModule = new CheckModule({ logger, Check, Monitor, User }); | ||
| const inviteModule = new InviteModule({ InviteToken, crypto, stringService }); | ||
| const statusPageModule = new StatusPageModule({ StatusPage, NormalizeData, stringService }); | ||
| const statusPageModule = new StatusPageModule({ StatusPage, NormalizeData, stringService, AppSettings }); |
There was a problem hiding this comment.
P2 | Confidence: Medium
Speculative: The PR injects the AppSettings model directly into the StatusPageModule. The related_context shows a settingsModule is already instantiated and injected into the main MongoDB class. This diverges from the apparent pattern of injecting services/modules rather than raw database models. Direct model injection bypasses any business logic, validation, or caching that the settingsModule might encapsulate, potentially leading to inconsistent data access patterns across the codebase. It also tightly couples the StatusPageModule to the database schema.
Code Suggestion:
// In services.js
const statusPageModule = new StatusPageModule({ StatusPage, NormalizeData, stringService, settingsModule });
// In statusPageModule.js constructor, adjust accordingly.
|
This relates to the issue i opened #3120 |
|
Jus to update, this PR is no good currently - I've spotted an issue with it and been working the problem today, hope to have an update shorlty. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @client/src/Pages/v1/Settings/index.jsx:
- Around line 165-169: You're doing an optimistic UI update by dispatching
setShowURL immediately, but there's no rollback when the server save fails;
capture the previous value (from settingsData.showURL or the incoming value)
before dispatching, then update useSaveSettings/saveSettings to handle failures:
on save error dispatch setShowURL(previousValue) to revert Redux, surface the
error to the user, and only keep the optimistic value on success (alternatively
move the dispatch into saveSettings and only call setShowURL after a successful
save if you prefer server-driven state).
In @server/src/controllers/v1/settingsController.ts:
- Around line 60-63: Remove the diagnostic console.log that prints req.body in
the settings controller: delete the line console.log("=== SETTINGS CONTROLLER
REQUEST BODY ===", req.body) (or replace it) and if logging is required use the
app's structured logger (e.g., processLogger or logger) and redact/omit
sensitive fields such as systemEmailPassword and pagespeedApiKey before logging
(implement a small sanitizer that copies req.body and deletes or masks those
keys, then log the sanitized object).
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
client/src/Pages/v1/Settings/index.jsxserver/src/controllers/v1/settingsController.tsserver/src/db/v1/modules/statusPageModule.jsserver/src/service/v1/system/settingsService.jsserver/src/validation/joi.js
✅ Files skipped from review due to trivial changes (1)
- server/src/service/v1/system/settingsService.js
🚧 Files skipped from review as they are similar to previous changes (1)
- server/src/db/v1/modules/statusPageModule.js
🔇 Additional comments (1)
server/src/validation/joi.js (1)
416-416: LGTM!Making
showURLan optional boolean in the validation schema is appropriate for a user-configurable setting.
|
|
||
| // Update Redux immediately for UI feedback | ||
| if (name === "showURL") { | ||
| dispatch(setShowURL(newValue ?? value)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "index.jsx" -path "*/Settings/*" | head -20Repository: bluewave-labs/Checkmate
Length of output: 107
🏁 Script executed:
git ls-files | grep -i "settings" | grep -E "\.(jsx|js)$"Repository: bluewave-labs/Checkmate
Length of output: 868
🏁 Script executed:
find . -type f -path "*/Pages/v1/Settings/index.jsx" 2>/dev/nullRepository: bluewave-labs/Checkmate
Length of output: 107
🏁 Script executed:
fd "index.jsx" --path "*Settings*"Repository: bluewave-labs/Checkmate
Length of output: 282
🏁 Script executed:
ls -la client/src/Pages/v1/ 2>/dev/null | head -20Repository: bluewave-labs/Checkmate
Length of output: 1146
🏁 Script executed:
find . -type d -name "Settings" 2>/dev/nullRepository: bluewave-labs/Checkmate
Length of output: 97
🏁 Script executed:
git ls-files client/src/Pages/v1/Settings/ 2>/dev/null | head -20Repository: bluewave-labs/Checkmate
Length of output: 596
🏁 Script executed:
cat client/src/Pages/v1/Settings/index.jsx 2>/dev/null | wc -lRepository: bluewave-labs/Checkmate
Length of output: 70
🏁 Script executed:
head -200 client/src/Pages/v1/Settings/index.jsx 2>/dev/nullRepository: bluewave-labs/Checkmate
Length of output: 5601
🏁 Script executed:
ls -la client/src/Pages/ 2>/dev/null || echo "Path not found, searching..."Repository: bluewave-labs/Checkmate
Length of output: 286
🏁 Script executed:
git ls-files | grep Settings | head -20Repository: bluewave-labs/Checkmate
Length of output: 702
🏁 Script executed:
git ls-files | head -50Repository: bluewave-labs/Checkmate
Length of output: 1734
🏁 Script executed:
sed -n '155,180p' client/src/Pages/v1/Settings/index.jsxRepository: bluewave-labs/Checkmate
Length of output: 664
🏁 Script executed:
cat client/src/Hooks/v1/settingsHooks.jsRepository: bluewave-labs/Checkmate
Length of output: 2229
🏁 Script executed:
wc -l client/src/Hooks/v1/settingsHooks.jsRepository: bluewave-labs/Checkmate
Length of output: 106
Redux state persists after failed save with no rollback mechanism.
Dispatching setShowURL immediately (line 169) provides UI feedback, but useSaveSettings lacks error handling to revert Redux state if the save fails. When the server request fails, the Redux state remains updated while local settingsData never reflects the change, creating state inconsistency between Redux and server.
Consider one of these approaches:
- Optimistic update with rollback: Add error handling in
saveSettingsto dispatch a revert action (e.g.,dispatch(setShowURL(previousValue))) on failure. - Server-driven state: Move the dispatch to
useSaveSettingsand only dispatch after successful save confirmation. - Unified state source: Remove the immediate dispatch and rely on
setSettingsDataalone, letting the post-save state update synchronize both local and Redux state.
🤖 Prompt for AI Agents
In @client/src/Pages/v1/Settings/index.jsx around lines 165 - 169, You're doing
an optimistic UI update by dispatching setShowURL immediately, but there's no
rollback when the server save fails; capture the previous value (from
settingsData.showURL or the incoming value) before dispatching, then update
useSaveSettings/saveSettings to handle failures: on save error dispatch
setShowURL(previousValue) to revert Redux, surface the error to the user, and
only keep the optimistic value on success (alternatively move the dispatch into
saveSettings and only call setShowURL after a successful save if you prefer
server-driven state).
- Remove all console.log statements added during development - Clean up code for production readiness
|
OK - Had to rethink this a bit, turned out the setting was not being saved - in the current release it is only for the current user session, so doesn't do what it says anyway! I've updated this so that this now saves to the database so the settings do now persist and corrected the filtering of the data. Summary of changes - Implementation Details1. Database Schema (Backend)File: Added showURL: {
type: Boolean,
default: false,
}This ensures the setting can be persisted to MongoDB and prevents Mongoose from silently ignoring the field (strict mode). 2. Server Validation (Backend)File: Added const updateAppSettingsBodyValidation = joi.object({
// ... other fields
showURL: joi.bool().optional(),
// ... other fields
});This validates incoming requests and allows the showURL field to be accepted by the API. 3. Status Page Data Filtering (Backend)File: Modified const appSettings = await this.AppSettings.findOne({ singleton: true }).lean();
const showURL = appSettings?.showURL === true;
const normalizedMonitors = monitors.map((monitor) => {
const normalizedChecks = this.NormalizeData(monitor.checks, 10, 100);
if (showURL !== true) {
const { url, port, secret, notifications, ...rest } = monitor;
return { ...rest, checks: normalizedChecks };
}
return { ...monitor, checks: normalizedChecks };
});When
4. Client Validation (Frontend)File: Added const settingsValidation = joi.object({
// ... other fields
showURL: joi.boolean().optional(),
// ... other fields
});Without this, Joi validation would reject the settings object as having an unknown field, preventing the save button from working. 5. Settings Form Handler (Frontend)File: Added proper boolean coercion for the // Ensure showURL is a proper boolean
if (name === "showURL") {
newValue = value === true || value === "true";
}This handles potential type coercion issues from MUI Select components and ensures the value is always a proper boolean before saving. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @server/src/db/v1/modules/statusPageModule.js:
- Around line 266-273: Prettier flagged inconsistent indentation in the monitors
mapping block; run Prettier (prettier --write) to fix formatting, then
optionally extract the inline mapper into a named function (e.g.,
buildNormalizedMonitor or normalizeMonitorMapper) to improve clarity: move the
mapping logic that references this.NormalizeData(...), showURL, and the
destructuring of { url, port, secret, notifications, ...rest } into that
function and have monitors.map(normalizeMonitorMapper) produce
normalizedMonitors so the code is both formatted consistently and the mapper
logic is clearer and reusable.
🧹 Nitpick comments (2)
server/src/db/v1/modules/statusPageModule.js (2)
263-264: Consider caching AppSettings to improve performance.The
AppSettingssingleton is fetched on every status page request. Since status pages are often publicly accessible and may be polled frequently, this adds unnecessary database load.💡 Suggested optimization approach
Consider implementing a caching strategy:
Option 1: Cache in memory with invalidation on updates:
- Store AppSettings in a module-level cache with TTL
- Invalidate cache when AppSettings are updated
Option 2: Pass showURL as a parameter from a cached layer above
Example for Option 1:
// At module level let appSettingsCache = null; let cacheTimestamp = null; const CACHE_TTL = 60000; // 1 minute // In getStatusPage method const now = Date.now(); if (!appSettingsCache || !cacheTimestamp || (now - cacheTimestamp) > CACHE_TTL) { appSettingsCache = await this.AppSettings.findOne({ singleton: true }).lean(); cacheTimestamp = now; } const showURL = appSettingsCache?.showURL === true;
90-281: Note: PR author mentioned an ongoing issue.In the PR comments, you mentioned "the PR currently has an issue, is working on a fix." The current implementation appears logically correct for the stated goal of conditionally hiding URLs. If the issue you're addressing relates to this method, please share details so it can be reviewed.
Additionally, there's a minor optimization opportunity: the aggregation pipeline (lines 120-254) projects
url,port,secret, andnotificationsfields, which are then conditionally stripped in JavaScript (lines 266-273). While functionally correct, you could optimize by conditionally projecting these fields in the aggregation pipeline itself. However, the current approach is simpler and more maintainable, so this is entirely optional given the typical small number of monitors per status page.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
client/src/Pages/v1/Settings/index.jsxclient/src/Validation/validation.jsserver/src/db/v1/models/AppSettings.jsserver/src/db/v1/modules/statusPageModule.js
🚧 Files skipped from review as they are similar to previous changes (1)
- client/src/Pages/v1/Settings/index.jsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-24T17:52:55.506Z
Learnt from: Jesulayomy
Repo: bluewave-labs/Checkmate PR: 2664
File: client/src/Pages/Uptime/Create/index.jsx:92-96
Timestamp: 2025-07-24T17:52:55.506Z
Learning: In the Uptime monitor components, the `formatAndSet` function was deprecated due to rendering issues caused by state mutations. The current approach stores intervals internally in milliseconds (API format) but converts for display using `const displayInterval = monitor?.interval / MS_PER_MINUTE || 1;` and converts user input back to milliseconds using `value = value * MS_PER_MINUTE` in the onChange handler.
Applied to files:
server/src/db/v1/modules/statusPageModule.js
🪛 GitHub Actions: Format Check (Client & Server)
server/src/db/v1/modules/statusPageModule.js
[error] 1-1: Prettier formatting issue detected. Code style issues found in src/db/v1/modules/statusPageModule.js. Run 'prettier --write' to fix.
🔇 Additional comments (3)
client/src/Validation/validation.js (1)
331-331: LGTM!The validation rule for
showURLis correctly defined as an optional boolean, consistent with the schema default and other boolean settings fields.server/src/db/v1/modules/statusPageModule.js (1)
8-8: LGTM!The constructor correctly accepts
AppSettingsvia dependency injection, consistent with the existing pattern for other dependencies.Also applies to: 12-12
server/src/db/v1/models/AppSettings.js (1)
58-61: No action needed. TheshowURLfield with defaultfalseis correctly implemented and requires no production database updates.The singleton document is automatically initialized with schema defaults via
setDefaultsOnInsert: trueinMongoDB.js(line 55) when the connection first runs. For existing documents without this field, Mongoose returns the default value, and the code safely handles missing fields with theappSettings?.showURL === truecheck inStatusPageModule.js. No migration is required.
| const normalizedMonitors = monitors.map((monitor) => { | ||
| return { | ||
| ...monitor, | ||
| checks: this.NormalizeData(monitor.checks, 10, 100), | ||
| }; | ||
| }); | ||
| const normalizedChecks = this.NormalizeData(monitor.checks, 10, 100); | ||
| if (showURL !== true) { | ||
| const { url, port, secret, notifications, ...rest } = monitor; | ||
| return { ...rest, checks: normalizedChecks }; | ||
| } | ||
| return { ...monitor, checks: normalizedChecks }; | ||
| }); |
There was a problem hiding this comment.
Fix formatting issue and consider extracting the mapper function.
The code correctly strips sensitive fields (url, port, secret, notifications) when showURL is not enabled, but there are two concerns:
-
Formatting: Pipeline reports a Prettier formatting issue in this section. The indentation appears inconsistent.
-
Code clarity: The mapper logic could be more readable if extracted.
🔧 Fix formatting and improve clarity
Required: Fix formatting
Run Prettier to fix the indentation:
prettier --write server/src/db/v1/modules/statusPageModule.jsOptional: Extract mapper for clarity
+ const createNormalizedMonitor = (monitor) => {
+ const normalizedChecks = this.NormalizeData(monitor.checks, 10, 100);
+
+ if (showURL !== true) {
+ const { url, port, secret, notifications, ...rest } = monitor;
+ return { ...rest, checks: normalizedChecks };
+ }
+
+ return { ...monitor, checks: normalizedChecks };
+ };
+
- const normalizedMonitors = monitors.map((monitor) => {
- const normalizedChecks = this.NormalizeData(monitor.checks, 10, 100);
- if (showURL !== true) {
- const { url, port, secret, notifications, ...rest } = monitor;
- return { ...rest, checks: normalizedChecks };
- }
- return { ...monitor, checks: normalizedChecks };
- });
+ const normalizedMonitors = monitors.map(createNormalizedMonitor);📝 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.
| const normalizedMonitors = monitors.map((monitor) => { | |
| return { | |
| ...monitor, | |
| checks: this.NormalizeData(monitor.checks, 10, 100), | |
| }; | |
| }); | |
| const normalizedChecks = this.NormalizeData(monitor.checks, 10, 100); | |
| if (showURL !== true) { | |
| const { url, port, secret, notifications, ...rest } = monitor; | |
| return { ...rest, checks: normalizedChecks }; | |
| } | |
| return { ...monitor, checks: normalizedChecks }; | |
| }); | |
| const createNormalizedMonitor = (monitor) => { | |
| const normalizedChecks = this.NormalizeData(monitor.checks, 10, 100); | |
| if (showURL !== true) { | |
| const { url, port, secret, notifications, ...rest } = monitor; | |
| return { ...rest, checks: normalizedChecks }; | |
| } | |
| return { ...monitor, checks: normalizedChecks }; | |
| }; | |
| const normalizedMonitors = monitors.map(createNormalizedMonitor); |
🤖 Prompt for AI Agents
In @server/src/db/v1/modules/statusPageModule.js around lines 266 - 273,
Prettier flagged inconsistent indentation in the monitors mapping block; run
Prettier (prettier --write) to fix formatting, then optionally extract the
inline mapper into a named function (e.g., buildNormalizedMonitor or
normalizeMonitorMapper) to improve clarity: move the mapping logic that
references this.NormalizeData(...), showURL, and the destructuring of { url,
port, secret, notifications, ...rest } into that function and have
monitors.map(normalizeMonitorMapper) produce normalizedMonitors so the code is
both formatted consistently and the mapper logic is clearer and reusable.
Summary of what we fixed based on AI review feedback: Formatting - Ran Prettier to fix code formatting Redux state rollback - Moved Redux dispatch to after successful save in settingsHooks.js Performance - Added caching layer in settingsModule (1-minute TTL) to prevent DB query on every status page request Architecture - Inject settingsModule instead of raw AppSettings model for better encapsulation
- Remove settingsModule caching that caused inconsistent state - Restore direct AppSettings query in statusPageModule - Keep immediate Redux dispatch for UI feedback - All functionality verified working"
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/src/Pages/v1/Settings/index.jsx (1)
171-175: LGTM!The Redux dispatch provides immediate UI feedback while the actual persistence happens on Save. This is the correct pattern for a server-persisted setting.
Minor note: The
?? valuefallback is unreachable forshowURLsincenewValueis always set to a boolean at line 96. Consider simplifying todispatch(setShowURL(newValue))for clarity, though the current code is functionally correct.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
client/src/Hooks/v1/settingsHooks.jsclient/src/Pages/v1/Settings/index.jsxserver/src/db/v1/modules/statusPageModule.js
🚧 Files skipped from review as they are similar to previous changes (1)
- server/src/db/v1/modules/statusPageModule.js
🧰 Additional context used
🧬 Code graph analysis (1)
client/src/Pages/v1/Settings/index.jsx (2)
client/src/Pages/v1/Logs/index.jsx (1)
value(18-18)client/src/Pages/v1/Account/components/ProfilePanel.jsx (1)
dispatch(31-31)
🔇 Additional comments (2)
client/src/Hooks/v1/settingsHooks.js (1)
58-60: LGTM!Minor formatting change that improves readability by separating state mutations from the success toast.
client/src/Pages/v1/Settings/index.jsx (1)
93-97: LGTM!The boolean coercion correctly handles MUI Select returning string values. The pattern
value === true || value === "true"properly normalizes both boolean and string inputs to a boolean for storage.
client/src/Pages/v1/Settings/index.jsx (1) 171-175: LGTM! The Redux dispatch provides immediate UI feedback while the actual persistence happens on Save. This is the correct pattern for a server-persisted setting. Minor note: The ?? value fallback is unreachable for showURL since newValue is always set to a boolean at line 96. Consider simplifying to dispatch(setShowURL(newValue)) for clarity, though the current code is functionally correct.
|
thats me done for now - hope this is good for you guys, it works well for me when testing in our cluster. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/src/Pages/v1/Settings/index.jsx (1)
164-180: Bug:saveSettingsis called regardless of validation result.The
saveSettingscall on line 179 is outside the validation success block, meaning it executes even when validation fails. This allows potentially invalid settings to be submitted to the server.🐛 Proposed fix: only save when validation passes
const handleSave = () => { // Validate const { error } = settingsValidation.validate(settingsData.settings, { abortEarly: false, }); if (!error || error.details.length === 0) { setErrors({}); + saveSettings(settingsData?.settings); } else { const newErrors = {}; error.details.forEach((err) => { newErrors[err.path[0]] = err.message; }); setErrors(newErrors); } - saveSettings(settingsData?.settings); };
🧹 Nitpick comments (1)
client/src/Pages/v1/Settings/index.jsx (1)
157-161: Minor UX inconsistency: Redux updated before persistence.The
showURLvalue is dispatched to Redux immediately for UI feedback, but it only persists after clicking Save. If the user changes this setting and navigates away without saving, the UI will reflect the unsaved value until a page refresh reloads the server state.Consider whether this behavior is intentional or if
showURLshould follow the same pattern astimezone/mode/language(dispatch + early return, no Save required).
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
client/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
client/src/Pages/v1/Settings/index.jsx
🔇 Additional comments (1)
client/src/Pages/v1/Settings/index.jsx (1)
93-97: LGTM!The boolean coercion correctly handles both the native boolean
trueand the string"true"that MUI Select may emit, ensuringshowURLis always stored as a proper boolean.
|
updated validation as per recomendation. Regarding the nitpick about Redux being updated before persistence - this is there for immediate UI feedback. The showURL toggle updates Redux immediately so users see the change reflected in the UI right away, then persists when they click Save. This is different from timezone/mode/language which save immediately, but matches the Save button pattern used by other settings on the page. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/src/Pages/v1/Settings/index.jsx (1)
164-180: Validation-driven save flow is correct.The fix properly ensures
saveSettingsis only invoked when validation passes, addressing the issue mentioned in the commit message.Minor inconsistency: Line 166 uses
settingsData.settingswithout optional chaining, while line 171 usessettingsData?.settings. Since line 166 would throw ifsettingsDatais null before reaching line 171, consider using consistent access patterns.♻️ Suggested consistency fix
const handleSave = () => { // Validate - const { error } = settingsValidation.validate(settingsData.settings, { + const { error } = settingsValidation.validate(settingsData?.settings, { abortEarly: false, }); if (!error || error.details.length === 0) { setErrors({}); saveSettings(settingsData?.settings);
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/src/Pages/v1/Settings/index.jsx
🧰 Additional context used
🧬 Code graph analysis (1)
client/src/Pages/v1/Settings/index.jsx (2)
client/src/Validation/validation.js (1)
settingsValidation(310-340)client/src/Hooks/v1/settingsHooks.js (3)
error(8-8)error(38-38)saveSettings(41-68)
🔇 Additional comments (2)
client/src/Pages/v1/Settings/index.jsx (2)
93-97: LGTM!The boolean normalization correctly handles both boolean
trueand string"true"from MUI Select, ensuringshowURLis always a proper boolean before being stored in state.
157-161: LGTM!The Redux dispatch after
setSettingsDatacorrectly ensures both the local form state (for persistence) and Redux UI state (for immediate feedback) are synchronized.
1 participant
Fix: Hide monitor URLs from public status page when setting is disabled
Issue: #3120 Data leak: Monitor URLs exposed on public status page
What changed
StatusPageModuleto respect the global "Display IP/URL on status page" settingAppSettings.showURLAppSettings(consistent with existing patterns)Files changed
server/src/db/v1/modules/statusPageModule.js– Added conditional URL stripping in response assemblyserver/src/config/services.js– InjectAppSettingsintoStatusPageModuleTesting
Summary by CodeRabbit
New Features
Improvements
Validation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.