feat(api): notifications/activities wiring and guest auth (4/4) - #437
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
motirebuma
left a comment
There was a problem hiding this comment.
hi @niamao, looks good. Resolvers are simple and well-tested.
What's good
-
activityResolver- properly paginated query that supports search, date filtering, activity type filtering + "activity" following feed fallback ifuser_idis empty.normalizeActivityEventsparser handles both cases whenactivityEventcomes as JSON string (query variables) or array.toActivityEntityproperly normalizes DB ObjectIds to strings. -
notificationResolver- simple and correct, queriesstatus: 'new'and sorts bycreated: -1, properly normalizes IDs. -
Schema wiring - both resolvers are attached in
server.ts, SDL queries are properly defined.activitiesquery accepts all the pagination/filter arguments, resolves to expected object.notificationsquery returns[Notification!]!, which is a list of non-null Notification objects. -
Tests - activity resolver is tested for auth, pagination, filtering, and fallback. Notification resolver is tested for auth and empty state, both using the same mockContext pattern as before.
Issues
activityEventis a JSON scalar in SDL - theactivitiesquery definesactivityEvent: JSON. While it's handled correctly by thenormalizeActivityEventsparser, it's not ideal for a couple of reasons:
GraphQL scalar JSON is just a string, so this field could technically accept any string, including non-array values, and it'd be handled as "any JSON". It's better to specify this field as either [String!] (array of strings, each representing an event type) or create an ActivityEventType enum if there's a finite list of event types. With this design, activityEvent field in SDL can be changed to [String!] and the query parser will still work.
-
notificationResolverlacks pagination. As written, it'll return all the notifications with status new, ordered by created DESC. If someone has thousands of notifications, this could become a problem. Perhaps add alimitargument to the query. Even though it's not used in the UI, it'll help with performance. -
toActivityEntityassumes thatdoc.userIdis present.userId: toId(doc.userId) ?? ''will silently swallow any cases whendoc.userIdwas unexpectedly absent. It's better to fail loudly, or at the very least, log this condition, since user ID is always supposed to be present in activity documents.
Nits
-
normalizeActivityEventsswallows any errors that may occur when parsing JSON string, which is correct and expected, but it may be worth logging in case of unexpected issues on the client side. -
The line
Activity.find(searchArgs).sort({ created: -1 }).skip(offset).limit(limit).lean()is correct, but if it's not already indexed, adding an index onuserIdandcreatedfields (as a descending array) would improve performance.
thank you @niamao
Register notification and activity resolvers, and improve guest-session handling in Apollo/auth utilities so unauthenticated flows fail cleanly. Co-authored-by: Cursor <[email protected]>
Use ActivityEventType[] instead of JSON for activityEvent, fail loudly on missing activity userId, paginate notifications with a capped limit, and add a userId+created index for the activity feed. Co-authored-by: Cursor <[email protected]>
|
Addressed the review suggestions:
|
motirebuma
left a comment
There was a problem hiding this comment.
hi @niamao, all review points addressed. Good job all around.
Previous issues
All addressed.
-
activityEventtype was too loose - FIXED. Changed fromactivityEvent: JSONtoactivityEvent: [ActivityEventType!]-normalizeActivityEventsparses string as ActivityEventType array. Invalid event strings are filtered out (logger.warn). JSON parse errors are not silenced - function exported and has tests for arrays, legacy strings, and filtering of unknown event types. -
Notifications query lacked pagination - FIXED. Added
limit: Intto the notifications query. Default limit is 50, max is 100 (see DEFAULT_NOTIFICATION_LIMIT and MAX_NOTIFICATION_LIMIT constants). Input is sanitized (Math.floor, clamped to [1, MAX] etc.). The resolver’s parameter type is changed to{ limit?: number | null }. -
toActivityEntitywas swallowing errors by defaulting to empty string - FIXED. Now throws GraphQLError with messagemissing required userId. Has a test case for it ('rejects activities missing userId').
What's good
Same as in the previous reviews:
-
activityResolver: pagination + search + dates + activity type filter + activity feed for followed users -
notificationResolver: unread count query is simple and does the right thing -
Schema wiring in the
server.tsis good (has SDL) - no issues found -
Test coverage for both resolvers is good - used the same mockContext pattern as in the existing code
Thank you @niamao
Stack
Part 4 of 4 — merge last, after Part 3 (#436 /
presence-persist).profile-bioavatar-consistencypresence-persistapi-wiringAfter Part 3 merges, retarget this PR’s base to
main(or rebase ontomain).Related issue: #362
Summary
notificationResolverandactivityResolverin the GraphQL serverTest plan