feat!: badges in sidebar #56
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis change introduces a new server-side layout loader and Svelte components to enhance the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SvelteLayout as +layout.svelte
participant ServerLoader as +layout.server.ts
participant SidePanel as SidePanel.svelte
participant Page as +page.svelte
User->>SvelteLayout: Navigates to /package or /package/[...package]
SvelteLayout->>ServerLoader: Calls load()
ServerLoader->>ServerLoader: Fetches all categorized packages
ServerLoader->>ServerLoader: For each package, fetches and filters releases
ServerLoader-->>SvelteLayout: Returns allReleases map (promises)
SvelteLayout->>SidePanel: Passes allPackages and allReleases as props
User->>Page: Navigates to a specific package page
Page->>SvelteLayout: Requests data via parent()
SvelteLayout-->>Page: Provides allReleases
Page->>Page: Awaits and renders releases for the selected package
SidePanel->>SidePanel: Displays package list and new release badges
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/package/SidePanel.svelteOops! Something went wrong! :( ESLint: 9.25.1 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@sveltejs/adapter-vercel' imported from /svelte.config.js src/routes/package/+layout.server.tsOops! Something went wrong! :( ESLint: 9.25.1 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@sveltejs/adapter-vercel' imported from /svelte.config.js ✨ Finishing Touches
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. 🪧 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: 4
♻️ Duplicate comments (1)
src/routes/package/SidePanel.svelte (1)
185-191: Add error handling for async badge data (duplicate pattern).Similar to the previous comment, this second instance of badge data awaiting also needs error handling.
🧹 Nitpick comments (5)
src/routes/package/+layout.server.ts (2)
60-70: Repeatedsort()inside loop is an unnecessary hotspotInside the
validReleasesloop you computecurrentNewestVersionby converting theSetto an array and callingsemver.rcompareevery iteration.
On large repositories this becomes O(R² log R). Track the newest version incrementally instead:- const currentNewestVersion = [...foundVersions].sort(semver.rcompare)[0]; - // … - foundVersions.add(cleanVersion); - releases.push({ cleanName, cleanVersion, ...release }); - - // If it is newer than the newest we got, set this repo as the "final repo" - if (!currentNewestVersion || semver.gt(cleanVersion, currentNewestVersion)) { + const isNewer = + !globalThis.__newestVersion || + semver.gt(cleanVersion, globalThis.__newestVersion); + foundVersions.add(cleanVersion); + releases.push({ cleanName, cleanVersion, ...release }); + + if (isNewer) { + globalThis.__newestVersion = cleanVersion; // … }(Use a local variable instead of
globalThis; shown only for brevity.)This removes the per-iteration sort while preserving behaviour.
128-137: Missing type on exportedloadreduces DXThe SvelteKit
loadhook can be typed viaLayoutServerLoadto enable auto-completion in consumers:-import type { LayoutServerLoad } from './$types'; // adjust relative path - -export async function load() { +import type { LayoutServerLoad } from './$types'; + +export const load: LayoutServerLoad = async () => { // … - return { allReleases }; -} + return { allReleases }; +};Helps tooling catch accidental shape changes early.
src/routes/package/[...package]/+page.svelte (1)
25-27:localStoragewrite fires on every state change
$effectexecutes on all reactive updates, so togglingshowPrereleases, accordion state, etc. will rewrite the same key many times per session. A one-offonMountis sufficient and cheaper:-import { onMount } from "svelte"; - -$effect(() => { - localStorage.setItem(`last-visited-${data.currentPackage.pkg.name}`, new Date().toISOString()); -}); +import { onMount } from "svelte"; + +onMount(() => { + try { + localStorage.setItem( + `last-visited-${data.currentPackage.pkg.name}`, + new Date().toISOString() + ); + } catch { + /* SSR or private-mode fallback – ignore */ + } +});Also guards against SSR and private-mode
QuotaExceededError.src/routes/package/SidePanel.svelte (2)
81-98: Consider adding error handling for localStorage.While there is a browser check at line 89, it would be better to add a try-catch block around localStorage operations to handle potential exceptions in private browsing modes or when storage is full.
function getUnvisitedReleases(pkgName: string, releases: CleanRelease[] | undefined) { if (!releases || !browser) return []; - const lastVisitedItem = localStorage.getItem(`last-visited-${pkgName}`); - if (!lastVisitedItem) return []; - const lastVisitedDate = new Date(lastVisitedItem); + try { + const lastVisitedItem = localStorage.getItem(`last-visited-${pkgName}`); + if (!lastVisitedItem) return []; + const lastVisitedDate = new Date(lastVisitedItem); + + return releases.filter( + ({ created_at, published_at }) => new Date(published_at ?? created_at) > lastVisitedDate + ); + } catch (error) { + console.error(`Error accessing localStorage for ${pkgName}:`, error); + return []; + } - - return releases.filter( - ({ created_at, published_at }) => new Date(published_at ?? created_at) > lastVisitedDate - ); }
152-159: Add error handling for async badge data.The await block for badge data doesn't include error handling. Consider adding an error branch to gracefully handle failed requests.
{#if linkedBadgeData} - {#await linkedBadgeData then data} + {#await linkedBadgeData} + <!-- Optional loading state --> + {:then data} {@render newBadge( getUnvisitedReleases(pkg.name, data?.releases).length )} + {:catch error} + <!-- Silently fail or show minimal error indicator --> + <span class="text-xs text-muted-foreground" title="Failed to load badge data">!</span> + {/await} {/if}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/routes/package/+layout.server.ts(1 hunks)src/routes/package/+layout.svelte(1 hunks)src/routes/package/SidePanel.svelte(1 hunks)src/routes/package/[...package]/+page.server.ts(1 hunks)src/routes/package/[...package]/+page.svelte(3 hunks)src/routes/package/[...package]/SidePanel.svelte(0 hunks)
💤 Files with no reviewable changes (1)
- src/routes/package/[...package]/SidePanel.svelte
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: WarningImHack3r
PR: WarningImHack3r/svelte-changelog#48
File: src/routes/package/[...package]/+page.server.ts:50-53
Timestamp: 2025-04-11T10:33:07.002Z
Learning: WarningImHack3r prefers to keep server-side console.log statements in the codebase as they find them useful for debugging and monitoring, even if they might clean them up later.
🔇 Additional comments (11)
src/routes/package/[...package]/+page.svelte (1)
62-117: Large nested template risks hydration mismatches without keysInside
{#each displayableReleases as release, index (release.id)}you rely onrelease.idfor keyed diffing—great.
However, the surrounding accordion’svalueprop consumesdisplayableReleases.map(({ id }) => id.toString()); ifshowPrereleasestoggles, the set of IDs can shrink, leaving an Accordion item rendered but no longer listed invalue. Some UI libraries throw on such mismatch.Consider recomputing the
valuearray inside the Accordion after the same filter to keep them in sync, or clear it when the filter changes.src/routes/package/+layout.svelte (4)
1-11: Structure and state management look good.The script section is well-organized with appropriate imports and state management. You're correctly using the newer Svelte
$props()and$state()APIs for reactive state management, which is a good practice.
13-16: Layout structure is appropriate.The responsive layout with flex container and proper gap spacing works well. You've appropriately structured the content container with flex-1 to take available space.
18-47: Mobile sheet implementation is well-structured.The slide-over sheet for mobile views is implemented correctly with:
- Proper accessibility through sr-only text for the menu button
- Effective use of the Snippet API for the trigger button
- Good conditional styling based on package description presence
- Appropriate binding of the showPrereleases state
This responsive approach for mobile devices is a good implementation choice.
49-58: Desktop sidebar implementation looks good.The desktop implementation of SidePanel is:
- Correctly hidden on mobile with appropriate responsive classes
- Using conditional margin adjustments based on description presence
- Properly binding to the showPrereleases state
The use of class arrays with conditional expressions is elegant and maintainable.
src/routes/package/SidePanel.svelte (6)
1-57: Props and types are well-defined.The component has complex but well-structured type definitions for its props. Using optional props with default values is a good practice. The TypeScript types properly describe the structure of the data being passed to the component.
58-63: Good use of persisted state.The use of persisted store for preserving the prerelease state per package is an excellent approach for maintaining user preferences across sessions.
65-79: Well-documented utility function.The getBadgeDataFromOther function is well-documented with JSDoc comments, making its purpose and parameters clear. The function cleanly extracts release data for a specific package.
101-105: Efficient badge snippet implementation.The badge snippet is clean and efficiently handles conditional rendering based on the count value. Only showing badges when there are new items is a good UX decision.
107-126: Good conditional rendering based on headless prop.The component handles different rendering modes effectively using the headless prop, which allows it to be used in different contexts. The card styling and header visibility toggle is well-implemented.
127-201: Package category rendering logic is robust.The component handles different layouts for categories with multiple packages versus single package categories intelligently. The active state highlighting and navigation links are properly implemented along with the badges for new releases.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/routes/package/SidePanel.svelte (1)
220-241: Excellent tooltip implementation for disabled checkboxGreat implementation of the tooltip to explain the disabled state of the prereleases checkbox. This addresses the previous review comment and follows UI best practices by clearly communicating to users why the control is disabled.
🧹 Nitpick comments (5)
src/routes/package/SidePanel.svelte (5)
89-105: Add error handling for localStorage operationsThe
getUnvisitedReleasesfunction directly accesses localStorage without any error handling. While the browser check helps, various scenarios (private browsing, storage quota exceeded, etc.) could cause these operations to fail.Consider adding try-catch blocks around the localStorage operations:
function getUnvisitedReleases(pkgName: string, releases: CleanRelease[] | undefined) { if (!releases || !browser) return []; + let lastVisitedItem; + try { const lastVisitedItem = localStorage.getItem(`last-visited-${pkgName}`); + } catch (error) { + console.warn(`Failed to access localStorage for ${pkgName}:`, error); + return releases.filter( + ({ created_at, published_at }) => + new Date(published_at ?? created_at).getTime() > + new Date().getTime() - 1000 * 60 * 60 * 24 * 7 + ); + } if (!lastVisitedItem) { return releases.filter( ({ created_at, published_at }) => new Date(published_at ?? created_at).getTime() > new Date().getTime() - 1000 * 60 * 60 * 24 * 7 ); } const lastVisitedDate = new Date(lastVisitedItem); return releases.filter( ({ created_at, published_at }) => new Date(published_at ?? created_at) > lastVisitedDate ); }
59-64: Consider potential race conditions in the nested effectsThe nested effects used for persisting the prerelease state could potentially lead to race conditions or unnecessary updates if
packageNameandshowPrereleaseschange simultaneously.Consider refactoring to use a single effect that depends on both values:
-$effect(() => { - let storedPrereleaseState = persisted(`show-${packageName}-prereleases`, showPrereleases); - $effect(() => { - storedPrereleaseState.value = showPrereleases; - }); -}); +$effect(() => { + let storedPrereleaseState = persisted(`show-${packageName}-prereleases`, showPrereleases); + storedPrereleaseState.value = showPrereleases; +});
73-80: Optimize package name comparison in getBadgeDataFromOtherThe current implementation searches through all entries in
otherReleasesto find a match, which could be inefficient for a large number of packages.Consider using direct property access with case normalization:
function getBadgeDataFromOther(pkgName: string) { - const data = Object.entries(otherReleases).find( - ([k]) => k.localeCompare(pkgName, undefined, { sensitivity: "base" }) === 0 - ); - if (!data) return undefined; - const [, v] = data; - return v; + // Try direct access first + if (otherReleases[pkgName]) return otherReleases[pkgName]; + + // Fall back to case-insensitive search + const normalizedPkgName = pkgName.toLowerCase(); + for (const [k, v] of Object.entries(otherReleases)) { + if (k.toLowerCase() === normalizedPkgName) return v; + } + + return undefined; }
96-97: Use constants for magic numbersThe code uses magic numbers for calculating the week timeframe (1000 * 60 * 60 * 24 * 7).
Extract this calculation into a named constant for better readability:
function getUnvisitedReleases(pkgName: string, releases: CleanRelease[] | undefined) { if (!releases || !browser) return []; + const ONE_WEEK_MS = 1000 * 60 * 60 * 24 * 7; + const lastVisitedItem = localStorage.getItem(`last-visited-${pkgName}`); if (!lastVisitedItem) { return releases.filter( ({ created_at, published_at }) => new Date(published_at ?? created_at).getTime() > - new Date().getTime() - 1000 * 60 * 60 * 24 * 7 + new Date().getTime() - ONE_WEEK_MS ); } // ...rest of function }
94-98: Avoid code duplication in date filtering logicThere's duplicate date comparison logic between the two filter conditions.
Refactor to reuse the date comparison logic:
function getUnvisitedReleases(pkgName: string, releases: CleanRelease[] | undefined) { if (!releases || !browser) return []; + const ONE_WEEK_MS = 1000 * 60 * 60 * 24 * 7; + const compareDate = new Date(localStorage.getItem(`last-visited-${pkgName}`)) || + new Date(new Date().getTime() - ONE_WEEK_MS); + + return releases.filter( + ({ created_at, published_at }) => + new Date(published_at ?? created_at) > compareDate + ); - const lastVisitedItem = localStorage.getItem(`last-visited-${pkgName}`); - if (!lastVisitedItem) { - return releases.filter( - ({ created_at, published_at }) => - new Date(published_at ?? created_at).getTime() > - new Date().getTime() - 1000 * 60 * 60 * 24 * 7 - ); - } - const lastVisitedDate = new Date(lastVisitedItem); - - return releases.filter( - ({ created_at, published_at }) => new Date(published_at ?? created_at) > lastVisitedDate - ); }Also applies to: 102-104
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/routes/package/+layout.server.ts(1 hunks)src/routes/package/SidePanel.svelte(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/routes/package/+layout.server.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: WarningImHack3r
PR: WarningImHack3r/svelte-changelog#56
File: src/routes/package/+layout.server.ts:28-40
Timestamp: 2025-04-25T10:58:24.052Z
Learning: WarningImHack3r prefers elegant, aesthetically pleasing solutions for performance optimizations rather than quick fixes that introduce nested async IIFEs or similar approaches that might make code harder to read.
Learnt from: WarningImHack3r
PR: WarningImHack3r/svelte-changelog#48
File: src/routes/package/[...package]/+page.server.ts:50-53
Timestamp: 2025-04-11T10:33:07.002Z
Learning: WarningImHack3r prefers to keep server-side console.log statements in the codebase as they find them useful for debugging and monitoring, even if they might clean them up later.
🔇 Additional comments (3)
src/routes/package/SidePanel.svelte (3)
108-112: Well-implemented reusable badge snippetGood use of Svelte's snippet feature to create a reusable badge component that conditionally renders based on count. This pattern enhances maintainability and consistency.
132-208: Well-structured package navigation with badge integrationThe implementation elegantly handles both multi-package categories and single-package categories with appropriate styling differences. The badge integration is smoothly handled with asynchronous resolution of release data.
1-58: Well-structured component with comprehensive type definitionsThe component has clean type definitions and well-organized imports. Props are properly defined with sensible defaults, and the types ensure proper type safety and intellisense support.
I had to make big changes to the loading mechanism to support loading (all packages) releases for the sidebar component in #56. This change led to the page relying on the sidebar loading to get its own data. Unfortunately, this led to significant slowdowns when navigating through packages (I'm still not sure _why_ thought). This commit makes the page completely independent from the sidebar, bringing back it's 2.0-release speed (and it's so good to get it back). HOWEVER, initial page loads seem a bit slower than right before this commit, but in my opinion it's a fair trade-off and it's not even that visible. From a user point of view, it's also more often more acceptable to wait a bit more for the initial data loading than between in-site navigation.
The sidebar currently looks fine but lacks a lot of functionality, especially when it comes to long package lists (e.g., the "Other" category).
The purpose of this PR is not only to help figure out what's new right from the list,
but also to provide a more compact view when there is a large number of packages in some sections.This PR will be controversially quite large, but the sidebar will be good after this.Features
Collapse large sections into a short list of 5 packages at mostAllow clicking an "Expand" button to see all the packages (and morph it into a "Collapse" button once the list is expanded)Roadmap
Badges
+layout.server.tsnext to the page? Also move the sidebar into a+layout.svelte?localStorageto store the last visit for each package, only show the releases more recent thanlocalStorageon the badgelocalStoragevalue on a package visitSection collapsing (later)
Summary by CodeRabbit
New Features
Refactor
Chores