feat: checks can now trigger incidents for status pages [sc-23915] #1056
Conversation
WalkthroughThe pull request adds incident management functionality by introducing a new interface and updating existing check constructs. A new Changes
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/cli/src/constructs/incident.ts (1)
3-3: Consider using an enum for better type safety.While using a union type for
IncidentSeverityworks, consider using TypeScript's enum for improved type safety, autocompletion, and easier refactoring.-type IncidentSeverity = 'MINOR' | 'MEDIUM' | 'MAJOR' | 'CRITICAL' +enum IncidentSeverity { + MINOR = 'MINOR', + MEDIUM = 'MEDIUM', + MAJOR = 'MAJOR', + CRITICAL = 'CRITICAL' +}packages/cli/src/constructs/project.ts (1)
111-111: Consider maintaining consistent ordering.The order of status page service check assignments differs between the data object definition (after status-page-service) and synthesize method (before status-page). While this doesn't affect functionality, consistent ordering would improve readability.
...this.synthesizeRecord(this.data['status-page-service']), - ...this.synthesizeRecord(this.data['status-page-service-check-assignment']), ...this.synthesizeRecord(this.data['status-page']), + ...this.synthesizeRecord(this.data['status-page-service-check-assignment']),packages/cli/src/constructs/status-page-service-check-assignment.ts (3)
23-23: Fix typo in JSDoc comment.There's a repetition in the parameter description: "status page assignment assignment configuration properties".
- * @param props status page assignment assignment configuration properties + * @param props status page service check assignment configuration properties
10-12: Enhance class documentation.The current JSDoc provides minimal information about what a Status Page Service Check Assignment is and how it's used. Consider expanding the documentation to explain its purpose in relation to triggering incidents, which is the main feature being implemented.
/** - * Creates a Check assignment for a Status Page Service + * Creates a Check assignment for a Status Page Service. + * + * This construct links a check to a status page service, allowing the check + * to trigger incidents on the status page when it fails. This is part of the + * incident management system for status pages. */
32-37: Consider adding validation in the synthesize method.The
synthesizemethod returns the object with properties but doesn't validate that the requiredstatusPageServiceIdis present. While TypeScript ensures it at compile time, adding runtime validation could prevent potential errors.synthesize () { + if (!this.statusPageServiceId) { + throw new Error(`${this.logicalId}: statusPageServiceId is required`) + } return { statusPageServiceId: this.statusPageServiceId, checkId: this.checkId, } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/cli/src/constructs/api-check.ts(1 hunks)packages/cli/src/constructs/browser-check.ts(1 hunks)packages/cli/src/constructs/check.ts(5 hunks)packages/cli/src/constructs/incident.ts(1 hunks)packages/cli/src/constructs/index.ts(1 hunks)packages/cli/src/constructs/multi-step-check.ts(1 hunks)packages/cli/src/constructs/project.ts(4 hunks)packages/cli/src/constructs/status-page-service-check-assignment.ts(1 hunks)packages/cli/src/constructs/tcp-check.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
packages/cli/src/constructs/incident.ts (1)
packages/cli/src/constructs/status-page-service.ts (1)
StatusPageService(14-39)
packages/cli/src/constructs/status-page-service-check-assignment.ts (2)
packages/cli/src/constructs/ref.ts (1)
Ref(1-10)packages/cli/src/constructs/project.ts (1)
Session(148-223)
packages/cli/src/constructs/project.ts (1)
packages/cli/src/constructs/status-page-service-check-assignment.ts (1)
StatusPageServiceCheckAssignment(13-38)
packages/cli/src/constructs/check.ts (3)
packages/cli/src/constructs/incident.ts (1)
IncidentTrigger(5-11)packages/cli/src/constructs/status-page-service-check-assignment.ts (1)
StatusPageServiceCheckAssignment(13-38)packages/cli/src/constructs/ref.ts (1)
Ref(1-10)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test - windows-latest
- GitHub Check: test - ubuntu-latest
🔇 Additional comments (14)
packages/cli/src/constructs/browser-check.ts (1)
95-95: Method call alignment looks good.The addition of
this.addStatusPageServiceCheckAssignments()follows the same pattern as the other check assignment methods, maintaining consistent structure in the constructor.packages/cli/src/constructs/api-check.ts (1)
207-207: Properly placed method call.The addition of
this.addStatusPageServiceCheckAssignments()follows the same pattern as the other check assignment methods in the constructor, maintaining consistent initialization order.packages/cli/src/constructs/tcp-check.ts (1)
94-94: Method call placement is appropriate.The
this.addStatusPageServiceCheckAssignments()method is correctly added after the existing assignments, following the same pattern as in other check types.packages/cli/src/constructs/multi-step-check.ts (1)
80-80: Method call sequence is consistent with other check types.The addition of
this.addStatusPageServiceCheckAssignments()is placed correctly after the other assignment methods, maintaining consistent initialization across all check constructs.packages/cli/src/constructs/index.ts (1)
34-35: LGTM: New module exports added correctly.The two new exports for status page service check assignments and incidents look good and follow the existing pattern.
packages/cli/src/constructs/incident.ts (1)
5-11: The interface is well-defined and contains all necessary properties.The
IncidentTriggerinterface provides a clear structure for incident creation with appropriate properties.packages/cli/src/constructs/project.ts (2)
40-40: New property added correctly to ProjectData interface.The status page service check assignment property has been added appropriately to track these assignments in the project.
59-59: Property initialization follows existing pattern.The initialization as an empty object matches the pattern used for other similar properties.
packages/cli/src/constructs/check.ts (5)
15-16: Imports correctly added.The necessary imports for
IncidentTriggerandStatusPageServiceCheckAssignmenthave been added appropriately.
103-108: Well-documented property with clear JSDoc comments.The JSDoc comments for the
triggerIncidentproperty provide clear documentation with a link to learn more about automated incidents.
172-172: Constructor initialization is correctly implemented.The
triggerIncidentproperty is properly initialized from the props in the constructor.
226-236: Method to add status page service check assignments is well-implemented.The
addStatusPageServiceCheckAssignments()method correctly handles creating assignments when a trigger is defined.However, I notice the
assignmentvariable is declared but appears unused, requiring an ESLint disable comment. Consider one of the following approaches:- // eslint-disable-next-line @typescript-eslint/no-unused-vars - const assignment = new StatusPageServiceCheckAssignment(...) + new StatusPageServiceCheckAssignment(...)Or if you need to track the reference:
- // eslint-disable-next-line @typescript-eslint/no-unused-vars const assignment = new StatusPageServiceCheckAssignment(...) + return assignment; // Return for potential future use
242-272:✅ Verification successful
Verify if triggerIncident should be included in synthesize method.
The
triggerIncidentproperty isn't included in the object returned by thesynthesize()method. Is this intentional? If the property needs to be passed to the backend, it should be added to the returned object.
🏁 Script executed:
#!/bin/bash # Check if other methods that use triggerIncident require it to be in the synthesized output # Look for any code that might expect triggerIncident in the synthesized output rg -A 3 -B 3 "triggerIncident.*synthesize" --glob "!*.test.*" # Check if there are any API requests that might need this property rg -A 3 -B 3 "triggerIncident.*api" --glob "!*.test.*" # Look at the StatusPageServiceCheckAssignment class implementation to see how it uses the data cat packages/cli/src/constructs/status-page-service-check-assignment.tsLength of output: 1290
Action Required: Confirm intentional exclusion of
triggerIncidentAfter reviewing the code and verifying against other constructs (e.g.,
StatusPageServiceCheckAssignment), there’s no evidence elsewhere in the repository that thetriggerIncidentproperty is expected in the synthesized output. Its omission appears intentional. If the backend ever requires it, the change can be revisited, but for now no modifications are needed.packages/cli/src/constructs/status-page-service-check-assignment.ts (1)
1-8: Implementation looks good.The implementation of the
StatusPageServiceCheckAssignmentclass is clean and follows the project's patterns. It correctly:
- Extends the
Constructbase class- Properly initializes properties from the provided props
- Registers with the Session management system
- Implements the synthesize method to provide the necessary output format
The interface definition is also appropriate with the
statusPageServiceIdas required andcheckIdas optional.Also applies to: 13-38
sorccu
force-pushed
the
simokinnunen/sc-23915/cli-incident-support-for-status-pages
branch
from
5bbbd41 to
d74d559
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/cli/src/constructs/check.ts (2)
226-236: Consider naming convention for unused variableThe method correctly handles status page service check assignments when a triggerIncident is configured. However, the variable
assignmentis explicitly marked as unused with an eslint-disable comment.Consider using a leading underscore to indicate an intentionally unused variable, which is a common TypeScript convention:
- // eslint-disable-next-line @typescript-eslint/no-unused-vars - const assignment = new StatusPageServiceCheckAssignment(... + const _assignment = new StatusPageServiceCheckAssignment(...This would make the intention clearer and eliminate the need for the eslint-disable comment.
243-251: Consider simplifying the IIFE patternThe current implementation uses an Immediately Invoked Function Expression (IIFE) to process the triggerIncident property, which works but adds some complexity.
Consider simplifying this to a more straightforward conditional:
- const triggerIncident = (() => { - if (this.triggerIncident) { - // Remove service from the payload, it is sent separately via - // StatusPageServiceCheckAssignment. - // eslint-disable-next-line @typescript-eslint/no-unused-vars - const { service, ...triggerIncident } = this.triggerIncident - return triggerIncident - } - })() + // Remove service from the payload, it is sent separately via StatusPageServiceCheckAssignment + const triggerIncident = this.triggerIncident + ? (({ service, ...rest }) => rest)(this.triggerIncident) + : undefinedOr even simpler:
- const triggerIncident = (() => { - if (this.triggerIncident) { - // Remove service from the payload, it is sent separately via - // StatusPageServiceCheckAssignment. - // eslint-disable-next-line @typescript-eslint/no-unused-vars - const { service, ...triggerIncident } = this.triggerIncident - return triggerIncident - } - })() + let processedTriggerIncident + if (this.triggerIncident) { + // Remove service from the payload, it is sent separately via StatusPageServiceCheckAssignment + const { service, ...rest } = this.triggerIncident + processedTriggerIncident = rest + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/cli/src/constructs/api-check.ts(1 hunks)packages/cli/src/constructs/browser-check.ts(1 hunks)packages/cli/src/constructs/check.ts(6 hunks)packages/cli/src/constructs/incident.ts(1 hunks)packages/cli/src/constructs/index.ts(1 hunks)packages/cli/src/constructs/multi-step-check.ts(1 hunks)packages/cli/src/constructs/project.ts(4 hunks)packages/cli/src/constructs/status-page-service-check-assignment.ts(1 hunks)packages/cli/src/constructs/tcp-check.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/cli/src/constructs/browser-check.ts
- packages/cli/src/constructs/index.ts
- packages/cli/src/constructs/api-check.ts
- packages/cli/src/constructs/tcp-check.ts
- packages/cli/src/constructs/incident.ts
- packages/cli/src/constructs/status-page-service-check-assignment.ts
- packages/cli/src/constructs/multi-step-check.ts
- packages/cli/src/constructs/project.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/cli/src/constructs/check.ts (3)
packages/cli/src/constructs/incident.ts (1)
IncidentTrigger(5-11)packages/cli/src/constructs/status-page-service-check-assignment.ts (1)
StatusPageServiceCheckAssignment(13-38)packages/cli/src/constructs/ref.ts (1)
Ref(1-10)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test - windows-latest
- GitHub Check: test - ubuntu-latest
🔇 Additional comments (6)
packages/cli/src/constructs/check.ts (6)
15-16: New imports are correctly added for new functionalityThe addition of
IncidentTriggerandStatusPageServiceCheckAssignmentimports provides the necessary types and classes for implementing the status page incident triggering functionality.
103-108: Well-documented new property with clear explanationThe JSDoc documentation for the
triggerIncidentproperty is thorough, clearly explaining its purpose and providing a link to additional documentation about automated incidents.
132-132: New property correctly added to Check classThe
triggerIncidentproperty is correctly defined as optional (?) and matches the type defined in the interface.
172-172: Constructor correctly initializes new propertyThe property is appropriately initialized from the props parameter, maintaining consistency with other property initializations in the constructor.
281-281: Correctly included in synthesized outputThe processed triggerIncident property is correctly included in the object returned by the synthesize method.
226-236:✅ Verification successful
Verify addStatusPageServiceCheckAssignments method is called
The method
addStatusPageServiceCheckAssignmentsis defined but not called within this file. According to the summary, this method should be called in the constructors of several classes.
🏁 Script executed:
#!/bin/bash # Search for calls to addStatusPageServiceCheckAssignments() in the repository rg "addStatusPageServiceCheckAssignments\(\)" packages/cli/src/constructs/ -lLength of output: 254
Verified: Method Invocation of addStatusPageServiceCheckAssignments is Correct
After checking the repository, the method defined in
packages/cli/src/constructs/check.tsis indeed invoked in the constructors of the following classes:
packages/cli/src/constructs/tcp-check.tspackages/cli/src/constructs/multi-step-check.tspackages/cli/src/constructs/browser-check.tspackages/cli/src/constructs/api-check.tsThis aligns with the intended design. No further modifications are required.
This simpler implementation is better suited for the underlying models.
Affected Components
Notes for the Reviewer
New Dependency Submission