UX-25: Notification type differentiation, grouping, and batch actions

[Chris0Jeky]

Summary

Implements #625 — notification list UX improvements with type differentiation, smart grouping, time-based sections, and batch actions.

Backend

• Extended NotificationType enum with BoardChange and System variants
• Added POST /api/notifications/mark-all-read endpoint with optional boardId filter
• Wired through INotificationService, NotificationService, INotificationRepository, NotificationRepository, and NoOpNotificationService

Frontend

• Type-specific left-border colors: amber (proposal), blue (mention), green (board change), purple (assignment), gray (system) via Tailwind classes
• Type badge labels alongside colors so color is not the sole accessibility differentiator
• Time-based section headers: "Today", "Yesterday", "This week", "Older"
• Smart grouping: consecutive same-type notifications collapse into expandable summary cards (e.g., "3 mention notifications")
• "Mark all read" batch button in header, visible only when unread notifications exist
• New useNotificationGrouping composable with pure functions for type normalization, border/badge classes, time grouping, and smart notification collapsing
• Frontend API and store wired for mark-all-read endpoint

Test plan

• 23 vitest tests for useNotificationGrouping composable (type normalization, labels, border/badge classes, time grouping, smart collapsing)
• 10 view tests for NotificationInboxView (Mark all read visibility, click behavior, time headers, collapsed groups, existing navigation tests preserved)
• 2 store tests for markAllRead action (success and error paths)
• 4 backend tests for MarkAllAsReadAsync (success, empty list, forbidden board access, validation error)
• All 1475 frontend tests pass
• All 1953 backend tests pass
• Frontend builds clean (npm run build)
• Backend builds clean (dotnet build -c Release)

Closes #625

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

[Chris0Jeky]

Self-Review Findings

Accessibility

• Color is not sole differentiator: Type badges with text labels ("Mention", "Proposal", "Board Change", etc.) accompany the colored left borders. Screen readers and color-blind users can distinguish notification types via the badge text.
• Dark mode variants included for all badge classes (dark:bg-*, dark:text-*).

Performance

• groupNotifications is O(n) — single pass through the sorted list. Safe for the 200-item fetch limit.
• Grouping runs in a computed, so it only recalculates when the notifications list changes.

API Backward Compatibility

• New enum values BoardChange (3) and System (4) are additive — existing numeric values (0, 1, 2) are unchanged.
• The mark-all-read endpoint is a new route, no existing routes modified.
• The MarkAllReadResponse type is new, no existing types changed.

TypeScript Type Safety

• NotificationTypeName union type covers all 5 enum variants.
• normalizeType returns the exhaustive union with a System fallback for unknown values.
• All switch statements in the composable cover every case and return (no any types).

Tailwind Classes

• Standard Tailwind color classes (amber-500, blue-500, green-500, purple-500, gray-400) — all in the default palette.
• border-l-4 is standard Tailwind utility for left border width.

Potential Improvements (not blocking)

• The GetUnreadByUserIdAsync repository method loads all unread notifications into memory before marking them. For users with very large unread counts (500+), a SQL-level UPDATE ... WHERE would be more efficient. However, the current 500-item max limit on the service makes this acceptable for now.
• Collapsed group expand/collapse state is stored in a local Set<string> — it resets on navigation. This is intentional (stateless between visits).

No blocking issues found.

[gemini-code-assist]

Code Review

This pull request introduces a 'Mark all as read' feature for notifications, including backend API support, repository methods, and a revamped frontend inbox with time-based grouping and smart collapsing of consecutive notifications. The review identified a critical logic bug in the frontend store where marking all as read ignores the board filter, potentially marking unrelated notifications as read. Additionally, there are concerns regarding performance and memory pressure in the backend implementation, as it currently fetches and iterates over all unread notifications in memory rather than using a bulk update. Improving the user experience by displaying board names instead of raw GUIDs was also suggested.

[gemini-code-assist]

There is a logic bug here: the markAllRead action updates the local state for all notifications in the store, even if a boardId filter was provided to the API call. This will cause the UI to incorrectly show all notifications as read when the user only intended to mark those for a specific board. The mapping logic should be filtered by boardId.

Suggested change

	notifications.value = notifications.value.map((item) => ({
	...item,
	isRead: true,
	readAt: item.readAt ?? new Date().toISOString(),
	}))
	notifications.value = notifications.value.map((item) =>
	(!boardId || item.boardId === boardId) && !item.isRead
	? { ...item, isRead: true, readAt: new Date().toISOString() }
	: item
	)

[gemini-code-assist]

