Feat/improved review process#353
Open
Producdevity wants to merge 14 commits into
Open
Conversation
…t aren't signed in default to false
…isk detection, with associated tests and UI components
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds review-risk computation and wiring: submission/author/review risk services, repository pending-query support, router branching for riskFilter, admin UI/filter components, tests for services and routers, Toggle/Admin UI primitives, and small config/CORS/dev-script tweaks. ChangesReview risk feature and infra
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/repositories/pc-listings.repository.ts (1)
855-882:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInconsistent shadow-ban filtering between pending listings repositories.
The
buildPendingListingsWheremethod in this file applies shadow-ban filtering to pending PC listings whencanSeeBannedUsersis false, but the equivalent method inlistings.repository.tsdoes not apply any shadow-ban filtering.This creates inconsistent behavior:
- Pending PC listings from shadow-banned users are hidden by default
- Pending handheld listings from shadow-banned users are shown
Both repositories should apply the same shadow-ban filtering logic, or neither should. Without alignment, moderators reviewing different listing types will have different visibility, which could lead to confusion and inconsistent workflows.
🤖 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/repositories/pc-listings.repository.ts` around lines 855 - 882, The PC listings method buildPendingListingsWhere applies shadow-ban filtering when canSeeBannedUsers is false but the handheld equivalent does not; align them by applying the same logic: compute const shadowBanFilter = canSeeBannedUsers ? undefined : buildShadowBanFilter(null) and merge it into the returned where clause as ...(shadowBanFilter && { author: shadowBanFilter }) (or conversely remove that merge from buildPendingListingsWhere if you intend no shadow-ban filtering), ensuring both repositories use the same approach for canSeeBannedUsers and buildShadowBanFilter so moderator visibility is consistent.
🧹 Nitpick comments (6)
src/server/api/routers/listings/admin.test.ts (2)
21-43: 💤 Low valueConsider extracting the inline mock implementation to a shared test helper.
The inline implementation of
createExistingAuthorBansMapduplicates business logic within the mock. If the real implementation changes, this could drift out of sync. Consider either importing the real helper or creating a shared test utility that both test files can use.🤖 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.test.ts` around lines 21 - 43, The inline mock function createExistingAuthorBansMap duplicates production logic and should be extracted: move the implementation into a shared test helper (e.g., tests/helpers or tests/utils) or import the real helper if appropriate, then replace the inline definition in admin.test.ts with an import of createExistingAuthorBansMap and update any other tests to reuse that single exported function so logic remains centralized and in-sync with production code.
131-217: ⚡ Quick winAdd test coverage for the non-risky filter path.
The test suite only covers
riskFilter: 'risky'. Consider adding test cases for:
riskFilter: 'all'to verify the standard pending listings path- Edge cases like no risky listings found, or pagination edge cases
This would ensure both code paths are exercised and prevent regressions.
🤖 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.test.ts` around lines 131 - 217, Add tests covering the non-risky and no-risky paths: in src/server/api/routers/listings/admin.test.ts add a test calling caller.getPending({ riskFilter: 'all', page: 1, limit: 20 }) that stubs mockGetPendingListings to return expected rows and asserts mockGetPendingListings was called (and mockGetPendingListingRiskCandidates was not), verifies returned listings and pagination; also add a test for riskFilter: 'risky' when mockGetPendingListingRiskCandidates returns an empty array to assert mockGetPendingListingsByIds is not called and result.listings.length and pagination.total are 0. Ensure you reference and use the existing helpers/mocks (createCaller, mockGetPendingListingRiskCandidates, mockGetPendingListingsByIds, mockGetPendingListings) and mirror the style/assertions used in the existing 'filters risk-only listings' test.src/server/api/routers/pcListings.test.ts (2)
81-103: ⚡ Quick winExtract the inline mock implementation to a shared test utility.
This duplicates the same inline implementation pattern from
admin.test.ts. Both test files would benefit from a shared test helper forcreateExistingAuthorBansMapto avoid duplication and ensure consistency.🤖 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.test.ts` around lines 81 - 103, Extract the inline createExistingAuthorBansMap implementation into a shared test utility function and have both tests import and use it instead of duplicating the logic; specifically, create and export a helper named createExistingAuthorBansMap that accepts the same listings shape and returns a Map<string, {reason:string}[]>, then replace the inline implementations in pcListings.test.ts and admin.test.ts with imports of that helper so both tests call the single shared function and retain identical behavior.
477-561: ⚡ Quick winAdd test coverage for the non-risky filter path.
Similar to
admin.test.ts, this test suite only coversriskFilter: 'risky'. Add test cases for:
riskFilter: 'all'or undefined to verify the standard pending listings retrieval- Edge cases such as empty results or different user roles
This ensures both code paths are tested and prevents regressions.
🤖 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.test.ts` around lines 477 - 561, The suite only tests the 'risky' riskFilter path; add tests that exercise the non-risky/default path by calling caller.pending with riskFilter: 'all' and with riskFilter omitted/undefined, verifying that mockRepositoryGetPendingListings is used (not getPendingListingRiskCandidates/getPendingListingsByIds), that returned pcListings match the mocked pending listings and pagination.total reflects the count, and add an edge-case test for empty results and a test using a non-moderator role (e.g., Role.USER) to validate permission-based branches; reference mocks and helpers like createCaller, caller.pending, mockRepositoryGetPendingListings, mockRepositoryGetPendingListingRiskCandidates, mockRepositoryGetPendingListingsByIds, mockComputeAuthorRiskProfiles and mockComputeSubmissionRiskProfiles to seed expected responses and assertions.src/components/ui/ReviewRiskIndicator.test.tsx (1)
33-68: ⚡ Quick winAdd test coverage for the investigate interaction path.
The suite does not currently verify the clickable
onInvestigatebehavior (mouse + keyboard), which is core logic in the component.🧪 Suggested test addition
-import { render, screen } from '@testing-library/react' -import { describe, expect, it } from 'vitest' +import { fireEvent, render, screen } from '@testing-library/react' +import { describe, expect, it, vi } from 'vitest' @@ it('renders for submission risk without author risk', () => { @@ }) + + it('invokes investigate callback for clickable indicator', () => { + const onInvestigate = vi.fn() + render( + <ReviewRiskIndicator + authorRiskProfile={authorRiskProfile} + submissionRiskProfile={submissionRiskProfile} + onInvestigate={onInvestigate} + />, + ) + + const button = screen.getByRole('button') + fireEvent.click(button) + fireEvent.keyDown(button, { key: 'Enter' }) + fireEvent.keyDown(button, { key: ' ' }) + + expect(onInvestigate).toHaveBeenCalledWith('author-1') + expect(onInvestigate).toHaveBeenCalledTimes(3) + }) })🤖 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/components/ui/ReviewRiskIndicator.test.tsx` around lines 33 - 68, Add tests for the investigate interaction by rendering ReviewRiskIndicator with a jest.fn() onInvestigate prop and asserting it is called on mouse click and keyboard activation; locate the interactable element via screen.getByRole('status') (or the same role used in the component), use userEvent.click(status) to simulate mouse activation and userEvent.keyboard('{Enter}') and userEvent.keyboard(' ') after status.focus() to simulate Enter/Space keyboard activation, and assert onInvestigate was invoked for each interaction; ensure the test covers both the case with author/submission risk and the case with only submission risk so the click target exists in both scenarios.src/server/repositories/pc-listings.repository.ts (1)
890-894: 💤 Low valueConsider extracting active ban criteria to a helper method.
The inline active ban filtering (lines 891-893) duplicates the same logic used in
forListinclude at lines 86-87. While correct, extracting this to agetActiveBanWhere()helper method (similar tolistings.repository.tsline 572-577) would improve maintainability and ensure consistency across query builders.♻️ Suggested refactor
Add a private helper method:
+ private getActiveBanWhere(): Prisma.UserBanWhereInput { + return { + isActive: true, + OR: [{ expiresAt: null }, { expiresAt: { gt: new Date() } }], + } + } + private getPendingListingRiskCandidateSelect() { return { id: true, authorId: true, author: { select: { userBans: { - where: { - isActive: true, - OR: [{ expiresAt: null }, { expiresAt: { gt: new Date() } }], - }, + where: this.getActiveBanWhere(), select: { reason: true }, }, }, },Then reuse in the
forListinclude at line 85:author: { include: { userBans: { - where: { - isActive: true, - OR: [{ expiresAt: null }, { expiresAt: { gt: new Date() } }], - }, + where: this.getActiveBanWhere(), select: { id: true, reason: true, bannedAt: true, expiresAt: true }, }, }, },🤖 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/repositories/pc-listings.repository.ts` around lines 890 - 894, Extract the duplicated active-ban predicate into a private helper (e.g., private getActiveBanWhere()) that returns the object { isActive: true, OR: [{ expiresAt: null }, { expiresAt: { gt: new Date() } }] }, add it to the PCListingsRepository class, and replace the inline userBans.where in the forList include and the other occurrence (the block using userBans: { where: { ... } }) to call getActiveBanWhere() so both queries reuse the same logic for consistency.
🤖 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/components/admin/ReviewRiskFilterButton.tsx`:
- Around line 12-19: This toggle button (Button in ReviewRiskFilterButton) needs
an accessible pressed state: add aria-pressed={props.isActive} to the Button
element so screen readers announce the on/off state; ensure props.isActive is a
boolean in the component's props type and keep onToggle as the click handler.
In `@src/server/api/routers/listings/admin.ts`:
- Around line 89-109: The final by-ID fetch (repository.getPendingListingsByIds)
can return listings that no longer meet the original scope (emulatorIds/pending)
between the two queries; update the fetch to re-apply the same scope checks used
to build riskCandidates: either modify repository.getPendingListingsByIds to
accept the same scope parameters (emulatorIds and pending status/search
constraints) and use them in the DB query, or after fetching pageListings filter
them against the original riskCandidates set (or a map keyed by id) so only
listings that still match the emulatorIds/pending scope are kept before calling
attachReviewRiskProfiles; reference getPendingListingRiskCandidates,
getRiskyReviewItemIds, getPendingListingsByIds, computeReviewRiskProfiles, and
attachReviewRiskProfiles.
In `@src/server/api/routers/pcListings.ts`:
- Around line 467-489: The refetch of listings via
repository.getPendingListingsByIds creates a TOCTOU gap because the original
candidate query used emulatorIds and other auth constraints; fix by keeping the
same authorization scope when loading the final page: either change
getPendingListingsByIds to accept the same filter arguments (emulatorIds,
canSeeBannedUsers, search/sort if needed) and call it with the same parameters
used for getPendingListingRiskCandidates, or avoid the re-query entirely by
selecting full listing fields in getPendingListingRiskCandidates and using those
results (so computeReviewRiskProfiles and attachReviewRiskProfiles still work).
Update calls around getPendingListingRiskCandidates, getPendingListingsByIds,
computeReviewRiskProfiles, and attachReviewRiskProfiles to ensure emulatorIds
and canSeeBannedUsers are preserved when producing paginatedPcListings.
---
Outside diff comments:
In `@src/server/repositories/pc-listings.repository.ts`:
- Around line 855-882: The PC listings method buildPendingListingsWhere applies
shadow-ban filtering when canSeeBannedUsers is false but the handheld equivalent
does not; align them by applying the same logic: compute const shadowBanFilter =
canSeeBannedUsers ? undefined : buildShadowBanFilter(null) and merge it into the
returned where clause as ...(shadowBanFilter && { author: shadowBanFilter }) (or
conversely remove that merge from buildPendingListingsWhere if you intend no
shadow-ban filtering), ensuring both repositories use the same approach for
canSeeBannedUsers and buildShadowBanFilter so moderator visibility is
consistent.
---
Nitpick comments:
In `@src/components/ui/ReviewRiskIndicator.test.tsx`:
- Around line 33-68: Add tests for the investigate interaction by rendering
ReviewRiskIndicator with a jest.fn() onInvestigate prop and asserting it is
called on mouse click and keyboard activation; locate the interactable element
via screen.getByRole('status') (or the same role used in the component), use
userEvent.click(status) to simulate mouse activation and
userEvent.keyboard('{Enter}') and userEvent.keyboard(' ') after status.focus()
to simulate Enter/Space keyboard activation, and assert onInvestigate was
invoked for each interaction; ensure the test covers both the case with
author/submission risk and the case with only submission risk so the click
target exists in both scenarios.
In `@src/server/api/routers/listings/admin.test.ts`:
- Around line 21-43: The inline mock function createExistingAuthorBansMap
duplicates production logic and should be extracted: move the implementation
into a shared test helper (e.g., tests/helpers or tests/utils) or import the
real helper if appropriate, then replace the inline definition in admin.test.ts
with an import of createExistingAuthorBansMap and update any other tests to
reuse that single exported function so logic remains centralized and in-sync
with production code.
- Around line 131-217: Add tests covering the non-risky and no-risky paths: in
src/server/api/routers/listings/admin.test.ts add a test calling
caller.getPending({ riskFilter: 'all', page: 1, limit: 20 }) that stubs
mockGetPendingListings to return expected rows and asserts
mockGetPendingListings was called (and mockGetPendingListingRiskCandidates was
not), verifies returned listings and pagination; also add a test for riskFilter:
'risky' when mockGetPendingListingRiskCandidates returns an empty array to
assert mockGetPendingListingsByIds is not called and result.listings.length and
pagination.total are 0. Ensure you reference and use the existing helpers/mocks
(createCaller, mockGetPendingListingRiskCandidates, mockGetPendingListingsByIds,
mockGetPendingListings) and mirror the style/assertions used in the existing
'filters risk-only listings' test.
In `@src/server/api/routers/pcListings.test.ts`:
- Around line 81-103: Extract the inline createExistingAuthorBansMap
implementation into a shared test utility function and have both tests import
and use it instead of duplicating the logic; specifically, create and export a
helper named createExistingAuthorBansMap that accepts the same listings shape
and returns a Map<string, {reason:string}[]>, then replace the inline
implementations in pcListings.test.ts and admin.test.ts with imports of that
helper so both tests call the single shared function and retain identical
behavior.
- Around line 477-561: The suite only tests the 'risky' riskFilter path; add
tests that exercise the non-risky/default path by calling caller.pending with
riskFilter: 'all' and with riskFilter omitted/undefined, verifying that
mockRepositoryGetPendingListings is used (not
getPendingListingRiskCandidates/getPendingListingsByIds), that returned
pcListings match the mocked pending listings and pagination.total reflects the
count, and add an edge-case test for empty results and a test using a
non-moderator role (e.g., Role.USER) to validate permission-based branches;
reference mocks and helpers like createCaller, caller.pending,
mockRepositoryGetPendingListings, mockRepositoryGetPendingListingRiskCandidates,
mockRepositoryGetPendingListingsByIds, mockComputeAuthorRiskProfiles and
mockComputeSubmissionRiskProfiles to seed expected responses and assertions.
In `@src/server/repositories/pc-listings.repository.ts`:
- Around line 890-894: Extract the duplicated active-ban predicate into a
private helper (e.g., private getActiveBanWhere()) that returns the object {
isActive: true, OR: [{ expiresAt: null }, { expiresAt: { gt: new Date() } }] },
add it to the PCListingsRepository class, and replace the inline userBans.where
in the forList include and the other occurrence (the block using userBans: {
where: { ... } }) to call getActiveBanWhere() so both queries reuse the same
logic for consistency.
🪄 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: e401bb9c-b655-46b0-ae0a-d291c6aea83a
📒 Files selected for processing (32)
next.config.tspackage.jsonsrc/app/admin/approvals/components/ApprovalModal.tsxsrc/app/admin/approvals/page.tsxsrc/app/admin/hooks/index.tssrc/app/admin/hooks/useReviewRiskFilter.tssrc/app/admin/pc-listing-approvals/components/ApprovalModal.tsxsrc/app/admin/pc-listing-approvals/page.tsxsrc/app/global-error.tsxsrc/app/testing/page.tsxsrc/components/admin/ReviewRiskFilterButton.tsxsrc/components/admin/index.tssrc/components/ui/ReviewRiskIndicator.test.tsxsrc/components/ui/ReviewRiskIndicator.tsxsrc/components/ui/ReviewRiskWarningBanner.test.tsxsrc/components/ui/ReviewRiskWarningBanner.tsxsrc/components/ui/index.tssrc/schemas/listing.tssrc/schemas/pcListing.tssrc/schemas/submissionRisk.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/repositories/listings.repository.tssrc/server/repositories/pc-listings.repository.tssrc/server/services/author-risk.service.tssrc/server/services/review-risk.service.test.tssrc/server/services/review-risk.service.tssrc/server/services/submission-risk.service.test.tssrc/server/services/submission-risk.service.tssrc/server/utils/query-builders.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/lib/cors.ts (1)
30-34: 💤 Low valueOptional: use a
Setto dedupe instead of repeatedincludesscans.
addMissingOriginsdoes an O(n)includesper candidate, andgetAllowedOriginscalls it 3–4 times. The list is small so this isn't a real performance issue, but aSet-based accumulator would be both cleaner and avoid the in-place mutation pattern.♻️ Sketch
-function addMissingOrigins(origins: string[], additionalOrigins: string[]) { - for (const origin of additionalOrigins) { - if (!origins.includes(origin)) origins.push(origin) - } -} +function addMissingOrigins(origins: Set<string>, additionalOrigins: readonly string[]) { + for (const origin of additionalOrigins) origins.add(origin) +}Then build
originsas aSet<string>ingetAllowedOriginsand return[...origins]at the end.🤖 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/lib/cors.ts` around lines 30 - 34, The current addMissingOrigins uses repeated origins.includes scans and mutates an array; change the logic to use a Set for deduplication: in getAllowedOrigins create a Set<string> (e.g., allowed = new Set()), add the base origins and then add additional origins with Set.add (or refactor addMissingOrigins to accept and add into a Set rather than an array), and finally return Array.from(allowed) (or [...allowed]); update references to addMissingOrigins/getAllowedOrigins accordingly to remove O(n) includes and in-place array mutation.
🤖 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/lib/cors.test.ts`:
- Around line 21-27: The test titled "allows localhost during CI production-mode
E2E runs" does not stub NODE_ENV so it does not actually exercise
production-mode logic in getAllowedOrigins(); update the test to stub
NODE_ENV='production' (alongside CI='true', NEXT_PUBLIC_ALLOWED_ORIGINS='' and
ALLOWED_ORIGINS='') so getAllowedOrigins() hits the intended production branch
that adds LOCAL_TEST_ORIGINS, or alternatively rename the test to remove the
"production-mode" claim if you prefer not to change environment stubs.
In `@src/lib/cors.ts`:
- Around line 56-58: The current CI-only guard that calls
addMissingOrigins(origins, LOCAL_TEST_ORIGINS) should also verify NODE_ENV to
mirror defensive checks elsewhere; update the condition around addMissingOrigins
(the block that uses process.env.CI) to require process.env.NODE_ENV !==
'production' in addition to process.env.CI === 'true' so it only runs in
non-production CI/test environments and remains consistent with
threeDsGameRefreshService.ts and switchGameRefreshService.ts.
---
Nitpick comments:
In `@src/lib/cors.ts`:
- Around line 30-34: The current addMissingOrigins uses repeated
origins.includes scans and mutates an array; change the logic to use a Set for
deduplication: in getAllowedOrigins create a Set<string> (e.g., allowed = new
Set()), add the base origins and then add additional origins with Set.add (or
refactor addMissingOrigins to accept and add into a Set rather than an array),
and finally return Array.from(allowed) (or [...allowed]); update references to
addMissingOrigins/getAllowedOrigins accordingly to remove O(n) includes and
in-place array mutation.
🪄 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: 07cb3a01-0789-4a1e-a6d8-ad69b493edc9
📒 Files selected for processing (2)
src/lib/cors.test.tssrc/lib/cors.ts
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
Fixes # (issue)
Type of change
How Has This Been Tested?
Screenshots (if applicable)
Checklist
Notes for reviewers
Summary by CodeRabbit
New Features
Improvements
Tests