Adds response time chart to status page #2925
Conversation
Enables display of response time charts on status pages. This feature allows users to visualize the performance of their monitors directly on the status page. It includes: - A new checkbox in the status page creation form to enable/disable the chart. - A new component to fetch and render the response time data. - A new API hook to fetch response time data for a specific monitor. - A date range selection component. A database migration is included to add the `showResponseTimeChart` field to existing status pages.
Moves hardcoded status page values into a central constants file. This enhances maintainability and ensures consistency across the application by using default values for status page configurations.
format files and make the checkbox for heartbeat more detailed as theres now 2 charts.
antonstrover
requested review from
Br0wnHammer,
Owaiseimdad,
ajhollid,
karenvicent and
mohicody
as code owners
WalkthroughAdds an optional “Response Time Chart” feature to status pages: new config toggle, persistence in model/validation/migrations, client defaults/constants, network submission, data fetching hook, wrapper component, list integration with date range control, and a minor chart component rename. Includes i18n updates and backend aggregation to surface the new field. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant PublicStatus as PublicStatus (Status Page)
participant MonitorsList
participant Wrapper as ResponseTimeChartWrapper
participant Hook as useMonitorResponseTimeData
participant API as NetworkService.getUptimeDetailsById
participant Chart as ResponseTimeChart
User->>PublicStatus: View status page
PublicStatus->>PublicStatus: dateRange = RECENT
alt statusPage.showResponseTimeChart
PublicStatus->>MonitorsList: monitors, statusPage, dateRange
loop For each monitor
MonitorsList->>Wrapper: monitorId, monitorName, dateRange
Wrapper->>Hook: { monitorId, dateRange, enabled: true }
Hook->>API: GET uptime details (monitorId, dateRange, normalize)
API-->>Hook: { groupedChecks }
Hook-->>Wrapper: groupedChecks, isLoading=false
Wrapper->>Chart: groupedChecks, dateRange, isLoading
Chart-->>User: Render response time chart
end
else
PublicStatus->>MonitorsList: monitors, statusPage (no charts)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Poem
✨ Finishing Touches
🧪 Generate unit tests
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: 3
🧹 Nitpick comments (7)
client/src/Utils/NetworkService.js (1)
935-937: FormData: boolean serialization is consistent; confirm server coercionAppending
String(form.showResponseTimeChart)matches the existing pattern for other booleans in this method. Please confirm server-side validation converts"true"/"false"strings to booleans for multipart forms, same as forshowCharts/showAdminLoginLink.If desired, factor a tiny helper to reduce repetition:
// inside createStatusPage(...) - if (form.showResponseTimeChart !== undefined) { - fd.append("showResponseTimeChart", String(form.showResponseTimeChart)); - } + const appendBool = (key, val) => val !== undefined && fd.append(key, String(val)); + appendBool("showResponseTimeChart", form.showResponseTimeChart);client/src/Pages/Uptime/Details/Components/Charts/ResponseTimeChart.jsx (1)
7-31: Use i18n for the chart headerHeader is hardcoded. Prefer existing
responseTimekey for consistency and localization.Apply:
+import { useTranslation } from "react-i18next"; -const ResponseTimeChart = ({ isLoading = false, groupedChecks = [], dateRange }) => { +const ResponseTimeChart = ({ isLoading = false, groupedChecks = [], dateRange }) => { + const { t } = useTranslation(); if (isLoading) { return <SkeletonLayout />; } return ( <ChartBox icon={<ResponseTimeIcon />} - header="Response Times" + header={t("responseTime")} >client/src/Pages/StatusPage/Status/Hooks/useMonitorResponseTimeData.jsx (1)
18-47: Prevent stale updates and clear loading when disabledGuard against setting state after unmount/param changes and ensure isLoading is reset when early-returning. This avoids flicker/race conditions if dateRange or monitor changes quickly.
useEffect(() => { - if (!enabled || !monitorId) { - setGroupedChecks([]); - return; - } + let cancelled = false; + if (!enabled || !monitorId) { + setGroupedChecks([]); + setIsLoading(false); + return; + } const fetchResponseTimeData = async () => { try { setIsLoading(true); setError(null); const res = await networkService.getUptimeDetailsById({ monitorId: monitorId, dateRange: dateRange, normalize: API_CONFIG.NORMALIZE_RESPONSE, }); - const { monitorData } = res?.data?.data ?? {}; - setGroupedChecks(monitorData?.groupedChecks || []); + if (cancelled) return; + const { monitorData } = res?.data?.data ?? {}; + setGroupedChecks(monitorData?.groupedChecks || []); } catch (error) { console.error("Error fetching response time data:", error); - setError(error); - setGroupedChecks([]); + if (!cancelled) { + setError(error); + setGroupedChecks([]); + } } finally { - setIsLoading(false); + if (!cancelled) setIsLoading(false); } }; fetchResponseTimeData(); - }, [monitorId, dateRange, enabled]); + return () => { + cancelled = true; + }; + }, [monitorId, dateRange, enabled]);Additionally, consider batching requests server-side for multiple monitors to reduce load when many charts are shown.
client/src/Pages/StatusPage/Status/Components/MonitorsList/ResponseTimeChartWrapper.jsx (2)
28-35: Minor: simplify spacing propMUI system props accept scale numbers; no need to call theme.spacing here.
- <Box mt={theme.spacing(CHART_SPACING.TOP_MARGIN)}> + <Box mt={CHART_SPACING.TOP_MARGIN}>
39-43: Constrain dateRange to known valuesHelps catch invalid props early.
- dateRange: PropTypes.string, + dateRange: PropTypes.oneOf(Object.values(DATE_RANGES)),client/src/Pages/StatusPage/Status/Components/MonitorsList/index.jsx (2)
62-68: Consider lazy-loading/virtualizing response-time chartsEach monitor triggers an API call; with many monitors this can spike client and server load. Options: IntersectionObserver to load when visible, expand-to-view, or batch endpoint.
79-80: Constrain dateRange propMatch the constants to prevent invalid values.
- dateRange: PropTypes.string, + dateRange: PropTypes.oneOf(Object.values(DATE_RANGES)),
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
client/src/Pages/StatusPage/Create/Components/Tabs/Content.jsx(1 hunks)client/src/Pages/StatusPage/Create/index.jsx(3 hunks)client/src/Pages/StatusPage/Status/Components/MonitorsList/ResponseTimeChartWrapper.jsx(1 hunks)client/src/Pages/StatusPage/Status/Components/MonitorsList/index.jsx(3 hunks)client/src/Pages/StatusPage/Status/Hooks/useMonitorResponseTimeData.jsx(1 hunks)client/src/Pages/StatusPage/Status/index.jsx(4 hunks)client/src/Pages/Uptime/Details/Components/Charts/ResponseTimeChart.jsx(2 hunks)client/src/Utils/NetworkService.js(1 hunks)client/src/Utils/statusPageConstants.js(1 hunks)client/src/Validation/validation.js(1 hunks)client/src/locales/en.json(1 hunks)server/src/db/models/StatusPage.js(1 hunks)server/src/db/mongo/migration/0002_addShowResponseTimeChart.js(1 hunks)server/src/db/mongo/migration/index.js(1 hunks)server/src/db/mongo/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/StatusPage/Status/Components/MonitorsList/index.jsxclient/src/Pages/StatusPage/Status/Hooks/useMonitorResponseTimeData.jsxclient/src/Pages/StatusPage/Status/index.jsx
🧬 Code graph analysis (8)
client/src/Pages/StatusPage/Status/Components/MonitorsList/ResponseTimeChartWrapper.jsx (3)
client/src/Utils/statusPageConstants.js (4)
DATE_RANGES(7-14)DATE_RANGES(7-14)CHART_SPACING(30-32)CHART_SPACING(30-32)client/src/Pages/StatusPage/Status/Hooks/useMonitorResponseTimeData.jsx (4)
useMonitorResponseTimeData(9-50)error(16-16)groupedChecks(15-15)isLoading(14-14)client/src/Pages/Uptime/Details/Components/Charts/ResponseTimeChart.jsx (1)
ResponseTimeChart(7-23)
client/src/Pages/StatusPage/Create/Components/Tabs/Content.jsx (2)
client/src/Components/Inputs/Checkbox/index.jsx (1)
Checkbox(47-105)client/src/Pages/StatusPage/Create/index.jsx (2)
form(40-53)handleFormChange(74-99)
client/src/Pages/StatusPage/Create/index.jsx (1)
client/src/Utils/statusPageConstants.js (2)
STATUS_PAGE_DEFAULTS(17-22)STATUS_PAGE_DEFAULTS(17-22)
client/src/Utils/NetworkService.js (1)
client/src/Pages/StatusPage/Create/index.jsx (1)
form(40-53)
server/src/db/mongo/migration/0002_addShowResponseTimeChart.js (2)
client/src/Pages/StatusPage/Create/index.jsx (1)
statusPage(69-70)client/src/Pages/StatusPage/Status/index.jsx (1)
statusPage(35-35)
client/src/Pages/StatusPage/Status/Components/MonitorsList/index.jsx (3)
client/src/Utils/statusPageConstants.js (2)
DATE_RANGES(7-14)DATE_RANGES(7-14)client/src/Pages/StatusPage/Status/index.jsx (2)
statusPage(35-35)dateRange(33-33)client/src/Pages/StatusPage/Status/Components/MonitorsList/ResponseTimeChartWrapper.jsx (1)
ResponseTimeChartWrapper(11-37)
client/src/Pages/StatusPage/Status/Hooks/useMonitorResponseTimeData.jsx (5)
client/src/Pages/StatusPage/Status/Components/MonitorsList/ResponseTimeChartWrapper.jsx (1)
useMonitorResponseTimeData(17-21)client/src/Utils/statusPageConstants.js (4)
DATE_RANGES(7-14)DATE_RANGES(7-14)API_CONFIG(25-27)API_CONFIG(25-27)client/src/Utils/NetworkService.js (1)
networkService(1146-1146)client/src/Pages/StatusPage/Status/index.jsx (1)
dateRange(33-33)client/src/Pages/Uptime/Details/index.jsx (1)
monitorData(51-56)
client/src/Pages/StatusPage/Status/index.jsx (3)
client/src/Utils/statusPageConstants.js (2)
DATE_RANGES(7-14)DATE_RANGES(7-14)client/src/Components/MonitorTimeFrameHeader/index.jsx (1)
MonitorTimeFrameHeader(6-76)client/src/Pages/StatusPage/Status/Components/MonitorsList/index.jsx (1)
MonitorsList(16-74)
🔇 Additional comments (10)
client/src/Validation/validation.js (1)
308-309: LGTM: new field wired into client validation
showResponseTimeChart: joi.boolean()aligns with existing booleans here and keeps it optional by default. No issues.server/src/validation/joi.js (1)
480-481: No action needed: updateStatusPage uses the create schema
updateStatusPage in server/src/controllers/statusPageController.js reuses createStatusPageBodyValidation, which already includesshowResponseTimeChart.server/src/db/models/StatusPage.js (1)
77-80: LGTM: schema field with sensible default
showResponseTimeChartadded withdefault: false. Matches UI behavior and validation.client/src/Pages/StatusPage/Create/Components/Tabs/Content.jsx (1)
93-99: LGTM: new checkbox is correctly wiredProps (
id,name,label,isChecked,onChange) match form state and translations. Nice.server/src/db/mongo/modules/statusPageModule.js (1)
100-114: LGTM: field surfaced in both “no monitors” path and aggregation
showResponseTimeChartis included in the early return and aggregation projection, ensuring parity across paths.Also applies to: 126-127, 233-234
client/src/Pages/StatusPage/Status/index.jsx (2)
32-34: LGTM: clean date-range state and header gatingState/init and conditional MonitorTimeFrameHeader wiring look correct and match the new feature toggle.
Also applies to: 163-170
174-174: LGTM: prop threadingPassing dateRange into MonitorsList ensures per-monitor charts align with the selected window.
client/src/Utils/statusPageConstants.js (1)
6-33: LGTM: centralized constantsGood consolidation of date ranges, defaults, API flags, and layout spacing.
client/src/Pages/StatusPage/Create/index.jsx (2)
21-21: LGTM: defaults centralized and wired through form/loadUsing STATUS_PAGE_DEFAULTS and adding showResponseTimeChart to ERROR_TAB_MAPPING and load-mapping looks correct.
Also applies to: 25-31, 49-53, 218-225
21-21: Confirm backend support for showResponseTimeChart
Joi schema (server/src/validation/joi.js:480) and StatusPage model (server/src/db/models/StatusPage.js:77-79) includeshowResponseTimeChart; migration 0002 and statusPageModule cover existing records.
| "showCharts": "Show Heartbeat Charts", | ||
| "showUptimePercentage": "Show uptime percentage", | ||
| "showResponseTimeChart": "Show Response Time Chart", | ||
| "removeLogo": "Remove Logo", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
LGTM: keys added/updated for new feature
showCharts label updated and showResponseTimeChart added. Consider syncing other locale files so this key isn’t missing outside English.
You can verify coverage with:
🏁 Script executed:
#!/bin/bash
rg -n -S '"showResponseTimeChart"\s*:' client/src/localesLength of output: 84
Add missing keys to all locale files
showResponseTimeChart (and the updated showCharts key) currently only exists in client/src/locales/en.json—sync these entries across every locale JSON in client/src/locales.
🤖 Prompt for AI Agents
In client/src/locales/en.json around lines 288–291, the keys
"showResponseTimeChart" and the updated "showCharts" currently exist only in
en.json; update every JSON file under client/src/locales to include the same
keys and values (or appropriate translations) so all locales stay in sync—open
each locale file and add "showCharts", "showUptimePercentage",
"showResponseTimeChart", and "removeLogo" with matching keys and translated
strings, then run a quick JSON lint to ensure no syntax errors.
| async function addShowResponseTimeChart() { | ||
| try { | ||
| const statusPages = await StatusPage.find({ showResponseTimeChart: { $exists: false } }); | ||
| for (const statusPage of statusPages) { | ||
| statusPage.showResponseTimeChart = false; | ||
| await statusPage.save(); | ||
| console.log(`Migrated status page ${statusPage._id}: showResponseTimeChart set to ${statusPage.showResponseTimeChart}`); | ||
| } | ||
| console.log("showResponseTimeChart migration complete."); | ||
| return true; | ||
| } catch (err) { | ||
| console.error("Migration error:", err); | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use a single bulk update (updateMany) instead of per-document saves
Iterating and saving each doc is slow and noisy. A single atomic update is faster and safer at scale.
- const statusPages = await StatusPage.find({ showResponseTimeChart: { $exists: false } });
- for (const statusPage of statusPages) {
- statusPage.showResponseTimeChart = false;
- await statusPage.save();
- console.log(`Migrated status page ${statusPage._id}: showResponseTimeChart set to ${statusPage.showResponseTimeChart}`);
- }
- console.log("showResponseTimeChart migration complete.");
- return true;
+ const res = await StatusPage.updateMany(
+ { showResponseTimeChart: { $exists: false } },
+ { $set: { showResponseTimeChart: false } }
+ );
+ console.log(
+ `showResponseTimeChart migration complete. matched=${res.matchedCount ?? res.matched} modified=${res.modifiedCount ?? res.modified}`
+ );
+ return res.acknowledged === true;📝 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.
| async function addShowResponseTimeChart() { | |
| try { | |
| const statusPages = await StatusPage.find({ showResponseTimeChart: { $exists: false } }); | |
| for (const statusPage of statusPages) { | |
| statusPage.showResponseTimeChart = false; | |
| await statusPage.save(); | |
| console.log(`Migrated status page ${statusPage._id}: showResponseTimeChart set to ${statusPage.showResponseTimeChart}`); | |
| } | |
| console.log("showResponseTimeChart migration complete."); | |
| return true; | |
| } catch (err) { | |
| console.error("Migration error:", err); | |
| return false; | |
| } | |
| } | |
| async function addShowResponseTimeChart() { | |
| try { | |
| const res = await StatusPage.updateMany( | |
| { showResponseTimeChart: { $exists: false } }, | |
| { $set: { showResponseTimeChart: false } } | |
| ); | |
| console.log( | |
| `showResponseTimeChart migration complete. matched=${res.matchedCount ?? res.matched} modified=${res.modifiedCount ?? res.modified}` | |
| ); | |
| return res.acknowledged === true; | |
| } catch (err) { | |
| console.error("Migration error:", err); | |
| return false; | |
| } | |
| } |
🤖 Prompt for AI Agents
In server/src/db/mongo/migration/0002_addShowResponseTimeChart.js around lines 3
to 17, the migration currently loads and saves each StatusPage document
individually which is slow and noisy; replace the per-document loop with a
single atomic updateMany call that matches { showResponseTimeChart: { $exists:
false } } and sets showResponseTimeChart to false for all matched docs, then log
the updateMany result (matchedCount/modifiedCount) and return success/failure
based on that result; keep the try/catch but remove per-document save and
console messages.
| import { addShowResponseTimeChart } from "./0002_addShowResponseTimeChart.js"; | ||
|
|
||
| const runMigrations = async () => { | ||
| await migrateStatusWindowThreshold(); | ||
| await addShowResponseTimeChart(); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fail fast on migration failure
Propagate a failed 0002 migration instead of silently continuing. This prevents starting with a partially migrated schema.
const runMigrations = async () => {
- await migrateStatusWindowThreshold();
- await addShowResponseTimeChart();
+ try {
+ await migrateStatusWindowThreshold();
+ const ok = await addShowResponseTimeChart();
+ if (!ok) throw new Error("addShowResponseTimeChart failed");
+ } catch (e) {
+ // Surface failure to the caller
+ throw e;
+ }
};📝 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.
| import { addShowResponseTimeChart } from "./0002_addShowResponseTimeChart.js"; | |
| const runMigrations = async () => { | |
| await migrateStatusWindowThreshold(); | |
| await addShowResponseTimeChart(); | |
| }; | |
| import { addShowResponseTimeChart } from "./0002_addShowResponseTimeChart.js"; | |
| const runMigrations = async () => { | |
| try { | |
| await migrateStatusWindowThreshold(); | |
| const ok = await addShowResponseTimeChart(); | |
| if (!ok) throw new Error("addShowResponseTimeChart failed"); | |
| } catch (e) { | |
| // Surface failure to the caller | |
| throw e; | |
| } | |
| }; |
🤖 Prompt for AI Agents
In server/src/db/mongo/migration/index.js around lines 2 to 7, the 0002
migration result must not be ignored — ensure any failure from
addShowResponseTimeChart causes runMigrations to fail fast. Do this by not
swallowing errors (remove any try/catch around the call) or, if
addShowResponseTimeChart returns a status instead of throwing, check its return
value and throw an Error when it indicates failure so the exception propagates
and prevents startup with a partially migrated schema.
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
Auto Pull Request Review from LlamaPReview
🎯 TL;DR & Recommendation
Recommendation: Request Changes
This PR adds response time charts to status pages but introduces a critical authentication issue that will break charts on public pages, alongside minor performance and maintainability concerns.
📄 Documentation Diagram
This diagram documents the new response time chart workflow on status pages, highlighting the data flow from user interaction to chart rendering.
sequenceDiagram
participant User as User
participant UI as Status Page UI
participant Hook as useMonitorResponseTimeData Hook
participant API as Backend API
participant Chart as ResponseTimeChart
User->>UI: Selects date range
UI->>Hook: Fetches data with monitorId and dateRange
Hook->>API: Calls getUptimeDetailsById
API-->>Hook: Returns response time data
Hook-->>UI: Provides groupedChecks
UI->>Chart: Renders chart with data
note over API: PR #2925 added response time fetch for status pages
🌟 Strengths
- Centralized constants enhance maintainability and consistency.
- Well-structured implementation with comprehensive UI changes and localization support.
| Priority | File | Category | Impact Summary | Anchors |
|---|---|---|---|---|
| P1 | client/src/Pages/StatusPage/Status/Hooks/useMonitorResponseTimeData.jsx | Architecture | Authentication issue breaks public status page charts | NetworkService.getUptimeDetailsById |
| P2 | client/src/Pages/StatusPage/Status/Components/MonitorsList/index.jsx | Performance | Multiple API calls could degrade performance with many monitors | |
| P2 | client/src/Utils/statusPageConstants.js | Maintainability | Date range inconsistencies risk breaking functionality | MonitorTimeFrameHeader |
| P2 | client/src/Pages/StatusPage/Status/Components/MonitorsList/ResponseTimeChartWrapper.jsx | Maintainability | Silent errors may confuse users when charts fail | |
| P2 | client/src/Pages/StatusPage/Create/index.jsx | Maintainability | Potential mismatch between client and server defaults | STATUS_PAGE_DEFAULTS |
🔍 Notable Themes
- Public Access Authentication: The core issue involves API calls that require authentication but are used in public contexts, necessitating a backend change for proper handling.
- Error Handling and User Feedback: Multiple findings highlight insufficient error reporting, which could lead to poor user experience when features fail silently.
📈 Risk Diagram
This diagram illustrates the authentication risk in the response time data fetch for public status pages.
sequenceDiagram
participant PublicUser as Public User
participant StatusPage as Status Page
participant Hook as useMonitorResponseTimeData
participant NetworkService as NetworkService
PublicUser->>StatusPage: Views public status page
StatusPage->>Hook: Attempts to fetch response time data
Hook->>NetworkService: Calls getUptimeDetailsById
NetworkService-->>Hook: Authentication error (likely 401)
Hook-->>StatusPage: Returns error, chart fails to load
note over NetworkService: R1(P1): Authentication required but not provided in public context
💡 LlamaPReview Community
Have feedback on this AI Code review tool? Join our GitHub Discussions to share your thoughts and help shape the future of LlamaPReview.
| setIsLoading(true); | ||
| setError(null); | ||
|
|
||
| const res = await networkService.getUptimeDetailsById({ |
There was a problem hiding this comment.
P1 | Confidence: High
The hook uses networkService.getUptimeDetailsById which likely requires authentication. However, status pages are public and don't provide authentication credentials. This will cause API calls to fail when viewed publicly, breaking response time charts on public status pages.
| showCharts: STATUS_PAGE_DEFAULTS.showCharts, | ||
| showUptimePercentage: STATUS_PAGE_DEFAULTS.showUptimePercentage, | ||
| showAdminLoginLink: STATUS_PAGE_DEFAULTS.showAdminLoginLink, | ||
| showResponseTimeChart: STATUS_PAGE_DEFAULTS.showResponseTimeChart, |
There was a problem hiding this comment.
P2 | Confidence: High
The constants centralization is good practice. However, ensure all default values align with model defaults (server/src/db/models/StatusPage.js) to avoid inconsistencies.
| /> | ||
| </Box> | ||
| </Stack> | ||
| {statusPage.showResponseTimeChart === true && ( |
There was a problem hiding this comment.
P2 | Confidence: Medium
This implementation triggers individual API calls for each monitor's response time data. For status pages with many monitors, this could cause performance issues due to multiple simultaneous API calls.
| */ | ||
|
|
||
| // Date range options for charts and data fetching | ||
| export const DATE_RANGES = { |
There was a problem hiding this comment.
P2 | Confidence: Medium
The date range values must match those expected by the MonitorTimeFrameHeader component and backend API. Inconsistencies could break date range functionality.
|
Hi @antonstrover, This looks like a nice feature, but there's a ton going on in this PR. PRs should be very focused on one specific fix/feature. IE if you want to wrap the chart in a wrapper, that should be a PR in its own right. If you want to refactor date ranges to a utility file, that should also be a separate PR. I understand that all these are related changes, but it is exponentially more difficult to review the PR safely and quickly if the changes are all in one large PR. My suggestion, if you'd like to get this reviewed in a timely manner, is to break it down into small, easily parsable PRs. Team member time is a very scarce resource; as such, they're not likely to be willing to commit to reviewing large and complex PRs. |
Thought i could get away with it, will start breaking it down |
|
Its going to be a while before i can actually get around to this, also not sure exactly how you'd want it in seperate PRs. |
2 participants
Describe your changes
I do not have a db with enough data to properly check functionality of the larger date ranges, maybe adding a script to scripts/dev/ that could seed a large dataset would be nice
Enables display of response time charts on status pages.
This feature allows users to visualize the performance of their monitors directly on the status page. It includes:
A database migration is included to add the
showResponseTimeChartfield to existing status pages.Write your issue number after "Fixes "
Fixes #2924 #2869
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.
npm run formatin server and client directories, which automatically formats your code.##Screenshots relating to UI changes:
Configuration content checkbox page:
'Recent' History Details page:
1 day History Details page:
1 Week history Details page:
1 Month history Details page (Not sure if this is a general bug in my code or just lack of data for it to display):
'Recent' History public page:
1 day History public page:
1 Week History public page:
1 month History public page (Not sure if this is a general bug in my code or just lack of data for it to display):
Enables display of response time charts on status pages.
This feature allows users to visualize the performance of their monitors directly on the status page. It includes:
A database migration is included to add the
showResponseTimeChartfield to existing status pages.Summary by CodeRabbit
New Features
Refactor
Chores