feat:publish auth notifications#32
Merged
Merged
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds scaffoldable auth notification templates (email verification, password reset), relaxes notification ChangesAuth Notification Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
packages/notifications/src/contracts.ts (2)
436-444: Type assertion bridges input/output signature mismatch.The type assertion on line 444 tells TypeScript that the normalized object (which has
via(): readonly NotificationChannelName[]from the input) satisfiesNotificationDefinition<TNotifiable, TBuild>(which declaresvia(): readonly Extract<keyof TBuild, string>[]).This is safe in practice because notification definitions are immediately consumed by the runtime dispatcher, which performs validation. However, if library consumers were to call
.via()directly and rely on the return type, they might encounter channels without builders despite TypeScript's guarantees.The tradeoff (better DX vs. weaker compile-time safety) appears intentional based on the PR objectives.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/notifications/src/contracts.ts` around lines 436 - 444, The type assertion hides a mismatch between definition.via() (which can return arbitrary channel names) and NotificationDefinition.via (which must be Extract<keyof TBuild, string>); fix by deriving normalized.via from definition.via but explicitly filtering/mapping its results to only include keys present on the computed build object (the build variable) so the runtime value matches the declared type, e.g. compute a filtered array from definition.via()?.filter(name => name in build) and use that as the via implementation for normalized (referencing normalized, definition, build, via and NotificationDefinition) instead of relying on a blanket as NotificationDefinition<...> assertion.
203-211: Type safety tradeoff:via()return type loosened.The new
NotificationDefinitionInputtype allowsvia()to return anyNotificationChannelName, not just channels with corresponding builders inbuild. This removes the need foras constassertions (better DX), but weakens compile-time guarantees.The type system previously ensured
via()could only returnExtract<keyof TBuild, string>[], catching channel/builder mismatches at compile time. Now these are caught at runtime (lines 417-418 validatebuildhas entries, and the runtime gracefully handles missing builders as per-channel failures per test coverage).This is a deliberate tradeoff documented in the PR summary, but be aware that TypeScript won't catch cases where
via()returns channels without builders.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/notifications/src/contracts.ts` around lines 203 - 211, The via() return type was loosened and now allows any NotificationChannelName which can mismatch TBuild; change the via signature in NotificationDefinitionInput to return readonly (Extract<keyof TBuild, string>)[] instead of readonly NotificationChannelName[] so TypeScript enforces that via() can only list channels that have corresponding builders in TBuild (update the via method on the NotificationDefinitionInput type and keep TBuild/NotificationBuildFactories references intact).apps/blog-nuxt/server/notifications/auth/email-verification.ts (1)
20-23: 💤 Low valueConsider more user-friendly date formatting.
Same as the password reset notification,
expiresAt.toUTCString()produces a technical format. Consider using a more readable format for better UX.Example:
- `This verification link expires at ${input.expiresAt.toUTCString()}.`, + `This verification link expires at ${input.expiresAt.toLocaleString('en-US', { dateStyle: 'medium', timeStyle: 'short', timeZone: 'UTC' })} UTC.`,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/blog-nuxt/server/notifications/auth/email-verification.ts` around lines 20 - 23, The expiration date string in the email body currently uses input.expiresAt.toUTCString(), which is technical; replace it with a more user-friendly formatted date (e.g. input.expiresAt.toLocaleString(locale, { dateStyle: 'medium', timeStyle: 'short' }) or the project's shared format helper used in the password reset notification) inside the same lines array so the message reads naturally and respects locale/timezone when rendering the expiresAt value.apps/blog-next/server/notifications/auth/password-reset.ts (1)
18-21: 💤 Low valueConsider more user-friendly date formatting.
expiresAt.toUTCString()produces a technical format like"Fri, 09 May 2026 14:30:00 GMT". For better UX, consider usingtoLocaleString()or a custom format that's easier for users to read, such as"May 9, 2026 at 2:30 PM UTC".Example:
- `This reset link expires at ${input.expiresAt.toUTCString()}.`, + `This reset link expires at ${input.expiresAt.toLocaleString('en-US', { dateStyle: 'medium', timeStyle: 'short', timeZone: 'UTC' })} UTC.`,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/blog-next/server/notifications/auth/password-reset.ts` around lines 18 - 21, The expiration line uses input.expiresAt.toUTCString(), which is technical; replace it with a user-friendly formatting (e.g., input.expiresAt.toLocaleString(...) or a small formatter that produces "May 9, 2026 at 2:30 PM UTC") so the lines array reads a human-friendly date; update the line that builds the reset message (the element referencing input.expiresAt in the lines array) to format the date with locale/time options and include the timezone label (UTC) for clarity.packages/cli/tests/cli.test.ts (2)
831-845: ⚡ Quick winAssert notification identifiers, not only
defineNotificationusage.Lines 841-844 only prove the helper is present; they can still pass if the notification IDs/types drift. Add explicit assertions for
auth.email-verificationandauth.password-reset.Suggested test tightening
expect(await readFile(join(authNotificationsRoot, 'server/notifications/auth/email-verification.ts'), 'utf8')) .toContain('export default defineNotification') + expect(await readFile(join(authNotificationsRoot, 'server/notifications/auth/email-verification.ts'), 'utf8')) + .toContain('auth.email-verification') expect(await readFile(join(authNotificationsRoot, 'server/notifications/auth/password-reset.ts'), 'utf8')) .toContain('export default defineNotification') + expect(await readFile(join(authNotificationsRoot, 'server/notifications/auth/password-reset.ts'), 'utf8')) + .toContain('auth.password-reset')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/tests/cli.test.ts` around lines 831 - 845, The test currently only checks for defineNotification usage; update the assertions to also verify the concrete notification identifiers by asserting the file contents for the exact ID strings "auth.email-verification" and "auth.password-reset". Locate the existing expects that read server/notifications/auth/email-verification.ts and server/notifications/auth/password-reset.ts (near the defineNotification checks) and add expects that the readFile result contains those identifier strings so the test fails if the notification IDs drift.
8585-8609: ⚡ Quick winAssert exact created/skipped paths instead of only counts.
Lines 8598-8608 use length checks only; this can miss wrong file targets. Verifying exact paths will make the test much more regression-resistant.
Suggested assertion upgrade
const first = await projectInternals.publishAuthNotificationsIntoProject(projectRoot) - expect(first.createdFiles).toHaveLength(2) - expect(first.skippedFiles).toHaveLength(0) + expect(first.createdFiles).toEqual(expect.arrayContaining([ + join(projectRoot, 'server/notifications/auth/email-verification.ts'), + join(projectRoot, 'server/notifications/auth/password-reset.ts'), + ])) + expect(first.skippedFiles).toEqual([]) @@ const second = await projectInternals.publishAuthNotificationsIntoProject(projectRoot) - expect(second.createdFiles).toHaveLength(0) - expect(second.skippedFiles).toHaveLength(2) + expect(second.createdFiles).toEqual([]) + expect(second.skippedFiles).toEqual(expect.arrayContaining([ + join(projectRoot, 'server/notifications/auth/email-verification.ts'), + join(projectRoot, 'server/notifications/auth/password-reset.ts'), + ]))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/tests/cli.test.ts` around lines 8585 - 8609, Update the test to assert exact file paths instead of only counts: after calling projectInternals.publishAuthNotificationsIntoProject(projectRoot) verify first.createdFiles and first.skippedFiles explicitly contain the full expected paths (e.g., using join(projectRoot, 'server/notifications/auth/email-verification.ts') and join(projectRoot, 'server/notifications/auth/password-reset.ts')) and likewise assert second.createdFiles and second.skippedFiles equal the expected arrays (or contain those exact paths) so the assertions reference the concrete file paths returned by publishAuthNotificationsIntoProject rather than only checking .length.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/blog-sveltekit/server/notifications/auth/email-verification.ts`:
- Line 22: The code uses input.expiresAt.toLocaleString() for the verification
expiry text which is inconsistent with other notifications; update the template
to use input.expiresAt.toUTCString() instead so the expiry timestamp is
consistently formatted in UTC across notifications (locate the interpolation
that constructs `This verification link expires at
${input.expiresAt.toLocaleString()}.` and replace toLocaleString() with
toUTCString()).
- Line 18: The email verification subject in
apps/blog-sveltekit/server/notifications/auth/email-verification.ts is
inconsistent ("Verify your email address NOW"); update the subject property to
match the other implementations by changing it to "Verify your email address" so
the subject string in that file aligns with
apps/blog-next/server/notifications/auth/email-verification.ts and the template
in packages/cli/src/project/scaffold/project-renderers.ts.
In `@apps/docs/docs/notifications/creating-notifications.md`:
- Around line 42-55: The example and explanation are inconsistent: update the
notification example so the via function accepts the recipient parameter (e.g.,
change via() to via(user) { return ['email'] }) to match the sentence that the
recipient passed to notify(...) is available as the first argument in via(...);
ensure this change appears in the invoicePaid notification defined with
defineNotification and that any references to via(...) in the snippet reflect
the recipient parameter.
In `@apps/docs/docs/notifications/setup-and-cli.md`:
- Around line 79-81: Update the CLI example to match the rest of the doc by
replacing the standalone invocation "holo auth:notifications:publish" with the
consistent form "npx holo auth:notifications:publish"; change the code block
showing the command so it uses the "npx holo" prefix (same style used throughout
the page) to avoid command-not-found confusion for local installs.
In `@packages/cli/src/cli.ts`:
- Around line 706-738: canSkipAppDiscovery's allowlist is missing the newly
added internal command name; add 'auth:notifications:publish' to the set/array
checked by canSkipAppDiscovery so the internal command won't trigger app
discovery errors. Locate the canSkipAppDiscovery function and include the exact
string 'auth:notifications:publish' alongside the other allowed command names
(e.g., in the array or map used for lookup) so the new command is recognized as
skippable.
In `@packages/core/src/portable/holo.ts`:
- Around line 1858-1871: The current resolveAuthNotification
(AuthNotificationModule, exportName, filePath) only checks for typeof object and
can accept malformed overrides that later break in notify(...); tighten the
guard to validate the minimal notification contract before returning: require
that the resolved notification is an object and either exposes a build function
(typeof notification.build === 'function') or has the basic fields used by
notify (e.g. title and body are non-empty strings), and throw the same Error
with filePath if the shape is invalid; update resolveAuthNotification to perform
these checks and return only a validated notification.
---
Nitpick comments:
In `@apps/blog-next/server/notifications/auth/password-reset.ts`:
- Around line 18-21: The expiration line uses input.expiresAt.toUTCString(),
which is technical; replace it with a user-friendly formatting (e.g.,
input.expiresAt.toLocaleString(...) or a small formatter that produces "May 9,
2026 at 2:30 PM UTC") so the lines array reads a human-friendly date; update the
line that builds the reset message (the element referencing input.expiresAt in
the lines array) to format the date with locale/time options and include the
timezone label (UTC) for clarity.
In `@apps/blog-nuxt/server/notifications/auth/email-verification.ts`:
- Around line 20-23: The expiration date string in the email body currently uses
input.expiresAt.toUTCString(), which is technical; replace it with a more
user-friendly formatted date (e.g. input.expiresAt.toLocaleString(locale, {
dateStyle: 'medium', timeStyle: 'short' }) or the project's shared format helper
used in the password reset notification) inside the same lines array so the
message reads naturally and respects locale/timezone when rendering the
expiresAt value.
In `@packages/cli/tests/cli.test.ts`:
- Around line 831-845: The test currently only checks for defineNotification
usage; update the assertions to also verify the concrete notification
identifiers by asserting the file contents for the exact ID strings
"auth.email-verification" and "auth.password-reset". Locate the existing expects
that read server/notifications/auth/email-verification.ts and
server/notifications/auth/password-reset.ts (near the defineNotification checks)
and add expects that the readFile result contains those identifier strings so
the test fails if the notification IDs drift.
- Around line 8585-8609: Update the test to assert exact file paths instead of
only counts: after calling
projectInternals.publishAuthNotificationsIntoProject(projectRoot) verify
first.createdFiles and first.skippedFiles explicitly contain the full expected
paths (e.g., using join(projectRoot,
'server/notifications/auth/email-verification.ts') and join(projectRoot,
'server/notifications/auth/password-reset.ts')) and likewise assert
second.createdFiles and second.skippedFiles equal the expected arrays (or
contain those exact paths) so the assertions reference the concrete file paths
returned by publishAuthNotificationsIntoProject rather than only checking
.length.
In `@packages/notifications/src/contracts.ts`:
- Around line 436-444: The type assertion hides a mismatch between
definition.via() (which can return arbitrary channel names) and
NotificationDefinition.via (which must be Extract<keyof TBuild, string>); fix by
deriving normalized.via from definition.via but explicitly filtering/mapping its
results to only include keys present on the computed build object (the build
variable) so the runtime value matches the declared type, e.g. compute a
filtered array from definition.via()?.filter(name => name in build) and use that
as the via implementation for normalized (referencing normalized, definition,
build, via and NotificationDefinition) instead of relying on a blanket as
NotificationDefinition<...> assertion.
- Around line 203-211: The via() return type was loosened and now allows any
NotificationChannelName which can mismatch TBuild; change the via signature in
NotificationDefinitionInput to return readonly (Extract<keyof TBuild, string>)[]
instead of readonly NotificationChannelName[] so TypeScript enforces that via()
can only list channels that have corresponding builders in TBuild (update the
via method on the NotificationDefinitionInput type and keep
TBuild/NotificationBuildFactories references intact).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8164af9b-75a6-4483-a58a-e70fb6dd27e8
📒 Files selected for processing (32)
apps/blog-next/server/notifications/auth/email-verification.tsapps/blog-next/server/notifications/auth/password-reset.tsapps/blog-nuxt/server/notifications/auth/email-verification.tsapps/blog-nuxt/server/notifications/auth/password-reset.tsapps/blog-sveltekit/server/notifications/auth/email-verification.tsapps/blog-sveltekit/server/notifications/auth/password-reset.tsapps/docs/docs/auth/email-verification.mdapps/docs/docs/auth/local-auth.mdapps/docs/docs/auth/password-reset.mdapps/docs/docs/mail/notifications.mdapps/docs/docs/notifications/creating-notifications.mdapps/docs/docs/notifications/custom-channels.mdapps/docs/docs/notifications/defining-notifications.mdapps/docs/docs/notifications/index.mdapps/docs/docs/notifications/queueing-notifications.mdapps/docs/docs/notifications/setup-and-cli.mdpackages/cli/src/cli.tspackages/cli/src/project.tspackages/cli/src/project/scaffold.tspackages/cli/src/project/scaffold/framework.tspackages/cli/src/project/scaffold/project-renderers.tspackages/cli/src/project/shared.tspackages/cli/tests/cli.test.tspackages/core/src/portable/holo.tspackages/core/tests/auth-runtime.test.tspackages/core/tests/runtime.test.tspackages/notifications/src/contracts.tspackages/notifications/tests/contracts.test.tspackages/notifications/tests/contracts.type.test.tspackages/notifications/tests/index.type.test.tspackages/notifications/tests/package.test.tspackages/notifications/tests/runtime.test.ts
Comment on lines
+42
to
+55
| If the message needs variables, create the notification from a function and pass the variables into that function. | ||
| The recipient passed to `notify(...)` is available as the first argument in `via(...)` and in every channel builder. | ||
|
|
||
| ```ts | ||
| import { defineNotification, notify } from '@holo-js/notifications' | ||
|
|
||
| const invoicePaid = (invoice: { | ||
| readonly id: string | ||
| readonly number: string | ||
| }) => defineNotification({ | ||
| type: 'invoice-paid', | ||
| via() { | ||
| return ['email'] | ||
| }, |
There was a problem hiding this comment.
Keep the example consistent with the via(...) explanation.
The paragraph says the recipient is available as the first argument to via(...), but the new snippet omits it. Either show via(user) { ... } here or soften the wording so the example and explanation match.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/docs/docs/notifications/creating-notifications.md` around lines 42 - 55,
The example and explanation are inconsistent: update the notification example so
the via function accepts the recipient parameter (e.g., change via() to
via(user) { return ['email'] }) to match the sentence that the recipient passed
to notify(...) is available as the first argument in via(...); ensure this
change appears in the invoicePaid notification defined with defineNotification
and that any references to via(...) in the snippet reflect the recipient
parameter.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/docs/docs/notifications/queueing-notifications.md`:
- Around line 73-76: The "Per-Channel Queue Settings" section shows a static
queue object (queue: { queue: 'notifications', afterCommit: true }) which
demonstrates global defaults rather than per-channel behavior; either rename the
section to reflect global queue defaults or update the example to use a
channel-aware API such as queue(channelName, options) (i.e., replace the static
queue object with a call to queue('email' or channel variable, { queue:
'notifications', afterCommit: true })) so the heading and code align; update any
surrounding explanatory text to match the chosen change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2855c2af-9896-4ab4-a6f0-fd7f76633587
📒 Files selected for processing (29)
apps/blog-next/server/notifications/auth/email-verification.tsapps/blog-next/server/notifications/auth/password-reset.tsapps/blog-nuxt/server/notifications/auth/email-verification.tsapps/blog-nuxt/server/notifications/auth/password-reset.tsapps/blog-sveltekit/server/notifications/auth/email-verification.tsapps/blog-sveltekit/server/notifications/auth/password-reset.tsapps/docs/docs/auth/email-verification.mdapps/docs/docs/auth/password-reset.mdapps/docs/docs/mail/notifications.mdapps/docs/docs/notifications.mdapps/docs/docs/notifications/creating-notifications.mdapps/docs/docs/notifications/defining-notifications.mdapps/docs/docs/notifications/index.mdapps/docs/docs/notifications/notification-channels.mdapps/docs/docs/notifications/notification-storage.mdapps/docs/docs/notifications/on-demand-notifications.mdapps/docs/docs/notifications/queueing-notifications.mdapps/docs/docs/notifications/setup-and-cli.mdpackages/adapter-nuxt/src/runtime/composables/forms.d.tspackages/cli/src/cli.tspackages/cli/src/project/scaffold/project-renderers.tspackages/cli/tests/cli.test.tspackages/core/src/portable/holo.tspackages/core/tests/runtime.test.tspackages/notifications/src/contracts.tspackages/notifications/src/runtime.tspackages/notifications/tests/contracts.test.tspackages/notifications/tests/contracts.type.test.tspackages/notifications/tests/runtime.test.ts
✅ Files skipped from review due to trivial changes (9)
- apps/docs/docs/notifications/setup-and-cli.md
- apps/docs/docs/auth/password-reset.md
- apps/docs/docs/notifications/on-demand-notifications.md
- apps/docs/docs/notifications/creating-notifications.md
- apps/docs/docs/notifications/notification-channels.md
- apps/docs/docs/notifications/index.md
- apps/docs/docs/mail/notifications.md
- apps/docs/docs/auth/email-verification.md
- packages/notifications/tests/contracts.test.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- apps/blog-next/server/notifications/auth/email-verification.ts
- apps/blog-sveltekit/server/notifications/auth/email-verification.ts
- packages/cli/src/cli.ts
- apps/blog-nuxt/server/notifications/auth/email-verification.ts
- packages/notifications/tests/contracts.type.test.ts
- packages/notifications/src/contracts.ts
- packages/cli/src/project/scaffold/project-renderers.ts
- apps/blog-next/server/notifications/auth/password-reset.ts
- apps/blog-sveltekit/server/notifications/auth/password-reset.ts
- packages/core/tests/runtime.test.ts
- packages/core/src/portable/holo.ts
- packages/cli/tests/cli.test.ts
This was referenced May 9, 2026
Merged
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 by CodeRabbit
New Features
Documentation
Refactor