built a newsletter section #128
Conversation
|
@Shivam107 is attempting to deploy a commit to the AJEET PRATAP SINGH's projects Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughAdds an end-to-end newsletter feature: new API router and service (list, bySlug), frontend pages/components/hooks with mock data, media rendering via react-player, Next image pattern and TypeScript declaration updates, plus layout and styling tweaks. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant IndexPage as Newsletter Index
participant DetailPage as Newsletter Detail (server)
participant Client as NewsletterDetailClient
participant Hooks as Client Hooks
participant TRPC as tRPC Client
participant API as Newsletter Router
participant Service as Newsletter Service
participant DB as Prisma
User->>IndexPage: open /dashboard/newsletters
IndexPage->>Hooks: useNewsletterList(search?)
Hooks-->>IndexPage: return mock data or live result
User->>DetailPage: open /dashboard/newsletters/[slug]
DetailPage->>Client: render with slug + fallback
Client->>Hooks: useNewsletterDetail(slug)
Hooks->>TRPC: newsletter.bySlug.query({slug})
TRPC->>API: bySlug({slug})
API->>Service: bySlug(slug)
Service->>DB: fetch issue + sections
DB-->>Service: issue data
Service-->>API: issue
API-->>TRPC: issue
TRPC-->>Hooks: issue
Hooks-->>Client: issue data
Client->>Client: render Hero + Sections (lazy ReactPlayer)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Potential attention points:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 12
🧹 Nitpick comments (19)
apps/web/src/app/(main)/dashboard/newsletters/engagementBar.tsx (1)
15-39: Good implementation with proper fallback handling.The share functionality correctly tries the native Share API before falling back to clipboard copy. The use of
window.location.originensures the shared URL points to the current deployment.Consider showing user feedback when sharing/copying fails (lines 27, 37), not just logging to console. A toast notification or error message would improve the user experience.
apps/web/src/types/next-auth.d.ts (1)
14-18: Module declaration works but consider file organization.The type declaration for
react-player/lazyis technically correct and will resolve TypeScript errors. However, this file is namednext-auth.d.tsbut now contains declarations unrelated to NextAuth.Consider moving react-player declarations to a separate type definition file (e.g.,
react-player.d.ts) for better code organization.apps/web/src/components/newsletter/NewsletterHero.tsx (1)
13-22: Consider explicit locale for date formatting.The date formatting uses
undefinedas the locale, which defaults to the user's browser locale. This is acceptable but could lead to inconsistent formatting across users. Consider using a specific locale (e.g.,"en-US") or making it configurable based on user preferences.apps/api/src/routers/newsletter.ts (1)
5-13: API router implementation is correct.The newsletter router properly defines two procedures with appropriate input validation and service integration.
The double
.optional()on line 7 (z.object({ search: z.string().optional() }).optional()) is valid but could be simplified toz.object({ search: z.string().optional() })if you always want to require passing an object (even if empty). The current pattern allows calling without any input at all, which works but is unconventional.If you prefer to simplify:
list: publicProcedure - .input(z.object({ search: z.string().optional() }).optional()) + .input(z.object({ search: z.string().optional() })) .query(async ({ input }) => newsletterService.list(input?.search)),Then change the usage to
input.searchinstead ofinput?.searchsince input will always be an object.apps/web/src/components/newsletter/NewsletterCard.tsx (3)
52-53: Fix useCallback deps (remove unused).Only sharePath is used; extra deps cause needless re-renders.
- [issue.summary, issue.title, sharePath] + [sharePath]
86-115: Remove unused MediaPreview or render it.Dead code hurts maintainability. If you don’t plan to show media here, delete it; otherwise, wire it into the card.
Option A — remove:
- function MediaPreview({ issue }: NewsletterCardProps) { - if (!issue.heroMediaUrl) { - return null; - } - if (issue.heroMediaType === "video") { - return ( - <div className="relative w-full overflow-hidden rounded-xl border border-ox-gray md:w-56"> - <video - src={issue.heroMediaUrl} - controls - preload="metadata" - className="h-full w-full rounded-xl" - /> - </div> - ); - } - return ( - <div className="relative h-36 w-full overflow-hidden rounded-xl border border-ox-gray md:h-40 md:w-56"> - <Image - src={issue.heroMediaUrl} - alt={issue.title} - fill - sizes="(min-width: 768px) 224px, 100vw" - className="object-cover" - /> - </div> - ); - }Option B — render next to meta (example):
- <div className="flex items-center justify-between p-5"> + <div className="flex items-center justify-between p-5 gap-4"> + {/* Preview (optional) */} + {/* <MediaPreview issue={issue} /> */}
70-80: A11y + dedupe: add aria-label and consider reusing EngagementBar.Improves accessibility and avoids duplicate share logic already implemented in EngagementBar.
- <button + <button onClick={(e) => { e.preventDefault(); handleShare(e); }} className="ml-4 flex items-center justify-center px-4 py-2 hover:bg-ox-black-2 rounded-lg transition" + aria-label={`Share ${issue.title}`} >Also consider replacing this button with for consistency.
apps/web/src/data/newsletters.ts (4)
23-51: Populate tags in mock data (minimal).Ensures filtering doesn’t crash. Adjust as needed.
publishedAt: "2025-09-28", readTime: "6 min read", heroMediaUrl: "/images/Screenshot-2025-10-22.png", heroMediaType: "image", + tags: ["premium", "week-1", "automation"],
52-83: Populate tags for week 2.publishedAt: "2025-10-05", readTime: "7 min read", heroMediaUrl: "/images/Screenshot-2025-10-22.png", heroMediaType: "image", + tags: ["premium", "week-2", "community"],
84-114: Populate tags for week 3.publishedAt: "2025-10-12", readTime: "8 min read", heroMediaUrl: "https://assets.opensox.dev/newsletters/122-week3-hero.mp4", heroMediaType: "video", + tags: ["premium", "week-3", "beta"],
115-147: Populate tags for week 4.publishedAt: "2025-10-19", readTime: "5 min read", heroMediaUrl: "https://assets.opensox.dev/newsletters/122-week4-hero.jpg", heroMediaType: "image", + tags: ["premium", "week-4", "events"],apps/api/src/services/newsletter.service.ts (2)
7-16: Normalize tag search tokens (and store tags lowercased).hasSome is case-sensitive; normalize tokens to reduce misses. Ensure DB tags are stored lowercased.
- const where: Prisma.NewsletterIssueWhereInput | undefined = search + const where: Prisma.NewsletterIssueWhereInput | undefined = search ? { OR: [ { title: { contains: search, mode: "insensitive" } }, { summary: { contains: search, mode: "insensitive" } }, - { tags: { hasSome: search.split(" ").filter(Boolean) } }, + { tags: { hasSome: search.split(" ").filter(Boolean).map(t => t.toLowerCase()) } }, ], } : undefined;
18-33: Add basic pagination to prevent large scans.Default to a reasonable limit; expose take/skip via router later.
return prisma.newsletterIssue.findMany({ ...(where && { where }), orderBy: { publishedAt: "desc" }, + take: 50, select: {Consider indexes: publishedAt (DESC), GIN on tags for array ops, trigram/GIN for title/summary if needed.
apps/web/src/app/(main)/dashboard/newsletters/[slug]/NewsletterDetailClient.tsx (1)
8-15: Remove unused import; keep only lazy import.Avoid dead imports and bundle bloat.
-import { NewsletterSectionRenderer } from "@/components/newsletter/NewsletterSectionRenderer";apps/web/src/app/(main)/dashboard/newsletters/page.tsx (2)
54-66: Always render the search input.Improves discoverability and consistent layout.
- {query && ( + {( <div className="mt-4 flex w-full max-w-lg items-center rounded-full border border-ox-gray bg-ox-black-2 px-4 py-2"> <input type="search" value={query} onChange={(e) => setQuery(e.target.value)} placeholder="Search newsletters" disabled={isLoading} className="w-full bg-transparent text-sm text-ox-white placeholder:text-ox-gray-light focus:outline-none" /> </div> - )} + )}
52-67: Consider reusing NewsletterHeader for nav/search consistency.Keeps UX consistent with detail pages and centralizes header logic.
apps/web/src/components/newsletter/NewsletterSectionRenderer.tsx (1)
7-14: Type ReactPlayer instead of casting to any.Improves safety; optional.
-const ReactPlayer = dynamic( - () => import("react-player").then((mod) => mod.default), - { - ssr: false, - loading: () => null, - } -) as any; +const ReactPlayer = dynamic(() => import("react-player"), { + ssr: false, + loading: () => null, +}) as unknown as typeof import("react-player")["default"];apps/web/src/components/newsletter/NewsletterHeader.tsx (2)
52-61: Add aria-current for active nav item.Improves screen reader feedback.
<Link key={link.href} href={link.href} target={link.external ? "_blank" : undefined} rel={link.external ? "noopener noreferrer" : undefined} className={cn( "rounded-full border border-ox-gray px-4 py-2 text-xs font-semibold uppercase tracking-wide transition hover:border-ox-purple hover:text-ox-purple md:text-sm", isActive ? "bg-ox-purple text-ox-white border-ox-purple" : "text-ox-gray-light" )} + aria-current={isActive ? "page" : undefined} >
81-85: Make chips configurable via props.Avoid hard-coded “Issue #122” and months; pass via props if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
apps/web/public/images/Screenshot-2025-10-22.pngis excluded by!**/*.pngapps/web/public/images/welcome.jpgis excluded by!**/*.jpg
📒 Files selected for processing (22)
apps/api/src/routers/_app.ts(2 hunks)apps/api/src/routers/newsletter.ts(1 hunks)apps/api/src/services/newsletter.service.ts(1 hunks)apps/web/next.config.js(1 hunks)apps/web/package.json(1 hunks)apps/web/src/app/(main)/dashboard/layout.tsx(1 hunks)apps/web/src/app/(main)/dashboard/newsletters/[slug]/NewsletterDetailClient.tsx(1 hunks)apps/web/src/app/(main)/dashboard/newsletters/[slug]/page.tsx(1 hunks)apps/web/src/app/(main)/dashboard/newsletters/engagementBar.tsx(1 hunks)apps/web/src/app/(main)/dashboard/newsletters/page.tsx(1 hunks)apps/web/src/app/(main)/dashboard/sidebar.tsx(1 hunks)apps/web/src/app/SessionWrapper.tsx(1 hunks)apps/web/src/app/api/newsletter.ts(1 hunks)apps/web/src/app/globals.css(1 hunks)apps/web/src/components/dashboard/Sidebar.tsx(1 hunks)apps/web/src/components/newsletter/NewsletterCard.tsx(1 hunks)apps/web/src/components/newsletter/NewsletterHeader.tsx(1 hunks)apps/web/src/components/newsletter/NewsletterHero.tsx(1 hunks)apps/web/src/components/newsletter/NewsletterSectionRenderer.tsx(1 hunks)apps/web/src/data/newsletters.ts(1 hunks)apps/web/src/types/next-auth.d.ts(1 hunks)apps/web/tailwind.config.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (12)
apps/web/src/app/SessionWrapper.tsx (2)
apps/web/src/components/providers/SessionProvider.tsx (1)
SessionProvider(11-13)apps/web/src/lib/auth/config.ts (1)
session(36-42)
apps/api/src/routers/newsletter.ts (2)
apps/api/src/trpc.ts (2)
router(35-35)publicProcedure(36-36)apps/api/src/services/newsletter.service.ts (1)
newsletterService(6-45)
apps/web/src/app/(main)/dashboard/newsletters/page.tsx (2)
apps/web/src/data/newsletters.ts (1)
NewsletterIssue(12-21)apps/web/src/app/api/newsletter.ts (1)
useNewsletterList(8-23)
apps/api/src/routers/_app.ts (1)
apps/api/src/routers/newsletter.ts (1)
newsletterRouter(5-13)
apps/web/src/components/newsletter/NewsletterHero.tsx (1)
apps/web/src/data/newsletters.ts (1)
NewsletterIssue(12-21)
apps/web/src/app/(main)/dashboard/layout.tsx (2)
apps/web/src/components/ui/FiltersContainer.tsx (1)
FiltersContainer(21-144)apps/web/src/components/dashboard/Sidebar.tsx (1)
Sidebar(31-117)
apps/web/src/components/newsletter/NewsletterHeader.tsx (1)
apps/web/src/lib/utils.ts (1)
cn(4-6)
apps/web/src/app/api/newsletter.ts (2)
apps/web/src/data/newsletters.ts (1)
newsletterIssues(23-147)apps/web/src/lib/trpc.ts (1)
trpc(4-4)
apps/web/src/app/(main)/dashboard/newsletters/[slug]/NewsletterDetailClient.tsx (4)
apps/web/src/data/newsletters.ts (1)
NewsletterIssue(12-21)apps/web/src/app/api/newsletter.ts (1)
useNewsletterDetail(25-27)apps/web/src/components/newsletter/NewsletterHero.tsx (1)
NewsletterHero(12-39)apps/web/src/app/(main)/dashboard/newsletters/engagementBar.tsx (1)
EngagementBar(11-49)
apps/web/src/components/newsletter/NewsletterCard.tsx (1)
apps/web/src/data/newsletters.ts (1)
NewsletterIssue(12-21)
apps/web/src/components/newsletter/NewsletterSectionRenderer.tsx (1)
apps/web/src/data/newsletters.ts (1)
NewsletterSection(1-10)
apps/web/src/app/(main)/dashboard/newsletters/[slug]/page.tsx (2)
apps/web/src/data/newsletters.ts (1)
newsletterIssues(23-147)apps/web/src/app/(main)/dashboard/newsletters/[slug]/NewsletterDetailClient.tsx (1)
NewsletterDetailClient(65-82)
🔇 Additional comments (12)
apps/web/src/app/SessionWrapper.tsx (1)
14-16: LGTM!The JSX formatting change improves readability while maintaining the component's functionality. The SessionProvider wrapper and session/children props handling remain unchanged.
apps/web/src/app/globals.css (1)
74-77: Good practice for layout stability.Adding
scrollbar-gutter: stableprevents layout shift when scrollbar appears/disappears, which is beneficial for the dynamic content in the newsletter and dashboard sections.Please verify that your project's target browsers support this property.
scrollbar-gutterhas solid support in modern browsers (Chrome 101+, Firefox, Edge) but may have limited support in older Safari versions. The change degrades gracefully in unsupported browsers, so if you're not targeting very old browsers, this is fine.Would you like me to check the current browser support for
scrollbar-gutter?apps/web/src/app/(main)/dashboard/layout.tsx (1)
20-27: LGTM! Scroll handling implemented correctly.The overflow and flex adjustments create a proper dashboard layout with independent scroll regions for the sidebar and main content while preventing the outer container from scrolling.
apps/web/src/components/dashboard/Sidebar.tsx (2)
11-24: Note: Duplicate route definition.This
SIDEBAR_ROUTESconstant duplicates the definition inapps/web/src/app/(main)/dashboard/sidebar.tsx(which is unused). See the comment on that file for recommendations to eliminate this duplication.
26-29: LGTM! Improved active route detection for nested paths.Switching from strict equality to
startsWithcorrectly handles nested routes. For example, when navigating to/dashboard/projects/123, the "Find Projects" link will remain active. This is the appropriate pattern for route matching in the dashboard navigation.apps/web/tailwind.config.ts (1)
57-57: No issues found. The previous value was invalid Tailwind syntax.The original review comment assumes changing
ox-whitefrom gray (text-slate-400) to white breaks existing components. However,"text-slate-400"is invalid Tailwind color syntax—it's a utility class name, not a valid color value. The change to"#ffffff"fixes this invalid configuration.All 11 usages of
ox-whitethroughout the codebase are part of the newsletter feature being added in this PR and are designed for white text on dark backgrounds (e.g.,bg-ox-black-1,bg-ox-black-2). No legacy code depends on the previous broken value, so there is no breaking change.Likely an incorrect or invalid review comment.
apps/web/next.config.js (1)
9-12: LGTM! Remote image pattern added correctly.The new remote pattern for
assets.opensox.devis properly configured and aligns with Next.js 15 image optimization requirements for loading external media assets used in the newsletter feature.apps/api/src/routers/_app.ts (2)
7-7: LGTM! Newsletter router properly imported.
23-23: LGTM! Newsletter router correctly registered.The newsletter router is properly integrated into the app router, making the newsletter API endpoints available.
apps/web/package.json (1)
35-35: Version is current and secure—no action needed.Verification confirms that react-player@3.3.3 is the latest available version on npm with no known security vulnerabilities in the GitHub advisory database.
apps/api/src/services/newsletter.service.ts (1)
35-44: Good: bySlug includes ordered sections.apps/web/src/components/newsletter/NewsletterSectionRenderer.tsx (1)
41-47: No action required—assets.opensox.dev is already configured in the Next.js image remote patterns allowlist.The verification confirms that
assets.opensox.devis explicitly listed inapps/web/next.config.jsunderremotePatternswith the https protocol, permitting the Image component to load remote images from this domain.
apps/web/src/app/(main)/dashboard/newsletters/[slug]/NewsletterDetailClient.tsx
Show resolved
Hide resolved
Shivam107
force-pushed
the
fix/newsletter
branch
from
c34962c to
9ab7036
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
apps/web/src/app/(main)/dashboard/newsletters/[slug]/page.tsx (1)
5-10: Fix PageProps: params should be a synchronous object, not a Promise.Per the previous review, Next.js 15 page props receive
paramsas a synchronous object, not a Promise. The async Request APIs (params(),searchParams()functions) are different from the props shape.Apply this diff:
interface PageProps { - params: Promise<{ slug: string }>; + params: { slug: string }; } export default async function NewsletterDetail({ params }: PageProps) { - const { slug } = await params; + const { slug } = params; const fallback = newsletterIssues.find((issue) => issue.slug === slug);Based on Next.js 15 docs.
🧹 Nitpick comments (5)
apps/web/next.config.js (1)
9-12: Domain ownership verified through active usage; optional pathname restriction recommended.The domain
assets.opensox.devis confirmed to be team-controlled, as evidenced by active references throughoutapps/web/src/data/newsletters.tsfor newsletter media assets. The configuration is syntactically correct and necessary for serving newsletter images and videos.The current pattern is sufficiently broad for the newsletter feature. However, as an optional security best practice, consider restricting to the
/newsletters/**pathname to limit exposure to only the intended asset directory:{ protocol: "https", hostname: "assets.opensox.dev", + pathname: "/newsletters/**", },apps/api/src/routers/newsletter.ts (1)
11-11: Consider adding slug format validation.The current validation accepts any string as a slug, but slugs typically follow a specific format (e.g., lowercase alphanumeric with hyphens). Adding format validation would catch invalid inputs earlier and provide better error messages to clients.
Apply this diff to add slug format validation:
- .input(z.object({ slug: z.string() })) + .input(z.object({ + slug: z.string().regex(/^[a-z0-9]+(?:-[a-z0-9]+)*$/, { + error: () => "Slug must be lowercase alphanumeric with hyphens (kebab-case)" + }) + }))Note: Adjust the regex pattern if your slug format differs.
apps/web/src/components/newsletter/NewsletterHeader.tsx (1)
8-12: Label improved but could be more specific.The previous "GitHub" label has been updated to "Community", which resolves the mismatch. However, "Discord" would be more specific and immediately clear to users where the link leads.
Apply this diff for maximum clarity:
- { href: "https://discord.gg/nC9x4hef", label: "Community", external: true }, + { href: "https://discord.gg/nC9x4hef", label: "Discord", external: true },apps/web/src/data/newsletters.ts (1)
1-10: Consider discriminated union for NewsletterSection to enforce required fields per type.The current design allows all media-related fields to be optional, but depending on
type, certain fields should be required (e.g., "image" requiressrcandalt, "video" requiressrc, "media" requires bothsrcandmediaSrc). This could permit invalid data at compile time.Refactor to a discriminated union:
-export type NewsletterSection = { - type: "text" | "image" | "video" | "media"; - heading: string; - body: string; - src?: string; - alt?: string; - mediaSrc?: string; - mediaType?: "image" | "video"; - mediaAlt?: string; -}; +export type NewsletterSection = + | { + type: "text"; + heading: string; + body: string; + } + | { + type: "image"; + heading: string; + body: string; + src: string; + alt: string; + } + | { + type: "video"; + heading: string; + body: string; + src: string; + } + | { + type: "media"; + heading: string; + body: string; + src: string; + alt: string; + mediaSrc: string; + mediaType: "image" | "video"; + mediaAlt?: string; + };apps/web/src/app/(main)/dashboard/newsletters/page.tsx (1)
33-33: Type assertion suggests missing type definitions.The manual type assertion indicates
useNewsletterListmay lack proper return type definitions. Consider adding explicit return types to the hook.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
apps/web/public/images/Screenshot-2025-10-22.pngis excluded by!**/*.pngapps/web/public/images/welcome.jpgis excluded by!**/*.jpg
📒 Files selected for processing (21)
apps/api/src/routers/_app.ts(2 hunks)apps/api/src/routers/newsletter.ts(1 hunks)apps/api/src/services/newsletter.service.ts(1 hunks)apps/web/next.config.js(1 hunks)apps/web/package.json(1 hunks)apps/web/src/app/(main)/dashboard/layout.tsx(1 hunks)apps/web/src/app/(main)/dashboard/newsletters/[slug]/NewsletterDetailClient.tsx(1 hunks)apps/web/src/app/(main)/dashboard/newsletters/[slug]/page.tsx(1 hunks)apps/web/src/app/(main)/dashboard/newsletters/engagementBar.tsx(1 hunks)apps/web/src/app/(main)/dashboard/newsletters/page.tsx(1 hunks)apps/web/src/app/SessionWrapper.tsx(1 hunks)apps/web/src/app/api/newsletter.ts(1 hunks)apps/web/src/app/globals.css(1 hunks)apps/web/src/components/dashboard/Sidebar.tsx(1 hunks)apps/web/src/components/newsletter/NewsletterCard.tsx(1 hunks)apps/web/src/components/newsletter/NewsletterHeader.tsx(1 hunks)apps/web/src/components/newsletter/NewsletterHero.tsx(1 hunks)apps/web/src/components/newsletter/NewsletterSectionRenderer.tsx(1 hunks)apps/web/src/data/newsletters.ts(1 hunks)apps/web/src/types/next-auth.d.ts(1 hunks)apps/web/tailwind.config.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/web/src/app/SessionWrapper.tsx
🚧 Files skipped from review as they are similar to previous changes (13)
- apps/web/src/app/globals.css
- apps/web/src/app/(main)/dashboard/newsletters/engagementBar.tsx
- apps/web/src/app/api/newsletter.ts
- apps/web/src/components/newsletter/NewsletterCard.tsx
- apps/web/src/app/(main)/dashboard/newsletters/[slug]/NewsletterDetailClient.tsx
- apps/web/src/app/(main)/dashboard/layout.tsx
- apps/api/src/routers/_app.ts
- apps/web/tailwind.config.ts
- apps/web/src/components/newsletter/NewsletterSectionRenderer.tsx
- apps/api/src/services/newsletter.service.ts
- apps/web/src/types/next-auth.d.ts
- apps/web/src/components/newsletter/NewsletterHero.tsx
- apps/web/src/components/dashboard/Sidebar.tsx
🧰 Additional context used
🧬 Code graph analysis (4)
apps/web/src/app/(main)/dashboard/newsletters/[slug]/page.tsx (2)
apps/web/src/data/newsletters.ts (1)
newsletterIssues(23-147)apps/web/src/app/(main)/dashboard/newsletters/[slug]/NewsletterDetailClient.tsx (1)
NewsletterDetailClient(65-82)
apps/web/src/app/(main)/dashboard/newsletters/page.tsx (2)
apps/web/src/data/newsletters.ts (1)
NewsletterIssue(12-21)apps/web/src/app/api/newsletter.ts (1)
useNewsletterList(6-21)
apps/web/src/components/newsletter/NewsletterHeader.tsx (1)
apps/web/src/lib/utils.ts (1)
cn(4-6)
apps/api/src/routers/newsletter.ts (2)
apps/api/src/trpc.ts (2)
router(35-35)publicProcedure(36-36)apps/api/src/services/newsletter.service.ts (1)
newsletterService(6-45)
🔇 Additional comments (10)
apps/web/package.json (1)
35-35: Version 3.3.3 is the latest stable release and no security vulnerabilities are reported for react-player v3.3.3. The dependency is correctly placed, properly formatted, and uses an appropriate version specifier.apps/api/src/routers/newsletter.ts (1)
1-3: LGTM!The imports are properly structured and follow ES module conventions.
apps/web/src/app/(main)/dashboard/newsletters/[slug]/page.tsx (1)
11-17: LGTM!The fallback lookup and notFound() flow is correct, and delegating to NewsletterDetailClient is appropriate.
apps/web/src/data/newsletters.ts (2)
12-21: LGTM!The NewsletterIssue type is well-structured and covers all necessary fields.
23-147: LGTM!The newsletter data is well-formed and provides good variety for testing the UI (text, image, video, and media sections).
apps/web/src/app/(main)/dashboard/newsletters/page.tsx (5)
7-11: LGTM!Lazy loading NewsletterCard with code splitting is a good performance optimization.
13-29: LGTM!CardSkeleton and NewsletterCardList components are well-structured with appropriate Suspense boundaries.
39-48: LGTM!The filtering logic correctly searches by title and summary with proper memoization.
67-75: LGTM!Conditional rendering for loading, results, and empty states is well-structured.
80-103: LGTM!SkeletonList and EmptyState components provide good user feedback for loading and empty states.
Shivam107
force-pushed
the
fix/newsletter
branch
from
9ab7036 to
b6e4500
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
apps/web/src/components/newsletter/NewsletterHeader.tsx (3)
14-21: Clarifydisabledapplies only when search is enabled.Document or model the dependency so consumers aren’t confused.
+/** + * Header for newsletters. + * Note: `disabled` only applies when `onSearch` is provided. + */ type Props = {
70-77: Add an accessible label to the search input.Placeholders aren’t labels; add aria-label for screen readers.
<input type="search" value={searchTerm} onChange={handleSearchChange} placeholder={searchPlaceholder} disabled={disabled} + aria-label={searchPlaceholder} className="w-full bg-transparent text-sm text-ox-white placeholder:text-ox-gray-light focus:outline-none" />
79-83: Remove placeholder chips or make them dynamic.Hardcoded chips are misleading and non-interactive. Easiest fix: remove them for now.
- <div className="flex flex-wrap gap-2 text-xs text-ox-gray-light"> - <span className="rounded-full border border-ox-gray px-3 py-1">Premium</span> - <span className="rounded-full border border-ox-gray px-3 py-1">Issue #122</span> - <span className="rounded-full border border-ox-gray px-3 py-1">September · October</span> - </div>apps/api/src/routers/newsletter.ts (2)
6-8: Remove double-optional input onlist.Require the object; keep
searchoptional. Cleaner client API.- list: publicProcedure - .input(z.object({ search: z.string().optional() }).optional()) - .query(async ({ input }) => newsletterService.list(input?.search)), + list: publicProcedure + .input(z.object({ search: z.string().optional() })) + .query(async ({ input }) => newsletterService.list(input.search)),
10-13: Return 404-style error when slug not found.Throw TRPCError NOT_FOUND instead of returning null.
+import { TRPCError } from "@trpc/server"; ... bySlug: publicProcedure .input(z.object({ slug: z.string() })) - .query(async ({ input }) => newsletterService.bySlug(input.slug)), + .query(async ({ input }) => { + const newsletter = await newsletterService.bySlug(input.slug); + if (!newsletter) { + throw new TRPCError({ + code: "NOT_FOUND", + message: `Newsletter with slug "${input.slug}" not found`, + }); + } + return newsletter; + }),apps/web/src/app/(main)/dashboard/newsletters/page.tsx (1)
54-65: Search input never renders; show it unconditionally and add a label.
{query && (...)}hides the input when empty. Always render so users can type.- {query && ( - <div className="mt-4 flex w-full max-w-lg items-center rounded-full border border-ox-gray bg-ox-black-2 px-4 py-2"> + <div className="mt-4 flex w-full max-w-lg items-center rounded-full border border-ox-gray bg-ox-black-2 px-4 py-2" role="search"> <input type="search" value={query} onChange={(e) => setQuery(e.target.value)} placeholder="Search newsletters" disabled={isLoading} + aria-label="Search newsletters" className="w-full bg-transparent text-sm text-ox-white placeholder:text-ox-gray-light focus:outline-none" /> </div> - )}
🧹 Nitpick comments (5)
apps/web/src/components/newsletter/NewsletterHeader.tsx (1)
47-61: Expose active state to assistive tech.Add aria-current="page" on the active internal link.
- <Link + <Link key={link.href} href={link.href} target={link.external ? "_blank" : undefined} rel={link.external ? "noopener noreferrer" : undefined} + aria-current={isActive ? "page" : undefined} className={cn(apps/web/src/app/(main)/dashboard/layout.tsx (1)
20-28: Enable reliable vertical scrolling in flex layout.With nested flex + overflow-y-auto, parents need min-h-0, and the row should have a fixed height to make child scrollbars work.
- <div className="flex flex-row w-full overflow-hidden"> + <div className="flex flex-row w-full overflow-hidden min-h-0 h-[calc(100vh-4rem)]"> {showFilters && <FiltersContainer></FiltersContainer>} <aside - className={`flex-shrink-0 w-48 md:w-[40%] xl:w-[20%] ${showSidebar ? "block relative" : "hidden"} xl:block overflow-y-auto`} + className={`flex-shrink-0 w-48 md:w-[40%] xl:w-[20%] ${showSidebar ? "block relative" : "hidden"} xl:block overflow-y-auto min-h-0`} > <Sidebar></Sidebar> </aside> - <main className="flex-grow overflow-y-auto">{children}</main> + <main className="flex-grow overflow-y-auto min-h-0">{children}</main> </div>Please verify on mobile: content should scroll, sidebar independently scrolls, no page lock.
apps/web/src/components/dashboard/Sidebar.tsx (1)
76-86: Improve UX/accessibility on nav clicks.
- Add aria-current for the active link.
- Auto-close the sidebar after navigating on mobile.
- <Link + <Link href={route.path} - className={getSidebarLinkClassName(pathname, route.path)} + className={getSidebarLinkClassName(pathname, route.path)} + aria-current={pathname.startsWith(route.path) ? "page" : undefined} + onClick={() => setShowSidebar(false)} key={route.path} >apps/web/src/app/api/newsletter.ts (1)
6-21: Unify data source and expose real loading/error states.Currently the list uses mock data with isLoading=false while detail uses TRPC. This is inconsistent and causes double filtering in the page.
Option A (recommended): switch list to TRPC and let the server filter.
-import { newsletterIssues } from "@/data/newsletters"; import { trpc } from "@/lib/trpc"; export const useNewsletterList = (search?: string) => { - // Use mock data immediately for instant load - const filteredData = search - ? newsletterIssues.filter((issue) => - `${issue.title} ${issue.summary}` - .toLowerCase() - .includes(search.toLowerCase()) - ) - : newsletterIssues; - - return { - data: filteredData, - isLoading: false, - error: null, - }; + const query = trpc.newsletter.list.useQuery( + { search }, + { keepPreviousData: true, staleTime: 30_000 } + ); + return query; };Option B: keep mock data, but rename to
getNewsletterListand removeisLoading/errorto avoid implying async behavior. Update callers accordingly.apps/web/src/app/(main)/dashboard/newsletters/page.tsx (1)
33-37: Avoid double filtering and remove assertion cast.Let this page handle filtering; fetch the list once without passing
query, and don’t assert types.- const { data, isLoading } = useNewsletterList(query) as { - data: NewsletterIssue[] | undefined; - isLoading: boolean; - }; - const issues = data?.length ? data : mockIssues; + const { data, isLoading } = useNewsletterList(); + const issues = (data && data.length > 0 ? data : mockIssues) as NewsletterIssue[]; @@ - const filteredIssues = useMemo<NewsletterIssue[]>(() => { + const filteredIssues = useMemo<NewsletterIssue[]>(() => { const normalized = query.trim().toLowerCase(); if (!normalized) return issues as NewsletterIssue[]; return (issues as NewsletterIssue[]).filter((issue) => `${issue.title} ${issue.summary}` .toLowerCase() .includes(normalized) ); }, [issues, query]); @@ - {isLoading ? ( + {isLoading ? ( <SkeletonList /> ) : filteredIssues.length > 0 ? ( <NewsletterCardList issues={filteredIssues} /> ) : ( <EmptyState query={query} /> )}Also applies to: 39-48, 67-75
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
apps/web/public/images/Screenshot-2025-10-22.pngis excluded by!**/*.pngapps/web/public/images/welcome.jpgis excluded by!**/*.jpg
📒 Files selected for processing (21)
apps/api/src/routers/_app.ts(2 hunks)apps/api/src/routers/newsletter.ts(1 hunks)apps/api/src/services/newsletter.service.ts(1 hunks)apps/web/next.config.js(1 hunks)apps/web/package.json(1 hunks)apps/web/src/app/(main)/dashboard/layout.tsx(1 hunks)apps/web/src/app/(main)/dashboard/newsletters/[slug]/NewsletterDetailClient.tsx(1 hunks)apps/web/src/app/(main)/dashboard/newsletters/[slug]/page.tsx(1 hunks)apps/web/src/app/(main)/dashboard/newsletters/engagementBar.tsx(1 hunks)apps/web/src/app/(main)/dashboard/newsletters/page.tsx(1 hunks)apps/web/src/app/SessionWrapper.tsx(1 hunks)apps/web/src/app/api/newsletter.ts(1 hunks)apps/web/src/app/globals.css(1 hunks)apps/web/src/components/dashboard/Sidebar.tsx(1 hunks)apps/web/src/components/newsletter/NewsletterCard.tsx(1 hunks)apps/web/src/components/newsletter/NewsletterHeader.tsx(1 hunks)apps/web/src/components/newsletter/NewsletterHero.tsx(1 hunks)apps/web/src/components/newsletter/NewsletterSectionRenderer.tsx(1 hunks)apps/web/src/data/newsletters.ts(1 hunks)apps/web/src/types/next-auth.d.ts(1 hunks)apps/web/tailwind.config.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/web/src/app/SessionWrapper.tsx
🚧 Files skipped from review as they are similar to previous changes (12)
- apps/web/src/components/newsletter/NewsletterHero.tsx
- apps/web/src/app/(main)/dashboard/newsletters/[slug]/page.tsx
- apps/web/package.json
- apps/web/src/app/(main)/dashboard/newsletters/[slug]/NewsletterDetailClient.tsx
- apps/api/src/services/newsletter.service.ts
- apps/web/src/data/newsletters.ts
- apps/web/src/components/newsletter/NewsletterCard.tsx
- apps/web/src/components/newsletter/NewsletterSectionRenderer.tsx
- apps/web/src/app/(main)/dashboard/newsletters/engagementBar.tsx
- apps/web/next.config.js
- apps/web/src/app/globals.css
- apps/web/src/types/next-auth.d.ts
🧰 Additional context used
🧬 Code graph analysis (6)
apps/web/src/app/api/newsletter.ts (2)
apps/web/src/data/newsletters.ts (1)
newsletterIssues(23-147)apps/web/src/lib/trpc.ts (1)
trpc(4-4)
apps/api/src/routers/newsletter.ts (2)
apps/api/src/trpc.ts (2)
router(35-35)publicProcedure(36-36)apps/api/src/services/newsletter.service.ts (1)
newsletterService(6-45)
apps/web/src/app/(main)/dashboard/newsletters/page.tsx (2)
apps/web/src/data/newsletters.ts (1)
NewsletterIssue(12-21)apps/web/src/app/api/newsletter.ts (1)
useNewsletterList(6-21)
apps/web/src/components/newsletter/NewsletterHeader.tsx (1)
apps/web/src/lib/utils.ts (1)
cn(4-6)
apps/api/src/routers/_app.ts (1)
apps/api/src/routers/newsletter.ts (1)
newsletterRouter(5-13)
apps/web/src/app/(main)/dashboard/layout.tsx (2)
apps/web/src/components/ui/FiltersContainer.tsx (1)
FiltersContainer(21-144)apps/web/src/components/dashboard/Sidebar.tsx (1)
Sidebar(31-117)
🔇 Additional comments (3)
apps/web/tailwind.config.ts (1)
57-57: No action required—the color change is correct and appropriate.The change from
"text-slate-400"to"#ffffff"is not a breaking change. The previous value was invalid (it's a Tailwind utility class name, not a valid color value), and the new value correctly implements the semantic meaning of the token name "ox-white."All 24+ usages across the codebase apply the token as text color (
text-ox-white), primarily in new newsletter components and existing dashboard components. The white color on dark backgrounds (bg-ox-black-1,bg-ox-black-2) provides appropriate contrast and readability. No components are negatively impacted by this change.apps/web/src/components/dashboard/Sidebar.tsx (1)
20-23: Newsletter route: LGTM.Good addition; prefix matching will cover nested routes like /dashboard/newsletters/[slug].
apps/api/src/routers/_app.ts (1)
7-7: Wire-up of newsletter router looks correct.Import and registration into appRouter are consistent with existing pattern.
Also applies to: 23-24
|
@Shivam107 hi shivam, aman is currently working on the overall dashboard. so once, it's done. i'll dive deep into it and will leave reviews because we'll have to make some changes after the dashboard revamp. nice ui btw ❤️ |
yes , that's what i was thinking |
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 (3)
apps/api/src/routers/_app.ts(2 hunks)apps/web/package.json(1 hunks)apps/web/src/components/dashboard/Sidebar.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/src/components/dashboard/Sidebar.tsx
- apps/web/package.json
🧰 Additional context used
🧬 Code graph analysis (1)
apps/api/src/routers/_app.ts (1)
apps/api/src/routers/newsletter.ts (1)
newsletterRouter(5-13)
🔇 Additional comments (1)
apps/api/src/routers/_app.ts (1)
8-8: LGTM!The newsletter router integration follows the established pattern and is correctly wired into the app router. The implementation is clean and consistent with existing router registrations.
Also applies to: 24-24
| import { userRouter } from "./user.js"; | ||
| import { projectRouter } from "./projects.js"; | ||
| import { authRouter } from "./auth.js"; | ||
| import { paymentRouter } from "./payment.js"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused import.
The paymentRouter import is no longer used in the appRouter after being replaced with newsletterRouter. This is dead code that should be removed.
Apply this diff to remove the unused import:
-import { paymentRouter } from "./payment.js";📝 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.
| import { paymentRouter } from "./payment.js"; |
🤖 Prompt for AI Agents
In apps/api/src/routers/_app.ts around line 6, the import "paymentRouter" is
unused after replacing it with "newsletterRouter"; remove the unused import
statement from the file (delete the import line for paymentRouter) and verify
there are no remaining references to paymentRouter in the file or exports so the
file compiles and lints cleanly.
2 participants
Fixes : #122
demoNewsletter1.mp4
any changes should i make on this?
Summary by CodeRabbit
New Features
UI/UX Updates
Chores