web: fix avatars in notifications #1292
Conversation
|
Warning Rate limit exceeded@ameer2468 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 31 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (2)
WalkthroughThe frontend NotificationItem swaps inline avatar markup for a SignedImageUrl component; the backend notifications GET handler is refactored to an Effect-based async pipeline that resolves author image URLs (via ImageUploads.resolveImageUrl) in parallel and filters errors before returning results. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant ImageUploads
participant Frontend as NotificationItem
rect rgb(240,248,255)
note over API: Old flow (sync map)
Client->>API: GET /api/notifications
API->>API: sync map/parse notifications
API->>Client: return notifications (avatars raw)
end
rect rgb(230,245,230)
note over API,ImageUploads: New flow (Effect-based, parallel)
Client->>API: GET /api/notifications
API->>ImageUploads: resolveImageUrl(author.avatar) [parallel...]
ImageUploads-->>API: signed URL / null
API->>API: filter/catch errors via Effect.catchAll
API->>Client: return notifications (resolved signed avatars)
end
Client->>Frontend: render notifications
Frontend->>Frontend: render <SignedImageUrl image=... name=... />
Frontend->>Client: display notification with signed avatar
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/web/app/api/notifications/route.ts (1)
131-131: Use a type predicate for proper TypeScript narrowing.
filter(Boolean)may not provide proper TypeScript type narrowing from(APINotification | null)[]toAPINotification[].Apply this diff for explicit type narrowing:
.pipe(runPromise) - .then((results) => results.filter(Boolean)); + .then((results) => results.filter((x): x is APINotification => x !== null));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/app/(org)/dashboard/_components/Notifications/NotificationItem.tsx(2 hunks)apps/web/app/api/notifications/route.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.Use strict TypeScript and avoid any; leverage shared types
Files:
apps/web/app/api/notifications/route.tsapps/web/app/(org)/dashboard/_components/Notifications/NotificationItem.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/web/app/api/notifications/route.tsapps/web/app/(org)/dashboard/_components/Notifications/NotificationItem.tsx
apps/web/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
On the client, always use
useEffectQueryoruseEffectMutationfrom@/lib/EffectRuntime; never callEffectRuntime.run*directly in components.
Files:
apps/web/app/api/notifications/route.tsapps/web/app/(org)/dashboard/_components/Notifications/NotificationItem.tsx
apps/web/app/api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/app/api/**/*.{ts,tsx}: Prefer Server Actions for API surface; when routes are necessary, implement under app/api and export only the handler from apiToHandler(ApiLive)
Construct API routes with @effect/platform HttpApi/HttpApiBuilder, declare contracts with Schema, and only export the handler
Use HttpAuthMiddleware for required auth and provideOptionalAuth for guests; avoid duplicating session lookups
Map domain errors to transport with HttpApiError.* and keep translation exhaustive (catchTags/tapErrorCause)
Inside HttpApiBuilder.group, acquire services with Effect.gen and provide dependencies via Layer.provide instead of manual provideService
Files:
apps/web/app/api/notifications/route.ts
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/**/*.{ts,tsx}: Use TanStack Query v5 for all client-side server state and fetching in the web app
Mutations should call Server Actions directly and perform targeted cache updates with setQueryData/setQueriesData
Run server-side effects via the ManagedRuntime from apps/web/lib/server.ts using EffectRuntime.runPromise/runPromiseExit; do not create runtimes ad hoc
Client code should use helpers from apps/web/lib/EffectRuntime.ts (useEffectQuery, useEffectMutation, useRpcClient); never call ManagedRuntime.make inside components
Files:
apps/web/app/api/notifications/route.tsapps/web/app/(org)/dashboard/_components/Notifications/NotificationItem.tsx
apps/web/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Server components needing Effect services must call EffectRuntime.runPromise(effect.pipe(provideOptionalAuth))
Files:
apps/web/app/api/notifications/route.tsapps/web/app/(org)/dashboard/_components/Notifications/NotificationItem.tsx
🧬 Code graph analysis (2)
apps/web/app/api/notifications/route.ts (1)
apps/web/lib/server.ts (1)
runPromise(130-142)
apps/web/app/(org)/dashboard/_components/Notifications/NotificationItem.tsx (1)
apps/web/components/SignedImageUrl.tsx (1)
SignedImageUrl(11-25)
⏰ 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: Analyze (rust)
🔇 Additional comments (3)
apps/web/app/(org)/dashboard/_components/Notifications/NotificationItem.tsx (1)
8-8: LGTM! Clean refactor to use shared avatar component.The replacement of inline avatar rendering with the
SignedImageUrlcomponent is a good improvement that centralizes avatar logic. The component properly receives the resolved avatar URL from the backend and maintains the same styling and behavior.Also applies to: 49-54
apps/web/app/api/notifications/route.ts (2)
5-5: LGTM! Imports support the Effect-based refactor.The new imports are necessary for the asynchronous avatar resolution pipeline.
Also applies to: 7-7, 11-11
94-95: ImageUploads is already provided in the Dependencies layer — no action needed.The
ImageUploads.Defaultlayer is explicitly included in theDependencieslayer at line 114 inapps/web/lib/server.ts. TheManagedRuntimecreated at line 128 incorporates these dependencies, makingImageUploadsavailable to all effects executed viarunPromise. The code at line 95 correctly yields the service.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/web/app/api/notifications/route.ts (1)
121-125: Consider more granular error handling to preserve notifications.Currently, any error (avatar resolution failure or parse error) causes the entire notification to be dropped. Users might miss important notifications if avatar resolution fails. Consider handling avatar resolution errors separately to preserve the notification with a null avatar.
Apply this diff to handle avatar errors more gracefully:
Effect.gen(function* () { // all notifications currently require an author if (!author) return null; - const resolvedAvatar = author.avatar - ? yield* imageUploads.resolveImageUrl(author.avatar) - : null; + const resolvedAvatar = author.avatar + ? yield* imageUploads.resolveImageUrl(author.avatar).pipe( + Effect.catchAll((error) => { + console.error(`Failed to resolve avatar for user ${author.id}:`, error); + return Effect.succeed(null); + }) + ) + : null; return APINotification.parse({ id: notification.id, type: notification.type, readAt: notification.readAt, videoId: notification.data.videoId, createdAt: notification.createdAt, data: notification.data, comment: notification.data.comment, author: { id: author.id, name: author.name ?? "Unknown", avatar: resolvedAvatar, }, }); }).pipe( Effect.catchAll((error) => { - console.error("Invalid notification data:", error); + console.error("Failed to parse notification:", error); return Effect.succeed(null); }), ),This way, avatar resolution failures don't cause notification loss—only parse errors do.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/app/api/notifications/route.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.Use strict TypeScript and avoid any; leverage shared types
Files:
apps/web/app/api/notifications/route.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/web/app/api/notifications/route.ts
apps/web/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
On the client, always use
useEffectQueryoruseEffectMutationfrom@/lib/EffectRuntime; never callEffectRuntime.run*directly in components.
Files:
apps/web/app/api/notifications/route.ts
apps/web/app/api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/app/api/**/*.{ts,tsx}: Prefer Server Actions for API surface; when routes are necessary, implement under app/api and export only the handler from apiToHandler(ApiLive)
Construct API routes with @effect/platform HttpApi/HttpApiBuilder, declare contracts with Schema, and only export the handler
Use HttpAuthMiddleware for required auth and provideOptionalAuth for guests; avoid duplicating session lookups
Map domain errors to transport with HttpApiError.* and keep translation exhaustive (catchTags/tapErrorCause)
Inside HttpApiBuilder.group, acquire services with Effect.gen and provide dependencies via Layer.provide instead of manual provideService
Files:
apps/web/app/api/notifications/route.ts
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/**/*.{ts,tsx}: Use TanStack Query v5 for all client-side server state and fetching in the web app
Mutations should call Server Actions directly and perform targeted cache updates with setQueryData/setQueriesData
Run server-side effects via the ManagedRuntime from apps/web/lib/server.ts using EffectRuntime.runPromise/runPromiseExit; do not create runtimes ad hoc
Client code should use helpers from apps/web/lib/EffectRuntime.ts (useEffectQuery, useEffectMutation, useRpcClient); never call ManagedRuntime.make inside components
Files:
apps/web/app/api/notifications/route.ts
apps/web/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Server components needing Effect services must call EffectRuntime.runPromise(effect.pipe(provideOptionalAuth))
Files:
apps/web/app/api/notifications/route.ts
🧬 Code graph analysis (1)
apps/web/app/api/notifications/route.ts (1)
apps/web/lib/server.ts (1)
runPromise(130-142)
⏰ 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). (4)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Typecheck
- GitHub Check: Analyze (rust)
🔇 Additional comments (1)
apps/web/app/api/notifications/route.ts (1)
5-5: LGTM: Imports align with Effect-based architecture.The new imports support the Effect-based refactoring and follow the coding guidelines for using Effect and the
runPromisehelper from@/lib/server.Also applies to: 7-7, 11-11
Summary by CodeRabbit
Improvements
Refactor