Refactor hardware modules around feature-owned architecture#441
Open
Producdevity wants to merge 16 commits into
Open
Refactor hardware modules around feature-owned architecture#441Producdevity wants to merge 16 commits into
Producdevity wants to merge 16 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Important Review skippedToo many files! This PR contains 271 files, which is 121 over the limit of 150. To get a review, narrow the scope: Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: Files selected for processing (271)
You can disable this status message by setting the Use the checkbox below for a quick retry:
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 (2 warnings)
✅ Passed checks (3 passed)
Tip: You can configure your own custom pre-merge checks in the settings. Finishing TouchesGenerate 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
+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.
…lated utilities and tests
…U and GPU entities.
Owner
Author
|
@coderabbitai review --dir src |
|
✅ Action performedFull review finished. |
Owner
Author
the review didn't start |
Producdevity
changed the title
Owner
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
Owner
Author
@coderabbitai you keep saying this but there has no review been performed. instructions: do a full review of this PR. |
|
✅ Action performedFull review finished. |
…based game image validation and rendering
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.
Summary
This PR establishes the new feature-owned architecture for hardware lookup domains, using CPU and GPU as the reference implementation for future refactors.
It moves CPU/GPU behavior out of legacy router-heavy patterns and into explicit feature modules with shared schemas/types, server-side repositories/services/policies/mappers, and client-side admin/query components. The goal is to make these domains consistent, testable, and easier to extend without duplicating shapes across web, mobile, and admin flows.
What changed
src/features/hardware/cpuandsrc/features/hardware/gpuas the first full examples of the new module structure.output(...)contracts for feature-owned tRPC proceduresArchitecture direction
CPU and GPU now serve as the first concrete pattern for future domain refactors:
This keeps the architecture enforced by code boundaries and imports instead of relying only on README guidance.
Verification
pnpm lintpnpm typespnpm testSummary by CodeRabbit
Release Notes
New Features
Improvements