The current implementation of MarkAllAsReadAsync fetches all unread notifications into memory and iterates over them to update each one individually. For users with a large volume of unread notifications, this can lead to significant memory pressure and performance issues. Consider implementing a bulk update operation in the repository (e.g., using EF Core's ExecuteUpdateAsync) to perform this change directly in the database.

[gemini-code-assist]

Fetching all unread notifications without a limit can be dangerous for performance if a user has accumulated a very large number of notifications. Since this method is primarily used for batch updates, consider if a bulk update approach would be more appropriate, or at least apply a reasonable maximum limit to the query.

[gemini-code-assist]

Displaying the raw activeBoardId (which is a GUID) to the user is not a good user experience. It would be much better to display the human-readable name of the board. If the board name is not available in the current context, consider fetching it or passing it as part of the navigation state.

[Chris0Jeky]

Adversarial Code Review -- PR #646

Reviewed the full diff (16 files, ~950 additions) across backend and frontend. Below are findings rated by confidence and severity.

---

Critical (90-100)

1. Store markAllRead blindly marks ALL local notifications as read, ignoring boardId filter (Confidence: 95)

File: frontend/taskdeck-web/src/store/notificationStore.ts (new markAllRead function)

The backend MarkAllAsReadAsync correctly respects the optional boardId filter -- it only marks notifications belonging to that board. However, the frontend store unconditionally sets isRead: true on every notification in local state:

notifications.value = notifications.value.map((item) => ({
  ...item,
  isRead: true,
  readAt: item.readAt ?? new Date().toISOString(),
}))

When the user is viewing a board-filtered notification list and clicks "Mark all read", this marks notifications from other boards as read in the UI even though the backend did not touch them. On next fetch the state will correct itself, but the intermediate optimistic state is wrong -- and any UI logic depending on unreadCount will show 0 incorrectly.

Fix: Filter by boardId when applying the local update:

notifications.value = notifications.value.map((item) => {
  if (boardId && item.boardId !== boardId) return item
  return { ...item, isRead: true, readAt: item.readAt ?? new Date().toISOString() }
})

---

2. CI gate failure: src/api/** branch coverage (47.82%) below 49% threshold (Confidence: 98)

File: frontend/taskdeck-web/src/api/notificationsApi.ts

The markAllRead method adds a ternary branch (boardId ? ... : '') that is not covered by tests. The Frontend Unit CI job fails on both ubuntu and windows with:

ERROR: Coverage for branches (47.82%) does not meet "src/api/**" threshold (49%)

Per CLAUDE.md: "Behavior changes ship with tests." The notificationsApi.markAllRead function needs at least two test cases (with and without boardId) to cover both branches and restore CI to green. This PR cannot merge with CI red (per project memory: "Never merge failing CI").

---

Important (80-89)

3. _authorizationService null-check silently skips board access validation (Confidence: 85)

File: backend/src/Taskdeck.Application/Services/NotificationService.cs (line ~89 in diff)

if (boardId.HasValue && _authorizationService is not null)

If _authorizationService is null (injected as optional), the authorization check is silently bypassed -- any user can mark-all-read for any boardId. This follows the existing pattern in GetNotificationsAsync, but it is worth noting that this means in any deployment where IAuthorizationService is not registered, there is no board-level access control on bulk mark-read.

This is not a new pattern (it matches line 42 of the existing service), so it is not a regression. However, the new endpoint is more destructive (bulk mutation vs. read-only query), so consider adding a guard:

if (boardId.HasValue && _authorizationService is null)
    return Result.Failure<int>(ErrorCodes.Forbidden, "Authorization service unavailable");

4. groupsForHeader is O(n) per header, making total rendering O(n * h) (Confidence: 82)

File: frontend/taskdeck-web/src/views/NotificationInboxView.vue (the groupsForHeader function)

function groupsForHeader(header: TimeGroup): NotificationGroup[] {
  return grouped.value.filter((g) => g.timeHeader === header)
}

This filters the entire grouped array once per time header in the template. With 4 headers and a 200-item limit this is fine in practice (at most ~800 comparisons), but it runs inside the render loop on every re-render. A computed Map<TimeGroup, NotificationGroup[]> would be O(n) total and avoid redundant passes:

const groupsByHeader = computed(() => {
  const map = new Map<TimeGroup, NotificationGroup[]>()
  for (const g of grouped.value) {
    const list = map.get(g.timeHeader) ?? []
    list.push(g)
    map.set(g.timeHeader, list)
  }
  return map
})

Not blocking, but worth refactoring for cleanliness.

5. New BoardChange and System types hardcode true/false for notification preferences without user controls (Confidence: 80)

File: backend/src/Taskdeck.Application/Services/NotificationService.cs (lines ~125-136 in diff)

NotificationType.BoardChange => true,   // immediate always on
NotificationType.System => true,         // immediate always on
// ...
NotificationType.BoardChange => false,   // digest always off
NotificationType.System => false,        // digest always off

The NotificationPreference entity has no columns for these new types, so users cannot toggle BoardChange/System notification delivery. The hardcoded true for immediate means these notifications are always delivered with no opt-out. This is fine as a v1 default, but the PR description doesn't mention this intentional limitation. Consider adding a code comment explaining that preference columns for these types are deferred.

6. timeGroup uses local timezone via new Date() constructor -- timezone-sensitive edge cases (Confidence: 80)

File: frontend/taskdeck-web/src/composables/useNotificationGrouping.ts (line ~47)

const todayStart = new Date(now.getFullYear(), now.getMonth(), now.getDate())

This computes "today" in the browser's local timezone, while createdAt timestamps from the backend are UTC ISO strings. new Date(createdAt) parses to a UTC-based Date object. A notification created at 2026-03-31T01:00:00Z would be classified as "Yesterday" for a user in UTC+5 (where local time is already March 31 06:00) but as "Today" for a UTC-0 user.

The tests pass because they use fixed UTC timestamps and a fixed now. This may produce surprising grouping for users in negative UTC offsets (e.g., US Pacific). Not blocking, but documenting the behavior or normalizing both dates to UTC would prevent future bug reports.

---

Summary

#	Severity	Description	Blocking?
1	Critical	Store markAllRead ignores boardId filter in local state	Yes
2	Critical	CI red -- src/api/** branch coverage below threshold	Yes
3	Important	Authorization silently skipped when service is null	No (matches existing pattern)
4	Important	groupsForHeader O(n*h) in render loop	No
5	Important	No user preference controls for new notification types	No (note it)
6	Important	Timezone-sensitive time grouping	No (document it)

Verdict: Do not merge. Issues #1 (boardId-aware optimistic update) and #2 (coverage threshold) must be fixed first. The remaining items can be addressed as follow-ups but should at minimum have tracking comments or TODOs.