feat: implement newsletters dashboard with filtering and detail views #159
Conversation
- Added main newsletters component with search and month filtering. - Created individual newsletter pages displaying detailed content. - Implemented reusable components for newsletter cards, filters, and empty states. - Introduced utility functions for filtering and grouping newsletters by month. - Added sample newsletter data for testing and display purposes.
|
@prudvinani 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. |
|
Warning Rate limit exceeded@prudvinani has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 13 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 (1)
WalkthroughAdds a newsletters feature: listing page with search and month filters, a detail page by ID, static newsletter data and types, new Input and Radix-based Select UI components, sidebar navigation entry for Newsletters, Next.js image host config, and a Radix Select dependency. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant NewslistPage as Newsletters Page
participant Data as Newsletter Data
participant UI as UI Components
User->>NewslistPage: Open listing / Enter search / Select month
NewslistPage->>Data: Read newsletters[]
Data-->>NewslistPage: Return list
rect rgb(220,235,255)
note right of NewslistPage: Filter & Group (memoized)
NewslistPage->>NewslistPage: Filter by search across title/preview/content/author/takeaways
NewslistPage->>NewslistPage: Filter by selected month (or all)
NewslistPage->>NewslistPage: Group by month (desc)
end
NewslistPage->>UI: Render grouped NewsletterCard(s)
UI-->>User: Display results
sequenceDiagram
actor User
participant DetailPage as Newsletter Detail Page
participant Params as Route Params (useParams)
participant Data as Newsletter Data
participant UI as UI Components
User->>DetailPage: Navigate to /dashboard/newsletters/:id
DetailPage->>Params: Read id
DetailPage->>Data: Lookup newsletter by id
alt found
Data-->>DetailPage: Newsletter
DetailPage->>DetailPage: Split content into paragraphs
DetailPage->>DetailPage: Insert contentImages at indices
DetailPage->>DetailPage: Convert URLs -> links
DetailPage->>UI: Render header, content, takeaways
UI-->>User: Show newsletter detail
else not found
DetailPage->>UI: Render not-found UI with back action
UI-->>User: Show not-found
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 5
🧹 Nitpick comments (11)
apps/web/src/app/(main)/dashboard/newsletters/page.tsx (2)
8-8: Consider removing the type assertion if data conforms to Newsletter[].The type assertion
as Newsletter[]may be unnecessary if thenewslettersdata already matches theNewsletter[]type. TypeScript should infer the type correctly if the data structure is aligned.If the assertion is needed due to type mismatch, verify the data structure in
data/newsletters.tsaligns with theNewsletterinterface.
7-9: Consider removing the wrapper div.The wrapper
<div>doesn't add semantic value or styling. You can return theNewsletterscomponent directly.export default function NewslettersPage() { - return ( - <div> - <Newsletters newsletters={newsletters as Newsletter[]} /> - </div> - ); + return <Newsletters newsletters={newsletters as Newsletter[]} />; }apps/web/src/app/(main)/dashboard/newsletters/components/NewsletterList.tsx (1)
16-24: Consider reusing NewsletterEmptyState component.The empty state message "No newsletters yet. Check back soon!" is duplicated here and in the
NewsletterEmptyStatecomponent. For consistency and maintainability, consider reusing theNewsletterEmptyStatecomponent.if (newsletters.length === 0) { - return ( - <div className="text-center py-12"> - <p className="text-muted-foreground"> - No newsletters yet. Check back soon! - </p> - </div> - ); + return <NewsletterEmptyState hasActiveFilters={false} onClearFilters={() => {}} />; }However, verify if this component's empty state should differ from the filter-driven empty state in the parent component.
apps/web/src/types/newsletter.ts (2)
36-36: Consider the date field type.The
datefield is typed asstring. While this is suitable for serialization and JSON compatibility, consider whether you need:
- A more specific type like
${number}-${number}-${number}for ISO date strings- Date parsing validation utilities if consuming this from external sources
- Documentation about the expected format (appears to be "YYYY-MM-DD" based on the sample data)
The current approach is acceptable for static data and JSON serialization. This is just a consideration for future enhancements if date validation or stronger typing becomes necessary.
9-11: Consider supporting additional heading levels.The heading type currently supports levels 1-3. Consider whether you need to support h4-h6 (
level: 1 | 2 | 3 | 4 | 5 | 6) for more granular content hierarchy in future newsletters.If the current levels (1-3) cover all use cases, the current implementation is fine.
apps/web/src/app/(main)/dashboard/newsletters/[id]/page.tsx (1)
101-101: Remove redundant type assertion.The
contentproperty is already typed asNewsletterContentItem[]in theNewsletterinterface, making theas NewsletterContentItem[]assertion unnecessary.Apply this diff:
- <NewsletterContent content={newsletter.content as NewsletterContentItem[]} /> + <NewsletterContent content={newsletter.content} />apps/web/src/app/(main)/dashboard/newsletters/utils/newsletter.filters.ts (2)
9-20: Remove unnecessary optional chaining on required properties.Based on the
NewsletterContentItemtype definition, thecontent,text, andurlproperties are required (not optional) for their respective item types. The optional chaining operators on lines 11, 15, and 16 are unnecessary.Apply this diff:
const matchesContent = newsletter.content?.some((item: NewsletterContentItem) => { if (item.type === "paragraph" || item.type === "heading" || item.type === "bold") { - return item.content?.toLowerCase().includes(query); + return item.content.toLowerCase().includes(query); } if (item.type === "link") { return ( - item.text?.toLowerCase().includes(query) || - item.url?.toLowerCase().includes(query) + item.text.toLowerCase().includes(query) || + item.url.toLowerCase().includes(query) ); } return false; });
22-22: Remove redundant|| falsein boolean expression.Since both
matchesBasicFieldsandmatchesContentare already boolean values, the|| falseis redundant.Apply this diff:
- return matchesBasicFields || matchesContent || false; + return matchesBasicFields || matchesContent;apps/web/src/app/(main)/dashboard/newsletters/components/NewsletterCard.tsx (1)
18-18: Replace hardcoded color with a theme variable.The hardcoded color
#693dabshould be replaced with a CSS variable or Tailwind theme color for better maintainability and theme consistency.Consider using a semantic color like
hover:border-primaryor defining this color in your Tailwind theme configuration:- <Card className="overflow-hidden hover:shadow-lg transition-all duration-300 border-border hover:border-[#693dab] cursor-pointer"> + <Card className="overflow-hidden hover:shadow-lg transition-all duration-300 border-border hover:border-primary cursor-pointer">apps/web/src/app/(main)/dashboard/newsletters/components/NewsletterContent.tsx (2)
5-5: Remove unused import.The
Imagecomponent fromnext/imageis imported but never used in this file.Apply this diff:
import { NewsletterContentItem } from "@/types/newsletter"; import Link from "next/link"; -import Image from "next/image";
23-37: Wrap variable declarations in block scope.Variable declarations in switch cases should be wrapped in blocks to prevent them from being erroneously accessible in other cases.
Based on learnings
Apply this diff:
case "heading": + { const HeadingTag = `h${item.level}` as keyof JSX.IntrinsicElements; const headingClasses = { }; return ( <HeadingTag key={index} className={headingClasses[item.level]} > {item.content} </HeadingTag> ); + }Apply the same pattern to the
"list"case on line 71.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
apps/web/src/app/(main)/dashboard/newsletters/Content.tsx(1 hunks)apps/web/src/app/(main)/dashboard/newsletters/[id]/page.tsx(1 hunks)apps/web/src/app/(main)/dashboard/newsletters/components/NewsletterCard.tsx(1 hunks)apps/web/src/app/(main)/dashboard/newsletters/components/NewsletterContent.tsx(1 hunks)apps/web/src/app/(main)/dashboard/newsletters/components/NewsletterEmptyState.tsx(1 hunks)apps/web/src/app/(main)/dashboard/newsletters/components/NewsletterFilters.tsx(1 hunks)apps/web/src/app/(main)/dashboard/newsletters/components/NewsletterList.tsx(1 hunks)apps/web/src/app/(main)/dashboard/newsletters/data/newsletters.ts(1 hunks)apps/web/src/app/(main)/dashboard/newsletters/page.tsx(1 hunks)apps/web/src/app/(main)/dashboard/newsletters/utils/newsletter.filters.ts(1 hunks)apps/web/src/app/(main)/dashboard/newsletters/utils/newsletter.utils.ts(1 hunks)apps/web/src/components/dashboard/Sidebar.tsx(2 hunks)apps/web/src/components/ui/input.tsx(1 hunks)apps/web/src/components/ui/select.tsx(1 hunks)apps/web/src/types/index.ts(1 hunks)apps/web/src/types/newsletter.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (11)
apps/web/src/app/(main)/dashboard/newsletters/components/NewsletterCard.tsx (2)
apps/web/src/types/newsletter.ts (1)
Newsletter(33-42)apps/web/src/app/(main)/dashboard/newsletters/utils/newsletter.utils.ts (1)
formatNewsletterDate(51-57)
apps/web/src/app/(main)/dashboard/newsletters/utils/newsletter.filters.ts (1)
apps/web/src/types/newsletter.ts (2)
Newsletter(33-42)NewsletterContentItem(3-31)
apps/web/src/app/(main)/dashboard/newsletters/components/NewsletterList.tsx (3)
apps/web/src/types/newsletter.ts (1)
Newsletter(33-42)apps/web/src/app/(main)/dashboard/newsletters/utils/newsletter.utils.ts (2)
groupByMonth(3-26)sortMonthKeys(28-34)apps/web/src/app/(main)/dashboard/newsletters/components/NewsletterCard.tsx (1)
NewsletterCard(13-64)
apps/web/src/app/(main)/dashboard/newsletters/components/NewsletterContent.tsx (1)
apps/web/src/types/newsletter.ts (1)
NewsletterContentItem(3-31)
apps/web/src/app/(main)/dashboard/newsletters/page.tsx (3)
apps/web/src/app/(main)/dashboard/newsletters/Content.tsx (1)
Newsletters(16-73)apps/web/src/app/(main)/dashboard/newsletters/data/newsletters.ts (1)
newsletters(2-212)apps/web/src/types/newsletter.ts (1)
Newsletter(33-42)
apps/web/src/app/(main)/dashboard/newsletters/[id]/page.tsx (4)
apps/web/src/app/(main)/dashboard/newsletters/data/newsletters.ts (1)
newsletters(2-212)apps/web/src/app/(main)/dashboard/newsletters/utils/newsletter.utils.ts (1)
formatNewsletterDate(51-57)apps/web/src/app/(main)/dashboard/newsletters/components/NewsletterContent.tsx (1)
NewsletterContent(11-114)apps/web/src/types/newsletter.ts (1)
NewsletterContentItem(3-31)
apps/web/src/app/(main)/dashboard/newsletters/Content.tsx (6)
apps/web/src/types/newsletter.ts (1)
Newsletter(33-42)apps/web/src/app/(main)/dashboard/newsletters/utils/newsletter.utils.ts (1)
getAvailableMonths(37-48)apps/web/src/app/(main)/dashboard/newsletters/utils/newsletter.filters.ts (1)
filterNewsletters(40-59)apps/web/src/app/(main)/dashboard/newsletters/components/NewsletterFilters.tsx (1)
NewsletterFilters(24-82)apps/web/src/app/(main)/dashboard/newsletters/components/NewsletterEmptyState.tsx (1)
NewsletterEmptyState(10-28)apps/web/src/app/(main)/dashboard/newsletters/components/NewsletterList.tsx (1)
NewsletterList(12-44)
apps/web/src/app/(main)/dashboard/newsletters/utils/newsletter.utils.ts (2)
apps/web/src/types/newsletter.ts (1)
Newsletter(33-42)apps/web/src/app/(main)/dashboard/newsletters/data/newsletters.ts (1)
newsletters(2-212)
apps/web/src/components/ui/input.tsx (1)
apps/web/src/lib/utils.ts (1)
cn(4-6)
apps/web/src/components/ui/select.tsx (1)
apps/web/src/lib/utils.ts (1)
cn(4-6)
apps/web/src/app/(main)/dashboard/newsletters/components/NewsletterFilters.tsx (2)
apps/web/src/components/ui/input.tsx (1)
Input(22-22)apps/web/src/components/ui/select.tsx (5)
Select(149-149)SelectTrigger(152-152)SelectValue(151-151)SelectContent(153-153)SelectItem(155-155)
🪛 Biome (2.1.2)
apps/web/src/app/(main)/dashboard/newsletters/components/NewsletterContent.tsx
[error] 24-24: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 25-29: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 72-72: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (10)
apps/web/src/components/ui/select.tsx (1)
1-160: LGTM! Well-structured Radix UI Select wrapper.The component follows React best practices with proper forwardRef usage, TypeScript typing, and displayName assignments. The className composition using the
cnutility is consistent, and all Radix UI primitives are correctly wrapped and exported.apps/web/src/types/index.ts (1)
3-3: LGTM! Consistent barrel export pattern.The newsletter types re-export follows the established pattern and properly centralizes type access.
apps/web/src/app/(main)/dashboard/newsletters/components/NewsletterEmptyState.tsx (1)
1-29: LGTM! Clean empty state implementation.The component correctly handles both filtered and unfiltered empty states with appropriate messaging and user actions.
apps/web/src/components/ui/input.tsx (1)
1-22: LGTM! Well-implemented Input component.The component follows React best practices with proper forwardRef usage, TypeScript typing, and comprehensive styling. The use of
cnutility for className composition is consistent with other UI components.apps/web/src/app/(main)/dashboard/newsletters/components/NewsletterList.tsx (1)
12-44: LGTM! Clean implementation of grouped newsletter display.The component properly groups newsletters by month, sorts them in descending order, and renders them in an organized structure. The use of utility functions keeps the logic clean and maintainable.
apps/web/src/app/(main)/dashboard/newsletters/data/newsletters.ts (1)
1-212: LGTM! Comprehensive sample newsletter data.The sample data is well-structured with rich content blocks covering various types (paragraphs, headings, links, lists). This provides good test coverage for the UI components.
apps/web/src/types/newsletter.ts (1)
1-42: LGTM! Well-designed type definitions.The discriminated union for
NewsletterContentItemprovides type-safe content blocks, and theNewsletterinterface is comprehensive with optional fields for flexibility. The types support both static and dynamic image sources appropriately.apps/web/src/app/(main)/dashboard/newsletters/components/NewsletterFilters.tsx (1)
24-82: LGTM!The
NewsletterFilterscomponent is well-implemented with clean state management, responsive design, and good user experience. The active filter detection and clear filters functionality work correctly.apps/web/src/app/(main)/dashboard/newsletters/Content.tsx (1)
16-73: LGTM!The
Newsletterscomponent is well-structured with appropriate use ofuseMemofor performance optimization. The filtering logic is correctly implemented with proper state management and conditional rendering.apps/web/src/app/(main)/dashboard/newsletters/utils/newsletter.utils.ts (1)
3-26: Utility functions are well-structured.The
groupByMonth,getAvailableMonths, andformatNewsletterDatefunctions are correctly implemented with appropriate logic for organizing and formatting newsletter data. The only concern is withsortMonthKeys(noted separately).Also applies to: 37-48, 51-57
apps/web/src/app/(main)/dashboard/newsletters/components/NewsletterContent.tsx
Outdated
Show resolved
Hide resolved
apps/web/src/app/(main)/dashboard/newsletters/data/newsletters.ts
Outdated
Show resolved
Hide resolved
apps/web/src/app/(main)/dashboard/newsletters/utils/newsletter.utils.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/web/next.config.js (1)
9-12: Consider adding pathname restrictions for tighter security.The remote pattern configuration is correct and enables loading newsletter images from Unsplash. However, the pattern is broad and allows any image from
images.unsplash.com.If your newsletter images follow a predictable URL structure, consider adding a
pathnamefield to restrict which images can be loaded.Example with pathname restriction:
{ protocol: "https", hostname: "images.unsplash.com", + pathname: "/photo-*/**", },Operational considerations:
- External images introduce latency and dependency on Unsplash availability
- Consider implementing fallback images for resilience
- Monitor Unsplash rate limits if serving high traffic
- Next.js will cache optimized images, but first load still depends on external service
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/next.config.js(1 hunks)apps/web/package.json(1 hunks)
🔇 Additional comments (1)
apps/web/package.json (1)
18-18: @radix-ui/react-select version is current and secure.The added version 2.2.6 is the latest available, and Next.js 15 supports React 18, which matches your project configuration. No security vulnerabilities were found. The dependency addition is safe to proceed with.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/web/src/app/(main)/dashboard/newsletters/utils/newsletter.utils.ts (1)
28-37: Strengthen date parsing with validation and whitespace handling.The current implementation improves on the past review but still has gaps: it doesn't validate the split result, handle extra whitespace, or guard against invalid month names.
Consider a more robust approach using month name mapping:
+const MONTH_MAP: { [key: string]: number } = { + january: 0, february: 1, march: 2, april: 3, may: 4, june: 5, + july: 6, august: 7, september: 8, october: 9, november: 10, december: 11 +}; + export const sortMonthKeys = (keys: string[]): string[] => { return keys.sort((a, b) => { - // Parse month and year separately for reliable date parsing - const [monthA, yearA] = a.split(" "); - const [monthB, yearB] = b.split(" "); - const dateA = new Date(`${monthA} 1, ${yearA}`); - const dateB = new Date(`${monthB} 1, ${yearB}`); - return dateB.getTime() - dateA.getTime(); + const [monthA, yearA] = a.trim().split(/\s+/); + const [monthB, yearB] = b.trim().split(/\s+/); + + if (!monthA || !yearA || !monthB || !yearB) { + return 0; // Handle malformed keys + } + + const monthIndexA = MONTH_MAP[monthA.toLowerCase()]; + const monthIndexB = MONTH_MAP[monthB.toLowerCase()]; + const numA = parseInt(yearA) * 12 + (monthIndexA ?? 0); + const numB = parseInt(yearB) * 12 + (monthIndexB ?? 0); + + return numB - numA; }); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/src/app/(main)/dashboard/newsletters/data/newsletters.ts(1 hunks)apps/web/src/app/(main)/dashboard/newsletters/utils/newsletter.utils.ts(1 hunks)apps/web/src/components/dashboard/Sidebar.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/app/(main)/dashboard/newsletters/data/newsletters.ts
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/src/app/(main)/dashboard/newsletters/utils/newsletter.utils.ts (2)
apps/web/src/types/newsletter.ts (1)
Newsletter(33-42)apps/web/src/app/(main)/dashboard/newsletters/data/newsletters.ts (1)
newsletters(2-212)
🔇 Additional comments (3)
apps/web/src/components/dashboard/Sidebar.tsx (3)
20-20: LGTM: Icon import is consistent with existing patterns.The
NewspaperIconimport from@heroicons/react/24/outlinecorrectly matches the icon library used for all other sidebar icons, resolving the previous concern about mixing icon libraries.
39-43: LGTM: Newsletters route correctly implemented and previous issues resolved.The newsletters route addition is well-structured and consistent with existing sidebar routes. The label is correctly spelled as "Newsletters" (one word), and the icon usage follows the established pattern with
className="size-5".
26-26: Import is used but appears unrelated to the newsletters feature scope—request manual verification of whether this should be in this PR.The
useFilterStoreimport on line 26 is actively used on lines 55 and 80 for the "Find Projects" functionality (viahandleFindProjects()), not for the newsletters feature. While the import is not dead code, the original concern about scope is valid: if this is a new addition to the codebase, it should clarify whether Find Projects filtering functionality belongs in a newsletters PR or if it's a separate feature being added concurrently.The line is marked as changed, which may indicate a new import, but the git history check failed and couldn't confirm whether this existed before.
apps/web/src/app/(main)/dashboard/newsletters/utils/newsletter.utils.ts
Outdated
Show resolved
Hide resolved
apps/web/src/app/(main)/dashboard/newsletters/utils/newsletter.utils.ts
Outdated
Show resolved
Hide resolved
apps/web/src/app/(main)/dashboard/newsletters/utils/newsletter.utils.ts
Outdated
Show resolved
Hide resolved
- Implemented premium access control for newsletters, displaying a loading state and a premium gate for unpaid users. - Added caching mechanism for newsletters data to improve performance. - Introduced new content types for newsletters, including code snippets and tables. - Created a dedicated component for the premium access gate with upgrade options.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/web/src/app/(main)/dashboard/newsletters/components/NewsletterContent.tsx (1)
71-98: Fix inverted alignment logic.Line 72 has a critical logic error:
const isRightAligned = item.align === "left";assignstruewhen alignment is"left", which is backwards. This causes lists to display with incorrect alignment.Apply this diff to fix:
case "list": + { - const isRightAligned = item.align === "left"; + const isRightAligned = item.align === "right"; if (isRightAligned) { return ( <ul key={index} className="my-4 space-y-2 list-none text-right"> {item.items.map((listItem, itemIndex) => ( <li key={itemIndex} className="text-foreground/90 flex items-center justify-end gap-2" > <span>{listItem}</span> <span className="text-foreground/60">•</span> </li> ))} </ul> ); } return ( <ul key={index} className="my-4 space-y-2 list-disc list-inside"> {item.items.map((listItem, itemIndex) => ( <li key={itemIndex} className="text-foreground/90"> {listItem} </li> ))} </ul> ); + }Also wraps the case in a block to satisfy the linter warning about switch declarations.
🧹 Nitpick comments (10)
apps/web/src/app/(main)/dashboard/newsletters/utils/newsletter.filters.ts (2)
9-29: Optional: Simplify the return statement.Line 28's
|| falseis redundant defensive coding sincematchesBasicFieldsis already boolean. However, it guards againstmatchesContentbeing undefined whencontentis empty.If preferred, simplify to:
- return matchesBasicFields || matchesContent || false; + return matchesBasicFields || (matchesContent ?? false);
37-49: Add date validation to prevent "Invalid Date" comparisons.If
newsletter.dateis malformed,new Date(newsletter.date)will produce an invalid date, andtoLocaleDateStringwill return"Invalid Date", which will never matchselectedMonthbut won't surface the error.Add validation:
const matchesMonthFilter = ( newsletter: Newsletter, selectedMonth: string ): boolean => { if (selectedMonth === "all") return true; const date = new Date(newsletter.date); + if (isNaN(date.getTime())) { + console.warn(`Invalid date for newsletter ${newsletter.id}: ${newsletter.date}`); + return false; + } const monthYear = date.toLocaleDateString("en-US", { month: "long", year: "numeric", }); return monthYear === selectedMonth; };apps/web/src/app/(main)/dashboard/newsletters/page.tsx (2)
12-18: Consider enhancing the loading state.The loading state is very basic. For a better user experience, consider using a proper loading skeleton or spinner component that matches your design system.
Example improvement:
if (isLoading) { return ( - <div className="min-h-screen bg-background flex items-center justify-center"> - <div className="text-foreground">Loading...</div> + <div className="min-h-screen bg-background flex items-center justify-center"> + <div className="flex flex-col items-center gap-4"> + <div className="h-8 w-8 animate-spin rounded-full border-4 border-ox-purple border-t-transparent"></div> + <p className="text-muted-foreground">Loading newsletters...</p> + </div> </div> ); }
24-28: Remove unnecessary type assertion.Line 26 uses
as Newsletter[]to cast the newsletters data, but the importednewslettersconstant should already have the correct type from its definition. Type assertions can hide type mismatches.Apply this diff:
return ( <div> - <Newsletters newsletters={newsletters as Newsletter[]} /> + <Newsletters newsletters={newsletters} /> </div> );If TypeScript complains after removing the assertion, the root cause is likely a type mismatch that should be fixed at the source rather than suppressed with a cast.
apps/web/src/app/(main)/dashboard/newsletters/[id]/page.tsx (2)
16-50: LGTM with optional optimization note.The subscription gating and not-found handling are implemented correctly. The linear search on line 20 is acceptable for small datasets, but consider building a lookup map if the newsletter count grows significantly.
Optional: For larger datasets, pre-compute a lookup:
const newsletterMap = new Map(newsletters.map(n => [n.id, n])); const newsletter = newsletterMap.get(id);
114-117: Remove unnecessary type assertion.Line 116 casts
newsletter.contenttoNewsletterContentItem[], but it should already have this type from the Newsletter interface. Type assertions can hide type mismatches.Apply this diff:
<div className="prose prose-lg max-w-none font-sans"> - <NewsletterContent content={newsletter.content as NewsletterContentItem[]} /> + <NewsletterContent content={newsletter.content} /> </div>apps/web/src/app/(main)/dashboard/newsletters/components/NewsletterContent.tsx (2)
23-37: Wrap case declarations in blocks to satisfy linter.The linter flags that
HeadingTagandheadingClassesdeclarations can leak to other switch cases. While not a functional bug here (the case returns), wrapping in a block is best practice.Apply this diff:
case "heading": + { const HeadingTag = `h${item.level}` as keyof JSX.IntrinsicElements; const headingClasses = { }; return ( <HeadingTag key={index} className={headingClasses[item.level]} > {item.content} </HeadingTag> ); + }
60-69: Consider using Next.js Image for optimization.The image case uses a native
<img>tag. While this works, Next.js Image provides automatic optimization, lazy loading, and better performance.However, since newsletter images use external URLs (per the data file), the current approach is acceptable. If you want to use Next.js Image, you'll need to configure
remotePatternsinnext.config.js.apps/web/src/app/(main)/dashboard/newsletters/utils/newsletter.utils.ts (2)
60-71: Add date validation to prevent "Invalid Date" in month filters.Unlike
groupByMonth, this function doesn't validate dates before formatting. Invalidnewsletter.datevalues will produce"Invalid Date"strings that could appear in the month filter dropdown.Apply this diff to add validation:
export const getAvailableMonths = (newsletters: Newsletter[]): string[] => { - const months = newsletters.map((n) => { + const months = newsletters.flatMap((n) => { const date = new Date(n.date); + if (isNaN(date.getTime())) { + return []; // Skip invalid dates + } return date.toLocaleDateString("en-US", { month: "long", year: "numeric", }); }); const uniqueMonths = Array.from(new Set(months)); return sortMonthKeys(uniqueMonths); };Using
flatMapwithreturn []skips invalid dates, while valid dates return an array with one element that gets flattened.
79-85: Add validation to prevent displaying "Invalid Date" to users.This function is used to display dates in the UI. Without validation, malformed
dateStringvalues will produce"Invalid Date"that gets shown to users.Apply this diff to add validation with a user-friendly fallback:
export const formatNewsletterDate = (dateString: string): string => { - return new Date(dateString).toLocaleDateString("en-US", { + const date = new Date(dateString); + if (isNaN(date.getTime())) { + return "Date unavailable"; + } + return date.toLocaleDateString("en-US", { month: "long", day: "numeric", year: "numeric", }); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/web/src/app/(main)/dashboard/newsletters/[id]/page.tsx(1 hunks)apps/web/src/app/(main)/dashboard/newsletters/components/NewsletterContent.tsx(1 hunks)apps/web/src/app/(main)/dashboard/newsletters/components/NewsletterPremiumGate.tsx(1 hunks)apps/web/src/app/(main)/dashboard/newsletters/page.tsx(1 hunks)apps/web/src/app/(main)/dashboard/newsletters/utils/newsletter.cache.ts(1 hunks)apps/web/src/app/(main)/dashboard/newsletters/utils/newsletter.filters.ts(1 hunks)apps/web/src/app/(main)/dashboard/newsletters/utils/newsletter.utils.ts(1 hunks)apps/web/src/types/newsletter.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/types/newsletter.ts
🧰 Additional context used
🧬 Code graph analysis (7)
apps/web/src/app/(main)/dashboard/newsletters/utils/newsletter.cache.ts (2)
apps/web/src/types/newsletter.ts (1)
Newsletter(43-52)apps/web/src/app/(main)/dashboard/newsletters/data/newsletters.ts (1)
newsletters(2-212)
apps/web/src/app/(main)/dashboard/newsletters/components/NewsletterPremiumGate.tsx (1)
apps/web/src/components/ui/card.tsx (4)
CardHeader(78-78)CardTitle(80-80)CardDescription(81-81)CardContent(82-82)
apps/web/src/app/(main)/dashboard/newsletters/components/NewsletterContent.tsx (1)
apps/web/src/types/newsletter.ts (1)
NewsletterContentItem(3-41)
apps/web/src/app/(main)/dashboard/newsletters/[id]/page.tsx (6)
apps/web/src/hooks/useSubscription.ts (1)
useSubscription(11-75)apps/web/src/app/(main)/dashboard/newsletters/data/newsletters.ts (1)
newsletters(2-212)apps/web/src/app/(main)/dashboard/newsletters/components/NewsletterPremiumGate.tsx (1)
NewsletterPremiumGate(9-66)apps/web/src/app/(main)/dashboard/newsletters/utils/newsletter.utils.ts (1)
formatNewsletterDate(79-85)apps/web/src/app/(main)/dashboard/newsletters/components/NewsletterContent.tsx (1)
NewsletterContent(11-155)apps/web/src/types/newsletter.ts (1)
NewsletterContentItem(3-41)
apps/web/src/app/(main)/dashboard/newsletters/page.tsx (5)
apps/web/src/hooks/useSubscription.ts (1)
useSubscription(11-75)apps/web/src/app/(main)/dashboard/newsletters/components/NewsletterPremiumGate.tsx (1)
NewsletterPremiumGate(9-66)apps/web/src/app/(main)/dashboard/newsletters/Content.tsx (1)
Newsletters(16-73)apps/web/src/app/(main)/dashboard/newsletters/data/newsletters.ts (1)
newsletters(2-212)apps/web/src/types/newsletter.ts (1)
Newsletter(43-52)
apps/web/src/app/(main)/dashboard/newsletters/utils/newsletter.filters.ts (1)
apps/web/src/types/newsletter.ts (2)
Newsletter(43-52)NewsletterContentItem(3-41)
apps/web/src/app/(main)/dashboard/newsletters/utils/newsletter.utils.ts (2)
apps/web/src/types/newsletter.ts (1)
Newsletter(43-52)apps/web/src/app/(main)/dashboard/newsletters/data/newsletters.ts (1)
newsletters(2-212)
🪛 Biome (2.1.2)
apps/web/src/app/(main)/dashboard/newsletters/components/NewsletterContent.tsx
[error] 24-24: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 25-29: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 72-72: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (6)
apps/web/src/app/(main)/dashboard/newsletters/components/NewsletterPremiumGate.tsx (1)
9-66: LGTM!The premium gate component is well-structured with clear messaging and appropriate CTAs for both upgrading and returning to the dashboard.
apps/web/src/app/(main)/dashboard/newsletters/utils/newsletter.filters.ts (1)
59-78: LGTM!The filtering logic correctly chains search and month filters, with appropriate handling of empty search queries.
apps/web/src/app/(main)/dashboard/newsletters/[id]/page.tsx (1)
67-84: LGTM!The conditional rendering for cover images correctly handles both string URLs and Next.js StaticImageData objects. This flexible approach supports both external URLs and imported images.
apps/web/src/app/(main)/dashboard/newsletters/components/NewsletterContent.tsx (1)
46-58: LGTM!The link rendering correctly includes
target="_blank"withrel="noopener noreferrer"for security when opening external links.apps/web/src/app/(main)/dashboard/newsletters/utils/newsletter.utils.ts (2)
8-35: LGTM!The date validation added on lines 13-16 correctly addresses the previous review concern about handling invalid dates. Invalid newsletters are logged and skipped, preventing "Invalid Date" strings from appearing as group keys.
43-52: LGTM!The reliable date parsing approach (lines 46-49) addresses the previous review concern by splitting month/year components and explicitly adding a day. This ensures consistent parsing across JavaScript engines.
apps/web/src/app/(main)/dashboard/newsletters/utils/newsletter.cache.ts
Outdated
Show resolved
Hide resolved
- Replaced <img> tags with <Image> components for better optimization in newsletter pages and cards. - Adjusted image classes to use 'object-contain' for improved layout. - Removed the newsletter caching utility as it is no longer needed.
- Removed premium access checks and loading states from newsletters and individual newsletter pages. - Updated NewsletterContent component to convert URLs in text to clickable links. - Enhanced list items to support clickable links and adjusted alignment logic. - Removed the NewsletterPremiumGate component as it is no longer needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/web/src/app/(main)/dashboard/newsletters/components/NewsletterContent.tsx (1)
19-42: Wrap switch case variable declarations in blocks.Multiple case clauses declare variables without block scope, which allows them to be erroneously accessed by other cases. While the current code doesn't exhibit this issue, wrapping each case body in blocks is a JavaScript/TypeScript best practice that prevents potential scope-related bugs.
Apply this diff to wrap the affected cases in blocks:
switch (item.type) { - case "paragraph": + case "paragraph": { // Convert URLs in text to clickable links const parts = item.content.split(urlRegex); return ( <p key={index} className="text-foreground/90 leading-relaxed"> {parts.map((part, partIndex) => { if (part.match(urlRegex)) { return ( <Link key={partIndex} href={part} target="_blank" rel="noopener noreferrer" className="text-blue-500 hover:text-blue-600 hover:underline font-medium" > {part} </Link> ); } return <span key={partIndex}>{part}</span>; })} </p> ); + } - case "heading": + case "heading": { const HeadingTag = `h${item.level}` as keyof JSX.IntrinsicElements; const headingClasses = { }; return ( <HeadingTag key={index} className={headingClasses[item.level]} > {item.content} </HeadingTag> ); + } - case "list": + case "list": { const isRightAligned = item.align === "right"; const renderListItem = (listItem: string, itemIndex: number) => { const parts = listItem.split(urlRegex); return parts.map((part, partIndex) => { if (part.match(urlRegex)) { return ( <Link key={partIndex} href={part} target="_blank" rel="noopener noreferrer" className="text-blue-500 hover:text-blue-600 hover:underline font-medium" > {part} </Link> ); } return <span key={partIndex}>{part}</span>; }); }; if (isRightAligned) { return ( <ul key={index} className="my-4 space-y-2 list-none text-right"> {item.items.map((listItem, itemIndex) => ( <li key={itemIndex} className="text-foreground/90 flex items-center justify-end gap-2" > <span>{renderListItem(listItem, itemIndex)}</span> <span className="text-foreground/60">•</span> </li> ))} </ul> ); } return ( <ul key={index} className="my-4 space-y-2 list-disc list-inside"> {item.items.map((listItem, itemIndex) => ( <li key={itemIndex} className="text-foreground/90"> {renderListItem(listItem, itemIndex)} </li> ))} </ul> ); + }Also applies to: 44-58, 94-141
apps/web/src/app/(main)/dashboard/newsletters/page.tsx (3)
8-12: Consider removing the redundant wrapper div.The outer
<div>serves no purpose since theNewsletterscomponent already provides its own wrapper.export default function NewslettersPage() { - return ( - <div> - <Newsletters newsletters={newsletters as Newsletter[]} /> - </div> - ); + return <Newsletters newsletters={newsletters as Newsletter[]} />; }
1-1: Consider removing the "use client" directive.This page component doesn't use any client-side hooks or browser APIs—it only passes props to the
Newsletterscomponent. In Next.js 15, server components are the default and offer better performance. TheNewsletterscomponent imported fromContent.tsx(verified to have"use client"on line 1) will continue to work as a client component regardless of whether the parent page is a server or client component.Remove the directive:
-"use client"; - import { Newsletter } from "@/types/newsletter"; import Newsletters from "./Content"; import { newsletters } from "./data/newsletters";
10-10: Add type annotation to thenewslettersexport to eliminate the type cast.The data structure in
newsletters.tsperfectly matches theNewsletterinterface—the cast is unnecessary. Add explicit type annotation to the export inapps/web/src/app/(main)/dashboard/newsletters/data/newsletters.ts:export const newsletters: Newsletter[] = [ // ... data ];Then remove the cast in
page.tsx:<Newsletters newsletters={newsletters} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/web/src/app/(main)/dashboard/newsletters/[id]/page.tsx(1 hunks)apps/web/src/app/(main)/dashboard/newsletters/components/NewsletterCard.tsx(1 hunks)apps/web/src/app/(main)/dashboard/newsletters/components/NewsletterContent.tsx(1 hunks)apps/web/src/app/(main)/dashboard/newsletters/data/newsletters.ts(1 hunks)apps/web/src/app/(main)/dashboard/newsletters/page.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/web/src/app/(main)/dashboard/newsletters/components/NewsletterCard.tsx
- apps/web/src/app/(main)/dashboard/newsletters/[id]/page.tsx
- apps/web/src/app/(main)/dashboard/newsletters/data/newsletters.ts
🧰 Additional context used
🧬 Code graph analysis (2)
apps/web/src/app/(main)/dashboard/newsletters/page.tsx (3)
apps/web/src/app/(main)/dashboard/newsletters/Content.tsx (1)
Newsletters(16-73)apps/web/src/app/(main)/dashboard/newsletters/data/newsletters.ts (1)
newsletters(2-224)apps/web/src/types/newsletter.ts (1)
Newsletter(43-52)
apps/web/src/app/(main)/dashboard/newsletters/components/NewsletterContent.tsx (1)
apps/web/src/types/newsletter.ts (1)
NewsletterContentItem(3-41)
🪛 Biome (2.1.2)
apps/web/src/app/(main)/dashboard/newsletters/components/NewsletterContent.tsx
[error] 21-21: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 45-45: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 46-50: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 95-95: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 97-115: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (1)
apps/web/src/app/(main)/dashboard/newsletters/components/NewsletterContent.tsx (1)
94-141: Previous alignment logic issue has been fixed.The inverted alignment logic flagged in the previous review has been corrected. Line 95 now properly assigns
isRightAligned = item.align === "right", ensuring right-aligned lists display correctly with the bullet on the right side.
- Removed the Content component and integrated its functionality directly into the NewslettersPage. - Enhanced the newsletters page with improved filtering, grouping, and rendering of newsletters. - Updated the data structure for newsletters to include new fields such as preview and contentImages. - Eliminated unused components and utility functions to streamline the codebase. - Improved the overall user experience with better layout and filtering options.
- Added support for additional image sources in the Next.js configuration. - Refactored the newsletter page to streamline content rendering and improve readability. - Removed redundant comments and unnecessary code, enhancing overall clarity and maintainability. - Introduced a new ContentImage component for better image handling within newsletters.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
apps/web/src/types/newsletter.ts (1)
1-11: Stabilize date typing (ISO) to avoid parsing pitfalls.Using loose string invites invalid Date parsing across browsers. Prefer an ISO 8601 string brand (or Date) to guarantee round‑trips and reliable sorting/formatting.
Example:
-export interface Newsletter { +type ISODateString = string; // e.g., "2024-11-15" +export interface Newsletter { id: number; title: string; - date: string; + date: ISODateString; author: string; preview: string; content: string; - image: string; + image?: string; contentImages?: string[]; takeaways: string[]; }If you adopt ISO dates, remember to format for display in the Card component.
Would you like a follow-up diff touching the Card to format ISO dates for display?
apps/web/src/app/(main)/dashboard/newsletters/page.tsx (3)
232-245: Add minimal ARIA for better a11y.Mark decorative icon as hidden; label the search and month controls.
-<Search className="absolute left-3 top-1/2 -translate-y-1/2 h-4 w-4 text-muted-foreground" /> +<Search aria-hidden="true" className="absolute left-3 top-1/2 -translate-y-1/2 h-4 w-4 text-muted-foreground" /> <Input type="text" placeholder="Search newsletters..." value={searchQuery} onChange={(e) => setSearchQuery(e.target.value)} - className="pl-10 bg-card border-border" + aria-label="Search newsletters" + className="pl-10 bg-card border-border" /> -<Select value={selectedMonth} onValueChange={setSelectedMonth}> - <SelectTrigger className="w-full sm:w-[200px] bg-card border-border"> +<Select value={selectedMonth} onValueChange={setSelectedMonth} aria-label="Filter by month"> + <SelectTrigger aria-label="Filter by month" className="w-full sm:w-[200px] bg-card border-border"> <SelectValue placeholder="Filter by month" /> </SelectTrigger>
166-171: If switching to ISO dates, format on display.Only needed if you adopt ISO strings in data.
- <span>{newsletter.date}</span> + <span> + {new Date(newsletter.date).toLocaleDateString("en-US", { + month: "short", + day: "numeric", + year: "numeric", + })} + </span>Ensure this path is covered by tests or stories once data format changes.
154-159: Removeunoptimizedand addsizesfor image optimization.Since
images.unsplash.comis already configured inremotePatternsin next.config, the Image component will optimize these images. Remove theunoptimizedprop and addsizesto enable responsive image optimization for thefilllayout:<Image src={newsletter.image} alt={newsletter.title} fill className="object-cover transition-transform duration-300 hover:scale-105" - unoptimized + sizes="(max-width: 640px) 100vw, (max-width: 1024px) 800px, 1024px" />apps/web/src/app/(main)/dashboard/newsletters/data/newsletters.ts (1)
15-32: Normalize dates to ISO 8601 format for reliable parsing and storage.All newsletter entries use non-ISO format ("Nov 15, 2024"), which relies on language-specific parsing heuristics. Convert to ISO format ("2024-11-15") and handle UI formatting in the display layer.
Apply to all entries:
- date: "Nov 15, 2024", + date: "2024-11-15",- date: "Oct 20, 2024", + date: "2024-10-20",- date: "Sep 15, 2024", + date: "2024-09-15",- date: "Dec 10, 2024", + date: "2024-12-10",- date: "Aug 1, 2024", + date: "2024-08-01",- date: "Jul 20, 2024", + date: "2024-07-20",Pair with display formatting changes in page.tsx to render these dates in user-friendly format.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/web/src/app/(main)/dashboard/newsletters/[id]/page.tsx(1 hunks)apps/web/src/app/(main)/dashboard/newsletters/data/newsletters.ts(1 hunks)apps/web/src/app/(main)/dashboard/newsletters/page.tsx(1 hunks)apps/web/src/types/newsletter.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/app/(main)/dashboard/newsletters/[id]/page.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/src/app/(main)/dashboard/newsletters/page.tsx (1)
apps/web/src/app/(main)/dashboard/newsletters/data/newsletters.ts (2)
Newsletter(1-11)newsletters(13-128)
apps/web/src/app/(main)/dashboard/newsletters/data/newsletters.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/web/next.config.js (1)
13-16: LGTM! Consider optional pathname restriction.The configuration correctly allows Next.js to load images from Unsplash for the newsletter feature. The syntax follows the existing pattern.
For slightly better security posture, you could add a
pathnameconstraint if you know the specific URL patterns used (e.g.,/photo-*). However, this is optional and only beneficial if the image URLs follow a predictable pattern:{ protocol: "https", hostname: "images.unsplash.com", + pathname: "/photo-*/**", },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/next.config.js(1 hunks)apps/web/src/app/(main)/dashboard/newsletters/[id]/page.tsx(1 hunks)apps/web/src/app/(main)/dashboard/newsletters/page.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/src/app/(main)/dashboard/newsletters/page.tsx
- apps/web/src/app/(main)/dashboard/newsletters/[id]/page.tsx
- Removed the local Newsletter interface definition and imported it from the types directory for better type management. - Simplified the getAvailableMonths function to filter out invalid dates and improve readability. - Enhanced the matchesMonthFilter function to handle invalid date cases more robustly.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/web/src/app/(main)/dashboard/newsletters/page.tsx (1)
115-121: Remove theunoptimizedprop to enable Next.js image optimization.Verification confirms that
images.unsplash.comis configured inremotePatternswithin next.config. With this configuration in place, theunoptimizedprop should be removed to leverage automatic image optimization (resizing, format conversion, lazy loading).Apply this change at
apps/web/src/app/(main)/dashboard/newsletters/page.tsxlines 115–121:<Image src={newsletter.image} alt={newsletter.title} fill className="object-cover transition-transform duration-300 hover:scale-105" - unoptimized />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/src/app/(main)/dashboard/newsletters/data/newsletters.ts(1 hunks)apps/web/src/app/(main)/dashboard/newsletters/page.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/web/src/app/(main)/dashboard/newsletters/data/newsletters.ts (1)
apps/web/src/types/newsletter.ts (1)
Newsletter(1-11)
apps/web/src/app/(main)/dashboard/newsletters/page.tsx (2)
apps/web/src/types/newsletter.ts (1)
Newsletter(1-11)apps/web/src/app/(main)/dashboard/newsletters/data/newsletters.ts (1)
newsletters(3-118)
🔇 Additional comments (4)
apps/web/src/app/(main)/dashboard/newsletters/data/newsletters.ts (1)
1-118: LGTM - Refactoring completed successfully.The previous review concerns have been addressed. The Newsletter type is now correctly imported from the centralized
@/types/newslettermodule, and the data file focuses solely on exporting the static newsletter data. The page component imports the type separately, so no re-export is needed here.apps/web/src/app/(main)/dashboard/newsletters/page.tsx (3)
19-19: LGTM - Type import centralized correctly.The Newsletter type is now imported from the centralized
@/types/newslettermodule rather than the data file, preventing type divergence as flagged in previous reviews.
56-65: LGTM - Invalid date filtering implemented.The previous review concern about guarding against invalid dates has been addressed. Line 59 filters out dates where
isNaN(d.getTime())before formatting, preventing RangeError exceptions.
77-89: LGTM - Consistent invalid date handling.The previous review concern has been addressed. Line 83 includes an invalid date check that returns
falsefor malformed dates, matching the behavior ingroupByMonth.
- Added aria-label to the search input field in the newsletters page to enhance accessibility for screen readers.
|
@prudvinani thanks for the submission! Unfortunately, we are moving with a different submission this time, so we won't be able to accept it. Still, you are welcome to make contributions! 🙏 |
2 participants
Fix : #155
scod.mp4