Conversation
…dules Add focused tests for critical logic in http-manager, react client, and server handlers. Remove clearMemoryStore from public exports as it's only needed internally for testing. Co-Authored-By: Claude Haiku 4.5 <[email protected]>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
jln13x
changed the title
Contributor
Greptile SummaryAdded comprehensive notification management system with type-safe inbox support. The implementation includes a core manager with memory adapter, HTTP client for browser use, React hooks/context for UI integration, and Next.js server handlers for API routes. Key architectural decisions:
Implementation highlights:
Confidence Score: 5/5
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+271
to
+273
| async markManyAsRead(ids: string[]): Promise<void> { | ||
| await Promise.all(ids.map((id) => manager.markAsRead(id))); | ||
| }, |
Contributor
There was a problem hiding this comment.
style: markManyAsRead uses Promise.all with individual markAsRead calls, triggering separate API requests for each ID. Consider adding a batch endpoint /read-many for better performance with large ID arrays
Suggested change
| async markManyAsRead(ids: string[]): Promise<void> { | |
| await Promise.all(ids.map((id) => manager.markAsRead(id))); | |
| }, | |
| async markManyAsRead(ids: string[]): Promise<void> { | |
| await request( | |
| baseUrl, | |
| "/read-many", | |
| { | |
| method: "POST", | |
| body: JSON.stringify({ ids }), | |
| }, | |
| fetchFn, | |
| deserializeFn | |
| ); | |
| }, |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/notification/src/client/http-manager.ts
Line: 271:273
Comment:
**style:** `markManyAsRead` uses `Promise.all` with individual `markAsRead` calls, triggering separate API requests for each ID. Consider adding a batch endpoint `/read-many` for better performance with large ID arrays
```suggestion
async markManyAsRead(ids: string[]): Promise<void> {
await request(
baseUrl,
"/read-many",
{
method: "POST",
body: JSON.stringify({ ids }),
},
fetchFn,
deserializeFn
);
},
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
322
to
327
| async deleteMany(filter: Omit<NotificationFilter<Types>, "userId">): Promise<number> { | ||
| // List matching notifications and delete each | ||
| const notifications = await manager.list(filter); | ||
| await Promise.all(notifications.map((n) => manager.delete(n.id))); | ||
| return notifications.length; | ||
| }, |
Contributor
There was a problem hiding this comment.
style: deleteMany lists all matching notifications client-side then deletes individually. For large result sets, consider adding a server endpoint that performs bulk deletion to reduce network overhead
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/notification/src/client/http-manager.ts
Line: 322:327
Comment:
**style:** `deleteMany` lists all matching notifications client-side then deletes individually. For large result sets, consider adding a server endpoint that performs bulk deletion to reduce network overhead
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.- Fix non-null assertions by adding proper type guards - Remove unused variables and functions (filterKey, serializeFilter, NextResponse) - Add htmlFor attributes to labels for accessibility - Replace forEach with for...of to avoid implicit returns - Remove unused imports Co-Authored-By: Claude Haiku 4.5 <[email protected]>
- Add getId handler for server-side auth context - Remove userId from HTTP client calls (determined server-side) - Fix data type from unknown to Record<string, unknown> Co-Authored-By: Claude Opus 4.5 <[email protected]>
…ientId
- Rename package from @jlnstack/notification to @jlnstack/notifications
- Refactor send() API from send(type, options) to send({ type, ...options })
- Rename userId to recipientId throughout to support notifications to
any entity type (users, workspaces, teams, etc.)
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add parseNotificationData function that validates data through Standard Schema - Add NotificationValidationError class with descriptive error messages - Require all notification type schemas to implement Standard Schema (Zod, Valibot, etc.) - Return 400 Bad Request for validation errors in server handlers - Add comprehensive tests for schema validation behavior Co-Authored-By: Claude Opus 4.5 <[email protected]>
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
1 participant
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.
Summary
Complete notification management system with type-safe inbox support. Includes core manager with memory adapter, HTTP client for browser use, React hooks/context for UI integration, and Next.js server handlers for API routes.
Key Features
🤖 Generated with Claude Code