Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
2 issues found across 16 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/importers/src/providers/instatus/mapper.ts">
<violation number="1" location="packages/importers/src/providers/instatus/mapper.ts:142">
P2: `to` and `from` share the same `Date` reference when `duration` is 0 or null. Any downstream mutation of one will silently affect the other. Create a new `Date` to avoid reference aliasing.</violation>
</file>
<file name="packages/importers/src/providers/instatus/client.ts">
<violation number="1" location="packages/importers/src/providers/instatus/client.ts:68">
P2: `getPages` doesn't paginate, unlike all other list endpoints. The Instatus API returns at most 50 pages per request by default. Use `requestAllPages` so that accounts with more than 50 status pages aren't silently truncated.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@claude review |
|
Claude finished @mxkaske's task in 1m 45s —— View job Code Review: feat/instatus-importer
Overall this is a solid, well-structured implementation. Good test coverage, clean separation between the client/mapper/provider layers, idempotency checks throughout the write phase, and sensible limit enforcement. A few real bugs and UX issues worth addressing before merge. Bugs1. Date reference aliasing in const to = duration > 0 ? new Date(from.getTime() + duration * 60_000) : from;When const to = duration > 0 ? new Date(from.getTime() + duration * 60_000) : new Date(from);2. getPages: () => request("/v2/pages", z.array(InstatusPageSchema)),Every other list endpoint uses getPages: () => requestAllPages("/v2/pages", InstatusPageSchema),3. Silent no-op when let pages = await client.getPages();
if (config.instatusPageId) {
pages = pages.filter((p) => p.id === config.instatusPageId);
}If the user passes an UX Issues4. Misleading description for Instatus Page ID field ( <FormDescription>
Import a specific page. Leave empty to import the first page.
</FormDescription>The actual behavior (per 5. Import button blocked by non-fatal plan warnings ( disabled={
!previewMutation.data ||
isPending ||
previewMutation.data.errors.length > 0
}
Minor Notes
|
There was a problem hiding this comment.
2 issues found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/importers/src/providers/instatus/provider.ts">
<violation number="1" location="packages/importers/src/providers/instatus/provider.ts:56">
P2: Use the already-declared `startedAt` variable instead of creating a new `Date`. The successful return path (line 188) uses the captured `startedAt`, so the error path should be consistent.</violation>
<violation number="2" location="packages/importers/src/providers/instatus/provider.ts:77">
P2: The `warnings` returned by `partitionComponents` are silently discarded. These warnings inform users when a referenced component group wasn't found and a synthetic group was created. Previously they were surfaced in the result's `errors` array. Either collect them into `errors` or remove the warning logic from `partitionComponents` if it's intentionally no longer needed.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/importers/src/providers/instatus/mapper.test.ts">
<violation number="1" location="packages/importers/src/providers/instatus/mapper.test.ts:208">
P3: This test is a duplicate of "maps email subscriber with specific components" above, which already asserts the full result including `sourceComponentIds: ["in_comp_001", "in_comp_003"]` via `toEqual`. If the intent is to verify extraction from *object* references (as opposed to plain string IDs), the test should use a distinct fixture whose `components` field contains object shapes rather than reusing `MOCK_SUBSCRIBERS[1]` with the same expected output.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| }); | ||
| }); | ||
|
|
||
| it("extracts component IDs from object references", () => { |
There was a problem hiding this comment.
P3: This test is a duplicate of "maps email subscriber with specific components" above, which already asserts the full result including sourceComponentIds: ["in_comp_001", "in_comp_003"] via toEqual. If the intent is to verify extraction from object references (as opposed to plain string IDs), the test should use a distinct fixture whose components field contains object shapes rather than reusing MOCK_SUBSCRIBERS[1] with the same expected output.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/importers/src/providers/instatus/mapper.test.ts, line 208:
<comment>This test is a duplicate of "maps email subscriber with specific components" above, which already asserts the full result including `sourceComponentIds: ["in_comp_001", "in_comp_003"]` via `toEqual`. If the intent is to verify extraction from *object* references (as opposed to plain string IDs), the test should use a distinct fixture whose `components` field contains object shapes rather than reusing `MOCK_SUBSCRIBERS[1]` with the same expected output.</comment>
<file context>
@@ -204,6 +205,11 @@ describe("mapSubscriber", () => {
});
});
+ it("extracts component IDs from object references", () => {
+ const result = mapSubscriber(MOCK_SUBSCRIBERS[1], 10);
+ expect(result?.sourceComponentIds).toEqual(["in_comp_001", "in_comp_003"]);
</file context>
No description provided.