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 selected for processing (1)
📝 WalkthroughWalkthroughAdds platform-aware storage for ConvexAuthProvider, a useAuthGate hook enforcing auth with safe returnTo redirects, web-responsive UI/layout changes, hidden-listing UI and messaging adjustments, schema and backend messaging fields (message.type, participantIds, lastMessageId), and backfill mutation for messaging fields. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Client as Client App
participant AuthGate as useAuthGate
participant ConvexAuth as ConvexAuthProvider
participant LoginPage as /auth/login
User->>Client: Trigger protected action (e.g., Create Listing)
Client->>AuthGate: requireAuth(action)
AuthGate->>ConvexAuth: check isAuthenticated
alt Not authenticated
AuthGate->>Client: Redirect to /auth/login?returnTo=/original/path
User->>LoginPage: Submit verification
LoginPage->>ConvexAuth: verify/authenticate
ConvexAuth-->>LoginPage: auth success
LoginPage->>Client: navigate to validated returnTo
Client->>AuthGate: re-invoke requireAuth(action)
AuthGate->>Client: execute original action
else Authenticated
AuthGate->>Client: execute original action
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ❌ 5❌ Failed checks (4 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/app/index.tsx (1)
262-282:⚠️ Potential issue | 🟠 Major
FlatListdoes not support changingnumColumnson the fly — add akeyprop.React Native's
FlatListrequires a remount whennumColumnschanges. Without akeytied tonumColumns, resizing the browser window across breakpoints (768/1024px) will either throw a warning or fail to re-layout the grid.🐛 Proposed fix
<FlatList data={listings} + key={numColumns} keyExtractor={(item) => item._id} renderItem={({ item }) => <ListingCard listing={item} />}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/index.tsx` around lines 262 - 282, FlatList's numColumns can change at runtime causing layout issues; modify the FlatList JSX (the component using props data={listings}, keyExtractor, renderItem={(...) => <ListingCard .../>}, numColumns={numColumns}, onEndReached={handleLoadMore}) to include a key prop derived from numColumns (e.g., key={`list-${numColumns}`}) so the list remounts when numColumns changes and properly reflows across breakpoints.
🧹 Nitpick comments (5)
frontend/app/index.tsx (1)
178-186:getNumColumnsis recalculated on every render — consideruseMemo.The function itself is trivial, but since its result drives the
keyprop onFlatList(once the fix above is applied), making the value stable viauseMemowould prevent unnecessary FlatList remounts from floating-point width jitter.♻️ Optional stabilization
- const getNumColumns = () => { - if (!isWeb) return 1; - if (isDesktop) return 3; - if (isTablet) return 2; - return 1; - }; - - const numColumns = getNumColumns(); + const numColumns = useMemo(() => { + if (!isWeb) return 1; + if (isDesktop) return 3; + if (isTablet) return 2; + return 1; + }, [isWeb, isDesktop, isTablet]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/index.tsx` around lines 178 - 186, getNumColumns is recomputed every render causing numColumns to change and potentially remount the FlatList; wrap the computation in useMemo so numColumns is stable across renders: memoize the result of getNumColumns (or inline its logic) with useMemo and dependencies that reflect device size flags (isWeb, isDesktop, isTablet) and then use that memoized numColumns for the FlatList key prop to avoid remounts.frontend/components/ListingCard.tsx (1)
42-51: Duplicate tag values would cause React key warnings.
key={tag}assumes tags are unique within the array. If the backend doesn't deduplicate tags, duplicate entries will produce React key collision warnings.Consider using the index as a fallback or a composite key:
- {displayTags.map((tag) => ( - <View key={tag} style={styles.tagChip}> + {displayTags.map((tag, index) => ( + <View key={`${tag}-${index}`} style={styles.tagChip}>This is low-risk if the backend normalizes/deduplicates tags on write.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/ListingCard.tsx` around lines 42 - 51, The tag mapping in ListingCard.tsx uses key={tag} for the rendered chips (inside displayTags.map) which can produce React key collisions if tags are duplicated; update the key to use a stable composite fallback such as `${tag}-${index}` or similar (use the map index or another unique identifier) when rendering the View with style={styles.tagChip} so each element produced by displayTags.map has a unique key (ensure the change affects the JSX that renders Text with styles.tagText and the optional tag count rendered with styles.tagCount); keep hasMoreTags and listing.tags logic unchanged.frontend/app/auth/login.tsx (1)
78-89: Duplicate redirect after successful verification.Both
handleVerifyCode(line 83) and theuseEffect(line 38) will callrouter.replaceto the same path when authentication succeeds — first from the handler, then reactively whenisAuthenticatedflips totrue. This is functionally harmless but redundant.Consider removing the redirect from
handleVerifyCodeand letting theuseEffecthandle all post-auth redirects consistently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/auth/login.tsx` around lines 78 - 89, Remove the direct redirect from the verification handler and centralize post-auth navigation in the reactive effect: in handleVerifyCode (the try block that calls signIn('resend-otp', ...)), delete the router.replace(redirectPath) logic and just call signIn and manage errors/loading; let the existing useEffect that watches isAuthenticated perform router.replace(returnTo || '/') so all redirects happen in one place (keep references to signIn, handleVerifyCode, isAuthenticated, router.replace, and returnTo to locate the changes).frontend/hooks/useAuthGate.ts (1)
16-19:showAlertandalertMessageoptions are declared but never used.The JSDoc mentions mobile alert behavior, but there's no
Alertimport or conditional logic for it. Either implement the mobile alert path or remove the unused options to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/hooks/useAuthGate.ts` around lines 16 - 19, The options showAlert and alertMessage in useAuthGate are declared but unused; either remove them and update the JSDoc, or implement the mobile alert flow: import Alert (e.g., from 'react-native') and, inside the unauthenticated branch of useAuthGate (the logic in useAuthGate that handles redirectTo), call Alert.alert with alertMessage (fallback to a sensible default) when options.showAlert is true before performing the redirect; ensure you reference the options object in the useAuthGate function and update imports/JSDoc accordingly.frontend/app/_layout.tsx (1)
12-43: Platform-aware storage approach is sound; optional simplification possible for the native path.The
defaultbranch wraps eachAsyncStoragemethod in a trivial async function that just forwards the call. SinceAsyncStoragemethods already returnPromise<string | null>andPromise<void>, which match theTokenStorageinterface expected byConvexAuthProvider, the unnecessary async/await wrapper can be removed.♻️ Optional simplification
default: { - getItem: async (key: string) => { - return await AsyncStorage.getItem(key); - }, - setItem: async (key: string, value: string) => { - await AsyncStorage.setItem(key, value); - }, - removeItem: async (key: string) => { - await AsyncStorage.removeItem(key); - }, + getItem: (key: string) => AsyncStorage.getItem(key), + setItem: (key: string, value: string) => AsyncStorage.setItem(key, value), + removeItem: (key: string) => AsyncStorage.removeItem(key), },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/_layout.tsx` around lines 12 - 43, The native/default branch of the Platform.select storage object wraps AsyncStorage calls in redundant async/await wrappers; update the default case so its getItem/setItem/removeItem directly return AsyncStorage.getItem/ setItem/ removeItem (matching the TokenStorage/ConvexAuthProvider expected signatures) instead of using extra async functions—locate the storage constant and modify the default object's methods to return AsyncStorage's promises directly.
🤖 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/app/auth/login.tsx`:
- Around line 34-40: The redirect uses the unvalidated returnTo query param in
the useEffect (see useEffect, isAuthenticated, returnTo, router.replace), which
can enable open redirects; sanitize returnTo before calling router.replace by
passing it through the existing getSafeRedirect helper (or equivalent
validation) and use the resulting safe path for router.replace; also apply the
same getSafeRedirect validation at the other redirect site that currently uses
returnTo (the other router.replace usage) so both redirects only ever navigate
to safe relative paths.
In `@frontend/app/listings/`[id].tsx:
- Around line 85-97: The current block renders only when !isAuthenticated so the
requireAuth callback (and its router.push to the same listing) is dead and there
is no "Message seller" action for authenticated non-seller users; change this by
splitting behavior: keep the existing unauthenticated button using
requireAuth(...) to send users to login (preserving return URL), and add a
separate button rendered when isAuthenticated && currentUserId !==
listing.sellerId that directly navigates to the messaging flow (call router.push
with a messaging route such as `/messages/new?listingId=${listing._id}` or
`/listings/${listing._id}/message`) so authenticated users can message the
seller; update the conditional rendering around isAuthenticated, requireAuth,
router.push, and listing._id (and any user id symbol like currentUserId or
sellerId) accordingly.
In `@frontend/hooks/useAuthGate.ts`:
- Around line 14-38: The wrapper requireAuth currently lies about its return
type and uses an unsafe cast; update its signature so it accurately reflects
that it may return undefined and avoids contravariant generic issues: change the
generic constraint to T extends (...args: any[]) => any (or drop T and use a
handler type like (...args: any[]) => any), and type the returned wrapper as
(...args: Parameters<T>) => ReturnType<T> | undefined (remove the final "as T"
cast). Ensure uses of isLoading, isAuthenticated, router.push, action, options
and pathname remain the same but return undefined explicitly on auth or loading
paths so the types match the implementation.
---
Outside diff comments:
In `@frontend/app/index.tsx`:
- Around line 262-282: FlatList's numColumns can change at runtime causing
layout issues; modify the FlatList JSX (the component using props
data={listings}, keyExtractor, renderItem={(...) => <ListingCard .../>},
numColumns={numColumns}, onEndReached={handleLoadMore}) to include a key prop
derived from numColumns (e.g., key={`list-${numColumns}`}) so the list remounts
when numColumns changes and properly reflows across breakpoints.
---
Nitpick comments:
In `@frontend/app/_layout.tsx`:
- Around line 12-43: The native/default branch of the Platform.select storage
object wraps AsyncStorage calls in redundant async/await wrappers; update the
default case so its getItem/setItem/removeItem directly return
AsyncStorage.getItem/ setItem/ removeItem (matching the
TokenStorage/ConvexAuthProvider expected signatures) instead of using extra
async functions—locate the storage constant and modify the default object's
methods to return AsyncStorage's promises directly.
In `@frontend/app/auth/login.tsx`:
- Around line 78-89: Remove the direct redirect from the verification handler
and centralize post-auth navigation in the reactive effect: in handleVerifyCode
(the try block that calls signIn('resend-otp', ...)), delete the
router.replace(redirectPath) logic and just call signIn and manage
errors/loading; let the existing useEffect that watches isAuthenticated perform
router.replace(returnTo || '/') so all redirects happen in one place (keep
references to signIn, handleVerifyCode, isAuthenticated, router.replace, and
returnTo to locate the changes).
In `@frontend/app/index.tsx`:
- Around line 178-186: getNumColumns is recomputed every render causing
numColumns to change and potentially remount the FlatList; wrap the computation
in useMemo so numColumns is stable across renders: memoize the result of
getNumColumns (or inline its logic) with useMemo and dependencies that reflect
device size flags (isWeb, isDesktop, isTablet) and then use that memoized
numColumns for the FlatList key prop to avoid remounts.
In `@frontend/components/ListingCard.tsx`:
- Around line 42-51: The tag mapping in ListingCard.tsx uses key={tag} for the
rendered chips (inside displayTags.map) which can produce React key collisions
if tags are duplicated; update the key to use a stable composite fallback such
as `${tag}-${index}` or similar (use the map index or another unique identifier)
when rendering the View with style={styles.tagChip} so each element produced by
displayTags.map has a unique key (ensure the change affects the JSX that renders
Text with styles.tagText and the optional tag count rendered with
styles.tagCount); keep hasMoreTags and listing.tags logic unchanged.
In `@frontend/hooks/useAuthGate.ts`:
- Around line 16-19: The options showAlert and alertMessage in useAuthGate are
declared but unused; either remove them and update the JSDoc, or implement the
mobile alert flow: import Alert (e.g., from 'react-native') and, inside the
unauthenticated branch of useAuthGate (the logic in useAuthGate that handles
redirectTo), call Alert.alert with alertMessage (fallback to a sensible default)
when options.showAlert is true before performing the redirect; ensure you
reference the options object in the useAuthGate function and update
imports/JSDoc accordingly.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
frontend/app/index.tsx (1)
174-176:requireAuthis redundant here since the button is only rendered when authenticated.The Create button is only rendered when
isAuthenticatedis true (line 200), sorequireAuthwill never actually gate this call. It's harmless as defense-in-depth, but worth noting it doesn't add functional value in this specific code path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/index.tsx` around lines 174 - 176, The handleCreateListing handler is unnecessarily wrapped in requireAuth even though the Create button is only rendered when isAuthenticated is true; remove the redundancy by replacing const handleCreateListing = requireAuth(() => { router.push('/listings/new'); }); with a plain handler (e.g., const handleCreateListing = () => router.push('/listings/new');) so that handleCreateListing directly calls router.push('/listings/new') and leave requireAuth for places where unauthenticated UI might trigger navigation.frontend/components/ListingCard.tsx (2)
63-71: Last card in a desktop row will carry an unbalancedmarginRight.
desktopListingCardappliesmarginRight: 16to all cards, including the last one in each row. Within aFlatListusingnumColumns, this creates uneven right-side spacing. If this becomes visually noticeable, you can address it by usinggap(supported in RN 0.71+) on the column wrapper instead, or conditionally omittingmarginRighton the last item.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/ListingCard.tsx` around lines 63 - 71, desktopListingCard currently applies marginRight: 16 to every card, causing the last item in each FlatList row (when using numColumns) to have extra right spacing; update the layout so the last item in a row doesn't get marginRight by either moving spacing to a column wrapper (use gap on the container if RN >= 0.71) or conditionally omitting marginRight for the last column item in the renderItem logic (check index and numColumns) so desktopListingCard no longer forces an unbalanced right margin.
44-45: Index-based key is acceptable here but masks duplicate tags.Using
${tag}-${index}prevents React key collisions when duplicate tags exist. Since tag chips are purely display-only with no interactive local state, index in the key is safe. However, the presence of duplicate tags in the data is likely a data-quality issue upstream — consider whether the backend or listing creation form should deduplicate tags before storage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/ListingCard.tsx` around lines 44 - 45, The tag rendering uses displayTags.map with key={`${tag}-${index}`}, which masks duplicate tags; dedupe tags before rendering instead of relying on an index-based key—compute a uniqueTags array (e.g., via Array.from(new Set(displayTags)) or displayTags.filter((t,i)=>displayTags.indexOf(t)===i) inside ListingCard and map over uniqueTags when creating the <View key={tag} style={styles.tagChip}> elements so keys are stable and duplicates are removed at render time (or consider moving deduplication upstream where tags are stored/created).
🤖 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/app/index.tsx`:
- Around line 259-270: The grid's last row is stretched by styles.columnWrapper
using justifyContent: 'space-between' when listings.length % numColumns !== 0;
fix by padding the data or changing layout: either append placeholder items to
listings until listings.length % numColumns === 0 (add an isPlaceholder flag so
keyExtractor and renderItem in FlatList, renderItem -> ListingCard, can return a
transparent/invisible placeholder component for items with isPlaceholder), or
change styles.columnWrapper to use justifyContent: 'flex-start' and give each
ListingCard a fixed width/margin (adjust ListingCard container style and
numColumns logic) so partial rows align correctly; update keyExtractor to handle
placeholder ids (e.g., `${_id || 'placeholder-' + index}`) and ensure
ListingCard gracefully renders placeholders.
- Around line 200-212: The header currently shows the Sign In button while auth
is resolving because isAuthenticated is false during isLoading; update the
conditional around the two TouchableOpacity blocks to early-return or hide both
buttons when isLoading is true (i.e., render the create/sign-in buttons only if
!isLoading && (isAuthenticated ? ... : ...)), so use the existing isLoading and
isAuthenticated flags to gate rendering of the createButton
(handleCreateListing) and signInButton (router.push('/auth/login')) until
loading completes.
---
Duplicate comments:
In `@frontend/app/auth/login.tsx`:
- Around line 33-48: Hoist the pure helper getSafeRedirect out of the React
component in login.tsx so it is not recreated on every render and doesn't need
to be added to the useEffect dependency array; move the getSafeRedirect function
declaration above the component (keeping its logic intact: return '/' if path is
falsy, doesn't start with '/' or startsWith('//'), otherwise return path) and
leave the useEffect that calls getSafeRedirect(returnTo) unchanged except
removing any local definition.
---
Nitpick comments:
In `@frontend/app/index.tsx`:
- Around line 174-176: The handleCreateListing handler is unnecessarily wrapped
in requireAuth even though the Create button is only rendered when
isAuthenticated is true; remove the redundancy by replacing const
handleCreateListing = requireAuth(() => { router.push('/listings/new'); }); with
a plain handler (e.g., const handleCreateListing = () =>
router.push('/listings/new');) so that handleCreateListing directly calls
router.push('/listings/new') and leave requireAuth for places where
unauthenticated UI might trigger navigation.
In `@frontend/components/ListingCard.tsx`:
- Around line 63-71: desktopListingCard currently applies marginRight: 16 to
every card, causing the last item in each FlatList row (when using numColumns)
to have extra right spacing; update the layout so the last item in a row doesn't
get marginRight by either moving spacing to a column wrapper (use gap on the
container if RN >= 0.71) or conditionally omitting marginRight for the last
column item in the renderItem logic (check index and numColumns) so
desktopListingCard no longer forces an unbalanced right margin.
- Around line 44-45: The tag rendering uses displayTags.map with
key={`${tag}-${index}`}, which masks duplicate tags; dedupe tags before
rendering instead of relying on an index-based key—compute a uniqueTags array
(e.g., via Array.from(new Set(displayTags)) or
displayTags.filter((t,i)=>displayTags.indexOf(t)===i) inside ListingCard and map
over uniqueTags when creating the <View key={tag} style={styles.tagChip}>
elements so keys are stable and duplicates are removed at render time (or
consider moving deduplication upstream where tags are stored/created).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
frontend/app/auth/login.tsx (1)
20-26: Optional: GuardgetSafeRedirectagainst a runtimestring[]value forreturnTo.
useLocalSearchParams<{ returnTo?: string }>()typesreturnToasstring | undefined, but the underlying URL parser can producestring[]if the param appears multiple times (e.g.,?returnTo=a&returnTo=b). Per the expo-router docs, the rest syntax returns astring[]— and in practice, duplicate query params from any browser URL can arrive as an array. If astring[]reachesgetSafeRedirect, calling.startsWith()on it will throw aTypeErrorinside theuseEffect, which can crash the login screen.🛡️ Proposed fix
function getSafeRedirect(path?: string): string { - if (!path || !path.startsWith('/') || path.startsWith('//')) { + if (!path || Array.isArray(path) || !path.startsWith('/') || path.startsWith('//')) { return '/'; } return path; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/auth/login.tsx` around lines 20 - 26, getSafeRedirect can receive a string[] (e.g., duplicate query params) and calling .startsWith on an array throws; update getSafeRedirect (and callers like useLocalSearchParams returnTo) to first ensure path is a string (e.g., if Array.isArray(path) use path[0] or treat as invalid) and then perform the existing checks (must be a single string starting with '/' but not '//'); if the value is not a string or is an empty string/array, return '/'.
🤖 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/app/index.tsx`:
- Around line 184-189: The brand name is inconsistent: update the string used in
the document.title assignment or the on-screen welcome text so both use the same
product name (e.g., "PolyBuys"); locate the document.title set inside the
useEffect and the component's rendered welcome text (the "Welcome to PolyBuy"
literal) and change one so both match exactly (preferably standardize on
"PolyBuys").
- Around line 253-264: The web multi-column layout has no horizontal spacing
because webListingCard overrides marginHorizontal to 0 while desktopListingCard
uses marginHorizontal: 8; update the styles so web multi-column items get
horizontal spacing — either remove/override the marginHorizontal: 0 in
webListingCard to match desktopListingCard (e.g., set marginHorizontal: 8) or
add a columnGap/rowGap to the columnWrapper style (and ensure FlatList uses
columnWrapper via columnWrapperStyle) so columns have consistent spacing; update
the webListingCard or styles.columnWrapper definitions accordingly.
---
Nitpick comments:
In `@frontend/app/auth/login.tsx`:
- Around line 20-26: getSafeRedirect can receive a string[] (e.g., duplicate
query params) and calling .startsWith on an array throws; update getSafeRedirect
(and callers like useLocalSearchParams returnTo) to first ensure path is a
string (e.g., if Array.isArray(path) use path[0] or treat as invalid) and then
perform the existing checks (must be a single string starting with '/' but not
'//'); if the value is not a string or is an empty string/array, return '/'.
Comment on lines
253
to
+264
| ) : ( | ||
| <FlatList | ||
| key={`list-${numColumns}`} | ||
| data={listings} | ||
| keyExtractor={(item) => item._id} | ||
| renderItem={({ item }) => <ListingCard listing={item} />} | ||
| contentContainerStyle={styles.listContainer} | ||
| contentContainerStyle={[ | ||
| styles.listContainer, | ||
| isWeb && numColumns > 1 && styles.webListContainer, | ||
| ]} | ||
| numColumns={numColumns} | ||
| columnWrapperStyle={numColumns > 1 ? styles.columnWrapper : undefined} |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n -A 6 "webListingCard|desktopListingCard|listingCard" --type-add 'tsx:*.tsx' --type tsxRepository: codebox-calpoly/PolyBuys
Length of output: 1728
🏁 Script executed:
# Check the columnWrapper style definition around lines 353-356
sed -n '350,360p' frontend/app/index.tsxRepository: codebox-calpoly/PolyBuys
Length of output: 305
🏁 Script executed:
# Also search for columnWrapper style definition more broadly
rg -n -B 2 -A 6 "columnWrapper" frontend/app/index.tsx --type-add 'tsx:*.tsx' --type tsxRepository: codebox-calpoly/PolyBuys
Length of output: 718
Fix spacing for multi-column layout on web.
desktopListingCard includes marginHorizontal: 8 to space items in desktop multi-column layouts, but webListingCard explicitly sets marginHorizontal: 0. Since columnWrapper defines no gap or columnGap, web multi-column grids will have flush items with no horizontal spacing. Add horizontal margin to webListingCard or define columnGap in columnWrapper.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/index.tsx` around lines 253 - 264, The web multi-column layout
has no horizontal spacing because webListingCard overrides marginHorizontal to 0
while desktopListingCard uses marginHorizontal: 8; update the styles so web
multi-column items get horizontal spacing — either remove/override the
marginHorizontal: 0 in webListingCard to match desktopListingCard (e.g., set
marginHorizontal: 8) or add a columnGap/rowGap to the columnWrapper style (and
ensure FlatList uses columnWrapper via columnWrapperStyle) so columns have
consistent spacing; update the webListingCard or styles.columnWrapper
definitions accordingly.
5 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/convex/messages.ts (1)
56-67:⚠️ Potential issue | 🟠 MajorValidate and restrict
typeat the mutation boundary.
typeis currently pass-through from client input. Add an allowlist check before persisting to avoid invalid message variants in production data.Suggested fix
export const sendMessage = action({ args: { conversationId: v.id('conversations'), body: v.string(), type: v.optional(v.string()), }, handler: async (ctx, args): Promise<{ messageId: string }> => { @@ - const type = args.type ?? 'text'; + const type = args.type ?? 'text'; + if (type !== 'text') { + throw new ConvexError('Unsupported message type'); + } @@ const result = await ctx.runMutation(internal.messages.internalSendMessage, { conversationId: args.conversationId, listingId: convo.listingId, senderId: userId, recipientId, body: args.body, type: type, });Also applies to: 110-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/convex/messages.ts` around lines 56 - 67, In the message creation mutation handler (handler: async (ctx, args): Promise<{ messageId: string }>), validate args.type against a strict allowlist (e.g., ['text','image','file'] or your app's allowed variants) before assigning const type = args.type ?? 'text' and before persisting; if args.type is present and not in the allowlist, throw a ConvexError with a clear message. Apply the same allowlist check in the other message-creation location mentioned (the other message handler) so only allowed message types are stored.
🧹 Nitpick comments (4)
frontend/app/index.tsx (1)
353-356: Web multi-column spacing still missing horizontal gaps.
columnWrapperdefines nogaporcolumnGap, and based on the relatedListingCard.tsxstyles,webListingCardsetsmarginHorizontal: 0whiledesktopListingCardusesmarginHorizontal: 8. This means tablet-width grids (2 columns, usingwebListingCard) will have flush items with no horizontal spacing.Consider adding
gaptocolumnWrapperor ensuringwebListingCardalso has horizontal margin:♻️ Option 1: Add gap to columnWrapper
columnWrapper: { justifyContent: 'flex-start', paddingHorizontal: 0, + gap: 16, },♻️ Option 2: Ensure webListingCard in ListingCard.tsx has margin
In
frontend/components/ListingCard.tsx, updatewebListingCardto include horizontal margin similar todesktopListingCard.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/index.tsx` around lines 353 - 356, The grid columnWrapper style currently lacks horizontal gap which causes two-column tablet layouts to render flush items; to fix, add a horizontal gap (e.g., gap or columnGap) to columnWrapper in frontend/app/index.tsx or alternatively update webListingCard in frontend/components/ListingCard.tsx to include marginHorizontal similar to desktopListingCard (e.g., marginHorizontal: 8) so tablet-width grids get consistent horizontal spacing between items.frontend/app/listings/[id].tsx (1)
97-103: Placeholder handler for "Message Seller" button.The
onPress={() => {}}is a no-op. Consider adding a TODO comment or disabling the button until the messaging feature is implemented, to avoid confusing users who tap a non-functional button.💡 Option: Disable button or add alert until messaging is ready
- <TouchableOpacity style={styles.messageButton} onPress={() => {}}> + <TouchableOpacity + style={[styles.messageButton, { opacity: 0.6 }]} + onPress={() => { + // TODO: Navigate to messaging when implemented + Alert.alert('Coming Soon', 'Messaging feature is under development.'); + }} + >🤖 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 97 - 103, The Message Seller button currently has a no-op onPress (onPress={() => {}}) which confuses users; update the TouchableOpacity (styles.messageButton) to either be disabled until messaging is implemented (use the disabled prop and a corresponding disabled style when currentUserSubject && currentUserSubject !== listing.sellerId) or replace the handler with a small placeholder action (e.g., show a user-facing alert/modal saying "Messaging coming soon") and add a TODO comment referencing the real messaging implementation; target the TouchableOpacity element and its onPress/disabled behavior when making the change.backend/convex/__tests__/messages.test.ts (1)
58-76: Add explicit assertions for the new message metadata behavior.Given this PR introduces message
typedefaults andlastMessageIdupdates, it would be valuable to assert both aftersendMessageto prevent regressions.Also applies to: 120-130
🤖 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 58 - 76, Add assertions that the newly created message has the expected default type and that the conversation's lastMessageId is updated: after calling asBuyer.action(api.messages.sendMessage, { conversationId, body: 'Hello seller!' }) and loading the message via ctx.db.get(result.messageId) assert message.type === 'text' (or the PR's default) in addition to existing fields, and then load the conversation via ctx.db.get(conversationId) and assert conversation.lastMessageId === result.messageId; use the existing symbols result.messageId, ctx.db.get, conversationId, and api.messages.sendMessage to locate where to add these checks.backend/convex/schema.ts (1)
121-121: Constrainmessages.typeto an allowlist.Line 121 accepts any string, which allows unsupported message types into storage. Prefer a bounded set (even if currently only
'text') to protect data integrity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/convex/schema.ts` at line 121, The messages.type field currently uses v.optional(v.string()), allowing any string; change it to a bounded allowlist (e.g., v.optional(v.enum(['text'])) or equivalent literal union) so only supported message types are stored. Locate the messages schema definition (the messages.type entry) and replace v.optional(v.string()) with an enum or literal union of allowed values (start with 'text'), and update any related TypeScript types or usages that assume a freeform string to the constrained union.
🤖 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/messages.ts`:
- Around line 282-305: The backfillMessagingFields mutation currently loads
entire conversations and messages tables in one run (variables
conversations/messages and counters conversationPatches/messagePatches) causing
unbounded work; change it to process records in bounded batches (e.g., pageSize
variable) and loop paginated queries (use a repeatable cursor/lastId or
query.take(pageSize)) to fetch, patch, and commit each batch, repeating until a
batch returns zero items so the migration can be retried safely and won’t exceed
runtime/write limits; ensure the patch logic for participantIds and type remains
idempotent so backfillMessagingFields can be safely re-run.
In `@frontend/app/listings/`[id].tsx:
- Line 1: Prettier reported formatting errors in the listings page import
statement (the line importing View, Text, StyleSheet, ScrollView,
TouchableOpacity, Platform from 'react-native'); run the formatter to correct
styling (e.g. execute npx prettier --write frontend/app/listings/[id].tsx) or
manually fix spacing/line breaks to match the project's Prettier rules so the
import and file pass CI.
---
Outside diff comments:
In `@backend/convex/messages.ts`:
- Around line 56-67: In the message creation mutation handler (handler: async
(ctx, args): Promise<{ messageId: string }>), validate args.type against a
strict allowlist (e.g., ['text','image','file'] or your app's allowed variants)
before assigning const type = args.type ?? 'text' and before persisting; if
args.type is present and not in the allowlist, throw a ConvexError with a clear
message. Apply the same allowlist check in the other message-creation location
mentioned (the other message handler) so only allowed message types are stored.
---
Nitpick comments:
In `@backend/convex/__tests__/messages.test.ts`:
- Around line 58-76: Add assertions that the newly created message has the
expected default type and that the conversation's lastMessageId is updated:
after calling asBuyer.action(api.messages.sendMessage, { conversationId, body:
'Hello seller!' }) and loading the message via ctx.db.get(result.messageId)
assert message.type === 'text' (or the PR's default) in addition to existing
fields, and then load the conversation via ctx.db.get(conversationId) and assert
conversation.lastMessageId === result.messageId; use the existing symbols
result.messageId, ctx.db.get, conversationId, and api.messages.sendMessage to
locate where to add these checks.
In `@backend/convex/schema.ts`:
- Line 121: The messages.type field currently uses v.optional(v.string()),
allowing any string; change it to a bounded allowlist (e.g.,
v.optional(v.enum(['text'])) or equivalent literal union) so only supported
message types are stored. Locate the messages schema definition (the
messages.type entry) and replace v.optional(v.string()) with an enum or literal
union of allowed values (start with 'text'), and update any related TypeScript
types or usages that assume a freeform string to the constrained union.
In `@frontend/app/index.tsx`:
- Around line 353-356: The grid columnWrapper style currently lacks horizontal
gap which causes two-column tablet layouts to render flush items; to fix, add a
horizontal gap (e.g., gap or columnGap) to columnWrapper in
frontend/app/index.tsx or alternatively update webListingCard in
frontend/components/ListingCard.tsx to include marginHorizontal similar to
desktopListingCard (e.g., marginHorizontal: 8) so tablet-width grids get
consistent horizontal spacing between items.
In `@frontend/app/listings/`[id].tsx:
- Around line 97-103: The Message Seller button currently has a no-op onPress
(onPress={() => {}}) which confuses users; update the TouchableOpacity
(styles.messageButton) to either be disabled until messaging is implemented (use
the disabled prop and a corresponding disabled style when currentUserSubject &&
currentUserSubject !== listing.sellerId) or replace the handler with a small
placeholder action (e.g., show a user-facing alert/modal saying "Messaging
coming soon") and add a TODO comment referencing the real messaging
implementation; target the TouchableOpacity element and its onPress/disabled
behavior when making the change.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
backend/convex/__tests__/messages.test.tsbackend/convex/__tests__/testUtils.tsbackend/convex/messages.tsbackend/convex/schema.tsfrontend/app/_layout.tsxfrontend/app/auth/login.tsxfrontend/app/index.tsxfrontend/app/listings/[id].tsxfrontend/app/listings/[id]/edit.tsxfrontend/components/HiddenBanner.tsxfrontend/components/ListingCard.tsxfrontend/components/ListingUnavailable.tsxfrontend/hooks/useAuthGate.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/components/ListingCard.tsx
- frontend/hooks/useAuthGate.ts
Comment on lines
+282
to
+305
| export const backfillMessagingFields = internalMutation({ | ||
| args: {}, | ||
| handler: async (ctx) => { | ||
| const conversations = await ctx.db.query('conversations').collect(); | ||
| let conversationPatches = 0; | ||
|
|
||
| for (const convo of conversations) { | ||
| if (!convo.participantIds || convo.participantIds.length !== 2) { | ||
| await ctx.db.patch(convo._id, { participantIds: [convo.buyerId, convo.sellerId] }); | ||
| conversationPatches += 1; | ||
| } | ||
| } | ||
|
|
||
| const messages = await ctx.db.query('messages').collect(); | ||
| let messagePatches = 0; | ||
|
|
||
| for (const message of messages) { | ||
| if (!message.type) { | ||
| await ctx.db.patch(message._id, { type: 'text' }); | ||
| messagePatches += 1; | ||
| } | ||
| } | ||
|
|
||
| return { conversationPatches, messagePatches }; |
Contributor
There was a problem hiding this comment.
Backfill mutation does unbounded work in one execution.
This loads and patches entire tables in a single run. On moderate/large datasets, this is likely to hit runtime/write limits and fail mid-migration. Please batch this migration (bounded page size + repeatable runs).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/convex/messages.ts` around lines 282 - 305, The
backfillMessagingFields mutation currently loads entire conversations and
messages tables in one run (variables conversations/messages and counters
conversationPatches/messagePatches) causing unbounded work; change it to process
records in bounded batches (e.g., pageSize variable) and loop paginated queries
(use a repeatable cursor/lastId or query.take(pageSize)) to fetch, patch, and
commit each batch, repeating until a batch returns zero items so the migration
can be retried safely and won’t exceed runtime/write limits; ensure the patch
logic for participantIds and type remains idempotent so backfillMessagingFields
can be safely re-run.
| @@ -1,16 +1,24 @@ | |||
| import { View, Text, StyleSheet, ScrollView, TouchableOpacity } from 'react-native'; | |||
| import { View, Text, StyleSheet, ScrollView, TouchableOpacity, Platform } from 'react-native'; | |||
Contributor
There was a problem hiding this comment.
Fix Prettier formatting issue flagged by CI.
The pipeline reports a Prettier check failure. Run npx prettier --write frontend/app/listings/[id].tsx to fix code style issues.
🧰 Tools
🪛 GitHub Actions: CI
[warning] 1-1: Code style issues found in frontend/app/listings/[id].tsx. Run 'npx prettier --write' to fix.
[error] 1-1: Prettier check failed. Run 'npx prettier --write' to fix code style issues in this file.
🤖 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 1, Prettier reported formatting
errors in the listings page import statement (the line importing View, Text,
StyleSheet, ScrollView, TouchableOpacity, Platform from 'react-native'); run the
formatter to correct styling (e.g. execute npx prettier --write
frontend/app/listings/[id].tsx) or manually fix spacing/line breaks to match the
project's Prettier rules so the import and file pass CI.
Collaborator
|
Created clean companion branch Feedback:
|
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
4 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 #35,44,45
Linear: POLY-35, POLY-44, POLY-$%
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
New Features
Improvements