feat: add admin auto-reject for review-risk reports#418
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughAdds an admin-only bulk auto-reject flow for review-risk-flagged pending listings: candidate selection logic, auto-reject services for handheld and PC listings, admin TRPC procedures, a reusable ReviewRiskAutoRejectPanel with tests, confirmation dialog plumbing, and approvals page integrations. ChangesAdmin Auto-Reject Review-Risk Listings
Sequence DiagramssequenceDiagram
participant Admin
participant Page
participant API
participant RiskService
participant Database
participant TrustService
participant Cache
participant Notifier
Admin->>Page: Click auto-reject (risk-only)
Page->>Page: Open confirmation dialog (details from preview)
Admin->>Page: Confirm
Page->>API: autoRejectRisky()
API->>RiskService: getAutoRejectableReviewRiskItemsForCandidates()
RiskService->>Database: load candidates + compute profiles
RiskService-->>API: eligible items w/ processedNotes
API->>Database: transaction update PENDING -> REJECTED (per eligible ids)
API->>TrustService: applyTrustAction per rejected listing
API->>Notifier: emit rejection notifications
API->>Cache: invalidate listing stats / compatibility
API-->>Page: { success, rejectedCount, skippedCount }
Page->>Page: show toast, invalidate queries, clear selection
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…g UI integration, admin-only permission, and submission/author risk evaluation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/api/routers/listings/admin.ts`:
- Around line 835-957: Extract the multi-step auto-reject workflow from the
router mutation autoRejectRisky into a dedicated service (e.g.,
AutoRejectService) and make the router call that service for orchestration only:
move Prisma operations (the transaction block that queries/updates listing via
tx.listing.*), the logic that builds processedNotesById/listingIds, calls to
getAutoRejectableReviewRiskItemsForCandidates and
ListingsRepository.getPendingListingRiskCandidates, trust actions via
applyTrustAction, notification emission via
notificationEventEmitter.emitNotificationEvent, cache invalidation calls
(listingStatsCache.delete and invalidateCatalogCompatibilityCacheForDevices),
and message construction into methods on the new service; keep the router
mutation to validate admin user and call
AutoRejectService.autoRejectRisky({adminUserId}) and return the service result.
Ensure the service returns the same shape ({ success, rejectedCount,
skippedCount, message }) and surface any errors for the router to handle.
- Around line 895-910: The current Promise.all over rejectedListingsWithAuthor
calls applyTrustAction after the DB commit, but a single rejection will throw
and cause the mutation to fail; change this to use per-item error isolation
(e.g., Promise.allSettled or try/catch per listing) so failures in
applyTrustAction do not propagate and fail the whole endpoint. For each listing
from rejectedListingsWithAuthor (and using processedNotesById, listing.id,
listing.authorId, adminUserId, TrustAction.LISTING_REJECTED, applyTrustAction),
catch/log errors for that specific call and continue processing the others, and
surface aggregated/logged errors separately instead of letting a single
rejection throw.
In `@src/server/api/routers/pcListings.test.ts`:
- Around line 997-1001: The test is using raw status strings in the mock
expectation; replace those literals with the enum constant
ApprovalStatus.PENDING (and similarly for other occurrences at the noted
assertion blocks) so assertions use the ApprovalStatus enum instead of
'PENDING'/'APPROVED' strings; update the expectations against
prisma.pcListing.findMany and other findMany/findUnique mocks to reference
ApprovalStatus.<ENUM_MEMBER> (e.g., ApprovalStatus.PENDING) while keeping the
rest of the matcher intact (preserve LISTING_ID, LISTING_ID_B and the where
shape).
In `@src/server/api/routers/pcListings.ts`:
- Around line 883-985: The autoRejectRisky adminProcedure mutation contains
multi-step business logic and raw Prisma writes (calls to
ctx.prisma.$transaction, PcListingsRepository.getPendingListingRiskCandidates,
processedNotesById, applyTrustAction, notificationEventEmitter,
listingStatsCache) which must be moved into a service; create e.g. a
PcListingsService with a method like autoRejectRiskyListings({ prisma,
currentUserId }) that performs getAutoRejectableReviewRiskItemsForCandidates,
the transaction that updates pcListing status, applies trust actions
(applyTrustAction), emits notifications
(notificationEventEmitter.emitNotificationEvent), clears listingStatsCache, and
returns { pendingListings, skippedCount } and any message data. In the router
mutation autoRejectRisky keep only auth/context extraction and response shaping:
call the new service method, then return the same { success, rejectedCount,
skippedCount, message } shape; update imports and unit tests accordingly and
ensure the service accepts prisma and session user id (or ctx) rather than using
ctx directly.
- Around line 905-925: The current transaction reads PENDING pcListing rows via
tx.pcListing.findMany but then updates by id only (tx.pcListing.update), which
can overwrite concurrent state changes; change the update to require the status
is still ApprovalStatus.PENDING (e.g., update with a compound where that
includes status or use tx.pcListing.updateMany with where: { id: listing.id,
status: ApprovalStatus.PENDING }) and handle the returned count/result to know
if the row was actually updated; update the same fields (status, processedAt,
processedByUserId, processedNotes using processedNotesById) only when the
precondition holds so you avoid races in the $transaction block that reads
pending listings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: dcf6b6b0-6687-4495-88d2-3936857c97f9
📒 Files selected for processing (12)
src/app/admin/approvals/page.tsxsrc/app/admin/pc-listing-approvals/page.tsxsrc/components/admin/review-risk/ReviewRiskAutoRejectPanel.test.tsxsrc/components/admin/review-risk/ReviewRiskAutoRejectPanel.tsxsrc/components/admin/review-risk/index.tssrc/server/api/routers/listings.tssrc/server/api/routers/listings/admin.test.tssrc/server/api/routers/listings/admin.tssrc/server/api/routers/pcListings.test.tssrc/server/api/routers/pcListings.tssrc/server/services/review-risk.service.test.tssrc/server/services/review-risk.service.ts
Comment on lines
+895
to
+910
| await Promise.all( | ||
| rejectedListingsWithAuthor.map((listing) => { | ||
| const processedNotes = | ||
| processedNotesById.get(listing.id) ?? 'Automatically rejected by review risk.' | ||
|
|
||
| return applyTrustAction({ | ||
| userId: listing.authorId, | ||
| action: TrustAction.LISTING_REJECTED, | ||
| context: { | ||
| listingId: listing.id, | ||
| adminUserId, | ||
| reason: processedNotes, | ||
| }, | ||
| }) | ||
| }), | ||
| ) |
There was a problem hiding this comment.
Don’t fail the mutation after commit when trust updates partially fail.
If any applyTrustAction call rejects, this throws after listings are already rejected in the transaction. The caller receives a failure and may retry, creating duplicate side effects.
Suggested hardening
- await Promise.all(
- rejectedListingsWithAuthor.map((listing) => {
+ const trustActionResults = await Promise.allSettled(
+ rejectedListingsWithAuthor.map((listing) => {
const processedNotes =
processedNotesById.get(listing.id) ?? 'Automatically rejected by review risk.'
return applyTrustAction({
userId: listing.authorId,
action: TrustAction.LISTING_REJECTED,
context: {
listingId: listing.id,
adminUserId,
reason: processedNotes,
},
})
}),
)
+ const trustFailures = trustActionResults.filter((r) => r.status === 'rejected')
+ if (trustFailures.length > 0) {
+ console.error('Some trust actions failed during autoRejectRisky', trustFailures)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await Promise.all( | |
| rejectedListingsWithAuthor.map((listing) => { | |
| const processedNotes = | |
| processedNotesById.get(listing.id) ?? 'Automatically rejected by review risk.' | |
| return applyTrustAction({ | |
| userId: listing.authorId, | |
| action: TrustAction.LISTING_REJECTED, | |
| context: { | |
| listingId: listing.id, | |
| adminUserId, | |
| reason: processedNotes, | |
| }, | |
| }) | |
| }), | |
| ) | |
| const trustActionResults = await Promise.allSettled( | |
| rejectedListingsWithAuthor.map((listing) => { | |
| const processedNotes = | |
| processedNotesById.get(listing.id) ?? 'Automatically rejected by review risk.' | |
| return applyTrustAction({ | |
| userId: listing.authorId, | |
| action: TrustAction.LISTING_REJECTED, | |
| context: { | |
| listingId: listing.id, | |
| adminUserId, | |
| reason: processedNotes, | |
| }, | |
| }) | |
| }), | |
| ) | |
| const trustFailures = trustActionResults.filter((r) => r.status === 'rejected') | |
| if (trustFailures.length > 0) { | |
| console.error('Some trust actions failed during autoRejectRisky', trustFailures) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/api/routers/listings/admin.ts` around lines 895 - 910, The current
Promise.all over rejectedListingsWithAuthor calls applyTrustAction after the DB
commit, but a single rejection will throw and cause the mutation to fail; change
this to use per-item error isolation (e.g., Promise.allSettled or try/catch per
listing) so failures in applyTrustAction do not propagate and fail the whole
endpoint. For each listing from rejectedListingsWithAuthor (and using
processedNotesById, listing.id, listing.authorId, adminUserId,
TrustAction.LISTING_REJECTED, applyTrustAction), catch/log errors for that
specific call and continue processing the others, and surface aggregated/logged
errors separately instead of letting a single rejection throw.
Comment on lines
+905
to
+925
| const transactionResult = await ctx.prisma.$transaction(async (tx) => { | ||
| const pendingListings = await tx.pcListing.findMany({ | ||
| where: { | ||
| id: { in: pcListingIds }, | ||
| status: ApprovalStatus.PENDING, | ||
| }, | ||
| select: { id: true, authorId: true }, | ||
| }) | ||
|
|
||
| for (const listing of pendingListings) { | ||
| await tx.pcListing.update({ | ||
| where: { id: listing.id }, | ||
| data: { | ||
| status: ApprovalStatus.REJECTED, | ||
| processedAt: new Date(), | ||
| processedByUserId: ctx.session.user.id, | ||
| processedNotes: | ||
| processedNotesById.get(listing.id) ?? 'Automatically rejected by review risk.', | ||
| }, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Guard against status races in auto-reject updates.
Line 906 reads PENDING rows, but Line 915 updates by id only. A concurrent approval/rejection after the read can still be overwritten to REJECTED.
Suggested fix
- const transactionResult = await ctx.prisma.$transaction(async (tx) => {
+ const transactionResult = await ctx.prisma.$transaction(async (tx) => {
const pendingListings = await tx.pcListing.findMany({
where: {
id: { in: pcListingIds },
status: ApprovalStatus.PENDING,
},
select: { id: true, authorId: true },
})
+ const rejectedListings: typeof pendingListings = []
+ let skippedCount = pcListingIds.length - pendingListings.length
+ const processedAt = new Date()
+
for (const listing of pendingListings) {
- await tx.pcListing.update({
- where: { id: listing.id },
+ const { count } = await tx.pcListing.updateMany({
+ where: { id: listing.id, status: ApprovalStatus.PENDING },
data: {
status: ApprovalStatus.REJECTED,
- processedAt: new Date(),
+ processedAt,
processedByUserId: ctx.session.user.id,
processedNotes:
processedNotesById.get(listing.id) ?? 'Automatically rejected by review risk.',
},
})
+ if (count === 1) rejectedListings.push(listing)
+ else skippedCount += 1
}
return {
- pendingListings,
- skippedCount: pcListingIds.length - pendingListings.length,
+ pendingListings: rejectedListings,
+ skippedCount,
}
})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/api/routers/pcListings.ts` around lines 905 - 925, The current
transaction reads PENDING pcListing rows via tx.pcListing.findMany but then
updates by id only (tx.pcListing.update), which can overwrite concurrent state
changes; change the update to require the status is still ApprovalStatus.PENDING
(e.g., update with a compound where that includes status or use
tx.pcListing.updateMany with where: { id: listing.id, status:
ApprovalStatus.PENDING }) and handle the returned count/result to know if the
row was actually updated; update the same fields (status, processedAt,
processedByUserId, processedNotes using processedNotesById) only when the
precondition holds so you avoid races in the $transaction block that reads
pending listings.
…improve auto-reject panel test cases
Producdevity
changed the title
…uding updates to handling and notification logic
Producdevity
force-pushed
the
bulk-risk-reject
branch
from
685b0f1 to
ab085f9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/api/routers/pcListings.ts`:
- Around line 893-897: The mutation autoRejectRisky (adminProcedure.mutation)
currently calls autoRejectRiskyPcReports without verifying the admin user
exists; add a guard that queries ctx.prisma.user.findUnique for
ctx.session.user.id and throw or return an appropriate error if not found before
calling autoRejectRiskyPcReports so you never pass a deleted/stale ID into
processedByUserId/notification or trust writes. Ensure the existence check
occurs in the same resolver (autoRejectRisky) and only forward ctx.prisma and
adminUserId to autoRejectRiskyPcReports after the user is confirmed.
In `@src/server/services/review-risk.service.ts`:
- Around line 100-113: hasHighSubmissionRisk currently returns true solely when
profile.highestSeverity === 'high', allowing malformed profiles with no actual
signals to count as auto-rejectable; update the logic so that
hasHighSubmissionRisk also ensures the profile contains real submission signals
(e.g., profile.signals exists and has length > 0) before returning true, and
keep isAutoRejectableReviewRisk's combination with hasAuthorRiskSignals
unchanged so the submission-side path only auto-rejects when there are both a
'high' severity and actual submission signals present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b746f559-aa64-49db-8639-922fe99d25f3
📒 Files selected for processing (16)
src/app/admin/approvals/page.tsxsrc/app/admin/pc-listing-approvals/page.tsxsrc/components/admin/review-risk/ReviewRiskAutoRejectPanel.test.tsxsrc/components/admin/review-risk/ReviewRiskAutoRejectPanel.tsxsrc/components/admin/review-risk/index.tssrc/components/ui/AlertDialog.tsxsrc/components/ui/ConfirmDialogProvider.test.tsxsrc/components/ui/ConfirmDialogProvider.tsxsrc/server/api/routers/listings.tssrc/server/api/routers/listings/admin.test.tssrc/server/api/routers/listings/admin.tssrc/server/api/routers/pcListings.test.tssrc/server/api/routers/pcListings.tssrc/server/services/review-risk-auto-reject.service.tssrc/server/services/review-risk.service.test.tssrc/server/services/review-risk.service.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/admin/review-risk/index.ts
- src/server/api/routers/listings/admin.test.ts
- src/app/admin/approvals/page.tsx
…sk profiles without signals
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds an admin-only bulk action to auto-reject pending handheld and PC compatibility reports that match review-risk rejection rules.
The auto-reject rule is:
The rejection workflow now lives in the service layer for both handheld and PC reports. It generates per-report processed notes, guards against status races before updating, and keeps trust/notification side effects from failing the endpoint after reports have already been rejected.
No linked issue.
Type of change
How Has This Been Tested?
Ran:
pnpm typespnpm lintpnpm testScreenshots (if applicable)
N/A
Checklist
Notes for reviewers
The button only appears for admins while viewing the review-risk queue. The confirmation dialog shows the matching count and scope before running the bulk rejection.
Summary by CodeRabbit
New Features
Bug Fixes
Tests