Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis change introduces a user reporting feature by creating a new ReportModal component and integrating it into the listings detail page. The modal allows non-listing-owners to report listings with predefined reasons and optional notes, submitting reports via a Convex backend mutation. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/convex/listings.ts (1)
19-35:⚠️ Potential issue | 🔴 CriticalCritical: Inverted ownership check breaks
updateListing,updateListingStatus, anddeleteListing.
verifyOwnershipis called byupdateListing(Line 269),updateListingStatus(Line 341), anddeleteListing(Line 351) — all of which require the caller to be the owner. The inverted condition (sellerId === userId→ throw) now allows any authenticated user to update or delete anyone's listing, while blocking the actual owner from modifying their own listing.You need two separate checks: one that enforces ownership (for CRUD operations) and one that enforces non-ownership (for reporting).
🐛 Proposed fix: separate ownership and non-ownership checks
async function verifyOwnership( ctx: MutationCtx, listingId: Id<'listings'> ): Promise<Doc<'listings'>> { const userId = await getAuthUserId(ctx); if (userId === null) { throw new Error('You must be logged in to perform this action'); } const listing = await ctx.db.get(listingId); if (!listing) { throw new Error('Listing not found'); } - if (listing.sellerId === userId) { - throw new Error('Cannot Report Own Listing'); + if (listing.sellerId !== userId) { + throw new Error('You do not have permission to modify this listing'); } return listing; } + +async function verifyNotOwner( + ctx: MutationCtx, + listingId: Id<'listings'> +): Promise<Doc<'listings'>> { + const userId = await getAuthUserId(ctx); + if (userId === null) { + throw new Error('You must be logged in to perform this action'); + } + const listing = await ctx.db.get(listingId); + if (!listing) { + throw new Error('Listing not found'); + } + if (listing.sellerId === userId) { + throw new Error('Cannot report your own listing'); + } + return listing; +}Then use
verifyNotOwnerin the future report mutation, and keepverifyOwnershipforupdateListing,updateListingStatus, anddeleteListing.
🤖 Fix all issues with AI agents
In `@frontend/app/listings/`[id].tsx:
- Line 37: The report menu is shown to unauthenticated users because
isOwnListing is false when currentUser is null; update the rendering and handler
to guard against unauthenticated access: either hide/skip rendering the report
menu entirely when currentUser is null (check currentUser before rendering the
menu component that shows "Report listing") or in the report flow (e.g., the
report button click handler / submit handler) add an explicit check for
currentUser and redirect to login or show a login prompt if missing; reference
the isOwnListing variable, currentUser, listing.sellerId and the report
modal/handler to locate where to add the guard.
- Around line 57-70: Summary: The menu renders an empty View for owners
(isOwnListing true), producing a confusing empty menu. Fix: update the menu
trigger and menu rendering so the "..." button and menuOpen UI only appear when
there are actual actions; either (A) hide the menu trigger when isOwnListing is
true and no owner actions exist by wrapping the trigger render with a condition
(e.g., render the Pressable that toggles menuOpen only when !isOwnListing or
actions.length > 0), or (B) add owner-specific actions inside the menu (e.g.,
include "Edit" and "Delete" entries that call appropriate handlers such as
setEditOpen or a deleteListing handler) so the View is never empty. Locate and
change the elements referencing isOwnListing, menuOpen, setMenuOpen,
setReportOpen and the Pressable that opens the menu to implement one of these
two options.
- Line 20: Remove the leftover debug placeholder comment "// HELP" in the
listings page file; locate the stray comment and delete it so the component /
page source (the file containing the listings [id] page) no longer contains that
unnecessary comment.
In `@frontend/components/ReportModal.tsx`:
- Around line 35-42: handleSubmit currently only awaits a 500ms timeout and
console.logs the report, so no backend call is made and the catch block with
rate-limit detection (and any error handling) is dead; either replace the
simulated sleep with a real API/mutation call (e.g., call the report creation
function or GraphQL mutation used elsewhere and await its promise, pass payload
{ targetId: props.targetId, targetType: props.targetType, reason, notes }, and
preserve the existing catch to handle rate-limit errors) or, if this is
intentionally stubbed, annotate handleSubmit with a clear TODO comment
indicating the missing backend call and why (so reviewers know it’s intentional)
and keep the simulated timeout only for UI feedback; reference handleSubmit, the
500ms Promise, and the existing catch/rate-limit logic when making the change.
- Around line 22-61: The modal preserves notes, reason, and error across
openings; add a local reset function (e.g., resetState) that calls setNotes(''),
setReason(null), setError(null) (and setSubmitting(false) if desired) and invoke
it after a successful submit in handleSubmit and whenever the modal is closed by
the consumer by wrapping props.onClose with a small onClose handler that calls
resetState then props.onClose (or call resetState inside any existing onClose
callback), referencing ReportModal, handleSubmit, notes, reason, error,
setNotes, setReason, and setError to locate the code to change.
🧹 Nitpick comments (3)
frontend/components/ReportModal.tsx (2)
72-73: InnerPressablewith empty handler is a fragile event-swallowing pattern.
onPress={() => {}}prevents the outer backdropPressablefrom closing the modal when the content area is tapped. This works but is non-obvious and fragile. AViewwithpointerEventsor explicitstopPropagationinonPresswould communicate the intent more clearly. Also, the content area lacks a wrappingViewwith styling for the card/dialog appearance (background, border-radius, elevation), which is needed for a modal dialog to look distinct from the backdrop.
64-106: Accessibility: radio buttons and form elements lack semantic roles and labels.The reason options are rendered as plain
Pressable+Textwith unicode radio glyphs (◉/○). Screen readers won't identify these as selectable radio buttons. Consider addingaccessibilityRole="radio",accessibilityState={{ checked: reason === r.label }}, and grouping them withaccessibilityRole="radiogroup". Similarly, the Submit and Cancel buttons should haveaccessibilityRole="button".package.json (1)
61-64: Movereact-native-toast-messageto the frontend workspace.
react-native-toast-messageis only imported and used infrontend/components/ReportModal.tsx, so it should be infrontend/package.jsonrather than the root. While the dependency currently works at the root level, monorepo best practices place dependencies in the workspace that consumes them. This improves clarity of dependencies and maintains proper workspace boundaries.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@frontend/components/ReportModal.tsx`:
- Around line 56-62: In ReportModal.tsx the code lowercases msg into the
variable lower but then checks isUnauth using an uppercase literal ("You must be
logged in to perform this action"), so that branch can never match; update the
check in the isUnauth computation to compare lowercase text consistently (e.g.,
use lower.includes('you must be logged in to perform this action') or call
.toLowerCase() on the literal) and ensure the other includes checks still
operate on the lower variable (isUnauth and isRateLimit should all use
lower.includes(...)).
- Around line 82-84: Backdrop taps and Android back button currently call
props.onClose directly (via onRequestClose and the outer Pressable onPress) and
bypass resetState, leaving stale notes/reason/error; extract a shared close
handler (e.g., handleClose) inside the ReportModal component that calls
resetState() then props.onClose(), and replace both
onRequestClose={props.onClose} and the Pressable onPress={props.onClose} with
onRequestClose={handleClose} and onPress={handleClose} so all dismissal paths
mirror the Cancel button behavior.
🧹 Nitpick comments (2)
frontend/components/ReportModal.tsx (2)
126-131: Modal overlay has no dimming — appears as a plain white page.The
transparentprop on<Modal>makes the underlying view transparent, but the container style uses opaque#fffwith no semi-transparent backdrop. This means users won't perceive it as a modal overlay. A typical pattern is a semi-transparent dark backdrop behind the content card.♻️ Sketch: add an overlay + content card split
const styles = StyleSheet.create({ container: { flex: 1, - backgroundColor: '#fff', - padding: 20, + backgroundColor: 'rgba(0,0,0,0.5)', + justifyContent: 'center', + alignItems: 'center', + }, + card: { + backgroundColor: '#fff', + borderRadius: 12, + padding: 20, + width: '90%', }, });Then wrap the inner content
Pressablewithstyle={styles.card}.
86-93: Radio options and buttons lack accessibility props.The reason
Pressableelements and action buttons have noaccessibilityRoleoraccessibilityLabel. Screen-reader users won't be able to distinguish these as radio buttons or understand their purpose.♻️ Example for reason items
<Pressable key={r.label} onPress={() => setReason(r.label)} + accessibilityRole="radio" + accessibilityState={{ checked: reason === r.label }} + accessibilityLabel={`${r.label}: ${r.desc}`} >Similarly, add
accessibilityRole="button"to the Submit and CancelPressableelements.
|
Created clean companion branch Feedback:
|
jaydonkc
force-pushed
the
feature/POLY-21-modal-UI-component
branch
from
23dae36 to
165039b
Compare
2 participants
Linked Issues
Closes #21
Linear: POLY-16 (e.g., POLY-123)
Summary
Added Report Modal and implementing functionality.
How to Test
Steps to verify locally:
npm run lintnpm run typechecknpm testnpm run dev:backend(in terminal A)npm run dev(in terminal B)Checklist
npm run lint)devScreenshots / Demos
(if UI or visible behavior - attach images, videos, or GIFs)
Summary by CodeRabbit
Release Notes