Feature: notifications #833
Conversation
WalkthroughThis update introduces a comprehensive notifications system across the web application. It adds a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DashboardUI
participant API
participant DB
participant NotificationLib
User->>DashboardUI: Clicks notification bell
DashboardUI->>API: Fetch notifications (GET /api/notifications)
API->>DB: Query notifications for user/org
DB-->>API: Return notifications
API-->>DashboardUI: Return notifications data
DashboardUI->>User: Display notifications dropdown
Note over DashboardUI,API: User marks all as read
DashboardUI->>API: Call markAllAsRead action
API->>DB: Update notifications (set readAt)
DB-->>API: Update success
API-->>DashboardUI: Confirm update
DashboardUI->>User: Update UI, remove unread indicator
Note over User,DashboardUI: User changes notification preferences
DashboardUI->>API: Call updatePreferences action
API->>DB: Update user preferences JSON
DB-->>API: Update success
API-->>DashboardUI: Confirm update
DashboardUI->>User: Show updated preferences
Note over API,NotificationLib: New comment triggers notification
API->>NotificationLib: createNotification({ type, data, ... })
NotificationLib->>DB: Check for duplicates, user prefs
alt Not duplicate and not paused
NotificationLib->>DB: Insert notification
NotificationLib->>DashboardUI: Revalidate cache
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (9)
apps/web/actions/notifications/mark-all-as-read.ts (1)
21-21: Consider using structured logging instead of console.log.For production environments, consider using a structured logging solution instead of console.log:
- console.log(error); + console.error("Failed to mark notifications as read:", error);Or better yet, integrate with your application's logging infrastructure if available.
apps/web/app/dashboard/_components/Notifications/mockData.ts (1)
3-49: Consider privacy implications of external URLs and improve data realism.Two concerns with the mock data:
External avatar URLs: Using Pexels URLs could raise privacy concerns as they make external requests. Consider using placeholder URLs or local assets for mock data.
All notifications marked as unread: Setting
hasUnread: truefor all mock notifications isn't realistic. Consider mixing read/unread states for better testing coverage.Apply this diff to improve the mock data:
- avatar: "https://images.pexels.com/photos/771742/pexels-photo-771742.jpeg?auto=compress&cs=tinysrgb&w=1260&h=750&dpr=2", - hasUnread: true + avatar: "/placeholder-avatar-1.jpg", + hasUnread: true }, content: "shared a new recording", time: "3 mins ago" }, { id: "2", type: NotificationType.COMMENT, user: { name: "James", - avatar: "https://images.pexels.com/photos/1040880/pexels-photo-1040880.jpeg?auto=compress&cs=tinysrgb&w=1260&h=750&dpr=2", - hasUnread: true + avatar: "/placeholder-avatar-2.jpg", + hasUnread: false }, content: "commented on your video", time: "12 mins ago", additionalText: "This is looking awesome, goodjob and i'm looking forward to seeing more." }, { id: "3", type: NotificationType.VIEW, user: { name: "Jasmin", avatar: "", - hasUnread: true + hasUnread: false }, content: "viewed your video", time: "20 mins ago" }, { id: "4", type: NotificationType.REACTION, user: { name: "Jasmin", avatar: "", hasUnread: trueapps/web/app/api/notifications/route.ts (1)
90-90: Verify type safety of count conversion.The
Number(count)conversion assumes the count is always a valid number. Consider adding validation to handle edge cases.- ] = Number(count); + ] = typeof count === 'number' ? count : parseInt(String(count), 10) || 0;apps/web/app/dashboard/_components/Notifications/index.tsx (1)
52-72: Consider scoping keyboard event listener.The global keyboard event listener might interfere with other components that also handle arrow keys. Consider adding a condition to only handle events when the component is focused or visible.
const handleKeyDown = (e: KeyboardEvent) => { - e.preventDefault(); + // Only handle if the notifications panel has focus + if (!scrollRef.current?.contains(document.activeElement)) return; + + e.preventDefault(); if (e.key === "ArrowUp") {apps/web/app/dashboard/_components/Notifications/FilterTabs.tsx (1)
20-36: Consider accessibility implications of custom scroll behaviorThe custom wheel event handler might interfere with accessibility tools. Also, users might not know they need to hold Ctrl for vertical scrolling.
Consider adding a visual indicator or using native horizontal scroll:
const handleWheel = (e: WheelEvent) => { + // Only hijack vertical scroll if not already scrolling horizontally + if (Math.abs(e.deltaX) > Math.abs(e.deltaY)) return; + if (!e.ctrlKey) { e.preventDefault(); } container.scrollLeft += e.deltaY; };apps/web/app/dashboard/_components/Notifications/SettingsDropdown.tsx (2)
25-31: Add missing semicolonMissing semicolon after the notificationOptions array declaration.
const notificationOptions: NotificationOption[] = [ { icon: faComment, label: "Comments", value: "pauseComments" }, { icon: faReply, label: "Replies", value: "pauseReplies" }, { icon: faEye, label: "Views", value: "pauseViews" }, { icon: faThumbsUp, label: "Reactions", value: "pauseReactions" } -] +];
53-53: Improve toast message readabilityThe toast message uses nested ternary operators making it hard to read.
-toast.success(`Notifications from ${option.label} have been ${!(currentPrefs[option.value] ?? false) ? "paused" : "unpaused"}`); +const isPaused = !(currentPrefs[option.value] ?? false); +const action = isPaused ? "paused" : "unpaused"; +toast.success(`${option.label} notifications have been ${action}`);apps/web/actions/notifications/create-notification.ts (1)
124-124: Consider debouncing dashboard revalidation.Each notification creation triggers a dashboard revalidation. For high-traffic videos, this could cause performance issues.
Consider implementing a debounced revalidation mechanism or batch processing notifications to reduce the frequency of revalidations.
packages/database/schema.ts (1)
66-75: LGTM! Well-structured preferences design.The JSON preferences column with TypeScript typing provides a clean, extensible way to store user notification preferences. The default null value enables gradual rollout.
Consider adding an index if you plan to query users by specific preference values frequently:
-- Example: If querying users who haven't paused comments CREATE INDEX idx_users_preferences_comments ON users ((JSON_EXTRACT(preferences, '$.notifications.pauseComments')));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
apps/web/actions/notifications/create-notification.ts(1 hunks)apps/web/actions/notifications/mark-all-as-read.ts(1 hunks)apps/web/actions/notifications/update-preferences.ts(1 hunks)apps/web/actions/videos/delete-comment.ts(2 hunks)apps/web/actions/videos/new-comment.ts(3 hunks)apps/web/app/(org)/dashboard/Contexts.tsx(6 hunks)apps/web/app/(org)/dashboard/_components/DashboardInner.tsx(6 hunks)apps/web/app/(org)/dashboard/dashboard-data.ts(6 hunks)apps/web/app/(org)/dashboard/layout.tsx(3 hunks)apps/web/app/api/notifications/preferences/route.ts(1 hunks)apps/web/app/api/notifications/route.ts(1 hunks)apps/web/app/dashboard/_components/Notifications/FilterTabs.tsx(1 hunks)apps/web/app/dashboard/_components/Notifications/NotificationFooter.tsx(1 hunks)apps/web/app/dashboard/_components/Notifications/NotificationHeader.tsx(1 hunks)apps/web/app/dashboard/_components/Notifications/NotificationItem.tsx(1 hunks)apps/web/app/dashboard/_components/Notifications/SettingsDropdown.tsx(1 hunks)apps/web/app/dashboard/_components/Notifications/Skeleton.tsx(1 hunks)apps/web/app/dashboard/_components/Notifications/index.tsx(1 hunks)apps/web/app/dashboard/_components/Notifications/mockData.ts(1 hunks)apps/web/app/dashboard/_components/Notifications/types.ts(1 hunks)apps/web/app/globals.css(1 hunks)apps/web/app/s/[videoId]/_components/Toolbar.tsx(1 hunks)apps/web/app/s/[videoId]/_components/tabs/Activity/Comment.tsx(6 hunks)apps/web/app/s/[videoId]/_components/tabs/Activity/Comments.tsx(4 hunks)apps/web/app/s/[videoId]/page.tsx(8 hunks)apps/web/components/VideoThumbnail.tsx(2 hunks)apps/web/utils/auth.ts(1 hunks)packages/database/migrations/meta/0007_snapshot.json(1 hunks)packages/database/migrations/meta/_journal.json(1 hunks)packages/database/schema.ts(3 hunks)packages/ui/style/tailwind.config.js(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Validate Migrations
packages/database/migrations/meta/_journal.json
[error] 2-2: Migration journal version cannot be changed (was: 5, now: unknown).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
🔇 Additional comments (55)
packages/ui/style/tailwind.config.js (2)
1-1: LGTM! Good convention for unused parameters.Prefixing unused parameters with underscores clearly indicates they're not used in the function body, following JavaScript best practices.
76-76: LGTM! Alpha-transparent gray colors support the new notification UI.The addition of
gray-acolor scale with alpha variants is well-suited for subtle UI elements like notification badges, dropdown backgrounds, and hover states in the new notification components.apps/web/app/s/[videoId]/_components/Toolbar.tsx (1)
10-10: LGTM! Updated motion library API usage.The change from
motion(Button)tomotion.create(Button)aligns with the updated usage pattern of the motion library while maintaining the same functionality.packages/database/migrations/meta/_journal.json (1)
54-60: Migration version consistency confirmedAll existing migration journal entries and snapshots—including the newly added entry—still use
"version": "5", so the journal version hasn’t actually changed. The pipeline error claiming “was: 5, now: unknown” isn’t due to this migration entry. Please investigate the migration loader or CI configuration for why it’s reporting an unknown version.Likely an incorrect or invalid review comment.
apps/web/app/globals.css (2)
552-554: LGTM! Clean scrollbar hiding for notification UI.The
.hide-scrollclass provides a clean way to hide scrollbars in WebKit browsers, which will improve the visual appearance of notification panels and dropdowns.
558-558: LGTM! Better scrollbar behavior with overflow auto.Changing from
overflow-y: scrolltooverflow-y: autoprevents unnecessary scrollbars from appearing when content doesn't overflow, creating a cleaner user experience.apps/web/utils/auth.ts (1)
16-16: LGTM! Type refinement improves precision.Excluding the
folderIdproperty from the video parameter type makes the function signature more precise, as this property isn't used in the access control logic. This aligns with consistent video object handling across the codebase.apps/web/actions/videos/new-comment.ts (2)
8-8: LGTM: Clean integration of notification creation.The import and integration of the notification system into the comment creation flow is well-structured.
26-30: LGTM: Notification type logic is correct.The conditional logic properly maps comment scenarios to notification types:
- Reply when parentCommentId exists
- Reaction for emoji type
- Standard comment otherwise
apps/web/components/VideoThumbnail.tsx (2)
45-46: LGTM: Clean template literal formatting.The single-line template literal maintains the same conditional logic while improving readability.
57-57: LGTM: Optimized useEffect dependencies.Using specific properties (
imageUrl.refetch,uploadingCapId) instead of the entireimageUrlobject prevents unnecessary re-renders and improves performance.apps/web/actions/notifications/mark-all-as-read.ts (1)
9-26: LGTM: Well-structured server action with proper authentication and error handling.The function correctly:
- Authenticates the user before proceeding
- Uses proper database operations with Drizzle ORM
- Handles errors appropriately
- Revalidates the dashboard path for fresh data
apps/web/app/(org)/dashboard/_components/DashboardInner.tsx (4)
3-3: LGTM: Clean imports and additions for notifications feature.The new imports are well-organized and necessary for the notification functionality.
Also applies to: 15-24, 29-29
53-53: LGTM: Proper integration with dashboard context and click-away functionality.The integration with
anyNewNotificationsfrom the dashboard context and theuseClickAwayhook implementation are well-structured for managing the notification dropdown state.Also applies to: 67-75
122-128: LGTM: Well-implemented notification indicator.The red pulsing dot with conditional rendering based on
anyNewNotificationsprovides clear visual feedback to users about new notifications.
348-349: LGTM: Consistent color update for notification dots.The color change from
bg-red-500tobg-red-400maintains consistency across notification indicators in the interface.apps/web/app/dashboard/_components/Notifications/mockData.ts (1)
1-1: LGTM!The import statement correctly references the local types file.
apps/web/app/api/notifications/preferences/route.ts (3)
16-25: LGTM!Authentication handling is correct with proper 401 response for unauthorized access.
27-39: LGTM!Database query is efficient and well-structured. The Zod validation ensures type safety and the response structure is appropriate.
40-49: LGTM!Error handling follows best practices with appropriate logging and generic error messages for security.
apps/web/app/(org)/dashboard/layout.tsx (5)
5-5: LGTM!Import addition for UserPreferences type is correct and consistent with existing imports.
31-32: LGTM!Variable declarations are properly typed with appropriate default values.
36-38: LGTM!Data assignments use appropriate optional chaining and fallback values for robust error handling.
43-44: LGTM!Error handling provides safe defaults that prevent application crashes when dashboard data fails to load.
73-74: LGTM!New props for notifications and user preferences are correctly passed to DashboardContexts, enabling notification system integration.
apps/web/actions/notifications/update-preferences.ts (5)
9-18: LGTM!Function signature is well-typed with comprehensive notification preferences structure including all notification types.
19-22: LGTM!Authentication check is proper with clear error handling for unauthorized access.
24-32: LGTM!Database update operation is secure and well-structured, properly setting nested notification preferences with user-specific filtering.
33-33: LGTM!Cache revalidation ensures the dashboard UI immediately reflects updated notification preferences.
34-37: LGTM!Error handling follows best practices with debugging logs and secure generic error messages.
apps/web/app/s/[videoId]/_components/tabs/Activity/Comments.tsx (4)
15-15: LGTM!Import of useSearchParams is appropriate for handling comment URL parameters for highlighting functionality.
31-44: LGTM!Smart implementation of conditional scrolling - when a specific comment is being highlighted via URL parameter, auto-scroll to bottom is appropriately disabled.
145-156: LGTM!Addition of optional parentId parameter enables proper handling of both root comment and reply deletions, aligning with the enhanced notification system.
169-189: LGTM!Simplified comment rendering removes unnecessary complexity while maintaining functionality. The direct mapping approach is cleaner and more straightforward.
apps/web/app/(org)/dashboard/Contexts.tsx (1)
9-9: LGTM! Clean context integration.The new
anyNewNotificationsanduserPreferencesproperties are properly integrated into the context with correct typing and threading through all necessary locations.Also applies to: 21-22, 51-52, 62-63, 142-143
apps/web/app/dashboard/_components/Notifications/NotificationHeader.tsx (2)
14-28: Excellent async operation handling.The
markAllAsReadHandlerproperly manages loading state, error handling, and cache invalidation. The loading state prevents double-clicks, and the cache invalidation ensures the UI stays in sync.
35-35: Good loading state UX.The conditional styling with
clsxproperly disables interaction during loading while providing visual feedback to the user.apps/web/app/api/notifications/route.ts (1)
19-25: LGTM! Proper authentication check.The authentication verification is correctly implemented with appropriate error response.
apps/web/app/dashboard/_components/Notifications/index.tsx (1)
42-45: LGTM! Excellent use of memoization.The filtered notifications and empty state calculations are properly memoized to prevent unnecessary re-computations.
Also applies to: 47-49
apps/web/app/s/[videoId]/_components/tabs/Activity/Comment.tsx (3)
12-13: LGTM! Clean import reordering.The import reordering improves code organization without affecting functionality.
62-66: Excellent comment highlighting implementation.The combination of DOM targeting with
idattribute and conditional motion animation provides smooth user experience when navigating to specific comments from notifications. The animation timing and easing values are well-chosen.Also applies to: 69-79
22-22: Good callback signature enhancement.The addition of optional
parentIdparameter toonDeleteproperly supports the enhanced notification system for handling nested comment deletions.Also applies to: 54-54
apps/web/app/dashboard/_components/Notifications/NotificationItem.tsx (2)
67-80: LGTM!The icon rendering logic is well-structured, with appropriate icons for each notification type and showing the reaction emoji for REACTION type.
34-39: Add image source validation for securityThe avatar image source is used directly without validation, which could potentially lead to XSS attacks if the author.avatar URL is malicious.
Consider using Next.js Image component or validate the URL:
-<img - src={notification.author.avatar} - alt={notification.author.name} - className="object-cover rounded-full size-10" -/> +<img + src={notification.author.avatar} + alt={notification.author.name} + className="object-cover rounded-full size-10" + onError={(e) => { + e.currentTarget.style.display = 'none'; + e.currentTarget.parentElement?.querySelector('.avatar-fallback')?.classList.remove('hidden'); + }} +/>Likely an incorrect or invalid review comment.
apps/web/actions/videos/delete-comment.ts (3)
28-46: Excellent transaction implementation and security validationGreat job implementing the deletion within a transaction and verifying comment ownership before proceeding. This ensures data consistency and prevents unauthorized deletions.
78-84: LGTM!Proper error handling with logging and re-throwing for upstream handling.
47-76: Remove portability concern: MySQL is the only supported dialect
The Drizzle configuration (packages/database/drizzle.config.ts) explicitly enforces amysqldialect (and throws otherwise), so usingJSON_EXTRACTis safe and expected in this codebase. You can ignore the portability warning for PostgreSQL or other databases here.Likely an incorrect or invalid review comment.
apps/web/app/dashboard/_components/Notifications/Skeleton.tsx (2)
4-55: Well-structured skeleton componentExcellent implementation of the skeleton loader that accurately mirrors the NotificationItem structure with appropriate sizing and spacing.
57-67: LGTM!Clean implementation with configurable count and proper border handling.
apps/web/app/dashboard/_components/Notifications/FilterTabs.tsx (1)
70-76: LGTM!Nice use of framer-motion for smooth tab indicator animations.
apps/web/app/dashboard/_components/Notifications/SettingsDropdown.tsx (1)
88-127: Excellent submenu interaction handlingGreat implementation of the submenu hover logic that prevents the common issue of submenus disappearing when moving between parent and child elements.
apps/web/app/s/[videoId]/page.tsx (1)
520-524: No changes needed—query safely handles non-existent commentIdThe
CASE WHENclause falls back to the defaultcreatedAt DESCordering if the providedcommentIdisn’t found, and Drizzle-ORM automatically parameterizes the input to prevent injection.apps/web/app/(org)/dashboard/dashboard-data.ts (1)
94-107: Efficient notification check implementation.Good use of
limit(1)to check for any unread notifications without fetching all of them.apps/web/app/dashboard/_components/Notifications/types.ts (1)
1-62: Well-structured type definitions for the notification system.The type definitions are comprehensive and align well with the database schema and PR objectives. The enum values fit within the varchar(10) constraint, and the filter mapping provides a clean abstraction for the UI.
packages/database/schema.ts (1)
348-357: LGTM! Relations properly defined.The notification relations correctly establish the connections between notifications, organizations, and users. The implementation follows the established patterns in the schema and provides the necessary foreign key relationships for the notification system.
apps/web/app/dashboard/_components/Notifications/NotificationItem.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/database/schema.ts (2)
66-75: Prefer a non-null default forpreferencesJSONUsing
nullforces every consumer to add null-checks before accessingpreferences.notifications.*.
Consider defaulting to an object with all flagsfalseto simplify queries / JSON extraction and avoid surprises onJSON_EXTRACT.- } | null>() - .default(null), + }>() + .default({ + notifications: { + pauseComments: false, + pauseReplies: false, + pauseViews: false, + pauseReactions: false, + }, + }),
313-342: Re-evaluate redundant single-column indexes
readAtIndex&createdAtIndexare largely covered by the composite
recipientReadIndexandrecipientCreatedIndex. Keeping both pairs doubles
write-amplification without a clear query benefit unless you also filter
by those columns alone.Action:
- Confirm whether queries ever filter only on
readAtorcreatedAt.- If not, drop the two single-column indexes to reduce size & insert cost:
- readAtIndex: index("read_at_idx").on(table.readAt), - createdAtIndex: index("created_at_idx").on(table.createdAt),Bonus: the current
varchar(10)limit could be tight for future types
(e.g."subscriptionCancel"). Bumping to 20 chars provides headroom.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/web/actions/notifications/create-notification.ts(1 hunks)apps/web/actions/videos/new-comment.ts(3 hunks)apps/web/app/(org)/dashboard/_components/DashboardInner.tsx(6 hunks)apps/web/app/api/notifications/preferences/route.ts(1 hunks)apps/web/app/dashboard/_components/Notifications/FilterTabs.tsx(1 hunks)apps/web/app/dashboard/_components/Notifications/NotificationItem.tsx(1 hunks)apps/web/app/dashboard/_components/Notifications/index.tsx(1 hunks)apps/web/app/s/[videoId]/page.tsx(9 hunks)packages/database/migrations/meta/0008_snapshot.json(1 hunks)packages/database/migrations/meta/_journal.json(1 hunks)packages/database/schema.ts(5 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/database/migrations/meta/_journal.json
- packages/database/migrations/meta/0008_snapshot.json
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/web/app/(org)/dashboard/_components/DashboardInner.tsx
- apps/web/app/api/notifications/preferences/route.ts
- apps/web/actions/videos/new-comment.ts
- apps/web/app/dashboard/_components/Notifications/NotificationItem.tsx
- apps/web/app/dashboard/_components/Notifications/FilterTabs.tsx
- apps/web/app/dashboard/_components/Notifications/index.tsx
- apps/web/actions/notifications/create-notification.ts
- apps/web/app/s/[videoId]/page.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (7)
packages/local-docker/package.json (1)
12-14: Pin zod to a specific 3.x and/or manage centrally to avoid version drift.Using
"^3"can lead to different minors across packages. Prefer a single pinned minor (e.g.,^3.23.8) or enforce via rootpnpm.overrides.Option A — pin in this package:
- "zod": "^3" + "zod": "^3.23.8"Option B — enforce globally (in repo root package.json):
{ "pnpm": { "overrides": { "zod": "3.23.8" } } }packages/web-api-contract/src/index.ts (3)
99-103: Count typing is too loose. Prefer a typed shape for client safety.
z.record(z.string(), z.number())weakens client guarantees. Consider a strict shape (e.g.,all,unread,view,comment,reply,reaction) to reduce runtime key checks.- count: z.record(z.string(), z.number()), + count: z.object({ + all: z.number(), + unread: z.number(), + view: z.number(), + comment: z.number(), + reply: z.number(), + reaction: z.number(), + }),
8-8: Optional: validate avatar as URL.If avatars are URLs, tighten validation:
z.string().url().nullable().- avatar: z.string().nullable(), + avatar: z.string().url().nullable(),
45-54: Commented-out “mention” variant. Track explicitly or remove until implemented.Avoid leaving commented code. Open an issue or add a TODO with an owner/date, or remove.
I can open a tracking issue and add a TODO reference inline if you want.
apps/web/lib/Notification.ts (3)
47-50: HydratedNotification is partial/awkward; either generalize or remove.It only covers
viewand a special-casedcomment. Consider a consistent hydrated type for all variants or drop it until needed.
131-132: Remove unused variable.
nowis declared but unused.- const now = new Date();
92-129: Enforce uniqueness at the DB to avoid race-condition duplicates.The read-then-insert pattern can race. Add a unique constraint keyed by logical identity (e.g., for comment/reply/reaction: (type, recipientId, orgId, commentId); for view: (type, recipientId, orgId, videoId, authorId)). Then use “insert ignore”/conflict handling.
I can draft a migration for
packages/database/schema.tswith Drizzle unique indexes if you share the current table definition.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (29)
apps/desktop/package.json(1 hunks)apps/discord-bot/package.json(1 hunks)apps/storybook/package.json(1 hunks)apps/tasks/package.json(1 hunks)apps/web/actions/videos/new-comment.ts(3 hunks)apps/web/app/api/notifications/route.ts(1 hunks)apps/web/app/dashboard/_components/Filter.ts(1 hunks)apps/web/app/dashboard/_components/Notifications/FilterTabs.tsx(1 hunks)apps/web/app/dashboard/_components/Notifications/NotificationItem.tsx(1 hunks)apps/web/app/dashboard/_components/Notifications/index.tsx(1 hunks)apps/web/app/dashboard/_components/Notifications/mockData.ts(1 hunks)apps/web/app/s/[videoId]/page.tsx(9 hunks)apps/web/lib/Notification.ts(1 hunks)apps/web/package.json(1 hunks)infra/package.json(1 hunks)packages/config/package.json(1 hunks)packages/database/package.json(1 hunks)packages/database/schema.ts(3 hunks)packages/env/package.json(1 hunks)packages/local-docker/package.json(1 hunks)packages/tsconfig/package.json(1 hunks)packages/ui-solid/package.json(1 hunks)packages/ui/package.json(1 hunks)packages/utils/package.json(1 hunks)packages/web-api-contract-effect/package.json(1 hunks)packages/web-api-contract/package.json(1 hunks)packages/web-api-contract/src/desktop.ts(2 hunks)packages/web-api-contract/src/index.ts(2 hunks)packages/web-api-contract/tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (17)
- apps/web/package.json
- infra/package.json
- apps/desktop/package.json
- packages/web-api-contract/package.json
- packages/config/package.json
- packages/database/package.json
- packages/utils/package.json
- packages/web-api-contract-effect/package.json
- packages/env/package.json
- packages/ui/package.json
- packages/ui-solid/package.json
- apps/tasks/package.json
- apps/discord-bot/package.json
- apps/storybook/package.json
- packages/web-api-contract/tsconfig.json
- packages/web-api-contract/src/desktop.ts
- apps/web/app/dashboard/_components/Filter.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/web/actions/videos/new-comment.ts
- apps/web/app/s/[videoId]/page.tsx
- apps/web/app/dashboard/_components/Notifications/mockData.ts
- apps/web/app/dashboard/_components/Notifications/NotificationItem.tsx
- apps/web/app/dashboard/_components/Notifications/index.tsx
- apps/web/app/api/notifications/route.ts
- apps/web/app/dashboard/_components/Notifications/FilterTabs.tsx
- packages/database/schema.ts
🔇 Additional comments (1)
packages/web-api-contract/src/index.ts (1)
11-61: Dates serialization confirmed
NextResponse.json stringifies Date objects to ISO-8601 strings, which satisfy thez.coerce.date()schema on the client. No changes needed.
| get: { | ||
| method: "GET", | ||
| path: "/notifications", | ||
| responses: { | ||
| 200: z.object({ | ||
| notifications: z.array(Notification), | ||
| count: z.record(z.string(), z.number()), | ||
| }), | ||
| }, | ||
| }, | ||
| }), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider pagination and filtering in the notifications GET endpoint.
Large orgs will need pagination and server-side filtering (type/unread). Add limit, cursor, and optional type to the query and return nextCursor.
- get: {
- method: "GET",
- path: "/notifications",
- responses: {
+ get: {
+ method: "GET",
+ path: "/notifications",
+ query: z.object({
+ limit: z.number().min(1).max(100).default(20).optional(),
+ cursor: z.string().optional(),
+ type: z.enum(["view", "comment", "reply", "reaction"]).optional(),
+ unreadOnly: z.boolean().optional(),
+ }),
+ responses: {
notifications: z.array(Notification),
- count: z.record(z.string(), z.number()),
+ count: z.object({
+ all: z.number(),
+ unread: z.number(),
+ view: z.number(),
+ comment: z.number(),
+ reply: z.number(),
+ reaction: z.number(),
+ }),
+ nextCursor: z.string().nullable(),
}),
},
},Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/web-api-contract/src/index.ts between lines 95 and 105, the GET
/notifications endpoint lacks pagination and filtering support. Modify the
endpoint to accept query parameters: limit (number), cursor (string), and an
optional type (string) for filtering notifications. Update the response schema
to include a nextCursor field to enable clients to paginate through results.
Ensure the query parameters and response are properly validated using zod
schemas.
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (9)
apps/web/utils/organization.ts (1)
25-25: Ensure consumers handle potentially “unlimited” semantics forremainingSeats.If you keep the dev override, verify no UI directly renders
remainingSeatsor divides by it. Prefer returning a boolean flag (e.g.,seatLimitDisabled) to control gating and display.apps/web/app/(org)/dashboard/_components/Notifications/NotificationFooter.tsx (1)
10-15: Make non-interactive icons hidden from screen readers (or convert to hints)These arrow glyphs look clickable but aren’t interactive. Hide purely decorative icons from AT, or render them as keyboard hint elements.
- <FontAwesomeIcon icon={faArrowDown} className="text-gray-2 size-2.5" /> + <FontAwesomeIcon icon={faArrowDown} className="text-gray-2 size-2.5" aria-hidden="true" /> ... - <FontAwesomeIcon icon={faArrowUp} className="text-gray-2 size-2.5" /> + <FontAwesomeIcon icon={faArrowUp} className="text-gray-2 size-2.5" aria-hidden="true" />If these are intended as controls, convert them to buttons with aria-labels and wire them to actual navigation; otherwise consider rendering with for clarity. Would you like me to wire them up to navigate the list?
apps/web/app/(org)/dashboard/_components/Notifications/NotificationHeader.tsx (1)
14-28: Guard against double-invoking while loadingMinor, but fast repeated clicks can enqueue multiple requests. Add a guard at the start.
const markAllAsReadHandler = async () => { + if (loading) return; try { setLoading(true);apps/web/app/(org)/dashboard/_components/Notifications/NotificationItem.tsx (2)
29-29: Avoid prefetching every notification link to reduce network overheadLong lists prefetching all links can be costly. Disable prefetch here; the page will still navigate instantly.
- <Link href={link}> + <Link href={link} prefetch={false}>
38-46: Optional: use next/image for avatars + better alt textSwitching to next/image will improve performance and optimize loading. Also consider alt={
Avatar of ${notification.author.name}} for clarity.apps/web/app/(org)/dashboard/_components/Notifications/FilterTabs.tsx (1)
46-49: Add tablist semantics for accessibilityExpose the container as a tablist to assistive tech.
- <div + <div + role="tablist" ref={scrollContainerRef} className="flex isolate overflow-x-auto relative gap-4 items-center px-6 border-r border-b border-l hide-scroll border-gray-3" >apps/web/app/(org)/dashboard/_components/Notifications/SettingsDropdown.tsx (1)
151-151: Export style consistencyYou export both a named and a default with the same symbol. Prefer one style to avoid import confusion.
Options:
- Keep only the named export and remove default, or
- Keep only default and import it as default everywhere.
apps/web/app/(org)/dashboard/_components/Notifications/Skeleton.tsx (1)
12-20: Mark decorative skeletons as hidden from screen readersThese blocks are purely visual placeholders. Add aria-hidden to avoid noisy SR output while loading.
- <div className={clsx("flex gap-3 p-4 border-r border-l border-gray-3", className)}> + <div aria-hidden="true" className={clsx("flex gap-3 p-4 border-r border-l border-gray-3", className)}>Also applies to: 24-41, 45-51
apps/web/app/(org)/dashboard/_components/Notifications/index.tsx (1)
97-103: Optional: expose loading state to AT via aria-busy on the scroll containerThis helps announce loading state without being verbose.
- <div ref={scrollRef} className="isolate flex-1 h-full custom-scroll"> + <div ref={scrollRef} aria-busy={notifications.isPending} className="isolate flex-1 h-full custom-scroll">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/web/app/(org)/dashboard/_components/DashboardInner.tsx(6 hunks)apps/web/app/(org)/dashboard/_components/Notifications/Filter.ts(1 hunks)apps/web/app/(org)/dashboard/_components/Notifications/FilterTabs.tsx(1 hunks)apps/web/app/(org)/dashboard/_components/Notifications/NotificationFooter.tsx(1 hunks)apps/web/app/(org)/dashboard/_components/Notifications/NotificationHeader.tsx(1 hunks)apps/web/app/(org)/dashboard/_components/Notifications/NotificationItem.tsx(1 hunks)apps/web/app/(org)/dashboard/_components/Notifications/SettingsDropdown.tsx(1 hunks)apps/web/app/(org)/dashboard/_components/Notifications/Skeleton.tsx(1 hunks)apps/web/app/(org)/dashboard/_components/Notifications/index.tsx(1 hunks)apps/web/utils/organization.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/web/app/(org)/dashboard/_components/Notifications/Filter.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/app/(org)/dashboard/_components/DashboardInner.tsx
🔇 Additional comments (3)
apps/web/utils/organization.ts (1)
27-27: No action needed.Closing brace change is incidental.
apps/web/app/(org)/dashboard/_components/Notifications/SettingsDropdown.tsx (1)
16-19: Verify updatePreferences is client-safe (not a direct server action import)If updatePreferences is a Server Action (with "use server"), importing it here will fail. Ensure this import targets a client-safe wrapper (e.g., API route, RPC, or a “safe action” client).
If it is a Server Action, move the call behind a client wrapper or invoke via fetch to an API route. I can generate the wrapper for you.
Also applies to: 37-62
apps/web/app/(org)/dashboard/_components/Notifications/Skeleton.tsx (1)
57-65: LGTM: simple, predictable skeleton list renderingIndex key usage is acceptable for static skeletons; border application between items is correct.
| <div key={filter} className="relative min-w-fit"> | ||
| <div | ||
| onClick={() => setActiveFilter(filter)} | ||
| className="flex relative gap-2 items-center py-4 cursor-pointer group" | ||
| > | ||
| <p |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use a button for each tab; expose pressed state
Clickable divs are not accessible by keyboard by default. Use a button and aria-pressed for the active tab.
- <div key={filter} className="relative min-w-fit">
- <div
- onClick={() => setActiveFilter(filter)}
- className="flex relative gap-2 items-center py-4 cursor-pointer group"
- >
+ <div key={filter} className="relative min-w-fit">
+ <button
+ type="button"
+ role="tab"
+ aria-pressed={activeFilter === filter}
+ onClick={() => setActiveFilter(filter)}
+ className="flex relative gap-2 items-center py-4 cursor-pointer group"
+ >
...
- <div className="flex justify-center items-center rounded-md size-4 bg-gray-4">
+ <div className="flex justify-center items-center rounded-md size-4 bg-gray-4" aria-hidden="true">
{loading ? (
<span className="size-1.5 rounded-full bg-gray-10" />
) : (
<p
className={clsx(
"text-[10px] transition-colors",
activeFilter === filter
? "text-gray-12"
: "text-gray-10 group-hover:text-gray-11"
)}
>
{filter === "all"
? totalCount
: count?.[filter.toLowerCase() as NotificationType] ?? 0}
</p>
)}
</div>
- </div>
+ </button>Also applies to: 63-66, 70-84
🤖 Prompt for AI Agents
In apps/web/app/(org)/dashboard/_components/Notifications/FilterTabs.tsx around
lines 51 to 56, replace the clickable div elements used for tabs with button
elements to improve accessibility. Add the aria-pressed attribute to each button
to indicate the active tab state. Apply the same changes to lines 63-66 and
70-84 to ensure all tab controls are accessible via keyboard and properly expose
their pressed state.
| const { className } = props; | ||
| const { activeOrganization } = useDashboardContext(); |
There was a problem hiding this comment.
Props spreading overrides className and onClick; compose handlers and pass only rest props
Spreading props after specifying className and onClick overrides them, dropping your panel styles and stopPropagation when a caller provides these props.
- const { className } = props;
+ const { className, onClick, ...rest } = props;- onClick={(e) => e.stopPropagation()}
+ onClick={(e) => {
+ e.stopPropagation();
+ onClick?.(e);
+ }}
className={clsx(
"flex absolute right-0 top-12 flex-col rounded-xl cursor-default w-[400px] h-[450px] bg-gray-1",
className
)}
- {...props}
+ {...rest}Also applies to: 83-89
🤖 Prompt for AI Agents
In apps/web/app/(org)/dashboard/_components/Notifications/index.tsx around lines
23 to 24 and also lines 83 to 89, the current code spreads all props after
explicitly setting className and onClick, which causes the spread props to
override these specific handlers and styles. To fix this, destructure className
and onClick from props first, then compose your own onClick handler with the
passed one, and finally spread only the remaining props. This ensures your panel
styles and event handling are preserved while still passing other props
correctly.
| const notifications = useQuery({ | ||
| queryKey: ["notifications", activeOrganization?.organization.id], | ||
| queryFn: async () => { | ||
| const resp = await api.notifications.get(); | ||
| if (resp.status !== 200) { | ||
| toast.error("Failed to fetch notifications"); | ||
| return { notifications: [], count: {} }; | ||
| } | ||
|
|
||
| return resp.body; | ||
| }, | ||
| refetchOnWindowFocus: false, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden query: avoid fetching before org is ready and handle thrown errors
Currently the query may run with undefined org and unhandled rejections end up in isError, producing an empty UI. Add enabled and wrap in try/catch to always return a safe fallback and toast once.
const notifications = useQuery({
queryKey: ["notifications", activeOrganization?.organization.id],
- queryFn: async () => {
- const resp = await api.notifications.get();
- if (resp.status !== 200) {
- toast.error("Failed to fetch notifications");
- return { notifications: [], count: {} };
- }
-
- return resp.body;
- },
- refetchOnWindowFocus: false,
+ queryFn: async () => {
+ try {
+ const resp = await api.notifications.get();
+ if (resp.status !== 200) {
+ toast.error("Failed to fetch notifications");
+ return { notifications: [], count: {} as Record<string, number> };
+ }
+ return resp.body;
+ } catch {
+ toast.error("Failed to fetch notifications");
+ return { notifications: [], count: {} as Record<string, number> };
+ }
+ },
+ refetchOnWindowFocus: false,
+ enabled: !!activeOrganization?.organization?.id,
});📝 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.
| const notifications = useQuery({ | |
| queryKey: ["notifications", activeOrganization?.organization.id], | |
| queryFn: async () => { | |
| const resp = await api.notifications.get(); | |
| if (resp.status !== 200) { | |
| toast.error("Failed to fetch notifications"); | |
| return { notifications: [], count: {} }; | |
| } | |
| return resp.body; | |
| }, | |
| refetchOnWindowFocus: false, | |
| }); | |
| const notifications = useQuery({ | |
| queryKey: ["notifications", activeOrganization?.organization.id], | |
| queryFn: async () => { | |
| try { | |
| const resp = await api.notifications.get(); | |
| if (resp.status !== 200) { | |
| toast.error("Failed to fetch notifications"); | |
| return { notifications: [], count: {} as Record<string, number> }; | |
| } | |
| return resp.body; | |
| } catch { | |
| toast.error("Failed to fetch notifications"); | |
| return { notifications: [], count: {} as Record<string, number> }; | |
| } | |
| }, | |
| refetchOnWindowFocus: false, | |
| enabled: !!activeOrganization?.organization?.id, | |
| }); |
🤖 Prompt for AI Agents
In apps/web/app/(org)/dashboard/_components/Notifications/index.tsx around lines
29 to 41, the query runs even if activeOrganization is undefined and does not
handle thrown errors properly. Fix this by adding an 'enabled' option to the
useQuery call that checks if activeOrganization?.organization.id is defined, and
wrap the queryFn in a try/catch block to catch errors, show a toast error
message once, and return a safe fallback object with empty notifications and
count.
apps/web/app/(org)/dashboard/_components/Notifications/SettingsDropdown.tsx
Show resolved
Hide resolved
apps/web/app/(org)/dashboard/_components/Notifications/SettingsDropdown.tsx
Show resolved
Hide resolved
| <Menu.Item> | ||
| {({ active }) => ( | ||
| <div | ||
| className={clsx( | ||
| "flex flex-1 items-center group justify-between px-2 py-1 min-w-fit text-[13px] text-gray-11 rounded-lg cursor-pointer outline-none", | ||
| active || showPauseSubmenu ? "bg-gray-3" : "" | ||
| )} | ||
| onMouseEnter={() => setShowPauseSubmenu(true)} | ||
| onMouseLeave={() => setShowPauseSubmenu(false)} | ||
| > | ||
| <div className="flex gap-2 items-center"> | ||
| <FontAwesomeIcon | ||
| icon={faBellSlash} | ||
| className="text-gray-10 text-[13px] size-3.5 transition-colors group-hover:text-gray-12" | ||
| /> | ||
| <p className="text-[13px] transition-colors group-hover:text-gray-12"> | ||
| Pause notifications | ||
| </p> | ||
| </div> | ||
| <FontAwesomeIcon | ||
| icon={faArrowUp} | ||
| className="text-gray-10 size-2.5 rotate-90 transition-colors group-hover:text-gray-12" | ||
| /> | ||
| </div> | ||
| )} | ||
| </Menu.Item> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve accessibility: use buttons for menu items and expose checked state
Use semantic buttons for items and announce the paused state via aria-checked. This also enables keyboard navigation without relying on hover.
- <Menu.Item>
- {({ active }) => (
- <div
+ <Menu.Item>
+ {({ active }) => (
+ <button
className={clsx(
"flex flex-1 items-center group justify-between px-2 py-1 min-w-fit text-[13px] text-gray-11 rounded-lg cursor-pointer outline-none",
active || showPauseSubmenu ? "bg-gray-3" : ""
)}
onMouseEnter={() => setShowPauseSubmenu(true)}
onMouseLeave={() => setShowPauseSubmenu(false)}
+ type="button"
+ aria-haspopup="menu"
>
<div className="flex gap-2 items-center">
<FontAwesomeIcon
icon={faBellSlash}
className="text-gray-10 text-[13px] size-3.5 transition-colors group-hover:text-gray-12"
/>
<p className="text-[13px] transition-colors group-hover:text-gray-12">
Pause notifications
</p>
</div>
<FontAwesomeIcon
icon={faArrowUp}
className="text-gray-10 size-2.5 rotate-90 transition-colors group-hover:text-gray-12"
/>
- </div>
+ </button>
)}
</Menu.Item>
...
- {notificationOptions.map((option, index) => (
- <div
+ {notificationOptions.map((option, index) => (
+ <button
key={index}
className="flex w-full items-center justify-between gap-2 px-2 py-1 text-[13px] text-gray-11 rounded-lg group hover:bg-gray-3 cursor-pointer outline-none"
onClick={async () =>
await updateNotificationPreferences(option)
}
+ type="button"
+ role="menuitemcheckbox"
+ aria-checked={!!userPreferences?.notifications?.[option.value]}
>
<div className="flex gap-1.5 items-center">
<FontAwesomeIcon
icon={option.icon}
className="text-gray-10 size-3.5 transition-colors group-hover:text-gray-12"
/>
<p className="text-[13px] transition-colors group-hover:text-gray-12">
{option.label}
</p>
</div>
- {userPreferences?.notifications[option.value] && (
+ {userPreferences?.notifications?.[option.value] && (
<FontAwesomeIcon
icon={faCheck}
className="text-gray-10 size-2.5 transition-colors group-hover:text-gray-12"
/>
)}
- </div>
+ </button>
))}Also applies to: 116-123, 134-139
🤖 Prompt for AI Agents
In apps/web/app/(org)/dashboard/_components/Notifications/SettingsDropdown.tsx
around lines 82 to 107, replace the div used for the menu item with a semantic
button element to improve accessibility and keyboard navigation. Add the
aria-checked attribute to the button to expose the paused state for screen
readers. Apply similar changes to the menu items in lines 116-123 and 134-139 to
maintain consistency and accessibility across all interactive elements.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
AGENTS.md (8)
31-31: Fix typo: “mangement” → “management”.Minor spelling issue.
-`cap-editor` is the code that runs and communicates with the editor UI to coordinate playback and state mangement. +`cap-editor` is the code that runs and communicates with the editor UI to coordinate playback and state management.
13-15: Tighten wording and clarify directory vs. package naming.More concise phrasing; clearer distinction between package names and directory names.
-Cap Desktop consumes a lot of crates inside `./crates`. -These crates have names prefixed with `cap-`, but their folders are not. -Some crates have READMEs you can use to learn more about their specific APIs. +Cap Desktop uses many crates in `./crates`. +Their package names are prefixed with `cap-`, but their directory names are not. +Some crates include READMEs with API details.
1-1: Unify heading hierarchy; consider renaming file to ARCHITECTURE.md.Use a single H1 and subordinate sections for clarity. Also consider renaming the file to ARCHITECTURE.md so its purpose is obvious.
-# Cap Web +# Architecture overview + +## Cap Web @@ -# Cap Desktop +## Cap Desktop @@ -## Crates +### Crates @@ -## Recording +### Recording @@ -## Editing +### Editing @@ -## Exporting +### ExportingAlso applies to: 6-6, 11-11, 17-17, 28-28, 33-33
4-4: Make paths clickable for easier navigation.-It is located at `./apps/web`. +Source: [apps/web](./apps/web). @@ -It is located at `./apps/desktop`. +Source: [apps/desktop](./apps/desktop).Also applies to: 9-9
24-26: Clarify phrasing and end bullets with periods for consistency.-- `cap-media` contains binding code for all of the above, connecting some of them with FFmpeg -- `cap-media-encoders` contains encoders for various audio and video formats, powered by various media frameworks -- `cap-recording` uses `cap-media` and `cap-media-encoders` to implement instant and studio recording +- `cap-media` provides bindings/glue for the above, integrating FFmpeg where needed. +- `cap-media-encoders` contains encoders for various audio and video formats, powered by various media frameworks. +- `cap-recording` uses `cap-media` and `cap-media-encoders` to implement instant and studio recording.
35-35: Grammar nit: “single files” → “a single file”.-Studio recordings can be exported to single files, and optionally uploaded to Cap Web. `cap-export` uses `cap-rendering` and `cap-media-encoders` to render the recording into a single file. +Studio recordings can be exported to a single file and optionally uploaded to Cap Web. `cap-export` uses `cap-rendering` and `cap-media-encoders` to render the recording into a single file.
21-23: Optional: link out to well-known external crates (makes the doc more useful).Add links for external dependencies readers may want to explore.
-- The external `cpal` crate is used for capturing audio from microphones. +- The external [`cpal`](https://crates.io/crates/cpal) crate is used for capturing audio from microphones. @@ -Studio recordings may be edited to add things like backgrounds, zooms, and cuts. Rendering is done using `wgpu` in `cap-rendering`. +Studio recordings may be edited to add things like backgrounds, zooms, and cuts. Rendering is done using [`wgpu`](https://docs.rs/wgpu/latest/wgpu/) in `cap-rendering`.If “scap” is an external crate, consider linking it as well; if it’s internal, link to its directory under
./crates.Also applies to: 30-30
5-6: Add a brief Notifications subsection under Cap Web to reflect this PR.Since this PR adds notifications, a short section helps readers discover the feature and where it lives.
It is located at `./apps/web`. +### Notifications + +Cap Web includes an organization-scoped notifications system (Views, Comments, Replies, Reactions), with per-type pauseable preferences. Notifications deep-link into videos and highlight the relevant comment. + +- Schema: [`packages/database/schema.ts`](./packages/database/schema.ts) (notifications table; user preferences JSON) +- Web UI: [`apps/web/app/(org)/dashboard/_components/Notifications/`](./apps/web/app/(org)/dashboard/_components/Notifications/) +- Server/Lib: [`apps/web/lib/Notification.ts`](./apps/web/lib/Notification.ts) + +Planned types: Mentions and Recording uploads. +apps/web/utils/organization.ts (1)
18-24: Optional: expose a boolean flag for “unlimited” seats in devReturning Number.MAX_SAFE_INTEGER is correct and JSON-safe, but it may render oddly in UI. Consider adding a flag for clearer semantics.
If you adopt the isDev variable above, extend the return value:
return { inviteQuota, memberCount, pendingInvitesCount, totalUsedSeats, remainingSeats, + seatLimitDisabled: isDev, };This keeps math finite while allowing the UI to display “∞” or a tooltip when seatLimitDisabled is true.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
AGENTS.md(1 hunks)apps/web/app/(org)/dashboard/_components/Notifications/index.tsx(1 hunks)apps/web/app/s/[videoId]/page.tsx(12 hunks)apps/web/components/VideoThumbnail.tsx(1 hunks)apps/web/lib/Notification.ts(1 hunks)apps/web/package.json(1 hunks)apps/web/utils/organization.ts(1 hunks)packages/database/schema.ts(3 hunks)packages/local-docker/package.json(0 hunks)packages/web-api-contract/src/index.ts(2 hunks)
💤 Files with no reviewable changes (1)
- packages/local-docker/package.json
✅ Files skipped from review due to trivial changes (2)
- apps/web/package.json
- apps/web/components/VideoThumbnail.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/web/app/(org)/dashboard/_components/Notifications/index.tsx
- apps/web/lib/Notification.ts
- packages/web-api-contract/src/index.ts
- packages/database/schema.ts
- apps/web/app/s/[videoId]/page.tsx
🧰 Additional context used
🪛 LanguageTool
AGENTS.md
[style] ~13-~13: Consider using a synonym to be more concise.
Context: ...ktop. ## Crates Cap Desktop consumes a lot of crates inside ./crates`. These crates ...
(A_LOT_OF)
[grammar] ~31-~31: Ensure spelling is correct
Context: ...tor UI to coordinate playback and state mangement. ## Exporting Studio recordings can be...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (2)
apps/web/utils/organization.ts (2)
12-12: LGTM: totalUsedSeats calculation is correctSumming members and pending invites is the right invariant to drive remaining seats.
25-25: No functional changeClosing brace appears as a touched line but has no behavioral impact.
| const remainingSeats = | ||
| process.env.NODE_ENv === "development" | ||
| ? Number.MAX_SAFE_INTEGER | ||
| : Math.max(0, inviteQuota - totalUsedSeats); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Critical: Typo in env check prevents dev override and may break client builds
Use process.env.NODE_ENV (uppercase V). The current process.env.NODE_ENv won't be replaced at build-time and will evaluate to undefined, disabling the intended dev behavior and potentially causing runtime issues in the browser.
Apply this fix:
- const remainingSeats =
- process.env.NODE_ENv === "development"
- ? Number.MAX_SAFE_INTEGER
- : Math.max(0, inviteQuota - totalUsedSeats);
+ const isDev = process.env.NODE_ENV === "development";
+ const remainingSeats = isDev
+ ? Number.MAX_SAFE_INTEGER
+ : Math.max(0, inviteQuota - totalUsedSeats);Run to catch similar typos across the repo:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find suspicious NODE_ENV typos
rg -n --no-heading -S 'process\\.env\\.NODE_ENv|NODE_ENv' || trueLength of output: 156
Fix typo in env check and introduce isDev flag
The check for process.env.NODE_ENv uses a lowercase “v,” so it never matches NODE_ENV at build time. Replace it with the correct key and pull out an isDev flag for clarity.
• File: apps/web/utils/organization.ts (lines 13–16)
- const remainingSeats =
- process.env.NODE_ENv === "development"
- ? Number.MAX_SAFE_INTEGER
- : Math.max(0, inviteQuota - totalUsedSeats);
+ const isDev = process.env.NODE_ENV === "development";
+ const remainingSeats = isDev
+ ? Number.MAX_SAFE_INTEGER
+ : Math.max(0, inviteQuota - totalUsedSeats);This fixes the env-key typo and makes the dev‐override intent explicit.
📝 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.
| const remainingSeats = | |
| process.env.NODE_ENv === "development" | |
| ? Number.MAX_SAFE_INTEGER | |
| : Math.max(0, inviteQuota - totalUsedSeats); | |
| const isDev = process.env.NODE_ENV === "development"; | |
| const remainingSeats = isDev | |
| ? Number.MAX_SAFE_INTEGER | |
| : Math.max(0, inviteQuota - totalUsedSeats); |
🤖 Prompt for AI Agents
In apps/web/utils/organization.ts around lines 13 to 16, fix the typo in the
environment variable check by replacing `process.env.NODE_ENv` with the correct
`process.env.NODE_ENV`. Additionally, create a separate `isDev` boolean flag
that checks if `NODE_ENV` equals "development" and use this flag in the
conditional expression for clarity and better readability.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
AGENTS.md (4)
1-10: Introduce a single top-level title and normalize section hierarchyUse one H1 for the doc and demote section headers to H2 for consistency/readability.
-# Cap Web +# Cap Architecture Overview + +## Cap Web @@ -# Cap Desktop +## Cap Desktop
13-15: Tighten wording and clarify directory namingReplace conversational phrasing and clarify that directory names are not prefixed.
-Cap Desktop consumes a lot of crates inside `./crates`. -These crates have names prefixed with `cap-`, but their folders are not. -Some crates have READMEs you can use to learn more about their specific APIs. +Cap Desktop uses many crates in `./crates`. +These crates' package names are prefixed with `cap-`, but their directory names are not. +Some crates include READMEs you can use to learn more about their specific APIs.
31-31: Fix typo: “mangement” → “management”-`cap-editor` is the code that runs and communicates with the editor UI to coordinate playback and state mangement. +`cap-editor` is the code that runs and communicates with the editor UI to coordinate playback and state management.
24-24: Minor wording for accuracy (“bindings”)“Provides bindings” reads better in Rust crate context.
-`cap-media` contains binding code for all of the above, connecting some of them with FFmpeg +`cap-media` provides bindings for the above and integrates some of them with FFmpeg
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
AGENTS.md(1 hunks)apps/web/app/(org)/dashboard/_components/Notifications/index.tsx(1 hunks)apps/web/app/s/[videoId]/page.tsx(12 hunks)apps/web/components/VideoThumbnail.tsx(1 hunks)apps/web/lib/Notification.ts(1 hunks)apps/web/package.json(1 hunks)apps/web/utils/organization.ts(1 hunks)packages/database/schema.ts(3 hunks)packages/local-docker/package.json(0 hunks)packages/web-api-contract/src/index.ts(2 hunks)
💤 Files with no reviewable changes (1)
- packages/local-docker/package.json
✅ Files skipped from review due to trivial changes (1)
- apps/web/package.json
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/web/components/VideoThumbnail.tsx
- apps/web/utils/organization.ts
- apps/web/app/(org)/dashboard/_components/Notifications/index.tsx
- apps/web/lib/Notification.ts
- apps/web/app/s/[videoId]/page.tsx
- packages/web-api-contract/src/index.ts
- packages/database/schema.ts
🧰 Additional context used
🪛 LanguageTool
AGENTS.md
[style] ~13-~13: Consider using a synonym to be more concise.
Context: ...ktop. ## Crates Cap Desktop consumes a lot of crates inside ./crates`. These crates ...
(A_LOT_OF)
[grammar] ~31-~31: Ensure spelling is correct
Context: ...tor UI to coordinate playback and state mangement. ## Exporting Studio recordings can be...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (1)
AGENTS.md (1)
33-36: Add a Notifications section to reflect this PR’s feature scopeThe doc currently omits the newly added Notifications system (types, interactions, preferences, and org scoping), which is central to this PR.
## Exporting Studio recordings can be exported to single files, and optionally uploaded to Cap Web. `cap-export` uses `cap-rendering` and `cap-media-encoders` to render the recording into a single file. + +## Notifications + +The Cap Web dashboard includes organization-scoped notifications to keep users informed about activity relevant to their recordings. + +- Types: Views, Comments, Replies, and Reactions (Mentions and Recording uploads within the organization are planned). +- Interactions: Clicking comment/reaction/reply notifications deep-links to the relevant video page, with the associated comment highlighted and shown first. +- Preferences: Users can pause specific types of notifications via settings. +- Scoping: Notifications are organization-dependent. + +You may also add a link to the demo asset from the PR description for quick reference.Likely an incorrect or invalid review comment.
AGENTS.md
Outdated
| @@ -0,0 +1,35 @@ | |||
| # Cap Web | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Filename-content mismatch: consider renaming AGENTS.md to ARCHITECTURE.md (or similar)
The content is a general architecture/overview document, not “Agents”-specific. Renaming will improve discoverability.
Run this script to check for existing architecture docs and references before renaming:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Checking for existing architecture/overview docs..."
fd -i -t f 'architecture\.md|overview\.md|system\.md|systems\.md|design\.md|architecture/|docs/architecture|docs/overview' || true
echo -e "\nSearching for references to AGENTS.md..."
rg -n --hidden --no-ignore -A2 -B2 'AGENTS\.md' || true
echo -e "\nProposed rename:"
echo "git mv AGENTS.md ARCHITECTURE.md"
echo -e "\nAfter renaming, update any references found above."Length of output: 652
Rename documentation file to ARCHITECTURE.md
No existing architecture/overview docs or references to AGENTS.md were found—this rename won’t conflict.
- Apply the rename:
git mv AGENTS.md ARCHITECTURE.md
🤖 Prompt for AI Agents
In AGENTS.md at line 1, rename the documentation file from AGENTS.md to
ARCHITECTURE.md using the command git mv AGENTS.md ARCHITECTURE.md to better
reflect its content and avoid confusion.
|
Don't merge until https://app.planetscale.com/cap/cap-production/notifications is merged |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/utils/web-api.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Clippy
🔇 Additional comments (1)
apps/web/utils/web-api.ts (1)
11-11: SSR/client baseUrl inversion is correctUsing an absolute URL on the server (no window) and a relative path in the browser resolves Node/SSR relative-fetch issues and matches expected environments. Good fix.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@ameer2468 Hi, I met a migration issue when working on a feature. The direct message was some migration file was not found. Tracking from git commit history I came down to this PR. It seems this PR added 2 new migrations into |
Hi @mogita Yes the journal files were intentionally committed, i should not have committed the SQL though because they are only for when a docker image is put out. |
Thanks for sharing the insight. I'm asking because my testing environment complains about the missing column when a user logs in. Since there's no production data, I'll manually add the column and wait for the SQL files to be committed so I can migrate once. Please advise otherwise. |
This PR: Introduces Notifications to the dashboard of Cap
Arc.2025-08-04.19.06.47.mp4
Notifications are ORG DEPENDANT
Supports various notification types: Views, Comments, Replies, Reactions. Mentions & Recording uploads within the org will come soon.
Settings to pause types of notifications.
Clickable comments, reactions, and replies. You will be redirected to the relevant video page and the comment will show first on the list.
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Style
Chores
zodacross multiple packages).