refactor#441
Open
Producdevity wants to merge 2 commits into
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
WalkthroughThis PR refactors the monolithic PC listings router into specialized sub-routers (core, admin, comments) and introduces a new dedicated reports router. Core CRUD, admin approval workflows, and comment management are extracted; a comprehensive reports system manages listing reports with filtering, sorting, status updates, and trust logging. Supporting utilities normalize custom fields and cache listing stats. Import paths are relocated and error handling is refined. ChangesPC Listings Routing Architecture
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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: 13
🧹 Nitpick comments (2)
src/server/api/routers/pcListings.ts (1)
6-8: ⚡ Quick winAvoid composing the router through
_def.procedures.
_defis tRPC internals, so flattening the core routes this way makes the composition layer depend on non-public structure. Please verify this against the@trpc/server11.17.0 public API and prefer a public composition helper or an exported plain procedure record if available.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/api/routers/pcListings.ts` around lines 6 - 8, The router is pulling procedures via the internal property coreRouter._def.procedures which relies on tRPC internals; instead, compose using the public API: either use the router merge/composition helper (e.g., createTRPCRouter().merge(coreRouter) or the tRPC-provided mergeRouter/merge method) or export a plain procedures record from the core module and spread that (e.g., export coreProcedures and use ...coreProcedures). Update pcListingsRouter to stop referencing _def and use createTRPCRouter + merge(coreRouter) or spread an exported public procedures object from coreRouter so composition uses only public symbols.src/app/pc-listings/[id]/components/PcReportListingModal.tsx (1)
46-51: ⚡ Quick winResolve the
react-hooks/set-state-in-effectwarning on modal reset.The current effect performs synchronous
setStatecalls and is already flagged in CI warnings. Please refactor reset timing/pattern to satisfy the hook rule.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/pc-listings/`[id]/components/PcReportListingModal.tsx around lines 46 - 51, The effect in PcReportListingModal currently performs multiple setState calls (setReason, setDescription, setError) and triggers the react-hooks/set-state-in-effect warning; replace the separate state hooks with a single useReducer (e.g., reportStateReducer with shape {reason, description, error}) and dispatch a single RESET action inside the effect when props.isOpen becomes true (or use a prevIsOpen ref to ensure you only dispatch on open transition) so the modal reset is done via one reducer action (dispatch({type: 'RESET'})) instead of multiple setState calls in useEffect.Source: Pipeline failures
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/schemas/pcListing.ts`:
- Around line 298-299: The page and limit schema entries currently allow floats;
update the PC listing schema's page and limit validators to require integers by
adding the .int() constraint (or use z.coerce.number().int() if you need to
accept numeric strings) so values like 1.5 are rejected at the schema boundary;
adjust the validators for the fields named page and limit in the schema (retain
the existing .min/.max/.default) so Prisma skip/take always receive integers.
In `@src/server/api/routers/pcListingReports.ts`:
- Around line 218-225: Avoid the check-then-delete race by removing the separate
pcListingReport.findUnique call and performing the delete atomically with
ctx.prisma.pcListingReport.delete, wrapping that call in a try/catch; if the
delete throws the Prisma "not found" error (e.g., PrismaClientKnownRequestError
with the P2025 code), return ResourceError.listingReport.notFound(), otherwise
rethrow or log unexpected errors. Locate the current usages of
pcListingReport.findUnique and pcListingReport.delete in this handler and
replace the two-step flow with the single delete + error handling logic.
- Around line 152-212: The multiple DB writes (prisma.pcListing.update,
TrustService.logAction, prisma.pcListingReport.update) must be executed inside a
single transaction to avoid partial state; wrap these operations in
ctx.prisma.$transaction and use the transactional client for all writes (create
a transaction-scoped TrustService with new TrustService(tx) and call
tx.pcListing.update, txTrustService.logAction, then tx.pcListingReport.update
inside the transaction callback) and return the pcListingReport result from the
transaction so all updates commit or roll back atomically.
- Around line 17-228: The router contains heavy Prisma/workflow logic (notably
in updateStatus, get, byId, and delete) and should delegate to
service/repository methods; extract business logic and DB calls into a new
PcListingReportService (or repository) with methods like
getReports(input,pagination), getById(id), createPcListingReport(payload) (you
already have ReportSubmissionService), updateStatus(reportId, status,
reviewNotes, reviewerId) which performs status transitions, pcListing updates,
and TrustService logging, and deleteReport(id); then simplify the router
handlers (stats/get/byId/create/updateStatus/delete) to validate
input/permissions, call the corresponding PcListingReportService methods, and
return results so the router contains only orchestration and not raw Prisma
queries or workflow rules.
- Around line 169-192: The handler currently always calls trustService.logAction
when status === ReportStatus.RESOLVED or DISMISSED which causes duplicate trust
effects on repeated updates; modify the code that performs logging (the block
that calls trustService.logAction) to first compare the incoming status with the
report's existing status (e.g., report.status or the fetched
existingReport.status) and only call trustService.logAction when the status is
actually changing to RESOLVED or to DISMISSED (i.e., previousStatus !==
newStatus && newStatus === ReportStatus.RESOLVED/DISMISSED), preserving the same
metadata and reviewerId/reason fields so repeated identical updates become
no-ops for trust logging.
In `@src/server/api/routers/pcListings/admin.ts`:
- Around line 674-694: The update currently calls ctx.prisma.pcListing.update
and returns updatedPcListing before deleting/creating custom fields, and those
operations run outside a transaction (functions: pcListing.update,
pcListingCustomFieldValue.deleteMany/createMany, toPrismaCustomFieldValue),
which can yield stale data or partial updates; fix by executing the update,
deleteMany, and createMany inside a single ctx.prisma.$transaction (or
interactive transaction) in the correct order, then fetch and return the final
pcListing with pcListingDetailInclude after the transaction completes so the
response reflects the atomic result of all changes.
- Around line 443-466: bulkApprove currently only checks the caller role and
updates all pending IDs, which allows developers to act on listings for
emulators they aren't verified for; change bulkApprove to enforce the same
per-emulator verification used by the single-item approve/reject handlers: after
loading pendingListings (ctx.prisma.pcListing.findMany) filter them to only
listings whose emulator the caller is verified for (or alternatively, if any
requested id is outside the caller’s verified scope, return an error), then call
updateMany only on the filtered IDs and/or return a failure when any id is
unauthorized; reuse the existing per-emulator check logic
(hasRolePermission/Developer verification helper used by the single-item
approve/reject) and ensure processedByUserId/processedAt are only set for
authorized listings.
In `@src/server/api/routers/pcListings/comments.ts`:
- Around line 140-146: When validating parentId in the comment creation flow,
also ensure the parent belongs to the same listing and is not soft-deleted:
fetch the parent via ctx.prisma.pcListingComment.findUnique as you already do,
then check that parentComment.pcListingId === pcListingId and that the parent is
not marked deleted (use your schema's soft-delete field, e.g.
parentComment.deletedAt === null or parentComment.deleted === false). If either
check fails, return the existing ResourceError.comment.parentNotFound() (or a
more specific error) instead of accepting the parentId.
- Around line 292-299: The handler currently fetches the comment via
ctx.prisma.pcListingComment.findUnique and proceeds even if it's soft-deleted;
before entering the transaction that updates votes (the function handling vote
updates), check comment.deletedAt and if it's set return
ResourceError.comment.notFound() (or the same error path used for missing
comments) so voting is rejected for soft-deleted comments; update the logic
around the existing comment lookup and before any call to the transaction/vote
update code to enforce this check.
In `@src/server/api/routers/pcListings/core.ts`:
- Around line 345-373: The update and custom-field replacement must be run
inside a single transaction so the base pcListing update and the
pcListingCustomFieldValue deleteMany/createMany are atomic; move the
ctx.prisma.pcListing.update, ctx.prisma.pcListingCustomFieldValue.deleteMany and
createMany calls into one ctx.prisma.$transaction (or use Prisma's interactive
transaction), perform deleteThenCreate of customFieldValues within that
transaction (using toPrismaCustomFieldValue for mapping), and after the
transaction completes re-read the pcListing (the updatedPcListing) with the same
includes (game.system, cpu.brand, gpu.brand, emulator, performance, author,
customFieldValues+customFieldDefinition.category) to return fresh
customFieldValues rather than the stale pre-transaction object.
In `@src/server/api/routers/pcListings/utils.ts`:
- Around line 11-27: Change isJsonRecord and conversion logic to only accept
plain objects: update isJsonRecord(value) to return true only when typeof value
=== 'object' && value !== null && !Array.isArray(value) &&
(Object.getPrototypeOf(value) === Object.prototype ||
Object.getPrototypeOf(value) === null). Then in toPrismaNestedJsonValue, when
encountering an object that fails isJsonRecord (e.g. Date, Map, Set, class
instance) do not recurse—reject it explicitly (throw a clear error or return
null per project convention) instead of treating it as an empty record;
reference functions isJsonRecord and toPrismaNestedJsonValue when making the
change.
In `@src/server/api/trpc.ts`:
- Around line 247-250: The guard calls call AppError.unauthorized() but do not
return, so execution continues and may access ctx.session.user.role; change
those guards to return the unauthorized result (e.g., return
AppError.unauthorized()) instead of just calling it, and apply the same fix to
the other role-check blocks that call hasRolePermission(...) and
AppError.unauthorized() (the other occurrences around the second and third role
checks). Ensure you update the checks that reference ctx.session,
AppError.unauthorized(), hasRolePermission, and Role.MODERATOR so they return
immediately on unauthorized.
In `@src/server/utils/security-validation.ts`:
- Line 2: Replace the non-actionable profanity TODOs in
src/server/utils/security-validation.ts (at the file header and at the comments
near lines referencing additional TODOs) with concise, actionable TODOs that
state the specific remediation: e.g., "Implement centralized input validation
using validateRequest() (or a new middleware) to enforce schema-based checks,
normalize/escape inputs, and add unit/integration tests; see tracking issue
`#1234`." For each replaced comment, mention the responsible symbol or area (e.g.,
the module-level initialization, the validateRequest/validateSecurity functions,
or any middleware functions nearby) and include a link or issue ID so future
contributors know where to track work; keep each TODO one sentence and
actionable.
---
Nitpick comments:
In `@src/app/pc-listings/`[id]/components/PcReportListingModal.tsx:
- Around line 46-51: The effect in PcReportListingModal currently performs
multiple setState calls (setReason, setDescription, setError) and triggers the
react-hooks/set-state-in-effect warning; replace the separate state hooks with a
single useReducer (e.g., reportStateReducer with shape {reason, description,
error}) and dispatch a single RESET action inside the effect when props.isOpen
becomes true (or use a prevIsOpen ref to ensure you only dispatch on open
transition) so the modal reset is done via one reducer action (dispatch({type:
'RESET'})) instead of multiple setState calls in useEffect.
In `@src/server/api/routers/pcListings.ts`:
- Around line 6-8: The router is pulling procedures via the internal property
coreRouter._def.procedures which relies on tRPC internals; instead, compose
using the public API: either use the router merge/composition helper (e.g.,
createTRPCRouter().merge(coreRouter) or the tRPC-provided mergeRouter/merge
method) or export a plain procedures record from the core module and spread that
(e.g., export coreProcedures and use ...coreProcedures). Update pcListingsRouter
to stop referencing _def and use createTRPCRouter + merge(coreRouter) or spread
an exported public procedures object from coreRouter so composition uses only
public symbols.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5a734384-74fb-497d-9dcf-19c0833c2c3a
📒 Files selected for processing (17)
src/app/pc-listings/[id]/components/PcReportListingModal.tsxsrc/schemas/pcListing.tssrc/server/api/root.tssrc/server/api/routers/listings/core.tssrc/server/api/routers/listings/index.tssrc/server/api/routers/pcListingReports.tssrc/server/api/routers/pcListings.test.tssrc/server/api/routers/pcListings.tssrc/server/api/routers/pcListings/admin.tssrc/server/api/routers/pcListings/comments.tssrc/server/api/routers/pcListings/core.tssrc/server/api/routers/pcListings/index.tssrc/server/api/routers/pcListings/utils.tssrc/server/api/trpc.tssrc/server/repositories/listings.repository.tssrc/server/utils/security-validation.tssrc/server/utils/validate-custom-fields.ts
💤 Files with no reviewable changes (2)
- src/server/api/routers/pcListings.test.ts
- src/server/api/routers/listings/index.ts
Comment on lines
+298
to
+299
| page: z.number().min(1).default(1), | ||
| limit: z.number().min(1).max(100).default(20), |
There was a problem hiding this comment.
Enforce integer pagination at schema boundary.
page and limit currently accept floats. That can propagate to Prisma skip/take and fail request execution for inputs like 1.5.
Suggested fix
- page: z.number().min(1).default(1),
- limit: z.number().min(1).max(100).default(20),
+ page: z.coerce.number().int().min(1).default(1),
+ limit: z.coerce.number().int().min(1).max(100).default(20),📝 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.
Suggested change
| page: z.number().min(1).default(1), | |
| limit: z.number().min(1).max(100).default(20), | |
| page: z.coerce.number().int().min(1).default(1), | |
| limit: z.coerce.number().int().min(1).max(100).default(20), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/schemas/pcListing.ts` around lines 298 - 299, The page and limit schema
entries currently allow floats; update the PC listing schema's page and limit
validators to require integers by adding the .int() constraint (or use
z.coerce.number().int() if you need to accept numeric strings) so values like
1.5 are rejected at the schema boundary; adjust the validators for the fields
named page and limit in the schema (retain the existing .min/.max/.default) so
Prisma skip/take always receive integers.
Comment on lines
+17
to
+228
| export const pcListingReportsRouter = createTRPCRouter({ | ||
| stats: permissionProcedure(PERMISSIONS.VIEW_STATISTICS).query(async ({ ctx }) => { | ||
| const [pending, underReview, resolved, dismissed] = await Promise.all([ | ||
| ctx.prisma.pcListingReport.count({ where: { status: ReportStatus.PENDING } }), | ||
| ctx.prisma.pcListingReport.count({ where: { status: ReportStatus.UNDER_REVIEW } }), | ||
| ctx.prisma.pcListingReport.count({ where: { status: ReportStatus.RESOLVED } }), | ||
| ctx.prisma.pcListingReport.count({ where: { status: ReportStatus.DISMISSED } }), | ||
| ]) | ||
|
|
||
| return { | ||
| total: pending + underReview + resolved + dismissed, | ||
| pending, | ||
| underReview, | ||
| resolved, | ||
| dismissed, | ||
| } | ||
| }), | ||
|
|
||
| get: permissionProcedure(PERMISSIONS.VIEW_USER_BANS) | ||
| .input(GetPcListingReportsSchema) | ||
| .query(async ({ ctx, input }) => { | ||
| const { | ||
| search, | ||
| status, | ||
| reason, | ||
| sortField = 'createdAt', | ||
| sortDirection = 'desc', | ||
| } = input ?? {} | ||
|
|
||
| const { page, limit } = validatePagination(input?.page, input?.limit, 50) | ||
| const sanitizedSearch = search ? sanitizeInput(search) : undefined | ||
| const offset = (page - 1) * limit | ||
|
|
||
| const where: Prisma.PcListingReportWhereInput = {} | ||
|
|
||
| if (sanitizedSearch) { | ||
| where.OR = [ | ||
| { pcListing: { game: { title: { contains: sanitizedSearch, mode: 'insensitive' } } } }, | ||
| { reportedBy: { name: { contains: sanitizedSearch, mode: 'insensitive' } } }, | ||
| { description: { contains: sanitizedSearch, mode: 'insensitive' } }, | ||
| ] | ||
| } | ||
|
|
||
| if (status) where.status = status | ||
| if (reason) where.reason = reason | ||
|
|
||
| const orderBy: Prisma.PcListingReportOrderByWithRelationInput = {} | ||
| if (sortField && sortDirection) orderBy[sortField] = sortDirection | ||
|
|
||
| const [reports, total] = await Promise.all([ | ||
| ctx.prisma.pcListingReport.findMany({ | ||
| where, | ||
| orderBy, | ||
| skip: offset, | ||
| take: limit, | ||
| include: { | ||
| pcListing: { | ||
| include: { | ||
| game: { select: { id: true, title: true } }, | ||
| author: { select: { id: true, name: true } }, | ||
| cpu: true, | ||
| gpu: true, | ||
| emulator: { select: { id: true, name: true } }, | ||
| }, | ||
| }, | ||
| reportedBy: { select: { id: true, name: true, email: true } }, | ||
| reviewedBy: { select: { id: true, name: true } }, | ||
| }, | ||
| }), | ||
| ctx.prisma.pcListingReport.count({ where }), | ||
| ]) | ||
|
|
||
| return { | ||
| reports, | ||
| pagination: paginate({ total: total, page, limit: limit }), | ||
| } | ||
| }), | ||
|
|
||
| byId: permissionProcedure(PERMISSIONS.VIEW_USER_BANS) | ||
| .input(GetReportByIdSchema) | ||
| .query(async ({ ctx, input }) => { | ||
| const report = await ctx.prisma.pcListingReport.findUnique({ | ||
| where: { id: input.id }, | ||
| include: { | ||
| pcListing: { | ||
| include: { | ||
| game: true, | ||
| author: { select: { id: true, name: true, email: true } }, | ||
| cpu: true, | ||
| gpu: true, | ||
| emulator: true, | ||
| performance: true, | ||
| }, | ||
| }, | ||
| reportedBy: { select: { id: true, name: true, email: true } }, | ||
| reviewedBy: { select: { id: true, name: true } }, | ||
| }, | ||
| }) | ||
|
|
||
| return report || ResourceError.listingReport.notFound() | ||
| }), | ||
|
|
||
| create: protectedProcedure.input(CreatePcListingReportSchema).mutation(async ({ ctx, input }) => { | ||
| const { pcListingId, reason, description } = input | ||
| const userId = ctx.session.user.id | ||
|
|
||
| validateEnum(reason, Object.values(ReportReason), 'reason') | ||
|
|
||
| const reportSubmissionService = new ReportSubmissionService(ctx.prisma) | ||
|
|
||
| return await reportSubmissionService.createPcListingReport({ | ||
| pcListingId, | ||
| reportedById: userId, | ||
| reason, | ||
| description, | ||
| }) | ||
| }), | ||
|
|
||
| updateStatus: permissionProcedure(PERMISSIONS.MANAGE_USER_BANS) | ||
| .input(UpdatePcListingReportSchema) | ||
| .mutation(async ({ ctx, input }) => { | ||
| const { reportId, status, reviewNotes } = input | ||
| const reviewerId = ctx.session.user.id | ||
|
|
||
| validateEnum(status, Object.values(ReportStatus), 'status') | ||
|
|
||
| const report = await ctx.prisma.pcListingReport.findUnique({ | ||
| where: { id: reportId }, | ||
| include: { pcListing: true }, | ||
| }) | ||
|
|
||
| if (!report) { | ||
| return ResourceError.listingReport.notFound() | ||
| } | ||
|
|
||
| if ( | ||
| status === ReportStatus.RESOLVED && | ||
| report.pcListing?.status === ApprovalStatus.APPROVED | ||
| ) { | ||
| await ctx.prisma.pcListing.update({ | ||
| where: { id: report.pcListingId }, | ||
| data: { | ||
| status: ApprovalStatus.REJECTED, | ||
| processedAt: new Date(), | ||
| processedByUserId: reviewerId, | ||
| processedNotes: `Rejected due to report: ${reviewNotes || 'No additional notes'}`, | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| const trustService = new TrustService(ctx.prisma) | ||
|
|
||
| if (status === ReportStatus.RESOLVED) { | ||
| await trustService.logAction({ | ||
| userId: report.reportedById, | ||
| action: TrustAction.REPORT_CONFIRMED, | ||
| metadata: { | ||
| reportId, | ||
| pcListingId: report.pcListingId, | ||
| reviewedBy: reviewerId, | ||
| reason: report.reason, | ||
| }, | ||
| }) | ||
| } else if (status === ReportStatus.DISMISSED) { | ||
| await trustService.logAction({ | ||
| userId: report.reportedById, | ||
| action: TrustAction.FALSE_REPORT, | ||
| metadata: { | ||
| reportId, | ||
| pcListingId: report.pcListingId, | ||
| reviewedBy: reviewerId, | ||
| reason: report.reason, | ||
| reviewNotes, | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| return ctx.prisma.pcListingReport.update({ | ||
| where: { id: reportId }, | ||
| data: { | ||
| status, | ||
| reviewNotes, | ||
| reviewedById: reviewerId, | ||
| reviewedAt: new Date(), | ||
| }, | ||
| include: { | ||
| pcListing: { | ||
| include: { | ||
| game: { select: { title: true } }, | ||
| author: { select: { name: true } }, | ||
| }, | ||
| }, | ||
| reportedBy: { select: { name: true } }, | ||
| reviewedBy: { select: { name: true } }, | ||
| }, | ||
| }) | ||
| }), | ||
|
|
||
| delete: permissionProcedure(PERMISSIONS.MANAGE_USER_BANS) | ||
| .input(DeleteReportSchema) | ||
| .mutation(async ({ ctx, input }) => { | ||
| const report = await ctx.prisma.pcListingReport.findUnique({ | ||
| where: { id: input.id }, | ||
| }) | ||
|
|
||
| if (!report) return ResourceError.listingReport.notFound() | ||
|
|
||
| return ctx.prisma.pcListingReport.delete({ | ||
| where: { id: input.id }, | ||
| }) | ||
| }), | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Move report workflow/business logic out of the router.
This router currently owns heavy Prisma access and workflow rules (status transitions + trust side effects). It should be a thin orchestration layer delegating to repository/service methods.
As per coding guidelines, routers in src/server/api/routers/**/*.ts should avoid raw Prisma/business logic and remain thin orchestration layers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/api/routers/pcListingReports.ts` around lines 17 - 228, The router
contains heavy Prisma/workflow logic (notably in updateStatus, get, byId, and
delete) and should delegate to service/repository methods; extract business
logic and DB calls into a new PcListingReportService (or repository) with
methods like getReports(input,pagination), getById(id),
createPcListingReport(payload) (you already have ReportSubmissionService),
updateStatus(reportId, status, reviewNotes, reviewerId) which performs status
transitions, pcListing updates, and TrustService logging, and deleteReport(id);
then simplify the router handlers (stats/get/byId/create/updateStatus/delete) to
validate input/permissions, call the corresponding PcListingReportService
methods, and return results so the router contains only orchestration and not
raw Prisma queries or workflow rules.
Sources: Coding guidelines, Learnings
Owner
Author
There was a problem hiding this comment.
is it at least consistent with listingsReport (in terms of structure and feature parity)
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find listing report router files
fd -t f 'listingReport' --exec echo {}
fd -t f 'listing-report' --exec echo {}
# Show the listing report router if it exists
rg -l 'listingReport' src/server/api/routers/ 2>/dev/nullLength of output: 840
🏁 Script executed:
#!/bin/bash
cat src/server/api/routers/listingReports.tsLength of output: 11900
@Producdevity Yes, pcListingReports.ts is highly consistent with listingReports.ts — the core six procedures (stats, get, byId, create, updateStatus, delete) are nearly line-for-line mirrors, with the same permission gates, pagination/sanitize pattern, sort logic, and trust action flows.
Three gaps vs listingReports.ts:
| Procedure | listingReports |
pcListingReports |
|---|---|---|
getUserReportStats |
✅ | ❌ |
getUserReports |
✅ (already cross-queries pcListingReport) |
❌ |
checkUserHasReports |
✅ | ❌ |
Note that getUserReports in listingReports.ts already aggregates both handheld and PC reports into a unified view, so pcListingReports may intentionally omit it — but getUserReportStats and checkUserHasReports currently only cover handheld listings.
One thing pcListingReports.ts actually fixes: the delete guard in listingReports.ts is missing a return:
// listingReports.ts — bug: result is discarded, delete proceeds anyway
if (!report) ResourceError.listingReport.notFound()
// pcListingReports.ts — correct
if (!report) return ResourceError.listingReport.notFound()Worth backporting that fix. The original refactor suggestion (extracting to a service layer) still applies equally to both routers since listingReports.ts has the same raw-Prisma pattern.
🧠 Learnings used
Learnt from: CR
Repo: Producdevity/EmuReady PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-06-05T11:58:33.136Z
Learning: Applies to src/server/api/routers/**/*.ts : Use specialized procedures such as `protectedProcedure`, `adminProcedure`, and `permissionProcedure(...)` instead of ad hoc permission checks in routers
Learnt from: CR
Repo: Producdevity/EmuReady PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-06-05T11:58:33.136Z
Learning: Applies to src/server/api/routers/**/*.ts : Do not put raw Prisma queries or business logic in routers
Learnt from: CR
Repo: Producdevity/EmuReady PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-06-05T11:58:33.137Z
Learning: Routers in `src/server/api/routers/` are thin orchestration layers; they handle auth context, schema-validated input, repository/service calls, and response formatting
Learnt from: CR
Repo: Producdevity/EmuReady PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-06-05T11:58:33.137Z
Learning: Applies to src/server/{repositories,services}/**/*.ts : Use transactions for multi-step writes that must stay consistent in Prisma operations
Comment on lines
+152
to
+212
| if ( | ||
| status === ReportStatus.RESOLVED && | ||
| report.pcListing?.status === ApprovalStatus.APPROVED | ||
| ) { | ||
| await ctx.prisma.pcListing.update({ | ||
| where: { id: report.pcListingId }, | ||
| data: { | ||
| status: ApprovalStatus.REJECTED, | ||
| processedAt: new Date(), | ||
| processedByUserId: reviewerId, | ||
| processedNotes: `Rejected due to report: ${reviewNotes || 'No additional notes'}`, | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| const trustService = new TrustService(ctx.prisma) | ||
|
|
||
| if (status === ReportStatus.RESOLVED) { | ||
| await trustService.logAction({ | ||
| userId: report.reportedById, | ||
| action: TrustAction.REPORT_CONFIRMED, | ||
| metadata: { | ||
| reportId, | ||
| pcListingId: report.pcListingId, | ||
| reviewedBy: reviewerId, | ||
| reason: report.reason, | ||
| }, | ||
| }) | ||
| } else if (status === ReportStatus.DISMISSED) { | ||
| await trustService.logAction({ | ||
| userId: report.reportedById, | ||
| action: TrustAction.FALSE_REPORT, | ||
| metadata: { | ||
| reportId, | ||
| pcListingId: report.pcListingId, | ||
| reviewedBy: reviewerId, | ||
| reason: report.reason, | ||
| reviewNotes, | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| return ctx.prisma.pcListingReport.update({ | ||
| where: { id: reportId }, | ||
| data: { | ||
| status, | ||
| reviewNotes, | ||
| reviewedById: reviewerId, | ||
| reviewedAt: new Date(), | ||
| }, | ||
| include: { | ||
| pcListing: { | ||
| include: { | ||
| game: { select: { title: true } }, | ||
| author: { select: { name: true } }, | ||
| }, | ||
| }, | ||
| reportedBy: { select: { name: true } }, | ||
| reviewedBy: { select: { name: true } }, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
Make updateStatus atomic across all writes.
pcListing update, trust logging, and pcListingReport update are separate writes. A mid-flow failure can persist partial state (e.g., listing rejected but report still not updated).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/api/routers/pcListingReports.ts` around lines 152 - 212, The
multiple DB writes (prisma.pcListing.update, TrustService.logAction,
prisma.pcListingReport.update) must be executed inside a single transaction to
avoid partial state; wrap these operations in ctx.prisma.$transaction and use
the transactional client for all writes (create a transaction-scoped
TrustService with new TrustService(tx) and call tx.pcListing.update,
txTrustService.logAction, then tx.pcListingReport.update inside the transaction
callback) and return the pcListingReport result from the transaction so all
updates commit or roll back atomically.
Owner
Author
There was a problem hiding this comment.
goal is to align pcListings with listings and then abstract and improve
There was a problem hiding this comment.
@Producdevity, understood — aligning the two routers first makes sense before introducing a shared transaction abstraction. Would you like me to open a follow-up issue to track wrapping the updateStatus writes in a transaction once that alignment work is done?
🧠 Learnings used
Learnt from: CR
Repo: Producdevity/EmuReady PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-06-05T11:58:33.136Z
Learning: Applies to src/server/api/routers/**/*.ts : Use specialized procedures such as `protectedProcedure`, `adminProcedure`, and `permissionProcedure(...)` instead of ad hoc permission checks in routers
Learnt from: CR
Repo: Producdevity/EmuReady PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-06-05T11:58:33.136Z
Learning: Applies to src/server/api/routers/**/*.ts : Do not put raw Prisma queries or business logic in routers
Learnt from: CR
Repo: Producdevity/EmuReady PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-06-05T11:58:33.137Z
Learning: Routers in `src/server/api/routers/` are thin orchestration layers; they handle auth context, schema-validated input, repository/service calls, and response formatting
Learnt from: CR
Repo: Producdevity/EmuReady PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-06-05T11:58:33.137Z
Learning: Applies to src/server/{repositories,services}/**/*.ts : Use transactions for multi-step writes that must stay consistent in Prisma operations
Learnt from: CR
Repo: Producdevity/EmuReady PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-06-05T11:58:33.137Z
Learning: Listings and PC Listings must stay behaviorally aligned for voting, comments, moderation, trust effects, notifications, and approval flows
Learnt from: CR
Repo: Producdevity/EmuReady PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-06-05T11:58:33.137Z
Learning: A Listing is a handheld compatibility report: game plus handheld device plus emulator; A PC Listing is a PC compatibility report: game plus PC hardware plus emulator
Comment on lines
+169
to
+192
| if (status === ReportStatus.RESOLVED) { | ||
| await trustService.logAction({ | ||
| userId: report.reportedById, | ||
| action: TrustAction.REPORT_CONFIRMED, | ||
| metadata: { | ||
| reportId, | ||
| pcListingId: report.pcListingId, | ||
| reviewedBy: reviewerId, | ||
| reason: report.reason, | ||
| }, | ||
| }) | ||
| } else if (status === ReportStatus.DISMISSED) { | ||
| await trustService.logAction({ | ||
| userId: report.reportedById, | ||
| action: TrustAction.FALSE_REPORT, | ||
| metadata: { | ||
| reportId, | ||
| pcListingId: report.pcListingId, | ||
| reviewedBy: reviewerId, | ||
| reason: report.reason, | ||
| reviewNotes, | ||
| }, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Guard against duplicate trust effects on repeated status updates.
If the same report is set to RESOLVED/DISMISSED multiple times, trust actions are logged repeatedly because current state isn’t checked before logging.
Suggested guard
+ if (report.status === status) {
+ return ctx.prisma.pcListingReport.findUnique({ where: { id: reportId } })
+ }📝 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.
Suggested change
| if (status === ReportStatus.RESOLVED) { | |
| await trustService.logAction({ | |
| userId: report.reportedById, | |
| action: TrustAction.REPORT_CONFIRMED, | |
| metadata: { | |
| reportId, | |
| pcListingId: report.pcListingId, | |
| reviewedBy: reviewerId, | |
| reason: report.reason, | |
| }, | |
| }) | |
| } else if (status === ReportStatus.DISMISSED) { | |
| await trustService.logAction({ | |
| userId: report.reportedById, | |
| action: TrustAction.FALSE_REPORT, | |
| metadata: { | |
| reportId, | |
| pcListingId: report.pcListingId, | |
| reviewedBy: reviewerId, | |
| reason: report.reason, | |
| reviewNotes, | |
| }, | |
| }) | |
| } | |
| if (report.status === status) { | |
| return ctx.prisma.pcListingReport.findUnique({ where: { id: reportId } }) | |
| } | |
| if (status === ReportStatus.RESOLVED) { | |
| await trustService.logAction({ | |
| userId: report.reportedById, | |
| action: TrustAction.REPORT_CONFIRMED, | |
| metadata: { | |
| reportId, | |
| pcListingId: report.pcListingId, | |
| reviewedBy: reviewerId, | |
| reason: report.reason, | |
| }, | |
| }) | |
| } else if (status === ReportStatus.DISMISSED) { | |
| await trustService.logAction({ | |
| userId: report.reportedById, | |
| action: TrustAction.FALSE_REPORT, | |
| metadata: { | |
| reportId, | |
| pcListingId: report.pcListingId, | |
| reviewedBy: reviewerId, | |
| reason: report.reason, | |
| reviewNotes, | |
| }, | |
| }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/api/routers/pcListingReports.ts` around lines 169 - 192, The
handler currently always calls trustService.logAction when status ===
ReportStatus.RESOLVED or DISMISSED which causes duplicate trust effects on
repeated updates; modify the code that performs logging (the block that calls
trustService.logAction) to first compare the incoming status with the report's
existing status (e.g., report.status or the fetched existingReport.status) and
only call trustService.logAction when the status is actually changing to
RESOLVED or to DISMISSED (i.e., previousStatus !== newStatus && newStatus ===
ReportStatus.RESOLVED/DISMISSED), preserving the same metadata and
reviewerId/reason fields so repeated identical updates become no-ops for trust
logging.
Comment on lines
+218
to
+225
| const report = await ctx.prisma.pcListingReport.findUnique({ | ||
| where: { id: input.id }, | ||
| }) | ||
|
|
||
| if (!report) return ResourceError.listingReport.notFound() | ||
|
|
||
| return ctx.prisma.pcListingReport.delete({ | ||
| where: { id: input.id }, |
There was a problem hiding this comment.
Avoid check-then-delete race in report deletion.
findUnique followed by delete can race and produce an unhandled not-found failure if another request deletes the same row between calls.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/api/routers/pcListingReports.ts` around lines 218 - 225, Avoid the
check-then-delete race by removing the separate pcListingReport.findUnique call
and performing the delete atomically with ctx.prisma.pcListingReport.delete,
wrapping that call in a try/catch; if the delete throws the Prisma "not found"
error (e.g., PrismaClientKnownRequestError with the P2025 code), return
ResourceError.listingReport.notFound(), otherwise rethrow or log unexpected
errors. Locate the current usages of pcListingReport.findUnique and
pcListingReport.delete in this handler and replace the two-step flow with the
single delete + error handling logic.
Comment on lines
+292
to
+299
| const comment = await ctx.prisma.pcListingComment.findUnique({ | ||
| where: { id: commentId }, | ||
| }) | ||
|
|
||
| if (!comment) { | ||
| return ResourceError.comment.notFound() | ||
| } | ||
|
|
There was a problem hiding this comment.
Block votes on soft-deleted comments.
delete only sets deletedAt, but vote accepts any comment found by id. That lets users keep changing score and trust state on comments that no longer appear in the thread. Reject when comment.deletedAt is set before entering the transaction.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/api/routers/pcListings/comments.ts` around lines 292 - 299, The
handler currently fetches the comment via ctx.prisma.pcListingComment.findUnique
and proceeds even if it's soft-deleted; before entering the transaction that
updates votes (the function handling vote updates), check comment.deletedAt and
if it's set return ResourceError.comment.notFound() (or the same error path used
for missing comments) so voting is rejected for soft-deleted comments; update
the logic around the existing comment lookup and before any call to the
transaction/vote update code to enforce this check.
Owner
Author
There was a problem hiding this comment.
is this issue also present in listing comments?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the listings comments router and check the vote handler
fd -t f 'comments.ts' src/server/api/routers/listings/Length of output: 218
🏁 Script executed:
#!/bin/bash
# Check the vote handler in listings comments router
rg -n 'vote|deletedAt|findUnique' src/server/api/routers/listings/comments.ts -A 5 -B 2Length of output: 7013
@Producdevity Yes, the same gap exists in src/server/api/routers/listings/comments.ts. The vote handler (line 241) fetches the comment and checks for existence but has no deletedAt guard before entering the transaction — identical to the PC listings case. Other mutations in that file (edit, delete, pinComment) all check deletedAt correctly.
The fix is the same: add a deletedAt check immediately after the comment lookup:
// listings/comments.ts — vote handler, after line 251
if (comment.deletedAt) return ResourceError.comment.notFound()Would you like me to open a follow-up issue to track this, or would you prefer to address it in this PR?
🧠 Learnings used
Learnt from: CR
Repo: Producdevity/EmuReady PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-06-05T11:58:33.136Z
Learning: Applies to src/server/api/routers/**/*.ts : Use specialized procedures such as `protectedProcedure`, `adminProcedure`, and `permissionProcedure(...)` instead of ad hoc permission checks in routers
Learnt from: CR
Repo: Producdevity/EmuReady PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-06-05T11:58:33.136Z
Learning: Applies to src/server/api/routers/**/*.ts : Do not put raw Prisma queries or business logic in routers
Comment on lines
+345
to
+373
| const updatedPcListing = await ctx.prisma.pcListing.update({ | ||
| where: { id }, | ||
| data: { ...updateData, updatedAt: new Date() }, | ||
| include: { | ||
| game: { include: { system: true } }, | ||
| cpu: { include: { brand: true } }, | ||
| gpu: { include: { brand: true } }, | ||
| emulator: true, | ||
| performance: true, | ||
| author: true, | ||
| customFieldValues: { | ||
| include: { customFieldDefinition: { include: { category: true } } }, | ||
| }, | ||
| }, | ||
| }) | ||
|
|
||
| if (customFieldValues) { | ||
| await ctx.prisma.pcListingCustomFieldValue.deleteMany({ where: { pcListingId: id } }) | ||
|
|
||
| if (customFieldValues.length > 0) { | ||
| await ctx.prisma.pcListingCustomFieldValue.createMany({ | ||
| data: customFieldValues.map((cfv) => ({ | ||
| pcListingId: id, | ||
| customFieldDefinitionId: cfv.customFieldDefinitionId, | ||
| value: toPrismaCustomFieldValue(cfv.value), | ||
| })), | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Make the listing update and custom-field replacement atomic.
updatedPcListing is loaded before the old custom fields are deleted and recreated. That causes two problems: a successful mutation can return stale customFieldValues, and a failure after deleteMany leaves the base listing updated with its custom fields partially or fully removed. Wrap the whole write in a transaction and re-read after the custom-field writes complete.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/api/routers/pcListings/core.ts` around lines 345 - 373, The update
and custom-field replacement must be run inside a single transaction so the base
pcListing update and the pcListingCustomFieldValue deleteMany/createMany are
atomic; move the ctx.prisma.pcListing.update,
ctx.prisma.pcListingCustomFieldValue.deleteMany and createMany calls into one
ctx.prisma.$transaction (or use Prisma's interactive transaction), perform
deleteThenCreate of customFieldValues within that transaction (using
toPrismaCustomFieldValue for mapping), and after the transaction completes
re-read the pcListing (the updatedPcListing) with the same includes
(game.system, cpu.brand, gpu.brand, emulator, performance, author,
customFieldValues+customFieldDefinition.category) to return fresh
customFieldValues rather than the stale pre-transaction object.
Comment on lines
+11
to
+27
| function isJsonRecord(value: unknown): value is Record<string, unknown> { | ||
| return typeof value === 'object' && value !== null && !Array.isArray(value) | ||
| } | ||
|
|
||
| function toPrismaNestedJsonValue(value: unknown): Prisma.InputJsonValue | null { | ||
| if (value === null) return null | ||
| if (typeof value === 'string') return value | ||
| if (typeof value === 'number') return value | ||
| if (typeof value === 'boolean') return value | ||
| if (Array.isArray(value)) return value.map(toPrismaNestedJsonValue) | ||
| if (isJsonRecord(value)) { | ||
| const result: Record<string, Prisma.InputJsonValue | null> = {} | ||
| for (const [key, entryValue] of Object.entries(value)) { | ||
| result[key] = toPrismaNestedJsonValue(entryValue) | ||
| } | ||
|
|
||
| return result |
There was a problem hiding this comment.
Reject non-plain objects before recursing into JSON conversion.
isJsonRecord() currently accepts any object, so values like Date, Map, Set, or class instances are treated as JSON records. For example, Object.entries(new Date()) is empty, so that input gets silently stored as {} instead of being rejected. Restrict this branch to plain objects only.
Suggested guard
function isJsonRecord(value: unknown): value is Record<string, unknown> {
- return typeof value === 'object' && value !== null && !Array.isArray(value)
+ if (typeof value !== 'object' || value === null || Array.isArray(value)) {
+ return false
+ }
+
+ const prototype = Object.getPrototypeOf(value)
+ return prototype === Object.prototype || prototype === null
}📝 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.
Suggested change
| function isJsonRecord(value: unknown): value is Record<string, unknown> { | |
| return typeof value === 'object' && value !== null && !Array.isArray(value) | |
| } | |
| function toPrismaNestedJsonValue(value: unknown): Prisma.InputJsonValue | null { | |
| if (value === null) return null | |
| if (typeof value === 'string') return value | |
| if (typeof value === 'number') return value | |
| if (typeof value === 'boolean') return value | |
| if (Array.isArray(value)) return value.map(toPrismaNestedJsonValue) | |
| if (isJsonRecord(value)) { | |
| const result: Record<string, Prisma.InputJsonValue | null> = {} | |
| for (const [key, entryValue] of Object.entries(value)) { | |
| result[key] = toPrismaNestedJsonValue(entryValue) | |
| } | |
| return result | |
| function isJsonRecord(value: unknown): value is Record<string, unknown> { | |
| if (typeof value !== 'object' || value === null || Array.isArray(value)) { | |
| return false | |
| } | |
| const prototype = Object.getPrototypeOf(value) | |
| return prototype === Object.prototype || prototype === null | |
| } | |
| function toPrismaNestedJsonValue(value: unknown): Prisma.InputJsonValue | null { | |
| if (value === null) return null | |
| if (typeof value === 'string') return value | |
| if (typeof value === 'number') return value | |
| if (typeof value === 'boolean') return value | |
| if (Array.isArray(value)) return value.map(toPrismaNestedJsonValue) | |
| if (isJsonRecord(value)) { | |
| const result: Record<string, Prisma.InputJsonValue | null> = {} | |
| for (const [key, entryValue] of Object.entries(value)) { | |
| result[key] = toPrismaNestedJsonValue(entryValue) | |
| } | |
| return result |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/api/routers/pcListings/utils.ts` around lines 11 - 27, Change
isJsonRecord and conversion logic to only accept plain objects: update
isJsonRecord(value) to return true only when typeof value === 'object' && value
!== null && !Array.isArray(value) && (Object.getPrototypeOf(value) ===
Object.prototype || Object.getPrototypeOf(value) === null). Then in
toPrismaNestedJsonValue, when encountering an object that fails isJsonRecord
(e.g. Date, Map, Set, class instance) do not recurse—reject it explicitly (throw
a clear error or return null per project convention) instead of treating it as
an empty record; reference functions isJsonRecord and toPrismaNestedJsonValue
when making the change.
Comment on lines
247
to
+250
| if (!ctx.session?.user) AppError.unauthorized() | ||
|
|
||
| if (!hasRolePermission(ctx.session.user.role, Role.MODERATOR)) { | ||
| AppError.insufficientRole(Role.MODERATOR) | ||
| return AppError.insufficientRole(Role.MODERATOR) |
There was a problem hiding this comment.
Return immediately after unauthorized checks in role procedures.
In these guards, AppError.unauthorized() is called without return. Execution continues and then accesses ctx.session.user.role, which can crash the request path for unauthenticated users.
Suggested fix
export const moderatorProcedure = t.procedure.use(performanceMiddleware).use(({ ctx, next }) => {
- if (!ctx.session?.user) AppError.unauthorized()
+ if (!ctx.session?.user) return AppError.unauthorized()
if (!hasRolePermission(ctx.session.user.role, Role.MODERATOR)) {
return AppError.insufficientRole(Role.MODERATOR)
}
return next({ ctx: { session: { ...ctx.session, user: ctx.session.user } } })
})
export const developerProcedure = t.procedure.use(performanceMiddleware).use(({ ctx, next }) => {
- if (!ctx.session?.user) AppError.unauthorized()
+ if (!ctx.session?.user) return AppError.unauthorized()
if (!hasRolePermission(ctx.session.user.role, Role.DEVELOPER)) {
return AppError.insufficientRole(Role.DEVELOPER)
}
return next({ ctx: { session: { ...ctx.session, user: ctx.session.user } } })
})
export const adminProcedure = t.procedure.use(performanceMiddleware).use(({ ctx, next }) => {
- if (!ctx.session?.user) AppError.unauthorized()
+ if (!ctx.session?.user) return AppError.unauthorized()
if (!hasRolePermission(ctx.session.user.role, Role.ADMIN)) {
return AppError.insufficientRole(Role.ADMIN)
}
return next({ ctx: { session: { ...ctx.session, user: ctx.session.user } } })
})Also applies to: 260-263, 273-276
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/api/trpc.ts` around lines 247 - 250, The guard calls call
AppError.unauthorized() but do not return, so execution continues and may access
ctx.session.user.role; change those guards to return the unauthorized result
(e.g., return AppError.unauthorized()) instead of just calling it, and apply the
same fix to the other role-check blocks that call hasRolePermission(...) and
AppError.unauthorized() (the other occurrences around the second and third role
checks). Ensure you update the checks that reference ctx.session,
AppError.unauthorized(), hasRolePermission, and Role.MODERATOR so they return
immediately on unauthorized.
Owner
Author
There was a problem hiding this comment.
does that matter? doesn't AppError.insufficientRole throw the error?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -e ts 'errors' src/lib
rg -n "unauthorized|insufficientRole|class AppError|static " src/lib/errors.ts | head -60Length of output: 2786
@Producdevity — you're right, my concern was incorrect. All AppError static methods are typed as (): never, meaning they always throw. TypeScript's control-flow analysis also recognises never-returning calls for type narrowing, so the missing return is a harmless style issue with no runtime or type-safety impact. Sorry for the noise.
✏️ Learnings added
Learnt from: Producdevity
Repo: Producdevity/EmuReady PR: 441
File: src/server/api/trpc.ts:247-250
Timestamp: 2026-06-08T19:01:43.515Z
Learning: In the EmuReady codebase (`src/lib/errors.ts`), all `AppError` static methods (e.g., `unauthorized`, `insufficientRole`, `forbidden`, `notFound`, etc.) are typed as returning `never` and always throw a `TRPCError` internally. Calling them without `return` is safe: execution does not continue and TypeScript narrows correctly. Adding `return` before them is a style choice only, not a correctness fix.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: Producdevity/EmuReady PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-06-05T11:58:33.136Z
Learning: Applies to src/server/api/routers/**/*.ts : Use specialized procedures such as `protectedProcedure`, `adminProcedure`, and `permissionProcedure(...)` instead of ad hoc permission checks in routers
Learnt from: CR
Repo: Producdevity/EmuReady PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-06-05T11:58:33.136Z
Learning: Applies to src/server/**/*.{ts,tsx} : Use `AppError` and `ResourceError` helpers instead of raw `Error`, raw strings, or one-off `TRPCError` usage
Learnt from: CR
Repo: Producdevity/EmuReady PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-06-05T11:58:33.137Z
Learning: Applies to src/server/**/*.{ts,tsx} : Pass `requestingUserRole` when admin override behavior is supported in write operations
| @@ -1,4 +1,5 @@ | |||
| import { AppError } from '@/lib/errors' | |||
| // TODO: carefully consider wtf this file is. seems like none of this is how it should be done. | |||
There was a problem hiding this comment.
Replace profanity TODOs with actionable technical notes.
These comments are non-actionable and not suitable for repository history. Please replace with concise TODOs that state specific remediation and (ideally) link a tracking issue.
Also applies to: 78-78, 94-94
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/utils/security-validation.ts` at line 2, Replace the
non-actionable profanity TODOs in src/server/utils/security-validation.ts (at
the file header and at the comments near lines referencing additional TODOs)
with concise, actionable TODOs that state the specific remediation: e.g.,
"Implement centralized input validation using validateRequest() (or a new
middleware) to enforce schema-based checks, normalize/escape inputs, and add
unit/integration tests; see tracking issue `#1234`." For each replaced comment,
mention the responsible symbol or area (e.g., the module-level initialization,
the validateRequest/validateSecurity functions, or any middleware functions
nearby) and include a link or issue ID so future contributors know where to
track work; keep each TODO one sentence and actionable.
Owner
Author
There was a problem hiding this comment.
Go through every function here and create one coherent github issue documenting what is suggested based on official source / nextjs documentation. do not rely on memory, find up to date info and docs. (1 issue for all the functions in this file)
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all usages of functions in security-validation.ts
rg -n "validateRequired|validateNonEmptyArray|validateStringFormat|validateEnum|validatePagination|sanitizeInput|ValidationPatterns|security-validation" --type ts -C 2Length of output: 16614
🌐 Web query:
Next.js App Router server-side input validation best practices Zod 2024 2025
Length of output: 20757
🌐 Web query:
Next.js recommended XSS prevention sanitization input site:nextjs.org OR site:owasp.org 2024
Length of output: 11679
🏁 Script executed:
#!/bin/bash
gh issue create \
--repo Producdevity/EmuReady \
--title "refactor: replace security-validation.ts helpers with Zod schemas and a proper sanitization library" \
--assignee Producdevity \
--body "## Overview
\`src/server/utils/security-validation.ts\` contains a collection of ad-hoc runtime validation and sanitization helpers that duplicate functionality already provided by Zod (used at every tRPC boundary) or that apply insufficient security controls. This issue tracks replacing each function with the idiomatic, officially-recommended approach.
**References:**
- Next.js Data Security guide: https://nextjs.org/docs/app/guides/data-security
- OWASP Input Validation Cheat Sheet: https://cheatsheetseries.owasp.org/cheatsheets/Input_Validation_Cheat_Sheet.html
- OWASP XSS Prevention Cheat Sheet: https://cheatsheetseries.owasp.org/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html
- Related PR: `#441`
---
## Functions & recommended replacements
### 1. \`validatePagination\`
**Current problem:** Duplicates pagination constraints that Zod can express declaratively. Called in many routers _after_ the input has already been parsed by a Zod schema, making it a redundant second pass.
**Recommendation:** Express pagination constraints directly in each router's Zod input schema:
\`\`\`ts
page: z.number().int().min(1).default(1),
limit: z.number().int().min(1).max(100).default(20),
\`\`\`
This is consistent with the pattern already introduced in the updated \`GetPcListingReportsSchema\` in \`src/schemas/pcListing.ts\`. All routers (\`games.ts\`, \`listingReports.ts\`, \`pcListingReports.ts\`, \`pcListings/core.ts\`, \`listings/core.ts\`) should migrate to schema-level constraints and stop calling \`validatePagination\`.
---
### 2. \`validateEnum\`
**Current problem:** Called inside router procedure handlers _after_ tRPC/Zod has already parsed and validated the input object. Any enum field that reaches this guard has already passed schema validation, so the check is a no-op under correct usage and masks a real schema gap if it is ever reached.
**Recommendation:** Move enum validation into the Zod input schema using \`z.nativeEnum()\` or \`z.enum([...])\`:
\`\`\`ts
reason: z.nativeEnum(ReportReason),
status: z.nativeEnum(ReportStatus),
\`\`\`
Per the Next.js Data Security guide, _\"Action arguments [should be] validated in the action or inside the Data Access Layer\"_ — the tRPC Zod schema is exactly that boundary.
---
### 3. \`sanitizeInput\`
**Current problem:** Strips only \`<\`, \`>\`, \`javascript:\`, and \`on*=\` patterns. OWASP's Input Validation Cheat Sheet explicitly states that _\"denylisting can be useful as an additional layer of defense to catch some common malicious patterns, [but] should not be relied upon as the primary method.\"_ This regex is trivially bypassed and gives false confidence.
In practice:
- **React** already HTML-escapes all values rendered via JSX, preventing reflected XSS in the UI.
- **Prisma** uses parameterized queries, preventing SQL injection.
- For user-supplied text stored and later rendered as **plain text**, no sanitization is needed beyond what Prisma + React provide.
- For any field that will be rendered as **HTML** (e.g. rich-text descriptions), use a library purpose-built for the job, such as [\`sanitize-html\`](https://www.npmjs.com/package/sanitize-html) or [\`DOMPurify\`](https://www.npmjs.com/package/dompurify) (JSDOM variant for Node).
**Recommendation:**
1. Remove \`sanitizeInput\` calls from plain-text fields (\`notes\`, \`searchTerm\`, report descriptions) — React + Prisma cover those.
2. If any field is genuinely rendered as HTML, introduce a dedicated server-side sanitizer (e.g. \`sanitize-html\`) scoped to that field only.
3. Add a [Content-Security-Policy header](https://nextjs.org/docs/app/api-reference/config/next-config-js/headers) in \`next.config\` as a defence-in-depth layer per the OWASP CSP Cheat Sheet.
---
### 4. \`validateRequired\`
**Current problem:** Replicates what a non-optional Zod field already enforces. Currently used in \`user-pc-presets.repository.ts\` to assert that \`userId\` and \`name\` are present.
**Recommendation:** Ensure the repository is only called with data that has already been validated by a Zod schema (e.g. \`z.string().min(1)\`). If the repository must remain callable without a prior Zod pass, use TypeScript's type system (non-nullable types) so the compiler—rather than a runtime guard—enforces the constraint.
---
### 5. \`validateNonEmptyArray\`
**Current problem:** Duplicates \`z.array(z.string().uuid()).min(1)\` which Zod expresses in one line.
**Recommendation:** Replace the guard in \`games.ts\` with \`z.array(...).min(1)\` in the input schema for the \`bulkApprove\`/\`bulkReject\` procedures.
---
### 6. \`validateStringFormat\` + \`ValidationPatterns\`
**Current problem:** Used to assert UUID format for \`userId\` in \`user-pc-presets.repository.ts\`. Zod provides \`z.string().uuid()\` natively.
**Recommendation:** Validate UUID fields in the tRPC input schema (\`z.string().uuid()\`) so the error is surfaced at the API boundary, not deep inside the repository.
---
## Suggested migration order
1. Update all Zod input schemas to encode pagination, enum, UUID, and non-empty-array constraints directly.
2. Remove all \`validatePagination\`, \`validateEnum\`, \`validateRequired\`, \`validateNonEmptyArray\`, \`validateStringFormat\` call sites.
3. Audit every \`sanitizeInput\` call site: remove where React + Prisma are sufficient; replace with \`sanitize-html\` where HTML rendering occurs.
4. Delete \`src/server/utils/security-validation.ts\` once no callers remain.
5. Add a CSP header in \`next.config\` as a complementary layer.
"Length of output: 5869
🏁 Script executed:
#!/bin/bash
# Get the issue URL
gh issue list --repo Producdevity/EmuReady --assignee Producdevity --limit 5 --json number,title,urlLength of output: 1023
@Producdevity, the issue has been created: #442
It covers all six exports in the file with function-by-function analysis and officially-sourced recommendations:
| Function | Problem | Recommended replacement |
|---|---|---|
validatePagination |
Redundant after Zod parsing | z.number().int().min(1).default(1) in input schema |
validateEnum |
No-op — Zod already ran | z.nativeEnum(ReportReason/ReportStatus) in input schema |
sanitizeInput |
Denylist regex is OWASP-insufficient | Remove for plain text (React + Prisma cover it); use sanitize-html for any HTML fields; add CSP header |
validateRequired |
Duplicates non-optional Zod fields | Non-nullable TypeScript types + Zod schema enforcement upstream |
validateNonEmptyArray |
Duplicates z.array(...).min(1) |
Move into input schema |
validateStringFormat / ValidationPatterns |
Duplicates z.string().uuid() |
Use Zod's built-in UUID validator at the API boundary |
The end goal tracked in the issue is deleting the file entirely once all call sites are migrated.
🧠 Learnings used
Learnt from: CR
Repo: Producdevity/EmuReady PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-06-05T11:58:33.137Z
Learning: Applies to src/server/api/routers/**/*.ts : Validate input at API boundaries with Zod schemas
Learnt from: CR
Repo: Producdevity/EmuReady PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-06-05T11:58:33.137Z
Learning: Applies to src/server/**/*.{ts,tsx} : Reads may accept user IDs for filtering, but writes must validate ownership or permission
Learnt from: CR
Repo: Producdevity/EmuReady PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-06-05T11:58:33.137Z
Learning: Applies to src/server/**/*.{ts,tsx} : For write operations, never trust a user ID supplied by input when ownership matters; use `ctx.session.user.id`
Learnt from: CR
Repo: Producdevity/EmuReady PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-06-05T11:58:33.136Z
Learning: Applies to src/server/**/*.{ts,tsx} : Use `AppError` and `ResourceError` helpers instead of raw `Error`, raw strings, or one-off `TRPCError` usage
Learnt from: CR
Repo: Producdevity/EmuReady PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-06-05T11:58:33.136Z
Learning: Applies to src/server/api/routers/**/*.ts : Use specialized procedures such as `protectedProcedure`, `adminProcedure`, and `permissionProcedure(...)` instead of ad hoc permission checks in routers
Learnt from: CR
Repo: Producdevity/EmuReady PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-06-05T11:58:33.137Z
Learning: Do not remove or rewrite existing TODO comments unless explicitly requested or the TODO is made obsolete by the code change
Learnt from: CR
Repo: Producdevity/EmuReady PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-06-05T11:58:33.137Z
Learning: Before any Next.js work, find and read the relevant doc in `node_modules/next/dist/docs/` as the docs are the source of truth, not training data
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
split pclistings router, and introduce proper structure
Fixes # (issue)
Type of change
How Has This Been Tested?
Screenshots (if applicable)
Checklist
Notes for reviewers
Summary by CodeRabbit
Release Notes
New Features
Improvements