Conversation
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds messaging data model and APIs, listing image upload and tags support, frontend create/edit listing screens with image upload UI and URL resolution hook, Jest testing/mocks and config, and Babel/TypeScript tooling updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (App)
participant Server as Convex Server (API)
participant DB as Database
participant Storage as Storage Service
Client->>Server: listUserConversations(limit, cursor)
Server->>DB: Query conversationParticipants by userId ordered by lastActivityAt
DB-->>Server: participant rows
Server->>DB: Fetch conversations & lastMessage(s)
DB-->>Server: conversation metadata + messages
Server->>DB: Aggregate unread counts per participant
DB-->>Server: unread counts
Server-->>Client: Paginated conversation list with previews & unread counts
sequenceDiagram
participant Client as Client (App)
participant Server as Convex Server (API)
participant Storage as Storage Service
participant DB as Database
Client->>Server: generateListingImageUploadUrl()
Server->>Storage: Request signed upload URL
Storage-->>Server: Signed upload URL
Server-->>Client: Return upload URL
Client->>Client: Compress & upload image to Storage (XHR, progress)
Storage-->>Client: Return storageId
Client->>Server: createListing(...imageStorageIds, tags, sellerEmail)
Server->>DB: Persist listing with images, tags, sellerEmail
DB-->>Server: Confirm persistence
Server-->>Client: Success response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (9)
frontend/app/index.tsx (1)
29-44:FlatListcard duplicatesListingCardand silently drops tag rendering.The inline
renderItemre-implements the same title/price/description card that already exists infrontend/components/ListingCard.tsx, but without the tag chips. Users will see tags on the detail page but not on the home list.♻️ Proposed refactor to use
ListingCard+import ListingCard from '../components/ListingCard'; <FlatList data={listings} keyExtractor={(item) => item._id} - renderItem={({ item }) => ( - <TouchableOpacity - style={styles.listingCard} - onPress={() => router.push(`/listings/${item._id}`)} - > - <Text style={styles.listingTitle}>{item.title}</Text> - <Text style={styles.listingPrice}>${item.price}</Text> - <Text style={styles.listingDescription}>{item.description}</Text> - </TouchableOpacity> - )} + renderItem={({ item }) => <ListingCard listing={item} />} contentContainerStyle={styles.listContainer} />The
listingCard,listingTitle,listingPrice, andlistingDescriptionstyles can then be removed from this file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/index.tsx` around lines 29 - 44, Replace the inline renderItem in the FlatList with the existing ListingCard component so the list reuses centralized rendering (use FlatList's renderItem={({item}) => <ListingCard listing={item} onPress={() => router.push(`/listings/${item._id}`)} />} ), ensuring ListingCard renders tag chips so tags appear on the home list as well; after switching, remove the now-unused styles listingCard, listingTitle, listingPrice, and listingDescription from this file to avoid duplication.backend/convex/tsconfig.json (1)
14-14: Jest types leak into production Convex function scope.Because
"exclude"already contains./__tests__, thistsconfig.jsondoes not compile test files — the"jest"entry intypesdoesn't help the test suite get typed, but it does inject Jest globals (describe,expect,test,jest, etc.) into the type namespace of every production Convex function. A strayexpect(…)call in a mutation would typecheck silently.The idiomatic fix is a dedicated
tsconfig.jsoninside__tests__/that extends the root Convex config and scopes the Jest types to test code only.♻️ Proposed fix
Revert the change in
backend/convex/tsconfig.json:- "types": ["jest"], + "types": [],Create
backend/convex/__tests__/tsconfig.json:{ "extends": "../tsconfig.json", "compilerOptions": { "types": ["jest"] }, "include": ["./**/*"] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/convex/tsconfig.json` at line 14, Remove the "types": ["jest"] entry from backend/convex/tsconfig.json so Jest globals are no longer injected into production Convex functions, and instead add a new tsconfig at backend/convex/__tests__/tsconfig.json that extends the root Convex tsconfig (extends: "../tsconfig.json"), sets compilerOptions.types to ["jest"], and includes the test files (include: ["./**/*"]) so Jest types are scoped only to the test folder.backend/convex/__tests__/messages.test.ts (2)
57-134: Tests cover the happy path and auth, but lack pagination and sendMessage coverage.Consider adding tests for:
- Cursor-based pagination (verifying
nextCursoris populated when results exceedlimit)sendMessagemutation (message insertion, participant row creation/updates, auth failures)- Conversations where the referenced document is null (deletion scenario)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/convex/__tests__/messages.test.ts` around lines 57 - 134, Tests for listUserConversationsHandler miss pagination and sendMessage mutation coverage; add unit tests that (1) verify cursor-based pagination by seeding participantRows/dummy conversations exceeding the default limit and asserting result.nextCursor is non-null and subsequent call with that cursor returns the next page, referencing listUserConversationsHandler and nextCursor; (2) exercise sendMessage mutation to assert message document insertion, creation/update of participantRows, and auth failures by calling sendMessage with valid ctx.userId and with null userId and checking participantRows and message docs (refer to sendMessage and participantRows); and (3) include a test where a conversation's referenced doc is null (simulate deleted conversation in docsById) and assert the handler skips or handles it gracefully (reference listUserConversationsHandler and docsById).
105-134: Unread count test only verifies passthrough of mock data, not actual computation.The
unreadCountis set to1in the fixture and asserted as1in the result. This doesn't exercise the increment logic insendMessage. Consider testingsendMessagewith a mock context to verify thatunreadCountis incremented for non-sender participants and unchanged for the sender.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/convex/__tests__/messages.test.ts` around lines 105 - 134, Current test only asserts passthrough of fixture unreadCount; update the test to exercise sendMessage logic: use buildCtx to create a context with multiple participants (including the sender and at least one other), call sendMessage(...) with the sender as the actor, then call listUserConversationsHandler(...) (or read the conversation via the same read path used in production) and assert that the non-sender participant's conversation.unreadCount increased while the sender's unreadCount stayed the same; reference sendMessage, listUserConversationsHandler, and buildCtx to locate and change the test setup and assertions.backend/convex/messages.ts (1)
80-81: Handler usesanyfor bothctxandargs, losing all type safety.Extracting the handler for testability is fine, but consider typing the parameters (e.g., using
QueryCtxfrom Convex and an explicit args interface) and adjusting the test mocks to conform. This avoids silent breakage when the handler's expectations evolve.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/convex/messages.ts` around lines 80 - 81, The exported testable handler listUserConversationsHandler currently types both parameters as any; change its signature to use Convex's QueryCtx (or the appropriate ctx type) for the first parameter and define an explicit args interface (e.g., ListUserConversationsArgs) describing expected fields for the second parameter, then update any tests/mocks to implement those shapes so TypeScript can verify calls and prevent silent breakage when the handler's expectations change.backend/convex/schema.ts (1)
48-55: Consider using a union type formessages.typeinstead of openv.string().Currently
type: v.string()allows any string, but the codebase only uses'text'(seemessages.tsline 35). A union (e.g.,v.union(v.literal('text'), v.literal('image'))) would give you schema-level validation and prevent invalid message types from being persisted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/convex/schema.ts` around lines 48 - 55, The messages table currently defines type as an open string (messages: defineTable ... type: v.string()) which permits any value; replace that with a union of allowed literals to enforce schema-level validation (for example change the type validator in messages to a union of v.literal('text') and any other supported types like v.literal('image') or just v.literal('text') if only text messages are used) so invalid message types cannot be persisted; update the validator in the messages table definition (and adjust any imports if needed) to use v.union and v.literal accordingly.backend/jest.config.js (1)
13-14: Consider removing.jsfromextensionsToTreatAsEsmfor clarity.The configuration
extensionsToTreatAsEsm: ['.ts', '.js']tells Jest to treat.jsfiles as ES modules, butbackend/convex/__mocks__/server.jsuses CommonJS syntax (module.exports). While ts-jest'sesModuleInterop: truesetting should handle the interop, including.jsin this array is non-standard and creates unnecessary fragility. Best practice is to only include.tsfiles here. Either remove.jsfrom the array or convert the mock to ESM syntax for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/jest.config.js` around lines 13 - 14, Update the Jest config so we only treat TypeScript files as ESM: remove '.js' from the extensionsToTreatAsEsm array in jest.config.js (symbol: extensionsToTreatAsEsm) or alternatively convert the CommonJS mock backend/convex/__mocks__/server.js to ESM (replace module.exports with export default) so the setting and mock format are consistent; pick one approach and apply it across the repo to avoid mixed ESM/CommonJS behavior.frontend/app/listings/[id].tsx (2)
3-3: Duplicateexpo-routerimport — merge into one
useLocalSearchParamsanduseRouterare both fromexpo-routerbut split across two import statements.♻️ Proposed fix
-import { useLocalSearchParams } from 'expo-router'; +import { useLocalSearchParams, useRouter } from 'expo-router'; import { useConvex, useQuery } from 'convex/react'; import { api } from 'convex/_generated/api'; import { Id } from 'convex/_generated/dataModel'; -import { useRouter } from 'expo-router';Also applies to: 7-7
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/listings/`[id].tsx at line 3, There are duplicate imports from 'expo-router' — merge the separate imports of useLocalSearchParams and useRouter into a single import statement (e.g., import { useLocalSearchParams, useRouter } from 'expo-router') so both hooks are imported together; update any other occurrences (e.g., the other import on line with useRouter) to remove the redundant import and keep only the unified import.
12-12: Duplicated image-URL resolution logic — consider a shared hook
ImageUploaderalready resolves storage IDs to URLs viauseQuery(api.listings.getListingImageUrl, ...). This component reimplements the same mapping imperatively usinguseConvex+Promise.all+useEffect. A small shared hook (e.g.,useResolvedImageUrls(ids: string[])) would eliminate the duplication and give both components Convex's reactive cache for free.Also applies to: 26-40
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/listings/`[id].tsx at line 12, The component duplicates image-URL resolution logic by using useConvex + Promise.all + useEffect instead of reusing ImageUploader's query; extract the shared logic into a new hook (e.g., useResolvedImageUrls(ids: string[])) that internally calls useQuery(api.listings.getListingImageUrl, ...) or the Convex query used by ImageUploader and returns the mapped URLs/reactive results; update this file to replace the manual useConvex/Promise.all/useEffect sequence with useResolvedImageUrls(listingImageIds) and update ImageUploader to consume the same hook so both components share the same reactive cache and behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/convex/listings.ts`:
- Line 96: Add a nullable array-of-strings `tags` field to the Convex listings
table schema so writes from createListing and updateListing succeed: edit your
schema.ts listings table definition to include tags:
v.optional(v.array(v.string())), ensuring the key name matches the field written
by createListing and updateListing; after adding the field run any schema
validation/compile steps to pick up the change.
In `@backend/convex/messages.ts`:
- Around line 92-106: The current pagination uses a single timestamp cursor
(args.cursor → parsedCursor → cursorValue) and filters with
withIndex('by_user_lastActivityAt') using lt('lastActivityAt', cursorValue),
which can drop rows with identical lastActivityAt; change to a stable compound
cursor (e.g., encode/decode { lastActivityAt, _id } from the conversation row)
and replace the single lt filter with a compound comparison (first compare
lastActivityAt, then _id as tie-breaker) when querying conversationParticipants,
or switch to Convex's built-in pagination helpers to return deterministic pages;
update cursor parsing/serialization where args.cursor is read and adjust the
take(limit + 1) logic to use the new cursor format.
In `@backend/package.json`:
- Line 17: The dependency entry "@types/jest": "^30.0.0" in backend/package.json
is incompatible with the runtime jest ^29.7.0 and the root workspace `@types/jest`
^29.5.14; change the version to a ^29.x release (e.g., align with ^29.5.14) so
type declarations match the Jest runtime and workspace, then reinstall/update
lockfile (npm/yarn install) and rebuild or re-run type checks to ensure no
remaining type mismatches.
In `@frontend/app/listings/`[id].tsx:
- Around line 77-82: Wrap the Edit button Pressable render in a guard that
checks ownership by comparing the current viewer's id (e.g., currentUserId or
session?.user?.id) against listing.sellerId (or other owner field) before
rendering; only render the Pressable (styles.editButton,
router.push(`/listings/${listing._id}/edit`), Text "Edit Listing") when the ids
match, otherwise omit the button to prevent non-owners from seeing the edit
action.
- Around line 31-33: The `(api as any)` cast is used because getListingImageUrl
isn't present in the generated Convex types; re-run the Convex codegen (npx
convex dev) to regenerate api.d.ts so getListingImageUrl appears, then remove
the cast and use convex.query((api).listings.getListingImageUrl, { storageId:
imageId as Id<'_storage'> }); also verify the function is correctly exported
from backend/convex/listings.ts (named getListingImageUrl) if codegen still
doesn't pick it up.
In `@frontend/app/listings/`[id]/edit.tsx:
- Around line 26-36: The useEffect that repopulates form fields on every change
to listing (the effect that calls setTitle, setDescription, setPrice,
setCategory, setCondition, setImages) overwrites in-progress user edits; add a
local "hasInitialized" (or similar) useState flag and only populate the form
from listing when hasInitialized is false and listing exists (or when listing.id
first becomes available), then set hasInitialized to true so subsequent listing
updates don't clobber user input; alternatively, gate population by checking
each field for empty/untouched before calling the corresponding setter.
In `@frontend/app/listings/new.tsx`:
- Around line 25-27: The submit guard uses the render-captured state
isSubmitting, which can allow rare double submissions on rapid taps; change to a
synchronous ref-based guard by adding a useRef like submittingRef and set it
true before invoking createListing and false on completion or error, replacing
checks of isSubmitting in the submit handler (and the other occurrence around
line 46) with checks against submittingRef.current so back-to-back presses
cannot bypass the guard.
- Around line 40-43: The form currently checks only the committed images array
(`images`) in new.tsx, which ignores ImageUploader's in-flight uploads and
allows submissions that orphan pending uploads; update ImageUploader to expose
pending state (e.g., add a hasPendingUploads prop or an onPendingChange
callback) and then use that signal in the parent (new.tsx) to either disable the
"Create Listing" submit button or show an Alert when hasPendingUploads is true;
locate the ImageUploader component and its usage in new.tsx and wire the new
prop/callback so submission is blocked or a warning displayed until
pendingUploads === 0.
- Around line 29-32: The current validation only checks sellerEmail for
emptiness; update the form validation (the block that checks title, description,
sellerEmail in new.tsx where sellerEmail is referenced) to also verify
sellerEmail matches a basic email regex and endsWith('@calpoly.edu') (use
sellerEmail.trim().toLowerCase() before checking), and if it fails call
Alert.alert with a clear message and return to block submission; ensure you
validate both general RFC-5322–like pattern and the exact '@calpoly.edu' domain
so malformed or wrong-domain emails are rejected before saving.
- Around line 56-57: Replace the non-blocking navigation call to avoid racing
with the native Alert and leaving the form on the back stack: change
router.push('/') to router.replace('/') in the success flow where
Alert.alert('Success', 'Listing created.') is called (or optionally invoke
router.replace('/') from the Alert.alert callback) so the submitted form is
removed from history and navigation doesn't race the alert overlay.
- Line 18: The frontend is currently accepting a user-controlled sellerEmail via
the sellerEmail state (const [sellerEmail, setSellerEmail]) which can be
spoofed; either stop sending sellerEmail from the client and let the server
derive the seller email from the authenticated identity.subject/email when
creating a listing, or if you must send it, add server-side validation that the
provided sellerEmail matches the authenticated user's email (identity) before
storing sellerId/email. Locate the sellerEmail state and the request payload
assembly in the New Listing component and remove sellerEmail from the payload or
update server-side createListing logic to ignore client-provided sellerEmail and
use identity.subject/email (or validate equality) to prevent impersonation.
In `@frontend/components/ImageUploader.tsx`:
- Around line 92-110: pickUsingWebInput currently calls
URL.createObjectURL(file) and returns the URI but never revokes it; update it so
the created blob URL is revoked when no longer needed by either (a) returning an
object that includes both uri and a revoke() callback that calls
URL.revokeObjectURL(uri) so callers (upload completion or cancel handlers) can
reuse it, or (b) storing the objectUrl in your upload/pending state and ensuring
URL.revokeObjectURL(objectUrl) is invoked when the upload finishes or the
pending upload is removed; reference pickUsingWebInput, URL.createObjectURL and
URL.revokeObjectURL and ensure any created input event handlers are cleaned up
after use.
- Around line 41-83: The effect is repeatedly re-triggering for storage IDs that
resolved to null because the unresolved filter uses !resolvedUrls[imageId]
(treats null same as undefined); update the unresolved calculation in the
useEffect so it only treats truly-unseen IDs as unresolved (e.g., check
resolvedUrls[storageId] === undefined), leaving the logic in resolveImageUrls
and setResolvedUrls unchanged; reference the useEffect/resolvedUrls/images
variables and the resolveImageUrls and setResolvedUrls functions so failed
(null) resolutions are not retried on every render.
- Around line 214-224: The startUpload function uses the outer-scoped images
value when calling onImagesChange which causes a stale-closure bug when multiple
uploads finish concurrently; update the call site in startUpload to append using
an updater callback (e.g. call onImagesChange(prev => [...prev, storageId]))
and, if necessary, update the onImagesChange prop type to accept a
callback-style updater; alternatively, maintain a ref that tracks the latest
images and use that ref when appending — reference startUpload, onImagesChange,
images, and setPendingUploads to locate and change the code.
- Around line 119-123: Replace the deprecated MediaTypeOptions usage in the
ImageUploader component by passing the new mediaTypes string array to
ImagePicker.launchImageLibraryAsync; specifically update the call in
ImageUploader where ImagePicker.launchImageLibraryAsync is invoked (the const
result = await ImagePicker.launchImageLibraryAsync(...) line) to use mediaTypes:
['images'] instead of ImagePicker.MediaTypeOptions.Images, leaving quality and
allowsEditing as-is.
---
Nitpick comments:
In `@backend/convex/__tests__/messages.test.ts`:
- Around line 57-134: Tests for listUserConversationsHandler miss pagination and
sendMessage mutation coverage; add unit tests that (1) verify cursor-based
pagination by seeding participantRows/dummy conversations exceeding the default
limit and asserting result.nextCursor is non-null and subsequent call with that
cursor returns the next page, referencing listUserConversationsHandler and
nextCursor; (2) exercise sendMessage mutation to assert message document
insertion, creation/update of participantRows, and auth failures by calling
sendMessage with valid ctx.userId and with null userId and checking
participantRows and message docs (refer to sendMessage and participantRows); and
(3) include a test where a conversation's referenced doc is null (simulate
deleted conversation in docsById) and assert the handler skips or handles it
gracefully (reference listUserConversationsHandler and docsById).
- Around line 105-134: Current test only asserts passthrough of fixture
unreadCount; update the test to exercise sendMessage logic: use buildCtx to
create a context with multiple participants (including the sender and at least
one other), call sendMessage(...) with the sender as the actor, then call
listUserConversationsHandler(...) (or read the conversation via the same read
path used in production) and assert that the non-sender participant's
conversation.unreadCount increased while the sender's unreadCount stayed the
same; reference sendMessage, listUserConversationsHandler, and buildCtx to
locate and change the test setup and assertions.
In `@backend/convex/messages.ts`:
- Around line 80-81: The exported testable handler listUserConversationsHandler
currently types both parameters as any; change its signature to use Convex's
QueryCtx (or the appropriate ctx type) for the first parameter and define an
explicit args interface (e.g., ListUserConversationsArgs) describing expected
fields for the second parameter, then update any tests/mocks to implement those
shapes so TypeScript can verify calls and prevent silent breakage when the
handler's expectations change.
In `@backend/convex/schema.ts`:
- Around line 48-55: The messages table currently defines type as an open string
(messages: defineTable ... type: v.string()) which permits any value; replace
that with a union of allowed literals to enforce schema-level validation (for
example change the type validator in messages to a union of v.literal('text')
and any other supported types like v.literal('image') or just v.literal('text')
if only text messages are used) so invalid message types cannot be persisted;
update the validator in the messages table definition (and adjust any imports if
needed) to use v.union and v.literal accordingly.
In `@backend/convex/tsconfig.json`:
- Line 14: Remove the "types": ["jest"] entry from backend/convex/tsconfig.json
so Jest globals are no longer injected into production Convex functions, and
instead add a new tsconfig at backend/convex/__tests__/tsconfig.json that
extends the root Convex tsconfig (extends: "../tsconfig.json"), sets
compilerOptions.types to ["jest"], and includes the test files (include:
["./**/*"]) so Jest types are scoped only to the test folder.
In `@backend/jest.config.js`:
- Around line 13-14: Update the Jest config so we only treat TypeScript files as
ESM: remove '.js' from the extensionsToTreatAsEsm array in jest.config.js
(symbol: extensionsToTreatAsEsm) or alternatively convert the CommonJS mock
backend/convex/__mocks__/server.js to ESM (replace module.exports with export
default) so the setting and mock format are consistent; pick one approach and
apply it across the repo to avoid mixed ESM/CommonJS behavior.
In `@frontend/app/index.tsx`:
- Around line 29-44: Replace the inline renderItem in the FlatList with the
existing ListingCard component so the list reuses centralized rendering (use
FlatList's renderItem={({item}) => <ListingCard listing={item} onPress={() =>
router.push(`/listings/${item._id}`)} />} ), ensuring ListingCard renders tag
chips so tags appear on the home list as well; after switching, remove the
now-unused styles listingCard, listingTitle, listingPrice, and
listingDescription from this file to avoid duplication.
In `@frontend/app/listings/`[id].tsx:
- Line 3: There are duplicate imports from 'expo-router' — merge the separate
imports of useLocalSearchParams and useRouter into a single import statement
(e.g., import { useLocalSearchParams, useRouter } from 'expo-router') so both
hooks are imported together; update any other occurrences (e.g., the other
import on line with useRouter) to remove the redundant import and keep only the
unified import.
- Line 12: The component duplicates image-URL resolution logic by using
useConvex + Promise.all + useEffect instead of reusing ImageUploader's query;
extract the shared logic into a new hook (e.g., useResolvedImageUrls(ids:
string[])) that internally calls useQuery(api.listings.getListingImageUrl, ...)
or the Convex query used by ImageUploader and returns the mapped URLs/reactive
results; update this file to replace the manual useConvex/Promise.all/useEffect
sequence with useResolvedImageUrls(listingImageIds) and update ImageUploader to
consume the same hook so both components share the same reactive cache and
behavior.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
frontend/components/ImageUploader.tsx (1)
248-295: Duplicated upload-enqueue logic across Camera / Photo Library branches.The Camera and Photo Library
onPresshandlers (lines 252–268 and 274–290) are nearly identical — they pick an image, generate a localId, enqueue a pending upload, and start the upload. Consider extracting a shared helper:Suggested refactor
+ async function pickAndUpload(picker: () => Promise<{ uri: string } | null>) { + const picked = await picker(); + if (!picked) return; + const localId = `pending-${Date.now()}`; + setPendingUploads((prev) => [ + ...prev, + { localId, uri: picked.uri, progress: 0, status: 'uploading' as const }, + ]); + await startUpload(picked.uri, localId); + } Alert.alert('Add image', 'Choose image source', [ { text: 'Camera', - onPress: () => { - void (async () => { - const picked = await pickFromCamera(); - if (!picked) return; - const localId = `pending-${Date.now()}`; - setPendingUploads((prev) => [ - ...prev, - { localId, uri: picked.uri, progress: 0, status: 'uploading' }, - ]); - await startUpload(picked.uri, localId); - })(); - }, + onPress: () => void pickAndUpload(pickFromCamera), }, { text: 'Photo Library', - onPress: () => { - void (async () => { - const picked = await pickFromLibrary(); - if (!picked) return; - const localId = `pending-${Date.now()}`; - setPendingUploads((prev) => [ - ...prev, - { localId, uri: picked.uri, progress: 0, status: 'uploading' }, - ]); - await startUpload(picked.uri, localId); - })(); - }, + onPress: () => void pickAndUpload(pickFromLibrary), }, { text: 'Cancel', style: 'cancel' }, ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/ImageUploader.tsx` around lines 248 - 295, Extract the duplicated logic in the Camera and Photo Library onPress handlers into a reusable helper (e.g., handlePickAndUpload) that takes the picker function as an argument (pickFromCamera or pickFromLibrary); the helper should await the picker, return early if no result, create the localId (`pending-${Date.now()}`), call setPendingUploads to add the pending item with uri, progress 0 and status 'uploading', then call startUpload with the picked.uri and localId. Replace both onPress anonymous async blocks to call this new helper with the appropriate picker to remove duplication.backend/convex/messages.ts (2)
163-174:.collect()loads all participant rows into memory, defeating cursor-based pagination.The query fetches every
conversationParticipantsrow for the user, then filters/sorts in-memory. For users with many conversations this becomes an O(n) scan on every page request. The compound cursor approach is correct for stable pagination, but consider at minimum adding a Convex.take()with a generous upper bound or implementing an approximation using the index range (.filterwithltonlastActivityAtfrom the cursor) before collecting, then applying the compound tiebreaker only on the boundary.For a college marketplace this is likely fine in the near term, but worth keeping in mind as usage grows.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/convex/messages.ts` around lines 163 - 174, The current code calls ctx.db.query(...).collect() (allParticipantRows) which loads every conversationParticipants row into memory and then filters/sorts in JS (isRowAfterCursor, compareRowsDescByActivityThenId), defeating cursor pagination; change the query to limit results server-side by using .take(limit + 1) on the Convex query when no complex tie-breaking is required, or if a compound cursor exists (parsedCursor) add a .withIndex(...).filter/.range using lastActivityAt < parsedCursor.lastActivityAt (and handle equals case via id boundary) before .collect(), then apply the existing isRowAfterCursor/compareRowsDescByActivityThenId only to the fetched page boundary to preserve correct ordering and return stable participantRows without loading all rows.
50-53: Remove(q: any)cast — the index callback should be properly typed.The Convex-generated types should infer the correct callback parameter type for
withIndex. Theanycast suppresses type checking on the index field name and equality filter. If this was added to work around a type error, it likely indicates the generated types are stale — re-runnpx convex devto regenerate.Suggested fix
const participantRows = await ctx.db .query('conversationParticipants') - .withIndex('by_conversationId', (q: any) => q.eq('conversationId', args.conversationId)) + .withIndex('by_conversationId', (q) => q.eq('conversationId', args.conversationId)) .collect();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/convex/messages.ts` around lines 50 - 53, Remove the unnecessary any cast on the withIndex callback so TypeScript uses Convex's generated types: replace .withIndex('by_conversationId', (q: any) => q.eq('conversationId', args.conversationId)) with .withIndex('by_conversationId', q => q.eq('conversationId', args.conversationId)) in the code that creates participantRows (ctx.db.query('conversationParticipants')...); if this caused a type error, regenerate the Convex types by running npx convex dev and then re-run the build so the correct callback type for withIndex is available.frontend/app/listings/[id].tsx (1)
40-44: Consider using a stable key instead ofurlfor image rendering.
key={url}works if URLs are unique, but Convex storage URLs are short-lived signed URLs that may change between renders (causing unnecessary remounts). Using the original storage ID as key would be more stable:Suggested fix
<View style={styles.imageRow}> - {imageUrls.map((url) => ( - <Image key={url} source={{ uri: url }} style={styles.image} /> + {imageUrls.map((url, index) => ( + <Image key={listing.images[index] ?? url} source={{ uri: url }} style={styles.image} /> ))} </View>However, the
imageUrlsarray has already been filtered (removing nulls), so the index no longer maps 1:1 tolisting.images. A cleaner approach would be to keep the paired data together before filtering:Alternative: pair IDs with URLs before filtering
- const imageUrls = useMemo( - () => mappedUrls.filter((url): url is string => typeof url === 'string' && url.length > 0), - [mappedUrls] - ); + const resolvedImages = useMemo( + () => + (listing?.images ?? []) + .map((id, i) => ({ id, url: mappedUrls[i] })) + .filter((entry): entry is { id: string; url: string } => + typeof entry.url === 'string' && entry.url.length > 0 + ), + [listing?.images, mappedUrls] + ); // In JSX: - {imageUrls.map((url) => ( - <Image key={url} source={{ uri: url }} style={styles.image} /> + {resolvedImages.map(({ id, url }) => ( + <Image key={id} source={{ uri: url }} style={styles.image} /> ))}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/listings/`[id].tsx around lines 40 - 44, The current Image list uses key={url} which is unstable for short-lived signed URLs; instead build a paired array of {id, url} from the original listing.images (preserving the storage ID), filter out null/undefined URLs, and then map that paired array in the render (replace imageUrls.map(...) with something like pairedImages.map(({id, url}) => <Image key={id} .../>)) so the Image components use the stable storage ID as the key; update the variable used in the JSX inside the component that renders styles.imageRow and adjust any upstream code that creates imageUrls to instead produce the paired id+url structure.frontend/hooks/useResolvedImageUrls.ts (1)
14-56:resolvedUrlsin the dependency array causes an extra no-op effect cycle after each batch resolves.When the effect resolves URLs and calls
setResolvedUrls, the state change triggers the effect again. The second run finds nothing unresolved and returns early — so it's safe, not an infinite loop. But it's an unnecessary re-run.Consider removing
resolvedUrlsfrom the dependency array and instead reading it via a ref, or moving the unresolved-check logic so the effect only depends onimageIds:Suggested approach
+ const resolvedUrlsRef = useRef(resolvedUrls); + resolvedUrlsRef.current = resolvedUrls; + useEffect(() => { let cancelled = false; async function resolveImageUrls() { const unresolved = imageIds.filter( - (storageId) => !isRemoteUrl(storageId) && resolvedUrls[storageId] === undefined + (storageId) => !isRemoteUrl(storageId) && resolvedUrlsRef.current[storageId] === undefined ); ... } void resolveImageUrls(); return () => { cancelled = true; }; - }, [convex, imageIds, resolvedUrls]); + }, [convex, imageIds]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/hooks/useResolvedImageUrls.ts` around lines 14 - 56, The effect in useResolvedImageUrls re-runs unnecessarily because resolvedUrls is in the dependency array; when resolveImageUrls updates state the effect runs again only to find nothing unresolved. Remove resolvedUrls from the dependency list and instead capture the latest resolvedUrls via a ref or compute unresolved solely from imageIds inside resolveImageUrls (use the current state via the ref before calling convex), keeping convex, imageIds in the dependency array; keep setResolvedUrls usage as-is in resolveImageUrls so updates still merge new entries.backend/convex/listings.ts (1)
125-138: Consider checkingargs.tags !== undefinedincreateListingfor consistency withupdateListing.The code currently passes
tags: args.tagsdirectly toctx.db.insert(). While Convex safely stripsundefinedvalues frominsert()operations (leaving the field absent),updateListingexplicitly checksargs.tags !== undefinedbefore patching (lines 189–191). This is becausectx.db.patch()treatsundefinedvalues as field removals, requiring the check. For clarity and consistency, applying the same check increateListingwould align both functions:const listingId = await ctx.db.insert('listings', { title: args.title, description: args.description, price: args.price, category: args.category, condition: args.condition, - tags: args.tags, + ...(args.tags !== undefined && { tags: args.tags }), images: args.images, sellerEmail, sellerId: identity.subject, status: 'active', createdAt: now, postedOn: now, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/convex/listings.ts` around lines 125 - 138, In createListing the code unconditionally passes tags: args.tags into ctx.db.insert while updateListing guards with args.tags !== undefined before patching; change createListing to only include the tags property when args.tags !== undefined (i.e., conditionally add tags to the insert payload) so behavior matches updateListing and avoids accidentally sending an undefined that could be interpreted differently by DB APIs; locate createListing and modify the object passed to ctx.db.insert accordingly, keeping the guard for args.tags !== undefined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/components/ImageUploader.tsx`:
- Around line 130-148: compressAndValidate currently always resizes to width:
1200 which upscales small images; change it to accept the original image
dimensions (from the picker result) and cap the target width to
Math.min(originalWidth, 1200) before calling manipulateAsync so you never
upscale; also handle potential Android orientation/dimension swap by normalizing
dimensions (e.g., run a no-op manipulateAsync pass if needed) so
compressAndValidate uses the correct originalWidth and originalHeight when
deciding the resize.
---
Duplicate comments:
In `@backend/convex/listings.ts`:
- Line 104: The review flagged a duplicate comment about the tags field; there's
no code change required because schema.ts already defines tags as
v.optional(v.array(v.string())) matching the mutation argument—please
resolve/close the duplicate review thread and remove the redundant comment so
only one resolved note remains referencing the tags definition in schema.ts.
---
Nitpick comments:
In `@backend/convex/listings.ts`:
- Around line 125-138: In createListing the code unconditionally passes tags:
args.tags into ctx.db.insert while updateListing guards with args.tags !==
undefined before patching; change createListing to only include the tags
property when args.tags !== undefined (i.e., conditionally add tags to the
insert payload) so behavior matches updateListing and avoids accidentally
sending an undefined that could be interpreted differently by DB APIs; locate
createListing and modify the object passed to ctx.db.insert accordingly, keeping
the guard for args.tags !== undefined.
In `@backend/convex/messages.ts`:
- Around line 163-174: The current code calls ctx.db.query(...).collect()
(allParticipantRows) which loads every conversationParticipants row into memory
and then filters/sorts in JS (isRowAfterCursor,
compareRowsDescByActivityThenId), defeating cursor pagination; change the query
to limit results server-side by using .take(limit + 1) on the Convex query when
no complex tie-breaking is required, or if a compound cursor exists
(parsedCursor) add a .withIndex(...).filter/.range using lastActivityAt <
parsedCursor.lastActivityAt (and handle equals case via id boundary) before
.collect(), then apply the existing
isRowAfterCursor/compareRowsDescByActivityThenId only to the fetched page
boundary to preserve correct ordering and return stable participantRows without
loading all rows.
- Around line 50-53: Remove the unnecessary any cast on the withIndex callback
so TypeScript uses Convex's generated types: replace
.withIndex('by_conversationId', (q: any) => q.eq('conversationId',
args.conversationId)) with .withIndex('by_conversationId', q =>
q.eq('conversationId', args.conversationId)) in the code that creates
participantRows (ctx.db.query('conversationParticipants')...); if this caused a
type error, regenerate the Convex types by running npx convex dev and then
re-run the build so the correct callback type for withIndex is available.
In `@frontend/app/listings/`[id].tsx:
- Around line 40-44: The current Image list uses key={url} which is unstable for
short-lived signed URLs; instead build a paired array of {id, url} from the
original listing.images (preserving the storage ID), filter out null/undefined
URLs, and then map that paired array in the render (replace imageUrls.map(...)
with something like pairedImages.map(({id, url}) => <Image key={id} .../>)) so
the Image components use the stable storage ID as the key; update the variable
used in the JSX inside the component that renders styles.imageRow and adjust any
upstream code that creates imageUrls to instead produce the paired id+url
structure.
In `@frontend/components/ImageUploader.tsx`:
- Around line 248-295: Extract the duplicated logic in the Camera and Photo
Library onPress handlers into a reusable helper (e.g., handlePickAndUpload) that
takes the picker function as an argument (pickFromCamera or pickFromLibrary);
the helper should await the picker, return early if no result, create the
localId (`pending-${Date.now()}`), call setPendingUploads to add the pending
item with uri, progress 0 and status 'uploading', then call startUpload with the
picked.uri and localId. Replace both onPress anonymous async blocks to call this
new helper with the appropriate picker to remove duplication.
In `@frontend/hooks/useResolvedImageUrls.ts`:
- Around line 14-56: The effect in useResolvedImageUrls re-runs unnecessarily
because resolvedUrls is in the dependency array; when resolveImageUrls updates
state the effect runs again only to find nothing unresolved. Remove resolvedUrls
from the dependency list and instead capture the latest resolvedUrls via a ref
or compute unresolved solely from imageIds inside resolveImageUrls (use the
current state via the ref before calling convex), keeping convex, imageIds in
the dependency array; keep setResolvedUrls usage as-is in resolveImageUrls so
updates still merge new entries.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
backend/convex/messages.ts (1)
149-152: Consider adding an explicit return type tolistUserConversationsHandler.The function body builds
conversationListas an untyped array, so the return type is inferred asPromise<any>. Declaring a concrete return interface would propagate type safety to thelistUserConversationsquery and any future consumers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/convex/messages.ts` around lines 149 - 152, The handler listUserConversationsHandler currently returns an untyped Promise (conversationList is inferred as any); add a concrete return type interface (e.g., Promise<ListUserConversationsResult> or similar) and apply it to listUserConversationsHandler signature so the array elements (conversationList) are typed; define the ListUserConversationsResult (or appropriate shape) describing each conversation item and use that type for conversationList and the function return to ensure type safety propagates to the listUserConversations query and consumers.backend/convex/__tests__/messages.test.ts (2)
187-236: Missing coverage forsendMessageerror paths.Two guard-rail branches in
sendMessagehave no tests:
Conversation not found(thrown whenctx.db.get(conversationId)returns null)User is not a participant in this conversation(thrown whensenderIdis absent fromparticipantIds)✅ Suggested additions
+ it('sendMessage throws when conversation does not exist', async () => { + const ctx = buildCtx({ userId: 'user_sender', docsById: {} }); + await expect( + sendMessage(ctx as any, { conversationId: 'missing_conv', body: 'hi' }) + ).rejects.toThrow('Conversation not found'); + }); + + it('sendMessage throws when sender is not a participant', async () => { + const ctx = buildCtx({ + userId: 'outsider', + docsById: { + conv_1: { + _id: 'conv_1', + listingId: 'l1', + buyerId: 'user_a', + sellerId: 'user_b', + participantIds: ['user_a', 'user_b'], + createdAt: 100, + }, + }, + }); + await expect( + sendMessage(ctx as any, { conversationId: 'conv_1', body: 'hi' }) + ).rejects.toThrow('User is not a participant'); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/convex/__tests__/messages.test.ts` around lines 187 - 236, Add two unit tests for sendMessage to cover its error paths: one where ctx.db.get(conversationId) returns null to assert the "Conversation not found" error is thrown, and another where the conversation exists but its participantIds does not include the senderId to assert the "User is not a participant in this conversation" error is thrown; reuse buildCtx to construct ctx (override docsById or participantRows accordingly) and call sendMessage with the same args as the happy-path test, then expect the appropriate exception messages to be thrown and no message to be inserted/updated.
121-127:order()mock silently ignores its direction argument.The pre-sorted
filteredarray happens to matchdescorder for the current handler, but any future test using.order('asc')will return wrong results without any indication.♻️ Proposed fix
- return { - order: () => ({ - collect: async () => filtered, - take: async (count: number) => filtered.slice(0, count), - }), - collect: async () => filtered, - }; + return { + order: (direction: 'asc' | 'desc' = 'asc') => { + const ordered = + direction === 'desc' ? filtered : filtered.slice().reverse(); + return { + collect: async () => ordered, + take: async (count: number) => ordered.slice(0, count), + }; + }, + collect: async () => filtered, + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/convex/__tests__/messages.test.ts` around lines 121 - 127, The mock implementation of order() ignores its direction argument, so calls like order('asc') incorrectly return the same pre-sorted filtered array; update the mock for order(direction?: string) to respect the direction param by producing a local array (e.g., sorted = direction === 'asc' ? [...filtered].reverse() : filtered) and return the object with collect and take using that sorted array (ensure take slices sorted, and collect returns sorted). Replace the existing order/collect/take mock to use the direction-aware sorted array so future tests using order('asc') behave correctly.frontend/hooks/useResolvedImageUrls.ts (2)
33-34: Silent error swallowing makes URL resolution failures invisibleAny error from
convex.query(api.listings.getListingImageUrl, …)is silently converted tonull. Aconsole.warnhere makes transient network/auth issues visible during development without changing observable behavior.♻️ Proposed fix
} catch { - return [storageId, null] as const; + // eslint-disable-next-line no-console + console.warn(`[useResolvedImageUrls] Failed to resolve URL for storageId: ${storageId}`); + return [storageId, null] as const; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/hooks/useResolvedImageUrls.ts` around lines 33 - 34, In useResolvedImageUrls, the catch block that swallows errors for the convex.query(api.listings.getListingImageUrl, ...) call should log the error so failures are visible during development; update the catch to call console.warn (including the error and the storageId being resolved) and then continue to return [storageId, null] as before so behavior is unchanged while making transient network/auth issues visible.
15-51: No in-flight guard — the same storage IDs can be fetched more than once
resolvedUrlsRef.current[storageId]remainsundefineduntil thesetResolvedUrlscallback executes. IfimageIdsgets a new object identity (e.g., the parent re-renders and passeslisting?.images ?? []while the first fetch batch is in-flight), the effect re-runs after cleanup (cancelled = true), sees all IDs as still-unresolved, and fires another full fetch round. The first round's state update is suppressed bycancelled, so the second round wins—but both rounds hit the network.Eagerly marking IDs as pending in the ref before
awaitprevents the duplicate work:♻️ Proposed fix — mark IDs in-flight before the async calls
const unresolved = imageIds.filter( (storageId) => !isRemoteUrl(storageId) && resolvedUrlsRef.current[storageId] === undefined ); if (unresolved.length === 0) { return; } + // Mark in-flight so concurrent effect runs skip these IDs. + for (const storageId of unresolved) { + resolvedUrlsRef.current[storageId] = null; + } + const nextEntries = await Promise.all(Note: Using
nullas the in-flight sentinel means a failed fetch and a pending fetch are indistinguishable — retries won't happen automatically. If retry-on-failure is needed later, introduce a distinct sentinel (e.g., a"pending"string or aSet).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/hooks/useResolvedImageUrls.ts` around lines 15 - 51, The effect can start duplicate fetches because resolvedUrlsRef.current[storageId] stays undefined until setResolvedUrls runs; in resolveImageUrls mark each unresolved storageId as in-flight in resolvedUrlsRef (and in the state via setResolvedUrls) before doing the Promise.all so subsequent runs see them as pending and skip refetching; use a distinct sentinel (e.g., "pending" or a symbol) instead of null so pending vs failed can be distinguished, and ensure the cleanup/cancel logic still prevents applying results from cancelled batches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/convex/listings.ts`:
- Line 104: The tags schema currently uses v.optional(v.array(v.string())) with
no limits, allowing arbitrarily many or long tags; create a reusable tags
validator (e.g., tagsSchema or validateTags) that enforces a maximum number of
tags (e.g., maxItems) and a maximum tag length (e.g., maxLength per string),
then replace the loose v.optional(v.array(v.string())) in the createListing and
updateListing handlers (and the other schema occurrences referenced near the
tags lines) with this new tagsSchema; on validation failure return a clear
validation error rather than accepting/truncating silently.
- Around line 113-121: The current logic allows a caller-supplied
args.sellerEmail to be used when identity.email is absent, enabling spoofing;
change the code in the listing creation/update flow to require a verified
identity email instead of falling back to args.sellerEmail: obtain
authenticatedEmail from identity.email, and if it's missing or empty throw an
Error (e.g., "Authenticated email required"), never accept providedEmail as the
canonical sellerEmail; update any uses of sellerEmail (the variable assigned
from authenticatedEmail ?? providedEmail) so they always use the
authenticatedEmail and stop trusting args.sellerEmail in
getListings()/getListing() visible fields.
In `@backend/convex/messages.ts`:
- Line 161: Replace the boolean-coercing default operator in the assignment to
preserve explicit zero values: change the `limit` initialization from using
`args.limit || 20` to the nullish-coalescing form so that `args.limit` (which
can be 0 per the validator) is respected; locate the `const limit = args.limit
|| 20;` line in messages.ts and update it to use `??` (i.e., `const limit =
args.limit ?? 20;`).
---
Nitpick comments:
In `@backend/convex/__tests__/messages.test.ts`:
- Around line 187-236: Add two unit tests for sendMessage to cover its error
paths: one where ctx.db.get(conversationId) returns null to assert the
"Conversation not found" error is thrown, and another where the conversation
exists but its participantIds does not include the senderId to assert the "User
is not a participant in this conversation" error is thrown; reuse buildCtx to
construct ctx (override docsById or participantRows accordingly) and call
sendMessage with the same args as the happy-path test, then expect the
appropriate exception messages to be thrown and no message to be
inserted/updated.
- Around line 121-127: The mock implementation of order() ignores its direction
argument, so calls like order('asc') incorrectly return the same pre-sorted
filtered array; update the mock for order(direction?: string) to respect the
direction param by producing a local array (e.g., sorted = direction === 'asc' ?
[...filtered].reverse() : filtered) and return the object with collect and take
using that sorted array (ensure take slices sorted, and collect returns sorted).
Replace the existing order/collect/take mock to use the direction-aware sorted
array so future tests using order('asc') behave correctly.
In `@backend/convex/messages.ts`:
- Around line 149-152: The handler listUserConversationsHandler currently
returns an untyped Promise (conversationList is inferred as any); add a concrete
return type interface (e.g., Promise<ListUserConversationsResult> or similar)
and apply it to listUserConversationsHandler signature so the array elements
(conversationList) are typed; define the ListUserConversationsResult (or
appropriate shape) describing each conversation item and use that type for
conversationList and the function return to ensure type safety propagates to the
listUserConversations query and consumers.
In `@frontend/hooks/useResolvedImageUrls.ts`:
- Around line 33-34: In useResolvedImageUrls, the catch block that swallows
errors for the convex.query(api.listings.getListingImageUrl, ...) call should
log the error so failures are visible during development; update the catch to
call console.warn (including the error and the storageId being resolved) and
then continue to return [storageId, null] as before so behavior is unchanged
while making transient network/auth issues visible.
- Around line 15-51: The effect can start duplicate fetches because
resolvedUrlsRef.current[storageId] stays undefined until setResolvedUrls runs;
in resolveImageUrls mark each unresolved storageId as in-flight in
resolvedUrlsRef (and in the state via setResolvedUrls) before doing the
Promise.all so subsequent runs see them as pending and skip refetching; use a
distinct sentinel (e.g., "pending" or a symbol) instead of null so pending vs
failed can be distinguished, and ensure the cleanup/cancel logic still prevents
applying results from cancelled batches.
| const userId = identity.subject; | ||
|
|
||
| // Step 2: Query participant rows by indexed userId + activity timestamp. | ||
| const limit = args.limit || 20; |
Contributor
There was a problem hiding this comment.
Use ?? instead of || for the limit default.
args.limit || 20 coerces 0 to 20. The validator (v.optional(v.number())) allows 0, and ?? correctly distinguishes "not provided" from "explicitly zero".
🐛 Proposed fix
- const limit = args.limit || 20;
+ const limit = args.limit ?? 20;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/convex/messages.ts` at line 161, Replace the boolean-coercing default
operator in the assignment to preserve explicit zero values: change the `limit`
initialization from using `args.limit || 20` to the nullish-coalescing form so
that `args.limit` (which can be 0 per the validator) is respected; locate the
`const limit = args.limit || 20;` line in messages.ts and update it to use `??`
(i.e., `const limit = args.limit ?? 20;`).
Collaborator
|
Created clean companion branch Feedback:
|
jaydonkc
force-pushed
the
POLY-41-image-uploader
branch
from
4bdbc93 to
5c5576b
Compare
5 tasks
5 tasks
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
2 participants
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.
Linked Issues
Closes #
Linear: (e.g., POLY-141)
Summary
Briefly explain the change and why.
Implemented a reusable image upload flow to replace URL-based image inputs.
What changed:
Added Convex storage endpoints in backend/convex/listings.ts:
generateListingImageUploadUrl (returns one-time upload URL)
getListingImageUrl (resolves storage ID to display URL)
Built ImageUploader in frontend/components/ImageUploader.tsx with:
image picker (camera/library/web file input fallback)
image compression/resizing (max 1200px, JPEG 80%)
per-image upload progress
error state with retry
remove image
Integrated uploader into listing forms:
frontend/app/listings/new.tsx
frontend/app/listings/[id]/edit.tsx
Updated listing detail page to render images from storage IDs:
frontend/app/listings/[id].tsx
Added route wiring/navigation for create/edit listing pages.
Why:
Better UX: users upload images directly instead of pasting links.
Cross-platform support for iOS, Android, and Web.
Stores stable storage IDs in DB instead of brittle raw URLs.
Reusable component architecture for future image upload features.
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