wip: added ui#186
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes introduce four new Svelte components for the account section, along with modifications to existing components. A layout component renders a fixed bottom navigation bar with dynamic active state management. The account page component displays a welcome message, saved articles, and highlights using a CSS grid. Additionally, a following page component provides a toggle interface between followed topics and authors, featuring a dropdown mechanism and card-based display for each category. Lastly, a continue reading component allows users to view articles in either a grid or list format. Changes
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/routes/[slug]/+page.svelteOops! Something went wrong! :( ESLint: 9.12.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@eslint/js' imported from /eslint.config.js 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
static/account-header-bg.pngis excluded by!**/*.png
📒 Files selected for processing (3)
src/routes/account/+layout.svelte(1 hunks)src/routes/account/+page.svelte(1 hunks)src/routes/account/following/+page.svelte(1 hunks)
🔇 Additional comments (4)
src/routes/account/+layout.svelte (2)
1-20: Well-structured navigation setupThe implementation of the navigation links using a structured array with labels, icons, and hrefs is clean and maintainable. This approach makes it easy to add or modify navigation items in the future.
28-38: LGTM: Active state handlingThe active state detection and styling based on the current URL pathname is implemented correctly. This provides good visual feedback to users about their current location in the application.
src/routes/account/following/+page.svelte (2)
8-9: Use Svelte reactive declarations for derived stateThe
$stateusage is correct, but consider using reactive declarations for values that can be derived from other state.
108-119: Add event handlers for card interactionsThe card components are well-structured visually, but lack click handlers or links to navigate to the respective content. Consider adding navigation functionality to these cards.
Are these cards supposed to be clickable? If so, add appropriate navigation elements:
- <div class="py-[31px] flex items-start justify-between"> + <a href="/articles/some-article-slug" class="py-[31px] flex items-start justify-between hover:opacity-90 transition">Also applies to: 156-206
There was a problem hiding this comment.
Actionable comments posted: 13
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/routes/account/+page.svelte(1 hunks)src/routes/account/continue-reading/+page.svelte(1 hunks)
🔇 Additional comments (5)
src/routes/account/+page.svelte (5)
6-6: Replace dummy data with proper data fetchingUsing
Array.from({ length: 5 })creates placeholder data with undefined values. For a production-ready component, implement proper data fetching from an API or store.
15-17: Fix typo in image source pathThere appears to be a typo in the image source filename.
- src="/contritbutors-header.png" + src="/contributors-header.png"
36-47: Move "View All" button outside of the item loopThe "View All" button appears inside each item in the grid, which means it's being repeated for each article. This is likely not the intended design.
52-56: Add loading states and error handling for imagesImages loaded from external sources might fail to load. Add appropriate error handling and loading states.
<img class="size-full object-cover group-hover:scale-110 transition will-change-transform" src="https://ghost-2077.arvensis.systems/content/images/2025/03/rsby3bojlx6dmvwsvt7a-1.webp" alt="thumbnail" + onerror="this.onerror=null; this.src='/placeholder.jpg';" + loading="lazy" />
150-311: Extract highlight card component to reduce duplicationThere's significant code duplication in the highlight cards section. Consider extracting this into a reusable component to improve maintainability.
…to feat-user-dashboard
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/routes/account/+page.svelte (1)
179-180: 🧹 Nitpick (assertive)Add responsive layout for saved articles.
The saved articles section uses a fixed grid column count without responsiveness. Consider adding responsive grid columns for better mobile experience.
-<div class="container"> - <div class="grid grid-cols-5 border-l border-[#202020]"> +<div class="container"> + <div class="grid grid-cols-1 sm:grid-cols-2 md:grid-cols-3 lg:grid-cols-4 xl:grid-cols-5 border-l border-[#202020]">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/lib/components/ui/PopularReads.svelte(1 hunks)src/routes/account/+page.svelte(1 hunks)src/routes/account/continue-reading/+page.svelte(1 hunks)
🔇 Additional comments (16)
src/lib/components/ui/PopularReads.svelte (1)
34-37: Improved error handling for category selection.This change enhances the
getPrimaryCategoryfunction to ensure it always returns a valid category. By storing the search result in a variable and providing a fallback to the first category when no primary category exists, the code now handles edge cases more effectively.src/routes/account/continue-reading/+page.svelte (8)
8-8: Replace dummy data with proper data fetching.Using
Array.from({ length: 5 })creates placeholder data with undefined values. For a production-ready component, implement proper data fetching from an API or store.-const articles = Array.from({ length: 5 }); +import { onMount } from 'svelte'; + +let articles = []; + +onMount(async () => { + try { + const response = await fetch('/api/continue-reading'); + articles = await response.json(); + } catch (error) { + console.error('Failed to fetch articles:', error); + // Implement proper error handling + } +});
46-47: Fix incorrect aria-label for grid view button.The aria-label for the grid view button incorrectly says "Switch to list view" but should say "Switch to grid view".
- aria-label="Switch to list view" + aria-label="Switch to grid view"
64-69: Move "View All" button outside of the item loop.The "View All" button appears inside each item in the grid, which means it's being repeated for each article. This is likely not the intended design.
<div class="grid grid-cols-3 gap-6"> + <div class="col-span-3 flex justify-end mb-2"> + <button + class="flex items-center gap-1 text-xs !font-mono text-neutral-20 hover:text-neutral-40 group transition" + >View All <ArrowRight + class="size-3.5 group-hover:translate-x-[2px] transition will-change-transform" + /></button + > + </div> {#each articles as article} <div> <div class="flex items-center justify-between"> <Badge class="rounded-none">Scaling</Badge> - - <button - class="flex items-center gap-1 text-xs !font-mono text-neutral-20 hover:text-neutral-40 group transition" - >View All <ArrowRight - class="size-3.5 group-hover:translate-x-[2px] transition will-change-transform" - /></button - > </div> <!-- Rest of the code -->
74-78: Add loading states and error handling for images.Images loaded from external sources might fail to load. Add appropriate error handling and loading states.
<img class="size-full object-cover group-hover:scale-110 transition will-change-transform" src="https://ghost-2077.arvensis.systems/content/images/2025/03/rsby3bojlx6dmvwsvt7a-1.webp" alt="thumbnail" + loading="lazy" + onerror="this.onerror=null; this.src='/placeholder.jpg';" />
103-103: Fix line height property value.The
leading-0property appears incorrect. In Tailwind CSS, line heights are typically specified asleading-none,leading-tight,leading-normal, etc., or with specific values likeleading-4.-<p class="w-20 text-right leading-0">40% Complete</p> +<p class="w-20 text-right leading-none">40% Complete</p>
115-120: Fix group hover effect in list view.The
group-hover:scale-110class is applied to the image, but no parent element has thegroupclass in this context, so the hover effect won't work.-<div class="w-[180px] h-[118.636px] relative"> +<div class="w-[180px] h-[118.636px] relative group"> <img class="size-full object-cover group-hover:scale-110 transition will-change-transform" src="https://ghost-2077.arvensis.systems/content/images/2025/03/rsby3bojlx6dmvwsvt7a-1.webp" alt="thumbnail" /> </div>
150-150: Fix line height property value in list view.The
leading-0property appears incorrect here as well.-<p class="w-20 text-right leading-0">40% Complete</p> +<p class="w-20 text-right leading-none">40% Complete</p>
1-160: Extract article card component to reduce duplication.There's significant code duplication between the grid and list views. Consider extracting the article display logic into a reusable component.
Create a new component
ArticleCard.svelte:<script lang="ts"> import { Badge } from '$lib/components/ui/badge'; import { cn } from '$lib/utils/ui-components'; export let viewStyle: 'GRID' | 'LIST'; export let title: string; export let date: string; export let readTime: string; export let imageUrl: string; export let category: string; export let progress: number; </script> {#if viewStyle === 'GRID'} <div> <div class="flex items-center justify-between"> <Badge>{category}</Badge> </div> <div class="mt-1 group"> <div class="h-[165px] relative overflow-hidden cursor-pointer"> <img class="size-full object-cover group-hover:scale-110 transition will-change-transform" src={imageUrl} alt="thumbnail" loading="lazy" onerror="this.onerror=null; this.src='/placeholder.jpg';" /> </div> <div class="mt-4"> <h3 class="font-powerGroteskBold text-lg leading-[18.9px] group-hover:underline underline-offset-[3px]" > {title} </h3> <div class="mt-2 text-xs font-mono divide-x divide-[#333] text-neutral-40 flex items-center gap-2" > <p>{date}</p> <p class="pl-2">{readTime}</p> </div> </div> <div class="mt-3 text-xs font-mono text-neutral-10 flex items-center justify-between gap-10" > <div class="bg-[#272728] h-2.5 flex-1 rounded-[40px] relative overflow-hidden"> <div class="bg-[#0AB2BA] absolute inset-y-0 w-1/2 rounded-[40px]" style="width: {progress}%"></div> </div> <p class="w-20 text-right leading-none">{progress}% Complete</p> </div> </div> </div> {:else} <div class="flex items-center gap-6"> <div class="w-[180px] h-[118.636px] relative group"> <img class="size-full object-cover group-hover:scale-110 transition will-change-transform" src={imageUrl} alt="thumbnail" loading="lazy" onerror="this.onerror=null; this.src='/placeholder.jpg';" /> </div> <div class="group space-y-2.5 flex-1"> <Badge class="bg-white rounded-[2px] text-neutral-80 px-1.5 py-1 h-[22px]">{category}</Badge> <h3 class="font-powerGroteskBold text-lg leading-[18.9px] group-hover:underline underline-offset-[3px] max-w-[782px]" > {title} </h3> <div class="text-xs font-mono divide-x divide-[#333] text-neutral-40 flex items-center gap-2" > <p>{date}</p> <p class="pl-2">{readTime}</p> </div> <div class="text-xs font-mono text-neutral-10 flex items-center justify-between gap-10" > <div class="bg-[#272728] h-2.5 flex-1 rounded-[40px] relative overflow-hidden max-w-[782px]" > <div class="bg-[#0AB2BA] absolute inset-y-0 w-1/2 rounded-[40px]" style="width: {progress}%"></div> </div> <p class="w-20 text-right leading-none">{progress}% Complete</p> </div> </div> </div> {/if}src/routes/account/+page.svelte (7)
94-97: Fix typo in image source path.There appears to be a typo in the image source filename.
-src="/contritbutors-header.png" +src="/contributors-header.png"
121-126: Move "View All" button outside of the item loop.The "View All" button appears inside each item in the grid, which means it's being repeated for each article. This is likely not the intended design.
<div class="mt-7 grid grid-cols-3 gap-6"> + <div class="col-span-3 flex justify-end mb-2"> + <button + class="flex items-center gap-1 text-xs !font-mono text-neutral-20 hover:text-neutral-40 group transition" + >View All <ArrowRight + class="size-3.5 group-hover:translate-x-[2px] transition will-change-transform" + /></button + > + </div> {#each Array.from({ length: 3 }) as article} <div> <div class="flex items-center justify-between"> <Badge class="rounded-none">Scaling</Badge> - - <button - class="flex items-center gap-1 text-xs !font-mono text-neutral-20 hover:text-neutral-40 group transition" - >View All <ArrowRight - class="size-3.5 group-hover:translate-x-[2px] transition will-change-transform" - /></button - > </div> <!-- Rest of the code -->
131-135: Add loading states and error handling for images.Images loaded from external sources might fail to load. Add appropriate error handling and loading states.
<img class="size-full object-cover group-hover:scale-110 transition will-change-transform" src="https://ghost-2077.arvensis.systems/content/images/2025/03/rsby3bojlx6dmvwsvt7a-1.webp" alt="thumbnail" + loading="lazy" + onerror="this.onerror=null; this.src='/placeholder.jpg';" />
160-160: Fix line height property value.The
leading-0property appears incorrect. In Tailwind CSS, line heights are typically specified asleading-none,leading-tight,leading-normal, etc., or with specific values likeleading-4.-<p class="w-20 text-right leading-0">40% Complete</p> +<p class="w-20 text-right leading-none">40% Complete</p>
171-175: Add link to "Saved Articles" section.The "Continue Reading" section has a link (
<a href>) that navigates to its dedicated page, but the "Saved Articles" section uses a<button>without navigation. For consistency, consider making this a link to a dedicated saved articles page.-<button - class="text-xl !font-powerGroteskBold inline-flex items-center gap-1.5 hover:text-neutral-20 transition group" - >Saved Articles - <!-- <ChevronRight class="size-5 group-hover:translate-x-[2px] transition will-change-transform" /> --> -</button> +<a href="/account/saved-articles" class="inline-flex items-center gap-4 group"> + <button + class="text-xl !font-powerGroteskBold inline-flex items-center gap-1.5 hover:text-neutral-20 transition group" + >Saved Articles + <ChevronRight class="size-5 group-hover:translate-x-[2px] transition will-change-transform" /> + </button> + <p class="text-[#0AB2BA] text-sm font-mono opacity-0 group-hover:opacity-100 transition"> + View all + </p> +</a>
234-238: Add link to "Highlights" section.Similar to the "Saved Articles" section, consider making the "Highlights" button a link to a dedicated highlights page for consistency with the "Continue Reading" section.
-<button - class="text-xl !font-powerGroteskBold inline-flex items-center gap-1.5 hover:text-neutral-20 transition group" - >Highlights - <ChevronRight class="size-5 group-hover:translate-x-[2px] transition will-change-transform" /> -</button> +<a href="/account/highlights" class="inline-flex items-center gap-4 group"> + <button + class="text-xl !font-powerGroteskBold inline-flex items-center gap-1.5 hover:text-neutral-20 transition group" + >Highlights + <ChevronRight class="size-5 group-hover:translate-x-[2px] transition will-change-transform" /> + </button> + <p class="text-[#0AB2BA] text-sm font-mono opacity-0 group-hover:opacity-100 transition"> + View all + </p> +</a>
241-402: Extract highlight card component to reduce duplication.There's significant code duplication in the highlight cards section. Consider extracting this into a reusable component to improve maintainability.
Create a new component
HighlightCard.svelte:<script lang="ts"> export let title: string; export let author: string; export let content: string; export let imageUrl: string; </script> <div class="bg-[#161617] pt-4 px-5 pb-5 rounded-[8px] group"> <div class="flex items-center gap-[14px]"> <div class="size-14 rounded-[8px] flex-shrink-0 relative overflow-hidden"> <img class="size-full object-cover group-hover:scale-110 transition will-change-transform" src={imageUrl} alt="thumbnail" /> </div> <div> <h3 class="font-powerGroteskBold leading-[17.6px]"> {title} </h3> <p class="text-xs text-neutral-40 font-mono mt-[2px]">by {author}</p> </div> </div> <p class="mt-4 text-sm text-neutral-10"> {content} </p> </div>
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (2)
src/routes/[slug]/+page.svelte (2)
118-169: 🧹 Nitpick (assertive)Optimize multiple database queries for reading progress.
Within the
updateReadingProgressfunction, eachReadHistoryrecord is fetched and updated with separate queries. If the user scrolls frequently, these calls could stack up. Consider updating progress less often or batching updates to reduce load on your database.
64-64: 🧹 Nitpick (assertive)Typographical fix – 'occured' → 'occurred'.
Minor spelling mistake in the error message could be corrected for clarity.
- throw error(500, 'An error occured'); + throw error(500, 'An error occurred');
♻️ Duplicate comments (14)
src/routes/account/continue-reading/+page.svelte (2)
120-123:⚠️ Potential issueMismatch in aria-label for the toggle button.
The second toggle button calls
viewStyle = 'GRID'but has the same aria-label text as the first. For accessibility, change it to "Switch to grid view."- aria-label="Switch to list view" + aria-label="Switch to grid view"
80-82: 🧹 Nitpick (assertive)Consider consistent navigation logic.
Using
history.back()may behave unexpectedly if the user arrived from a different route. For consistency with line 57, use SvelteKit'sgototo direct users to a known fallback (e.g.,/account).- const handleBackNavigate = () => { - history.back(); - }; + const handleBackNavigate = () => { + goto('/account'); + };src/routes/account/+page.svelte (12)
104-108: 🧹 Nitpick (assertive)Extract shared utility function for code reuse.
This
getPrimaryCategoryfunction appears to be duplicated across components according to previous review comments. It should be extracted to a shared utility file.-function getPrimaryCategory(article: ArticleMetadata) { - const primary = article.categories.find((category) => category.is_primary); - - return primary ?? article.categories[0]; -} +import { getPrimaryCategory } from '$lib/utils/article-helpers';
129-132:⚠️ Potential issueFix typo in image source path.
There's a typo in the image source filename.
-src="/contritbutors-header.png" +src="/contributors-header.png"
156-176: 🛠️ Refactor suggestionMove "View All" button outside of the loop.
The "View All" button appears inside each article in the grid, causing it to be repeated multiple times. It should be outside the loop for better UX.
<div class="mt-7 grid grid-cols-3 gap-6"> + <div class="col-span-3 flex justify-end mb-2"> + <a + href={`/category`} + data-sveltekit-preload-data + > + <button + class="flex items-center gap-1 text-xs !font-mono text-neutral-20 hover:text-neutral-40 group transition" + >View All <ArrowRight + class="size-3.5 group-hover:translate-x-[2px] transition will-change-transform" + /></button + > + </a> + </div> {#each historyArticles as article} {@const primaryCategory = getPrimaryCategory(article)?.name} {@const formattedDate = format(article.created_at, 'dd MMM yyyy')} {@const progress = Math.floor(Number(article.progress))} <div> <div class="flex items-center justify-between"> <a href={`/category/${formatCategorySlug(primaryCategory)}`} data-sveltekit-preload-data > <Badge class="rounded-none">{primaryCategory}</Badge></a > - - <a - href={`/category/${formatCategorySlug(primaryCategory)}`} - data-sveltekit-preload-data - > - <button - class="flex items-center gap-1 text-xs !font-mono text-neutral-20 hover:text-neutral-40 group transition" - >View All <ArrowRight - class="size-3.5 group-hover:translate-x-[2px] transition will-change-transform" - /></button - ></a - > </div>
214-216: 🧹 Nitpick (assertive)Fix line height property value.
The
leading-0property is incorrect. In Tailwind CSS, line heights should be specified using valid values.-<p class="w-20 text-right leading-0"> +<p class="w-20 text-right leading-none"> {progress}% Complete </p>
227-232: 🧹 Nitpick (assertive)Add link to "Saved Articles" section.
For consistency with the "Continue Reading" section, make this a link to a dedicated saved articles page.
-<button - class="text-xl !font-powerGroteskBold inline-flex items-center gap-1.5 hover:text-neutral-20 transition group" - >Saved Articles - <!-- <ChevronRight class="size-5 group-hover:translate-x-[2px] transition will-change-transform" /> --> -</button> +<a href="/account/saved-articles" class="inline-flex items-center gap-4 group"> + <button + class="text-xl !font-powerGroteskBold inline-flex items-center gap-1.5 hover:text-neutral-20 transition group" + >Saved Articles + <ChevronRight class="size-5 group-hover:translate-x-[2px] transition will-change-transform" /> + </button> + <p class="text-[#0AB2BA] text-sm font-mono opacity-0 group-hover:opacity-100 transition"> + View all + </p> +</a>
237-284: 🧹 Nitpick (assertive)Add responsive layout for saved articles grid.
The saved articles section uses a fixed 5-column grid which won't work well on smaller screens.
-<div class="grid grid-cols-5 border-l border-[#202020]"> +<div class="grid grid-cols-1 sm:grid-cols-2 md:grid-cols-3 lg:grid-cols-4 xl:grid-cols-5 border-l border-[#202020]">
243-246: 🧹 Nitpick (assertive)Remove commented-out code.
Commented-out code should be removed as it creates confusion and clutters the file.
<div class={cn( 'px-5 pt-[31px] pb-[50px] border-r border-[#202020]' - // i === 0 && 'border-l border-[#202020]', - // savedArticles.length - 1 === i && '!border-r border-[#202020]' )} >
291-295: 🧹 Nitpick (assertive)Add link to "Highlights" section.
For consistency with other sections, make the highlights button a link to a dedicated page.
-<button - class="text-xl !font-powerGroteskBold inline-flex items-center gap-1.5 hover:text-neutral-20 transition group" - >Highlights - <ChevronRight class="size-5 group-hover:translate-x-[2px] transition will-change-transform" /> -</button> +<a href="/account/highlights" class="inline-flex items-center gap-4 group"> + <button + class="text-xl !font-powerGroteskBold inline-flex items-center gap-1.5 hover:text-neutral-20 transition group" + >Highlights + <ChevronRight class="size-5 group-hover:translate-x-[2px] transition will-change-transform" /> + </button> + <p class="text-[#0AB2BA] text-sm font-mono opacity-0 group-hover:opacity-100 transition"> + View all + </p> +</a>
298-459: 🛠️ Refactor suggestionExtract highlight card component to reduce duplication.
There's significant code duplication in the highlight cards section. Consider extracting this into a reusable component.
Create a new
HighlightCard.sveltecomponent and use it for all highlight cards to improve maintainability.
302-306: 🧹 Nitpick (assertive)Add loading states and error handling for images.
Images loaded from external sources might fail to load. Add appropriate error handling and loading states.
<img class="size-full object-cover group-hover:scale-110 transition will-change-transform" src="https://ghost-2077.arvensis.systems/content/images/2025/03/rsby3bojlx6dmvwsvt7a-1.webp" alt="thumbnail" + onerror="this.onerror=null; this.src='/placeholder.jpg';" + loading="lazy" />
298-459: 🛠️ Refactor suggestionReplace hardcoded highlights data with proper data fetching.
The highlights section uses hardcoded data. For a production-ready component, implement proper data fetching from an API or store.
+ let highlights = $state<any[]>([]); + + const getHighlights = async (userId: string) => { + try { + const { data, error: highlightsError } = await supabase + .from('UserHighlights') + .select('*') + .eq('userId', userId) + .order('created_at', { ascending: false }); + + if (data) { + highlights = data; + } + + if (highlightsError) { + toast.error('Error fetching highlights'); + } + } catch (err) { + console.error(err); + toast.error('An error occurred fetching highlights'); + } + }; + + // Update handleDataFetch to include: + // await getHighlights(user.id);Then in the template:
<div class="mt-7 grid grid-cols-3 gap-4"> {#if highlights.length === 0} <div class="col-span-3 text-center py-10"> <p class="text-lg text-neutral-40">You haven't highlighted any content yet.</p> </div> {:else} {#each highlights as highlight} <!-- Highlight card component --> {/each} {/if} </div>
238-284: 🧹 Nitpick (assertive)Add empty state handling for saved articles.
The component doesn't handle cases where
savedArticlesis empty, which could lead to an empty grid with no user feedback.<div class="grid grid-cols-5 border-l border-[#202020]"> + {#if savedArticles.length === 0} + <div class="col-span-5 text-center py-10"> + <p class="text-lg text-neutral-40">You haven't saved any articles yet.</p> + <a href="/discover" class="text-[#0AB2BA] mt-2 inline-block hover:underline">Discover articles</a> + </div> + {:else} {#each savedArticles as article, i} <!-- Existing article rendering --> {/each} + {/if} </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
src/lib/components/ui/ArticleBottomBar.svelte(1 hunks)src/routes/[slug]/+page.svelte(2 hunks)src/routes/account/+page.svelte(1 hunks)src/routes/account/continue-reading/+page.svelte(1 hunks)src/routes/bookmarks/+page.svelte(0 hunks)
💤 Files with no reviewable changes (1)
- src/routes/bookmarks/+page.svelte
🔇 Additional comments (5)
src/lib/components/ui/ArticleBottomBar.svelte (1)
27-27: Prevent unnecessary scroll handling on desktop.This newly added early return is a good optimization to avoid executing unnecessary logic on non-mobile devices.
src/routes/account/continue-reading/+page.svelte (4)
149-159: Repeated 'View All' button within the loop.This button appears for each article, likely causing duplication and user confusion. Move the "View All" button outside the individual item loop to avoid repeated rendering.
198-200: Use valid Tailwind line-height values.The class
leading-0is typically invalid. Tailwind usesleading-none,leading-tight, etc. This issue was flagged previously.- <p class="w-20 text-right leading-0"> + <p class="w-20 text-right leading-none">
218-219: Missing parent 'group' class for hover effect.This image uses
group-hover:scale-110but is missing a.groupclass on a wrapping container, so the hover effect won’t trigger.- <div class="w-[180px] h-[118.636px] relative"> + <div class="w-[180px] h-[118.636px] relative group">
262-262: Use valid Tailwind line-height utilities.Again,
leading-0is invalid. Replace with a valid Tailwind utility, e.g.,leading-none.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
src/routes/account/+page.svelte (2)
125-125: 🛠️ Refactor suggestionUse dynamic username instead of hardcoded welcome message
The welcome message should include the user's name for a personalized experience.
-<h1 class="text-[32px] font-powerGroteskBold">Welcome back</h1> +<h1 class="text-[32px] font-powerGroteskBold">Welcome back, {user?.user_metadata?.full_name || 'there'}</h1>
214-216:⚠️ Potential issueFix incorrect line height property
The
leading-0property is not a valid Tailwind class. Use a proper line height value.-<p class="w-20 text-right leading-0"> +<p class="w-20 text-right leading-none"> {progress}% Complete </p>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
src/lib/components/ui/Nav.svelte(3 hunks)src/lib/components/ui/PopularReads.svelte(2 hunks)src/routes/account/+page.svelte(1 hunks)src/routes/account/continue-reading/+page.svelte(1 hunks)
🔇 Additional comments (28)
src/lib/components/ui/PopularReads.svelte (2)
35-37: Consider handling empty categories
Ifarticle.categorieshappens to be empty, accessingarticle.categories[0]may yield an undefined value and cause a runtime error. Please ensure thatarticle.categoriesis non-empty or handle this scenario with a fallback.
60-62: Heading style update looks good
No concerns here. The new font class and text changes are clear and consistent.src/lib/components/ui/Nav.svelte (2)
3-5: Import additions
These new imports (Supabase and Lucide icons) align well with subsequent usage.
57-83: Verify user metadata checks
While referencinguser.user_metadata.email_verified, ensureuser_metadatais always defined to prevent potential runtime errors if the structure is missing or null.src/routes/account/continue-reading/+page.svelte (14)
1-15: Imports look fine
No issues noted with these import statements—they appear consistent with usage throughout this component.
16-17: Type definition is clear
DefiningArticleWithProgressis a straightforward way to track reading progress.
18-19: Appropriate state variables
Using$stateforhistoryArticlesandisLoadingis a logical approach for maintaining component state.
21-42: Potential performance bottleneck
CallinggetGhostArticleBySluginside a loop can be slow for large histories. Consider batching or handling multiple slugs in a single fetch.
44-66: Correct the spelling
A minor spelling oversight: use "occurred" instead of "occured" in the error message.
68-72: Handle empty categories
As in similar logic elsewhere, callingarticle.categories[0]could fail ifcategoriesis empty. Consider a fallback or guard.
74-76: Data fetch on mount
TriggeringhandleDataFetch()within$effectis valid here; no further issues found.
78-82: Use predictable navigation
history.back()can be unreliable. Considergoto('/account')or another fixed route for a more stable user experience.
85-91: Loading overlay implementation
This loading state is straightforward and provides clear feedback during data retrieval.
122-123: Aria label mismatch
This button toggles to the grid view, but its label says "Switch to list view."
149-159: "View All" button duplication
The "View All" button appears for each item. If it's meant to show all articles, consider placing it outside the loop to avoid repetition.
198-200: Replace non-standardleading-0
Tailwind CSS doesn't provideleading-0. Useleading-noneor an alternative valid utility.
217-219: Missing.groupon parent
.group-hover:scale-110won't apply without a.groupclass on the containing element.
262-262: Invalid line-height utility
leading-0is not a recognized Tailwind class; redefine asleading-noneor another appropriate value.src/routes/account/+page.svelte (10)
104-108: Reuse the enhanced getPrimaryCategory function from PopularReadsThis function implementation is identical to the one in PopularReads.svelte. Consider extracting this to a utility function to maintain DRY principles.
-function getPrimaryCategory(article: ArticleMetadata) { - const primary = article.categories.find((category) => category.is_primary); - - return primary ?? article.categories[0]; -} +import { getPrimaryCategory } from '$lib/utils/article-helpers';
129-132: Fix typo in image source pathThere appears to be a typo in the image source filename.
- src="/contritbutors-header.png" + src="/contributors-header.png"
150-222: Add empty state handling for history articlesThe component doesn't handle cases where there are no recently read articles, which could lead to an empty view without user feedback.
<div class="mt-7 grid grid-cols-1 md:grid-cols-2 lg:grid-cols-3 gap-6"> + {#if historyArticles.length === 0} + <div class="col-span-1 md:col-span-2 lg:col-span-3 text-center py-10"> + <p class="text-lg text-neutral-40">You haven't read any articles yet.</p> + <a href="/discover" class="text-[#0AB2BA] mt-2 inline-block hover:underline">Discover articles</a> + </div> + {:else} {#each historyArticles as article} {@const primaryCategory = getPrimaryCategory(article)?.name} {@const formattedDate = format(article.created_at, 'dd MMM yyyy')} {@const progress = Math.floor(Number(article.progress))} <!-- Existing article rendering --> {/each} + {/if} </div>
226-232: Add link to "Saved Articles" sectionSimilar to the "Continue Reading" section, the "Saved Articles" heading should be a link to a dedicated page.
-<button - class="text-xl !font-powerGroteskBold inline-flex items-center gap-1.5 hover:text-neutral-20 transition group" - >Saved Articles - <!-- <ChevronRight class="size-5 group-hover:translate-x-[2px] transition will-change-transform" /> --> -</button> +<a href="/account/saved-articles" class="inline-flex items-center gap-4 group"> + <button + class="text-xl !font-powerGroteskBold inline-flex items-center gap-1.5 hover:text-neutral-20 transition group" + >Saved Articles + <ChevronRight class="size-5 group-hover:translate-x-[2px] transition will-change-transform" /> + </button> + <p class="text-[#0AB2BA] text-sm font-mono opacity-0 group-hover:opacity-100 transition"> + View all + </p> +</a>
238-239: Add empty state handling for saved articlesThe component doesn't handle cases where there are no saved articles, which could lead to an empty view without user feedback.
<div class="grid grid-cols-1 md:grid-cols-3 lg:grid-cols-4 xl:grid-cols-5 border-l border-[#202020]" > + {#if savedArticles.length === 0} + <div class="col-span-1 md:col-span-3 lg:col-span-4 xl:col-span-5 text-center py-10 px-5"> + <p class="text-lg text-neutral-40">You haven't saved any articles yet.</p> + <a href="/discover" class="text-[#0AB2BA] mt-2 inline-block hover:underline">Discover articles</a> + </div> + {:else} {#each savedArticles as article, i} <!-- Existing code --> {/each} + {/if}
287-291: Add link to "Highlights" sectionSimilar to the "Saved Articles" section, consider making the "Highlights" button a link to a dedicated highlights page for consistency with the "Continue Reading" section.
-<button - class="text-xl !font-powerGroteskBold inline-flex items-center gap-1.5 hover:text-neutral-20 transition group" - >Highlights - <ChevronRight class="size-5 group-hover:translate-x-[2px] transition will-change-transform" /> -</button> +<a href="/account/highlights" class="inline-flex items-center gap-4 group"> + <button + class="text-xl !font-powerGroteskBold inline-flex items-center gap-1.5 hover:text-neutral-20 transition group" + >Highlights + <ChevronRight class="size-5 group-hover:translate-x-[2px] transition will-change-transform" /> + </button> + <p class="text-[#0AB2BA] text-sm font-mono opacity-0 group-hover:opacity-100 transition"> + View all + </p> +</a>
294-455: Extract highlight card component to reduce duplicationThere's significant code duplication in the highlight cards section. Consider extracting this into a reusable component.
Create a new component
HighlightCard.svelte:<script lang="ts"> export let title: string; export let author: string; export let content: string; export let imageUrl: string; </script> <div class="bg-[#161617] pt-4 px-5 pb-5 rounded-[8px] group"> <div class="flex items-center gap-[14px]"> <div class="size-14 rounded-[8px] flex-shrink-0 relative overflow-hidden"> <img class="size-full object-cover group-hover:scale-110 transition will-change-transform" src={imageUrl} alt={`Thumbnail for article: ${title}`} loading="lazy" /> </div> <div> <h3 class="font-powerGroteskBold leading-[17.6px]"> {title} </h3> <p class="text-xs text-neutral-40 font-mono mt-[2px]">by {author}</p> </div> </div> <p class="mt-4 text-sm text-neutral-10"> {content} </p> </div>Then replace the repeated highlight sections with:
<div class="mt-7 grid grid-cols-1 md:grid-cols-2 lg:grid-cols-3 gap-4"> <HighlightCard title="DePIN On Ethereum: Redefining Coordination Systems" author="sm-stack" content="Also known as 'soft confirmations', the sequencer's confirmation of transaction success (or failure) are useful for time-sensitive users who want to avoid waiting for transactions to finalize on Ethereum L1 before knowing a transaction's status." imageUrl="https://ghost-2077.arvensis.systems/content/images/2025/03/rsby3bojlx6dmvwsvt7a-1.webp" /> <!-- Repeat for other highlight cards --> </div>
39-52: Improve error handling for bookmarksThe current error handling has a specific case for 'PGRST116' (no results found) but could be improved to handle more specific error scenarios.
if (dataError) { if (dataError.code === 'PGRST116') { const { error: createError } = await supabase .from('UserBookmarks') .insert({ userId: id, articleIds: [] }); if (createError) { - toast.error('An error occured. Please try again.'); + console.error('Error creating bookmarks record:', createError); + toast.error('Could not initialize bookmarks. Please try again later.'); } + } else if (dataError.code === 'PGRST104') { + toast.error('Database connection error. Please try again later.'); } else { - toast.error('Error fetching bookmarks'); + console.error('Error fetching bookmarks:', dataError); + toast.error('Could not load your bookmarks. Please try again later.'); throw error(500, 'Error fetching bookmarks'); } }
17-19: Consider initializing loading state to false by defaultCurrently,
isLoadingis initialized totrue, but it's immediately set totrueagain inhandleDataFetch. This could cause UI flashing if there's any delay between component mount and the effect execution.-let isLoading = $state(true); +let isLoading = $state(false);
298-302: Add loading states and error handling for imagesImages loaded from external sources might fail to load. Add appropriate error handling and loading states.
<img class="size-full object-cover group-hover:scale-110 transition will-change-transform" src="https://ghost-2077.arvensis.systems/content/images/2025/03/rsby3bojlx6dmvwsvt7a-1.webp" alt="thumbnail" + onerror="this.onerror=null; this.src='/placeholder.jpg';" + loading="lazy" />
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/routes/account/continue-reading/+page.svelte (2)
125-126:⚠️ Potential issueCorrect aria-label to reflect 'Switch to grid view'
When the button setsviewStyle = 'GRID', the aria-label should say "Switch to grid view" for accurate accessibility.- aria-label="Switch to list view" + aria-label="Switch to grid view"
65-65: 🧹 Nitpick (assertive)Fix typo in error message
You have "An error occured" in the thrown error message. Please replace with "occurred" for correctness.- throw error(500, 'An error occured'); + throw error(500, 'An error occurred');src/routes/account/+page.svelte (1)
144-155:⚠️ Potential issueAvoid nesting button inside an anchor
Nesting a<button>inside an<a>is invalid HTML and may cause accessibility issues. Use one interactive element.-<a href="/account/continue-reading" class="inline-flex items-center gap-4 group"> - <button - class="text-xl !font-powerGroteskBold inline-flex items-center gap-1.5 hover:text-neutral-20 transition group" - > - Continue Reading - <ChevronRight - class="size-5 group-hover:translate-x-[2px] transition will-change-transform" - /> - </button> - <p class="text-[#0AB2BA] text-sm font-mono opacity-0 group-hover:opacity-100 transition"> - View all - </p> -</a> +<a + href="/account/continue-reading" + class="text-xl !font-powerGroteskBold inline-flex items-center gap-1.5 hover:text-neutral-20 transition group" +> + Continue Reading + <ChevronRight class="size-5 group-hover:translate-x-[2px] transition will-change-transform" /> +</a> +<p class="text-[#0AB2BA] text-sm font-mono opacity-0 group-hover:opacity-100 transition"> + View all +</p>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/routes/account/+page.svelte(1 hunks)src/routes/account/continue-reading/+page.svelte(1 hunks)
🔇 Additional comments (6)
src/routes/account/continue-reading/+page.svelte (2)
221-221: Use valid Tailwind line-height utilities
Theleading-0class is not recognized by Tailwind. Consider usingleading-none,leading-tight, or a similar valid utility class.- <p class="w-20 text-right leading-0"> + <p class="w-20 text-right leading-none">Also applies to: 265-265
156-162: Move "View All" button outside the loop
Placing "View All" inside each article block repeats it unnecessarily. It's best to place such a navigation button outside the loop.src/routes/account/+page.svelte (4)
136-136: Fix filename typo
Replace"contritbutors-header.png"with"contributors-header.png"to match the correct spelling.- src="/contritbutors-header.png" + src="/contributors-header.png"
221-221: Use valid Tailwind line-height utility
leading-0is not recognized and should be replaced with a valid utility likeleading-none.- <p class="w-20 text-right leading-0"> + <p class="w-20 text-right leading-none">
236-239: Use a link for "Saved Articles"
For consistency with the "Continue Reading" section, consider making this a link to a dedicated page.-<button - class="text-xl !font-powerGroteskBold inline-flex items-center gap-1.5 hover:text-neutral-20 transition group" ->Saved Articles +<a + class="text-xl !font-powerGroteskBold inline-flex items-center gap-1.5 hover:text-neutral-20 transition group" + href="/account/saved-articles" +>Saved Articles
295-298: Add a link for "Highlights"
Rather than a plain button, use a link for "Highlights" to match the pattern used elsewhere for navigation.-<button - class="text-xl !font-powerGroteskBold inline-flex items-center gap-1.5 hover:text-neutral-20 transition group" ->Highlights -<ChevronRight class="size-5 group-hover:translate-x-[2px] transition will-change-transform" /> -</button> +<a + href="/account/highlights" + class="text-xl !font-powerGroteskBold inline-flex items-center gap-1.5 hover:text-neutral-20 transition group" +> + Highlights + <ChevronRight class="size-5 group-hover:translate-x-[2px] transition will-change-transform" /> +</a>
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (5)
src/lib/services/ghost.service.ts (5)
5-11: 🧹 Nitpick (assertive)Consider documenting the userCache parameter purpose
While the implementation is correct, adding a JSDoc comment would make the function's behavior more explicit to other developers.
export const fetchGhostArticles = async ( category?: string, page = 1, limit = 15, bookmarks?: string[], + /** Controls whether to use cached data (true) or always fetch fresh data (false) */ userCache = true ) => {
12-19: 🛠️ Refactor suggestionPotential issue with cache key not including bookmarks
The cache key is generated using only category, page, and limit, but not bookmarks. This could lead to incorrect cached data being returned if different bookmarks are provided with the same other parameters.
- const cacheKey = `${category || 'all'}-${page || 1}-${limit}`; + const bookmarksKey = bookmarks?.length ? `-bookmarks:${bookmarks.join(',')}` : ''; + const cacheKey = `${category || 'all'}-${page || 1}-${limit}${bookmarksKey}`;
26-27:⚠️ Potential issuePotential filter conflict with both category and bookmarks
When both category and bookmarks are provided, the current implementation might cause one filter to override the other since both are setting the same
filterproperty in the API request object.- ...(category && { filter: `tag:${category}` }), - ...((bookmarks || []).length > 0 && { filter: `slug:[${bookmarks}]` }) + filter: [ + ...(category ? [`tag:${category}`] : []), + ...((bookmarks || []).length > 0 ? [`slug:[${bookmarks}]`] : []) + ].join('+')
57-93: 🧹 Nitpick (assertive)Consider applying the userCache parameter to fetchGhostArticlesFilters for consistency
For code consistency, consider adding the
userCacheparameter to thefetchGhostArticlesFiltersfunction as well, following the same pattern implemented infetchGhostArticles.-export const fetchGhostArticlesFilters = async (categories: string[], page = 1, limit = 6) => { +export const fetchGhostArticlesFilters = async (categories: string[], page = 1, limit = 6, userCache = true) => { const cacheKey = `${categories.join(',') || 'all'}-${page || 1}-${limit}`; - if (cache.has(cacheKey)) { + if (userCache && cache.has(cacheKey)) { const cached = cache.get(cacheKey); if (cached && isCacheValid(cached.timestamp)) { return cached.data; } }
38-38: 🧹 Nitpick (assertive)Consider respecting userCache for cache setting too
Currently, the function still sets the cache even when
userCacheis false. Consider making the caching behavior consistent by also skipping the cache setting operation whenuserCacheis false.- cache.set(cacheKey, { data: articles, timestamp: Date.now() }); + if (userCache) { + cache.set(cacheKey, { data: articles, timestamp: Date.now() }); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/lib/services/ghost.service.ts(1 hunks)src/routes/account/+page.svelte(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/lib/services/ghost.service.ts (1)
src/lib/utils/cache.ts (1)
cache(1-1)
🔇 Additional comments (8)
src/lib/services/ghost.service.ts (2)
9-10: New parameter enables cache controlThe addition of the
userCacheparameter with a default value oftrueallows callers to bypass the cache when needed, which is a useful enhancement for scenarios where fresh data is required.
14-14: Cache check now respects the userCache parameterGood implementation of the conditional cache check that only retrieves from cache when
userCacheis true.src/routes/account/+page.svelte (6)
46-46: Correct the spelling of “occured.”
Consider changing “occured” to “occurred,” as previously noted.
109-109: Spelling fix needed
Replace "An error occured" with "An error occurred" to align with proper spelling.
147-157: Avoid embedding a within an element
This pattern violates HTML standards and can lead to accessibility issues. Use a single clickable element.
223-223: Use valid Tailwind line-height classes
leading-0is likely incorrect. Replace with a supported class such asleading-none.
237-241: Convert “Saved Articles” button to a link
For consistent navigation, consider using an anchor tag instead of a button here.
302-464: Extract highlight card code into a reusable component
Repeating the same markup for each highlight is prone to duplication. Extracting aHighlightCardcomponent would simplify maintenance.
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
src/routes/[slug]/+page.svelte (1)
118-169: 🧹 Nitpick (assertive)Improve reading progress tracking with database persistence.
The updated implementation adds proper debouncing with timeout management and database persistence for reading progress. This ensures that reading position is saved only when needed (after 2 seconds of inactivity) and only for logged-in users.
A few observations:
- Good error handling for when the record doesn't exist (PGRST116 error code)
- Proper cleanup of timeouts to prevent memory leaks
Consider extracting the database operations into a separate function to improve readability:
async function updateReadingProgress() { const article = document.querySelector('article'); if (!article) return; const windowHeight = window.innerHeight; const articleHeight = article.offsetHeight; const scrollTop = window.scrollY; const totalHeight = articleHeight - windowHeight; const currentProgress = (scrollTop / totalHeight) * 100; progress = Math.min(Math.max(currentProgress, 0), 100); if (isLoggedIn) { if (timeoutId) { clearTimeout(timeoutId); } timeoutId = setTimeout(async () => { - const { data, error } = await supabase - .from('ReadHistory') - .select('*') - .eq('userId', userId) - .eq('articleId', $slug) - .limit(1) - .single(); - - if (data) { - const { error } = await supabase - .from('ReadHistory') - .update({ progress }) - .eq('userId', userId) - .eq('articleId', $slug) - .select(); - - if (error) { - console.error('Failed to update progress:', error); - } - } - - if (error) { - if (error.code === 'PGRST116') { - const { error: createError } = await supabase - .from('ReadHistory') - .insert({ userId, articleId: $slug, progress }); - - if (createError) { - console.error('Failed to update progress:', error); - } - } - } + await saveReadingProgress(userId, $slug, progress); }, 2000); } } + async function saveReadingProgress(userId: string, articleId: string, progress: number) { + const { data, error } = await supabase + .from('ReadHistory') + .select('*') + .eq('userId', userId) + .eq('articleId', articleId) + .limit(1) + .single(); + + if (data) { + const { error } = await supabase + .from('ReadHistory') + .update({ progress }) + .eq('userId', userId) + .eq('articleId', articleId) + .select(); + + if (error) { + console.error('Failed to update progress:', error); + } + } + + if (error) { + if (error.code === 'PGRST116') { + const { error: createError } = await supabase + .from('ReadHistory') + .insert({ userId, articleId, progress }); + + if (createError) { + console.error('Failed to update progress:', error); + } + } + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/routes/[slug]/+page.svelte(6 hunks)
🔇 Additional comments (6)
src/routes/[slug]/+page.svelte (6)
115-116: Good addition of a derived store for the slug.Using a derived store to extract the article slug from the URL is a good practice, making the slug reactive and available throughout the component.
838-845: Good state management for the new highlight feature.The state variables for the highlighting feature are well-defined using Svelte's reactive state system.
846-857: Well-implemented selection check function.The
hasActiveSelectionfunction properly checks if there is valid text selected within the article element.
891-904: Good utility function to get selection offsets.This helper function cleanly extracts the start and end offsets of the current selection, which will be useful for storing highlights.
931-945: Good event listener management with proper cleanup.The effect correctly adds and removes event listeners when the component mounts and unmounts, which prevents memory leaks.
1523-1553: Good binding of the article element for the highlighting feature.The
bind:this={articleRef}properly connects the article DOM element to the variable used for checking text selections. This is an essential part of making the highlighting feature work correctly.
| <!-- Hightlight menu --> | ||
| {#if menuVisible} | ||
| <div | ||
| class="absolute rounded-2xl border border-[#333] bg-[#19191b] z-[9999] text-sm" | ||
| style="top: {menuY}px; left: {menuX}px;" | ||
| > | ||
| <button class="!font-mono p-1" onclick={highlightText}>Highlight</button> | ||
| <!-- <button>Respond</button> | ||
| <button>Share</button> | ||
| <button>Private note</button> --> | ||
| </div> | ||
| {/if} |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Implement the remaining highlighting UI functionality.
The highlighting menu is implemented but only has a single functional button. The other buttons are commented out, suggesting incomplete functionality.
Consider implementing at least some of the basic functionality for these buttons, or removing the commented code if they're not planned for the current release:
<!-- Hightlight menu -->
{#if menuVisible}
<div
class="absolute rounded-2xl border border-[#333] bg-[#19191b] z-[9999] text-sm"
style="top: {menuY}px; left: {menuX}px;"
>
<button class="!font-mono p-1" onclick={highlightText}>Highlight</button>
- <!-- <button>Respond</button>
- <button>Share</button>
- <button>Private note</button> -->
+ <button class="!font-mono p-1" onclick={() => {
+ const text = encodeURIComponent(selectionText);
+ const url = `https://twitter.com/intent/tweet?text="${text}" - from article: ${encodedUrl}`;
+ window.open(url, '_blank');
+ menuVisible = false;
+ }}>Share</button>
</div>
{/if}📝 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.
| <!-- Hightlight menu --> | |
| {#if menuVisible} | |
| <div | |
| class="absolute rounded-2xl border border-[#333] bg-[#19191b] z-[9999] text-sm" | |
| style="top: {menuY}px; left: {menuX}px;" | |
| > | |
| <button class="!font-mono p-1" onclick={highlightText}>Highlight</button> | |
| <!-- <button>Respond</button> | |
| <button>Share</button> | |
| <button>Private note</button> --> | |
| </div> | |
| {/if} | |
| <!-- Hightlight menu --> | |
| {#if menuVisible} | |
| <div | |
| class="absolute rounded-2xl border border-[#333] bg-[#19191b] z-[9999] text-sm" | |
| style="top: {menuY}px; left: {menuX}px;" | |
| > | |
| <button class="!font-mono p-1" onclick={highlightText}>Highlight</button> | |
| <button class="!font-mono p-1" onclick={() => { | |
| const text = encodeURIComponent(selectionText); | |
| const url = `https://twitter.com/intent/tweet?text="${text}" - from article: ${encodedUrl}`; | |
| window.open(url, '_blank'); | |
| menuVisible = false; | |
| }}>Share</button> | |
| </div> | |
| {/if} |
| const handleClickOutsideSelection = (event: MouseEvent): void => { | ||
| const target = event.target as HTMLElement; | ||
|
|
||
| const active = hasActiveSelection(); | ||
|
|
||
| if ( | ||
| !articleRef || | ||
| (!target.closest('.menu') && !articleRef.contains(target)) || | ||
| (menuVisible && target.closest('.menu') && articleRef.contains(target)) | ||
| ) { | ||
| menuVisible = false; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Improve the click outside detection logic.
The current implementation has complex conditions that could be simplified. Consider using a clearer approach for determining when to hide the menu.
const handleClickOutsideSelection = (event: MouseEvent): void => {
const target = event.target as HTMLElement;
const active = hasActiveSelection();
- if (
- !articleRef ||
- (!target.closest('.menu') && !articleRef.contains(target)) ||
- (menuVisible && target.closest('.menu') && articleRef.contains(target))
- ) {
- menuVisible = false;
- }
+ // Hide menu if we click outside the menu and selection is no longer active
+ if (menuVisible && !target.closest('.menu') && !active) {
+ menuVisible = false;
+ }
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleClickOutsideSelection = (event: MouseEvent): void => { | |
| const target = event.target as HTMLElement; | |
| const active = hasActiveSelection(); | |
| if ( | |
| !articleRef || | |
| (!target.closest('.menu') && !articleRef.contains(target)) || | |
| (menuVisible && target.closest('.menu') && articleRef.contains(target)) | |
| ) { | |
| menuVisible = false; | |
| } | |
| }; | |
| const handleClickOutsideSelection = (event: MouseEvent): void => { | |
| const target = event.target as HTMLElement; | |
| const active = hasActiveSelection(); | |
| // Hide menu if we click outside the menu and selection is no longer active | |
| if (menuVisible && !target.closest('.menu') && !active) { | |
| menuVisible = false; | |
| } | |
| }; |
| const highlightText = (): void => { | ||
| if (!selectionText) return; | ||
|
|
||
| const selectionRange = getSelectionOffsets(); | ||
|
|
||
| console.log('Highlighting text:', selectionText, selectionRange); | ||
| // Implement storing `selectionText`, startOffset, endOffset in DB logic here | ||
|
|
||
| menuVisible = false; | ||
| }; | ||
|
|
There was a problem hiding this comment.
Complete the highlight text implementation.
The highlightText function is currently logging but not implementing the actual highlight functionality. The TODO comment indicates that storing in DB logic is missing.
const highlightText = (): void => {
if (!selectionText) return;
const selectionRange = getSelectionOffsets();
- console.log('Highlighting text:', selectionText, selectionRange);
- // Implement storing `selectionText`, startOffset, endOffset in DB logic here
+ // Store the highlight in the database
+ if (isLoggedIn && userId && selectionRange) {
+ storeHighlight(selectionText, selectionRange.startOffset, selectionRange.endOffset);
+ } else if (!isLoggedIn) {
+ toast.error('You need to be logged in to highlight text');
+ showAuthBanner = true;
+ bannerText = 'Highlighting articles';
+ bannerSubTitle = joinPhrases(bannerSubtitlePhrases, 'customize');
+ }
menuVisible = false;
};
+ async function storeHighlight(text: string, startOffset: number, endOffset: number) {
+ try {
+ const { error } = await supabase
+ .from('UserHighlights')
+ .insert({
+ userId,
+ articleId: $slug,
+ text,
+ startOffset,
+ endOffset,
+ createdAt: new Date().toISOString()
+ });
+
+ if (error) {
+ toast.error('Failed to save highlight. Please try again.');
+ console.error('Failed to save highlight:', error);
+ } else {
+ toast.success('Highlight saved');
+ }
+ } catch (error) {
+ toast.error('An error occurred. Please try again.');
+ console.error('Error saving highlight:', error);
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const highlightText = (): void => { | |
| if (!selectionText) return; | |
| const selectionRange = getSelectionOffsets(); | |
| console.log('Highlighting text:', selectionText, selectionRange); | |
| // Implement storing `selectionText`, startOffset, endOffset in DB logic here | |
| menuVisible = false; | |
| }; | |
| const highlightText = (): void => { | |
| if (!selectionText) return; | |
| const selectionRange = getSelectionOffsets(); | |
| // Store the highlight in the database | |
| if (isLoggedIn && userId && selectionRange) { | |
| storeHighlight(selectionText, selectionRange.startOffset, selectionRange.endOffset); | |
| } else if (!isLoggedIn) { | |
| toast.error('You need to be logged in to highlight text'); | |
| showAuthBanner = true; | |
| bannerText = 'Highlighting articles'; | |
| bannerSubTitle = joinPhrases(bannerSubtitlePhrases, 'customize'); | |
| } | |
| menuVisible = false; | |
| }; | |
| async function storeHighlight(text: string, startOffset: number, endOffset: number) { | |
| try { | |
| const { error } = await supabase | |
| .from('UserHighlights') | |
| .insert({ | |
| userId, | |
| articleId: $slug, | |
| text, | |
| startOffset, | |
| endOffset, | |
| createdAt: new Date().toISOString() | |
| }); | |
| if (error) { | |
| toast.error('Failed to save highlight. Please try again.'); | |
| console.error('Failed to save highlight:', error); | |
| } else { | |
| toast.success('Highlight saved'); | |
| } | |
| } catch (error) { | |
| toast.error('An error occurred. Please try again.'); | |
| console.error('Error saving highlight:', error); | |
| } | |
| } |
| const handleSelection = (event: MouseEvent): void => { | ||
| const selection: Selection | null = window.getSelection(); | ||
| if (!selection || selection.rangeCount === 0) { | ||
| menuVisible = false; | ||
| return; | ||
| } | ||
|
|
||
| const range: Range = selection.getRangeAt(0); | ||
|
|
||
| console.log({ range }); | ||
|
|
||
| // Ensure the selection is inside the article | ||
| if (!articleRef || !articleRef.contains(range.commonAncestorContainer)) { | ||
| menuVisible = false; | ||
| return; | ||
| } | ||
|
|
||
| selectionText = selection.toString().trim(); | ||
|
|
||
| if (!selectionText) { | ||
| menuVisible = false; | ||
| return; | ||
| } | ||
|
|
||
| // Get bounding box of selection | ||
| const rect: DOMRect = range.getBoundingClientRect(); | ||
| menuX = rect.left + window.scrollX; | ||
| menuY = rect.top + window.scrollY - 40; // Position above the selection | ||
|
|
||
| menuVisible = true; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove debug console log in the handleSelection function.
The function logic is good, but there's a debug console.log statement that should be removed.
function handleSelection(event: MouseEvent): void {
const selection: Selection | null = window.getSelection();
if (!selection || selection.rangeCount === 0) {
menuVisible = false;
return;
}
const range: Range = selection.getRangeAt(0);
- console.log({ range });
// Ensure the selection is inside the article
if (!articleRef || !articleRef.contains(range.commonAncestorContainer)) {
menuVisible = false;
return;
}Committable suggestion skipped: line range outside the PR's diff.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores