Implement tag based filtering in listing queries#34
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:
📝 WalkthroughWalkthroughReplaces the no-arg getListings with a filterable, paginated getListings(category, minPrice, maxPrice, tags, limit, cursor), adds tag normalization and validation, introduces getCurrentUserSubject, expands listings tests for many filter combinations, and updates frontend hook/useQuery usage and return shape to include isLoading. Also adds TypeScript Babel preset and adjusts Jest transforms. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant FrontendHook as Frontend Hook
participant Backend as Convex getListings
participant DB as Convex DB
Client->>FrontendHook: request listings (filters, cursor, limit)
FrontendHook->>Backend: call getListings(args)
Backend->>DB: query by_status_category (if category) or by_status
DB-->>Backend: listing records
Backend->>Backend: normalize tags, validate prices, filter by isHidden/tags/price, sort newest, apply cursor/limit
Backend-->>FrontendHook: listings page + nextCursor
FrontendHook-->>Client: listings, isLoading=false
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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: 1
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)
62-62:⚠️ Potential issue | 🟡 MinorTypo in comment: "tages" → "tags".
-// Normalize tages to lowercase and within 1-20 characters exclusive +// Normalize tags to lowercase and within 1-20 characters exclusive
🤖 Fix all issues with AI agents
In `@backend/convex/listings.ts`:
- Around line 220-249: Replace the unsafe double-cast and remove the runtime
fallback heuristic: when querying the 'by_tag' index in the block that iterates
normalizedTags (use the listingMap / tagResults logic), stop using "tag as
unknown as Doc<'listings'>['tags']" and instead pass a properly-typed value
(e.g., an array containing the tag if the index expects string[] or the exact
element form the index schema requires) so TypeScript reflects the real index
type; then remove the "if (results.length === 0) { mustFilterTagsInMemory =
true; ... }" fallback entirely (delete the mustFilterTagsInMemory flag usage and
the branch that loads all active listings via 'by_status_category'/'by_status'),
and simply return the listingMap results (or an empty array) when no matches are
found; ensure tests or deployment checks cover whether array-index element
matching is supported rather than attempting to detect it at runtime.
🧹 Nitpick comments (3)
backend/convex/__tests__/listings.test.ts (1)
364-432: Consider splitting into individual test cases for better failure diagnostics.All seven filter scenarios (single tag, multiple tags, tag+category, tag+price, combined, case-insensitive, no-match) run sequentially in a single test. If an early assertion fails, the remaining scenarios are skipped, and the test name doesn't tell you which combination broke. Separate
it()blocks would improve debuggability and CI output.That said, the coverage itself is solid and the
ids()helper for order-independent comparison is a nice touch.backend/convex/listings.ts (2)
264-273: Redundant in-memory category filter after index-based retrieval.When
normalizedTagsis empty andargs.categoryis set, the code already fetches viaby_status_categoryindex (lines 251–256), then re-filters by category at line 272. The second pass is a no-op in that branch.This is harmless but worth noting for clarity—a comment or restructuring the filter block to skip already-indexed predicates would make intent clearer.
284-295: Offset-based cursor pagination is fragile with mutable data.Using a numeric offset as a "cursor" means that if listings are inserted or deleted between paginated calls, results can shift—causing duplicates or skipped items. This is a known limitation of offset pagination versus keyset/token-based pagination.
This may be acceptable for now, but worth noting as a future improvement, especially since
searchAndFilterListings(line 184–194) has the same pattern.
SamanSP1386
left a comment
SamanSP1386
left a comment
There was a problem hiding this comment.
There is some warnings and issues, check the ai bot and make sure all of them are solved.
…ent-tag-based-filtering-in-listing-queries
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@backend/convex/listings.ts`:
- Around line 265-276: The current pagination logic (using args.cursor,
startIndex, limit, endIndex and results.slice) returns only the sliced array and
must be changed to return pagination metadata; update the function (same one
using startIndex/limit/endIndex and referenced alongside
searchAndFilterListings) to return an object { items: ..., nextCursor:
string|null, hasMore: boolean } by computing hasMore = endIndex < results.length
and nextCursor = hasMore ? String(endIndex) : null (or undefined) while keeping
items = results.slice(startIndex, endIndex); ensure callers are updated to
expect this object instead of a raw array.
- Around line 73-81: normalizeSearchTags currently returns an unbounded number
of tags which can trigger one DB index query per tag; change normalizeSearchTags
(and its pipeline) to apply the same maximum tag count as normalizeTags by
trimming/deduplicating/mapping/filtering as you already do and then slicing the
result to TAG_CONSTRAINTS.MAX_TAGS (or the constant used for the 5-tag cap)
before returning, so callers like getListings cannot cause unbounded sequential
DB queries.
🧹 Nitpick comments (2)
backend/convex/listings.ts (1)
219-232: Tag index path fetches listings of all statuses before filtering.The
by_tagindex query (lines 222–228) does not include a status constraint, so it loads inactive, sold, and deleted listings into memory before the status filter on line 250 discards them. At scale this wastes reads. If the index can be extended to include status (e.g., aby_tag_statuscomposite index), it would avoid fetching non-active documents.backend/convex/__tests__/listings.test.ts (1)
363-434: Skipped test provides no CI coverage — consider tracking when to enable it.The test is well-structured and covers important filter combinations (single tag, multi-tag OR, tag+category, tag+price, case insensitivity, no matches). However, since it's permanently skipped, it provides zero regression protection and may silently drift out of sync with the implementation. Consider adding a TODO/issue reference so it's unskipped once
convex-testsupports array-index element matching, or write a parallel integration test that runs against a real backend.Would you like me to open an issue to track enabling this test?
| // Apply cursor/limit pagination (if provided) | ||
| let startIndex = 0; | ||
| if (args.cursor) { | ||
| const cursorIndex = parseInt(args.cursor, 10); | ||
| if (!isNaN(cursorIndex) && cursorIndex >= 0) { | ||
| startIndex = cursorIndex; | ||
| } | ||
| } | ||
|
|
||
| const limit = args.limit && args.limit > 0 ? args.limit : undefined; | ||
| const endIndex = limit !== undefined ? startIndex + limit : results.length; | ||
| return results.slice(startIndex, endIndex); |
There was a problem hiding this comment.
Pagination args are accepted but the response doesn't include pagination metadata.
cursor and limit are accepted as args, but the return value is just a bare array — no nextCursor or hasMore is returned. Callers have no way to know whether more pages exist, making the pagination unusable in practice. Either remove the pagination args or return pagination metadata like searchAndFilterListings does.
🤖 Prompt for AI Agents
In `@backend/convex/listings.ts` around lines 265 - 276, The current pagination
logic (using args.cursor, startIndex, limit, endIndex and results.slice) returns
only the sliced array and must be changed to return pagination metadata; update
the function (same one using startIndex/limit/endIndex and referenced alongside
searchAndFilterListings) to return an object { items: ..., nextCursor:
string|null, hasMore: boolean } by computing hasMore = endIndex < results.length
and nextCursor = hasMore ? String(endIndex) : null (or undefined) while keeping
items = results.slice(startIndex, endIndex); ensure callers are updated to
expect this object instead of a raw array.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@backend/convex/listings.ts`:
- Around line 237-239: The in-memory category filter using args.category on the
results array is redundant because the earlier index query already filters by
status='active' and category; remove the block that does results =
results.filter((l) => l.category === args.category) (the conditional that checks
args.category and re-filters results) so the function returns the query results
directly without the extra in-memory check.
🧹 Nitpick comments (2)
backend/convex/__tests__/listings.test.ts (2)
363-432: Solid filter coverage, but consider adding pagination and edge-case tests.The test comprehensively covers tag, category, and price filter combinations, including case insensitivity and no-match scenarios. A few gaps:
- No test for
cursor/limitpagination — these args are part of the API surface but untested.- No test with empty args
{}— verifying all listings are returned when no filters are applied.- No test for
minPricewithoutmaxPrice(and vice versa already tested).These can be deferred but would strengthen confidence in the query behavior.
400-400: Nit: type theidshelper more precisely.The inline
Array<{ _id: string }>type loses the actual ConvexIdtype. Consider usingDoc<'listings'>or at least{ _id: any }[]since.sort()on Convex IDs may not produce meaningful ordering (they're opaque strings). The tests pass because both sides are sorted the same way, so this is cosmetic.
- Merge tag filtering tests with basic filtering tests from dev - Add @babel/preset-typescript to babel.config.js for proper TS support - Configure jest to use ts-jest for .ts files and babel-jest for .js files - Add price validation to getListings (minPrice/maxPrice checks) - Add getCurrentUserSubject query from dev branch - Fix test ordering by using _creationTime instead of createdAt - All 55 tests now passing
There was a problem hiding this comment.
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)
62-62:⚠️ Potential issue | 🟡 MinorTypo in comment: "tages" → "tags".
Fix
-// Normalize tages to lowercase and within 1-20 characters exclusive +// Normalize tags to lowercase and within 1-20 characters exclusive
🧹 Nitpick comments (2)
backend/convex/listings.ts (1)
73-81: No upper bound on search tags — but impact is now minimal.The previous review flagged the lack of a cap here. Since tags are now only used for in-memory filtering (not individual index queries), the performance risk is greatly reduced. Still, adding a reasonable cap (e.g., 10) is cheap insurance against abuse and keeps parity with
normalizeTags's limit of 5.Proposed fix
function normalizeSearchTags(tags: string[]): string[] { - return [ + const MAX_SEARCH_TAGS = 10; + return [ ...new Set( tags .map((tag) => tag.trim().toLowerCase()) .filter((tag) => tag.length >= 1 && tag.length <= TAG_CONSTRAINTS.MAX_TAG_LENGTH) ), - ]; + ].slice(0, MAX_SEARCH_TAGS); }backend/convex/__tests__/listings.test.ts (1)
486-500: Consider adding a test for negativemaxPrice.The implementation validates
maxPrice < 0separately (listings.ts lines 234-236), but only negativeminPriceis tested here. Adding arejects invalid maxPricetest case would improve coverage of that branch.
…-in-listing-queries
- Remove duplicate getListings export (kept complete version with tag filtering) - Remove duplicate getCurrentUserSubject export - Remove unused normalizeTags function - All tests passing, linting clean
- Remove unfiltered listings query on line 13 - Keep filtered listings query that respects filter state - Fixes TypeScript error: Cannot redeclare block-scoped variable
3 participants
Linked Issues
Closes #26
Linear: POLY-26
Summary
Adds tag-based filtering to getListings, while preserving existing category/price filters. Updates useQuery calls to pass an empty args object and adds query tests covering tag filter combinations.
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
N/A
Summary by CodeRabbit
New Features
UX
Tests
Breaking Changes