feat: set up deep linking and improve message coverage [POLY-43]#45
Merged
feat: set up deep linking and improve message coverage [POLY-43]#45
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 (7)
📝 WalkthroughWalkthroughThis pull request introduces deep linking support for iOS and Android with corresponding configuration files, extends the messaging system to track message types and conversation participants, implements authentication-aware UI flows on listing detail pages with hidden listing management, adds SEO enhancements for web, and creates new UI components for listing unavailability and hidden listing banners. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
frontend/app.json (1)
25-25: AndroidpathPrefixis overly permissive — consider adding trailing slash
pathPrefix: "/listings"will match any path beginning with that string, including hypothetical/listingsold,/listings-archive, etc. Using"/listings/"(with trailing slash) or switching topathPatternwith a regex ensures only real listing detail paths are intercepted.🔧 Proposed fix
- "pathPrefix": "/listings" + "pathPrefix": "/listings/"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app.json` at line 25, The current pathPrefix value "pathPrefix": "/listings" is too permissive; update the app routing configuration to use a trailing slash or a precise pattern so only real listing detail paths match — either change the pathPrefix value to "/listings/" or replace pathPrefix with a pathPattern using a regex like ^/listings/ to strictly match listing routes; locate the pathPrefix entry in frontend/app.json and make one of these changes and ensure any consumers expecting the old prefix are updated accordingly.backend/convex/__tests__/messages.test.ts (2)
81-93: Repeated insert-then-delete pattern — extract to atestUtilshelperThe pattern of inserting a
conversationsrecord and immediately deleting it to obtain a valid-format but non-existent ID appears four times (lines 81–93, 254–266, 633–645, 820–832). A helper intestUtils.tswould remove the duplication:// In testUtils.ts export async function createDeletedConversationId( t: any, listingId: Id<'listings'>, buyerId: Id<'users'>, sellerId: Id<'users'> ): Promise<Id<'conversations'>> { return await t.run(async (ctx: any) => { const id = await ctx.db.insert('conversations', { listingId, buyerId, sellerId, createdAt: Date.now(), updatedAt: Date.now(), buyerLastReadAt: 0, sellerLastReadAt: 0, }); await ctx.db.delete(id); return id; }); }🤖 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 81 - 93, Extract the repeated "insert then delete" pattern into a helper function named createDeletedConversationId in testUtils.ts: implement createDeletedConversationId(t, listingId, buyerId, sellerId) to call t.run, use ctx.db.insert to create the conversation with the same fields (createdAt, updatedAt, buyerLastReadAt, sellerLastReadAt) then ctx.db.delete the inserted id and return the id; replace the four inline blocks that use t.run/ctx.db.insert followed by ctx.db.delete (the ones producing a conversationId) with calls to this new createDeletedConversationId helper to remove duplication.
103-126:process.env.OPENAI_API_KEYcleanup should be inafterEachor atry/finally
delete process.env.OPENAI_API_KEYat line 125 only runs if the assertion above resolves normally. If the test framework itself throws (or the mock setup fails), the env var leaks into subsequent tests, potentially affecting their moderation-related behavior.🔧 Proposed fix using try/finally
global.fetch = jest.fn().mockResolvedValue({ ... }) as any; - process.env.OPENAI_API_KEY = 'mock_key'; + const originalKey = process.env.OPENAI_API_KEY; + process.env.OPENAI_API_KEY = 'mock_key'; + try { await expect(async () => { await asBuyer.action(api.messages.sendMessage, { conversationId, body: 'bad message', }); }).rejects.toThrow('inappropriate content'); - delete process.env.OPENAI_API_KEY; + } finally { + if (originalKey === undefined) { + delete process.env.OPENAI_API_KEY; + } else { + process.env.OPENAI_API_KEY = originalKey; + } + }🤖 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 103 - 126, The test "throws error when message is flagged by moderation" sets process.env.OPENAI_API_KEY but deletes it only after the assertion, which can leak the env var if the test fails; fix by ensuring cleanup always runs — either move deletion of process.env.OPENAI_API_KEY into a file-level afterEach/afterAll or wrap the test body in a try/finally so process.env.OPENAI_API_KEY is deleted in the finally block; update the test that calls api.messages.sendMessage and any setup that assigns process.env.OPENAI_API_KEY to guarantee the env var is cleared regardless of test outcome.
🤖 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.json`:
- Around line 40-43: Enable typed routes by generating the Expo ambient types on
CI: add a CI setup step (after installing dependencies and before running type
checks) that runs "npx expo customize tsconfig.json" so expo-env.d.ts is created
and TypeScript can resolve route strings; this will allow you to remove the "as
any" workarounds currently used in frontend/app/index.tsx and
frontend/app/listings/new.tsx when experiments.typedRoutes is true.
In `@frontend/app/index.tsx`:
- Line 171: The router.push call using a loose cast currently uses "as any" (see
the object with text 'Sign In' and onPress: () => router.push('/auth/login' as
any)); replace that cast with the correct Href type from expo-router (import
Href from 'expo-router' or use the named type) so it becomes
router.push('/auth/login' as Href), or remove the cast entirely once typedRoutes
are available; update the import to include the Href type and ensure the onPress
arrow uses that typed cast instead of "as any".
In `@frontend/app/listings/`[id].tsx:
- Line 51: The meta tag currently uses the placeholder string
"app-id=YOUR_APP_ID" which prevents Safari from showing the Smart App Banner;
update the meta element in listings/[id].tsx (the <meta name="apple-itunes-app"
...> tag) to use a real numeric App Store ID pulled from configuration (e.g.,
process.env.NEXT_PUBLIC_APPSTORE_ID or a runtime config) and only render the
meta tag when that env var/value is present and numeric so the banner will
appear after the app is published.
- Around line 1-4: Consolidate duplicate imports from 'react-native' by merging
the separate Platform import into the existing import that currently brings in
View, Text, StyleSheet, ScrollView, TouchableOpacity—update the single import
statement (the one that references View, Text, StyleSheet, ScrollView,
TouchableOpacity) to also import Platform and remove the separate Platform
import to avoid duplicate module imports.
In `@frontend/public/.well-known/apple-app-site-association`:
- Line 6: The apple-app-site-association's "appID" value is wrong and will break
Universal Links; update the "appID" to use the exact bundle identifier from
app.json (i.e., use "com.polybuy.app" to match the "bundleIdentifier" field) and
replace the "TEAMID" placeholder with your real 10-character Apple Team ID so
the appID becomes "TEAMIDcom.polybuy.app" (with the actual TEAMID), ensuring the
appID and bundleIdentifier match exactly.
In `@frontend/public/.well-known/assetlinks.json`:
- Line 7: The assetlinks.json currently contains a placeholder
"sha256_cert_fingerprints" value which will break Android App Links
verification; replace "YOUR_SHA256_CERT_FINGERPRINT" in the
"sha256_cert_fingerprints" array with the actual SHA-256 certificate fingerprint
of your release signing certificate (obtain via keytool -list -v -keystore
your-release.keystore -alias your-key-alias or from Play Console → Setup → App
integrity) and ensure the value is the exact fingerprint string used to sign the
production APK/AAB.
---
Nitpick comments:
In `@backend/convex/__tests__/messages.test.ts`:
- Around line 81-93: Extract the repeated "insert then delete" pattern into a
helper function named createDeletedConversationId in testUtils.ts: implement
createDeletedConversationId(t, listingId, buyerId, sellerId) to call t.run, use
ctx.db.insert to create the conversation with the same fields (createdAt,
updatedAt, buyerLastReadAt, sellerLastReadAt) then ctx.db.delete the inserted id
and return the id; replace the four inline blocks that use t.run/ctx.db.insert
followed by ctx.db.delete (the ones producing a conversationId) with calls to
this new createDeletedConversationId helper to remove duplication.
- Around line 103-126: The test "throws error when message is flagged by
moderation" sets process.env.OPENAI_API_KEY but deletes it only after the
assertion, which can leak the env var if the test fails; fix by ensuring cleanup
always runs — either move deletion of process.env.OPENAI_API_KEY into a
file-level afterEach/afterAll or wrap the test body in a try/finally so
process.env.OPENAI_API_KEY is deleted in the finally block; update the test that
calls api.messages.sendMessage and any setup that assigns
process.env.OPENAI_API_KEY to guarantee the env var is cleared regardless of
test outcome.
In `@frontend/app.json`:
- Line 25: The current pathPrefix value "pathPrefix": "/listings" is too
permissive; update the app routing configuration to use a trailing slash or a
precise pattern so only real listing detail paths match — either change the
pathPrefix value to "/listings/" or replace pathPrefix with a pathPattern using
a regex like ^/listings/ to strictly match listing routes; locate the pathPrefix
entry in frontend/app.json and make one of these changes and ensure any
consumers expecting the old prefix are updated accordingly.
Comment on lines
+40
to
+43
| "scheme": "polybuys", | ||
| "experiments": { | ||
| "typedRoutes": true | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the frontend/app.json file at the specified lines
cat -n frontend/app.json | sed -n '35,50p'Repository: codebox-calpoly/PolyBuys
Length of output: 373
🏁 Script executed:
# Search for as any workarounds in the codebase
rg "as any" --type ts --type tsx --type js --type jsx -C 2Repository: codebox-calpoly/PolyBuys
Length of output: 95
🏁 Script executed:
# Check if expo-env.d.ts exists
find . -name "expo-env.d.ts" -o -name ".gitignore" | head -20Repository: codebox-calpoly/PolyBuys
Length of output: 123
🏁 Script executed:
# Check if expo-env.d.ts is mentioned in .gitignore
cat .gitignore 2>/dev/null | grep -i expoRepository: codebox-calpoly/PolyBuys
Length of output: 95
🏁 Script executed:
# Look for typedRoutes configuration in the project
rg "typedRoutes" --type json --type ts --type tsx --type js --type jsxRepository: codebox-calpoly/PolyBuys
Length of output: 95
🏁 Script executed:
# Search for as any workarounds in TypeScript/JavaScript files
rg "as any" --type ts --type js -C 2Repository: codebox-calpoly/PolyBuys
Length of output: 17175
🏁 Script executed:
# Check expo-env.d.ts existence in the entire repo
find . -type f -name "*expo-env*" -o -type f -name "*.d.ts" | grep -E "(expo|env)" | head -20Repository: codebox-calpoly/PolyBuys
Length of output: 50
🏁 Script executed:
# Check frontend directory structure for TypeScript definitions
ls -la frontend/ | head -20Repository: codebox-calpoly/PolyBuys
Length of output: 826
🏁 Script executed:
# Look at the package.json to understand the project setup
cat frontend/package.json | grep -A 5 -B 5 "expo"Repository: codebox-calpoly/PolyBuys
Length of output: 1145
🏁 Script executed:
# Search for tsconfig.json in frontend
find frontend -name "tsconfig.json" -exec cat {} \;Repository: codebox-calpoly/PolyBuys
Length of output: 631
🏁 Script executed:
# Check the specific route usages with as any
cat -n frontend/app/index.tsx | sed -n '1,100p' | grep -A 2 -B 2 "as any"Repository: codebox-calpoly/PolyBuys
Length of output: 50
🏁 Script executed:
# Check the other route usage
cat -n frontend/app/listings/new.tsx | grep -A 2 -B 2 "as any"Repository: codebox-calpoly/PolyBuys
Length of output: 235
🏁 Script executed:
# Check if expo-env.d.ts was generated or should be in repo
git status --ignored | grep -i expoRepository: codebox-calpoly/PolyBuys
Length of output: 50
🏁 Script executed:
# Verify the actual issue: check if TypeScript would error without as any workaround
cat frontend/tsconfig.jsonRepository: codebox-calpoly/PolyBuys
Length of output: 631
🌐 Web query:
Expo typedRoutes expo-env.d.ts CI generation setup
💡 Result:
To make experiments.typedRoutes work in CI (so expo-env.d.ts and the route types exist during tsc/lint), do this:
- Enable typed routes
- In
app.json/app.config.*:Expo Router typed routes are generated by Expo CLI when enabled. [1]{ "expo": { "experiments": { "typedRoutes": true } } }
- Ensure your repo already has a
tsconfig.json
- Generate/normalize it once locally (and commit the
tsconfig.jsonchanges):This command is also what Expo documents for type generation. [2]npx expo customize tsconfig.json
- Run type generation in CI before typechecking
- Add a CI step (after
install, beforetsc/eslint):Expo explicitly recommends this when you need typed-route types generated on CI without starting the dev server. [1]npx expo customize tsconfig.json
- Do NOT commit
expo-env.d.ts
- When typed routes are enabled, Expo CLI generates a git-ignored
expo-env.d.ts, updates.gitignore, and updatestsconfig.jsonincludeto includeexpo-env.d.tsand.expo. It should stay uncommitted. [1]
Example GitHub Actions job steps (core part):
- run: npm ci
- run: npx expo customize tsconfig.json
- run: npm run tscSources: Expo Router Typed Routes docs [1]; Expo TypeScript guide (type generation via expo customize) [2].
Remove as any workarounds by generating typed routes on CI
With experiments.typedRoutes: true enabled, Expo needs to generate expo-env.d.ts before TypeScript can resolve route strings. Without this file on CI, valid routes appear as type errors, forcing the as any escape hatches at frontend/app/index.tsx and frontend/app/listings/new.tsx. Add npx expo customize tsconfig.json as a CI setup step (after dependencies install, before type checking) to generate the required types without starting the dev server.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app.json` around lines 40 - 43, Enable typed routes by generating
the Expo ambient types on CI: add a CI setup step (after installing dependencies
and before running type checks) that runs "npx expo customize tsconfig.json" so
expo-env.d.ts is created and TypeScript can resolve route strings; this will
allow you to remove the "as any" workarounds currently used in
frontend/app/index.tsx and frontend/app/listings/new.tsx when
experiments.typedRoutes is true.
| Alert.alert('Sign In Required', 'Please sign in to create a listing', [ | ||
| { text: 'Cancel', style: 'cancel' }, | ||
| { text: 'Sign In', onPress: () => router.push('/auth/login') }, | ||
| { text: 'Sign In', onPress: () => router.push('/auth/login' as any) }, |
Contributor
There was a problem hiding this comment.
Same CI-failing as any cast — apply the same as Href fix
Identical issue to frontend/app/listings/new.tsx line 45. Replace as any with as Href from expo-router, or drop the cast once typedRoutes types are properly generated and included.
🔧 Proposed fix
- import { useLocalSearchParams, useRouter } from 'expo-router';
+ import { useLocalSearchParams, useRouter, type Href } from 'expo-router';
...
- { text: 'Sign In', onPress: () => router.push('/auth/login' as any) },
+ { text: 'Sign In', onPress: () => router.push('/auth/login' as Href) },🧰 Tools
🪛 GitHub Actions: CI
[error] 171-171: ESLint: Unexpected any. Specify a different type. (no-explicit-any)
🪛 GitHub Check: Lint & Type Check
[warning] 171-171:
Unexpected any. Specify a different type
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/index.tsx` at line 171, The router.push call using a loose cast
currently uses "as any" (see the object with text 'Sign In' and onPress: () =>
router.push('/auth/login' as any)); replace that cast with the correct Href type
from expo-router (import Href from 'expo-router' or use the named type) so it
becomes router.push('/auth/login' as Href), or remove the cast entirely once
typedRoutes are available; update the import to include the Href type and ensure
the onPress arrow uses that typed cast instead of "as any".
| content={`$${listing.price} - ${listing.description.substring(0, 100)}${listing.description.length > 100 ? '...' : ''}`} | ||
| /> | ||
| <meta property="og:url" content={`https://polybuys.com/listings/${listing._id}`} /> | ||
| <meta name="apple-itunes-app" content="app-id=YOUR_APP_ID" /> |
Contributor
There was a problem hiding this comment.
apple-itunes-app placeholder won't show the Smart App Banner
app-id=YOUR_APP_ID is a placeholder. Safari only renders the Smart App Banner when a valid numeric App Store ID is present. This should be tracked and substituted once the app is published.
🤖 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 51, The meta tag currently uses the
placeholder string "app-id=YOUR_APP_ID" which prevents Safari from showing the
Smart App Banner; update the meta element in listings/[id].tsx (the <meta
name="apple-itunes-app" ...> tag) to use a real numeric App Store ID pulled from
configuration (e.g., process.env.NEXT_PUBLIC_APPSTORE_ID or a runtime config)
and only render the meta tag when that env var/value is present and numeric so
the banner will appear after the app is published.
| "target": { | ||
| "namespace": "android_app", | ||
| "package_name": "com.polybuy.app", | ||
| "sha256_cert_fingerprints": ["YOUR_SHA256_CERT_FINGERPRINT"] |
Contributor
There was a problem hiding this comment.
Placeholder SHA256 fingerprint must be replaced before production
Android will fetch this file during App Links verification and compare the sha256_cert_fingerprints against the installed APK's signing certificate. A literal placeholder string will cause verification to fail and Android App Links will not activate.
Obtain the fingerprint from your release keystore:
keytool -list -v -keystore your-release.keystore -alias your-key-aliasor from the Play Console under Setup → App integrity → App signing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/public/.well-known/assetlinks.json` at line 7, The assetlinks.json
currently contains a placeholder "sha256_cert_fingerprints" value which will
break Android App Links verification; replace "YOUR_SHA256_CERT_FINGERPRINT" in
the "sha256_cert_fingerprints" array with the actual SHA-256 certificate
fingerprint of your release signing certificate (obtain via keytool -list -v
-keystore your-release.keystore -alias your-key-alias or from Play Console →
Setup → App integrity) and ensure the value is the exact fingerprint string used
to sign the production APK/AAB.
Collaborator
|
Linting error |
jaydonkc
force-pushed
the
feature/POLY-43-deep-linking
branch
from
c9e07ed to
429d223
Compare
Collaborator
|
Created clean companion branch Feedback:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
frontend/public/.well-known/assetlinks.json (1)
7-7:⚠️ Potential issue | 🟠 MajorReplace the SHA-256 placeholder with the release signing fingerprint.
Android App Links auto-verification will fail while this placeholder remains.
🔧 Proposed fix
- "sha256_cert_fingerprints": ["YOUR_SHA256_CERT_FINGERPRINT"] + "sha256_cert_fingerprints": ["<RELEASE_CERT_SHA256_FINGERPRINT>"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/public/.well-known/assetlinks.json` at line 7, Replace the placeholder value in the assetlinks JSON entry "sha256_cert_fingerprints" (the array currently containing "YOUR_SHA256_CERT_FINGERPRINT") with your release app signing SHA-256 fingerprint string from the release keystore/Play App Signing so Android App Links can auto-verify; keep it as a quoted string inside the array and ensure it exactly matches the release signing fingerprint format.frontend/public/.well-known/apple-app-site-association (1)
6-6:⚠️ Potential issue | 🟠 MajorReplace the
TEAMIDplaceholder before merge.Universal Links won’t verify with a literal placeholder in
appID; this must be the real 10-character Apple Team ID.🔧 Proposed fix
- "appID": "TEAMID.com.polybuy.app", + "appID": "<APPLE_TEAM_ID>.com.polybuy.app",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/public/.well-known/apple-app-site-association` at line 6, The appID value currently uses the literal placeholder "TEAMID.com.polybuy.app" in the apple-app-site-association file; replace "TEAMID" with your actual 10-character Apple Team ID so the appID becomes "<Your10CharTeamID>.com.polybuy.app" (ensure the appID string under the "appID" key is updated accordingly).
🧹 Nitpick comments (4)
backend/convex/messages.ts (2)
56-67: Validatetypebefore persisting.Current flow accepts any non-null string and stores it. Add allow-list validation (or normalization) before calling
internalSendMessageto keep message contract stable.Suggested runtime guard
+const ALLOWED_MESSAGE_TYPES = new Set(['text']); ... - const type = args.type ?? 'text'; + const type = args.type ?? 'text'; + if (!ALLOWED_MESSAGE_TYPES.has(type)) { + throw new ConvexError('Invalid message 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, The handler currently accepts any args.type string and passes it through; add an allow-list check/normalization for message types before calling internalSendMessage (e.g., validate args.type against permitted types like 'text', 'image', etc., or default to 'text'), and throw a ConvexError for invalid types; update the handler (referencing handler, args.type, and internalSendMessage) to perform this runtime guard so only allowed/normalized types are persisted.
224-228: Use full composite index equality instead of post-filter.Since
by_listing_buyer_sellernow includessellerId, include it in.withIndex(...)and drop the.filter(...)to avoid extra scan work.Suggested query update
const existing = await ctx.db .query('conversations') .withIndex('by_listing_buyer_seller', (q) => - q.eq('listingId', args.listingId).eq('buyerId', buyerId) + q.eq('listingId', args.listingId).eq('buyerId', buyerId).eq('sellerId', sellerId) ) - .filter((q) => q.eq(q.field('sellerId'), sellerId)) .first();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/convex/messages.ts` around lines 224 - 228, The query currently uses .withIndex('by_listing_buyer_seller', (q) => q.eq('listingId', args.listingId).eq('buyerId', buyerId)) and then a .filter(...) on sellerId which forces a post-filter scan; instead include sellerId in the composite index equality so the index is fully used: change the .withIndex call to also .eq('sellerId', sellerId) and remove the .filter(...) so .first() operates on the indexed equality across listingId, buyerId, and sellerId.backend/convex/schema.ts (1)
121-121: Constrainmessages.typeat the schema layer.
v.optional(v.string())permits arbitrary values. If the app only supports specific message types, encode that as a union to prevent invalid data drift.Suggested schema tightening
- type: v.optional(v.string()), + type: v.optional(v.union(v.literal('text'))),🤖 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 schema currently allows arbitrary message.type values via v.optional(v.string()); tighten this by replacing that with an explicit union/enum of the supported message type literals (e.g. v.optional(v.union([v.literal('text'), v.literal('image'), v.literal('system')])) or equivalent in your validator) so only known types are accepted; update the messages.type schema entry and any related type usages to include all supported message-type strings.frontend/app/listings/[id].tsx (1)
154-156: Use typed object-form navigation instead ofas nevercast for query parameters.At line 155, the
as nevercast suppresses route typing. The codebase already uses the recommended pattern elsewhere in this file. Use object-form navigation withparams:🔧 Suggested refactor
- router.push(`/auth/login?returnTo=${encodeURIComponent(redirectTo)}` as never); + router.push({ + pathname: '/auth/login', + params: { returnTo: redirectTo }, + });This approach is consistent with expo-router v6 best practices and the existing pattern in
navigateToFeedWithTag(earlier 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 around lines 154 - 156, Replace the current router.push call that uses a string and an `as never` cast with the object-form navigation to preserve typings: build the redirectTo value as before (const redirectTo = `/listings/${listing._id}`) and call router.push({ pathname: '/auth/login', params: { returnTo: redirectTo } }) (referencing router.push, redirectTo and listing._id) so query parameters are typed instead of suppressed by `as never`.
🤖 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-306: The backfillMutation backfillMessagingFields currently
does unbounded full-table reads via ctx.db.query('conversations').collect() and
ctx.db.query('messages').collect(); change the handler to paginate/batch the
reads and patches instead of collecting everything into memory — use a loop that
queries with a reasonable page size (e.g., limit) and a cursor/offset (or
startAfter using the last item's _id) to fetch chunks, apply ctx.db.patch for
each item in the chunk (keeping the same logic for setting participantIds and
type), and continue until no more rows; ensure you update the
conversationPatches and messagePatches counters and avoid long-running single
queries in backfillMessagingFields by processing and committing each batch
before fetching the next.
In `@frontend/app/index.tsx`:
- Around line 115-116: The defensive filter on allListings into listings can
cause an empty visible page when all items on the current page are hidden;
update the rendering logic in frontend/app/index.tsx to detect when
listings.length === 0 but there are more pages and auto-advance/fetch the next
page instead of showing the empty state. Specifically, after computing listings
from allListings (filtering by isHidden), check the pagination signals (e.g.,
hasNextPage, pageIndex or totalPages) and call the existing next-page loader
(e.g., fetchNextPage / loadMore / incrementPage) until either listings is
non-empty or no more pages remain, then render the empty state only when no
visible items and no further pages.
In `@frontend/app/listings/`[id].tsx:
- Around line 163-165: The Message Seller button has an empty onPress; add a
handler (e.g., handleMessageSeller) and wire it to TouchableOpacity's onPress so
tapping starts or navigates to a conversation. Implement handleMessageSeller to:
1) check auth (currentUser or isAuthenticated) and redirect to login if
unauthenticated; 2) call the conversation starter (e.g., startConversation or
createConversation) with the listing id and seller id (use listing.id and
listing.sellerId or listing.ownerId); 3) navigate to the chat screen (e.g.,
navigation.navigate('Chat' or 'Conversation', {conversationId})) or open the
chat modal on success; replace the empty onPress={() => {}} with
onPress={handleMessageSeller} and handle errors with a user-friendly alert.
---
Duplicate comments:
In `@frontend/public/.well-known/apple-app-site-association`:
- Line 6: The appID value currently uses the literal placeholder
"TEAMID.com.polybuy.app" in the apple-app-site-association file; replace
"TEAMID" with your actual 10-character Apple Team ID so the appID becomes
"<Your10CharTeamID>.com.polybuy.app" (ensure the appID string under the "appID"
key is updated accordingly).
In `@frontend/public/.well-known/assetlinks.json`:
- Line 7: Replace the placeholder value in the assetlinks JSON entry
"sha256_cert_fingerprints" (the array currently containing
"YOUR_SHA256_CERT_FINGERPRINT") with your release app signing SHA-256
fingerprint string from the release keystore/Play App Signing so Android App
Links can auto-verify; keep it as a quoted string inside the array and ensure it
exactly matches the release signing fingerprint format.
---
Nitpick comments:
In `@backend/convex/messages.ts`:
- Around line 56-67: The handler currently accepts any args.type string and
passes it through; add an allow-list check/normalization for message types
before calling internalSendMessage (e.g., validate args.type against permitted
types like 'text', 'image', etc., or default to 'text'), and throw a ConvexError
for invalid types; update the handler (referencing handler, args.type, and
internalSendMessage) to perform this runtime guard so only allowed/normalized
types are persisted.
- Around line 224-228: The query currently uses
.withIndex('by_listing_buyer_seller', (q) => q.eq('listingId',
args.listingId).eq('buyerId', buyerId)) and then a .filter(...) on sellerId
which forces a post-filter scan; instead include sellerId in the composite index
equality so the index is fully used: change the .withIndex call to also
.eq('sellerId', sellerId) and remove the .filter(...) so .first() operates on
the indexed equality across listingId, buyerId, and sellerId.
In `@backend/convex/schema.ts`:
- Line 121: The schema currently allows arbitrary message.type values via
v.optional(v.string()); tighten this by replacing that with an explicit
union/enum of the supported message type literals (e.g.
v.optional(v.union([v.literal('text'), v.literal('image'),
v.literal('system')])) or equivalent in your validator) so only known types are
accepted; update the messages.type schema entry and any related type usages to
include all supported message-type strings.
In `@frontend/app/listings/`[id].tsx:
- Around line 154-156: Replace the current router.push call that uses a string
and an `as never` cast with the object-form navigation to preserve typings:
build the redirectTo value as before (const redirectTo =
`/listings/${listing._id}`) and call router.push({ pathname: '/auth/login',
params: { returnTo: redirectTo } }) (referencing router.push, redirectTo and
listing._id) so query parameters are typed instead of suppressed by `as never`.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
backend/convex/__tests__/messages.test.tsbackend/convex/__tests__/testUtils.tsbackend/convex/messages.tsbackend/convex/schema.tsfrontend/app.jsonfrontend/app/index.tsxfrontend/app/listings/[id].tsxfrontend/app/listings/[id]/edit.tsxfrontend/components/HiddenBanner.tsxfrontend/components/ListingUnavailable.tsxfrontend/public/.well-known/apple-app-site-associationfrontend/public/.well-known/assetlinks.json
Comment on lines
+282
to
+306
| 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 full-table reads in a single run.
collect() across entire conversations and messages can fail or timeout as data grows. This should be batched/paginated to stay operationally safe.
🤖 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 - 306, The backfillMutation
backfillMessagingFields currently does unbounded full-table reads via
ctx.db.query('conversations').collect() and ctx.db.query('messages').collect();
change the handler to paginate/batch the reads and patches instead of collecting
everything into memory — use a loop that queries with a reasonable page size
(e.g., limit) and a cursor/offset (or startAfter using the last item's _id) to
fetch chunks, apply ctx.db.patch for each item in the chunk (keeping the same
logic for setting participantIds and type), and continue until no more rows;
ensure you update the conversationPatches and messagePatches counters and avoid
long-running single queries in backfillMessagingFields by processing and
committing each batch before fetching the next.
0f395bd to
f066dc8
Compare
jaydonkc
force-pushed
the
feature/POLY-43-deep-linking
branch
from
f066dc8 to
5ecc2e7
Compare
This was referenced Mar 9, 2026
Merged
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: POLY-43
Summary
This PR introduces deep linking / universal links for the PolyBuys app and adds robust backend test coverage to the messages module.
Deep Linking & Web Improvements:
polybuys://scheme and associated domains for deep linking inapp.json.assetlinks.json) and Apple App Site Association (apple-app-site-association) files. Team ID, App ID, and SHA256 will need substitution before production deployment.app/listings/[id].tsx) with Facebook/iMessage compatible Open Graph tags for rich link previews./authinstead of/auth/login.Backend Test Coverage:
backend/convex/messages.ts.sendMessage,getConversationHistory,getOrCreateConversation,markMessagesAsRead, andmessagesByConversationto verify unauthenticated access rejection.How to Test
Steps to verify locally:
npm run lintnpm run typechecknpm run test --workspace=backendnpm run dev:backend(in terminal A)npm run dev(in terminal B)npx uri-scheme open "polybuys://listings/<id>" --iosto verify app deep linking.Checklist
npm run lint)devSummary by CodeRabbit
New Features
Improvements