Conversation
|
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:
📝 WalkthroughWalkthroughThis PR introduces comprehensive tagging and filtering functionality across the marketplace application. Backend changes include tag validation with deduplication and length constraints, enhanced queries supporting category/price/tag filtering, and a new getCurrentUserSubject endpoint. Frontend changes span navigation setup for listing creation/editing, a refactored home page with multi-filter UI, listing detail pages with tag display and edit capabilities, two new listing forms (create and edit), and supporting filter components. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend as Frontend App<br/>(index.tsx)
participant API as Backend API<br/>(listings.ts)
participant DB as Database
User->>Frontend: Selects tags via FilterBar
Frontend->>Frontend: Updates selectedTags state<br/>and URL params
User->>Frontend: Submits filter/loads page
Frontend->>API: getListings(tags, category,<br/>minPrice, maxPrice, limit)
API->>API: validateTags()<br/>validateFilters()
API->>DB: Query by_status index<br/>for active listings
DB->>API: Return active listings
API->>API: Apply tag filter (OR logic)<br/>Apply price/category filters
API->>Frontend: Return filtered listings
Frontend->>Frontend: Update listings state
Frontend->>User: Render ListingCard components<br/>with filtered results
User->>Frontend: Tap tag chip on listing
Frontend->>Frontend: navigateToFeedWithTag()<br/>Update URL with tag
Frontend->>API: getListings with tag param
API->>DB: Query and filter
DB->>API: Return tagged listings
API->>Frontend: Return results
Frontend->>User: Display filtered feed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ 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: 8
🤖 Fix all issues with AI agents
In `@frontend/app/listings/`[id].tsx:
- Around line 62-69: The Edit button is rendered unconditionally; update the JSX
in listings/[id].tsx to conditionally render the TouchableOpacity
(styles.editButton) only when the current user owns the listing (e.g., compare
the authenticated user's id/session user id with listing.sellerId) or has an
admin flag; if your app uses a hook like useSession/useAuth or a currentUser
prop, use that symbol to perform the check before calling
router.push(`/listings/${listing._id}/edit`), and remove or disable the button
for non-owners so they cannot navigate to the edit form.
- Around line 112-126: The tag chips render flush because the tag style (used by
tagContainer/tag/tagText) is missing spacing; update the tag style (the "tag"
style object) to add marginRight: 6 and marginBottom: 4 (matching
ListingCard.tsx's tagChip behavior) so wrapped chips have horizontal and
vertical spacing; keep the existing backgroundColor/padding/borderRadius intact.
In `@frontend/app/listings/`[id]/edit.tsx:
- Around line 37-48: The effect that seeds the form (useEffect with dependency
[listing]) currently resets user edits whenever a new listing object arrives;
modify it to populate only once per loaded listing by introducing a guard (e.g.,
a hasPopulated ref/state) and checking it before calling setTitle,
setDescription, setPrice, setCategory, setCondition, setImages, and setTags;
ensure the guard resets when the listing identity changes (e.g., compare
listing.id or set hasPopulated = false on listing id change) so the form still
seeds for different listings but won't overwrite in-progress edits from live
updates.
- Line 103: The current ternary (tags: tags.length > 0 ? tags : undefined) hides
an empty array and prevents clearing tags; change it to always pass the tags
array (e.g., tags: tags) so an empty [] is sent to the backend and the handler
can clear existing tags; update the assignment in edit.tsx where the payload is
built (the variable named tags) to remove the undefined branch.
In `@frontend/app/listings/new.tsx`:
- Around line 49-76: In handleSubmit, trim the title once into a variable (e.g.,
trimmedTitle) and use that for validation and for the submission payload so
checks and submitted value match; update the title length check to use
trimmedTitle.length and replace any usage of raw title with trimmedTitle when
building the data sent to the backend (refer to handleSubmit and where the
payload is assembled).
In `@frontend/components/TagInput.tsx`:
- Around line 61-88: In handleInputChange, when splitting on comma and computing
tagToAdd you currently only handle length and maxTags but not duplicates, so a
duplicate (tagToAdd where tags.includes(tagToAdd) is true) leaves the comma in
the input and no feedback; update the conditional branches in handleInputChange
to explicitly handle the duplicate case (tags.includes(tagToAdd)) by calling
setInputText(parts.slice(1).join(',') || '') to remove the consumed token and
setError('Tag already added' or similar) so the input is cleared and the user
gets feedback; ensure you reference tagToAdd, tags, setInputText, and setError
in the fix.
In `@frontend/components/TagPicker.tsx`:
- Around line 20-26: handleAddTag currently only trims the input but does not
lowercase it, causing mismatches with saved/lowercased tags; change the logic in
handleAddTag to compute a normalized value (e.g., const normalized =
trimmed.toLowerCase()), check against inputTags using that normalized value
(e.g., !inputTags.includes(normalized)), and add the normalized tag to state via
setInputTags([...inputTags, normalized]) while still clearing the search text
with setSearchText(''). This ensures consistency with TagInput's lowercase
behavior and fixes filtering in getListings.
- Around line 17-18: The local state inputTags is initialized from selectedTags
only at mount which makes it stale if the parent changes selection while the
modal is hidden; update inputTags whenever the modal is opened or selectedTags
changes by importing useEffect and adding an effect that watches visible and
selectedTags and calls setInputTags(selectedTags) when visible becomes true (or
when selectedTags changes while visible), ensuring the TagPicker's inputTags
stays in sync with the external selectedTags.
🧹 Nitpick comments (6)
backend/convex/schema.ts (1)
28-28: Schema addition looks correct.
v.optional(v.array(v.string()))is the right choice for an optional tags field. Worth noting: there is no index ontags, so tag-based filtering ingetListingswill require a full scan (or post-filter) of the result set. This is fine at low volume but consider adding an index if tag filtering becomes a hot path.backend/convex/__tests__/schema.test.ts (1)
14-29: Weakened assertion no longer verifies the full field set.Switching from
.toEqual([...])toexpect.arrayContaining(...)means this test will pass even if unexpected fields are added to the schema. The previous strict check was more valuable — it would catch accidental additions.Since
tagsjust needs to be in the list, consider keeping strict equality:♻️ Proposed fix — keep strict equality
- expect(fields).toEqual( - expect.arrayContaining([ - 'title', - 'description', - 'price', - 'sellerEmail', - 'sellerId', - 'images', - 'condition', - 'category', - 'status', - 'createdAt', - 'postedOn', - 'tags', - ]) - ); + expect(fields.sort()).toEqual([ + 'category', + 'condition', + 'createdAt', + 'description', + 'images', + 'postedOn', + 'price', + 'sellerEmail', + 'sellerId', + 'status', + 'tags', + 'title', + ]);frontend/app/listings/[id]/edit.tsx (1)
1-251: Significant code duplication withnew.tsx.The form structure, validation logic, image handlers (
handleAddImage,handleImageChange,handleRemoveImage), and the entireStyleSheetare nearly identical betweenedit.tsxandnew.tsx. Consider extracting shared pieces (aListingFormcomponent, shared styles, shared validation helper) to reduce duplication.frontend/app/index.tsx (1)
11-27: Consider the dual source of truth between URL params and local state.
selectedTagsis initialized from URL params viauseEffect, butFilterBarupdates only the local state (not the URL). This means:
- Tag selections made via
FilterBarwon't survive navigation (back/forward).- If another part of the app navigates here with
?tags=..., theuseEffectwill overwrite any in-progress filter selections.This is acceptable for an initial implementation, but worth noting if deep-link consistency or browser-back behavior matters later.
backend/convex/listings.ts (2)
42-57: Consider normalizing and deduplicating tags server-side.The frontend
TagInputlowercases tags, but a direct API call (or theTagPickerin its current state) could submit mixed-case or duplicate tags. Adding server-side normalization provides defense-in-depth.Proposed enhancement
function validateTags(tags: string[] | undefined) { if (tags === undefined) return; + // Normalize: lowercase and deduplicate + const seen = new Set<string>(); + for (let i = 0; i < tags.length; i++) { + tags[i] = tags[i].trim().toLowerCase(); + if (seen.has(tags[i])) { + throw new Error('Duplicate tags are not allowed'); + } + seen.add(tags[i]); + } + if (tags.length > 5) { throw new Error('Maximum 5 tags allowed'); }Note: Mutating the input array in-place keeps it simple for Convex handlers since
argsis consumed immediately. Alternatively, return a normalized copy if purity is preferred.
59-79: Tag filtering fetches all active listings into memory.The OR-logic filter on Lines 70–74 loads every active listing and filters in JS. This is fine for early-stage usage, but will degrade as the listings table grows. Convex doesn't natively support array-contains index queries, so this is a known trade-off.
If this becomes a bottleneck, consider a separate
listing_tagstable (one row per listing–tag pair) with aby_tagindex, then join back to listings.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@frontend/app/listings/`[id]/edit.tsx:
- Around line 78-85: In handleSubmit, the client-side length check uses the
untrimmed title which can allow whitespace-padded values to pass but then fail
server validation; change the validation to use a trimmedTitle (e.g., const
trimmedTitle = title.trim()) and validate trimmedTitle.length >= 5, and ensure
the submission payload sends trimmedTitle instead of raw title so the same value
validated is what gets submitted (update references in handleSubmit and the
payload construction).
In `@frontend/components/TagInput.tsx`:
- Around line 96-108: The TextInput's maxLength prop is currently limiting the
entire input string (maxLength={maxLength}), which truncates pasted multi-tag
strings; remove or increase that prop so pasted comma-separated tags aren't
silently cut off. Update the TagInput component by removing the maxLength prop
on the TextInput (or set it to a much larger value), relying on the existing
per-tag validation in handleInputChange and handleAddTag to enforce the per-tag
limit; ensure inputText, handleInputChange, and handleAddTag continue to
validate each tag individually after paste.
🧹 Nitpick comments (2)
backend/convex/listings.ts (2)
42-57: Consider normalizing and deduplicating tags server-side.The frontend lowercases and trims tags, but the backend only validates constraints without normalizing. If the mutation is called directly (e.g., Convex dashboard, tests, another client), tags like
" Desk "or"DESK"would be stored as-is, potentially causing filter mismatches sincegetListingsuses strict equality via.includes().Also, duplicate tags in the array aren't rejected.
♻️ Proposed fix — normalize + deduplicate
function validateTags(tags: string[] | undefined) { if (tags === undefined) return; + // Normalize: trim and lowercase + for (let i = 0; i < tags.length; i++) { + tags[i] = tags[i].trim().toLowerCase(); + } + + // Deduplicate + const unique = new Set(tags); + if (unique.size !== tags.length) { + throw new Error('Duplicate tags are not allowed'); + } + if (tags.length > 5) { throw new Error('Maximum 5 tags allowed'); } for (const tag of tags) { if (tag.length > 20) { throw new Error('Tags must be 20 characters or less'); } if (tag.trim().length === 0) { throw new Error('Tags cannot be empty'); } } }Alternatively, mutate in-place to normalize rather than rejecting duplicates, returning the cleaned array for the caller to use.
60-79: In-memory tag filtering loads all active listings before filtering.
getListingsfetches every active listing via.collect()and then applies the tag filter in JS. This is fine for a small dataset but will degrade as the number of active listings grows. Worth keeping in mind if the listing count scales significantly.For now this is acceptable since Convex doesn't natively support array-contains indexes, but consider documenting this as a known scalability limitation.
SamanSP1386
left a comment
SamanSP1386
left a comment
There was a problem hiding this comment.
Many of the issues were solved but there is still some potential issues recommended by the ai bot. Take a look at them.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/components/TagInput.tsx`:
- Around line 84-88: The error branches for TagInput (when tagToAdd.length >
maxLength or tags.length >= maxTags) set errors but do not clear the input,
leaving stale comma-separated text; update those branches in the TagInput
component to also call setInputText('') (the same way the duplicate-tag branch
does) after setting the error so the consumed portion is cleared; modify the
branches that check maxLength and maxTags (referencing tagToAdd, maxLength,
maxTags, tags, setError, setInputText) to clear the input consistently.
🧹 Nitpick comments (2)
frontend/app/listings/[id]/edit.tsx (2)
22-25: Unsafe cast of route paramid.
useLocalSearchParams()returnsstring | string[]for each param. Casting directly toId<'listings'>without a guard means an array value (e.g., from a malformed URL) would silently produce an invalid query argument. The loading/null checks downstream mitigate the blast radius, but a guard would be more defensive.Suggested guard
- const { id } = useLocalSearchParams(); - const listing = useQuery(api.listings.getListing, { - id: id as Id<'listings'>, - }); + const { id } = useLocalSearchParams(); + const listingId = typeof id === 'string' ? id : undefined; + const listing = useQuery( + api.listings.getListing, + listingId ? { id: listingId as Id<'listings'> } : 'skip' + );
219-237:key={index}on image inputs — works but fragile.Using array index as
keyfor the image URL inputs means React may reuse the wrong input instance when an item is removed from the middle of the list (e.g., removing image 2 of 4 could leave stale focus/cursor state). Since the values are fully controlled state this won't cause data bugs, but using a stable key (e.g., a counter-based ID assigned on add) would be more robust.
SamanSP1386
left a comment
SamanSP1386
left a comment
There was a problem hiding this comment.
There is still one warning left. Check the ai bot and make sure it is solved
- Add tag filtering to getListings query (OR logic) - Add category and price range filters - Create unified FilterBar with all three filter types - Add CategoryPicker and PriceRangePicker components - Support URL params for tag deep linking - Add comprehensive validation for price inputs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/app/index.tsx`:
- Around line 22-30: The effect that syncs URL `tags` into state (useEffect ->
setSelectedTags) rehydrates cleared UI filters because the URL param isn't
removed; update the clear flow (handleClearTags) to also remove the URL param
(e.g., call router.setParams({ tags: undefined }) or equivalent) when the user
clears tags, and/or change the effect to only run on mount by guarding with a
mounted flag so it doesn't overwrite user-cleared state on subsequent
navigations; ensure you reference and update the existing useEffect,
setSelectedTags, and handleClearTags handlers.
🧹 Nitpick comments (5)
backend/convex/__tests__/schema.test.ts (1)
14-29: Redundant assertion —arrayContainingis subsumed by the exacttoEqualon lines 30-43.Both assertions compare against the identical field list. Because lines 30-43 already assert strict equality (order + contents), the weaker
arrayContainingcheck can never fail independently. You can safely remove one of the two:
- Keep only
arrayContainingif you want the test to be resilient to new fields being added later.- Keep only the exact
toEqualif you want to lock down the full set and order.Having both with the same list adds no additional coverage.
frontend/components/CategoryPicker.tsx (1)
12-19: Consider deriving categories from the sharedCATEGORY_LABELSinFilterBar.tsxto avoid duplication.Both this file and
FilterBar.tsx(lines 25-31) independently enumerate the same set of category values and labels. If a new category is added to one but not the other, they'll silently diverge.♻️ Suggested approach
Export
CATEGORY_LABELSfromFilterBar.tsx, then buildCATEGORIEShere dynamically:import { CATEGORY_LABELS, type Category } from './FilterBar'; const CATEGORIES: { value: Category | undefined; label: string }[] = [ { value: undefined, label: 'All Categories' }, ...Object.entries(CATEGORY_LABELS).map(([value, label]) => ({ value: value as Category, label, })), ];frontend/components/PriceRangePicker.tsx (1)
39-45: useEffect dependency onminPrice/maxPricemay reset user input mid-edit.If the parent component re-renders while the modal is open (e.g., due to unrelated state changes that cause new object references), and
minPrice/maxPriceare included as deps, the user's typed input could be overwritten. Consider gating the reset on avisibletransition (false → true) rather than includingminPrice/maxPricein the dependency array.♻️ Suggested fix using a ref to track previous visibility
-import React, { useState, useEffect } from 'react'; +import React, { useState, useEffect, useRef } from 'react';+ const prevVisible = useRef(false); + useEffect(() => { - if (visible) { + if (visible && !prevVisible.current) { setMin(minPrice?.toString() ?? ''); setMax(maxPrice?.toString() ?? ''); setError(''); } - }, [visible, minPrice, maxPrice]); + prevVisible.current = visible; + }, [visible, minPrice, maxPrice]);frontend/components/FilterBar.tsx (2)
156-167:padding: 0overridespaddingHorizontalandpaddingVertical— dead style properties.In React Native,
paddingtakes precedence over the axis-specific variants when both are present. Lines 159-160 (paddingHorizontal: 14,paddingVertical: 8) have no effect becausepadding: 0on line 165 resets all sides. The chip padding actually comes fromchipMainButton.Remove the dead properties to avoid confusion:
🧹 Suggested fix
chip: { flexDirection: 'row', alignItems: 'center', - paddingHorizontal: 14, - paddingVertical: 8, borderRadius: 20, backgroundColor: '#f0f0f0', borderWidth: 1, borderColor: '#e0e0e0', - padding: 0, // Reset padding as inner elements handle spacing + padding: 0, overflow: 'hidden', },
5-11: Consider co-locating shared types in a dedicated file.
CategoryandFiltersare exported from this UI component but consumed across multiple files (CategoryPicker.tsx,index.tsx, etc.). As more components depend on these types, a shared types file (e.g.,types/filters.ts) would provide a cleaner dependency direction — filter types shouldn't require importing a UI component.
| useEffect(() => { | ||
| if (tags) { | ||
| const tagArray = Array.isArray(tags) ? tags : [tags]; | ||
| setSelectedTags(tagArray); | ||
| } else { | ||
| // Reset tags when navigating without tag params | ||
| setSelectedTags([]); | ||
| } | ||
| }, [tags]); |
There was a problem hiding this comment.
Tags cleared via UI may reappear if the URL tags param persists.
When a user arrives via a deep link (e.g., ?tags=textbooks), the useEffect correctly populates selectedTags. However, if the user then clears tags through the filter UI (handleClearTags), the URL search param tags is never removed. On the next trigger of this effect (e.g., component remount or navigation), the cleared tags will be restored from the stale URL param.
Consider calling router.setParams({ tags: undefined }) (or equivalent) when the user clears tags, or guard the effect so it only runs on mount.
🤖 Prompt for AI Agents
In `@frontend/app/index.tsx` around lines 22 - 30, The effect that syncs URL
`tags` into state (useEffect -> setSelectedTags) rehydrates cleared UI filters
because the URL param isn't removed; update the clear flow (handleClearTags) to
also remove the URL param (e.g., call router.setParams({ tags: undefined }) or
equivalent) when the user clears tags, and/or change the effect to only run on
mount by guarding with a mounted flag so it doesn't overwrite user-cleared state
on subsequent navigations; ensure you reference and update the existing
useEffect, setSelectedTags, and handleClearTags handlers.
|
Only one more warning left, Tags cleared via UI may reappear if the URL tags param persists. |
- Remove redundant arrayContaining assertion in schema test - Extract shared filter types to frontend/types/filters.ts - Remove duplicate CATEGORIES from CategoryPicker - Fix PriceRangePicker useEffect to only reset on modal open - Remove dead padding properties from chip style - Fix URL param sync in handleTagsChange
3 participants
Linked Issues
Add tag filter UI and display tags on listings add tags and Add tag input to listing create and edit forms
Closes #
Linear: POLY-28 POLY-17 POLY-27
Summary
Briefly explain the change and why.
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