feat: trust system overhaul, GameNative emulator support, vote system fixes, and E2E test cleanup#339
Conversation
… logic with tests for moderation UI enhancements
… setup dependencies
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds transaction-aware trust handling and moderator tooling (panel, votes UI), GameNative import/seed support, vote-related DB indexes and enum, extensive router/service/refactor for vote/trust/nullification flows, many new tests, and large Playwright E2E test and fixture simplifications. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ListingDetailsClient as ListingDetailsClient<br/>(Component)
participant PrismaDB as Prisma<br/>Database
participant TrustService
participant TrustLog as TrustActionLog<br/>Table
Client->>ListingDetailsClient: User submits vote
activate ListingDetailsClient
ListingDetailsClient->>PrismaDB: Start transaction (tx)
activate PrismaDB
PrismaDB->>PrismaDB: create/update/delete vote record
PrismaDB->>TrustService: handleListingVoteTrustEffects(tx, action, ...)
activate TrustService
alt created
TrustService->>TrustLog: logAction(voter and author)
else updated
TrustService->>TrustLog: reverseLogAction(previous)
TrustService->>TrustLog: logAction(new)
else deleted
TrustService->>TrustLog: reverseLogAction(previous)
end
TrustService-->>PrismaDB: trust logs written within tx
deactivate TrustService
PrismaDB-->>ListingDetailsClient: commit transaction
deactivate PrismaDB
ListingDetailsClient-->>Client: update UI
deactivate ListingDetailsClient
sequenceDiagram
participant User
participant ModeratorInfoPanel as ModeratorInfoPanel<br/>(Component)
participant TRPC as TRPC Router<br/>(admin.moderatorInfo)
participant ListingsRepo as ListingsRepository
participant PrismaDB as Prisma<br/>Database
User->>ModeratorInfoPanel: Click "Show Moderator Info"
activate ModeratorInfoPanel
ModeratorInfoPanel->>TRPC: query moderatorInfo(id,type)
activate TRPC
TRPC->>ListingsRepo: getModeratorInfo(id)
activate ListingsRepo
ListingsRepo->>PrismaDB: fetch listing approval + votes
PrismaDB-->>ListingsRepo: return rows
ListingsRepo->>ListingsRepo: computeVoteCounts(votes)
ListingsRepo-->>TRPC: { approval, votes, voteCounts }
deactivate ListingsRepo
TRPC-->>ModeratorInfoPanel: return data
deactivate TRPC
ModeratorInfoPanel->>ModeratorInfoPanel: render ApprovalSection & VotesSection
ModeratorInfoPanel-->>User: display moderator info
deactivate ModeratorInfoPanel
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
src/components/ui/modals/Modal.tsx (1)
68-76:⚠️ Potential issue | 🟡 MinorDialog is unnamed when
props.titleis omitted.
aria-label={props.title}resolves toundefinedwhen no title is passed (e.g., callers that render custom headers viachildrenand/or sethideCloseButton). React then drops the attribute and the dialog has no accessible name, which defeats the purpose ofrole="dialog"+aria-modal="true"for screen readers.Consider falling back to
aria-labelledbypointing at the internal<h2>when present, or only applyingaria-labelwhen a string title exists and otherwise accepting anariaLabel/ariaLabelledByprop from the caller.Proposed adjustment
- role="dialog" - aria-modal="true" - aria-label={props.title} + role="dialog" + aria-modal="true" + {...(props.title ? { 'aria-label': props.title } : {})}Also note:
role="dialog"without focus trapping / initial focus management is an a11y gap, but that is pre-existing and out of scope here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/modals/Modal.tsx` around lines 68 - 76, The dialog's aria-label is set directly from props.title which can be undefined, leaving the dialog unnamed to assistive tech; update the Modal component so it only sets aria-label when props.title is a non-empty string, otherwise accept and apply an ariaLabel or ariaLabelledBy prop from the caller; additionally, when the component renders its internal <h2> (the title heading), generate and assign a stable id (e.g., headerId) to that <h2> and set aria-labelledby to that id when no aria-label is provided so the dialog always has an accessible name; ensure the logic in the JSX that currently uses aria-label={props.title} (and the title rendering path) is adjusted to prefer aria-label when provided, fall back to ariaLabel/ariaLabelledBy props, and finally fall back to aria-labelledby=headerId if an internal title <h2> exists.src/server/api/routers/listings/comments.ts (2)
395-416:⚠️ Potential issue | 🟠 MajorMove notification and analytics side effects after commit.
These side effects fire from inside the transaction callback. If analytics throws, or if the transaction later fails to commit, the vote changes roll back after a notification may already have been emitted. Return the notification/analytics payload from the transaction and emit after
$transactionresolves.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/api/routers/listings/comments.ts` around lines 395 - 416, The notification and analytics calls (notificationEventEmitter.emitNotificationEvent and analytics.engagement.commentVote) are executed inside the DB transaction callback; instead return a payload from the transaction (e.g. include the notification payload and analytics args using identifiers like comment, commentId, finalVoteValue, existingVote?.value, trustActionNeeded, ctx.session.user.id) instead of emitting/calling them there, then after the prisma.$transaction / transaction promise resolves perform notificationEventEmitter.emitNotificationEvent(...) and analytics.engagement.commentVote(...) using the returned payload so side effects only run after a successful commit.
329-366:⚠️ Potential issue | 🟠 MajorThis transaction still does not close the concurrent insert race.
Line 334 reads
existingVoteinside the transaction, butfindUniquedoes not lock a missing(userId, commentId)row under default Read Committed isolation. Two concurrent first votes can both readnull, then both attemptcreateat line 364, causing the second to fail with Prisma P2002 on the unique constraint.Add explicit conflict handling via retry logic, a safe
upsertpattern, or serializable isolation with transaction restart semantics to prevent duplicate inserts.Additionally, side effects leak inside the transaction. Lines 379–413 emit notifications and analytics events within the transaction scope. If the transaction fails (e.g., due to the P2002 above), these side effects may still propagate, causing inconsistency between the database and notification/analytics state. Move side effects outside the transaction after successful commit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/api/routers/listings/comments.ts` around lines 329 - 366, The transaction can still race because tx.commentVote.findUnique followed by tx.commentVote.create allows two concurrent requests to both see null and cause a Prisma P2002; change the logic in the voting transaction (functions/blocks using ctx.prisma.$transaction and tx.commentVote.findUnique/create/update) to either perform a safe upsert (atomic create-or-update), implement retry-on-conflict around the transaction for P2002 errors, or run under serializable isolation with transaction restart semantics so duplicate inserts cannot occur; also remove all side effects (notification/analytics emissions that currently run inside the transaction after the db writes) from the $transaction block and instead collect the results (e.g., voteResult, scoreChange, trustActionNeeded) and perform notifications/analytics only after the transaction successfully commits.src/server/api/routers/listings/core.ts (1)
269-286:⚠️ Potential issue | 🟠 Major
findUniquedoes not lock the unique key, leaving a createrace exposed.Line 271's comment claiming a
SELECT ... FOR UPDATEequivalent is incorrect. The read at line 272 does not lock anything; two concurrent first votes can both see no existing row and both reachtx.vote.createon line 284, causing the second to fail with P2002 (unique constraint violation). There is no error handling for this constraint failure. Use an explicit upsert pattern, retry logic on P2002, or raise the transaction isolation level to Serializable with client-side retry handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/api/routers/listings/core.ts` around lines 269 - 286, The transaction uses tx.vote.findUnique then tx.vote.create which does not lock and allows a race causing P2002 unique constraint errors; change the logic to use an atomic upsert (tx.vote.upsert) for userId_listingId or implement client-side retry handling around the $transaction to catch Prisma P2002 errors and retry the transaction (or set Serializable isolation with retry) instead of blindly calling tx.vote.create; update references in this block (the $transaction callback, tx.vote.findUnique, tx.vote.create) to use tx.vote.upsert or wrap the $transaction in retry logic that specifically handles PrismaClientKnownRequestError with code "P2002".src/server/api/routers/pcListings.ts (2)
668-680:⚠️ Potential issue | 🔴 CriticalPreserve per-emulator developer authorization in bulk PC moderation.
bulkApprove/bulkRejectonly check that the actor is a developer, but unlike the single-item paths they never verify the developer is assigned to each listing’s emulator beforeupdateMany. That lets any developer bulk process PC listings for unrelated emulators.🛡️ Proposed fix
const pendingListings = await ctx.prisma.pcListing.findMany({ where: { id: { in: input.pcListingIds }, status: ApprovalStatus.PENDING }, - select: { id: true, gameId: true, authorId: true }, + select: { id: true, gameId: true, authorId: true, emulatorId: true }, }) +if (!isModerator && isDeveloper) { + const verifiedEmulatorIds = await repository.getVerifiedEmulatorIds(ctx.session.user.id) + const unauthorizedListings = pendingListings.filter( + (listing) => !verifiedEmulatorIds.includes(listing.emulatorId), + ) + + if (unauthorizedListings.length > 0) { + return ResourceError.pcListing.mustBeVerifiedToApprove() + } +}Apply the same check in
bulkReject, returningmustBeVerifiedToReject()there.Also applies to: 730-745
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/api/routers/pcListings.ts` around lines 668 - 680, bulkApprove and bulkReject perform updateMany without ensuring the acting developer is assigned to each listing's emulator, allowing developers to moderate listings for emulators they don't own; fix by filtering pendingListings to only those whose gameId (or emulator assignment) is associated with the current developer before calling ctx.prisma.pcListing.updateMany. In practice, in the bulkApprove and bulkReject handlers use the same per-listing authorization check used by single-item paths: fetch pendingListings (as in the pendingListings variable), validate each listing's emulator ownership against ctx.session.user.id (or lookup developer-emulator assignments), and for any listings not owned return mustBeVerifiedToReject() (or exclude them) so updateMany only touches authorized IDs; ensure processedByUserId and processedAt behavior remains the same and reference functions/vars pcListing.updateMany, pendingListings, bulkApprove, bulkReject, and mustBeVerifiedToReject().
1629-1629:⚠️ Potential issue | 🟠 MajorRemove the remaining report-author email projection.
This admin report query still returns
reportedBy.email, which leaves a PII leak in a PR explicitly reducing moderator/admin email exposure.🛡️ Proposed fix
- reportedBy: { select: { id: true, name: true, email: true } }, + reportedBy: { select: { id: true, name: true } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/api/routers/pcListings.ts` at line 1629, The admin report query in the pcListings router is still selecting reportedBy.email and must stop returning PII; locate the query that includes reportedBy: { select: { id: true, name: true, email: true } } and remove the email key so it becomes reportedBy: { select: { id: true, name: true } }; update any type/usage expectations that assume reportedBy.email is present (e.g., response shaping or serializers) to avoid referencing the removed field.src/server/api/routers/listings/admin.ts (1)
621-708:⚠️ Potential issue | 🔴 CriticalDo not let transaction error results fall through to post-commit work.
The transaction callback can return
ResourceError.../AppError..., but the new post-transaction trust code immediately dereferencestransactionResult.validListings/transactionResult.listingsToReject. Those validation paths will now mask the intended error with a runtimeTypeError.🛡️ Proposed fix direction
- if (unauthorizedListings.length > 0) { - return ResourceError.listing.mustBeVerifiedToApprove() - } + if (unauthorizedListings.length > 0) { + throw ResourceError.listing.mustBeVerifiedToApprove() + } ... - if (listingsToApprove.length === 0) { - return AppError.badRequest( + if (listingsToApprove.length === 0) { + throw AppError.badRequest( 'No valid pending listings found to approve. The listings may have already been processed.', ) }Apply the same pattern in
bulkReject, or move these validation checks before opening the transaction.Also applies to: 809-873
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/api/routers/listings/admin.ts` around lines 621 - 708, The transaction callback is returning ResourceError/AppError objects which can be returned as transactionResult and then cause post-commit TypeErrors when code dereferences transactionResult.validListings; fix by ensuring the tx callback never returns error objects — either perform those validation checks before opening the transaction (e.g., move the moderator/verified and "no valid pending listings" checks out of the tx) or, if kept inside the tx, throw the ResourceError/AppError instead of returning it so the outer try/catch sees the rejection; then before running post-commit work (the code that reads transactionResult.validListings and calls applyTrustAction / TrustAction.LISTING_APPROVED) assert transactionResult is the expected shape (or catch/propagate thrown errors) to guarantee you only run post-commit actions on a successful transaction.src/server/api/routers/mobile/listings.ts (1)
528-614:⚠️ Potential issue | 🟠 MajorNotification & analytics emit inside
$transaction— diverges fromvoteand can fire on rollback.In the
votemutation above (lines 240–314) theLISTING_VOTEDemit is correctly placed outsidectx.prisma.$transaction, so it only runs after the transaction commits. Here invoteComment, both theCOMMENT_VOTEDemit (lines 590–600) andanalytics.engagement.commentVote(...)(lines 606–611) execute inside the$transactioncallback. If any awaited call later in the callback throws (e.g., thecomment.updateat line 565 is already before the emit, but future edits could add writes after; also any error thrown after the emit would still commit the side effect), the transaction rolls back while the notification/analytics event has already been dispatched — leading to phantom notifications for votes that were never persisted.Move the notification emission and analytics call out of the transaction (mirror the
votemutation): return the necessary values from the$transactioncallback, then emit afterawait ctx.prisma.$transaction(...)resolves. This keeps DB writes atomic and side effects deferred until after commit.🛠️ Suggested shape
const result = await ctx.prisma.$transaction(async (tx) => { // ... existing findUnique / create / update / delete / score update / handleCommentVoteTrustEffects ... return { voteResult, trustActionNeeded, existingVote, finalVoteValue } }) if ( result.trustActionNeeded !== null && result.trustActionNeeded !== 'remove' && input.value !== null ) { notificationEventEmitter.emitNotificationEvent({ /* ...as before... */ }) } analytics.engagement.commentVote({ /* ...as before... */ }) return result.voteResult🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/api/routers/mobile/listings.ts` around lines 528 - 614, The notification and analytics side-effects are executed inside the ctx.prisma.$transaction in voteComment, which can cause phantom notifications if the transaction later rolls back; move notificationEventEmitter.emitNotificationEvent and analytics.engagement.commentVote out of the transaction by returning the minimal needed values from the $transaction callback (e.g., voteResult, trustActionNeeded, existingVote, finalVoteValue, comment/listing ids) and then, after await ctx.prisma.$transaction(...) resolves, perform the conditional emit and analytics call (mirroring the pattern used in vote) and finally return voteResult.src/lib/trust/service.ts (1)
246-383:⚠️ Potential issue | 🟠 MajorRemove unused
targetUserIdparameter from both method signatures, or explicitly capture it intometadataif intended for audit purposes.
targetUserIdis declared in bothlogAction(line 249) andreverseLogAction(line 318) but is never destructured or used. Callers insrc/server/utils/vote-trust-effects.tspasstargetUserId: voterIdexplicitly, expecting it to be persisted, yet it's silently dropped.The
TrustActionLogschema has notargetUserIdcolumn—only ametadataJSON field—and the schema comment indicatestargetUserIdshould be stored there. However, callers already includevoterIdin the metadata object (baseMeta), so the separate parameter is redundant and misleading.Options:
- Remove
targetUserIdfrom both method signatures (it's unused and callers already passvoterIdin metadata).- If
targetUserIdshould be a standard audit field, extract it in the methods and merge it into metadata as a fallback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/trust/service.ts` around lines 246 - 383, Remove the unused targetUserId parameter from the logAction and reverseLogAction method signatures (in Service.logAction and Service.reverseLogAction), delete any destructuring or references to it, and update their param types to only include userId, action/originalAction, and metadata; ensure metadata continues to carry any voter/target identifiers (merge targetUserId into metadata only if you decide to keep it as audit info) so trustActionLog.create still persists the needed info in metadata.src/server/services/vote-nullification.service.ts (1)
201-275:⚠️ Potential issue | 🟡 MinorSilent serialization risk when caller passes a
Prisma.TransactionClient.The signature accepts
PrismaClientOrTransactionand comments advertise parallelism across different vote types ("across different vote types in parallel", lines 255–256, 264). This is true whenprismais aPrismaClient, but once a transactional flow passestx, the Promise.all blocks (lines 208, 257, 270 innullifyUserVotes; lines 372, 414, 427 inrestoreUserVotes) execute serially on the single connection owned by the interactive transaction. The code remains correct, just no longer parallel—and a long-running interactive transaction performing sequential batches of 4findMany→ 4updateMany→ 4 recalc passes risks hitting the interactive-transaction timeout on large users.
TrustServicealready implements a solution pattern (if ('$transaction' in this.prisma)) to detect the context and conditionally wrap execution. Apply a similar approach here to preserve parallelism when usingPrismaClientwhile gracefully degrading to serial execution when passed aTransactionClient, or document at each Promise.all site that parallelism only applies with a top-level client.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/services/vote-nullification.service.ts` around lines 201 - 275, nullifyUserVotes currently uses Promise.all for parallel findMany/update/recalc calls which is safe with a top-level PrismaClient but will silently serialize when given a Prisma TransactionClient; detect transaction context (e.g. if ('$transaction' in prisma) or similar check used in TrustService) and in that case avoid concurrent Promise.all batches—run the batches sequentially (await each call in order) to prevent long-running interactive-transaction timeouts; apply the same pattern to the Promise.all sites in nullifyUserVotes (the initial findMany block, the batchUpdateNullifiedAt block, and the recalculateListingScores/recalculateCommentScores block) and likewise update restoreUserVotes equivalents so behavior is parallel for PrismaClient and serial for TransactionClient.src/app/listings/new/NewListingPage.tsx (1)
125-137:⚠️ Potential issue | 🟡 MinorEden driver reconciliation runs for non-Eden emulators.
The reconciliation block is keyed only by
f.name === 'dynamic_driver_version'. The newgamenativeCustomFieldsSeederdefines a field with the same name, so on a GameNative import this block will pass the GameNative driver string throughreconcileDriverValueagainst Eden's release list. In the common case it’s a no-op, but if a GameNative value coincidentally matches an Eden filename it will be silently rewritten to the Eden canonical option value, which isn’t a valid GameNative selection.Consider gating this on
selectedEmulatorSlug === 'eden'(or moving it into the Eden-specific path).🛡️ Suggested guard
- // Reconcile Eden driver filename (if present) to canonical driver option value - { + // Reconcile Eden driver filename (if present) to canonical driver option value + if (selectedEmulatorSlug === 'eden') { const driverField = parsedCustomFields.find((f) => f.name === 'dynamic_driver_version') const releases = driverVersionsQuery.data?.releases if (driverField && releases) { const current = valueMap.get(driverField.id) const canonical = reconcileDriverValue( typeof current === 'string' ? current : String(current ?? ''), releases, ) if (canonical) valueMap.set(driverField.id, canonical) } }Remember to add
selectedEmulatorSlugto thehandleImportResultdependency list.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/listings/new/NewListingPage.tsx` around lines 125 - 137, The Eden-specific reconciliation currently runs for any import because it only checks parsedCustomFields for a field named 'dynamic_driver_version'; restrict this by wrapping the block (the lookup using parsedCustomFields.find, driverField, reconcileDriverValue, and valueMap.set) in a guard that checks selectedEmulatorSlug === 'eden' so only Eden imports are reconciled, and add selectedEmulatorSlug to the handleImportResult dependency list to avoid stale closures.
🟡 Minor comments (17)
tests/error-handling.spec.ts-114-127 (1)
114-127:⚠️ Potential issue | 🟡 Minor
jsErrorsis collected but never asserted — test doesn't actually verify resilience to console errors.The
pageerrorlistener pushes intojsErrors, but the array is never read. As written, this is equivalent to "navigate to games, then home" and the test name is misleading. Either assert onjsErrors(e.g.,expect(jsErrors).toEqual([])or filter out known-benign ones) or remove the listener and rename.🐛 Proposed fix
test('should remain functional even if console errors occur', async ({ page }) => { const jsErrors: string[] = [] page.on('pageerror', (error) => { jsErrors.push(error.message) }) const gamesPage = new GamesPage(page) await gamesPage.goto() await gamesPage.verifyPageLoaded() await gamesPage.navigateToHome() await expect(page).toHaveURL('/') + + // Surface unexpected runtime errors that would otherwise be silently swallowed + expect(jsErrors, `Unexpected page errors: ${jsErrors.join('\n')}`).toEqual([]) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/error-handling.spec.ts` around lines 114 - 127, The test sets up jsErrors and a page.on('pageerror') listener but never asserts on jsErrors; update the test to either assert that jsErrors is empty (e.g., expect(jsErrors).toEqual([]) or expect(jsErrors.length).toBe(0)) after the navigation steps, or filter out known benign messages before asserting, so the test backing the name "should remain functional even if console errors occur" actually verifies resilience; locate the jsErrors array and the page.on('pageerror') listener near GamesPage.goto(), GamesPage.verifyPageLoaded(), and gamesPage.navigateToHome() to add the assertion or remove the listener and rename the test accordingly.tests/custom-fields.spec.ts-46-53 (1)
46-53:⚠️ Potential issue | 🟡 Minor
page.locator('h3')is too broad as a "template cards" locator.Any
h3on the page (modal headings, sidebar sections, etc.) will satisfytemplateCards.first().or(emptyState), so the empty-state branch is effectively unreachable and the test no longer meaningfully distinguishes the two states. Consider a scoped selector such as a test id on template cards or a container-scoped query (e.g.,page.locator('[data-testid="template-card"] h3')).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/custom-fields.spec.ts` around lines 46 - 53, The locator page.locator('h3') in the test 'should show template cards or empty state in the list' is too broad and can match unrelated headings (making emptyState unreachable); update the test to use a scoped selector that targets template cards specifically (e.g., use a data-testid or container-scoped query such as page.locator('[data-testid="template-card"] h3') or page.locator('.template-card h3') instead of page.locator('h3')), keep the emptyState = page.getByText(/no custom field templates/i) and the final visibility assertion (templateCards.first().or(emptyState)) so the test correctly distinguishes between cards and the empty state.tests/pages/AuthPage.ts-15-17 (1)
15-17:⚠️ Potential issue | 🟡 Minor
verifyUserNotAuthenticatedcan false-positive on footer "Sign in" text.
this.page.getByText(/sign in/i).first()matches any "sign in" text anywhere on the page, including footer links, marketing copy, or inline "Sign in to comment" prompts — all of which can be visible even when the user is authenticated on some layouts. Relying onsignInButton(the navbar control) alone is a stronger signal; the.or(...)fallback weakens the guarantee this helper is intended to provide.Suggested tightening
- await expect(this.signInButton.or(this.page.getByText(/sign in/i).first())).toBeVisible() + await expect(this.signInButton).toBeVisible()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/pages/AuthPage.ts` around lines 15 - 17, The helper verifyUserNotAuthenticated currently uses a broad fallback this.page.getByText(/sign in/i).first() which can match unrelated "Sign in" text (e.g., footer), causing false positives; tighten the check by removing the .or(...) fallback and rely on the navbar-specific locator this.signInButton only, or if a fallback is still desired scope it to the navbar (e.g., use the nav locator like nav.getByText(...) or the existing navbar locator) so verifyUserNotAuthenticated only asserts visibility of the actual navbar sign-in control (referencing verifyUserNotAuthenticated and this.signInButton).tests/accessibility.spec.ts-28-52 (1)
28-52:⚠️ Potential issue | 🟡 MinorDon’t sample only the first few interactive elements.
Lines 28-52 and 265-277 can miss unlabeled buttons/links later in the DOM, so the accessibility tests may pass while regressions remain on the page. Since these are already bulk
evaluateAllcalls, check the full collection or filter to visible elements instead of usingslice(...).♻️ Proposed fix
- elements.slice(0, 5).map((el) => ({ + elements.map((el) => ({ ariaLabel: el.getAttribute('aria-label'), text: el.textContent, title: el.getAttribute('title'), })), @@ - elements.slice(0, 5).map((el) => ({ + elements.map((el) => ({ text: el.textContent, ariaLabel: el.getAttribute('aria-label'), })), @@ - elements.slice(0, 10).map((el) => ({ + elements.map((el) => ({ text: el.textContent?.trim() ?? '', ariaLabel: el.getAttribute('aria-label'), title: el.getAttribute('title'), })),Also applies to: 265-277
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/accessibility.spec.ts` around lines 28 - 52, The test currently samples only the first five elements by using slice(...) inside the page.locator(...).evaluateAll calls (producing buttonData and linkData), which can miss unlabeled interactive elements later in the DOM; update both evaluateAll invocations to operate over the full collection (remove slice) and/or filter to visible elements (e.g., check element.offsetParent or getClientRects().length inside the evaluator) before mapping so the assertions on buttonData and linkData cover all relevant/visible buttons and links rather than just the first five.tests/accessibility.spec.ts-153-171 (1)
153-171:⚠️ Potential issue | 🟡 MinorDon’t treat placeholders as form labels.
Line 170 allows inputs with only a placeholder to pass the “proper form labels” test. That weakens the accessibility coverage because placeholders disappear during input and are not a reliable label replacement.
♻️ Proposed fix
for (const input of inputAccessibility) { const hasAccessibleName = input.hasLabel || input.ariaLabel || input.ariaLabelledBy - expect(hasAccessibleName || !!input.placeholder).toBe(true) + expect(hasAccessibleName).toBe(true) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/accessibility.spec.ts` around lines 153 - 171, The test currently treats placeholder as an acceptable accessible name for inputs (computed in inputAccessibility and asserted in the loop), which is incorrect; update the assertion so that only a visible/associated label or ARIA labeling passes: compute hasAccessibleName using input.hasLabel || input.ariaLabel || input.ariaLabelledBy (keep the existing properties) and remove the placeholder fallback from the expectation so the test requires hasAccessibleName to be truthy for each input; update the for loop assertion around inputAccessibility to expect(hasAccessibleName).toBe(true) (no placeholder check).tests/admin-reports.spec.ts-23-25 (1)
23-25:⚠️ Potential issue | 🟡 MinorMake the table-row assertion meaningful.
Line 25 always passes because
count()cannot be negative. If the seeded reports fail to render, this test still passes while later tests fail elsewhere.💚 Proposed fix
const reportRows = reportsTable.locator('tbody tr') const rowCount = await reportRows.count() - expect(rowCount).toBeGreaterThanOrEqual(0) + expect(rowCount).toBeGreaterThan(0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/admin-reports.spec.ts` around lines 23 - 25, The current assertion expects rowCount to be >= 0 which always passes; change it to assert a meaningful minimum (e.g., expect(rowCount).toBeGreaterThan(0)) or compare against the known seeded report count variable (if available) so failures surface when reports don't render; update the assertion that uses reportRows, rowCount and reportsTable in the test to use the stricter check or the seededReports.length reference.tests/auth.spec.ts-129-132 (1)
129-132:⚠️ Potential issue | 🟡 MinorAssert the actual back-navigation target.
Line 131 passes for any local route, including staying on
/games/new, so this test can miss a brokengoBack()flow. Since the previous page is/games, assert that route explicitly.💚 Proposed fix
await page.goto('/games/new') await page.goBack() - await expect(page).toHaveURL(/localhost|127\.0\.0\.1/) + await expect(page).toHaveURL(/\/games(?:[?#].*)?$/) await authPage.verifyUserNotAuthenticated()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/auth.spec.ts` around lines 129 - 132, The test currently uses a loose URL assertion after page.goBack() (expect(page).toHaveURL(/localhost|127\.0\.0\.1/)) which will pass for any local route; change it to assert the exact back-navigation target (/games) so the back flow is validated. Locate the sequence using page.goto('/games/new'), page.goBack(), expect(page).toHaveURL(...), and authPage.verifyUserNotAuthenticated() and replace the broad regex with an assertion that the page URL is the /games route (e.g., check for '/games' explicitly or a regex that anchors to the /games path).tests/helpers/data-factory.ts-191-194 (1)
191-194:⚠️ Potential issue | 🟡 MinorPC listing submit assertion can mask a failed submission.
await expect(page).toHaveURL(/\/pc-listings/)matches/pc-listings/newas well. If the submit silently fails (validation error, network, etc.) the page stays on/pc-listings/newand this assertion still passes, leaving the test suite to fail later with a misleading "listing not found" style error. The handheld helper at line 159 correctly uses a negative lookahead — please mirror that here.🛠️ Proposed fix
- await expect(page).toHaveURL(/\/pc-listings/) + await expect(page).toHaveURL(/\/pc-listings(?!\/new)/)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/helpers/data-factory.ts` around lines 191 - 194, The current submit assertion uses expect(page).toHaveURL(/\/pc-listings/) which also matches /pc-listings/new and can hide a failed submission; update the assertion that follows page.getByRole('button', { name: /create compatibility report/i }).click() to use a regex that excludes the "new" path (e.g., a negative lookahead or an exact-match pattern) so the test only passes when navigation reaches the actual listings page and not the form at /pc-listings/new.tests/helpers/data-factory.ts-332-372 (1)
332-372:⚠️ Potential issue | 🟡 MinorTrust-score parsing assumes the label contains only digits.
parseInt((await scoreElement.textContent()) ?? '0', 10)on line 347 will returnNaNif the element ever contains anything other than a leading digit (e.g."Score: 500","1,000","+500").NaN > 0is false so thewhileloop is skipped and the helper silently returns without resetting the score, and the final assertion on line 371 (toHaveText('0')) would then fail with a confusing message. Consider validating the parsed value and/or extracting the numeric run explicitly.Additionally, the batching loop at lines 355-364 awaits only the toast
trust score adjusted; if the same toast lingers from the previous batch, the assertion could pass before the new adjustment is actually applied. Waiting for the toast to be hidden between iterations (or for a fresh toast instance) would make the loop deterministic.🛡️ Sketch
- const currentScore = parseInt((await scoreElement.textContent()) ?? '0', 10) + const rawScore = (await scoreElement.textContent())?.replace(/[^\d-]/g, '') ?? '0' + const currentScore = Number.parseInt(rawScore, 10) + if (!Number.isFinite(currentScore)) { + throw new Error(`resetUserTrustScore: unexpected score text "${rawScore}"`) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/helpers/data-factory.ts` around lines 332 - 372, The helper resetUserTrustScore wrongly assumes scoreElement.textContent() is a plain integer and may parse NaN; extract the numeric value from dialog.getByLabel('Trust score value') using a regex that strips non-digit characters (handle commas and optional +/−), parseInt the clean string and validate (throw or fail the test if NaN) before using currentScore/remaining; also make the batch loop deterministic by, after clicking Apply and seeing the "trust score adjusted" toast, wait for that toast to be hidden (or wait for a new toast instance) before continuing to the next iteration so each batch is applied sequentially and awaited.src/server/repositories/pc-listings.repository.ts-980-988 (1)
980-988:⚠️ Potential issue | 🟡 MinorUnbounded
findManyon votes.For listings with very high vote counts this loads every vote row (with a user join) into memory on every moderator panel open. Consider capping with
take(e.g. most recent N) and/or exposing pagination; the counts already come fromcomputeVoteCountsso trimming the row list doesn't affect displayed totals.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/repositories/pc-listings.repository.ts` around lines 980 - 988, The unbounded prisma.pcListingVote.findMany call inside PcListingsRepository (wrapped by this.handleDatabaseOperation and using PcListingsRepository.moderatorInfoVoteSelect) can blow memory for high-vote listings; limit the result set by adding a take (e.g. take: 100) and optionally skip/page parameters (or accept a pageSize argument) and wire those through the calling moderator endpoint, while leaving computeVoteCounts to provide totals; ensure the change is applied where pcListingId votes are fetched so pagination parameters can be passed through and default to a safe cap if none provided.src/server/utils/spam-check.ts-25-30 (1)
25-30:⚠️ Potential issue | 🟡 Minor
entityId: params.userIdmisrepresents the analytics event.The spam check fires before any listing/comment row exists, so there's no real
entityIdto report — but stuffinguserIdinto theentityIdfield will corrupt downstream analytics (joins, funnels, per-entity aggregations will treat users as entities). Consider either omittingentityId(passundefined/nullif the schema allows), using a sentinel like'pre-create', or adding an explicituserIdfield to the event schema.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/utils/spam-check.ts` around lines 25 - 30, The analytics event currently misuses params.userId as entityId in the analytics.contentQuality.spamDetected call; update the invocation so it does not populate entityId with the user id (set entityId to undefined/null or a clear sentinel like "pre-create") and instead include the user id in a dedicated field (e.g., userId: params.userId) so downstream joins/aggregations aren’t corrupted—change the call site in spam-check.ts where analytics.contentQuality.spamDetected(...) is invoked to pass entityId as null/undefined/"pre-create" and add userId: params.userId.src/app/listings/components/shared/details/ModeratorInfoPanel.tsx-28-35 (1)
28-35:⚠️ Potential issue | 🟡 MinorExpose the panel state to assistive tech.
The toggle visually expands/collapses content, but the button does not announce that state. Add
aria-expanded; addaria-controlstoo if you assign the panel body an id.♿ Proposed fix
<button type="button" + aria-expanded={isExpanded} onClick={() => setIsExpanded((prevValue) => !prevValue)} className="w-full flex items-center gap-2 px-4 py-3 text-sm font-semibold text-amber-800 dark:text-amber-200 hover:bg-amber-100/50 dark:hover:bg-amber-900/30 transition-colors" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/listings/components/shared/details/ModeratorInfoPanel.tsx` around lines 28 - 35, The toggle button in ModeratorInfoPanel.tsx should expose its state to assistive tech: wire aria-expanded to the isExpanded state used by setIsExpanded (e.g., aria-expanded={isExpanded}) and add aria-controls pointing to the panel body’s id; assign an id (like "moderator-info-panel" or a generated unique id) to the panel content element and ensure that element uses that same id so the button’s aria-controls references it (optionally also reflect hidden state on the panel with aria-hidden when collapsed).src/server/api/routers/pcListings.test.ts-17-21 (1)
17-21:⚠️ Potential issue | 🟡 MinorStale mock export —
handleVoteTrustEffectswas removed.Per
vote-trust-effects.ts, the oldhandleVoteTrustEffectshelper was replaced by the listing/comment-specific variants and no longer exists. The extrahandleVoteTrustEffects: vi.fn()on line 20 is dead weight and can mislead future readers. Safe to drop.🧹 Proposed cleanup
vi.mock('@/server/utils/vote-trust-effects', () => ({ handleListingVoteTrustEffects: (...args: unknown[]) => mockHandleListingVoteTrustEffects(...args), handleCommentVoteTrustEffects: (...args: unknown[]) => mockHandleCommentVoteTrustEffects(...args), - handleVoteTrustEffects: vi.fn(), }))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/api/routers/pcListings.test.ts` around lines 17 - 21, The test mock exports an obsolete symbol handleVoteTrustEffects; remove the dead export from the vi.mock call so only the current handlers are exported—keep handleListingVoteTrustEffects and handleCommentVoteTrustEffects mapped to the mocks (mockHandleListingVoteTrustEffects/mockHandleCommentVoteTrustEffects) and delete the handleVoteTrustEffects: vi.fn() entry to avoid misleading readers and stale API surface in src/server/api/routers/pcListings.test.ts.src/server/utils/emulator-config/gamenative/gamenative.converter.ts-244-252 (1)
244-252:⚠️ Potential issue | 🟡 Minor
dxvk_version='other'will serialize as a literalversion=other.The seeder defines
{ value: 'other', label: 'Other (specify in notes)' }fordxvk_version. When a user picks that, this merge step will appendversion=otherintodxwrapperConfig, which GameNative won't recognize as an actual version and which downstream parsers (including the re-importer inparser.ts) will round-trip as the string'other'. Consider skipping the merge (or falling back toDEFAULT_DXVK_VERSION) when the value is the'other'sentinel.🛠️ Suggested guard
- const dxvkVersion = fieldValuesByName.get('dxvk_version') - if (dxvkVersion && String(dxvkVersion).trim()) { + const dxvkVersion = fieldValuesByName.get('dxvk_version') + const dxvkVersionStr = dxvkVersion ? String(dxvkVersion).trim() : '' + if (dxvkVersionStr && dxvkVersionStr.toLowerCase() !== 'other') { config.dxwrapperConfig = appendToConfigString( config.dxwrapperConfig, 'version', - String(dxvkVersion).trim(), + dxvkVersionStr, ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/utils/emulator-config/gamenative/gamenative.converter.ts` around lines 244 - 252, The merge step currently appends dxvkVersion verbatim (dxvkVersion, via appendToConfigString into config.dxwrapperConfig), causing the sentinel 'other' to become an invalid literal; update the logic in gamenative.converter.ts to detect the sentinel (e.g., dxvkVersion === 'other' or equals the seeder's sentinel) and instead skip appending or substitute DEFAULT_DXVK_VERSION before calling appendToConfigString so downstream parsers (parser.ts) don't receive the literal 'other'.prisma/seeders/gamenativeCustomFieldsSeeder.ts-37-47 (1)
37-47:⚠️ Potential issue | 🟡 MinorWrapper option values are inconsistent and should be standardized to lowercase.
The seeder uses
'Wrapper'(capitalized, no kebab-case) while the variants use'Wrapper-v2','Wrapper-leegao','Wrapper-legacy'(capitalized with kebab-case). TheGRAPHICS_DRIVER_MAPPINGhandles both'Wrapper'and lowercase'wrapper'(and all variants), anddetectGraphicsDriverperforms case-insensitive matching, so this inconsistency is functionally benign.However, the
GraphicsDrivertype definition (gamenative.types.ts, lines 22–31) uses only lowercase kebab-case as the source of truth. For consistency and cleaner reverse-mapping, standardize all wrapper options in the seeder to lowercase:'wrapper','wrapper-v2','wrapper-leegao','wrapper-legacy'. This aligns with the type system and avoids unnecessary mapping indirection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@prisma/seeders/gamenativeCustomFieldsSeeder.ts` around lines 37 - 47, The wrapper option values in the seeder are inconsistent with the canonical GraphicsDriver type; update the options array entries currently using 'Wrapper', 'Wrapper-v2', 'Wrapper-leegao', 'Wrapper-legacy' to use lowercase kebab-case 'wrapper', 'wrapper-v2', 'wrapper-leegao', 'wrapper-legacy' so they match the GraphicsDriver definition and avoid extra mapping via GRAPHICS_DRIVER_MAPPING/detectGraphicsDriver.src/server/utils/emulator-config/gamenative/gamenative.converter.test.ts-263-284 (1)
263-284:⚠️ Potential issue | 🟡 MinorSet the bionic container in this bionic-specific test.
As written, this only proves these Box64 versions are accepted under the default container variant. Add
container_variant: 'bionic'to the fixture, or rename the test to avoid false coverage.💚 Proposed test adjustment
customFieldValues: [ + { + customFieldDefinition: { + name: 'container_variant', + label: 'Container Variant', + type: 'SELECT', + }, + value: 'bionic', + }, { customFieldDefinition: { name: 'box64_version', label: 'Box64 Version', type: 'SELECT',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/utils/emulator-config/gamenative/gamenative.converter.test.ts` around lines 263 - 284, The test named "should accept bionic-specific Box64 versions" is missing the bionic container variant and thus doesn't prove bionic-specific behavior; update the fixture passed to convertToGameNativeConfig in that test to include container_variant: 'bionic' (or alternatively rename the test if you intend to test default container behavior) so the assertion on config.box64Version actually exercises the bionic branch—locate the call to convertToGameNativeConfig in the test and add container_variant: 'bionic' inside the object passed to it.src/server/utils/emulator-config/gamenative/gamenative.types.ts-33-40 (1)
33-40:⚠️ Potential issue | 🟡 MinorAlign the SD 8 Elite version key with the driver literal.
GraphicsDriveruses'sd-8-elite', butGraphicsDriverVersionsusessd8elite, forcing theGraphicsDriverVersionForconditional type to special-case the mapping. Rename the key to'sd-8-elite'to align the types.Proposed fix
export type GraphicsDriverVersions = { turnip: '25.1.0' | '25.2.0' | '25.3.0' | '25.0.0' | '24.1.0' virgl: '23.1.9' vortek: '2.1' zink: '22.2.5' adreno: '819.2' | '805' - sd8elite: '800.51' | '2-842.6' + 'sd-8-elite': '800.51' | '2-842.6' }: T extends 'sd-8-elite' - ? GraphicsDriverVersions['sd8elite'] + ? GraphicsDriverVersions['sd-8-elite'] : string🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/utils/emulator-config/gamenative/gamenative.types.ts` around lines 33 - 40, The type key in GraphicsDriverVersions must match the driver literal used by GraphicsDriver so change the sd8elite property name to the exact literal 'sd-8-elite' (i.e., replace the identifier sd8elite with the string key 'sd-8-elite') and keep its version union unchanged; then remove any special-case handling in the conditional type GraphicsDriverVersionFor that was added to map sd8elite to 'sd-8-elite' so all driver-to-version lookups use the unified GraphicsDriverVersions and GraphicsDriver names consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4caaaf98-fa95-4c47-ab02-b4d2f696e019
📒 Files selected for processing (126)
playwright.config.tsprisma/migrations/20260414175740_add_vote_change_reversal_trust_action/migration.sqlprisma/schema.prismaprisma/seed.tsprisma/seeders/gamenativeCustomFieldsSeeder.tssrc/app/admin/users/components/UserDetailsModal.tsxsrc/app/admin/users/components/UserRoleModal.tsxsrc/app/listings/ListingsPage.tsxsrc/app/listings/[id]/components/ListingDetailsClient.tsxsrc/app/listings/components/shared/details/AdminUserLink.tsxsrc/app/listings/components/shared/details/ApprovalSection.tsxsrc/app/listings/components/shared/details/ModeratorInfoPanel.tsxsrc/app/listings/components/shared/details/VoteDirectionIcon.test.tsxsrc/app/listings/components/shared/details/VoteDirectionIcon.tsxsrc/app/listings/components/shared/details/VoteRow.test.tsxsrc/app/listings/components/shared/details/VoteRow.tsxsrc/app/listings/components/shared/details/VotesSection.tsxsrc/app/listings/components/shared/details/utils/logVoteError.test.tssrc/app/listings/components/shared/details/utils/logVoteError.tssrc/app/listings/components/shared/details/utils/refreshListingDetail.test.tssrc/app/listings/components/shared/details/utils/refreshListingDetail.tssrc/app/listings/new/NewListingPage.tsxsrc/app/pc-listings/PcListingsPage.tsxsrc/app/pc-listings/[id]/components/PcListingDetailsClient.tsxsrc/app/users/[id]/UserProfilePage.tsxsrc/components/listings/AuthorDisplay.tsxsrc/components/ui/Badge.tsxsrc/components/ui/BannedUserBadge.test.tsxsrc/components/ui/BannedUserBadge.tsxsrc/components/ui/TrustLevelBadge.test.tsxsrc/components/ui/TrustLevelBadge.tsxsrc/components/ui/VoteButtons.tsxsrc/components/ui/index.tssrc/components/ui/modals/Modal.tsxsrc/lib/errors.tssrc/lib/trust/config.tssrc/lib/trust/service.test.tssrc/lib/trust/service.tssrc/schemas/listing.tssrc/schemas/pcListing.tssrc/server/api/routers/listings.tssrc/server/api/routers/listings/admin.tssrc/server/api/routers/listings/comments.test.tssrc/server/api/routers/listings/comments.tssrc/server/api/routers/listings/core.test.tssrc/server/api/routers/listings/core.tssrc/server/api/routers/mobile/listings.test.tssrc/server/api/routers/mobile/listings.tssrc/server/api/routers/mobile/pcListings.tssrc/server/api/routers/pcListings.test.tssrc/server/api/routers/pcListings.tssrc/server/api/routers/trust.test.tssrc/server/api/routers/trust.tssrc/server/api/routers/userBans.tssrc/server/api/utils/listingHelpers.test.tssrc/server/api/utils/listingHelpers.tssrc/server/repositories/listings.repository.tssrc/server/repositories/pc-listings.repository.tssrc/server/services/audit.service.tssrc/server/services/vote-nullification.service.test.tssrc/server/services/vote-nullification.service.tssrc/server/utils/emulator-config/gamenative/gamenative.converter.test.tssrc/server/utils/emulator-config/gamenative/gamenative.converter.tssrc/server/utils/emulator-config/gamenative/gamenative.defaults.tssrc/server/utils/emulator-config/gamenative/gamenative.types.tssrc/server/utils/moderator-info.test.tssrc/server/utils/moderator-info.tssrc/server/utils/spam-check.test.tssrc/server/utils/spam-check.tssrc/server/utils/spamDetection.tssrc/server/utils/vote-trust-effects.test.tssrc/server/utils/vote-trust-effects.tssrc/shared/emulator-config/gamenative/index.tssrc/shared/emulator-config/gamenative/mapping.tssrc/shared/emulator-config/gamenative/parser.test.tssrc/shared/emulator-config/gamenative/parser.tssrc/test/setup.tssrc/utils/user-bans.test.tssrc/utils/user-bans.tstests/accessibility.spec.tstests/admin-dashboard.spec.tstests/admin-permissions.spec.tstests/admin-reports.spec.tstests/admin-users.spec.tstests/android-downloads.spec.tstests/auth.spec.tstests/badge-system.spec.tstests/browsing.spec.tstests/commenting.spec.tstests/custom-fields.spec.tstests/data-setup.spec.tstests/error-handling.spec.tstests/filtering.spec.tstests/fixtures/test-fixtures.tstests/forms.spec.tstests/full-listing-flow.spec.tstests/game-management.spec.tstests/helpers/cookie-helper.tstests/helpers/data-factory.tstests/helpers/navigation.tstests/helpers/test-config.tstests/igdb-search.spec.tstests/listing-approval.spec.tstests/listings-success-rate-sorting.spec.tstests/navigation.spec.tstests/notification-system.spec.tstests/pages/AuthPage.tstests/pages/BasePage.tstests/pages/CookieBanner.tstests/pages/GameFormPage.tstests/pages/GamesPage.tstests/pages/HomePage.tstests/pages/ListingFormPage.tstests/pages/ListingsPage.tstests/pagination.spec.tstests/pc-listings.spec.tstests/pc-voting.spec.tstests/performance.spec.tstests/router-coverage.spec.tstests/search.spec.tstests/trust-integration.spec.tstests/trust-system.spec.tstests/user-flows.spec.tstests/user-moderation.spec.tstests/validate-tests.spec.tstests/voting.spec.ts
💤 Files with no reviewable changes (7)
- tests/android-downloads.spec.ts
- tests/helpers/cookie-helper.ts
- tests/pages/ListingFormPage.ts
- tests/pages/HomePage.ts
- tests/fixtures/test-fixtures.ts
- tests/helpers/navigation.ts
- tests/pages/CookieBanner.ts
| await applyTrustAction({ | ||
| userId: ctx.session.user.id, | ||
| action: TrustAction.LISTING_CREATED, | ||
| context: { pcListingId: created.id }, | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C3 'applyTrustAction\s*\(' --type=ts
rg -nP -C5 'LISTING_CREATED' --type=tsRepository: Producdevity/EmuReady
Length of output: 17629
🏁 Script executed:
#!/bin/bash
# First, let's see the full context of the mobile create method including repository.create
sed -n '140,190p' src/server/api/routers/mobile/pcListings.ts
echo "---"
# Check the web version for comparison
sed -n '250,300p' src/server/api/routers/pcListings.tsRepository: Producdevity/EmuReady
Length of output: 3610
🏁 Script executed:
#!/bin/bash
# Let's look at the repository.create implementation to understand transaction handling
fd -t f 'repository|Repository' --type f | head -20
rg -l 'class.*Repository|export.*create.*repository' --type ts | head -10Repository: Producdevity/EmuReady
Length of output: 1554
🏁 Script executed:
#!/bin/bash
# Check if there's try/catch wrapping around applyTrustAction calls
rg -B5 -A5 'applyTrustAction' src/server/api/routers/mobile/pcListings.ts | head -40
echo "---"
# Also check the web version
rg -B5 -A5 'applyTrustAction' src/server/api/routers/pcListings.ts | head -40Repository: Producdevity/EmuReady
Length of output: 1893
🏁 Script executed:
#!/bin/bash
# Get the create method implementation in the repository
rg -A 50 'create\s*\(' src/server/repositories/pc-listings.repository.ts | head -80Repository: Producdevity/EmuReady
Length of output: 2474
🏁 Script executed:
#!/bin/bash
# Check the applyTrustAction function for error handling
sed -n '32,120p' src/lib/trust/service.tsRepository: Producdevity/EmuReady
Length of output: 2639
🏁 Script executed:
#!/bin/bash
# Let's check if there's a try/catch or error handling around the applyTrustAction call in the mobile create
sed -n '150,185p' src/server/api/routers/mobile/pcListings.tsRepository: Producdevity/EmuReady
Length of output: 1320
🏁 Script executed:
#!/bin/bash
# Check the repository.create to see if it wraps the entire operation in a transaction
rg -B 10 'async create' src/server/repositories/pc-listings.repository.ts | head -30Repository: Producdevity/EmuReady
Length of output: 268
🏁 Script executed:
#!/bin/bash
# Look for the full create method including the transaction wrapper if any
wc -l src/server/repositories/pc-listings.repository.tsRepository: Producdevity/EmuReady
Length of output: 119
🏁 Script executed:
#!/bin/bash
# Get the full create method from the repository (starts around line 125)
sed -n '125,250p' src/server/repositories/pc-listings.repository.tsRepository: Producdevity/EmuReady
Length of output: 4277
🏁 Script executed:
#!/bin/bash
# Check if there's any transaction wrapper or error handling in the create mutation
rg -B 5 -A 30 'create: mobileProtectedProcedure' src/server/api/routers/mobile/pcListings.tsRepository: Producdevity/EmuReady
Length of output: 1280
🏁 Script executed:
#!/bin/bash
# Check the full repository.create implementation to see if there's transaction handling
sed -n '130,280p' src/server/repositories/pc-listings.repository.tsRepository: Producdevity/EmuReady
Length of output: 5152
🏁 Script executed:
#!/bin/bash
# Look for PR description or comments mentioning transactions
rg -i 'transaction|runs trust effects' --type=ts --type=md | head -20Repository: Producdevity/EmuReady
Length of output: 1834
🏁 Script executed:
#!/bin/bash
# Get the actual create method end to see if there's transaction handling
sed -n '130,220p' src/server/repositories/pc-listings.repository.tsRepository: Producdevity/EmuReady
Length of output: 2785
🏁 Script executed:
#!/bin/bash
# Check if there's a try-catch wrapper anywhere in the mutations
rg -B 2 -A 15 'applyTrustAction' src/server/api/routers/mobile/pcListings.ts | grep -A 15 'create:'Repository: Producdevity/EmuReady
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Search more specifically for the create method that takes the input parameter
rg -A 100 'async create\(input:' src/server/repositories/pc-listings.repository.ts | head -150Repository: Producdevity/EmuReady
Length of output: 3438
🏁 Script executed:
#!/bin/bash
# Check if repository.create is wrapped in a transaction
rg 'prisma.\$transaction|this.prisma.\$transaction' src/server/repositories/pc-listings.repository.tsRepository: Producdevity/EmuReady
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Let's see the complete create method end to understand the full scope
sed -n '130,250p' src/server/repositories/pc-listings.repository.ts | tail -100Repository: Producdevity/EmuReady
Length of output: 3469
🏁 Script executed:
#!/bin/bash
# Check if there's any error handling pattern in other mutations in the same file
rg -B 3 -A 10 'try\s*{' src/server/api/routers/mobile/pcListings.ts | head -30Repository: Producdevity/EmuReady
Length of output: 142
🏁 Script executed:
#!/bin/bash
# Check for try-catch error handling in the mobile create mutation
rg -B 3 -A 10 'try {' src/server/api/routers/mobile/pcListings.ts | head -50Repository: Producdevity/EmuReady
Length of output: 138
🏁 Script executed:
#!/bin/bash
# Check the web version for comparison - does it have error handling?
rg -B 3 -A 10 'try {' src/server/api/routers/pcListings.ts | head -50Repository: Producdevity/EmuReady
Length of output: 138
🏁 Script executed:
#!/antml/bash
# Get the remaining part of the repository create method
sed -n '195,230p' src/server/repositories/pc-listings.repository.tsRepository: Producdevity/EmuReady
Length of output: 1402
🏁 Script executed:
#!/bin/bash
# Use grep instead for try-catch pattern
grep -n 'catch' src/server/api/routers/mobile/pcListings.ts | head -10Repository: Producdevity/EmuReady
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Get the end of the create method in the repository
sed -n '200,235p' src/server/repositories/pc-listings.repository.tsRepository: Producdevity/EmuReady
Length of output: 1404
🏁 Script executed:
#!/bin/bash
# Check if any create mutations have try-catch
grep -n 'catch' src/server/api/routers/pcListings.ts | head -10Repository: Producdevity/EmuReady
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Look at the full create method without line limit issues
awk '/async create\(input:/{flag=1} flag{print} /^ \}$/{if(flag) {count++; if(count==1) exit}}' src/server/repositories/pc-listings.repository.ts | head -200Repository: Producdevity/EmuReady
Length of output: 3803
🏁 Script executed:
#!/bin/bash
# Search for catch keyword differently
grep -c 'catch' src/server/api/routers/mobile/pcListings.ts
grep -c 'catch' src/server/api/routers/pcListings.tsRepository: Producdevity/EmuReady
Length of output: 68
🏁 Script executed:
#!/bin/bash
# Check the listings/core.ts to see if web listing creation has any error handling
sed -n '150,220p' src/server/api/routers/listings/core.tsRepository: Producdevity/EmuReady
Length of output: 2553
🏁 Script executed:
#!/bin/bash
# Let's verify the listings/core.ts repository implementation for comparison
rg -A 80 'async create\(input:' src/server/repositories/listings.repository.ts | head -150Repository: Producdevity/EmuReady
Length of output: 2723
Trust action runs outside any transaction and lacks error handling.
applyTrustAction is invoked after repository.create returns. If the trust call throws, the listing is already persisted but the mutation rejects, leaving the listing without its associated trust event and returning an error to the client.
Note: While the web listing repository wraps creation in a transaction, applyTrustAction is still invoked outside that transaction in both implementations, creating the same risk. Recommend either:
- Moving the trust call into the same transaction as the listing insert (via the repository), or
- Wrapping it in a
try/catchwith structured logging so trust failures don't fail the listing creation flow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/api/routers/mobile/pcListings.ts` around lines 170 - 174, The
trust event call applyTrustAction is executed after repository.create and can
throw, causing the whole mutation to fail even though the listing was persisted;
either move the trust invocation into the same DB transaction inside the
repository.create flow (add code in the listing repository to call
applyTrustAction/record the trust event as part of its transaction) or wrap
applyTrustAction(...) here in a try/catch that logs structured errors (include
userId, TrustAction.LISTING_CREATED and created.id) and does not re-throw so the
listing creation succeeds even if the trust step fails.
| if (input.recaptchaToken) { | ||
| const clientIP = ctx.headers ? getClientIP(ctx.headers) : undefined | ||
| const captchaResult = await verifyRecaptcha({ | ||
| token: input.recaptchaToken, | ||
| expectedAction: RECAPTCHA_CONFIG.actions.VOTE, | ||
| userIP: clientIP, | ||
| }) | ||
|
|
||
| if (!captchaResult.success) return AppError.captcha(captchaResult.error) | ||
| } |
There was a problem hiding this comment.
Require the captcha token instead of verifying only when present.
Line 965 makes captcha optional: a client can omit recaptchaToken and skip verification entirely. If PC voting is meant to be captcha-protected, fail closed when the token is missing.
🛡️ Proposed fix
- if (input.recaptchaToken) {
- const clientIP = ctx.headers ? getClientIP(ctx.headers) : undefined
- const captchaResult = await verifyRecaptcha({
- token: input.recaptchaToken,
- expectedAction: RECAPTCHA_CONFIG.actions.VOTE,
- userIP: clientIP,
- })
-
- if (!captchaResult.success) return AppError.captcha(captchaResult.error)
- }
+ if (!input.recaptchaToken) {
+ return AppError.captcha('Missing reCAPTCHA token')
+ }
+
+ const clientIP = ctx.headers ? getClientIP(ctx.headers) : undefined
+ const captchaResult = await verifyRecaptcha({
+ token: input.recaptchaToken,
+ expectedAction: RECAPTCHA_CONFIG.actions.VOTE,
+ userIP: clientIP,
+ })
+
+ if (!captchaResult.success) return AppError.captcha(captchaResult.error)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/api/routers/pcListings.ts` around lines 965 - 974, The current
logic in the vote path (pcListings.ts) only verifies captcha when
input.recaptchaToken exists, allowing omission; change it to fail when the token
is missing by checking input.recaptchaToken at the top of the block and
returning AppError.captcha (or a similar error) if absent, then proceed to call
verifyRecaptcha({ token: input.recaptchaToken, expectedAction:
RECAPTCHA_CONFIG.actions.VOTE, userIP: ctx.headers ? getClientIP(ctx.headers) :
undefined }) and keep the existing handling of captchaResult.success (return
AppError.captcha(captchaResult.error) on failure).
| // Fetch existingVote INSIDE the transaction to avoid race conditions between | ||
| // concurrent votes on the same (user, pcListing) pair. | ||
| const voteResult = await ctx.prisma.$transaction(async (tx) => { | ||
| const existingVote = await tx.pcListingVote.findUnique({ | ||
| where: { userId_pcListingId: { userId, pcListingId } }, | ||
| }) | ||
|
|
||
| return vote | ||
| }) | ||
| let result: { | ||
| vote: { userId: string; pcListingId: string; value: boolean } | null | ||
| action: 'created' | 'updated' | 'deleted' | ||
| previousValue: boolean | null | ||
| } | ||
| } else { | ||
| // Create new vote and update counts in transaction | ||
| voteResult = await ctx.prisma.$transaction(async (tx) => { | ||
|
|
||
| if (!existingVote) { | ||
| const vote = await tx.pcListingVote.create({ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether the repository already has a retryable transaction helper
# that supports Prisma isolation options or P2002/P2034 retry handling.
fd -i '^transactions\.ts$' src --exec sed -n '1,240p' {}
rg -n -C3 'P2002|P2034|TransactionIsolationLevel|isolationLevel|withRetryTransaction' src/server src/libRepository: Producdevity/EmuReady
Length of output: 14885
🏁 Script executed:
sed -n '975,1000p' src/server/api/routers/pcListings.tsRepository: Producdevity/EmuReady
Length of output: 1053
🏁 Script executed:
sed -n '1335,1380p' src/server/api/routers/pcListings.tsRepository: Producdevity/EmuReady
Length of output: 1787
Use withRetryTransaction with P2002 handling instead of plain $transaction.
The current code uses ctx.prisma.$transaction without automatic retry logic. Even with the transaction, concurrent requests can both read no existing vote and both attempt to create, hitting a P2002 unique constraint violation. The comment claiming isolation prevents this is incorrect.
Use withRetryTransaction from src/server/utils/transactions.ts, but first update isRetryableError to handle P2002:
function isRetryableError(error: unknown): boolean {
if (error instanceof Error) {
const message = error.message.toLowerCase()
return (
message.includes('deadlock') ||
message.includes('timeout') ||
message.includes('connection') ||
message.includes('transaction') ||
message.includes('p2002') // Add unique constraint violation
)
}
return false
}Then wrap the transaction call with withRetryTransaction and use SERIALIZABLE isolation to fully prevent the race.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/api/routers/pcListings.ts` around lines 982 - 996, Replace the
direct use of ctx.prisma.$transaction in the vote handling block with the
project's withRetryTransaction helper so transient P2002 unique-constraint races
are retried; specifically, call withRetryTransaction(async (tx) => { ... }, {
isolation: 'SERIALIZABLE' }) around the code that uses
tx.pcListingVote.findUnique and tx.pcListingVote.create/update/delete, and
update the isRetryableError helper to treat P2002 (unique constraint) as
retryable by checking for "p2002" in the error message so the retry wrapper will
re-run transactions that fail due to concurrent creates.
| beforeEach(() => { | ||
| prisma = createMockPrisma() | ||
| mockApplyManualTrustAdjustment.mockResolvedValue(undefined) | ||
| mockApplyManualAdjustment.mockResolvedValue(undefined) | ||
| mockApplyBulkManualAdjustments.mockResolvedValue(0) | ||
| }) |
There was a problem hiding this comment.
Clear mock call history between tests.
mockApplyBulkManualAdjustments is asserted with exact call counts in later tests, but beforeEach only resets implementations. Prior test calls can leak and make this suite order-dependent.
💚 Proposed fix
beforeEach(() => {
+ vi.clearAllMocks()
prisma = createMockPrisma()
mockApplyManualAdjustment.mockResolvedValue(undefined)
mockApplyBulkManualAdjustments.mockResolvedValue(0)
})📝 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.
| beforeEach(() => { | |
| prisma = createMockPrisma() | |
| mockApplyManualTrustAdjustment.mockResolvedValue(undefined) | |
| mockApplyManualAdjustment.mockResolvedValue(undefined) | |
| mockApplyBulkManualAdjustments.mockResolvedValue(0) | |
| }) | |
| beforeEach(() => { | |
| vi.clearAllMocks() | |
| prisma = createMockPrisma() | |
| mockApplyManualAdjustment.mockResolvedValue(undefined) | |
| mockApplyBulkManualAdjustments.mockResolvedValue(0) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/services/vote-nullification.service.test.ts` around lines 151 -
155, The tests leak previous mock call history because beforeEach only sets
return values; update the beforeEach in vote-nullification.service.test.ts to
also clear/reset mock histories for mockApplyBulkManualAdjustments and
mockApplyManualAdjustment (e.g., call mockClear() or mockReset() on both) before
re-applying mockResolvedValue so later exact-call-count assertions on
mockApplyBulkManualAdjustments are reliable; locate the beforeEach that
initializes prisma/createMockPrisma and modify it to clear these mocks first.
| const styles = await page | ||
| .locator('p, span, div, h1, h2, h3, h4, h5, h6') | ||
| .filter({ hasText: /\S+/ }) | ||
| const sampleSize = Math.min(await textElements.count(), 10) | ||
|
|
||
| for (let i = 0; i < sampleSize; i++) { | ||
| const element = textElements.nth(i) | ||
|
|
||
| const styles = await element.evaluate((el) => { | ||
| const computed = window.getComputedStyle(el) | ||
| return { | ||
| color: computed.color, | ||
| backgroundColor: computed.backgroundColor, | ||
| fontSize: computed.fontSize, | ||
| fontWeight: computed.fontWeight, | ||
| } | ||
| }) | ||
|
|
||
| // Skip transparent elements as they inherit parent background | ||
| if ( | ||
| styles.backgroundColor === 'rgba(0, 0, 0, 0)' || | ||
| styles.backgroundColor === 'transparent' | ||
| ) { | ||
| .evaluateAll((elements) => | ||
| elements.slice(0, 10).map((el) => { | ||
| const computed = window.getComputedStyle(el) | ||
| return { | ||
| color: computed.color, | ||
| backgroundColor: computed.backgroundColor, | ||
| fontSize: computed.fontSize, | ||
| } | ||
| }), | ||
| ) | ||
|
|
||
| for (const s of styles) { | ||
| if (s.backgroundColor === 'rgba(0, 0, 0, 0)' || s.backgroundColor === 'transparent') { | ||
| continue | ||
| } | ||
|
|
||
| // Basic check: text should not be same color as background | ||
| if (styles.color !== 'rgba(0, 0, 0, 0)' && styles.color !== 'transparent') { | ||
| expect(styles.color).not.toBe(styles.backgroundColor) | ||
| if (s.color !== 'rgba(0, 0, 0, 0)' && s.color !== 'transparent') { | ||
| expect(s.color).not.toBe(s.backgroundColor) | ||
| } | ||
|
|
||
| // Text should be readable size | ||
| const fontSize = parseInt(styles.fontSize) | ||
| expect(fontSize).toBeGreaterThanOrEqual(12) | ||
| expect(parseInt(s.fontSize)).toBeGreaterThanOrEqual(12) | ||
| } |
There was a problem hiding this comment.
Restore a real contrast-ratio assertion.
This now only checks that text and background colors differ, which lets low-contrast combinations pass. The test name says “sufficient color contrast”, so it should compute the contrast ratio or delegate to an accessibility checker.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/accessibility.spec.ts` around lines 95 - 119, The test currently only
checks that text and background colors differ; replace that with a real
contrast-ratio check: after collecting styles (variable styles) compute the WCAG
contrast ratio between s.color and s.backgroundColor using relative luminance
and contrastRatio helper logic, then assert the ratio meets the appropriate
threshold (use 4.5:1 for normal text and 3:1 for "large" text determined from
parseInt(s.fontSize) — e.g., treat fontSize >= 18px as large). Keep existing
skips for transparent backgrounds and ensure you call the contrast check where
the loop iterates over styles (referring to styles, s.color, s.backgroundColor,
s.fontSize) and replace the simple color inequality assertion with
expect(contrastRatio).toBeGreaterThanOrEqual(threshold).
| async function selectFirstFilterOption(page: Page, filterButton: Locator) { | ||
| await filterButton.click() | ||
| const firstOption = page.locator('label:has(input[type="checkbox"])').first() | ||
| await expect(firstOption).toBeVisible() | ||
| await firstOption.click() | ||
| // MultiSelect doesn't close on Escape and a lingering open dropdown blocks | ||
| // subsequent clicks, so toggle the filter button to dismiss it. | ||
| await filterButton.click() | ||
| await expect(firstOption).toBeHidden() |
There was a problem hiding this comment.
Scope the option lookup to the opened dropdown.
Line 7 uses the first checkbox label on the whole page. In emulator/performance tests, that can select a device option or wait on a hidden label instead of the menu just opened.
💚 Proposed fix
async function selectFirstFilterOption(page: Page, filterButton: Locator) {
await filterButton.click()
- const firstOption = page.locator('label:has(input[type="checkbox"])').first()
+ const firstOption = page.locator('label:has(input[type="checkbox"]):visible').first()
await expect(firstOption).toBeVisible()
await firstOption.click()
// MultiSelect doesn't close on Escape and a lingering open dropdown blocks
// subsequent clicks, so toggle the filter button to dismiss it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/filtering.spec.ts` around lines 5 - 13, In selectFirstFilterOption, the
locator for firstOption is global and can match labels outside the opened menu;
after clicking filterButton, scope the lookup to the dropdown container that was
just opened (e.g., locate the popup or the nearest sibling/ancestor menu
associated with filterButton) and then find the first
label:has(input[type="checkbox"]) inside that container; also wait for that
container to be visible before interacting and continue toggling filterButton to
dismiss and assert the scoped option is hidden. Use the existing symbols
selectFirstFilterOption, filterButton and firstOption to replace the global
page.locator with a locator scoped to the opened dropdown.
| const nextButton = page.getByRole('button', { name: /go to next page/i }) | ||
| const hasNextPage = await nextButton.isVisible().catch(() => false) | ||
| await expect(nextButton).toBeVisible() | ||
| await expect(nextButton).toBeEnabled() | ||
|
|
||
| if (hasNextPage && (await nextButton.isEnabled())) { | ||
| await nextButton.click() | ||
| await page.waitForLoadState('domcontentloaded') | ||
|
|
||
| // Sort params should persist on page 2 | ||
| await expect(page).toHaveURL(/sortField=successRate/) | ||
| await expect(page).toHaveURL(/sortDirection=desc/) | ||
| await nextButton.click() | ||
| await page.waitForLoadState('domcontentloaded') | ||
|
|
||
| // Table should still have rows | ||
| const rows = page.locator('tbody tr') | ||
| expect(await rows.count()).toBeGreaterThan(0) | ||
| } | ||
| await expect(page).toHaveURL(/sortField=successRate/) | ||
| await expect(page).toHaveURL(/sortDirection=desc/) | ||
| expect(await page.locator('tbody tr').count()).toBeGreaterThan(0) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect data-setup to confirm it seeds enough listings for pagination
fd -t f 'data-setup.spec.ts' tests | xargs rg -nP -C3 'listing|seed|create'Repository: Producdevity/EmuReady
Length of output: 1491
🏁 Script executed:
# Read the full test file to understand context and requirements
fd -t f 'listings-success-rate-sorting.spec.ts' tests | xargs cat -nRepository: Producdevity/EmuReady
Length of output: 4175
🏁 Script executed:
# Search for pagination/page size configuration
rg -nP 'pageSize|limit|page.*size|pagination' tests/ --type tsRepository: Producdevity/EmuReady
Length of output: 2344
🏁 Script executed:
# Check if there are other data creation mechanisms or fixtures
rg -nP 'createListing|create.*listing' tests/ --type ts -A2Repository: Producdevity/EmuReady
Length of output: 643
🏁 Script executed:
# Search for page size, limit, or pagination configuration in the app
rg -nP 'pageSize|itemsPerPage|limit|perPage|PAGE_SIZE|LIMIT' . --type ts --type js --type tsx --type jsx -iRepository: Producdevity/EmuReady
Length of output: 92
🏁 Script executed:
# Check the data factory to see if createHandheldListing/createPcListing create one or multiple listings
fd -t f 'data-factory' tests | xargs cat -n | head -100Repository: Producdevity/EmuReady
Length of output: 4305
🏁 Script executed:
# Search for pagination configuration or mock data setup
rg -nP 'pagination|mock.*listing|fixture.*listing' tests/ --type ts -A2 -B2Repository: Producdevity/EmuReady
Length of output: 4410
🏁 Script executed:
# Search for items per page or page size in config/app files
rg -nP 'itemsPerPage|pageSize|itemLimit|ITEMS_PER_PAGE' . --type ts -B2 -A2Repository: Producdevity/EmuReady
Length of output: 29294
🏁 Script executed:
# Look at the pagination tests to see how many listings are expected
fd -t f 'pagination.spec.ts' tests | xargs cat -nRepository: Producdevity/EmuReady
Length of output: 5071
🏁 Script executed:
# Check if there's a seeder or setup that creates more listings
rg -nP 'seeder|seed.*listing' . --type ts --type js -iRepository: Producdevity/EmuReady
Length of output: 7672
🏁 Script executed:
# Check the listingsSeeder to see how many listings it creates
fd -t f 'listingsSeeder.ts' prisma | xargs wc -lRepository: Producdevity/EmuReady
Length of output: 101
🏁 Script executed:
# Read the listingsSeeder to understand the scope
fd -t f 'listingsSeeder.ts' prisma/seeders | xargs head -150Repository: Producdevity/EmuReady
Length of output: 5151
🏁 Script executed:
# Search for how many listings are created in listingsSeeder
rg -nP 'for.*|while.*|\.create\(' prisma/seeders/listingsSeeder.ts | head -30Repository: Producdevity/EmuReady
Length of output: 1846
🏁 Script executed:
# Check how many devices are created in the devicesSeeder
rg -nP 'prisma.device.create|createMany' prisma/seeders/devicesSeeder.ts | wc -lRepository: Producdevity/EmuReady
Length of output: 66
🏁 Script executed:
# Look at the structure to understand device creation better
fd -t f 'devicesSeeder.ts' prisma/seeders | xargs tail -100Repository: Producdevity/EmuReady
Length of output: 3210
🏁 Script executed:
# Check if pagination tests are isolated to the test user or use all listings
rg -nP 'data-setup|afterEach|beforeEach|test.use' tests/pagination.spec.ts -A2Repository: Producdevity/EmuReady
Length of output: 47
🏁 Script executed:
# Read the full pagination spec file to see setup
fd -t f 'pagination.spec.ts' tests | xargs cat -n | head -30Repository: Producdevity/EmuReady
Length of output: 1713
🏁 Script executed:
# Check the listings page to see if it uses specific user or all listings
rg -nP 'goto|api|query' tests/pages/ListingsPage.ts -A2Repository: Producdevity/EmuReady
Length of output: 359
🏁 Script executed:
# Find how many listings the listingsSeeder actually creates
fd -t f 'listingsSeeder.ts' prisma/seeders | xargs cat | grep -A 20 'for (let i = 0; i < selectedGames'Repository: Producdevity/EmuReady
Length of output: 905
🏁 Script executed:
# Find the listings API route/endpoint to check default page size
fd -t f 'route.ts|listings' src/server/api | grep -i listing | head -10Repository: Producdevity/EmuReady
Length of output: 311
🏁 Script executed:
# Search for the default limit/pageSize in listings endpoint
rg -nP 'limit.*=|pageSize.*=|DEFAULT.*LIMIT' src/server/api --type ts -B2 -A2Repository: Producdevity/EmuReady
Length of output: 14053
🏁 Script executed:
# Check what the Pagination component defaults to
rg -nP 'limit.*default|pageSize.*default' src/app/listings --type ts -B2 -A2Repository: Producdevity/EmuReady
Length of output: 670
🏁 Script executed:
# Find the listings schema to check the default limit
rg -nP 'GetListingsSchema|limit.*default' src/server/api/routers/listings --type ts -A5Repository: Producdevity/EmuReady
Length of output: 1015
🏁 Script executed:
# Check the ListingsPage implementation for default page size
fd -t f 'ListingsPage.tsx' src/app | xargs cat | grep -A 30 'const.*limit\|page.*size' | head -50Repository: Producdevity/EmuReady
Length of output: 2337
🏁 Script executed:
# Look at the schema definition for the handheld listings query
rg -nP 'export.*GetListingsSchema|limit.*=' src/server --type ts | grep -i listing | head -10Repository: Producdevity/EmuReady
Length of output: 1076
🏁 Script executed:
# Find the PAGINATION constant definition
rg -nP 'PAGINATION.*=.*{|DEFAULT_LIMIT' src/server --type ts -A2 -B1Repository: Producdevity/EmuReady
Length of output: 9000
🏁 Script executed:
# Check how listingsState is initialized in ListingsPage
fd -t f 'ListingsPage.tsx' src/app | xargs grep -A 30 'listingsState\|useListingsState\|useState.*limit'Repository: Producdevity/EmuReady
Length of output: 50377
🏁 Script executed:
# Search for the listings state initialization
rg -nP 'limit.*=' src/app/listings/ListingsPage.tsx -B2 -A2Repository: Producdevity/EmuReady
Length of output: 47
🏁 Script executed:
# Find useListingsState hook definition
fd -t f 'useListingsState' src/app/listings | xargs cat | head -100Repository: Producdevity/EmuReady
Length of output: 3120
🏁 Script executed:
# Search for PAGINATION constant and DEFAULT_LIMIT value
rg -nP 'export.*PAGINATION|DEFAULT_LIMIT.*=' src/server/constants --type ts -A1Repository: Producdevity/EmuReady
Length of output: 125
🏁 Script executed:
# Find PAGINATION constant definition
rg -nP 'PAGINATION.*=|DEFAULT_LIMIT' src/data --type ts -A2 -B1Repository: Producdevity/EmuReady
Length of output: 527
🏁 Script executed:
# Find where PAGINATION constant is defined
rg -nP 'export.*PAGINATION' src --type ts -l | head -5Repository: Producdevity/EmuReady
Length of output: 86
Test will fail without sufficient seeded listings. The pagination assertion at lines 71–72 expects the "next page" button to be visible and enabled. However, data-setup.spec.ts creates only 2 listings (1 handheld + 1 PC), and the default page size is 10 items. This means the test user will only see page 1 with all listings, making the "next page" button invisible or disabled—causing the test to fail.
Expand data-setup.spec.ts to create at least 11 listings (or match your target page size) to ensure pagination is actually triggered. The unconditional assertions are good for surfacing real issues; ensure the test data supports them.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/listings-success-rate-sorting.spec.ts` around lines 70 - 79, The test
assumes pagination (checks nextButton from page.getByRole and expects more than
one page) but the seed data only creates 2 listings; update the test data seeder
(data-setup.spec.ts) to create at least pageSize + 1 listings (e.g., 11 if
default page size is 10) so the "next page" button becomes visible/enabled;
modify the seeding helper (seedListings / createListing / createListings
function used in data-setup.spec.ts) to loop and insert the required number of
listings (mixing handheld and PC as needed) before running the tests.
| protected constructor(page: Page) { | ||
| this.page = page | ||
| this.cookieBanner = new CookieBanner(page) | ||
| } | ||
|
|
||
| // Navigation elements that appear on all pages | ||
| get navigation() { | ||
| return this.page.locator('nav').first() | ||
| page.context().addInitScript(COOKIE_CONSENT_SCRIPT) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Verify the current Playwright TypeScript API for BrowserContext.addInitScript return type and whether callers must await it before navigation.
💡 Result:
The current Playwright TypeScript API for BrowserContext.addInitScript returns Promise<Disposable>. Callers must await the call because it is asynchronous (returns a Promise), but they do not need to await it specifically before calling navigation methods like page.goto. The init script will be evaluated on subsequent page creations and navigations after the await resolves, as per the official documentation and TypeScript definitions. Examples from docs: await browserContext.addInitScript({ path: 'preload.js' }); await page.goto('https://example.com'); // Can follow immediately after await The Disposable (added in v1.59) allows removing the init script via disposable.dispose for future navigations.
Citations:
- 1: https://github.com/microsoft/playwright/blob/main/packages/playwright-core/types/types.d.ts
- 2: https://playwright.dev/docs/api/class-browsercontext
- 3: https://playwright.dev/docs/next/api/class-browsercontext
- 4: https://playwright.dev/docs/next/api/class-disposable
- 5: https://playwright.dev/docs/api/class-disposable
- 6: feat(api): introduce disposable return values microsoft/playwright#39504
🏁 Script executed:
# First, let's look at the complete BasePage class
cat -n tests/pages/BasePage.tsRepository: Producdevity/EmuReady
Length of output: 3517
🏁 Script executed:
# Check if there are subclasses that override navigation methods
fd -e ts -e js tests/pages/ | grep -v node_modules | head -20Repository: Producdevity/EmuReady
Length of output: 418
🏁 Script executed:
# Look for usage patterns of BasePage in tests to understand the race condition risk
rg "new.*BasePage\|extends BasePage" tests/ -A 3 -B 1 --max-count=20Repository: Producdevity/EmuReady
Length of output: 47
🏁 Script executed:
# Find BasePage subclasses
rg "extends BasePage" tests/ -A 5Repository: Producdevity/EmuReady
Length of output: 1418
🏁 Script executed:
# Look at the test structure to understand instantiation patterns
fd . tests/ --full-path --extension ts | head -20Repository: Producdevity/EmuReady
Length of output: 586
🏁 Script executed:
# Check test files that import BasePage or its subclasses
rg "from.*BasePage|import.*BasePage" tests/ -B 2 -A 5 --max-count=10Repository: Producdevity/EmuReady
Length of output: 1907
Await init script registration before navigating to prevent race condition.
Line 26 drops the addInitScript(...) promise. Since constructors cannot be async, tests that instantiate a page object and immediately navigate can load before the cookie-consent script completes registration with the browser context. Playwright's browserContext.addInitScript returns Promise<Disposable> which must resolve before init scripts take effect on subsequent navigations.
Store the promise and await it before any navigation. Add the check to navigateToHome(), navigateToHandheld(), navigateToPC(), navigateToGames(), and clickLogo().
Suggested fix
export abstract class BasePage {
readonly page: Page
+ private readonly cookieConsentReady: Promise<void>
protected constructor(page: Page) {
this.page = page
- page.context().addInitScript(COOKIE_CONSENT_SCRIPT)
+ this.cookieConsentReady = page.context().addInitScript(COOKIE_CONSENT_SCRIPT)
}
+
+ protected async ensureCookieConsentScriptReady() {
+ await this.cookieConsentReady
+ }
async navigateToHome() {
+ await this.ensureCookieConsentScriptReady()
await this.page.goto('/')
await expect(this.page).toHaveURL('/')
}Apply ensureCookieConsentScriptReady() to the other navigation methods and any subclass overrides.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/pages/BasePage.ts` around lines 24 - 27, The constructor currently
calls page.context().addInitScript(COOKIE_CONSENT_SCRIPT) without awaiting it,
causing a race when tests navigate immediately; modify the class to store the
addInitScript promise (e.g., a private field like cookieScriptReady =
page.context().addInitScript(...)) and add an async helper
ensureCookieConsentScriptReady() that awaits that promise, then call await
this.ensureCookieConsentScriptReady() at the start of navigateToHome(),
navigateToHandheld(), navigateToPC(), navigateToGames(), and clickLogo() so
navigation only proceeds after the init script registration completes.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/shared/emulator-config/gamenative/mapping.ts (1)
106-114: Hoist the reverse maps and rename the misleadinguppervariable.
createReverseLookup(BOX64_PRESET_MAPPING)/createReverseLookup(FEXCORE_PRESET_MAPPING)are rebuilt on every invocation — unlike the other three reverse maps which are module-level constants. Also,const upper = String(value)never uppercases anything, which makes the subsequent.toLowerCase()fallback confusing.♻️ Proposed refactor
const GRAPHICS_DRIVER_REVERSE = createReverseLookup(GRAPHICS_DRIVER_MAPPING) const DX_WRAPPER_REVERSE = createReverseLookup(DX_WRAPPER_MAPPING) const STARTUP_SELECTION_REVERSE = createReverseLookup(STARTUP_SELECTION_MAPPING) +const BOX64_PRESET_REVERSE = createReverseLookup(BOX64_PRESET_MAPPING) +const FEXCORE_PRESET_REVERSE = createReverseLookup(FEXCORE_PRESET_MAPPING) @@ box64_preset: { jsonPath: 'box64Preset', fromConfig: (value) => { - const upper = String(value) - // Reverse: uppercase → lowercase for custom field - const reversed = createReverseLookup(BOX64_PRESET_MAPPING) - return reversed[upper] ?? upper.toLowerCase() + const str = String(value) + return BOX64_PRESET_REVERSE[str] ?? str.toLowerCase() }, }, @@ fex_core_preset: { jsonPath: 'fexcorePreset', fromConfig: (value) => { - const upper = String(value) - const reversed = createReverseLookup(FEXCORE_PRESET_MAPPING) - return reversed[upper] ?? upper.toLowerCase() + const str = String(value) + return FEXCORE_PRESET_REVERSE[str] ?? str.toLowerCase() }, },Also applies to: 177-184
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/shared/emulator-config/gamenative/mapping.ts`:
- Around line 97-100: The startup_selection mapping currently returns undefined
for unknown values because fromConfig uses
STARTUP_SELECTION_REVERSE[String(value)] with no fallback; change fromConfig in
the startup_selection entry to return STARTUP_SELECTION_REVERSE[String(value)]
|| String(value) (matching the fallback used by graphics_driver and dx_wrapper)
so unrecognized startupSelection values are preserved as strings instead of
being dropped.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b80f8eea-ec8d-49a5-83f5-2a65c6b7590b
📒 Files selected for processing (3)
prisma/seeders/gamenativeCustomFieldsSeeder.tssrc/shared/emulator-config/gamenative/mapping.tstests/listings-success-rate-sorting.spec.ts
✅ Files skipped from review due to trivial changes (1)
- prisma/seeders/gamenativeCustomFieldsSeeder.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/listings-success-rate-sorting.spec.ts
1 participant
Trust system overhaul, GameNative support, vote fixes, and E2E cleanup
Description
Started as GameNative emulator support, expanded significantly after review uncovered pre-existing bugs, including 4 critical auth bypasses and several vote system race conditions.
What's in here:
VOTE_CHANGE_REVERSALtrust actionreverseLogAction, vote-change reversals, bulk adjustments, trust effects now run inside transactionstrust.tsendpoints calledAppError.insufficientRole()withoutreturn, so the check fell through and any authenticated user could hit SUPER_ADMIN-only endpoints. All 4 patched + regression testedreturnpattern inadmin.tslet banned authors' listings get approved despite the rejection branch runningexistingVotefetched outside$transactionin 4 procedures (pcListings.vote + 3 voteComment). Concurrent votes from the same user would P2002. Moved all inside txLISTING_VOTED/COMMENT_VOTEDno longer fires on toggle-off, mobile now has parity with web, reCAPTCHA added to PC votebatchUpdateNullifiedAthelper replaces 8 duplicate loops, bulk approve/reject trust actions parallelizedBannedUserBadgecomponent (replaces 6 duplicated inline checks),checkSpamContentutility,ModeratorInfoPanel+ vote detail components, configurable sort on admin processed listingspc-voting.spec.tsandtrust-integration.spec.ts__tests__/dirs)Type of change
How Has This Been Tested?
Checklist
Notes for reviewers
The 4 auth bypasses in
trust.tswere the most critical find,AppError.insufficientRole()throws but withoutreturnthe function keeps executing. Regression tests now cover all 4 endpoints across 5 role levels.Vote nullification refactor keeps the serial-within-batch strategy (connection pool safety) but parallelizes across vote types.
Out-of-scope items left as TODOs: PC admin processed-listings endpoint (doesn't exist yet), web route spam detection.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes