Repository navigation
Migration to OpenStatus from Atlassian StatusPage - #1933
Conversation
…, and fixtures Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…gration Add DB write service with phased transactions and per-resource error handling, tRPC router with preview/run endpoints, and register on the lambda router. Also enrich provider output with sourceGroupId on components and sourceComponentIds on maintenances for proper relational linking. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@aggmoulik is attempting to deploy a commit to the OpenStatus Team on Vercel. A member of the Team first needs to authorize it. |
mxkaske
left a comment
There was a problem hiding this comment.
make sure to align with the current dependencies (and lets fix the versions)
There was a problem hiding this comment.
Pull request overview
Adds a new @openstatus/importers package with a Statuspage (Atlassian) importer and wires it into the API to preview and execute migrations into OpenStatus.
Changes:
- Introduces a Statuspage client, API type schemas, mapping utilities, and provider implementation (with Bun tests/fixtures).
- Adds API service + TRPC router endpoints to preview and run imports, including phase-by-phase DB writes and idempotency.
- Registers the new router in the API lambda entrypoint and adds the importers workspace dependency.
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/importers/tsconfig.json | Creates a TS build config for the new importers package. |
| packages/importers/package.json | Defines the new @openstatus/importers package exports and scripts. |
| packages/importers/src/types.ts | Adds shared import/provider result and config types. |
| packages/importers/src/index.ts | Exposes public API and a provider registry. |
| packages/importers/src/providers/statuspage/api-types.ts | Adds Zod schemas/types for Statuspage API payloads. |
| packages/importers/src/providers/statuspage/client.ts | Implements a typed Statuspage API client. |
| packages/importers/src/providers/statuspage/mapper.ts | Maps Statuspage entities into OpenStatus-ready shapes. |
| packages/importers/src/providers/statuspage/provider.ts | Orchestrates fetching + mapping into phased import summaries. |
| packages/importers/src/providers/statuspage/index.ts | Exports Statuspage provider/client/types. |
| packages/importers/src/providers/statuspage/fixtures.ts | Adds fixtures for provider/client/mapper tests. |
| packages/importers/src/providers/statuspage/*.test.ts | Adds Bun tests for client, mapper, and provider. |
| packages/api/package.json | Adds @openstatus/importers workspace dependency. |
| packages/api/src/service/import.ts | Implements preview/run import flows and DB phase writers. |
| packages/api/src/router/import.ts | Adds TRPC endpoints for previewing and running an import. |
| packages/api/src/router/import.test.ts | Adds integration-style tests for preview/run + idempotency. |
| packages/api/src/lambda.ts | Registers the new importRouter. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Moulik Aggarwal <qwertymoulik@gmail.com>
Signed-off-by: Moulik Aggarwal <qwertymoulik@gmail.com>
|
@claude review |
|
@claude review |
|
Claude Code is working… I'll analyze this and get back to you. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 19 changed files in this pull request and generated 9 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async getIncidents(pageId: string): Promise<StatuspageIncident[]> { | ||
| return this.request( | ||
| `/pages/${pageId}/incidents`, | ||
| z.array(StatuspageIncidentSchema), | ||
| ); | ||
| } | ||
|
|
||
| async getScheduledIncidents(pageId: string): Promise<StatuspageIncident[]> { | ||
| return this.request( | ||
| `/pages/${pageId}/incidents/scheduled`, | ||
| z.array(StatuspageIncidentSchema), | ||
| ); | ||
| } | ||
|
|
||
| async getSubscribers(pageId: string): Promise<StatuspageSubscriber[]> { | ||
| return this.request( | ||
| `/pages/${pageId}/subscribers`, | ||
| z.array(StatuspageSubscriberSchema), | ||
| ); | ||
| } |
There was a problem hiding this comment.
The Statuspage API for /pages/{page_id}/incidents and /pages/{page_id}/subscribers returns paginated results (defaulting to 25 items per page). The current getIncidents and getSubscribers client methods only fetch the first page, so organizations with more than 25 incidents or subscribers will have their data silently truncated. The client should implement pagination (e.g., looping through pages until all results are collected) for these endpoints.
| "./statuspage": "./src/providers/statuspage/index.ts", | ||
| "./statuspage/fixtures": "./src/providers/statuspage/fixtures.ts" |
There was a problem hiding this comment.
The fixtures.ts file is exposed as a public package entry point ("./statuspage/fixtures": "./src/providers/statuspage/fixtures.ts") in package.json. While this is useful for sharing test fixtures across packages, exporting test-specific data from the production package creates a code smell. Consider moving it to a dedicated test utilities package or a separate export namespace that signals it is not production API surface.
| "./statuspage": "./src/providers/statuspage/index.ts", | |
| "./statuspage/fixtures": "./src/providers/statuspage/fixtures.ts" | |
| "./statuspage": "./src/providers/statuspage/index.ts" |
| case "webhook": | ||
| return { | ||
| email: "webhook@imported.openstatus.dev", | ||
| pageId, | ||
| channelType: "webhook", | ||
| webhookUrl: subscriber.endpoint ?? "", |
There was a problem hiding this comment.
The mapSubscriber function sets email: "webhook@imported.openstatus.dev" for webhook subscribers (line 156). However, the pageSubscriber database schema enforces a CHECK constraint requiring email IS NULL when channelType = 'webhook'. This placeholder email will cause a constraint violation when the subscriber is inserted. The email field should be null for webhook subscribers (and the return type should reflect this). The hardcoded value was likely intended as a placeholder but it conflicts with the database-level constraint.
| const existingBySlug = await db | ||
| .select() | ||
| .from(page) | ||
| .where(eq(page.slug, data.slug)) | ||
| .get(); | ||
|
|
||
| if (existingBySlug) { | ||
| resource.openstatusId = existingBySlug.id; | ||
| resource.status = "skipped"; | ||
| phase.status = "completed"; | ||
| return existingBySlug.id; |
There was a problem hiding this comment.
The slug-based idempotency check in writePagePhase (lines 234–244) queries for a page by data.slug without also filtering by workspaceId. Since page.slug is globally unique across all workspaces, this could return a page belonging to a different workspace. If another workspace already has a page with the same slug as the one being imported (e.g., a common subdomain), the import would silently use that foreign page as the target and write components, incidents, and subscribers into it. The query should additionally filter by eq(page.workspaceId, workspaceId) to scope the lookup to the current workspace only.
| // Insert status report | ||
| const [insertedReport] = await db | ||
| .insert(statusReport) | ||
| .values({ | ||
| title: data.report.title, | ||
| status: data.report.status, | ||
| workspaceId, | ||
| pageId, | ||
| }) | ||
| .returning({ id: statusReport.id }); | ||
|
|
||
| if (!insertedReport) { | ||
| resource.status = "failed"; | ||
| resource.error = "Failed to insert status report"; | ||
| continue; | ||
| } | ||
|
|
||
| // Insert status report updates | ||
| if (data.updates.length > 0) { | ||
| await db.insert(statusReportUpdate).values( | ||
| data.updates.map((u) => ({ | ||
| status: u.status, | ||
| message: u.message, | ||
| date: u.date, | ||
| statusReportId: insertedReport.id, | ||
| })), | ||
| ); | ||
| } | ||
|
|
||
| // Link to page components | ||
| const componentLinks: Array<{ | ||
| statusReportId: number; | ||
| pageComponentId: number; | ||
| }> = []; | ||
| for (const sourceCompId of data.sourceComponentIds) { | ||
| const osCompId = componentIdMap.get(sourceCompId); | ||
| if (osCompId) { | ||
| componentLinks.push({ | ||
| statusReportId: insertedReport.id, | ||
| pageComponentId: osCompId, | ||
| }); | ||
| } | ||
| } | ||
| if (componentLinks.length > 0) { | ||
| await db.insert(statusReportsToPageComponents).values(componentLinks); | ||
| } | ||
|
|
||
| resource.openstatusId = insertedReport.id; | ||
| resource.status = "created"; | ||
| } catch (err) { | ||
| resource.status = "failed"; | ||
| resource.error = err instanceof Error ? err.message : String(err); | ||
| } | ||
| } | ||
|
|
||
| phase.status = computePhaseStatus(phase.resources); | ||
| } |
There was a problem hiding this comment.
The writeIncidentsPhase and writeMaintenancesPhase functions have no idempotency check — they always insert a new status report or maintenance record without checking whether one with the same title and pageId already exists. Re-running the import (which is supported as shown in the idempotency test) will create duplicate status reports and maintenance records. An idempotency check (e.g., by title + pageId) should be added, similar to how writeComponentsPhase and writeComponentGroupsPhase check by name + pageId.
| provider: z.enum(["statuspage"]), | ||
| apiKey: z.string().min(1), | ||
| statuspagePageId: z.string().optional(), | ||
| }), | ||
| ) | ||
| .query(async (opts) => { | ||
| return previewImport({ | ||
| apiKey: opts.input.apiKey, | ||
| statuspagePageId: opts.input.statuspagePageId, | ||
| workspaceId: opts.ctx.workspace.id, | ||
| }); | ||
| }), |
There was a problem hiding this comment.
The provider input field is accepted and validated by both preview and run procedures (enforcing z.enum(["statuspage"])), but it is never actually used to dispatch to the correct provider — both procedures always instantiate StatuspageImportProvider directly in the service layer. As more providers are added to IMPORT_PROVIDERS, callers could pass provider: "someOtherProvider" and the system would silently run the Statuspage importer instead. The provider input should be forwarded to the service and used to select the correct provider implementation.
| const [inserted] = await db | ||
| .insert(pageSubscriber) | ||
| .values({ | ||
| email: data.email, | ||
| pageId, | ||
| channelType: "webhook", | ||
| webhookUrl: data.webhookUrl, | ||
| }) |
There was a problem hiding this comment.
In writeSubscribersPhase, when inserting a webhook subscriber (lines 624–631), the code sets email: data.email where data.email is the hardcoded string "webhook@imported.openstatus.dev" from the mapper. However, the pageSubscriber table has a CHECK constraint that requires email to be NULL for webhook channel type (the constraint is: channelType = 'webhook' AND webhookUrl IS NOT NULL AND email IS NULL). Passing a non-null email for a webhook subscriber will violate this constraint and cause a database error at runtime. The email field should be set to null (or omitted) when inserting webhook subscribers.
…assertion Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Resources marked as "skipped" by the component limit truncation were being processed and overwritten by writeComponentsPhase. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Create a fresh page instead of relying on seeded pageId: 1 to ensure predictable DB state for the component limit enforcement test. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Create a fresh page instead of relying on seeded pageId: 1 to ensure predictable DB state for the component limit enforcement test. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@claude review |
|
Claude finished @mxkaske's task in 2m 26s —— View job Code Review: StatusPage Importer
Great feature addition overall! The architecture is clean and the phased import approach with idempotency is solid. Several issues need to be addressed before merging though. Critical Issues1. No idempotency for incidents/maintenances (
2. The client has both 3. The Design Issues4. Both 5. Double
6. Using 7. Router naming inconsistency ( importRouter: importRouter, // inconsistentAll other routers follow Minor Issues8. from: new Date(incident.scheduled_for as string), // cast, not guaranteed safeThe 9. Test fixtures shouldn't be part of the production package API surface. Remove 10. TypeScript version uses caret ( Per @mxkaske's suggestion: 11. Form default value type mismatch ( defaultValues: {
provider: undefined, // type is z.enum(["statuspage"]), undefined is not valid12. Typo ( "Import a specific page. Leave empty to import accross pages." → across 13. Maintenance skipped by The SummaryThe overall structure is well thought out. Must-fix before merging: idempotency for incidents/maintenances, the API key security issue with GET requests, and the unused |
No description provided.