Repository navigation
feat(notifications): @mention notifications for channels and tasks - #1298
Conversation
… and tasks When a user is @mentioned in a channel message (top-level or thread reply) or in a task description (create or update), createMentionNotification is called for each mentioned user who passes a canUserViewPage gate. Task updates diff old vs new mentions so previously-notified users are not re-notified. All notification calls are wrapped in try/catch so failures never fail the API response after DB commit. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ry, logging, clean tests - Switch notification Promise.all to per-call .catch() so each mention is attempted independently; one failure no longer blocks the rest - Add loggers.api.error() to task routes (spec required error logging; silent catch was non-compliant) - Remove redundant description type/length guard in tasks/route.ts — extractMentionedUserIds already handles empty/null safely - Remove WHAT-comments on mention blocks; remove explicit :string annotation from mentionTargets.map - Fix 3 vacuous task-update mention tests that passed for wrong reasons due to mockResolvedValue override: use explicit mockResolvedValueOnce chains so existingTask.description is distinct from the new value - Fix loggers.api mock in taskId tests to expose .error() directly - Assert createMentionNotification not called when alsoSendToParent is set (broad fan-out path suppresses targeted mention notification) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR adds mention-notification side effects to four API endpoints: channel message posts (top-level and thread replies), task creation, and task description updates. Each endpoint extracts ChangesMention Notifications across Channel Messages and Tasks APIs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
https://github.com/2witstudios/PageSpace/blob/1436ff547a7cbcfa5933b41347bbd50c099ea0c1/apps/web/src/app/api/channels/[pageId]/messages/route.ts#L451-L452
The mention-notification path for thread replies reuses candidateTargets that explicitly excludes followerSet, so any user who already follows the thread is filtered out before createMentionNotification runs. In practice, @-mentioning a thread follower only emits a thread_updated inbox event and never creates a durable MENTION notification (no notification center entry/email/push), which is inconsistent with top-level channel and task mention behavior.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
TypeScript infers transactionTaskResult as { id: string; title: string }[]
from the default value; our new mockNewTask objects were missing title.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/app/api/pages/[pageId]/tasks/__tests__/route.test.ts (1)
158-195:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
vi.resetAllMocks()clearsmockCreateMentionNotification's implementation, causing the happy-path notification test to silently exercise the error path.
vi.resetAllMocks()(line 159) resets allvi.fn()implementations, including the module-scopemockCreateMentionNotification. After this call,mockCreateMentionNotification()returnsundefinedinstead ofPromise.resolve(undefined). In the route, the code calls:createMentionNotification(e.id, pageId, userId).catch(...)…which becomes
undefined.catch(...)→TypeError, caught by the outertry/catch, logged, and silently discarded. The assertion in the test "fires createMentionNotification for@mentionedusers…" (line 752) still passes because the mock was called before theTypeError, but the actual notification send path is never verified.The channel tests correctly guard against this by calling
mockCreateMentionNotification.mockResolvedValue(undefined)in eachbeforeEachaftervi.clearAllMocks().🛠 Proposed fix
beforeEach(() => { vi.resetAllMocks(); + mockCreateMentionNotification.mockResolvedValue(undefined); // Reset default mock for taskStatusConfigs.findMany vi.mocked(db.query.taskStatusConfigs.findMany).mockResolvedValue([] as never);🤖 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/app/api/pages/`[pageId]/tasks/__tests__/route.test.ts around lines 158 - 195, The test's vi.resetAllMocks() clears the module-scoped mockCreateMentionNotification implementation causing createMentionNotification(...) to become undefined and throw; after the reset, re-establish the mock's resolved-value behavior (e.g. call mockCreateMentionNotification.mockResolvedValue(undefined) or equivalent) in the beforeEach so createMentionNotification remains a Promise-returning mock during tests; locate and update the beforeEach block that currently calls vi.resetAllMocks() to add a restoring call for mockCreateMentionNotification (and any similar module-scoped mocks) so the happy-path notification test exercises the success path rather than hitting a TypeError.
🤖 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/web/src/app/api/pages/`[pageId]/tasks/route.ts:
- Around line 509-530: The mention notification is using the task list's pageId
instead of the individual task page id; update the POST route to pass
result.page.id to createMentionNotification (replace
createMentionNotification(e.id, pageId, userId) with
createMentionNotification(e.id, result.page.id, userId)) and do the same in the
PATCH route (use the task-specific page id variable analogous to
result.page.id), and also update the permission checks that call canUserViewPage
to validate against the task's page id rather than the task list pageId; keep
existing error handling with loggers.api.error and preserve use of
extractMentionedUserIds and the candidates filtering by userId.
---
Outside diff comments:
In `@apps/web/src/app/api/pages/`[pageId]/tasks/__tests__/route.test.ts:
- Around line 158-195: The test's vi.resetAllMocks() clears the module-scoped
mockCreateMentionNotification implementation causing
createMentionNotification(...) to become undefined and throw; after the reset,
re-establish the mock's resolved-value behavior (e.g. call
mockCreateMentionNotification.mockResolvedValue(undefined) or equivalent) in the
beforeEach so createMentionNotification remains a Promise-returning mock during
tests; locate and update the beforeEach block that currently calls
vi.resetAllMocks() to add a restoring call for mockCreateMentionNotification
(and any similar module-scoped mocks) so the happy-path notification test
exercises the success path rather than hitting a TypeError.
🪄 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
Run ID: 5606feff-6e19-4c4e-8511-e2077ac43a12
📒 Files selected for processing (6)
apps/web/src/app/api/channels/[pageId]/messages/__tests__/route.test.tsapps/web/src/app/api/channels/[pageId]/messages/route.tsapps/web/src/app/api/pages/[pageId]/tasks/[taskId]/__tests__/route.test.tsapps/web/src/app/api/pages/[pageId]/tasks/[taskId]/route.tsapps/web/src/app/api/pages/[pageId]/tasks/__tests__/route.test.tsapps/web/src/app/api/pages/[pageId]/tasks/route.ts
| if (description) { | ||
| try { | ||
| const mentionedIds = extractMentionedUserIds(description); | ||
| const candidates = mentionedIds.filter((id) => id !== userId); | ||
| if (candidates.length > 0) { | ||
| const viewChecks = await Promise.all( | ||
| candidates.map(async (id) => ({ id, canView: await canUserViewPage(id, pageId) })) | ||
| ); | ||
| await Promise.all( | ||
| viewChecks | ||
| .filter((e) => e.canView) | ||
| .map((e) => | ||
| createMentionNotification(e.id, pageId, userId).catch((err) => | ||
| loggers.api.error('Failed to send mention notification', err as Error) | ||
| ) | ||
| ) | ||
| ); | ||
| } | ||
| } catch (err) { | ||
| loggers.api.error('Failed to resolve mention targets', err as Error); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the createMentionNotification implementation to check what the second param (pageId) represents
rg -nP --type=ts -C 10 'export.*(function|const|async function)\s+createMentionNotification'
# Also check call sites for usage context
rg -nP --type=ts 'createMentionNotification\(' -A2Repository: 2witstudios/PageSpace
Length of output: 5560
🏁 Script executed:
#!/bin/bash
# Read the full createMentionNotification implementation
sed -n '436,500p' packages/lib/src/notifications/notifications.tsRepository: 2witstudios/PageSpace
Length of output: 1666
🏁 Script executed:
#!/bin/bash
# Read lines around 509-530 in the tasks route file to see context
sed -n '490,540p' apps/web/src/app/api/pages/[pageId]/tasks/route.tsRepository: 2witstudios/PageSpace
Length of output: 1542
🏁 Script executed:
#!/bin/bash
# Find the PATCH route for tasks and look for mention notification code
sed -n '390,410p' apps/web/src/app/api/pages/[pageId]/tasks/[taskId]/route.tsRepository: 2witstudios/PageSpace
Length of output: 955
Pass the individual task's page ID to mention notifications, not the task list page.
In both the POST and PATCH routes, createMentionNotification(e.id, pageId, userId) uses pageId (the task list page) instead of the individual task's page ID. This causes the notification message to display the task list title instead of the task title, and users will be navigated to the task list rather than the specific task they were mentioned in.
Use result.page.id in the POST route (line 521) and the equivalent task page ID in the PATCH route (line 402). Also verify that permission checks validate access to the actual task page being mentioned in, not just the task list.
🤖 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/app/api/pages/`[pageId]/tasks/route.ts around lines 509 - 530,
The mention notification is using the task list's pageId instead of the
individual task page id; update the POST route to pass result.page.id to
createMentionNotification (replace createMentionNotification(e.id, pageId,
userId) with createMentionNotification(e.id, result.page.id, userId)) and do the
same in the PATCH route (use the task-specific page id variable analogous to
result.page.id), and also update the permission checks that call canUserViewPage
to validate against the task's page id rather than the task list pageId; keep
existing error handling with loggers.api.error and preserve use of
extractMentionedUserIds and the candidates filtering by userId.
Followers were excluded from `mentionTargets` (correct — to avoid double-counting `channel_updated` unread), but this also suppressed the durable MENTION notification for them. Split the logic: only non-followers get `channel_updated`; all @mentioned non-sender users (followers + non-followers who pass view check) get `createMentionNotification`. Adds a regression test for the follower-mention case. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
vi.resetAllMocks() clears the implementation, making .catch() call on undefined throw a TypeError that is silently caught. Re-establish the resolved-value implementation in beforeEach so the notification success path is actually exercised. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
createMentionNotification uses the pageId to look up the page title and build navigation links. Passing the task list pageId caused notifications to display the list title and navigate to the list instead of the task. - POST tasks: use result.page.id (the created task's own page) - PATCH task: use existingTask.pageId (the task's own page) - Permission gate: canUserViewPage now checks the task page, not the list Update test assertions and descriptions to reflect the corrected page IDs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Addressed all review findings — summary of fixes pushed in three commits after the initial implementation: Codex (follower @mentions missing MENTION notification): Fixed in CodeRabbit ( CodeRabbit (task mention notifications used task list |
…1298) * feat(notifications): fire @mention notifications for channel messages and tasks When a user is @mentioned in a channel message (top-level or thread reply) or in a task description (create or update), createMentionNotification is called for each mentioned user who passes a canUserViewPage gate. Task updates diff old vs new mentions so previously-notified users are not re-notified. All notification calls are wrapped in try/catch so failures never fail the API response after DB commit. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * refactor(notifications): address review findings — best-effort delivery, logging, clean tests - Switch notification Promise.all to per-call .catch() so each mention is attempted independently; one failure no longer blocks the rest - Add loggers.api.error() to task routes (spec required error logging; silent catch was non-compliant) - Remove redundant description type/length guard in tasks/route.ts — extractMentionedUserIds already handles empty/null safely - Remove WHAT-comments on mention blocks; remove explicit :string annotation from mentionTargets.map - Fix 3 vacuous task-update mention tests that passed for wrong reasons due to mockResolvedValue override: use explicit mockResolvedValueOnce chains so existingTask.description is distinct from the new value - Fix loggers.api mock in taskId tests to expose .error() directly - Assert createMentionNotification not called when alsoSendToParent is set (broad fan-out path suppresses targeted mention notification) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(test): add missing title field in mockNewTask TypeScript infers transactionTaskResult as { id: string; title: string }[] from the default value; our new mockNewTask objects were missing title. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(notifications): fire MENTION for @mentioned thread followers Followers were excluded from `mentionTargets` (correct — to avoid double-counting `channel_updated` unread), but this also suppressed the durable MENTION notification for them. Split the logic: only non-followers get `channel_updated`; all @mentioned non-sender users (followers + non-followers who pass view check) get `createMentionNotification`. Adds a regression test for the follower-mention case. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test(tasks): restore mockCreateMentionNotification after resetAllMocks vi.resetAllMocks() clears the implementation, making .catch() call on undefined throw a TypeError that is silently caught. Re-establish the resolved-value implementation in beforeEach so the notification success path is actually exercised. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(notifications): use task page ID in mention notifications createMentionNotification uses the pageId to look up the page title and build navigation links. Passing the task list pageId caused notifications to display the list title and navigate to the list instead of the task. - POST tasks: use result.page.id (the created task's own page) - PATCH task: use existingTask.pageId (the task's own page) - Permission gate: canUserViewPage now checks the task page, not the list Update test assertions and descriptions to reflect the corrected page IDs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
1 participant
Summary
createMentionNotificationfor each @mentioned user who passes acanUserViewPagecheck.catch()for best-effort delivery (one failure never blocks others)loggers.api.error()added to all task-route catch blocks for observabilitycanUserViewPagealways checked before notifying — prevents crafted mentions from leaking channel/page existence to unauthorized usersTest plan
createMentionNotificationfires for viewable @mentioned non-senders; skipped for sender self-mentions and users who can't view the channel; errors don't fail the 201 responsementionTargets; suppressed whenalsoSendToParentis set (broad fan-out covers everyone); errors don't fail the 201mockResolvedValueOncechains —canUserViewPagegate and error isolation are actually exercised (not vacuous)🤖 Generated with Claude Code
Summary by CodeRabbit