Add Response Time Chart option to Status Page configuration and display Closes #2924 #3026
Conversation
bansalhimanshu0296
requested review from
Br0wnHammer,
Owaiseimdad,
ajhollid and
karenvicent
as code owners
WalkthroughAdds showResponseTimeChart end-to-end: UI toggle, form state, submission, client/server validation, DB schema, locale entries, per-monitor conditional ResponseTimeChart rendering, and renames/prop rename of the ResponseTimeChart component to use Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as StatusPage Create UI
participant Form as Form State
participant API as NetworkService
participant Server
participant DB as Database
participant View as Status Page View
participant Monitors as MonitorsList
User->>UI: Toggle showResponseTimeChart
UI->>Form: handleFormChange(showResponseTimeChart)
User->>UI: Submit form
UI->>API: createStatusPage(form with showResponseTimeChart)
API->>Server: POST payload
Server->>Server: Joi validation (includes showResponseTimeChart)
Server->>DB: Save StatusPage (showResponseTimeChart default true)
Server-->>API: Success
API-->>UI: Confirm
Note over View,Monitors: Rendering status page monitors
View->>Monitors: pass statusPage.showResponseTimeChart
alt showResponseTimeChart enabled
Monitors->>Monitors: fetch monitor stats per monitor
Monitors->>Monitors: render ResponseTimeChart per monitor (passes monitorIsLoading)
else disabled
Monitors->>Monitors: skip chart rendering
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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[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 (7)
client/src/locales/ar.json (1)
286-286: Consider providing Arabic translation for completeness.The
showResponseTimeChartkey has an empty translation value. While this doesn't break functionality (will fall back to the key), providing the Arabic translation would improve the user experience for Arabic-speaking users.client/src/locales/vi.json (1)
287-287: Consider providing Vietnamese translation for completeness.The
showResponseTimeChartkey has an empty translation value. Providing the Vietnamese translation would improve the user experience for Vietnamese-speaking users.client/src/locales/es.json (1)
286-286: Consider providing Spanish translation for completeness.The
showResponseTimeChartkey has an empty translation value. Providing the Spanish translation would improve the user experience for Spanish-speaking users.client/src/locales/uk.json (1)
287-287: Consider providing Ukrainian translation for completeness.The
showResponseTimeChartkey has an empty translation value. Providing the Ukrainian translation would improve the user experience for Ukrainian-speaking users.client/src/Pages/v1/StatusPage/Status/Components/MonitorsList/index.jsx (3)
25-25: Remove unused state setter or wire it up.
setDateRangeisn’t used; simplify to avoid lints unless a control will be added soon.Apply this diff if no date range control yet:
- const [dateRange, setDateRange] = useState("recent"); + const [dateRange] = useState("recent");
46-46: Drop redundant key on child.The parent Stack already has a stable
key. Remove thekeyonHostto reduce noise.- <Host - key={monitor._id} + <Host url={monitor.url} title={monitor.name} ...
72-85: Consider batching chart data fetches to reduce N+1 requests.Per‑monitor fetches scale poorly on status pages with many monitors. Longer term, expose grouped checks in the status page payload or add a batch endpoint to fetch multiple monitors’ grouped checks in one request.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
client/src/Components/v1/Charts/ResponseTimeChart/ResponseTimeChart.jsx(2 hunks)client/src/Pages/v1/StatusPage/Create/Components/Tabs/Content.jsx(1 hunks)client/src/Pages/v1/StatusPage/Create/index.jsx(3 hunks)client/src/Pages/v1/StatusPage/Status/Components/MonitorsList/index.jsx(4 hunks)client/src/Pages/v1/Uptime/Details/index.jsx(1 hunks)client/src/Utils/NetworkService.js(1 hunks)client/src/Validation/validation.js(1 hunks)client/src/locales/ar.json(1 hunks)client/src/locales/cs.json(1 hunks)client/src/locales/de.json(1 hunks)client/src/locales/en.json(1 hunks)client/src/locales/es.json(1 hunks)client/src/locales/fi.json(1 hunks)client/src/locales/fr.json(1 hunks)client/src/locales/ja.json(1 hunks)client/src/locales/pt-BR.json(1 hunks)client/src/locales/ru.json(1 hunks)client/src/locales/th.json(1 hunks)client/src/locales/tr.json(1 hunks)client/src/locales/uk.json(1 hunks)client/src/locales/vi.json(1 hunks)client/src/locales/zh-CN.json(1 hunks)client/src/locales/zh-TW.json(1 hunks)client/tsconfig.tsbuildinfo(1 hunks)server/src/db/v1/models/StatusPage.js(1 hunks)server/src/db/v1/modules/statusPageModule.js(3 hunks)server/src/validation/joi.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-24T17:52:55.506Z
Learnt from: Jesulayomy
PR: bluewave-labs/Checkmate#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:
client/src/Pages/v1/StatusPage/Status/Components/MonitorsList/index.jsx
🧬 Code graph analysis (3)
client/src/Utils/NetworkService.js (1)
client/src/Pages/v1/StatusPage/Create/index.jsx (1)
form(39-52)
client/src/Pages/v1/StatusPage/Create/Components/Tabs/Content.jsx (2)
client/src/Components/v1/Inputs/Checkbox/index.jsx (1)
Checkbox(46-104)client/src/Pages/v1/StatusPage/Create/index.jsx (2)
form(39-52)handleFormChange(74-99)
client/src/Pages/v1/StatusPage/Status/Components/MonitorsList/index.jsx (2)
client/src/Hooks/v1/monitorHooks.js (2)
monitorStats(278-278)useFetchUptimeMonitorById(274-300)client/src/Components/v1/Charts/ResponseTimeChart/ResponseTimeChart.jsx (1)
ResponseTimeChart(7-23)
🪛 Biome (2.1.2)
client/src/Pages/v1/StatusPage/Status/Components/MonitorsList/index.jsx
[error] 34-34: This hook is being called from a nested function, but all hooks must be called unconditionally from the top-level component.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🔇 Additional comments (17)
client/tsconfig.tsbuildinfo (1)
1-1: Add.tsbuildinfoto.gitignoreto prevent committing generated TypeScript build artifacts.The
client/tsconfig.tsbuildinfofile is an auto-generated TypeScript compiler cache that should not be tracked in version control. While the file is currently committed, this is an anti-pattern that can lead to merge conflicts and unnecessary noise in diffs. The modification in this PR (adding paths for new components) occurs automatically during the build process and does not represent meaningful changes to review.Add the following to
.gitignore:*.tsbuildinfoConsider also running
git rm --cached client/tsconfig.tsbuildinfoto remove it from tracking without deleting the local file, so it will no longer appear in future commits.client/src/locales/fi.json (1)
286-286: LGTM! Finnish translation provided.The Finnish translation "Näytä vasteaikakaavio" is appropriately provided and follows the same pattern as the adjacent
showChartstranslation.server/src/validation/joi.js (1)
481-481: LGTM! Validation correctly added.The
showResponseTimeChartfield is appropriately validated as an optional boolean, consistent with the pattern used for similar feature flags likeshowChartsandshowUptimePercentage.client/src/locales/de.json (1)
290-290: LGTM! German translation provided.The German translation "Reaktionszeitdiagramm anzeigen" is appropriately provided and follows the same pattern as the adjacent
showChartstranslation.client/src/locales/en.json (1)
290-290: LGTM! English translation properly defined.The English translation "Show Response Time Chart" is clear and follows the same pattern as adjacent feature flag labels like
showChartsandshowUptimePercentage.client/src/Pages/v1/Uptime/Details/index.jsx (1)
6-6: LGTM: Import path updated to reflect component relocation.The import path adjustment aligns with the ResponseTimeChart component reorganization.
client/src/Pages/v1/StatusPage/Create/Components/Tabs/Content.jsx (1)
93-99: LGTM: Checkbox for showResponseTimeChart follows established patterns.The new checkbox is properly wired to
form.showResponseTimeChartand uses the translation key. Implementation matches the existing feature checkboxes (showCharts, showUptimePercentage, showAdminLoginLink).server/src/db/v1/models/StatusPage.js (1)
73-76: LGTM: Schema field added correctly.The
showResponseTimeChartfield is properly defined as a Boolean with a sensible default value oftrue, consistent with similar feature flags in the schema.client/src/Validation/validation.js (1)
306-306: LGTM: Validation rule added correctly.The
showResponseTimeChartvalidation asjoi.boolean()is appropriate and consistent with the other feature flag validations in the schema.client/src/Utils/NetworkService.js (1)
932-934: LGTM: FormData handling follows established pattern.The handling of
showResponseTimeChartmirrors the existing pattern forshowChartsandshowUptimePercentage, with proper undefined checks and string conversion for FormData.client/src/Pages/v1/StatusPage/Create/index.jsx (3)
24-30: LGTM: ERROR_TAB_MAPPING correctly updated.The
showResponseTimeChartfield is properly included in the second tab's error mapping array, ensuring validation errors navigate users to the correct tab.
50-50: LGTM: Sensible default value.Setting
showResponseTimeChart: trueby default provides a good user experience, enabling the feature out-of-the-box similar to other chart features.
220-220: LGTM: Backward-compatible data loading.The fallback to
truewhen loading existing status pages handles cases where the field doesn't exist in older data, ensuring backward compatibility.client/src/locales/th.json (1)
290-290: LGTM: Translation key added correctly.The
showResponseTimeCharttranslation key is properly positioned with other feature flags and aligns with the new UI checkbox label.client/src/Components/v1/Charts/ResponseTimeChart/ResponseTimeChart.jsx (1)
7-7: LGTM: Component name typo corrected.The component name has been corrected from
ResponseTImeCharttoResponseTimeChart, fixing the typo. All related references (PropTypes assignment and default export) have been updated consistently.Also applies to: 25-25, 31-31
server/src/db/v1/modules/statusPageModule.js (2)
100-113: LGTM: Propagates showResponseTimeChart in empty‑monitors path.Correctly includes
showResponseTimeChartin the early return payload; backward compatibility maintained since UI checks!== false.Also applies to: 124-124
231-231: LGTM: Field added to aggregation projection.
showResponseTimeChartnow flows through the aggregated path as well.
client/src/Pages/v1/StatusPage/Status/Components/MonitorsList/index.jsx
Outdated
Show resolved
Hide resolved
client/src/Pages/v1/StatusPage/Status/Components/MonitorsList/index.jsx
Outdated
Show resolved
Hide resolved
![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 valuable response time chart feature but has critical performance and architectural issues that could degrade user experience and system stability.
📄 Documentation Diagram
This diagram documents the new response time chart configuration and display workflow on status pages.
sequenceDiagram
participant User as User
participant Config as Status Page Config
participant Backend as Backend
participant Frontend as Frontend Display
User->>Config: Toggle "Show Response Time Chart"
Config->>Backend: Save setting (showResponseTimeChart)
Backend-->>Config: Confirm save
User->>Frontend: View Status Page
Frontend->>Backend: Fetch status page data
Backend-->>Frontend: Return data incl. showResponseTimeChart
note over Frontend: PR #35;3026 added chart rendering logic
Frontend->>Backend: Fetch monitor response times (if enabled)
Backend-->>Frontend: Return response time data
Frontend-->>User: Display response time chart
🌟 Strengths
- Solid backend integration with proper schema and validation updates.
- Comprehensive localization support across multiple languages.
| Priority | File | Category | Impact Summary | Anchors |
|---|---|---|---|---|
| P1 | Pages/v1/StatusPage/.../MonitorsList/index.jsx | Performance | Severe perf degradation from N API calls | useFetchUptimeMonitorById |
| P1 | Pages/v1/StatusPage/.../MonitorsList/index.jsx | Architecture | Data sync issue and hook violation | local_only |
| P2 | Pages/v1/StatusPage/.../MonitorsList/index.jsx | Bug | Incorrect loading state for charts | ResponseTimeChart |
| P2 | Pages/v1/Uptime/Details/index.jsx | Architecture | Fragile import paths | local_only |
| P2 | server/src/db/v1/models/StatusPage.js | Maintainability | Backward compat default value | statusPageModule.js |
🔍 Notable Themes
- Performance and architecture flaws in the MonitorsList component risk system scalability.
- Import path consistency and loading state management need attention across components.
📈 Risk Diagram
This diagram illustrates the performance risk from multiple API calls in the monitor list loop.
sequenceDiagram
participant MonitorsList as MonitorsList Component
participant Hook as useFetchUptimeMonitorById
participant API as Backend API
MonitorsList->>MonitorsList: Render monitors.map
loop For each monitor
MonitorsList->>Hook: Call useFetchUptimeMonitorById
note over Hook: R1(P1): Severe performance degradation from multiple API calls
Hook->>API: Fetch monitor data
API-->>Hook: Return data
end
MonitorsList-->>User: Render charts
⚠️ **Unanchored Suggestions (Manual Review Recommended)**
The following suggestions could not be precisely anchored to a specific line in the diff. This can happen if the code is outside the changed lines, has been significantly refactored, or if the suggestion is a general observation. Please review them carefully in the context of the full file.
📁 File: client/src/Pages/v1/StatusPage/Status/Components/MonitorsList/index.jsx
The implementation calls useFetchUptimeMonitorById inside a .map() loop, creating N API calls for N monitors on every render. This will cause severe performance degradation on status pages with multiple monitors. The hook appears to be designed for single monitor details pages, not bulk operations. Each monitor triggers a separate network request, potentially overwhelming both client and server resources.
Suggestion:
// Fetch all monitor data in bulk at the component level
const { monitorData } = useFetchBulkMonitorData({
monitorIds: monitors.map(m => m._id),
dateRange
});
// Then in map:
{monitors?.map((monitor) => {
const monitorDataForThis = monitorData[monitor._id];
// ... rest of component
Related Code:
{monitors?.map((monitor) => {
const status = determineState(monitor);
const [monitorData, monitorStats, monitorIsLoading, monitorNetworkError] =
useFetchUptimeMonitorById({
monitorId: monitor._id,
dateRange,
trigger: false,
});
return (
📁 File: client/src/Pages/v1/StatusPage/Status/Components/MonitorsList/index.jsx
The dateRange state is defined at the component level but individual chart fetches don't react to its changes. The trigger: false parameter suggests the hook won't refetch when dateRange updates. This creates a data synchronization issue where changing the date range won't update the response time charts. The architecture violates React hooks rules by calling hooks inside loops.
Suggestion:
// Move dateRange state management to a context or prop
// Ensure hook dependencies properly track dateRange changes
useEffect(() => {
if (trigger) {
refetch(); // Assuming the hook supports refetch
}
}, [dateRange, trigger, refetch]);
Related Code:
const [dateRange, setDateRange] = useState("recent");
// Inside map:
useFetchUptimeMonitorById({
monitorId: monitor._id,
dateRange,
trigger: false,
});
💡 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 MonitorTimeFrameHeader from "@/Components/v1/MonitorTimeFrameHeader/index.jsx"; | ||
| import ChartBoxes from "./Components/ChartBoxes/index.jsx"; | ||
| import ResponseTimeChart from "./Components/Charts/ResponseTimeChart.jsx"; | ||
| import ResponseTimeChart from "../../../../Components/v1/Charts/ResponseTimeChart/ResponseTimeChart.jsx"; |
There was a problem hiding this comment.
P2 | Confidence: High
The component sharing between status page and uptime details is architecturally sound. However, the import path uses deep relative imports (../../../../) which are fragile and hard to maintain. Consider using the established @/ alias pattern for better maintainability.
| import ResponseTimeChart from "../../../../Components/v1/Charts/ResponseTimeChart/ResponseTimeChart.jsx"; | |
| import ResponseTimeChart from "@/Components/v1/Charts/ResponseTimeChart/ResponseTimeChart.jsx"; |
| showResponseTimeChart: { | ||
| type: Boolean, | ||
| default: true, | ||
| }, |
There was a problem hiding this comment.
P2 | Confidence: High
The backend changes properly maintain data consistency by adding the field to both the Mongoose schema and the aggregation projections in statusPageModule.js. The default value of true ensures backward compatibility, but consider documenting this default behavior since existing status pages will automatically show response time charts after deployment.
| <ResponseTimeChart | ||
| isLoading={monitorIsLoading} | ||
| groupedChecks={monitorData?.groupedChecks} | ||
| dateRange={dateRange} | ||
| /> |
There was a problem hiding this comment.
P2 | Confidence: Medium
The isLoading prop is incorrectly passed from the parent MonitorsList's loading state instead of the individual monitor's loading state (monitorIsLoading). This causes all charts to show loading skeletons even when only one monitor is loading, creating a poor user experience.
| <ResponseTimeChart | |
| isLoading={monitorIsLoading} | |
| groupedChecks={monitorData?.groupedChecks} | |
| dateRange={dateRange} | |
| /> | |
| <ResponseTimeChart | |
| isLoading={monitorIsLoading} | |
| groupedChecks={monitorData?.groupedChecks} | |
| dateRange={dateRange} | |
| /> |
Evidence: symbol:ResponseTimeChart, path:client/src/Components/v1/Charts/ResponseTimeChart/ResponseTimeChart.jsx
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
client/src/Pages/v1/StatusPage/Status/Components/MonitorsList/index.jsx (3)
25-25: Remove unusedsetDateRangesetter.The
dateRangestate is initialized butsetDateRangeis never used. If this is a placeholder for future functionality (e.g., allowing users to change the date range), consider adding a TODO comment. Otherwise, remove the setter or use a constant instead of state.Apply this diff if no future updates are planned:
- const [dateRange, setDateRange] = useState("recent"); + const dateRange = "recent";
48-48: Remove unusedmonitorStatsvariable.The
monitorStatsvariable is destructured from the hook but never used in the component. Consider removing it to reduce noise.Apply this diff:
- const [monitorData, monitorStats, monitorIsLoading] = useFetchUptimeMonitorById({ + const [monitorData, , monitorIsLoading] = useFetchUptimeMonitorById({
44-100: Add PropTypes toMonitorListItemfor consistency.The
MonitorListItemcomponent should define PropTypes for its props to maintain consistency with the parentMonitorsListcomponent and improve type safety.Add PropTypes after the component definition:
MonitorListItem.propTypes = { monitor: PropTypes.shape({ _id: PropTypes.string.isRequired, url: PropTypes.string, name: PropTypes.string, percentageColor: PropTypes.string, percentage: PropTypes.number, checks: PropTypes.array, }).isRequired, statusPage: PropTypes.object, showURL: PropTypes.bool, dateRange: PropTypes.string.isRequired, };
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/src/Pages/v1/StatusPage/Status/Components/MonitorsList/index.jsx(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-24T17:52:55.506Z
Learnt from: Jesulayomy
PR: bluewave-labs/Checkmate#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:
client/src/Pages/v1/StatusPage/Status/Components/MonitorsList/index.jsx
🧬 Code graph analysis (1)
client/src/Pages/v1/StatusPage/Status/Components/MonitorsList/index.jsx (3)
client/src/Hooks/v1/useMonitorUtils.js (1)
determineState(33-38)client/src/Hooks/v1/monitorHooks.js (2)
monitorStats(278-278)useFetchUptimeMonitorById(274-300)client/src/Components/v1/Charts/ResponseTimeChart/ResponseTimeChart.jsx (1)
ResponseTimeChart(7-23)
🔇 Additional comments (2)
client/src/Pages/v1/StatusPage/Status/Components/MonitorsList/index.jsx (2)
31-39: Excellent fix for the Rules of Hooks violation.The extraction of
MonitorListItemresolves the critical issue from the previous review. The hook is now called at the component's top level, maintaining proper React hooks ordering.
48-52: Correctly using per-monitor loading state.The hook is properly called at the component's top level, and
monitorIsLoadingis correctly used instead of the parentisLoading(line 92), resolving the major issue from the previous review.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
client/src/Pages/v1/Uptime/Details/index.jsx (1)
6-6: LGTM! Import path and prop usage are correct.The import now uses the @ alias pattern (addressing the previous review comment), and the prop correctly uses
monitorIsLoadingto match the updated ResponseTimeChart API.Also applies to: 171-175
client/src/Pages/v1/StatusPage/Status/Components/MonitorsList/index.jsx (2)
44-52: LGTM! Hook usage now complies with Rules of Hooks.The extraction of
MonitorListItemensuresuseFetchUptimeMonitorByIdis called at the top level, fixing the previous critical issue. The per-monitor data fetching is now architecturally sound.
84-97: LGTM! ResponseTimeChart correctly uses per-monitor loading state.The chart now receives
monitorIsLoadinginstead of the parent'sisLoading, ensuring accurate loading indicators for each monitor. The conditional rendering based onstatusPage.showResponseTimeChartcorrectly implements the feature flag.
🧹 Nitpick comments (1)
client/src/Pages/v1/StatusPage/Status/Components/MonitorsList/index.jsx (1)
25-25: Consider making dateRange configurable in the future.The hardcoded
"recent"value is appropriate for the current status page requirements. However, if future enhancements require different time ranges per status page, this could be refactored to accept dateRange as a prop or configuration.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
client/src/Components/v1/Charts/ResponseTimeChart/ResponseTimeChart.jsx(2 hunks)client/src/Pages/v1/StatusPage/Status/Components/MonitorsList/index.jsx(2 hunks)client/src/Pages/v1/Uptime/Details/index.jsx(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-24T17:52:55.506Z
Learnt from: Jesulayomy
PR: bluewave-labs/Checkmate#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:
client/src/Pages/v1/StatusPage/Status/Components/MonitorsList/index.jsx
🧬 Code graph analysis (1)
client/src/Pages/v1/StatusPage/Status/Components/MonitorsList/index.jsx (3)
client/src/Hooks/v1/useMonitorUtils.js (1)
determineState(33-38)client/src/Hooks/v1/monitorHooks.js (1)
useFetchUptimeMonitorById(274-300)client/src/Components/v1/Charts/ResponseTimeChart/ResponseTimeChart.jsx (1)
ResponseTimeChart(7-27)
🔇 Additional comments (1)
client/src/Components/v1/Charts/ResponseTimeChart/ResponseTimeChart.jsx (1)
7-14: LGTM! Component rename and prop update are correct.The typo fix (ResponseTImeChart → ResponseTimeChart) and prop rename (isLoading → monitorIsLoading) improve clarity and align with the per-monitor loading semantics.
| ResponseTimeChart.propTypes = { | ||
| isLoading: PropTypes.bool, | ||
| groupedChecks: PropTypes.array, | ||
| dateRange: PropTypes.string, | ||
| }; |
There was a problem hiding this comment.
Fix PropTypes: update isLoading to monitorIsLoading.
The PropTypes definition still references isLoading, but the component now uses monitorIsLoading. This mismatch will cause PropTypes validation warnings and confusion.
Apply this diff to fix the PropTypes definition:
ResponseTimeChart.propTypes = {
- isLoading: PropTypes.bool,
+ monitorIsLoading: PropTypes.bool,
groupedChecks: PropTypes.array,
dateRange: PropTypes.string,
};📝 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.
| ResponseTimeChart.propTypes = { | |
| isLoading: PropTypes.bool, | |
| groupedChecks: PropTypes.array, | |
| dateRange: PropTypes.string, | |
| }; | |
| ResponseTimeChart.propTypes = { | |
| monitorIsLoading: PropTypes.bool, | |
| groupedChecks: PropTypes.array, | |
| dateRange: PropTypes.string, | |
| }; |
🤖 Prompt for AI Agents
In client/src/Components/v1/Charts/ResponseTimeChart/ResponseTimeChart.jsx
around lines 29 to 33, the PropTypes object still defines isLoading but the
component uses monitorIsLoading; update the PropTypes key to monitorIsLoading:
PropTypes.bool (replacing isLoading) so the prop name matches the component and
avoids validation warnings.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
client/src/Pages/v1/StatusPage/Status/Components/MonitorsList/index.jsx (2)
14-14: Clean up unused imports and props.Line 14 imports
useStatebut it's never used (dateRange is a constant, not state). Lines 18-19 declareisLoadingandshouldRenderprops that are never referenced in the component body. These appear to be leftover artifacts from the refactoring that extracted MonitorListItem.Apply this diff to remove unused code:
-import { useState } from "react"; +// import { useState } from "react"; // Remove if truly unused const MonitorsList = ({ - isLoading = false, - shouldRender = true, monitors = [], statusPage = {}, }) => {Additionally, consider whether
dateRange = "recent"at line 25 should remain hardcoded or be made configurable via props for future flexibility.Also applies to: 18-19, 25-25
102-114: Optional: Enhance PropTypes specificity.The PropTypes provide basic validation but could be more precise. For instance,
monitor.checksis typed as a generic array, andstatusPageas a generic object.Consider adding more specific shapes if stricter validation would help catch bugs:
MonitorListItem.propTypes = { monitor: PropTypes.shape({ _id: PropTypes.string.isRequired, url: PropTypes.string, name: PropTypes.string, percentageColor: PropTypes.string, percentage: PropTypes.number, - checks: PropTypes.array, + checks: PropTypes.arrayOf(PropTypes.object), }).isRequired, - statusPage: PropTypes.object, + statusPage: PropTypes.shape({ + showCharts: PropTypes.bool, + showResponseTimeChart: PropTypes.bool, + }), showURL: PropTypes.bool, dateRange: PropTypes.string.isRequired, };
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
client/src/Components/v1/Charts/ResponseTimeChart/ResponseTimeChart.jsx(2 hunks)client/src/Pages/v1/StatusPage/Status/Components/MonitorsList/index.jsx(2 hunks)client/src/Pages/v1/Uptime/Details/index.jsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- client/src/Pages/v1/Uptime/Details/index.jsx
- client/src/Components/v1/Charts/ResponseTimeChart/ResponseTimeChart.jsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-24T17:52:55.506Z
Learnt from: Jesulayomy
PR: bluewave-labs/Checkmate#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:
client/src/Pages/v1/StatusPage/Status/Components/MonitorsList/index.jsx
🧬 Code graph analysis (1)
client/src/Pages/v1/StatusPage/Status/Components/MonitorsList/index.jsx (3)
client/src/Hooks/v1/useMonitorUtils.js (1)
determineState(33-38)client/src/Hooks/v1/monitorHooks.js (1)
useFetchUptimeMonitorById(274-300)client/src/Components/v1/Charts/ResponseTimeChart/ResponseTimeChart.jsx (1)
ResponseTimeChart(7-27)
🔇 Additional comments (2)
client/src/Pages/v1/StatusPage/Status/Components/MonitorsList/index.jsx (2)
48-52: Consider handling networkError from the fetch hook.The destructuring skips
networkError(the 4th return value fromuseFetchUptimeMonitorById). If a monitor fetch fails, users won't see any error indication—the chart may just remain in a loading state or show stale/empty data.Verify whether silent failure is acceptable, or add error handling:
const [monitorData, , monitorIsLoading, networkError] = useFetchUptimeMonitorById({ monitorId: monitor._id, dateRange, trigger: false, }); if (networkError) { return <Box>Error loading monitor data</Box>; // or appropriate error UI }
84-97: LGTM! ResponseTimeChart integration looks good.The conditional rendering correctly checks
statusPage.showResponseTimeChart !== false, and the component now receives the per-monitor loading state (monitorIsLoading) instead of the parent's loading state, addressing previous review feedback.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
client/src/Pages/v1/StatusPage/Status/Components/MonitorsList/index.jsx (4)
13-15: Remove unused import.
useStateis imported but not used. Drop it to avoid dead code.Apply this diff:
-import { useState } from "react";
43-52: Avoid per‑monitor fetch when charts are disabled.
useFetchUptimeMonitorByIdruns for every monitor even whenstatusPage.showResponseTimeChart === false, causing unnecessary network calls. Gate the hook by splitting into a base item (no fetch) and a “with chart” item (does fetch), and render conditionally.Apply these diffs to rename the current component and prep for split:
- function MonitorListItem({ monitor, statusPage, showURL, dateRange }) { + function MonitorListItemWithChart({ monitor, statusPage, showURL, dateRange }) {-MonitorListItem.propTypes = { +MonitorListItemWithChart.propTypes = { monitor: PropTypes.shape({ _id: PropTypes.string.isRequired, url: PropTypes.string, name: PropTypes.string, percentageColor: PropTypes.string, percentage: PropTypes.number, checks: PropTypes.array, }).isRequired, statusPage: PropTypes.object, showURL: PropTypes.bool, dateRange: PropTypes.string.isRequired, };Then add this base component (no fetch, no chart):
function MonitorListItem({ monitor, statusPage, showURL }) { const theme = useTheme(); const { determineState } = useMonitorUtils(); const status = determineState(monitor); return ( <Stack width="100%" gap={theme.spacing(2)}> <Host url={monitor.url} title={monitor.name} percentageColor={monitor.percentageColor} percentage={monitor.percentage} showURL={showURL} /> <Stack direction="row" alignItems="center" gap={theme.spacing(20)}> {statusPage.showCharts !== false && ( <Box flex={9}> <StatusPageBarChart checks={monitor?.checks?.slice().reverse()} /> </Box> )} <Box flex={statusPage.showCharts !== false ? 1 : 10}> <StatusLabel status={status} text={status} customStyles={{ textTransform: "capitalize" }} /> </Box> </Stack> </Stack> ); } MonitorListItem.propTypes = { monitor: PropTypes.shape({ _id: PropTypes.string.isRequired, url: PropTypes.string, name: PropTypes.string, percentageColor: PropTypes.string, percentage: PropTypes.number, checks: PropTypes.array, }).isRequired, statusPage: PropTypes.object, showURL: PropTypes.bool, };Finally, render the appropriate component in the map:
{monitors?.map((monitor) => statusPage.showResponseTimeChart !== false ? ( <MonitorListItemWithChart key={monitor._id} monitor={monitor} statusPage={statusPage} showURL={showURL} dateRange="recent" /> ) : ( <MonitorListItem key={monitor._id} monitor={monitor} statusPage={statusPage} showURL={showURL} /> ) )}Also applies to: 53-99
101-113: Tighten PropTypes fordateRange(optional).If values are constrained (e.g., "recent", "24h", "7d"), prefer
oneOf([...])to catch typos early.Example:
- dateRange: PropTypes.string.isRequired, + dateRange: PropTypes.oneOf(["recent", "24h", "7d", "30d"]).isRequired,
18-22: Align props with PropTypes (or remove unused props).
MonitorsListreceivesisLoadingandshouldRenderbut doesn’t use them, and they’re absent from PropTypes. Either remove them from the signature or add to PropTypes. Prefer removal to reduce API surface.Apply this diff to remove unused props:
-const MonitorsList = ({ - isLoading = false, - shouldRender = true, - monitors = [], - statusPage = {}, -}) => { +const MonitorsList = ({ monitors = [], statusPage = {} }) => {Also applies to: 115-118
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/src/Pages/v1/StatusPage/Status/Components/MonitorsList/index.jsx(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-24T17:52:55.506Z
Learnt from: Jesulayomy
PR: bluewave-labs/Checkmate#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:
client/src/Pages/v1/StatusPage/Status/Components/MonitorsList/index.jsx
🧬 Code graph analysis (1)
client/src/Pages/v1/StatusPage/Status/Components/MonitorsList/index.jsx (3)
client/src/Hooks/v1/useMonitorUtils.js (1)
determineState(33-38)client/src/Hooks/v1/monitorHooks.js (1)
useFetchUptimeMonitorById(274-300)client/src/Components/v1/Charts/ResponseTimeChart/ResponseTimeChart.jsx (1)
ResponseTimeChart(7-27)
🔇 Additional comments (2)
client/src/Pages/v1/StatusPage/Status/Components/MonitorsList/index.jsx (2)
30-38: Hooks fix via child component looks good.Mapping to a child component resolves Rules-of-Hooks and keeps per‑monitor scoping tidy. Key usage is correct.
90-94: Correct prop name for loading state.Passing
monitorIsLoadingtoResponseTimeChartmatches the component’s API and fixes earlier flashing.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
client/src/Pages/v1/StatusPage/Status/Components/MonitorsList/index.jsx (2)
16-21: Remove unused props or document why they're preserved.The
isLoadingandshouldRenderprops are declared in theMonitorsListsignature but never used in the component body. This appears to be leftover from the refactoring.If these props are no longer needed, apply this diff:
const MonitorsList = ({ - isLoading = false, - shouldRender = true, monitors = [], statusPage = {}, }) => {If they're preserved for future use, add a comment explaining why.
109-109: Consider refining the PropType for statusPage.The
statusPagePropType uses a genericPropTypes.object. For better type safety and documentation, specify the shape to include the fields used in this component:showChartsandshowResponseTimeChart.Apply this diff:
- statusPage: PropTypes.object, + statusPage: PropTypes.shape({ + showCharts: PropTypes.bool, + showResponseTimeChart: PropTypes.bool, + }),
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/src/Pages/v1/StatusPage/Status/Components/MonitorsList/index.jsx(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-24T17:52:55.506Z
Learnt from: Jesulayomy
PR: bluewave-labs/Checkmate#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:
client/src/Pages/v1/StatusPage/Status/Components/MonitorsList/index.jsx
🧬 Code graph analysis (1)
client/src/Pages/v1/StatusPage/Status/Components/MonitorsList/index.jsx (3)
client/src/Hooks/v1/useMonitorUtils.js (1)
determineState(33-38)client/src/Hooks/v1/monitorHooks.js (1)
useFetchUptimeMonitorById(274-300)client/src/Components/v1/Charts/ResponseTimeChart/ResponseTimeChart.jsx (1)
ResponseTimeChart(7-27)
🔇 Additional comments (3)
client/src/Pages/v1/StatusPage/Status/Components/MonitorsList/index.jsx (3)
13-14: LGTM!The imports are correctly added to support the new response time chart feature and per-monitor data fetching.
42-98: LGTM! Past review concerns addressed.The
MonitorListItemcomponent correctly:
- Calls the hook at the top level (fixes Rules of Hooks violation from past review)
- Passes
monitorIsLoadingtoResponseTimeChartinstead of the parent'sisLoading(fixes incorrect loading state from past review)- Implements the conditional rendering for
showResponseTimeChartwith the correct!== falsecheck to support the default-true behaviorBased on past review comments and relevant code snippets.
29-37: The hardcodeddateRange="recent"is intentional and correctly designed.The parent Status component has no dateRange state or prop, and MonitorsList was deliberately designed without a dateRange prop parameter. The hardcoded value is consistent with the current architecture—there's no evidence the parent should control the date range at this level. The code is functioning as intended.
Likely an incorrect or invalid review comment.
1 participant
Describe your changes
###Summary:
This PR introduces a new feature that allows users to toggle and view a Response Time Chart for monitors on the public status page. It adds both frontend and backend support to manage the visibility of this chart.
Key Changes:
🖥️ Frontend Updates
⚙️ Backend Updates
Screen.Recording.2025-10-17.at.12.00.17.AM-2.mov
Write your issue number after "Fixes "
Fixes #2924
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
Bug Fixes
New Features
Localization