feat(notifications): F05 in-app notification center — follow/like/comment fan-out, flat notify* - #124
Conversation
…ment fan-out, flat notify* preference rename Implements OpenSpec change f05-in-app-notifications. - DB: extend notificationTypeEnum with follow/like/comment; rename 5 user_preferences notification columns with notify_ prefix via data-preserving RENAME COLUMN. New migration 0012_f05_notifications (Drizzle Kit --custom workaround for non-interactive column-rename TTY prompt; snapshot manually aligned). - Shared: flatten UserPreferencesSchema, SelfPreferencesSchema, UserPreferencesOutputSchema, UserPreferences interface to 5 flat notify* top-level fields. F04 latent notifyMentionedInComment omission fixed structurally by the flatten (one-line flatten vs re-list every nest field at every projection site). - API: add 3 new fan-out creators (createFollowNotification, createLikeNotification, createCommentNotification) mirroring createMentionNotifications; new findNotifyTarget model loader; deps proxy extended. Wire at the 3 call sites fire-and-forget: follow/service.ts, recipe/service.ts:toggleLike, comment/service.ts:runCommentNotificationSideEffects. preference/index.ts + user/model.ts:findById flatten. utils/notify/index.ts 5 helper gate lines renamed. - Web: SettingsPage 5 toggles + toUserPreferences adapter + savePreferences PATCH all flattened. NotificationItem per-type switch (mention/follow/ like/comment + mentionGeneric fallback) with per-type link targets. NotificationListPage All/Unread filter via ?unreadOnly=true URL state. - i18n: en + tr parity (5 new keys); remove obsolete settings.emailNotifications. - Tests: shared schema + pbt; API notification service 3 new suites × 7 scenarios each + findNotifyTarget DB-backed suite + preference+user + follow/recipe/comment DB-backed fan-out tests + openapi coverage test; web NotificationItem/NotificationListPage/SettingsPage tests. - Plans: plans/F05-in-app-notifications.md prepended with ship banner; plans/F29-weekly-email-digest.md prepended with forward-compat note for the future implementer (don't add nested weeklyDigest, use flat notifyWeeklyDigest per the F05 pattern). Verification: make fmt + make check (302 files) + make lint (661 files) + make test (api 316 + shared 136 + web 1289 — all green) + openapi coverage test (19 in-scope routes, zero orphan tags).
|
Warning Review limit reached
Next review available in: 54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change adds persisted follow, like, and comment notifications, flattens notification preferences into ChangesIn-app notification expansion
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant A
participant B
A->>B: interaction
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
…ness check was producing a spurious 0013 migration When I used 'drizzle-kit generate --custom' (TTY-less workaround) for the F05 migration, I hand-edited the snapshot for the column renames but missed the enum extension. CI's db:generate step then compared the on-disk schema (4-value enum) against the snapshot (1-value enum) and produced a spurious 0013_quiet_paibok migration adding the 3 missing enum values — failing the freshness check. Fix: update meta/0012_snapshot.json public.notification_type enum values from ['mention'] to ['mention','follow','like','comment']. Re-ran 'make db-generate' — outputs 'No schema changes, nothing to migrate 😴'. Also extends AGENTS.md with the full --custom snapshot hygiene rule so this failure category is documented: every schema change in a --custom migration MUST be reflected in the manually-edited snapshot (renames, enum values, columns, indexes, constraints), with db:generate's 'nothing to migrate 😴' output as the freshness gate.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/modules/follow/service.ts (1)
35-51: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRemove the duplicate new-follower email fan-out.
createFollowNotificationalready checksnotifyNewFollower, persists thefollowrecord, and callsnotifyNewFollower, so the directnotifyNewFollower(...)inapps/api/src/modules/follow/service.ts:38-41sends the same email twice for one follow event. Keep the fan-out increateFollowNotificationonly, remove the direct import if unused, and await the single entry point in the fire-and-forget path.🤖 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/api/src/modules/follow/service.ts` around lines 35 - 51, Remove the direct notifyNewFollower call from the async follow handler and delete its import if unused; await createFollowNotification as the sole fan-out entry point while preserving the existing error logging and fire-and-forget wrapper.
🧹 Nitpick comments (4)
apps/web/src/pages/notifications/NotificationListPage.test.tsx (1)
183-188: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove stale TODO comments describing an unlanded feature that already ships in this PR.
The comment states T38's All/Unread filter UI "has NOT landed yet," but
NotificationListPage.tsx(lines 133-152, same PR) already renders theAll/Unreadbuttons and the tests below pass against that implementation. Leaving this comment risks confusing future readers into thinking these tests are speculative.♻️ Proposed cleanup
- // ── F05 filter UI tests (T41) ── - // TODO: needs signature confirm — actual `notificationApi.list(page: number, unreadOnly?: boolean)` - // (positional, NOT the object form the design hint described). T38's All/Unread filter UI - // has NOT landed yet; these tests will only pass once buttons labeled 'All' / 'Unread' - // (via t('notifications.all') / t('notifications.unread')) render in the page header. + // ── F05 filter UI tests (T41) ── + // Covers the All/Unread filter buttons in NotificationListPage.tsx, which call + // `notificationApi.list(page, unreadOnly)` positionally.🤖 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/web/src/pages/notifications/NotificationListPage.test.tsx` around lines 183 - 188, Remove the stale TODO and explanatory comments above the F05 filter UI tests in NotificationListPage.test.tsx, including the outdated claim that the All/Unread filter UI has not landed; leave the tests and implementation unchanged.apps/web/src/components/layout/NotificationItem.tsx (1)
68-95: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the repeated interpolation-with-fallback pattern.
The
like,comment, andmentioncases repeat the samemeta.recipeTitle ? t(key)...replace(...) : t('notifications.mentionGeneric')...shape. A small helper reduces duplication and keeps the three branches from drifting apart as more types are added.♻️ Proposed refactor
+ function withRecipeTitleOrGeneric(key: string, extra: Record<string, string> = {}) { + if (!meta.recipeTitle) return t('notifications.mentionGeneric').replace('{username}', username); + let result = t(key).replace('{recipeTitle}', meta.recipeTitle); + for (const [k, v] of Object.entries(extra)) result = result.replace(`{${k}}`, v); + return result; + } + const text = (() => { switch (notification.type) { case 'follow': return t('notifications.follow').replace('{actorUsername}', username); case 'like': - return meta.recipeTitle - ? t('notifications.like') - .replace('{actorUsername}', username) - .replace('{recipeTitle}', meta.recipeTitle) - : t('notifications.mentionGeneric').replace('{username}', username); + return withRecipeTitleOrGeneric('notifications.like', { actorUsername: username }); case 'comment': - return meta.recipeTitle - ? t('notifications.comment') - .replace('{actorUsername}', username) - .replace('{recipeTitle}', meta.recipeTitle) - : t('notifications.mentionGeneric').replace('{username}', username); + return withRecipeTitleOrGeneric('notifications.comment', { actorUsername: username }); case 'mention': - return meta.recipeTitle - ? t('notifications.mention') - .replace('{username}', username) - .replace('{recipeTitle}', meta.recipeTitle) - : t('notifications.mentionGeneric').replace('{username}', username); + return withRecipeTitleOrGeneric('notifications.mention', { username }); default: return t('notifications.mentionGeneric').replace('{username}', username); } })();🤖 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/web/src/components/layout/NotificationItem.tsx` around lines 68 - 95, Extract the repeated recipe-title interpolation and generic fallback logic from the notification text switch into a small local helper, then have the like, comment, and mention cases call it with their respective translation keys and placeholders. Preserve the existing follow behavior, translation keys, replacement values, and fallback behavior.apps/api/src/modules/follow/service.test.ts (1)
67-91: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
makeUseranddeleteUserWithBadgesare duplicated.Both helpers are identical to the versions in
apps/api/src/modules/recipe/service.test.tsLines 67-91. Move them to a shared test helper module and import them in both files. This keeps the badge-race retry logic in one place.🤖 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/api/src/modules/follow/service.test.ts` around lines 67 - 91, Move the duplicated makeUser and deleteUserWithBadges helpers from the follow and recipe service tests into a shared test-helper module, preserving their current database insertion and badge-race retry behavior. Import and use the shared helpers in both service.test.ts files, removing the local definitions.apps/api/src/modules/notification/service.ts (1)
158-412: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider extracting the shared fan-out body.
The three creators repeat the same nine steps: self-check,
findNotifyTarget, preference gate,createin an inner try, email in an inner try, and the outer log-and-rethrow. Only the preference key, the record fields, and the email call differ. Extract a helper that accepts those three parts. This keeps future notification types consistent and removes about 180 duplicated lines.🤖 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/api/src/modules/notification/service.ts` around lines 158 - 412, Extract the shared fan-out workflow from createFollowNotification, createLikeNotification, and createCommentNotification into a helper accepting the preference key, notification record fields, and email callback. Preserve each creator’s self-check, target lookup, preference gating, isolated create/email errors, completion logging, and outer log-and-rethrow behavior, then have the three public creators supply only their type-specific values.
🤖 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/api/src/modules/notification/service.test.ts`:
- Around line 380-490: Update the creator suites for createFollowNotification,
createLikeNotification, and createCommentNotification to assert email behavior
through their production call sites rather than assuming the creators own
delivery. Resolve duplicate ownership first, then add call-site-level assertions
that the combined follow, recipe, and comment flows send exactly one email,
while preserving the existing preference and failure coverage.
In `@apps/api/src/modules/notification/service.ts`:
- Around line 223-227: Email ownership is duplicated across creators and
callers, causing duplicate sends and bypassing preference gates. Make
notification creators the sole owners: keep the follow block in
apps/api/src/modules/notification/service.ts lines 223-227 and equivalent
creator blocks at lines 296-305 and 393-402, remove caller sends and correct
gating comments in apps/api/src/modules/comment/service.ts lines 164-183 and
apps/api/src/modules/recipe/service.ts lines 566-596. Update
apps/api/src/modules/notification/service.test.ts lines 380-701 to assert one
email per call site.
In `@apps/api/src/modules/preference/index.ts`:
- Around line 87-107: Update the PATCH validation flow around
UserPreferencesSchema to use a PATCH-only schema where notification booleans are
optional and have no defaults, then build flatData from only fields actually
present in the request. Ensure omitted preferences remain unchanged when
updating through the PATCH handler, including the fields copied by the flatData
construction.
In `@apps/web/src/pages/notifications/NotificationListPage.tsx`:
- Around line 133-152: Update the filter button group’s aria-label in the
notification list component to use a dedicated translation key describing
notification filtering, such as notifications.filterLabel, instead of
notifications.title. Add the corresponding translation entry in the project’s
existing notification translations.
In `@plans/F05-in-app-notifications.md`:
- Around line 3-6: Remove the standalone blank blockquote line between the
shipped banner and the implementation note in the plan content, keeping both
lines in one continuous blockquote so Markdownlint MD028 passes.
In `@plans/F29-weekly-email-digest.md`:
- Around line 3-5: Fix the MD028 violation in the blockquote at the start of the
document by replacing the blank line between the two quoted lines with a
blockquote continuation marker, joining them into one contiguous blockquote.
---
Outside diff comments:
In `@apps/api/src/modules/follow/service.ts`:
- Around line 35-51: Remove the direct notifyNewFollower call from the async
follow handler and delete its import if unused; await createFollowNotification
as the sole fan-out entry point while preserving the existing error logging and
fire-and-forget wrapper.
---
Nitpick comments:
In `@apps/api/src/modules/follow/service.test.ts`:
- Around line 67-91: Move the duplicated makeUser and deleteUserWithBadges
helpers from the follow and recipe service tests into a shared test-helper
module, preserving their current database insertion and badge-race retry
behavior. Import and use the shared helpers in both service.test.ts files,
removing the local definitions.
In `@apps/api/src/modules/notification/service.ts`:
- Around line 158-412: Extract the shared fan-out workflow from
createFollowNotification, createLikeNotification, and createCommentNotification
into a helper accepting the preference key, notification record fields, and
email callback. Preserve each creator’s self-check, target lookup, preference
gating, isolated create/email errors, completion logging, and outer
log-and-rethrow behavior, then have the three public creators supply only their
type-specific values.
In `@apps/web/src/components/layout/NotificationItem.tsx`:
- Around line 68-95: Extract the repeated recipe-title interpolation and generic
fallback logic from the notification text switch into a small local helper, then
have the like, comment, and mention cases call it with their respective
translation keys and placeholders. Preserve the existing follow behavior,
translation keys, replacement values, and fallback behavior.
In `@apps/web/src/pages/notifications/NotificationListPage.test.tsx`:
- Around line 183-188: Remove the stale TODO and explanatory comments above the
F05 filter UI tests in NotificationListPage.test.tsx, including the outdated
claim that the All/Unread filter UI has not landed; leave the tests and
implementation unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6bf88442-f430-4e40-965f-a824258c0fab
📒 Files selected for processing (38)
apps/api/src/modules/comment/service.test.tsapps/api/src/modules/comment/service.tsapps/api/src/modules/follow/service.test.tsapps/api/src/modules/follow/service.tsapps/api/src/modules/notification/model.test.tsapps/api/src/modules/notification/model.tsapps/api/src/modules/notification/service.test.tsapps/api/src/modules/notification/service.tsapps/api/src/modules/preference/index.tsapps/api/src/modules/preference/model.test.tsapps/api/src/modules/recipe/service.test.tsapps/api/src/modules/recipe/service.tsapps/api/src/modules/user/model.test.tsapps/api/src/modules/user/model.tsapps/api/src/utils/notify/index.tsapps/web/src/components/layout/NotificationItem.test.tsxapps/web/src/components/layout/NotificationItem.tsxapps/web/src/pages/notifications/NotificationListPage.test.tsxapps/web/src/pages/notifications/NotificationListPage.tsxapps/web/src/pages/settings/SettingsPage.test.tsxapps/web/src/pages/settings/SettingsPage.tsxopenspec/changes/f05-in-app-notifications/tasks.mdpackages/db/drizzle/0012_f05_notifications.sqlpackages/db/drizzle/meta/0012_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/schema.tspackages/shared/src/i18n/en.jsonpackages/shared/src/i18n/tr.jsonpackages/shared/src/schemas/responses/output-schema-acceptance.pbt.test.tspackages/shared/src/schemas/responses/preference.test.tspackages/shared/src/schemas/responses/preference.tspackages/shared/src/schemas/responses/user.test.tspackages/shared/src/schemas/responses/user.tspackages/shared/src/schemas/user.test.tspackages/shared/src/schemas/user.tspackages/shared/src/types/user.tsplans/F05-in-app-notifications.mdplans/F29-weekly-email-digest.md
| try { | ||
| await deps.notifyNewFollower({ followingId, followerUsername }); | ||
| } catch (err) { | ||
| logger.error({ err, followerId, followingId }, 'follow email failed'); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Follow, like, and comment emails are each sent twice. The new fan-out creators send the same email that the existing call sites already send. No call site removed its original send, so every follow, like, and comment produces two identical emails. The same duplication breaks the preference gate: the caller-side send runs before any notify* lookup, so an opted-out recipient still receives one email despite the "single flag gates both" claim in the docs. Pick one owner for each email channel, then apply it at every site.
apps/api/src/modules/notification/service.ts#L223-L227: decide email ownership here. If the creator owns the email, keep this block and the equivalent blocks at Lines 296-305 and Lines 393-402. If the caller owns it, delete all three blocks and dropnotifyNewFollower,notifyRecipeLiked, andnotifyRecipeCommentedfromdeps.apps/api/src/modules/comment/service.ts#L164-L183: remove theawait deps.notifyRecipeCommented(...)call at Lines 166-171 if the creator owns the email, and correct the gating claim in the comment at Lines 162-163.apps/api/src/modules/recipe/service.ts#L566-L596: remove theawait notifyRecipeLiked(...)call at Lines 581-586 if the creator owns the email, and correct the gating claim in the doc at Lines 568-569.apps/api/src/modules/notification/service.test.ts#L380-L701: after the ownership change, assert the email count at the call-site level so a reintroduced duplicate fails a test.
📍 Affects 4 files
apps/api/src/modules/notification/service.ts#L223-L227(this comment)apps/api/src/modules/comment/service.ts#L164-L183apps/api/src/modules/recipe/service.ts#L566-L596apps/api/src/modules/notification/service.test.ts#L380-L701
🤖 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/api/src/modules/notification/service.ts` around lines 223 - 227, Email
ownership is duplicated across creators and callers, causing duplicate sends and
bypassing preference gates. Make notification creators the sole owners: keep the
follow block in apps/api/src/modules/notification/service.ts lines 223-227 and
equivalent creator blocks at lines 296-305 and 393-402, remove caller sends and
correct gating comments in apps/api/src/modules/comment/service.ts lines 164-183
and apps/api/src/modules/recipe/service.ts lines 566-596. Update
apps/api/src/modules/notification/service.test.ts lines 380-701 to assert one
email per call site.
| > **✅ Shipped via OpenSpec change `f05-in-app-notifications` (2026-08-02).** | ||
| > | ||
| > The implemented shape is **Option C — flat top-level `notify*` schema fields + `notify_`-prefixed DB columns** (the 11-point What Changes in the OpenSpec `proposal.md` is the source of truth; the validation preface below is preserved as the historical 2026-07-13 audit context). | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the blank line inside the blockquote.
The standalone > line creates a blank line inside the blockquote. Markdownlint MD028 flags this pattern. Keep the banner and the implementation note in one blockquote without the empty quoted line.
Proposed fix
> **✅ Shipped via OpenSpec change `f05-in-app-notifications` (2026-08-02).**
->
> The implemented shape is **Option C — flat top-level `notify*` schema fields + `notify_`-prefixed DB columns** (the 11-point What Changes in the OpenSpec `proposal.md` is the source of truth; the validation preface below is preserved as the historical 2026-07-13 audit context).📝 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.
| > **✅ Shipped via OpenSpec change `f05-in-app-notifications` (2026-08-02).** | |
| > | |
| > The implemented shape is **Option C — flat top-level `notify*` schema fields + `notify_`-prefixed DB columns** (the 11-point What Changes in the OpenSpec `proposal.md` is the source of truth; the validation preface below is preserved as the historical 2026-07-13 audit context). | |
| > **✅ Shipped via OpenSpec change `f05-in-app-notifications` (2026-08-02).** | |
| > The implemented shape is **Option C — flat top-level `notify*` schema fields + `notify_`-prefixed DB columns** (the 11-point What Changes in the OpenSpec `proposal.md` is the source of truth; the validation preface below is preserved as the historical 2026-07-13 audit context). |
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 6-6: Blank line inside blockquote
(MD028, no-blanks-blockquote)
🤖 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 `@plans/F05-in-app-notifications.md` around lines 3 - 6, Remove the standalone
blank blockquote line between the shipped banner and the implementation note in
the plan content, keeping both lines in one continuous blockquote so
Markdownlint MD028 passes.
Source: Linters/SAST tools
…e schema, a11y/i18n nits Email ownership ( Inline 1+2, Outside 1): - follow/recipe/comment services no longer call notifyNewFollower / notifyRecipeLiked / notifyRecipeCommented directly — the notification creators (createFollowNotification / createLikeNotification / createCommentNotification) are the SOLE owners of both the in-app record AND the email. The previous double-send bypassed the preference gate and sent two emails per event. - Removed direct notify* imports and deps entries from the three callers; updated creator docblocks to state sole ownership. - comment/service.test.ts: replaced notifyCalls recorder with commentCalls (the creator stub); reworked the rejection-resilience test to stub createCommentNotification instead of notifyRecipeCommented. - follow/service.test.ts: refreshed the DB-integration comment block. PATCH preference schema (Inline 4): - Added UserPreferencesPatchSchema (all fields .optional(), no .default()) in schemas/user.ts. Zod v4 .partial() preserves .default(), which would still fill omitted booleans to true and overwrite stored values — the patch schema is rebuilt with .optional() to avoid that. - preference/index.ts PATCH route now validates with UserPreferencesPatchSchema; omitted fields parse to undefined and the flatData !== undefined guards skip them, so omitted preferences stay unchanged. - Added 4 unit tests for the patch schema (omitted → undefined, present, empty, invalid enum). a11y / i18n (Inline 5): - NotificationListPage filter group aria-label now uses notifications.filterLabel instead of notifications.title. - Added notifications.filterLabel to en + tr i18n and the web test mock maps. plans MD028 (Inline 6+7): - F05 + F29 plans: joined the two separated blockquotes into one contiguous blockquote via > continuation lines. stale TODOs (Nitpick 4): - NotificationListPage.test.tsx: removed the outdated 'F05 filter UI has NOT landed' TODO block and the 'T38 may revert' TODO. Skipped nitpicks (ponytail: abstraction for ~3 sites is scaffolding): - shared test helpers (makeUser / deleteUserWithBadges duplicated 2x) - extract fan-out helper from 3 creators - extract NotificationItem text interpolation helper Validation: make check (302 files), make lint (661 files), make fmt, shared 137 passed, api 316 passed, web 1289 passed.
Review fixes pushed (
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/api/src/modules/comment/service.test.ts (1)
737-738: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the complete
createCommentNotificationpayload.The contract in
apps/api/src/modules/notification/service.ts, Lines 332-425, consumescommenterId,commenterUsername,recipeAuthorId,recipeId,recipeSlug,recipeTitle, andcommentId. These assertions check onlyrecipeAuthorId.A wrong actor, recipe, slug, title, or comment ID can pass these tests while creating an incorrect notification record or email. Assert the full captured object in both cases.
Also applies to: 820-821
🤖 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/api/src/modules/comment/service.test.ts` around lines 737 - 738, Update both notification assertions in the comment service tests to verify the complete captured createCommentNotification payload, including commenterId, commenterUsername, recipeAuthorId, recipeId, recipeSlug, recipeTitle, and commentId. Replace the single recipeAuthorId check in each case with an object-level assertion matching the expected values for that test scenario.
🤖 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.
Nitpick comments:
In `@apps/api/src/modules/comment/service.test.ts`:
- Around line 737-738: Update both notification assertions in the comment
service tests to verify the complete captured createCommentNotification payload,
including commenterId, commenterUsername, recipeAuthorId, recipeId, recipeSlug,
recipeTitle, and commentId. Replace the single recipeAuthorId check in each case
with an object-level assertion matching the expected values for that test
scenario.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d8afbf37-ed49-46e5-8884-d0d17233b597
📒 Files selected for processing (19)
AGENTS.mdapps/api/src/modules/comment/service.test.tsapps/api/src/modules/comment/service.tsapps/api/src/modules/follow/service.test.tsapps/api/src/modules/follow/service.tsapps/api/src/modules/notification/service.tsapps/api/src/modules/preference/index.tsapps/api/src/modules/recipe/service.tsapps/web/src/pages/notifications/NotificationListPage.test.tsxapps/web/src/pages/notifications/NotificationListPage.tsxopenspec/changes/f05-in-app-notifications/tasks.mdpackages/db/drizzle/meta/0012_snapshot.jsonpackages/shared/src/i18n/en.jsonpackages/shared/src/i18n/tr.jsonpackages/shared/src/schemas/index.tspackages/shared/src/schemas/user.test.tspackages/shared/src/schemas/user.tsplans/F05-in-app-notifications.mdplans/F29-weekly-email-digest.md
🚧 Files skipped from review as they are similar to previous changes (11)
- plans/F29-weekly-email-digest.md
- apps/web/src/pages/notifications/NotificationListPage.tsx
- packages/shared/src/i18n/tr.json
- packages/shared/src/i18n/en.json
- apps/web/src/pages/notifications/NotificationListPage.test.tsx
- apps/api/src/modules/follow/service.test.ts
- openspec/changes/f05-in-app-notifications/tasks.md
- plans/F05-in-app-notifications.md
- apps/api/src/modules/comment/service.ts
- apps/api/src/modules/notification/service.ts
- apps/api/src/modules/recipe/service.ts
…alls Replace the two single-field recipeAuthorId checks (lines ~738, ~821) with object-level toEqual assertions matching the complete captured createCommentNotification payload (commenterId, commenterUsername, recipeAuthorId, recipeId, recipeSlug, recipeTitle, commentId), mirroring the full-payload pattern already used in the 'invoked when commenter != recipe author' test below.
1 participant
F05 In-App Notification Center
Summary
Implements the OpenSpec change
f05-in-app-notificationsto extend BrewForm's notification fan-out beyond mentions (F04) to follow / like / comment types, and flattens the preference namespace to be channel-agnostic — theemailNotificationsnest is gone; every flag now gates BOTH the in-app notification record AND the email.Diff size: 36 files changed, +1578 / -242, 2 new files (migration + snapshot).
What Changes
1. Schema + migration
notificationTypeEnumextended withfollow/like/comment(wasmention-only).badge/systemdeferred (no call sites — YAGNI).user_preferencesnotification columns renamed with thenotify_prefix via Drizzle data-preservingRENAME COLUMN(no data loss):new_follower→notify_new_followerrecipe_liked→notify_recipe_likedrecipe_commented→notify_recipe_commentedfollowed_user_posted→notify_followed_user_postedmentioned_in_comment→notify_mentioned_in_commentpackages/db/drizzle/0012_f05_notifications.sql(hand-written because Drizzle Kit 0.31's interactive rename prompt requires a TTY —--customflag used as the non-interactive pivot; the snapshot JSON was manually aligned to keep futuredb-generateruns clean).2. Shared schema flatten
UserPreferencesSchema(input),SelfPreferencesSchema(GET/me),UserPreferencesOutputSchema(GET/preferences), andUserPreferencesinterface all flattened to 5 top-levelnotify*boolean fields — request and response now identical (F04 asymmetry gone).notifyMentionedInCommentwas missing from/meandUserPreferencesinterface) is fixed structurally by the flatten — adding a flat field is one line, whereas the nest required re-listing every field at every projection site.en.json+tr.json(parity required): addednotifications.follow/like/comment/all/unread; removed obsoletesettings.emailNotificationskey.3. API fan-out
apps/api/src/modules/notification/service.ts:createFollowNotification,createLikeNotification,createCommentNotification. Each mirrors the F04createMentionNotificationspattern (load recipient prefs viafindNotifyTarget→ drop self-action → gate on the matchingnotify*preference flag → insert notification record → send email via the existingnotify*helper → catch-log-continue per failure).findNotifyTarget(userId)added toapps/api/src/modules/notification/model.ts(single-recipient loader, parallel tofindMentionTargets).findMentionTargetsprojection renamedmentionedInComment → notifyMentionedInComment.depsproxy extended to includenotifyNewFollower,notifyRecipeLiked,notifyRecipeCommentedso unit tests can stub all three new email side-effects alongside the existingnotifyMentioned..catch(err => logger.error(...))— caller never blocks on notification):follow/service.ts:followUser— afternotifyNewFollower(record for followed user)recipe/service.ts:toggleLike— inside theresult.liked && recipe.authorId !== userIdIIFE (record for recipe author)comment/service.ts:runCommentNotificationSideEffects— inside therecipe.authorId !== userIdblock colocated withnotifyRecipeCommented(record for recipe author — distinct from the existingcreateMentionNotificationswhich targets@mentionedusers on the same comment)preference/index.ts:PATCH /preferencesflatten shrinks fromif (body.emailNotifications !== undefined) { flatData.X = body.emailNotifications.X; ... }to 5 direct per-field copies.user/model.ts:findByIdprojection flattens —preferencesobject now exposes 5notify*fields directly (no nestedemailNotifications).utils/notify/index.ts— 5 helper gate lines renamed fromrecipient.prefs.X === falsetorecipient.prefs.notifyX === false.4. Web
SettingsPage.tsx: 5 toggles +toUserPreferences()adapter +savePreferences()PATCH payload all flattened to read/writeprefs.notifyXdirectly; section header switched fromsettings.emailNotificationsto the existingsettings.notificationskey.NotificationItem.tsx: per-typeswitchonnotification.type(mention/follow/like/commentwith fallback tomentionGenericfor forward-compat with future enum additions); per-type link target (follow →/u/{actor}, comment →/recipes/{slug}#{commentId}, mention/like →/recipes/{slug}); metadata interface widened to include optionalfollowerUsername.NotificationListPage.tsx:All/Unreadfilter toggle (URL-driven via?unreadOnly=true, loader re-fetches on filter change). Filter pills labeled with the newnotifications.all/notifications.unreadi18n keys.5. Tests
All new code is exercised:
UserPreferencesSchemaflat shape + default-true + legacy-strip behavior;SelfUserOutputSchemawithnotifyMentionedInComment(F04 latent bug regression guard);UserPreferencesOutputSchema5-field round-trip + missing-notifyMentionedInCommentrejection; PBT arb regenerated with the flat shape.notification/service.test.ts: 3 newdescribesuites (one per new creator) × 7itblocks each (opt-out skip, missing-prefs-null opted-in, self-skip, target-not-found, happy-path record+email shapes, email-throws logged-and-continue, model.create-throws logged-and-continue with email still firing via independent try/catch). ExistingcreateMentionNotificationssuite updated tonotifyMentionedInComment.notification/model.test.ts:findNotifyTargetsuite (5 DB-backed tests: prefs joined, prefs-null row, missing user, soft-deleted, banned). ExistingfindMentionTargetstest'smentionedInCommentreferences renamed tonotify_mentioned_in_comment.findById5-flag assertion incl.notifyMentionedInComment(F04 latent bug regression guard);preference/model.test.tsaddsnotifyMentionedInCommentround-trip (F05 column rename regression guard).createCommentNotificationdeps slot so F04 mention tests don't leak.openapi.coverage.test.ts: PATCH/preferencesOpenAPI metadata still passes (no orphan tags, request body flat shape auto-generated viajsonRequestBody(UserPreferencesSchema); all 19 in-scope routes documented).NotificationItem.test.tsxaddsfollow/like/comment/ unknown-type rendering tests + link target assertions;NotificationListPage.test.tsxadds All/Unread filter tests;SettingsPage.test.tsx5-toggle flat round-trip.Migration notes
ALTER TABLE ... RENAME COLUMN(data-preserving, no drop+create). Existinguser_preferencesrows carry over theirtrue/falsevalues unchanged.emailNotificationsrequest body alias (Zod default strips unknown keys, so legacy clients sending the nested shape silently persistnotify* = truedefaults — visibility behavior change documented in the spec).Non-goals
badge/systemnotification types (deferred YAGNI — no fan-out call sites today).notifyMentionedInComment).notifyFollowersOfNewRecipein-app record (no matching enum type — email-only remains).Verification
make fmtmake check(api + web + db + shared)make lintmake test-sharedmake test-apimake test-web(Vitest)/api/v1/openapi.jsonintrospection)OpenSpec change
openspec/changes/f05-in-app-notifications/proposal.md,design.md,tasks.md,specs/notifications/spec.md,specs/api-type-safety/spec.md— allisComplete: true.plans/F05-in-app-notifications.md(prepended with ship banner pointing at the OpenSpec change as source of truth),plans/F29-weekly-email-digest.md(prepended with forward-compat note so future implementers don't blindly follow the pre-F05 nested-shape suggestion).Caveats
db-generaterequires a TTY for column rename disambiguation prompts; this non-interactive environment errors out ("Interactive prompts require a TTY terminal"). Pivot used:drizzle-kit generate --custom --name=f05_notificationscreates an empty migration shell non-interactively, then the SQL is hand-written per the design's R17 template (5RENAME COLUMN+ 3ALTER TYPE ADD VALUE). The snapshot JSON was manually updated to reflect post-migration column names so futuredb-generateruns won't re-prompt for the same renames.admin/model.test.ts:getTopUsershad accumulateddiff-*user pollution from prior test runs (per AGENTS.md: "seed cannot remove stray rows left by API tests"). The test DB was dropped and re-provisioned before final verification —getTopUserspasses in isolation.Summary by CodeRabbit
New Features
Bug Fixes