Add DM and store notifications support - #209
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
📝 WalkthroughWalkthroughAdds direct-message and store-related notification types, fetches them in parallel from DM/store contracts, enriches notifications with conversation/order/review metadata, extends types and settings, and surfaces new “Messages” and “Orders” tabs with per-notification actions and UI elements. Changes
Sequence DiagramsequenceDiagram
participant UI as UI Layer
participant Service as NotificationService
participant DM as DM Contract
participant Store as Storefront Contract
participant Enricher as Enrichment Logic
UI->>Service: fetchNotifications(userId)
par Parallel Queries
Service->>DM: getNewMessages(userId)
Service->>Store: getNewOrders(userId)
Service->>Store: getOrderStatusUpdates(userId)
Service->>Store: getNewReviews(userId)
Service->>Service: getSocialNotifications()
end
DM-->>Service: rawNotifications (newMessage)
Store-->>Service: rawNotifications (orderReceived, orderStatusUpdate, newReview)
Service-->>Service: rawNotifications (followers, mentions, likes, etc.)
Service->>Enricher: enrichNotifications(aggregatedRaw)
Enricher->>Enricher: attach conversationId, messagePreview, orderId, storeId, orderStatus, reviewRating
Enricher-->>Service: enrichedNotifications
Service-->>UI: Notification[]
UI->>UI: Filter by tab (messages, orders, follow, etc.)
UI->>UI: Render items with actions (Open Messages / View Order / View Review)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
Deploying yappr with
|
| Latest commit: |
0591bf6
|
| Status: | 🚫 Build failed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/services/notification-service.ts`:
- Around line 266-287: The four methods getNewMessages, getNewOrders,
getOrderStatusUpdates, and getNewReviews currently call sdk.documents.query(...)
with unsafe "as any" casts and untyped document variables; replace those calls
with the queryDocuments(sdk, queryOptions) helper and strongly type the results
using the existing interfaces DirectMessageDocument, StoreOrderDocument,
OrderStatusUpdateDocument, and StoreReviewDocument respectively, remove
normalizeSDKResponse and the any casts, type local vars (e.g., documents,
orders, statusUpdates, reviews) appropriately, and use .catch(() => []) on
queryDocuments to return an empty array on error so downstream code (like
building orderStoreMap from order.$id) has proper types.
- Add new notification types: newMessage, orderReceived, orderStatusUpdate, newReview - Query DM contract for new direct messages (recipientId index) - Query storefront contract for new orders, status updates, and reviews - Add Messages and Orders tabs to notifications page - Display action buttons for navigating to messages and orders - Show order status badges and review ratings in notifications - Add notification settings for orders and reviews - All notification queries run in parallel for efficiency - Proper chunking for order status updates to handle large order counts https://claude.ai/code/session_01ABKfq9pPndsHuxrweAZ66L
Convert null to undefined for optional fields to satisfy TypeScript types. https://claude.ai/code/session_01ABKfq9pPndsHuxrweAZ66L
Fix TypeScript error where order.$id was unknown type. https://claude.ai/code/session_01ABKfq9pPndsHuxrweAZ66L
30e4cf1 to
018227f
Compare
…ifications - Replace sdk.documents.query with queryDocuments helper - Use DirectMessageDocument, StoreOrderDocument, OrderStatusUpdateDocument, and StoreReviewDocument interfaces for proper typing - Remove normalizeSDKResponse and unsafe 'as any' casts - Use .catch(() => []) for error handling on all queries - Add recipientId field to DirectMessageDocument interface https://claude.ai/code/session_01ABKfq9pPndsHuxrweAZ66L
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/services/notification-service.ts (1)
96-96:⚠️ Potential issue | 🔴 CriticalBuild failure:
normalizeSDKResponseis not defined or imported.The pipeline fails with "Cannot find name 'normalizeSDKResponse'" at this line. This function is used here and at lines 139 and 247, but it's never imported. The new methods (getNewMessages, getNewOrders, etc.) correctly use the
queryDocumentshelper which handles normalization internally.Either import
normalizeSDKResponsefrom./sdk-helpers, or refactor these older methods to usequeryDocumentsfor consistency with the new code.Option 1: Import normalizeSDKResponse
-import { identifierToBase58, queryDocuments, QueryDocumentsOptions } from './sdk-helpers'; +import { identifierToBase58, queryDocuments, QueryDocumentsOptions, normalizeSDKResponse } from './sdk-helpers';Option 2 (preferred): Refactor to use queryDocuments consistently
async getNewFollowers(userId: string, sinceTimestamp: number): Promise<RawNotification[]> { try { const sdk = await getEvoSdk(); - // SDK query types are incomplete, cast needed for valid query options - const response = await sdk.documents.query({ + const queryOptions: QueryDocumentsOptions = { dataContractId: YAPPR_CONTRACT_ID, documentTypeName: 'follow', where: [ ['followingId', '==', userId], ['$createdAt', '>', sinceTimestamp] ], orderBy: [['followingId', 'asc'], ['$createdAt', 'asc']], limit: NOTIFICATION_QUERY_LIMIT - } as any); + }; - const documents = normalizeSDKResponse(response); + const documents = await queryDocuments(sdk, queryOptions); - return documents.map((doc: any) => ({ + return documents.map((doc) => ({ id: doc.$id, type: 'follow' as const, fromUserId: doc.$ownerId, // The follower createdAt: doc.$createdAt }));Apply the same pattern to
getPrivateFeedNotificationsandgetNewMentions.
🤖 Fix all issues with AI agents
In `@lib/services/notification-service.ts`:
- Around line 385-400: The map lookup can fail because orderStoreMap is keyed by
order.$id while the lookup uses the potentially different format
identifierToBase58(doc.orderId); fix by normalizing both sides to the same
identifier format: when building orderStoreMap, use a normalized key (e.g.,
identifierToBase58(order.$id) or identifierToBase58(order.storeId) as
appropriate) and when creating the status update compute a normalizedOrderId
(e.g., doc.orderId ? identifierToBase58(doc.orderId) :
identifierToBase58(doc.$id)) and use orderStoreMap.get(normalizedOrderId) for
storeId; update references to orderStoreMap, orders, identifierToBase58,
statusUpdates, doc.orderId and doc.$id accordingly and ensure you preserve
existing null/undefined fallbacks.
🧹 Nitpick comments (1)
app/notifications/page.tsx (1)
388-425: Consider extracting notification URL to avoid duplication.The URL for
newMessageaction button (line 391) duplicates the logic ingetNotificationUrl(lines 51-54). Consider reusinggetNotificationUrlhere for consistency.Suggested refactor
{/* Action buttons for DM notifications */} {notification.type === 'newMessage' && ( <Link - href={notification.conversationId ? `/messages?conversation=${notification.conversationId}` : '/messages'} + href={getNotificationUrl(notification) || '/messages'} onClick={(e) => e.stopPropagation()}
| const orderStoreMap = new Map<string, string>(); | ||
| for (const order of orders) { | ||
| const storeId = order.storeId ? identifierToBase58(order.storeId) : undefined; | ||
| if (storeId) { | ||
| orderStoreMap.set(order.$id, storeId); | ||
| } | ||
| } | ||
|
|
||
| return statusUpdates.map((doc) => { | ||
| const orderId = (doc.orderId ? identifierToBase58(doc.orderId) : null) ?? doc.$id; | ||
| return { | ||
| id: `status-${doc.$id}`, | ||
| type: 'orderStatusUpdate' as const, | ||
| fromUserId: doc.$ownerId, // The seller who posted the update | ||
| orderId, | ||
| storeId: orderStoreMap.get(orderId), |
There was a problem hiding this comment.
Potential key mismatch in orderStoreMap lookup.
The map is keyed by order.$id (line 389), but the lookup on line 400 uses orderId which is derived from identifierToBase58(doc.orderId). If the status update's orderId field stores a different format than order.$id, this lookup will silently fail and storeId will be undefined.
Consider normalizing both to the same format:
Suggested fix
// Build a map of orderId -> storeId from orders for enrichment
const orderStoreMap = new Map<string, string>();
for (const order of orders) {
const storeId = order.storeId ? identifierToBase58(order.storeId) : undefined;
if (storeId) {
- orderStoreMap.set(order.$id, storeId);
+ // Use base58-converted $id for consistent lookup
+ const orderIdKey = identifierToBase58(order.$id) ?? order.$id;
+ orderStoreMap.set(orderIdKey, storeId);
}
}
return statusUpdates.map((doc) => {
const orderId = (doc.orderId ? identifierToBase58(doc.orderId) : null) ?? doc.$id;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const orderStoreMap = new Map<string, string>(); | |
| for (const order of orders) { | |
| const storeId = order.storeId ? identifierToBase58(order.storeId) : undefined; | |
| if (storeId) { | |
| orderStoreMap.set(order.$id, storeId); | |
| } | |
| } | |
| return statusUpdates.map((doc) => { | |
| const orderId = (doc.orderId ? identifierToBase58(doc.orderId) : null) ?? doc.$id; | |
| return { | |
| id: `status-${doc.$id}`, | |
| type: 'orderStatusUpdate' as const, | |
| fromUserId: doc.$ownerId, // The seller who posted the update | |
| orderId, | |
| storeId: orderStoreMap.get(orderId), | |
| const orderStoreMap = new Map<string, string>(); | |
| for (const order of orders) { | |
| const storeId = order.storeId ? identifierToBase58(order.storeId) : undefined; | |
| if (storeId) { | |
| // Use base58-converted $id for consistent lookup | |
| const orderIdKey = identifierToBase58(order.$id) ?? order.$id; | |
| orderStoreMap.set(orderIdKey, storeId); | |
| } | |
| } | |
| return statusUpdates.map((doc) => { | |
| const orderId = (doc.orderId ? identifierToBase58(doc.orderId) : null) ?? doc.$id; | |
| return { | |
| id: `status-${doc.$id}`, | |
| type: 'orderStatusUpdate' as const, | |
| fromUserId: doc.$ownerId, // The seller who posted the update | |
| orderId, | |
| storeId: orderStoreMap.get(orderId), |
🤖 Prompt for AI Agents
In `@lib/services/notification-service.ts` around lines 385 - 400, The map lookup
can fail because orderStoreMap is keyed by order.$id while the lookup uses the
potentially different format identifierToBase58(doc.orderId); fix by normalizing
both sides to the same identifier format: when building orderStoreMap, use a
normalized key (e.g., identifierToBase58(order.$id) or
identifierToBase58(order.storeId) as appropriate) and when creating the status
update compute a normalizedOrderId (e.g., doc.orderId ?
identifierToBase58(doc.orderId) : identifierToBase58(doc.$id)) and use
orderStoreMap.get(normalizedOrderId) for storeId; update references to
orderStoreMap, orders, identifierToBase58, statusUpdates, doc.orderId and
doc.$id accordingly and ensure you preserve existing null/undefined fallbacks.
|
🕓 Ready for review — 59 ahead in queue (commit 0591bf6) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
At exact head 0591bf6, the PR fails TypeScript compilation because normalizeSDKResponse is still referenced after its import was removed. The new DM and store notification paths also contain confirmed query, authorization, persistence, badge, and navigation defects, so the PR requires changes.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 9 blocking
2 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `lib/services/notification-service.ts`:
- [BLOCKING] lib/services/notification-service.ts:4: Restore the removed normalizeSDKResponse import
This refactor removed `normalizeSDKResponse` from the import, but lines 96, 139, and 247 still call it. `npx tsc --noEmit` reproduces three TS2304 errors, so the production build cannot pass type checking.
- [BLOCKING] lib/services/notification-service.ts:277-285: Query the v3 DM model instead of a nonexistent recipient field
The active v3 write path creates `directMessage` documents with only `conversationId` and `encryptedContent`; recipient identity is stored on `conversationInvite`. The deployed v3 model therefore has no `recipientId` field or `receiverMessages` index on `directMessage`. This query is rejected and the catch converts that failure to an empty array, silently suppressing every received-message notification. Adding `recipientId` to the TypeScript interface does not change deployed documents.
- [BLOCKING] lib/services/notification-service.ts:372-378: Select the compound index for order status queries
The storefront contract exposes status lookups through `orderAndTime`, whose fields are `[orderId, $createdAt]`. This `in` plus timestamp-range query omits the matching ordering clauses, unlike the repository's other compound Platform queries. Platform can reject the query, and the catch then turns the failure into an empty result, preventing buyers from receiving status notifications.
- [BLOCKING] lib/services/notification-service.ts:347-351: Fetch status updates for orders beyond the newest 100
The buyer-order query retrieves only the newest 100 orders and never paginates. A recent status update for the buyer's 101st-most-recent order can therefore never be discovered. The later chunking does not address this because `orderIds` can contain at most 100 entries, so it always produces at most one chunk.
- [BLOCKING] lib/services/notification-service.ts:393-403: Verify status updates were authored by the order's seller
The contract validates the shape of `orderStatusUpdate` but does not require its owner to match the referenced order's `sellerId`. Any identity can publish an update for a public order ID, and this new notification path presents that identity and supplied status as an authentic seller update. Use the already-fetched orders to discard updates whose `$ownerId` does not match the corresponding seller.
In `lib/store.ts`:
- [BLOCKING] lib/store.ts:234-236: Merge new settings into persisted notification preferences
Zustand persist performs a shallow merge by default. For existing users, the persisted `notificationSettings` object lacks `orders` and `reviews` and replaces the complete default object during hydration. Those properties remain undefined, so the notifications page treats them as disabled while the settings page's `Object.entries` rendering provides no switches through which to enable them.
In `app/notifications/page.tsx`:
- [BLOCKING] app/notifications/page.tsx:391: Use the query parameter supported by the messages page
The messages page reads only `startConversation`; it never reads `conversation`. The new Open Messages action therefore opens the inbox without selecting the sender's conversation. The DM branch in `getNotificationUrl()` also constructs the same unsupported parameter.
- [BLOCKING] app/notifications/page.tsx:419: Navigate review notifications to a page that displays reviews
`/orders/seller` loads seller orders and status controls but never fetches or renders store reviews, so the new View Review action cannot show the referenced review. The notification already carries `storeId`, and `/store/view?id=...` is the route that loads that store's reviews.
In `components/layout/sidebar.tsx`:
- [BLOCKING] components/layout/sidebar.tsx:74: Exclude disabled notification categories from the sidebar badge
The notification page hides disabled message, order, and review notifications and hides Mark all as read when no enabled unread notifications remain. The sidebar still counts every unread notification without consulting those preferences. Disabling one of the new categories can therefore leave a persistent badge for notifications the user cannot see or clear from the page.
| import { normalizeSDKResponse, identifierToBase58, queryDocuments, QueryDocumentsOptions } from './sdk-helpers'; | ||
| import { YAPPR_CONTRACT_ID } from '../constants'; | ||
| import { Notification, User, Post } from '../types'; | ||
| import { identifierToBase58, queryDocuments, QueryDocumentsOptions } from './sdk-helpers'; |
There was a problem hiding this comment.
🔴 Blocking: Restore the removed normalizeSDKResponse import
This refactor removed normalizeSDKResponse from the import, but lines 96, 139, and 247 still call it. npx tsc --noEmit reproduces three TS2304 errors, so the production build cannot pass type checking.
| import { identifierToBase58, queryDocuments, QueryDocumentsOptions } from './sdk-helpers'; | |
| import { normalizeSDKResponse, identifierToBase58, queryDocuments, QueryDocumentsOptions } from './sdk-helpers'; |
source: ['codex']
| where: [ | ||
| ['recipientId', '==', userId], | ||
| ['$createdAt', '>', sinceTimestamp] | ||
| ], | ||
| orderBy: [['recipientId', 'asc'], ['$createdAt', 'asc']], | ||
| limit: NOTIFICATION_QUERY_LIMIT | ||
| }; | ||
|
|
||
| const documents = await queryDocuments(sdk, queryOptions).catch(() => []) as DirectMessageDocument[]; |
There was a problem hiding this comment.
🔴 Blocking: Query the v3 DM model instead of a nonexistent recipient field
The active v3 write path creates directMessage documents with only conversationId and encryptedContent; recipient identity is stored on conversationInvite. The deployed v3 model therefore has no recipientId field or receiverMessages index on directMessage. This query is rejected and the catch converts that failure to an empty array, silently suppressing every received-message notification. Adding recipientId to the TypeScript interface does not change deployed documents.
source: ['codex']
| where: [ | ||
| ['orderId', 'in', chunk], | ||
| ['$createdAt', '>', sinceTimestamp] | ||
| ], | ||
| limit: NOTIFICATION_QUERY_LIMIT | ||
| }; | ||
| return queryDocuments(sdk, statusQueryOptions).catch(() => []) as Promise<OrderStatusUpdateDocument[]>; |
There was a problem hiding this comment.
🔴 Blocking: Select the compound index for order status queries
The storefront contract exposes status lookups through orderAndTime, whose fields are [orderId, $createdAt]. This in plus timestamp-range query omits the matching ordering clauses, unlike the repository's other compound Platform queries. Platform can reject the query, and the catch then turns the failure into an empty result, preventing buyers from receiving status notifications.
| where: [ | |
| ['orderId', 'in', chunk], | |
| ['$createdAt', '>', sinceTimestamp] | |
| ], | |
| limit: NOTIFICATION_QUERY_LIMIT | |
| }; | |
| return queryDocuments(sdk, statusQueryOptions).catch(() => []) as Promise<OrderStatusUpdateDocument[]>; | |
| const statusQueryOptions: QueryDocumentsOptions = { | |
| dataContractId: YAPPR_STOREFRONT_CONTRACT_ID, | |
| documentTypeName: 'orderStatusUpdate', | |
| where: [ | |
| ['orderId', 'in', chunk], | |
| ['$createdAt', '>', sinceTimestamp] | |
| ], | |
| orderBy: [['orderId', 'asc'], ['$createdAt', 'asc']], | |
| limit: NOTIFICATION_QUERY_LIMIT | |
| }; |
source: ['codex']
| orderBy: [['$ownerId', 'asc'], ['$createdAt', 'desc']], | ||
| limit: NOTIFICATION_QUERY_LIMIT | ||
| }; | ||
|
|
||
| const orders = await queryDocuments(sdk, ordersQueryOptions).catch(() => []) as StoreOrderDocument[]; |
There was a problem hiding this comment.
🔴 Blocking: Fetch status updates for orders beyond the newest 100
The buyer-order query retrieves only the newest 100 orders and never paginates. A recent status update for the buyer's 101st-most-recent order can therefore never be discovered. The later chunking does not address this because orderIds can contain at most 100 entries, so it always produces at most one chunk.
source: ['codex']
| return statusUpdates.map((doc) => { | ||
| const orderId = (doc.orderId ? identifierToBase58(doc.orderId) : null) ?? doc.$id; | ||
| return { | ||
| id: `status-${doc.$id}`, | ||
| type: 'orderStatusUpdate' as const, | ||
| fromUserId: doc.$ownerId, // The seller who posted the update | ||
| orderId, | ||
| storeId: orderStoreMap.get(orderId), | ||
| orderStatus: doc.status, | ||
| createdAt: doc.$createdAt | ||
| }; |
There was a problem hiding this comment.
🔴 Blocking: Verify status updates were authored by the order's seller
The contract validates the shape of orderStatusUpdate but does not require its owner to match the referenced order's sellerId. Any identity can publish an update for a public order ID, and this new notification path presents that identity and supplied status as an authentic seller update. Use the already-fetched orders to discard updates whose $ownerId does not match the corresponding seller.
source: ['codex']
| {/* Action buttons for DM notifications */} | ||
| {notification.type === 'newMessage' && ( | ||
| <Link | ||
| href={notification.conversationId ? `/messages?conversation=${notification.conversationId}` : '/messages'} |
There was a problem hiding this comment.
🔴 Blocking: Use the query parameter supported by the messages page
The messages page reads only startConversation; it never reads conversation. The new Open Messages action therefore opens the inbox without selecting the sender's conversation. The DM branch in getNotificationUrl() also constructs the same unsupported parameter.
| href={notification.conversationId ? `/messages?conversation=${notification.conversationId}` : '/messages'} | |
| href={`/messages?startConversation=${notification.from.id}`} |
source: ['codex']
| )} | ||
| {notification.type === 'newReview' && ( | ||
| <Link | ||
| href="/orders/seller" |
There was a problem hiding this comment.
🔴 Blocking: Navigate review notifications to a page that displays reviews
/orders/seller loads seller orders and status controls but never fetches or renders store reviews, so the new View Review action cannot show the referenced review. The notification already carries storeId, and /store/view?id=... is the route that loads that store's reviews.
| href="/orders/seller" | |
| href={notification.storeId ? `/store/view?id=${notification.storeId}` : '/store'} |
source: ['codex']
3 participants
Summary
Extends the notification system to support direct messages and e-commerce features including new orders, order status updates, and customer reviews. This enables sellers to receive notifications for store activity and buyers to track order progress.
Key Changes
Notification Types
newMessage,orderReceived,orderStatusUpdate,newReviewNotificationinterface to include optional fields for DM and store data (conversationId, orderId, orderStatus, reviewRating, etc.)Notification Service (
lib/services/notification-service.ts)getNewMessages()to query direct messages from the DM contractgetNewOrders()to fetch new orders received by sellersgetOrderStatusUpdates()to track order status changes for buyers (with chunked querying for efficiency)getNewReviews()to fetch new reviews on seller storesfetchNotifications()to run all notification queries in parallel batchesUI Components (
app/notifications/page.tsx)Settings & Store
ordersandreviewsnotification settings toNotificationSettingsConstants
YAPPR_DM_CONTRACT_IDandYAPPR_STOREFRONT_CONTRACT_IDImplementation Details
https://claude.ai/code/session_01ABKfq9pPndsHuxrweAZ66L
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.