Fix incidents duplication front end #3086
Conversation
karenvicent
requested review from
Br0wnHammer,
Owaiseimdad and
ajhollid
as code owners
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Navigation & Routes client/src/Components/v1/Sidebar/index.jsx, client/src/Routes/index.jsx, client/src/Pages/v1/Incidents/index.jsx |
Added "Checks" menu entry and new checks/:monitorId? route; renamed Incidents component export to Checks; switched incidents/:monitorId? route to render new Incidents2. |
New Incidents2 Page client/src/Pages/v1/Incidents2/index.jsx |
New page composing Breadcrumbs, IncidentsSummaryPanel, OptionsHeader and IncidentTable; manages filters, pagination, monitor lookup and update trigger; wires to fetch hooks. |
Summary Panels & Reusable UI client/src/Pages/v1/Incidents2/Components/IncidentsSummaryPanel/index.jsx, client/src/Pages/v1/Incidents2/Components/SummaryCard/index.jsx, client/src/Pages/v1/Incidents2/Components/IncidentsSummaryPanel/skeleton.jsx |
New IncidentsSummaryPanel orchestrating sub-panels; SummaryCard reusable container; PanelSkeleton for loading states. |
Active / Latest / Statistics Panels client/src/Pages/v1/Incidents2/Components/ActiveIncidentsPanel/index.jsx, client/src/Pages/v1/Incidents2/Components/LatestIncidentsPanel/index.jsx, client/src/Pages/v1/Incidents2/Components/LatestIncidentsPanel/IncidentItem.jsx, client/src/Pages/v1/Incidents2/Components/StatisticsPanel/index.jsx |
Added panels: active incidents summary, latest incidents list (with IncidentItem and duration calc), and statistics panel (resolutions breakdown, averages). |
Options Header client/src/Pages/v1/Incidents2/Components/OptionsHeader/index.jsx |
New header with monitor selector, filter select, and date-range buttons; exposes selection callbacks via props. |
Incident Table client/src/Pages/v1/Incidents2/Components/IncidentTable/index.jsx |
New paginated incident table with loading/error/empty states, columns (monitor, status, times, resolution, HTTP status, message, actions) and resolve action flow. |
Data Fetching Hook client/src/Pages/v1/Incidents2/hooks/useFetchIncidents.js |
New hook providing fetchIncidentsByTeam, fetchActiveIncidents, fetchResolvedIncidents, fetchIncidentById, resolveIncident, fetchIncidentSummary; manages loading, count, errors and toasts. |
Network Client Extensions client/src/Utils/NetworkService.js |
Added getIncidentsByTeam, getIncidentById, resolveIncidentManually, getIncidentSummary; exported setNetworkService and networkService reference. |
Translations client/src/locales/en.json |
Added checks menu text, checksPageTitle, checks action keys, incidentsPage object and filter labels (including incidentsOptionsHeaderFilterActive) and other incident-related strings. |
Client UI Fix client/src/Components/v1/GenericFallback/index.jsx |
Replaced invalid Box prop with backgroundColor: "transparent" in sx styling for loading image. |
Server: DB & Service server/src/db/v1/modules/incidentModule.js, server/src/service/v1/business/incidentService.js |
getIncidentsByTeam added with date-range handling and monitor lookup; getIncidentSummary signature changed to accept { limit = 10 } and now returns aggregated metrics (totals, avg resolution hours, top monitor, latestIncidents); service layer updated to pass limit. |
Server Docs server/src/controllers/v1/incidentController.js |
JSDoc updated to reflect new summary parameters/response shape. |
Sequence Diagram(s)
sequenceDiagram
actor User
participant Page as Incidents2 (Page)
participant Hook as useFetchIncidents (Hook)
participant Client as NetworkService (Client)
participant API as Backend API
User->>Page: Open page / change filter / paginate
Page->>Hook: fetchIncidentsByTeam(config)
Hook->>Hook: set isLoading
Hook->>Client: getIncidentsByTeam(config)
Client->>API: GET /incidents/team (filters, page, rows)
API-->>Client: incidents[], incidentsCount
Client-->>Hook: response
Hook-->>Page: incidents, incidentsCount (isLoading=false)
Page->>Page: render IncidentTable
Page->>Hook: fetchIncidentSummary({ limit })
Hook->>Client: getIncidentSummary({ limit })
Client->>API: GET /incidents/team/summary?limit=...
API-->>Client: summary{totalActive, avgResolutionTimeHours, topMonitor, latestIncidents}
Client-->>Hook: summary
Hook-->>Page: summary
Page->>Page: render Active/Latest/Statistics panels
User->>IncidentTable: Click Resolve
IncidentTable->>Hook: resolveIncident(id)
Hook->>Client: resolveIncidentManually(id)
Client->>API: PUT /incidents/{id}/resolve
API-->>Client: success
Client-->>Hook: success
Hook->>Page: toggle updateTrigger
Page->>Hook: re-fetch incidents & summary
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~60 minutes
- Review focus:
- server/src/db/v1/modules/incidentModule.js — aggregation pipelines, index usage, monitor lookup correctness and performance
- client/src/Pages/v1/Incidents2/hooks/useFetchIncidents.js — state transitions, error/toast handling, concurrent requests
- client/src/Pages/v1/Incidents2/index.jsx and child components — prop wiring, pagination edge cases, updateTrigger correctness
- NetworkService additions — consistent error handling and API contract (query params)
- Translation keys and route/sidebar changes — ensure no missing keys or broken links
Poem
🐇 I hopped through code, a carrot in my paw,
New panels, checks, and tables—what a draw!
Filters flutter, summaries shine bright,
Incidents2 leaps into the night.
Hooray for fixes, snacks, and flight! 🥕✨
Pre-merge checks and finishing touches
❌ Failed checks (2 warnings, 1 inconclusive)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Linked Issues check | The PR addresses issue #2860 (successive failed pings as separate incidents) by implementing incidents aggregation logic and display infrastructure, but the PR lacks explicit backend logic to merge/collapse consecutive failures into single incidents. | Verify that backend incident aggregation logic prevents duplicate incidents for successive failures and that frontend correctly displays aggregated incidents; consider adding server-side deduplication or merge logic if not present. | |
| Out of Scope Changes check | The PR includes several out-of-scope changes: renaming Incidents page to Checks, creating a separate Checks route, and restructuring menu navigation—these exceed the stated objective of fixing incidents duplication. | Isolate incidents management changes into a separate, focused PR; move Checks page renaming, routing changes, and menu restructuring to a distinct PR to maintain PR granularity. | |
| Title check | ❓ Inconclusive | The PR title 'Fix incidents duplication front end' is vague and overly broad; it does not clearly convey the main implementation purpose (Incidents Management v1 with filtering, display, and resolution). | Revise title to better reflect the primary objective, e.g., 'Implement incidents management v1 with filtering and resolution' or 'Add incidents dashboard with summary, table, and manual resolution'. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description check | ✅ Passed | The PR description comprehensively covers the changes, linked issues, feature implementation, future enhancements, and includes all required checklist items marked complete with verification artifacts (screenshots). |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
fix-incidents-duplication-front-end
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (10)
client/src/Pages/v1/Incidents2/Components/LatestIncidentsPanel/IncidentItem.jsx (1)
10-10: UnusedonClickprop declared but not implemented.The
onClickprop is defined in PropTypes (line 150) but is never destructured from props (line 10) or attached to any element. The component hascursor: "pointer"styling (line 52) suggesting click functionality was intended.Either implement the click handler or remove the unused prop and cursor styling:
-const IncidentItem = ({ incident }) => { +const IncidentItem = ({ incident, onClick }) => {And attach it to the outer Box:
<Box + onClick={onClick} sx={{ cursor: "pointer",Or if click handling is not needed yet, remove the prop and cursor:
- cursor: "pointer",- onClick: PropTypes.func,Also applies to: 49-56, 139-151
server/src/db/v1/modules/incidentModule.js (1)
299-340: Multiple sequential aggregations may impact performance.
getIncidentSummaryexecutes 4 separate aggregate pipelines (countsPipeline,resolutionTimePipeline,monitorPipeline,latestIncidentsPipeline). While each is efficient individually, running them sequentially increases latency.Consider consolidating into a single aggregation using
$facetif performance becomes a concern:const combinedPipeline = [ { $match: matchStage }, { $facet: { counts: [...groupingStages], resolutionTime: [...resolutionStages], topMonitor: [...monitorStages], latestIncidents: [...latestStages] } } ];For now, this is acceptable given the improved code readability.
client/src/Components/v1/Sidebar/index.jsx (1)
51-52: Consider using theIncidentsicon for "Checks" instead ofDocs.The "Checks" menu item reuses the
Docsicon, but semantically "Checks" relates more to incident/monitoring functionality. Using a distinct or more relevant icon would improve visual differentiation from the actual "Docs" link in the other menu section (line 72).client/src/Pages/v1/Incidents2/Components/IncidentsSummaryPanel/skeleton.jsx (1)
1-2: Prefer named import for consistency.The import uses destructured named import from
@mui/materialwhich may bundle more than needed. Consider importing directly from the subpath:-import { Skeleton } from "@mui/material"; +import Skeleton from "@mui/material/Skeleton";This aligns with the pattern used elsewhere (e.g.,
SummaryCardimport) and can improve tree-shaking.client/src/Pages/v1/Incidents2/Components/LatestIncidentsPanel/index.jsx (1)
83-97: Consider removing unusedonIncidentClickprop.The
onIncidentClickprop is declared in PropTypes but is never used in the component implementation or passed to child components. This may be intended for future functionality (v2 enhancements mentioned in PR description), but currently creates a minor inconsistency.If this prop is not needed for current functionality, consider removing it from PropTypes. If it's reserved for future use, consider adding a JSDoc comment noting it's for future implementation.
client/src/Pages/v1/Incidents2/Components/OptionsHeader/index.jsx (1)
37-37: Consider explicit null return for clarity.The early return when
!shouldRenderreturnsundefinedimplicitly. While valid in React, explicitly returningnullwould be clearer and more conventional.-if (!shouldRender) return; +if (!shouldRender) return null;client/src/Pages/v1/Incidents2/Components/ActiveIncidentsPanel/index.jsx (1)
138-143: Consider removing unusedonIncidentClickprop.Similar to
LatestIncidentsPanel, theonIncidentClickprop is declared but never used. This is likely reserved for future functionality (v2 enhancements) but creates a minor inconsistency in the current implementation.client/src/Pages/v1/Incidents2/hooks/useFetchIncidents.js (1)
168-179: Addtto useCallback dependency array for correctness.The
fetchIncidentSummaryfunction usestfromuseTranslationbut doesn't include it in the dependency array. Whiletis typically stable, including it ensures correctness and satisfies exhaustive-deps.-}, []); +}, [t]);client/src/Pages/v1/Incidents2/Components/IncidentTable/index.jsx (2)
86-103: Consider making the resolution type color mapping more maintainable.The color logic hardcodes "manual" to accent and everything else to success. If additional resolution types (e.g., "auto", "scheduled", "api") are added in the future, they'll all default to the success color without explicit handling.
Consider creating a mapping object for better maintainability:
+const resolutionTypeColors = { + manual: "accent", + auto: "success", + // Add more types as needed +}; + { id: "resolutionType", content: t("incidentsPage.resolutionType"), render: (row) => { if (row.resolutionType) { + const colorKey = resolutionTypeColors[row.resolutionType] || "success"; return ( <Typography variant="body2" sx={{ textTransform: "capitalize", - color: - row.resolutionType === "manual" - ? theme.palette.accent.main - : theme.palette.success.main, + color: theme.palette[colorKey].main, }} > {row.resolutionType} </Typography> ); } return "-"; }, },
64-70: Document the startTime fallback behavior.The code falls back to
row.createdAtwhenstartTimeis missing. While this may be intentional (legacy data migration or backward compatibility), it should be documented to clarify why this fallback exists.Add a comment explaining the fallback:
render: (row) => { + // Fallback to createdAt for backward compatibility with older incident records const formattedDate = formatDateWithTz( row.startTime || row.createdAt, "YYYY-MM-DD HH:mm:ss A", uiTimezone ); return formattedDate; },
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
client/src/Components/v1/Sidebar/index.jsx(1 hunks)client/src/Pages/v1/Incidents/index.jsx(3 hunks)client/src/Pages/v1/Incidents2/Components/ActiveIncidentsPanel/index.jsx(1 hunks)client/src/Pages/v1/Incidents2/Components/IncidentTable/index.jsx(1 hunks)client/src/Pages/v1/Incidents2/Components/IncidentsSummaryPanel/index.jsx(1 hunks)client/src/Pages/v1/Incidents2/Components/IncidentsSummaryPanel/skeleton.jsx(1 hunks)client/src/Pages/v1/Incidents2/Components/LatestIncidentsPanel/IncidentItem.jsx(1 hunks)client/src/Pages/v1/Incidents2/Components/LatestIncidentsPanel/index.jsx(1 hunks)client/src/Pages/v1/Incidents2/Components/OptionsHeader/index.jsx(1 hunks)client/src/Pages/v1/Incidents2/Components/StatisticsPanel/index.jsx(1 hunks)client/src/Pages/v1/Incidents2/Components/SummaryCard/index.jsx(1 hunks)client/src/Pages/v1/Incidents2/hooks/useFetchIncidents.js(1 hunks)client/src/Pages/v1/Incidents2/index.jsx(1 hunks)client/src/Routes/index.jsx(2 hunks)client/src/Utils/NetworkService.js(1 hunks)client/src/locales/en.json(5 hunks)server/src/controllers/v1/incidentController.js(1 hunks)server/src/db/v1/modules/incidentModule.js(4 hunks)server/src/service/v1/business/incidentService.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (11)
client/src/Pages/v1/Incidents2/Components/IncidentsSummaryPanel/skeleton.jsx (1)
client/src/Pages/v1/Incidents2/Components/SummaryCard/index.jsx (1)
SummaryCard(5-61)
server/src/service/v1/business/incidentService.js (2)
client/src/Pages/v1/Incidents/index.jsx (1)
summary(40-42)client/src/Pages/v1/Incidents2/Components/IncidentsSummaryPanel/index.jsx (1)
summary(13-21)
client/src/Pages/v1/Incidents2/Components/IncidentsSummaryPanel/index.jsx (4)
client/src/Pages/v1/Incidents2/hooks/useFetchIncidents.js (2)
useFetchIncidents(24-194)fetchIncidentSummary(168-179)client/src/Pages/v1/Incidents2/Components/ActiveIncidentsPanel/index.jsx (1)
ActiveIncidentsPanel(23-136)client/src/Pages/v1/Incidents2/Components/LatestIncidentsPanel/index.jsx (1)
LatestIncidentsPanel(20-81)client/src/Pages/v1/Incidents2/Components/StatisticsPanel/index.jsx (1)
StatisticsPanel(33-226)
client/src/Pages/v1/Incidents2/Components/SummaryCard/index.jsx (10)
client/src/Components/v1/Sidebar/index.jsx (1)
theme(83-83)client/src/Pages/v1/Incidents/index.jsx (1)
theme(38-38)client/src/Pages/v1/Incidents2/Components/ActiveIncidentsPanel/index.jsx (1)
theme(24-24)client/src/Pages/v1/Incidents2/Components/IncidentTable/index.jsx (1)
theme(32-32)client/src/Pages/v1/Incidents2/Components/LatestIncidentsPanel/IncidentItem.jsx (1)
theme(12-12)client/src/Pages/v1/Incidents2/Components/LatestIncidentsPanel/index.jsx (1)
theme(21-21)client/src/Pages/v1/Incidents2/Components/OptionsHeader/index.jsx (1)
theme(20-20)client/src/Pages/v1/Incidents2/Components/StatisticsPanel/index.jsx (1)
theme(34-34)client/src/Pages/v1/Incidents2/index.jsx (1)
theme(54-54)client/src/Pages/v1/StatusPage/Status/index.jsx (1)
sx(38-38)
client/src/Pages/v1/Incidents2/Components/LatestIncidentsPanel/index.jsx (11)
client/src/Components/v1/Sidebar/index.jsx (2)
theme(83-83)useTranslation(84-84)client/src/Pages/v1/Incidents/index.jsx (2)
theme(38-38)useTranslation(23-23)client/src/Pages/v1/Incidents2/Components/ActiveIncidentsPanel/index.jsx (2)
theme(24-24)useTranslation(25-25)client/src/Pages/v1/Incidents2/Components/IncidentTable/index.jsx (2)
theme(32-32)useTranslation(31-31)client/src/Pages/v1/Incidents2/Components/LatestIncidentsPanel/IncidentItem.jsx (3)
theme(12-12)useTranslation(11-11)IncidentItem(10-137)client/src/Pages/v1/Incidents2/Components/OptionsHeader/index.jsx (2)
theme(20-20)useTranslation(21-21)client/src/Pages/v1/Incidents2/Components/StatisticsPanel/index.jsx (2)
theme(34-34)useTranslation(35-35)client/src/Pages/v1/Incidents2/Components/SummaryCard/index.jsx (2)
theme(6-6)SummaryCard(5-61)client/src/Pages/v1/Incidents2/index.jsx (2)
theme(54-54)useTranslation(20-20)client/src/Pages/v1/Incidents2/hooks/useFetchIncidents.js (3)
useTranslation(29-29)isLoading(27-27)incidents(25-25)client/src/Pages/v1/Incidents2/Components/IncidentsSummaryPanel/skeleton.jsx (1)
PanelSkeleton(8-18)
client/src/Pages/v1/Incidents2/Components/LatestIncidentsPanel/IncidentItem.jsx (1)
client/src/Components/v1/Label/index.jsx (1)
StatusLabel(135-156)
client/src/Utils/NetworkService.js (1)
client/src/Utils/timeUtils.js (1)
options(118-126)
client/src/Pages/v1/Incidents2/Components/IncidentTable/index.jsx (2)
client/src/Components/v1/Label/index.jsx (1)
StatusLabel(135-156)client/src/Utils/timeUtils.js (1)
formattedDate(135-135)
client/src/Pages/v1/Incidents2/Components/OptionsHeader/index.jsx (9)
client/src/Components/v1/Sidebar/index.jsx (2)
theme(83-83)useTranslation(84-84)client/src/Pages/v1/Incidents/index.jsx (6)
theme(38-38)useTranslation(23-23)monitors(39-39)selectedMonitor(28-28)filter(29-29)dateRange(30-30)client/src/Pages/v1/Incidents2/Components/ActiveIncidentsPanel/index.jsx (2)
theme(24-24)useTranslation(25-25)client/src/Pages/v1/Incidents2/Components/IncidentTable/index.jsx (2)
theme(32-32)useTranslation(31-31)client/src/Pages/v1/Incidents2/Components/LatestIncidentsPanel/IncidentItem.jsx (2)
theme(12-12)useTranslation(11-11)client/src/Pages/v1/Incidents2/Components/LatestIncidentsPanel/index.jsx (2)
theme(21-21)useTranslation(22-22)client/src/Pages/v1/Incidents2/Components/StatisticsPanel/index.jsx (2)
theme(34-34)useTranslation(35-35)client/src/Pages/v1/Incidents2/Components/SummaryCard/index.jsx (1)
theme(6-6)client/src/Pages/v1/Incidents2/hooks/useFetchIncidents.js (1)
useTranslation(29-29)
client/src/Pages/v1/Incidents2/Components/ActiveIncidentsPanel/index.jsx (4)
client/src/Pages/v1/Incidents/index.jsx (2)
theme(38-38)useTranslation(23-23)client/src/Pages/v1/Incidents2/Components/IncidentTable/index.jsx (2)
theme(32-32)useTranslation(31-31)client/src/Pages/v1/Incidents2/Components/SummaryCard/index.jsx (2)
theme(6-6)SummaryCard(5-61)client/src/Pages/v1/Incidents2/Components/IncidentsSummaryPanel/skeleton.jsx (1)
PanelSkeleton(8-18)
client/src/Pages/v1/Incidents2/hooks/useFetchIncidents.js (1)
client/src/Utils/NetworkService.js (1)
networkService(1187-1187)
🔇 Additional comments (24)
client/src/Pages/v1/Incidents/index.jsx (1)
21-25: LGTM! Component renamed consistently from Incidents to Checks.The renaming is applied uniformly across the component definition, breadcrumbs, action button labels, and the default export.
Also applies to: 87-88, 118-118
client/src/Pages/v1/Incidents2/Components/SummaryCard/index.jsx (1)
1-68: LGTM! Clean reusable component implementation.The SummaryCard component is well-structured with appropriate prop types and theme-based styling. Minor note: there's an empty line at line 41 inside the sx object that could be removed for cleaner formatting.
client/src/Utils/NetworkService.js (2)
1153-1182: LGTM! New incident API methods follow established patterns.The new methods (
getIncidentsByTeam,getIncidentById,resolveIncidentManually,getIncidentSummary) are consistent with existing NetworkService patterns and handle query parameters appropriately.
1187-1192: LGTM! Dependency injection pattern for NetworkService.The exported
setNetworkServicesetter andnetworkServicereference enable test doubles or alternate implementations to be injected, which is a good pattern for testability.client/src/locales/en.json (2)
1145-1190: LGTM! Comprehensive translation keys for the new Incidents feature.The
incidentsPagenamespace provides well-organized keys covering filters, panels, statuses, and error messages for the new incident management UI.
467-468: LGTM! New Checks-related translation keys added consistently.The
menu.checks,checksPageTitle, and action button translation keys align with the component renaming in the Incidents/index.jsx file.Also applies to: 477-477, 1068-1068, 1070-1070
server/src/db/v1/modules/incidentModule.js (4)
190-209: Date range filtering logic looks correct.The conditional logic properly handles the three status cases:
- Active incidents: no date filter (always visible)
- Resolved incidents: filter by
endTime- All incidents:
$orto combine bothThe empty
ifblock on lines 196-198 is intentional but could benefit from a brief inline comment explaining the no-op is deliberate.
218-259: Consider moving the$lookupafter$facetto reduce memory usage.The
$lookupis performed on all matched documents before pagination. For large datasets, this could be inefficient since monitor details are fetched for documents that will be skipped.However, given the
$facetstructure and the need to includemonitorNamein results, this approach is acceptable for the current use case. If performance becomes an issue with large incident volumes, consider restructuring the pipeline.
272-297: LGTM on counts pipeline structure.The grouping by status with resolution type counting is correct and efficient as a single pass through the data.
404-418: Return structure is well-documented and comprehensive.The summary object provides useful metrics for the UI. The rounding of
avgResolutionTimeHoursto 2 decimal places is appropriate.server/src/service/v1/business/incidentService.js (1)
212-236: Clean implementation aligning with module changes.The service correctly passes the
limitparameter to the database module. Error handling and logging remain consistent with other methods.client/src/Pages/v1/Incidents2/Components/IncidentsSummaryPanel/skeleton.jsx (1)
8-18: Simple and reusable skeleton implementation.The component correctly wraps
SummaryCardwith a rounded skeleton placeholder. TheminHeight: "25vh"provides consistent sizing across loading states.server/src/controllers/v1/incidentController.js (1)
73-110: Documentation accurately reflects the API changes.The JSDoc updates properly document:
- The new
limitparameter with default value of 10- The comprehensive return object structure including
totalActive,avgResolutionTimeHours,topMonitor, etc.- Updated example endpoint path
client/src/Routes/index.jsx (1)
32-33: LGTM! Route configuration is correct.The new routes for Checks and Incidents2 are properly configured and follow the existing routing patterns in the application.
Also applies to: 133-140
client/src/Pages/v1/Incidents2/index.jsx (2)
60-76: Verify fetch function stability and consider adding to dependencies.The useEffect calls
fetchActiveIncidents,fetchResolvedIncidents, orfetchIncidentsbut doesn't include them in the dependency array. While this works if these functions remain stable, examining theuseFetchIncidentshook reveals that these functions are not wrapped inuseCallback, meaning they're recreated on every render.This could lead to:
- ESLint warnings about missing dependencies
- Potential stale closure issues if the functions capture values that change
Consider either:
- Adding these functions to the dependency array (requires memoizing them in the hook)
- Using a ref to store the latest config and calling a stable fetch function
Review the
useFetchIncidentshook implementation to confirm whether the fetch functions should be memoized withuseCallback.
98-105: LGTM! Pagination handlers are correctly implemented.The pagination logic properly resets the page to 0 when
rowsPerPagechanges, which is good UX practice and prevents edge cases where the new page count might be less than the current page.client/src/Pages/v1/Incidents2/Components/IncidentsSummaryPanel/index.jsx (1)
26-50: LGTM! Summary fetching logic is well-structured.The useEffect correctly handles loading states, errors, and data updates. The dependency array properly includes
fetchIncidentSummary(which is memoized in the hook) andupdateTrigger.client/src/Pages/v1/Incidents2/Components/ActiveIncidentsPanel/index.jsx (1)
23-135: LGTM! Well-structured panel with clear state handling.The component properly handles all UI states (loading, error, zero count, active incidents) with appropriate visual feedback and theming. The conditional rendering logic is clear and maintainable.
client/src/Pages/v1/Incidents2/Components/OptionsHeader/index.jsx (1)
93-121: Remove the non-existentfilledprop from MUI Button.The
filledprop is not part of Material-UI's Button component API. MUI Button uses thevariantprop to control button styling (e.g.,variant="contained"for filled buttons). The current implementation passes an invalid prop that will be ignored. Either remove thefilledprop or use the correct MUI variant prop if you intend to change button styling based on thedateRangestate.Likely an incorrect or invalid review comment.
client/src/Pages/v1/Incidents2/Components/IncidentTable/index.jsx (5)
1-15: LGTM!The imports are well-organized, grouped logically (components, then utils/libraries), and all are utilized within the component.
17-32: LGTM!Component props are well-defined with sensible defaults, and the hooks (Redux selector, i18n, theme) are appropriately initialized.
149-157: LGTM!The loading and network error states are handled correctly with appropriate early returns and fallback components.
167-184: LGTM!The defensive array normalization and render logic are well-structured. The DataTable and Pagination components receive all necessary props correctly.
187-198: LGTM!PropTypes validation is comprehensive and correctly marks required vs. optional props (e.g.,
networkErroris appropriately optional).
client/src/Pages/v1/Incidents2/Components/IncidentTable/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 new incidents management system but has a critical UI inconsistency where the checks page displays incorrect titles, alongside several minor bugs and architectural coupling issues that could lead to silent failures or breakages.
📄 Documentation Diagram
This diagram documents the new incidents management workflow introduced in this PR.
sequenceDiagram
participant U as User
participant I as Incidents2 Component
participant H as useFetchIncidents Hook
participant B as Backend API
U->>I: Navigate to /incidents
I->>H: Fetch incidents summary and data
H->>B: Call getIncidentSummary and getIncidentsByTeam
B-->>H: Return incidents data
H-->>I: Update state with data
I->>I: Render summary panels and table
note over I: PR #35;3086 added new summary panels
I-->>U: Display incidents management interface
🌟 Strengths
- Comprehensive feature addition with new summary panels, filtering, and table components.
- Good use of hooks and modular design for incidents data fetching.
| Priority | File | Category | Impact Summary (≤12 words) | Anchors |
|---|---|---|---|---|
| P1 | client/src/Pages/v1/Incidents/index.jsx | Architecture | UI inconsistency: Checks page shows "Incidents" title incorrectly. | path:Routes/index.jsx |
| P2 | Pages/v1/Incidents2/index.jsx | Bug | State initialization could cause unexpected UI behavior with undefined filters. | method:fetchIncidentsByTeam |
| P2 | Pages/v1/Incidents2/hooks/useFetchIncidents.js | Architecture | Tight coupling with backend logic risks breakage on API changes. | path:server/.../incidentModule.js |
| P2 | Pages/v1/Incidents2/Components/StatisticsPanel/index.jsx | Bug | NaN calculation for resolution percentages may hide data errors. | – |
| P2 | Pages/v1/Incidents2/Components/IncidentTable/index.jsx | Maintainability | Unhandled promise rejection could lead to silent UI failures. | method:resolveIncident |
🔍 Notable Themes
- State Management Fragility: Multiple findings highlight reliance on undefined values and implicit backend contracts, increasing maintenance risk.
- Defensive Programming Gaps: Lack of null/undefined checks in calculations and error handling could obscure bugs in production.
📈 Risk Diagram
This diagram illustrates the risk of UI inconsistency due to incorrect translation keys after component renaming.
sequenceDiagram
participant U as User
participant R as Router
participant C as Checks Component
participant T as Translation Service
U->>R: Navigate to /checks
R->>C: Route to Checks component
C->>T: Request translation for "incidentsPageTitle"
T-->>C: Return "Incidents" (incorrect key)
C->>C: Render with "Incidents" title
note over C,T: R1(P1): UI inconsistency in page title
C-->>U: Display page titled "Incidents" instead of "Checks"
⚠️ **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/Incidents/index.jsx
This file is being renamed from Incidents to Checks and its default export changed. The Routes/index.jsx file confirms the route /incidents now points to Incidents2, and /checks points to this (renamed) component. However, the component's internal breadcrumb and UI text still reference "Incidents" (via the t("incidentsPageTitle") key). The user will navigate to /checks but see a page titled "Incidents". This is a direct user-facing inconsistency and a breaking change for the UI/UX. The translation key usage must be updated to "checksPageTitle" as added to the locale file.
Suggestion:
const BREADCRUMBS = [{ name: t("checksPageTitle"), path: "/checks" }];
Related Code:
const Incidents = () => {
// Redux state
const { t } = useTranslation();
const BREADCRUMBS = [
{ name: t("incidentsPageTitle", "Incidents"), path: "/incidents" },
];
💡 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.
| const [selectedMonitor, setSelectedMonitor] = useState(undefined); | ||
| const [filter, setFilter] = useState(undefined); | ||
| const [dateRange, setDateRange] = useState("all"); | ||
| const [page, setPage] = useState(0); | ||
| const [rowsPerPage, setRowsPerPage] = useState(10); |
There was a problem hiding this comment.
P2 | Confidence: High
The component initializes selectedMonitor and filter as undefined. However, the fetchIncidentsByTeam method (called in the effect) and the backend's getIncidentsByTeam function treat undefined values differently from explicit "all" filters. The backend expects monitorId: undefined and status: undefined to mean "no filter". While this works, it creates a hidden dependency on default behavior that could break if the backend logic changes. More importantly, the OptionsHeader component passes these values to child components without null-checking undefined, which could lead to unexpected UI behavior (like displaying "undefined" in dropdowns).
| const [selectedMonitor, setSelectedMonitor] = useState(undefined); | |
| const [filter, setFilter] = useState(undefined); | |
| const [dateRange, setDateRange] = useState("all"); | |
| const [page, setPage] = useState(0); | |
| const [rowsPerPage, setRowsPerPage] = useState(10); | |
| const [selectedMonitor, setSelectedMonitor] = useState("0"); // "0" represents "all monitors" | |
| const [filter, setFilter] = useState("all"); |
Evidence: method:fetchIncidentsByTeam, method:getIncidentsByTeam
| } catch (error) { | ||
| setNetworkError(true); | ||
| console.error(t("incidentsPage.errorFetchingIncidents"), error); | ||
| createToast({ body: error.message || t("incidentsPage.errorFetchingIncidents") }); |
There was a problem hiding this comment.
P2 | Confidence: High
The hook's fetchIncidentsByTeam directly passes the config object to networkService.getIncidentsByTeam. The related context shows the backend's getIncidentsByTeam method in incidentModule.js has complex, conditional date-range logic that differs for active vs. resolved vs. "all" incidents. The frontend's config object includes dateRange and status (via filter). If the status is undefined (representing "all"), the backend's logic applies an $or clause. This creates a tight, undocumented coupling between the frontend's state representation (undefined for "all") and the backend's aggregation pipeline. Changing one side without the other will break filtering. This is a high-risk architectural pattern for a core data-fetching hook.
| const totalResolutions = | ||
| summary?.totalManualResolutions + summary?.totalAutomaticResolutions; | ||
| const automaticPercentage = | ||
| totalResolutions > 0 | ||
| ? (summary?.totalAutomaticResolutions / totalResolutions) * 100 | ||
| : 0; |
There was a problem hiding this comment.
P2 | Confidence: High
The calculation for totalResolutions directly adds two potentially undefined values from the summary object. If either totalManualResolutions or totalAutomaticResolutions is undefined, the addition will result in NaN (e.g., undefined + 5 = NaN). This will cause totalResolutions to be NaN, making the condition totalResolutions > 0 evaluate to false, and automaticPercentage will be set to 0, hiding the calculation error. The UI may show a 0% bar incorrectly. The backend's getIncidentSummary method should always return numbers, but defensive programming is required.
| const totalResolutions = | |
| summary?.totalManualResolutions + summary?.totalAutomaticResolutions; | |
| const automaticPercentage = | |
| totalResolutions > 0 | |
| ? (summary?.totalAutomaticResolutions / totalResolutions) * 100 | |
| : 0; | |
| const manual = summary?.totalManualResolutions || 0; | |
| const auto = summary?.totalAutomaticResolutions || 0; | |
| const totalResolutions = manual + auto; | |
| const automaticPercentage = totalResolutions > 0 ? (auto / totalResolutions) * 100 : 0; |
| const handleResolveIncident = async (incidentId) => { | ||
| await resolveIncident(incidentId); | ||
| handleUpdateTrigger(); | ||
| }; |
There was a problem hiding this comment.
P2 | Confidence: Medium
The resolveIncident function from the hook returns a Promise, but the handleResolveIncident function does not handle potential rejection (network errors, server errors). The resolveIncident hook method already shows a toast on error, so the UI feedback exists. However, the await without a try/catch means an unhandled promise rejection will occur in the React component if the hook's error handling fails or throws. This is a minor maintainability and robustness issue. It's better to wrap the call and silently catch or explicitly handle the error at the call site.
Code Suggestion:
const handleResolveIncident = async (incidentId) => {
try {
await resolveIncident(incidentId);
handleUpdateTrigger();
} catch (error) {
// Error is already handled by resolveIncident hook's toast.
// Optional: Log for debugging.
console.debug('Incident resolution failed silently at UI layer:', error);
}
};
Evidence: method:resolveIncident, path:client/src/Pages/v1/Incidents2/hooks/useFetchIncidents.js
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
client/src/Pages/v1/Incidents2/hooks/useFetchIncidents.js (1)
42-58: Addtto the dependency array.The
fetchIncidentsByTeamfunction usestfromuseTranslation(line 53-54) but omits it from the dependency array. When the user changes language, the callback won't update, resulting in stale error messages.🔎 Apply this diff to fix the dependency array:
- }, []); + }, [t]);
🧹 Nitpick comments (3)
server/src/db/v1/modules/incidentModule.js (2)
169-200: Documentation is thorough but verbose.The extensive documentation explaining the status parameter contract is helpful for maintainability. However, it could be more concise while still conveying the essential information. Consider reducing the WARNING section and consolidating the parameter descriptions.
305-457: Consider consolidating aggregation pipelines for better performance.The method executes 4 separate aggregation queries against the incidents collection. While functionally correct, this could impact performance with large datasets. Consider using MongoDB's
$facetoperator to combine related pipelines (e.g., counts and resolution time) into a single aggregation.Note: Line 410 properly validates the
limitparameter withMath.max(1, parseInt(limit) || 10), which addresses the previous review comment about validating limit input.Example consolidation approach
const summaryPipeline = [ { $match: matchStage }, { $facet: { counts: [ { $group: { _id: "$status", count: { $sum: 1 }, manualResolutions: { $sum: { $cond: [{ $eq: ["$resolutionType", "manual"] }, 1, 0] } }, automaticResolutions: { $sum: { $cond: [{ $eq: ["$resolutionType", "automatic"] }, 1, 0] } } } } ], resolutionTime: [ { $match: { status: false, endTime: { $exists: true, $ne: null } } }, { $group: { _id: null, avgResolutionTime: { $avg: { $subtract: ["$endTime", "$startTime"] } } } } ], topMonitor: [ { $group: { _id: "$monitorId", count: { $sum: 1 } } }, { $sort: { count: -1 } }, { $limit: 1 }, // ... lookup and project ], latestIncidents: [ { $sort: { createdAt: -1 } }, { $limit: Math.max(1, parseInt(limit) || 10) }, // ... lookup and project ] } } ];This reduces database round-trips from 4 to 1.
client/src/Pages/v1/Incidents2/Components/StatisticsPanel/index.jsx (1)
228-232: Consider using PropTypes.shape for better type safety.The
summaryanderrorprops use genericPropTypes.object, which doesn't provide detailed validation. Consider usingPropTypes.shape()to document and validate the expected structure, especially forsummarywhich has multiple accessed properties.🔎 Example with more specific PropTypes:
StatisticsPanel.propTypes = { - summary: PropTypes.object, + summary: PropTypes.shape({ + total: PropTypes.number, + avgResolutionTimeHours: PropTypes.number, + topMonitor: PropTypes.shape({ + monitorName: PropTypes.string, + }), + totalManualResolutions: PropTypes.number, + totalAutomaticResolutions: PropTypes.number, + }), isLoading: PropTypes.bool, error: PropTypes.object, };
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
client/src/Components/v1/GenericFallback/index.jsx(1 hunks)client/src/Pages/v1/Incidents2/Components/IncidentTable/index.jsx(1 hunks)client/src/Pages/v1/Incidents2/Components/StatisticsPanel/index.jsx(1 hunks)client/src/Pages/v1/Incidents2/hooks/useFetchIncidents.js(1 hunks)client/src/Pages/v1/Incidents2/index.jsx(1 hunks)client/src/locales/en.json(5 hunks)server/src/db/v1/modules/incidentModule.js(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- client/src/locales/en.json
- client/src/Pages/v1/Incidents2/index.jsx
🧰 Additional context used
🧬 Code graph analysis (4)
client/src/Pages/v1/Incidents2/Components/IncidentTable/index.jsx (3)
client/src/Components/v1/GenericFallback/index.jsx (2)
theme(15-15)GenericFallback(14-83)client/src/Components/v1/Label/index.jsx (1)
StatusLabel(135-156)client/src/Utils/timeUtils.js (1)
formattedDate(135-135)
client/src/Pages/v1/Incidents2/hooks/useFetchIncidents.js (1)
client/src/Utils/NetworkService.js (1)
networkService(1187-1187)
server/src/db/v1/modules/incidentModule.js (4)
client/src/Pages/v1/Incidents2/index.jsx (3)
dateRange(28-28)page(29-29)rowsPerPage(30-30)client/src/Pages/v1/Incidents/index.jsx (1)
dateRange(30-30)server/src/db/v1/modules/monitorModuleQueries.js (1)
matchStage(1089-1089)client/src/Pages/v1/Incidents2/hooks/useFetchIncidents.js (1)
incidents(25-25)
client/src/Pages/v1/Incidents2/Components/StatisticsPanel/index.jsx (7)
client/src/Components/v1/GenericFallback/index.jsx (1)
theme(15-15)client/src/Pages/v1/Incidents/index.jsx (3)
theme(38-38)useTranslation(23-23)summary(40-42)client/src/Components/v1/Sidebar/index.jsx (2)
theme(83-83)useTranslation(84-84)client/src/Pages/v1/Incidents2/Components/LatestIncidentsPanel/index.jsx (2)
theme(21-21)useTranslation(22-22)client/src/Pages/v1/Incidents2/Components/SummaryCard/index.jsx (2)
theme(6-6)SummaryCard(5-61)client/src/Pages/v1/Incidents2/Components/OptionsHeader/index.jsx (2)
theme(20-20)useTranslation(21-21)client/src/Pages/v1/Incidents2/Components/IncidentsSummaryPanel/skeleton.jsx (1)
PanelSkeleton(8-18)
🔇 Additional comments (6)
client/src/Components/v1/GenericFallback/index.jsx (1)
49-49: LGTM! Correct use of MUI styling.Moving the background color styling into the
sxprop is the proper approach for MUI components, and explicitly settingbackgroundColor: "transparent"ensures the GIF's transparency is preserved.server/src/db/v1/modules/incidentModule.js (2)
226-242: Date range filtering logic is correct.The conditional date filtering with
$orlogic for the "all incidents" case (whenstatusBoolean === undefined) correctly handles the three scenarios:
- Active incidents: no date filter (they're ongoing)
- Resolved incidents: filter by
endTime- All incidents: show active + resolved within range
The MongoDB query structure properly ANDs the
$orclause with other filters inmatchStage.
251-291: Monitor lookup and projection implementation is solid.The
$lookupstage joins the monitors collection, and the$mapprojection correctly extractsmonitorNameusing$mergeObjects. While the nested structure is complex, it handles the array unpacking and null cases properly.client/src/Pages/v1/Incidents2/Components/IncidentTable/index.jsx (3)
34-41: LGTM! Error handling now prevents UI inconsistency.The
handleResolveIncidentfunction now properly wraps the API call in try-catch and only triggers the UI update on success. This addresses the past review concern about the UI refreshing even when the resolution fails.
163-169: LGTM! Empty state check now works correctly.The empty state logic now properly checks
incidents?.length === 0instead of the previousincidents == undefined, which correctly displays the "No incidents found" message when there are zero incidents.
43-151: Well-structured table headers with proper fallbacks.The headers array is comprehensive with appropriate render functions for each column. All render functions include proper fallbacks (e.g., "N/A", "-") for missing data, which prevents UI errors.
| /** | ||
| * StatisticsPanel Component | ||
| * | ||
| * Displays key metrics and statistics about incidents. | ||
| * | ||
| * @param {Object} props | ||
| * @param {Object} props.statistics - Statistics data object | ||
| * @param {number} props.statistics.total - Total incidents count | ||
| * @param {number} props.statistics.totalTrend - Trend percentage for total (optional) | ||
| * @param {string} props.statistics.mttr - Mean Time To Resolve (formatted string, e.g., "2h 15m") | ||
| * @param {number} props.statistics.mttrTrend - Trend percentage for MTTR (optional) | ||
| * @param {Object} props.statistics.topMonitor - Top monitor with most incidents | ||
| * @param {string} props.statistics.topMonitor.name - Monitor name | ||
| * @param {number} props.statistics.topMonitor.count - Incident count | ||
| * @param {number} props.statistics.topMonitor.percentage - Percentage of total | ||
| * @param {string} props.statistics.uptimeAffected - Total downtime (formatted string, e.g., "12h 45m") | ||
| * @param {number} props.statistics.availability - Availability percentage | ||
| * @param {boolean} props.isLoading - Loading state | ||
| * @param {Object} props.error - Error object if any | ||
| */ |
There was a problem hiding this comment.
Update JSDoc to match actual prop structure.
The JSDoc documents a statistics prop, but the component actually receives a summary prop (line 33). Additionally, the documented properties don't match the actual structure used in the render logic (e.g., mttr, totalTrend, uptimeAffected, availability are documented but never used; actual properties like avgResolutionTimeHours, totalManualResolutions, totalAutomaticResolutions are used but not documented).
🔎 Apply this diff to correct the JSDoc:
/**
* StatisticsPanel Component
*
* Displays key metrics and statistics about incidents.
*
* @param {Object} props
- * @param {Object} props.statistics - Statistics data object
- * @param {number} props.statistics.total - Total incidents count
- * @param {number} props.statistics.totalTrend - Trend percentage for total (optional)
- * @param {string} props.statistics.mttr - Mean Time To Resolve (formatted string, e.g., "2h 15m")
- * @param {number} props.statistics.mttrTrend - Trend percentage for MTTR (optional)
- * @param {Object} props.statistics.topMonitor - Top monitor with most incidents
- * @param {string} props.statistics.topMonitor.name - Monitor name
- * @param {number} props.statistics.topMonitor.count - Incident count
- * @param {number} props.statistics.topMonitor.percentage - Percentage of total
- * @param {string} props.statistics.uptimeAffected - Total downtime (formatted string, e.g., "12h 45m")
- * @param {number} props.statistics.availability - Availability percentage
+ * @param {Object} props.summary - Summary data object
+ * @param {number} props.summary.total - Total incidents count
+ * @param {number} props.summary.avgResolutionTimeHours - Average resolution time in hours
+ * @param {Object} props.summary.topMonitor - Top monitor with most incidents
+ * @param {string} props.summary.topMonitor.monitorName - Monitor name
+ * @param {number} props.summary.totalManualResolutions - Count of manual resolutions
+ * @param {number} props.summary.totalAutomaticResolutions - Count of automatic resolutions
* @param {boolean} props.isLoading - Loading state
* @param {Object} props.error - Error object if any
*/🤖 Prompt for AI Agents
In client/src/Pages/v1/Incidents2/Components/StatisticsPanel/index.jsx around
lines 13 to 32, the JSDoc currently documents a statistics prop that does not
match the component which actually receives a summary prop and different fields;
update the JSDoc to reference props.summary (not props.statistics) and document
the real fields used by the component (e.g., summary.total,
summary.avgResolutionTimeHours, summary.totalManualResolutions,
summary.totalAutomaticResolutions, summary.topMonitor { name, count, percentage
}, plus props.isLoading and props.error), remove the unused/incorrect entries
(mttr, totalTrend, uptimeAffected, availability, etc.), and ensure each field
has a brief type and description matching how the component reads them.
| const automaticPercentage = | ||
| totalResolutions > 0 | ||
| ? (summary?.totalAutomaticResolutions / totalResolutions) * 100 | ||
| : 0; |
There was a problem hiding this comment.
Use nullish-coalesced value for consistency in percentage calculation.
Line 40 uses summary?.totalAutomaticResolutions directly, but this could be undefined, resulting in NaN for automaticPercentage. For consistency with the totalResolutions calculation (lines 36-37), use a nullish-coalesced value.
🔎 Apply this diff to fix the inconsistency:
const totalResolutions =
(summary?.totalManualResolutions || 0) + (summary?.totalAutomaticResolutions || 0);
const automaticPercentage =
totalResolutions > 0
- ? (summary?.totalAutomaticResolutions / totalResolutions) * 100
+ ? ((summary?.totalAutomaticResolutions || 0) / totalResolutions) * 100
: 0;📝 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 automaticPercentage = | |
| totalResolutions > 0 | |
| ? (summary?.totalAutomaticResolutions / totalResolutions) * 100 | |
| : 0; | |
| const totalResolutions = | |
| (summary?.totalManualResolutions || 0) + (summary?.totalAutomaticResolutions || 0); | |
| const automaticPercentage = | |
| totalResolutions > 0 | |
| ? ((summary?.totalAutomaticResolutions || 0) / totalResolutions) * 100 | |
| : 0; |
🤖 Prompt for AI Agents
In client/src/Pages/v1/Incidents2/Components/StatisticsPanel/index.jsx around
lines 38 to 41, the percentage calculation uses
summary?.totalAutomaticResolutions which can be undefined and lead to NaN;
change the numerator to use a nullish-coalesced value (e.g.
summary?.totalAutomaticResolutions ?? 0) so the expression becomes safe and
consistent with the totalResolutions calculation, keeping the existing guard
(totalResolutions > 0) and percentage math unchanged.
| const fetchIncidentById = useCallback(async (incidentId) => { | ||
| if (!incidentId) { | ||
| console.error(t("incidentsPage.noIncidentIdProvided")); | ||
| return null; | ||
| } | ||
|
|
||
| try { | ||
| setIsLoading(true); | ||
| setNetworkError(false); | ||
|
|
||
| const res = await networkService.getIncidentById(incidentId); | ||
| return res.data?.data || null; | ||
| } catch (error) { | ||
| setNetworkError(true); | ||
| console.error(t("incidentsPage.errorFetchingIncident"), error); | ||
| createToast({ body: error.message || t("incidentsPage.errorFetchingIncident") }); | ||
| return null; | ||
| } finally { | ||
| setIsLoading(false); | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
Add t to the dependency array.
The fetchIncidentById function uses t from useTranslation (lines 112, 124, 125) but omits it from the dependency array. This can cause stale translations after language changes.
🔎 Apply this diff to fix the dependency array:
- }, []);
+ }, [t]);🤖 Prompt for AI Agents
In client/src/Pages/v1/Incidents2/hooks/useFetchIncidents.js around lines 110 to
130, the fetchIncidentById callback references the translation function t but
does not include it in the dependency array; update the useCallback dependency
array to include t so the function is recreated when translations change (i.e.,
change the final [] to [t]).
| const resolveIncident = useCallback(async (incidentId, options = {}) => { | ||
| if (!incidentId) { | ||
| console.error(t("incidentsPage.noIncidentIdProvided")); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| setIsLoading(true); | ||
| setNetworkError(false); | ||
|
|
||
| const res = await networkService.resolveIncidentManually(incidentId, options); | ||
| if (res.data?.success) { | ||
| createToast({ body: t("incidentsPage.incidentResolvedSuccessfully") }); | ||
| } else { | ||
| createToast({ body: t("incidentsPage.errorResolvingIncident") }); | ||
| } | ||
| } catch (error) { | ||
| setNetworkError(true); | ||
| console.error(t("incidentsPage.errorResolvingIncident"), error); | ||
| const errorMessage = | ||
| error.response?.data?.message || | ||
| error.message || | ||
| t("incidentsPage.errorResolvingIncident"); | ||
| createToast({ body: errorMessage }); | ||
| } finally { | ||
| setIsLoading(false); | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
Add t to the dependency array.
The resolveIncident function uses t from useTranslation (lines 143, 153, 155, 159, 163) but omits it from the dependency array. Without this, translated strings won't update when the user switches languages.
🔎 Apply this diff to fix the dependency array:
- }, []);
+ }, [t]);📝 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 resolveIncident = useCallback(async (incidentId, options = {}) => { | |
| if (!incidentId) { | |
| console.error(t("incidentsPage.noIncidentIdProvided")); | |
| return; | |
| } | |
| try { | |
| setIsLoading(true); | |
| setNetworkError(false); | |
| const res = await networkService.resolveIncidentManually(incidentId, options); | |
| if (res.data?.success) { | |
| createToast({ body: t("incidentsPage.incidentResolvedSuccessfully") }); | |
| } else { | |
| createToast({ body: t("incidentsPage.errorResolvingIncident") }); | |
| } | |
| } catch (error) { | |
| setNetworkError(true); | |
| console.error(t("incidentsPage.errorResolvingIncident"), error); | |
| const errorMessage = | |
| error.response?.data?.message || | |
| error.message || | |
| t("incidentsPage.errorResolvingIncident"); | |
| createToast({ body: errorMessage }); | |
| } finally { | |
| setIsLoading(false); | |
| } | |
| }, []); | |
| const resolveIncident = useCallback(async (incidentId, options = {}) => { | |
| if (!incidentId) { | |
| console.error(t("incidentsPage.noIncidentIdProvided")); | |
| return; | |
| } | |
| try { | |
| setIsLoading(true); | |
| setNetworkError(false); | |
| const res = await networkService.resolveIncidentManually(incidentId, options); | |
| if (res.data?.success) { | |
| createToast({ body: t("incidentsPage.incidentResolvedSuccessfully") }); | |
| } else { | |
| createToast({ body: t("incidentsPage.errorResolvingIncident") }); | |
| } | |
| } catch (error) { | |
| setNetworkError(true); | |
| console.error(t("incidentsPage.errorResolvingIncident"), error); | |
| const errorMessage = | |
| error.response?.data?.message || | |
| error.message || | |
| t("incidentsPage.errorResolvingIncident"); | |
| createToast({ body: errorMessage }); | |
| } finally { | |
| setIsLoading(false); | |
| } | |
| }, [t]); |
🤖 Prompt for AI Agents
In client/src/Pages/v1/Incidents2/hooks/useFetchIncidents.js around lines 141 to
168, the useCallback for resolveIncident references the translation function t
but omits it from the dependency array; update the dependency array to include t
so translations update when the language changes (ensure you add t to the array
while keeping other necessary dependencies as appropriate).
| const fetchIncidentSummary = useCallback(async (config = {}) => { | ||
| try { | ||
| const res = await networkService.getIncidentSummary(config); | ||
| return res.data?.data || null; | ||
| } catch (error) { | ||
| console.error(t("incidentsPage.errorFetchingIncidentSummary"), error); | ||
| createToast({ | ||
| body: error.message || t("incidentsPage.errorFetchingIncidentSummary"), | ||
| }); | ||
| return null; | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
Add t to the dependency array.
The fetchIncidentSummary function uses t from useTranslation (lines 182, 184) but omits it from the dependency array. This will prevent error messages from updating when the language changes.
🔎 Apply this diff to fix the dependency array:
- }, []);
+ }, [t]);📝 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 fetchIncidentSummary = useCallback(async (config = {}) => { | |
| try { | |
| const res = await networkService.getIncidentSummary(config); | |
| return res.data?.data || null; | |
| } catch (error) { | |
| console.error(t("incidentsPage.errorFetchingIncidentSummary"), error); | |
| createToast({ | |
| body: error.message || t("incidentsPage.errorFetchingIncidentSummary"), | |
| }); | |
| return null; | |
| } | |
| }, []); | |
| const fetchIncidentSummary = useCallback(async (config = {}) => { | |
| try { | |
| const res = await networkService.getIncidentSummary(config); | |
| return res.data?.data || null; | |
| } catch (error) { | |
| console.error(t("incidentsPage.errorFetchingIncidentSummary"), error); | |
| createToast({ | |
| body: error.message || t("incidentsPage.errorFetchingIncidentSummary"), | |
| }); | |
| return null; | |
| } | |
| }, [t]); |
🤖 Prompt for AI Agents
In client/src/Pages/v1/Incidents2/hooks/useFetchIncidents.js around lines 177 to
188, the useCallback for fetchIncidentSummary captures the translation function
t but does not include it in the dependency array; update the hook's dependency
array to include t (e.g., change [] to [t]) so the callback is recreated when
the translation function changes and error messages reflect language updates.
Describe your changes
Incidents Management - Version 1
Overview
Initial implementation of the incidents management system with basic functionality for fetching, displaying, and resolving incidents.
Features Included (v1)
✅ Fetch incidents by team with filtering (status, monitor, date range, pagination)
✅ Display active and resolved incidents
✅ Manual incident resolution
✅ Incident summary statistics panel
✅ Basic incident table with pagination
Future Enhancements (v2)
🔜 Comment options for incident resolution
🔜 Confirmation modal for manual resolution
🔜 Incident detail view/modal
Write your issue number after "Fixes "
Fixes #2860 #3006
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
New Features
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.