[VW-38] Asset uploadIntegration Endpoint #58
Conversation
* asset detail WIP, seed script includes issues, deleted mprocs * asset detail page nearly finished * working pagination * deviceGroup * undo seed file * undo seed file part 2 * linting * use client, fix to db seed * DRY-ified code and added IssueStatus.ACTIVE migration * updated cpe to deviceGroup.cpe * linting and tsc check * properly invalidating query, fixed render and infinite update bugs, other touch ups * rolled back infinite update fix & linting --------- Co-authored-by: taylor <taylor@hadron.lan> Co-authored-by: Cassidy Diamond <cassidy.diamond@bugcrowd.com>
also adds ability to rotate integration api keys, migration to add ExternalVulnerabilityMapping renames `asset` column on `ExternalAssetMapping` to `item`. My logic here is that if we have to duplicate `ExternalVulnerabilityMapping`, at least if they both use fields with the same name (`item`, vs "asset" and "vulnerability") we can re-use code for both of them also renames some integration trpc endpoints for consistency, removes depreciated Google fonts
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
📝 WalkthroughWalkthroughThis pull request adds asset integration upload processing with test coverage, introduces a comprehensive asset detail view component displaying issues/vulnerabilities with tabbed status filters, and implements backend logic for synchronizing external assets via integration with error handling and sync status tracking. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Server
participant PrismaDB as Database/Prisma
Client->>Server: POST /integrationUpload<br/>(assets array + API key)
Server->>PrismaDB: Find Integration by API key
PrismaDB-->>Server: Integration record
loop For each asset in input
Server->>PrismaDB: Check ExternalAssetMapping
alt Mapping exists
PrismaDB-->>Server: Mapping found
Server->>PrismaDB: Update mapping.lastSynced
Server->>PrismaDB: Update Asset record
else Mapping not found
Server->>PrismaDB: Lookup Asset by unique fields
alt Asset exists
PrismaDB-->>Server: Asset found
Server->>PrismaDB: Update Asset
else Asset not found
Server->>PrismaDB: Compute deviceGroup (cpeToDeviceGroup)
Server->>PrismaDB: Create Asset + ExternalAssetMapping
end
end
end
Server->>PrismaDB: Persist SyncStatus record<br/>(success/error state)
PrismaDB-->>Server: SyncStatus created
Server-->>Client: Response with counts,<br/>retry flag, syncedAt
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/features/issues/components/issue.tsx (2)
165-195: “Active Issues” rendering assumes the input list is already filtered.
visibleIssuesand the overflow count use the fullissueslist. If the list contains non-active statuses, the “Active Issues” label and counts become incorrect. Consider explicitly filtering active issues before slicing/counting.🛠️ Suggested fix
- const isIssuesOverflow = issues.length > ACTIVE_ISSUES_SHOWN_MAX; - const visibleIssues = issues.slice( + const activeIssues = issues.filter((i) => i.status === IssueStatus.ACTIVE); + const isIssuesOverflow = activeIssues.length > ACTIVE_ISSUES_SHOWN_MAX; + const visibleIssues = activeIssues.slice( 0, - isIssuesOverflow ? ACTIVE_ISSUES_SHOWN_MAX : issues.length, + isIssuesOverflow ? ACTIVE_ISSUES_SHOWN_MAX : activeIssues.length, );
163-311: Links should not always point to the first issue’s asset.When
type === "vulnerabilities", issues can span multiple assets. Usingissues[0].assetIdfor “View All Active Issues” and non‑active links can send users to the wrong asset. Consider routing bytype, or suppressing asset links in the vulnerability context.🛠️ Suggested fix
- const assetId = issues[0].assetId; + const assetId = issues[0].assetId; + const vulnerabilityId = issues[0].vulnerabilityId; + const contextHref = + type === "assets" + ? `/assets/${assetId}` + : `/vulnerabilities/${vulnerabilityId}`;- <Link + <Link className="text-primary hover:underline" target="_blank" rel="noopener noreferrer" - href={`/assets/${assetId}`} + href={contextHref} > View All Active Issues </Link>- <Link + <Link key={i} className="text-primary hover:underline" target="_blank" rel="noopener noreferrer" - href={`/assets/${assetId}?issueStatus=${statusCountTuple.issueStatus}`} + href={`${contextHref}?issueStatus=${statusCountTuple.issueStatus}`} >src/features/integrations/hooks/use-integrations.ts (2)
1-9: Add"use client"directive to this hook module.This file uses React Query hooks (client-side only), so it must be marked as a client component in Next.js App Router.
✅ Suggested fix
+ "use client"; + import { useMutation, useQueryClient, useSuspenseQuery, } from "@tanstack/react-query";
31-92: Cache invalidation misses paginated queries — usequeryFilterfor partial matching.
useSuspenseIntegrationsincludes pagination params in the query key via...params(line 18), butinvalidateQueries()calls only pass{ resourceType }. In TanStack Query,queryOptions()creates an exact key match, so queries with pagination params won't invalidate. UsequeryFilter()instead to match all queries for that resource type regardless of pagination state.🛠️ Suggested fix
- queryClient.invalidateQueries( - trpc.integrations.getMany.queryOptions({ - resourceType: data.integration.resourceType, - }), - ); + queryClient.invalidateQueries( + trpc.integrations.getMany.queryFilter({ + resourceType: data.integration.resourceType, + }), + );- queryClient.invalidateQueries( - trpc.integrations.getMany.queryOptions({ - resourceType: data.resourceType, - }), - ); + queryClient.invalidateQueries( + trpc.integrations.getMany.queryFilter({ + resourceType: data.resourceType, + }), + );Apply this pattern to all three mutation hooks:
useCreateIntegration(lines 36–38),useUpdateIntegration(lines 62–64), anduseRemoveIntegration(lines 88–90).src/features/issues/hooks/use-issues.ts (2)
1-9: Add"use client"directive to this hook module.This file exports only React hooks that use client-only APIs: React Query hooks (useMutation, useSuspenseQuery, useQueryClient), useTRPC() from the client TRPC instance, and sonner toast. Without the "use client" directive, it will fail to execute in the App Router context when imported into client components.
✅ Suggested fix
+ "use client"; + import { useMutation, useQueryClient, useSuspenseQuery, } from "@tanstack/react-query";
20-29: Invalidate all issue status variants when status is updated.The
getManyInternalByStatusAndAssetIdquery requires bothassetIdandissueStatusparameters. When only providingassetIdtoqueryFilter, cached queries keyed with specific status values won't be invalidated, leaving stale data for other issue statuses. After an issue's status changes, all status-specific pages for that asset need to be refreshed.🛠️ Suggested fix
- queryClient.invalidateQueries( - trpc.issues.getManyInternalByStatusAndAssetId.queryFilter({ - assetId: data.assetId, - }), - ); + for (const status of Object.values(IssueStatus)) { + queryClient.invalidateQueries( + trpc.issues.getManyInternalByStatusAndAssetId.queryFilter({ + assetId: data.assetId, + issueStatus: status, + }), + ); + }src/features/integrations/server/routers.ts (1)
126-150: Add ownership validation to update and remove operations.The
updateandremoveoperations allow any authenticated user to modify or delete any integration, which contradicts the ownership validation enforced in therotateKeyoperation (line 95) and throughout other routers (e.g., workflows router). While the comments claim this is intentional, the lack of documentation and inconsistency with the established pattern indicates this is likely an authorization gap rather than a deliberate design choice.Add userId checks to both operations:
Example fix
update: protectedProcedure .input( z.object({ id: z.string(), data: integrationInputSchema, }), ) .mutation(async ({ ctx, input }) => { const { id, data } = input; return prisma.integration.update({ where: { id, userId: ctx.auth.user.id }, data, include: { syncStatus: true }, }); }), remove: protectedProcedure .input(z.object({ id: z.string() })) .mutation(async ({ ctx, input }) => { return prisma.integration.delete({ where: { id: input.id, userId: ctx.auth.user.id }, }); }),
🤖 Fix all issues with AI agents
In `@src/app/api/v1/__tests__/assets.test.ts`:
- Around line 656-669: The test builds a modified payload named
updateAssetsPayload (and updates items[*].upstreamApi) but mistakenly sends
assetIntegrationPayload to the POST; change the request call that creates
integrationRes to send updateAssetsPayload instead of assetIntegrationPayload so
the API receives the modified upstreamApi values (locate the
request(BASE_URL).post("/assets/integrationUpload") that sets
authHeader/jsonHeader and currently .send(assetIntegrationPayload) and replace
the argument with updateAssetsPayload).
- Around line 751-752: The test titled "create+update Assets uploadIntegration
endpoint int test" is duplicated (also present at line ~496); locate the
duplicate test declaration (the it("create+update Assets uploadIntegration
endpoint int test", ...) block using setupMockIntegration) and either remove the
unintended copy or change its description string to a distinct name that
reflects its behavior (e.g., append "duplicate" or describe the specific
scenario being tested) so test reports are unambiguous; ensure any related
helper usage like setupMockIntegration remains intact if you keep the test.
In `@src/features/assets/components/asset.tsx`:
- Around line 94-103: The <li> with click and key handlers should be made
accessible by adding role="button" and tabIndex={0} so screen readers and
keyboard users recognize it as interactive; ensure the existing onKeyDown
handler on the same <li> (which checks e.key for "Enter" or " ") continues to
call e.preventDefault() and router.push(`/issues/${issue.id}`) so both Enter and
Space activate router.push; update the <li> element where router.push and
issue.id are referenced to include these ARIA/keyboard affordances.
In `@src/features/assets/server/routers.ts`:
- Around line 423-444: The update path when a mapping exists (foundMapping)
updates prisma.externalAssetMapping and then prisma.asset.update with assetData
but never sets deviceGroupId, so deviceGroup changes from the incoming cpe are
lost; modify the foundMapping branch to include deviceGroupId in the
prisma.asset.update data (either by merging assetData with { deviceGroupId } or
explicitly setting data.deviceGroupId) using the same deviceGroupId resolution
logic used elsewhere (from cpe) and keep lastSynced update on
externalAssetMapping; reference symbols: foundMapping,
prisma.externalAssetMapping.update, prisma.asset.update, assetData,
deviceGroupId, cpe, lastSynced.
- Around line 447-455: Prisma OR is including nulls causing false matches;
before calling prisma.asset.findFirst, build an OR array that only includes
non-null/undefined checks for assetData.hostname, assetData.macAddress, and
assetData.serialNumber (e.g., push { hostname: value } only if
assetData.hostname != null), and then call prisma.asset.findFirst with that
filtered OR array; if the filtered array is empty, skip the query or set
foundAsset to null instead of calling prisma with an OR that would match null
fields. Ensure you modify the code around prisma.asset.findFirst and use the
assetData.hostname/macAddress/serialNumber checks to construct the safe
conditions.
In `@src/features/integrations/server/routers.ts`:
- Around line 99-119: The rotation flow is non-atomic: if auth.api.deleteApiKey
succeeds but createIntegrationApiKey fails, the integration is left without a
key; fix by creating the new key first via
createIntegrationApiKey(integration.name, ctx.auth.user.id), then update the
integration record with prisma.integration.update({ where: { id: input.id },
data: { apiKeyId: newApiKey.id } }), and only after a successful DB update call
auth.api.deleteApiKey({ body: { keyId: integration.apiKeyId }, headers: await
headers() }) to remove the old key; ensure you handle errors so the old key is
retained if any step fails.
🧹 Nitpick comments (14)
src/features/assets/hooks/use-asset-params.ts (1)
1-10: Add a"use client"directive to mark this hook module as client‑only.
This prevents accidental server imports and aligns with the coding guideline to mark client modules with"use client". SinceuseQueryStatesfrom nuqs relies onuseSearchParams(a client-only API), this directive clarifies the module's client requirement.♻️ Proposed change
+"use client"; + import { useQueryStates } from "nuqs"; import { assetDetailParams, assetsParams } from "../params";src/features/assets/server/routers.ts (3)
583-594:PrismaClientUnknownRequestErroris imported but not handled.The error handler checks for
PrismaClientKnownRequestErrorandPrismaClientValidationErrorbut doesn't handlePrismaClientUnknownRequestErrordespite importing it on line 23. Either handle it or remove the unused import.♻️ Proposed fix: Handle or remove unused import
const handlePrismaError = (e: unknown): string => { let message = "Internal Server Error"; if ( e instanceof PrismaClientKnownRequestError || - e instanceof PrismaClientValidationError + e instanceof PrismaClientValidationError || + e instanceof PrismaClientUnknownRequestError ) { message = e.message; } return message; };
516-523:errorMessagestores "success" even on successful sync.The
SyncStatusrecord always storesresponse.messageinerrorMessage, which will be "success" when there's no error. This is semantically misleading and could confuse debugging or auditing.♻️ Proposed fix: Only store errorMessage on error
await prisma.syncStatus.create({ data: { integrationId: foundIntegration.id, status: response.shouldRetry ? SyncStatusEnum.Error : SyncStatusEnum.Success, - errorMessage: response.message, + errorMessage: response.shouldRetry ? response.message : null, syncedAt: lastSynced, }, });
406-514: Consider wrapping item processing in a transaction for atomicity.The loop processes items sequentially with individual database operations. If an error occurs mid-way, some assets/mappings will be created while others won't, leaving the database in a partial state. While
shouldRetrysignals this to the caller, using a transaction would ensure all-or-nothing semantics.If partial success is intentional (processing as many items as possible), consider tracking which items failed and returning that information to the caller for targeted retry.
src/app/api/v1/__tests__/assets.test.ts (2)
663-663: Remove debugconsole.logstatement.This appears to be a leftover debug statement that should be removed before merging.
♻️ Proposed fix
- console.log(updateAssetsPayload.items[1].upstreamApi);
102-109: Consider addressing the TypeScript error instead of using@ts-ignore.The comment indicates TypeScript wants a
Userfield. If the Prisma model doesn't require it (due to default values or relations), the type definition may be out of sync. Consider investigating whether the type can be corrected or if a more specific ignore (like@ts-expect-errorwith a reason) would be clearer.prisma/migrations/20260127223247_vuln_mappings_sync_status/migration.sql (1)
54-61: Consider adding an index onintegrationIdforexternal_asset_mappings.While you have an index on
itemIdand unique constraints covering(itemId, integrationId)and(integrationId, externalId), queries that filter only byintegrationId(e.g., "find all mappings for an integration") would benefit from a dedicated index. Theexternal_item_mappingstable doesn't have this index either.♻️ Proposed addition
-- CreateIndex CREATE INDEX "external_asset_mappings_integrationId_idx" ON "external_asset_mappings"("integrationId"); -- CreateIndex CREATE INDEX "external_item_mappings_integrationId_idx" ON "external_item_mappings"("integrationId");prisma/schema.prisma (1)
251-267: Table nameexternal_item_mappingsdoesn't match model nameExternalVulnerabilityMapping.The model is named
ExternalVulnerabilityMappingbut maps toexternal_item_mappings. This could cause confusion when querying the database directly or in logs. Consider renaming the table toexternal_vulnerability_mappingsfor consistency withexternal_asset_mappings.Note: This would require updating the migration as well. If the naming is intentional (e.g., planning to use
external_item_mappingsfor multiple item types), consider adding a comment explaining the rationale.prisma/migrations/20260123032859_issue_status_pending_to_active/migration.sql (1)
18-19: RedundantALTER TABLEstatement.Line 15 already sets the default to
'ACTIVE'within the transaction. Line 19 outside the transaction sets the same default again, which is unnecessary.This doesn't cause any issues, but the redundancy could be removed for cleaner migration code.
src/features/issues/hooks/use-issues.ts (1)
57-88: Simplify page selection to use the hook’sissueStatus.The current loop depends on
params.issueStatus, which can diverge from theissueStatusargument (e.g., prefetching or switching tabs). Using the argument directly avoids mismatch and reduces complexity.♻️ Suggested refactor
- let page = 1; - for (const status of Object.values(IssueStatus)) { - if (params.issueStatus === status) { - const key = `${status.toLowerCase()}Page`; - if (key in params) { - const val = params[key as keyof typeof params]; - if (typeof val === "number") { - page = val; - break; - } - } - } - } + const key = `${issueStatus.toLowerCase()}Page`; + const page = + typeof params[key as keyof typeof params] === "number" + ? (params[key as keyof typeof params] as number) + : 1;src/features/integrations/components/integrations.tsx (2)
519-532: Variable shadowing:datacallback parameter shadows outerdataprop.The
dataparameter inonSuccessshadows the component'sdataprop. While this works here because the outerdataisn't used inside the callback, it reduces readability and could cause bugs if the callback logic changes.Suggested fix
const handleRotate = () => { rotateIntegration.mutate( { id: data.id }, { - onSuccess: (data) => { - setKey(data.apiKey); + onSuccess: (result) => { + setKey(result.apiKey); setSuccessOpen(true); }, onError: () => { setRotateOpen(true); }, }, ); };
552-554: Consider adding loading state to the "Rotate API Key" button.Unlike the "Update" and "Delete" buttons which show loading states (
isPending), the "Rotate API Key" button doesn't indicate when rotation is in progress. This could lead to users clicking multiple times.Suggested fix
- <Button size="sm" onClick={() => setRotateOpen(true)}> - {"Rotate API Key"} + <Button + size="sm" + onClick={() => setRotateOpen(true)} + disabled={rotateIntegration.isPending} + > + {rotateIntegration.isPending ? "Rotating..." : "Rotate API Key"} </Button>src/features/assets/components/asset.tsx (2)
234-270: Fragile: Results array order must matchObject.values(IssueStatus)order.The code pushes results in order
[aResult, fpResult, rResult](lines 250-251) and then accesses them via indexresults[i]while iteratingObject.values(IssueStatus)(lines 262-268). This silently breaks if the enum order changes or a new status is added.Consider using a Map keyed by status for explicit, order-independent access.
Suggested approach
- const results: PaginatedResponse<{ vulnerability: Vulnerability } & Issue>[] = - []; - let showTabs = false; - for (const res of [aResult, fpResult, rResult]) { - results.push(res.data); - if (res.data.totalCount > 0) { - showTabs = true; - } - } + const resultsMap = new Map<IssueStatus, PaginatedResponse<{ vulnerability: Vulnerability } & Issue>>([ + [IssueStatus.ACTIVE, aResult.data], + [IssueStatus.FALSE_POSITIVE, fpResult.data], + [IssueStatus.REMEDIATED, rResult.data], + ]); + + const showTabs = Array.from(resultsMap.values()).some(r => r.totalCount > 0); // Then in the render: // {Object.values(IssueStatus).map((status) => { // const result = resultsMap.get(status)!; // return ( // <TabsTrigger ...> // {issueStatusNames[status]} ({result.totalCount}) // </TabsTrigger> // ); // })}
162-172: Consider pagination with ellipsis for large page counts.Rendering all page numbers with
[...Array(totalPages)]can create a cluttered UI and performance issues when there are many pages (e.g., 50+ issues). Consider implementing an ellipsis pattern that shows first, last, and pages around the current page.
| if (foundMapping) { | ||
| try { | ||
| await prisma.externalAssetMapping.update({ | ||
| where: { id: foundMapping.id }, | ||
| data: { lastSynced } | ||
| }) | ||
|
|
||
| await prisma.asset.update({ | ||
| where: { id: foundMapping.itemId }, | ||
| data: { | ||
| ...assetData, | ||
| }, | ||
| }); | ||
| } catch (error: unknown) { | ||
| response.message = handlePrismaError(error); | ||
| response.shouldRetry = true; | ||
| break; | ||
| } | ||
|
|
||
| response.updatedAssetsCount++; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Asset update when mapping exists doesn't update deviceGroup.
When an existing mapping is found, the asset is updated with assetData but the deviceGroup association is not updated even if the incoming cpe has changed. This is inconsistent with the create flow (lines 462-468) and the "asset exists without mapping" flow (lines 500-507), both of which set deviceGroupId.
🐛 Proposed fix: Update deviceGroup when mapping exists
// If we have a ExternalAssetMapping, update the sync time and asset
if (foundMapping) {
try {
+ const deviceGroup = await cpeToDeviceGroup(cpe);
+
await prisma.externalAssetMapping.update({
where: { id: foundMapping.id },
data: { lastSynced }
})
await prisma.asset.update({
where: { id: foundMapping.itemId },
data: {
...assetData,
+ deviceGroupId: deviceGroup.id,
},
});
} catch (error: unknown) {📝 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.
| if (foundMapping) { | |
| try { | |
| await prisma.externalAssetMapping.update({ | |
| where: { id: foundMapping.id }, | |
| data: { lastSynced } | |
| }) | |
| await prisma.asset.update({ | |
| where: { id: foundMapping.itemId }, | |
| data: { | |
| ...assetData, | |
| }, | |
| }); | |
| } catch (error: unknown) { | |
| response.message = handlePrismaError(error); | |
| response.shouldRetry = true; | |
| break; | |
| } | |
| response.updatedAssetsCount++; | |
| continue; | |
| } | |
| if (foundMapping) { | |
| try { | |
| const deviceGroup = await cpeToDeviceGroup(cpe); | |
| await prisma.externalAssetMapping.update({ | |
| where: { id: foundMapping.id }, | |
| data: { lastSynced } | |
| }) | |
| await prisma.asset.update({ | |
| where: { id: foundMapping.itemId }, | |
| data: { | |
| ...assetData, | |
| deviceGroupId: deviceGroup.id, | |
| }, | |
| }); | |
| } catch (error: unknown) { | |
| response.message = handlePrismaError(error); | |
| response.shouldRetry = true; | |
| break; | |
| } | |
| response.updatedAssetsCount++; | |
| continue; | |
| } |
🤖 Prompt for AI Agents
In `@src/features/assets/server/routers.ts` around lines 423 - 444, The update
path when a mapping exists (foundMapping) updates prisma.externalAssetMapping
and then prisma.asset.update with assetData but never sets deviceGroupId, so
deviceGroup changes from the incoming cpe are lost; modify the foundMapping
branch to include deviceGroupId in the prisma.asset.update data (either by
merging assetData with { deviceGroupId } or explicitly setting
data.deviceGroupId) using the same deviceGroupId resolution logic used elsewhere
(from cpe) and keep lastSynced update on externalAssetMapping; reference
symbols: foundMapping, prisma.externalAssetMapping.update, prisma.asset.update,
assetData, deviceGroupId, cpe, lastSynced.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/features/assets/server/routers.ts`:
- Around line 514-523: The syncStatus.create call is saving response.message
into errorMessage even on success; change the value passed to errorMessage in
prisma.syncStatus.create (the block around prisma.syncStatus.create and the
response/shouldRetry handling) so that errorMessage is null when
response.shouldRetry is false (i.e., on SyncStatusEnum.Success) and set to
response.message only when response.shouldRetry is true (SyncStatusEnum.Error);
ensure the status assignment (SyncStatusEnum.Error/SyncStatusEnum.Success)
remains the same and only the errorMessage field is conditionally null or the
error text.
🧹 Nitpick comments (2)
src/features/assets/server/routers.ts (2)
404-512: Consider wrapping the entire operation in a transaction for atomicity.The current implementation processes items sequentially without a transaction. If an error occurs mid-loop, some assets and mappings will be created while others won't, leaving the database in a partially synced state. While
shouldRetryhelps signal this, subsequent retries could create duplicate assets if the matching logic fails.Consider using
prisma.$transaction()to ensure all-or-nothing semantics, or implement idempotent upsert logic to handle retries safely.
583-594: Avoid exposing raw Prisma error messages to clients.Returning
e.messagedirectly exposes internal database details (table names, field names, constraint names) to API consumers. Consider mapping known error codes to user-friendly messages instead.♻️ Suggested approach
const handlePrismaError = (e: unknown): string => { - let message = "Internal Server Error"; - - if ( - e instanceof PrismaClientKnownRequestError || - e instanceof PrismaClientValidationError - ) { - message = e.message; + if (e instanceof PrismaClientKnownRequestError) { + // Map common error codes to user-friendly messages + switch (e.code) { + case "P2002": + return "A record with this identifier already exists"; + case "P2025": + return "Record not found"; + default: + return "Database operation failed"; + } } - - return message; + if (e instanceof PrismaClientValidationError) { + return "Invalid data provided"; + } + return "Internal Server Error"; };
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/features/assets/server/routers.ts`:
- Around line 1-2: This module is server-only and must import "server-only" to
prevent client bundling; add a top-level import "server-only" as the very first
statement in src/features/assets/server/routers.ts (above existing imports like
z and Asset/SyncStatusEnum) so the runtime/build will error if this router is
accidentally imported into a Client Component.
🧹 Nitpick comments (1)
src/features/assets/server/routers.ts (1)
423-433: Wrap mapping + asset operations in a transaction for atomicity.Both locations have sequential operations where
lastSyncedcan be recorded even if the asset update fails. Lines 423-433 update an existing mapping then asset; lines 496-514 create a new mapping then update an asset. Wrapping each pair inprisma.$transaction(async (tx) => { ... })ensures both operations succeed or both roll back, keeping sync state accurate.♻️ Suggested refactor pattern
- await prisma.externalAssetMapping.update({ - where: { id: foundMapping.id }, - data: { lastSynced }, - }); - - await prisma.asset.update({ - where: { id: foundMapping.itemId }, - data: { - ...assetData, - }, - }); + await prisma.$transaction(async (tx) => { + await tx.asset.update({ + where: { id: foundMapping.itemId }, + data: { + ...assetData, + }, + }); + await tx.externalAssetMapping.update({ + where: { id: foundMapping.id }, + data: { lastSynced }, + }); + });
| import { z } from "zod"; | ||
| import { type Asset, SyncStatusEnum } from "@/generated/prisma"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
head -20 src/features/assets/server/routers.tsRepository: PATCH-UPGRADE/viper
Length of output: 632
🌐 Web query:
Next.js 15 server-only directive import placement App Router
💡 Result:
In Next.js App Router (including Next.js 15), server-only is not a "use ..." directive. It’s a side-effect import, so the only real requirement is:
- Put
import 'server-only'at the top level of the module you want to mark as server-only (i.e., not inside a function / conditional). - It does not need to be the very first line the way
"use client"/"use server"do—but it’s commonly placed first for clarity. Next.js uses it to produce a build-time error if that module is imported from a Client Component. [1]
Example:
// app/lib/db.ts
import 'server-only'
import { sql } from '@vercel/postgres'
export async function getUser(id: string) {
return sql`select * from users where id = ${id}`
}[1] https://nextjs.org/docs/app/getting-started/server-and-client-components
Add server-only directive for this router module.
This is a server-only file but lacks the directive that prevents accidental client bundling. In Next.js 15 App Router, import "server-only" at the top level produces a build-time error if the module is imported from a Client Component.
📌 Suggested change
+import "server-only";
import { z } from "zod";📝 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.
| import { z } from "zod"; | |
| import { type Asset, SyncStatusEnum } from "@/generated/prisma"; | |
| import "server-only"; | |
| import { z } from "zod"; | |
| import { type Asset, SyncStatusEnum } from "@/generated/prisma"; |
🤖 Prompt for AI Agents
In `@src/features/assets/server/routers.ts` around lines 1 - 2, This module is
server-only and must import "server-only" to prevent client bundling; add a
top-level import "server-only" as the very first statement in
src/features/assets/server/routers.ts (above existing imports like z and
Asset/SyncStatusEnum) so the runtime/build will error if this router is
accidentally imported into a Client Component.
NOTE:
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.