Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
✅ Deploy preview added
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Hey there and thank you for opening this pull request! 👋 We require pull request titles to follow specific formatting rules and it looks like your proposed title needs to be adjusted. Your PR title is: Requirements:
Expected format: Details: PR title must end with 'fixes TICKET-ID' (e.g., 'fixes NOV-123') or include ticket ID in branch name |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| "esbuild-plugin-solid": "^0.6.0", | ||
| "http-server": "^0.13.0", | ||
| "jest": "^29.3.1", | ||
| "jsdom": "^30.0.1", |
There was a problem hiding this comment.
The new jsdom@30.0.1 test dependency requires Node 22.22.2 or newer, while the repository declares all Node 22 releases as supported. Developers using Node 22.0.0 through 22.22.1 therefore satisfy the repository's engine declaration but cannot reliably run the migrated Vitest suites. The same dependency was added to packages/react/package.json; either use a compatible jsdom release or raise the declared Node floor.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/js/package.json
Line: 127
Comment:
**Node support range mismatch**
The new `jsdom@30.0.1` test dependency requires Node 22.22.2 or newer, while the repository declares all Node 22 releases as supported. Developers using Node 22.0.0 through 22.22.1 therefore satisfy the repository's engine declaration but cannot reliably run the migrated Vitest suites. The same dependency was added to `packages/react/package.json`; either use a compatible jsdom release or raise the declared Node floor.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| const deliveredAt = useMemo( | ||
| () => | ||
| Array.isArray(notification.deliveredAt) | ||
| ? notification.deliveredAt.map((date) => formatToRelativeTime({ fromDate: new globalThis.Date(date), locale })) | ||
| : null, | ||
| [notification.deliveredAt, locale] | ||
| ); |
There was a problem hiding this comment.
The minute tick re-renders this block, but deliveredAt is memoized without a time-dependent value. Its relative timestamps therefore do not advance unless the notification snapshot or locale changes, while the other date variants continue updating. Notifications with multiple delivery times will show stale elapsed values.
| const deliveredAt = useMemo( | |
| () => | |
| Array.isArray(notification.deliveredAt) | |
| ? notification.deliveredAt.map((date) => formatToRelativeTime({ fromDate: new globalThis.Date(date), locale })) | |
| : null, | |
| [notification.deliveredAt, locale] | |
| ); | |
| const deliveredAt = Array.isArray(notification.deliveredAt) | |
| ? notification.deliveredAt.map((date) => formatToRelativeTime({ fromDate: new globalThis.Date(date), locale })) | |
| : null; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react/src/components/notification-item/parts/Date.tsx
Line: 51-57
Comment:
**Delivery times stay stale**
The minute tick re-renders this block, but `deliveredAt` is memoized without a time-dependent value. Its relative timestamps therefore do not advance unless the notification snapshot or locale changes, while the other date variants continue updating. Notifications with multiple delivery times will show stale elapsed values.
```suggestion
const deliveredAt = Array.isArray(notification.deliveredAt)
? notification.deliveredAt.map((date) => formatToRelativeTime({ fromDate: new globalThis.Date(date), locale }))
: null;
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| const NotificationItemStub = () => null; | ||
|
|
||
| export const NotificationItem = Object.assign(NotificationItemStub, { | ||
| Avatar: NotificationItemStub, | ||
| Content: NotificationItemStub, | ||
| Text: NotificationItemStub, | ||
| Subject: NotificationItemStub, | ||
| Body: NotificationItemStub, | ||
| DefaultActions: NotificationItemStub, | ||
| CustomActions: NotificationItemStub, | ||
| Date: NotificationItemStub, | ||
| Dot: NotificationItemStub, | ||
| }); |
There was a problem hiding this comment.
The new server export types NotificationItem and all its static blocks as zero-argument functions, while the browser component requires a notification prop. It also omits NotificationItemProps from the server type exports. Code checked through the React or Next.js server conditions therefore rejects valid <NotificationItem notification={...}> usage and cannot import its public prop type. Type the stubs with their client props and re-export NotificationItemProps.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react/src/server/index.tsx
Line: 40-52
Comment:
**Server stub rejects props**
The new server export types `NotificationItem` and all its static blocks as zero-argument functions, while the browser component requires a `notification` prop. It also omits `NotificationItemProps` from the server type exports. Code checked through the React or Next.js server conditions therefore rejects valid `<NotificationItem notification={...}>` usage and cannot import its public prop type. Type the stubs with their client props and re-export `NotificationItemProps`.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
1 participant
What changed? Why was the change needed?
Inbox: improved rendering architecture
Screenshots
Expand for optional sections
Related enterprise PR
Special notes for your reviewer
Greptile Summary
The PR introduces a shared, framework-neutral Inbox UI core, changes the engine/host bridge to support in-place outlet updates, and adds composable React-native notification-item blocks while retaining Solid-rendered interactive islands.
@novu/js/ui-core.NotificationItemAPI through React and Next.js.Confidence Score: 3/5
The latest changes introduce no accepted new defect, but the PR is not yet safe to merge because two previously reported compatibility issues remain unresolved.
The declared Node range still permits Node 22 versions below the minimum required by jsdom 30.0.1, so supported contributors can be unable to run the migrated suites. The React server-condition
NotificationItemstub is still inferred as a zero-argument component andNotificationItemPropsis still not publicly re-exported there, so valid server-condition TypeScript usage remains rejected. The delivery-time display also remains stale because its memo excludes the minute tick that triggers surrounding renders.Files Needing Attention: packages/js/package.json, packages/react/package.json, packages/react/src/server/index.tsx, packages/react/src/components/notification-item/parts/Date.tsx
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart LR Data["@novu/js data and immutable notifications"] Core["@novu/js/ui-core<br/>stores, styles, formatting, controllers"] Engine["Solid Inbox engine<br/>shell and interactive islands"] Bridge["Bridge<br/>mount, update, unmount"] Host["React host<br/>outlets and NotificationItem blocks"] Data --> Core Core --> Engine Core --> Host Engine --> Bridge Bridge --> Host Host -->|mount islands| BridgeReviews (2): Last reviewed commit: "Merge branch 'next' into inbox-rendering..." | Re-trigger Greptile