Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR addresses “orphaned” status page components that reference monitors after monitors are deleted (soft-deleted), and adjusts page component limit enforcement to validate the requested target state rather than the existing state.
Changes:
- Validate
pageComponent.updateOrderagainst the incoming total component count (including grouped components) instead of blocking based on current DB count. - On monitor deletion (single and bulk), delete
pageComponentrows referencing the deleted monitor(s) to avoid orphaned components. - Add equivalent page component cleanup to the server RPC
DeleteMonitorimplementation.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/api/src/router/pageComponent.ts | Enforces page component limits based on the incoming “new” component count. |
| packages/api/src/router/monitor.ts | Deletes monitor-linked pageComponent rows during monitor delete / bulk delete. |
| apps/server/src/routes/rpc/services/monitor/index.ts | Cleans up related junction tables and pageComponent rows on RPC monitor deletion. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Validate the incoming component count against the limit | ||
| const newComponentCount = | ||
| opts.input.components.length + | ||
| opts.input.groups.reduce((sum, g) => sum + g.components.length, 0); | ||
|
|
||
| if (newComponentCount > pageComponentLimit) { | ||
| throw new TRPCError({ |
There was a problem hiding this comment.
The new limit validation changes behavior but there’s no test covering the boundary cases (exactly at the limit should succeed; exceeding the limit via components+group components should throw). Please add/adjust tests for pageComponent.updateOrder to lock in this logic and prevent regressions.
| .delete(maintenancesToMonitors) | ||
| .where(inArray(maintenancesToMonitors.monitorId, opts.input.ids)); | ||
| await tx | ||
| .delete(pageComponent) | ||
| .where(inArray(pageComponent.monitorId, opts.input.ids)); | ||
| }); |
There was a problem hiding this comment.
Same atomicity concern as the single-delete path: the soft-delete update is executed before the cleanup transaction. Wrapping both the monitor update and the related deletes in one transaction would avoid partially-applied deletions if the cleanup fails.
No description provided.