Conversation
|
@shinokada is attempting to deploy a commit to the Themesberg Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughAdds a new Svelte ScrollSpy component, theme, types, public exports, documentation and examples, an E2E test, and removes height/overflow constraints from the app root container. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Nav as ScrollSpy (nav)
participant IO as IntersectionObserver
participant Root as Window/ScrollContainer
rect rgba(235,245,255,0.6)
Note over Nav: mount → resolve scrollElement, init observer & scroll listener
Nav->>Root: resolve root (window or provided container)
Nav->>IO: observe each target section (dense thresholds)
end
User->>Nav: click / key (Enter/Space) on item
Nav->>Nav: scrollToSection(id) (compute offset, preventDefault)
Nav->>Root: perform scroll (smooth or instant)
Nav->>User: onNavigate callback
Root->>IO: sections enter/exit viewport
IO->>Nav: intersection updates (ratios)
Nav->>Nav: updateActiveSection() → set activeId
Nav->>User: onActiveChange(activeId)
Root->>Nav: scroll events
Nav->>Nav: update isSticky state if configured
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 6
🧹 Nitpick comments (6)
src/routes/docs-examples/extend/scroll-spy/md/installation.md (1)
1-1: Installation command lacks context and explanation.While the pnpm command syntax is correct, an installation documentation file typically includes explanatory text (e.g., what these packages provide, prerequisites, verification steps, or next steps). Additionally, verify whether both packages should be development-only dependencies via the
-Dflag—flowbitemay need to be a runtime dependency if it's required by the component at runtime.Consider expanding this to include:
- A brief description of what each package provides
- Clarification on why both are needed
- Verification instructions (e.g., confirming installation was successful)
For example:
+# Installation + +Install the required packages: + pnpm i -D flowbite-svelte flowbite + +This installs `flowbite-svelte` (Svelte components) and `flowbite` (base utilities) as development dependencies.Alternatively, if
flowbiteis required at runtime, remove the-Dflag:-pnpm i -D flowbite-svelte flowbite +pnpm i flowbite-svelte flowbitesrc/routes/docs-examples/extend/scroll-spy/right-position/+page.svelte (1)
25-26: Drop debug$inspectbefore publishing.The
$inspectrune will dump every state change to the browser console on the live docs site. Please remove it (or guard it behind a dev-only flag) to keep the examples clean.- $inspect('Current section: ', currentSection);src/routes/docs-examples/extend/scroll-spy/Simple.svelte (1)
25-26: Remove the$inspectdebug log.This example shouldn’t emit development diagnostics in production. Dropping the
$inspectcall keeps the live docs console clean.- $inspect('Current section: ', currentSection);src/routes/docs-examples/extend/scroll-spy/Original.svelte (1)
25-26: Delete the$inspectinstrumentation.Leaving this in will spam the console whenever readers scroll the example. Please strip it out before shipping.
- $inspect('Current section: ',currentSection);src/routes/docs-examples/extend/scroll-spy/left-position/+page.svelte (1)
25-32: Remove debug$inspectbefore publishing docsThe runtime
"$inspect"trace will spam the console in production examples. Please drop it now that the example is complete.src/routes/docs-examples/extend/scroll-spy/+page.svelte (1)
57-60: Remove debug$inspectbefore publishing docsSame as the other example: please remove the
$inspectcall so the published docs don’t emit debug noise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
src/app.html(1 hunks)src/lib/index.ts(1 hunks)src/lib/scroll-spy/ScrollSpy.svelte(1 hunks)src/lib/scroll-spy/index.ts(1 hunks)src/lib/scroll-spy/theme.ts(1 hunks)src/lib/types.ts(1 hunks)src/routes/docs-examples/extend/scroll-spy/+page.svelte(1 hunks)src/routes/docs-examples/extend/scroll-spy/Original.svelte(1 hunks)src/routes/docs-examples/extend/scroll-spy/Simple.svelte(1 hunks)src/routes/docs-examples/extend/scroll-spy/left-position/+page.svelte(1 hunks)src/routes/docs-examples/extend/scroll-spy/md/installation.md(1 hunks)src/routes/docs-examples/extend/scroll-spy/md/offset.md(1 hunks)src/routes/docs-examples/extend/scroll-spy/md/position.md(1 hunks)src/routes/docs-examples/extend/scroll-spy/md/style.md(1 hunks)src/routes/docs-examples/extend/scroll-spy/md/usage.md(1 hunks)src/routes/docs-examples/extend/scroll-spy/right-position/+page.svelte(1 hunks)src/routes/docs-examples/extend/scroll-spy/top-position/+page.svelte(1 hunks)src/routes/docs/extend/scroll-spy.md(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-11-22T08:37:27.996Z
Learnt from: mrh1997
Repo: themesberg/flowbite-svelte PR: 1442
File: src/lib/utils/Popper.svelte:144-145
Timestamp: 2024-11-22T08:37:27.996Z
Learning: In `src/lib/utils/Popper.svelte`, inconsistencies in event handling (e.g., using both `mouseenter`/`mouseleave` and `pointerenter`/`pointerleave`) may be intentional to minimize modifications and reduce the risk of new problems.
Applied to files:
src/routes/docs-examples/extend/scroll-spy/Original.sveltesrc/routes/docs-examples/extend/scroll-spy/left-position/+page.sveltesrc/lib/scroll-spy/ScrollSpy.sveltesrc/routes/docs-examples/extend/scroll-spy/+page.svelte
📚 Learning: 2024-11-12T10:36:34.807Z
Learnt from: LahTeuto
Repo: themesberg/flowbite-svelte PR: 1484
File: src/lib/navbar/NavLi.svelte:34-34
Timestamp: 2024-11-12T10:36:34.807Z
Learning: When enhancing ARIA roles in Svelte components, avoid setting `role="presentation"` on elements that should retain the `link` role, as it can diminish ARIA role coverage and negatively affect accessibility.
Applied to files:
src/routes/docs-examples/extend/scroll-spy/+page.svelte
🧬 Code graph analysis (1)
src/lib/scroll-spy/theme.ts (1)
src/lib/theme/themeUtils.ts (1)
Classes(12-14)
🪛 LanguageTool
src/routes/docs-examples/extend/scroll-spy/md/style.md
[grammar] ~1-~1: Hier könnte ein Fehler sein.
Context:
(QB_NEW_DE)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Playwright and Lint
| <div class="min-h-screen dark:bg-gray-900"> | ||
| <!-- ScrollSpy Navigation - add scrollContainer prop --> | ||
| <ScrollSpy | ||
| items={navigationItems} | ||
| sticky={true} | ||
| offset={75} | ||
| smoothScroll={true} | ||
| scrollContainer="#scroll-container" | ||
| onActiveChange={handleActiveChange} | ||
| onNavigate={handleNavigate} | ||
| /> | ||
|
|
||
| <!-- Main Content --> | ||
| <main class="container mx-auto px-4 py-8"> |
There was a problem hiding this comment.
Provide a real scroll container for scrollContainer prop
The component is told to use scrollContainer="#scroll-container", but there is no element with that id (nor an overflow container), so ScrollSpy immediately logs warnings and falls back to window, defeating the point of the example. Please add the matching container and make it scrollable.
- <div class="min-h-screen dark:bg-gray-900">
+ <div
+ id="scroll-container"
+ class="h-screen overflow-y-auto dark:bg-gray-900"
+ >With that change the example stops warning and actually demonstrates the custom scroll container behaviour.
📝 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.
| <div class="min-h-screen dark:bg-gray-900"> | |
| <!-- ScrollSpy Navigation - add scrollContainer prop --> | |
| <ScrollSpy | |
| items={navigationItems} | |
| sticky={true} | |
| offset={75} | |
| smoothScroll={true} | |
| scrollContainer="#scroll-container" | |
| onActiveChange={handleActiveChange} | |
| onNavigate={handleNavigate} | |
| /> | |
| <!-- Main Content --> | |
| <main class="container mx-auto px-4 py-8"> | |
| <div | |
| id="scroll-container" | |
| class="h-screen overflow-y-auto dark:bg-gray-900" | |
| > | |
| <!-- ScrollSpy Navigation - add scrollContainer prop --> | |
| <ScrollSpy | |
| items={navigationItems} | |
| sticky={true} | |
| offset={75} | |
| smoothScroll={true} | |
| scrollContainer="#scroll-container" | |
| onActiveChange={handleActiveChange} | |
| onNavigate={handleNavigate} | |
| /> | |
| <!-- Main Content --> | |
| <main class="container mx-auto px-4 py-8"> |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/lib/scroll-spy/ScrollSpy.svelte (1)
228-252: Consider removingas anycasts for scroll listener options.Lines 245 and 250 use
as anyto work around TypeScript's addEventListener types. If the project is on a recent TypeScript version, the{ passive: true }option should be recognized without casting.Try removing the cast:
- container.addEventListener('scroll', handleScroll, { passive: true } as any); + container.addEventListener('scroll', handleScroll, { passive: true });If TypeScript accepts it, remove the cast on line 250 as well:
- container.removeEventListener('scroll', handleScroll as any); + container.removeEventListener('scroll', handleScroll);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/lib/scroll-spy/ScrollSpy.svelte(1 hunks)src/lib/scroll-spy/theme.ts(1 hunks)src/routes/docs-examples/extend/scroll-spy/+page.svelte(1 hunks)src/routes/docs-examples/extend/scroll-spy/md/offset.md(1 hunks)src/routes/docs-examples/extend/scroll-spy/md/position.md(1 hunks)src/routes/docs-examples/extend/scroll-spy/md/style.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/routes/docs-examples/extend/scroll-spy/md/offset.md
- src/routes/docs-examples/extend/scroll-spy/md/position.md
- src/lib/scroll-spy/theme.ts
- src/routes/docs-examples/extend/scroll-spy/+page.svelte
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-22T08:37:27.996Z
Learnt from: mrh1997
Repo: themesberg/flowbite-svelte PR: 1442
File: src/lib/utils/Popper.svelte:144-145
Timestamp: 2024-11-22T08:37:27.996Z
Learning: In `src/lib/utils/Popper.svelte`, inconsistencies in event handling (e.g., using both `mouseenter`/`mouseleave` and `pointerenter`/`pointerleave`) may be intentional to minimize modifications and reduce the risk of new problems.
Applied to files:
src/lib/scroll-spy/ScrollSpy.svelte
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Playwright and Lint
🔇 Additional comments (9)
src/lib/scroll-spy/ScrollSpy.svelte (8)
1-10: LGTM: Clean imports and well-defined interface.The ScrollSpyItem interface is clearly typed with required id/label and optional href.
12-56: LGTM: Comprehensive props with good defaults.The interface is well-documented and covers all necessary configuration options.
58-64: LGTM: State initialization is sound.The use of Map for tracking intersecting sections is an efficient choice.
66-90: LGTM: Theme derivation is well-structured.The reactive theme computation and class merging logic correctly handle user customization.
92-143: LGTM: Navigation logic is comprehensive.The scrollToSection function correctly handles both window and container contexts, with proper error handling and offset calculation. The manual activeId update at line 134 provides immediate visual feedback while the IntersectionObserver confirms the final position.
145-175: LGTM: Active section selection logic is solid.The dual-sort strategy (intersection ratio, then item order) provides stable, predictable behavior when multiple sections are visible.
177-212: LGTM: IntersectionObserver setup is correct.The observer is properly scoped to the component instance (the past issue with global window storage has been resolved). The 101 thresholds provide smooth, granular intersection tracking.
256-281: LGTM: Accessible markup with semantic structure.The navigation uses appropriate ARIA attributes and supports keyboard interaction with tabindex and onkeydown handlers.
src/routes/docs-examples/extend/scroll-spy/md/style.md (1)
1-18: LGTM: Documentation example is complete and correct.The
itemsarray is properly defined before use (the past review issue has been resolved). The example clearly demonstrates custom styling with theactiveClassprop.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/lib/scroll-spy/theme.ts (1)
33-64: Move sticky class to a compound variant.The
stickyvariant currently applies thestickyclass wheneversticky={true}, regardless ofposition. Forleft/rightpositions that usefixedpositioning, thestickyclass is ineffective. The past review suggested using a compound variant to ensurestickyapplies only whenposition="top"ANDsticky={true}.Apply this diff to implement the compound variant pattern:
sticky: { true: { - base: "sticky" + base: "" }, false: { base: "" } }, @@ defaultVariants: { position: "top", sticky: true, isSticky: false, active: false }, - compoundVariants: [] + compoundVariants: [ + { + position: "top", + sticky: true, + classes: { base: "sticky" } + } + ]
🧹 Nitpick comments (1)
src/lib/scroll-spy/ScrollSpy.svelte (1)
242-242: Remove redundanttabindex="0".Anchor elements are already focusable by default. The
tabindex="0"attribute is redundant and can be removed.Apply this diff:
onkeydown={(e) => handleKeydown(e, item.id)} - tabindex="0"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/lib/scroll-spy/ScrollSpy.svelte(1 hunks)src/lib/scroll-spy/index.ts(1 hunks)src/lib/scroll-spy/theme.ts(1 hunks)src/lib/theme/themes.ts(1 hunks)src/lib/types.ts(9 hunks)src/routes/docs-examples/extend/scroll-spy/+page.svelte(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/scroll-spy/index.ts
- src/routes/docs-examples/extend/scroll-spy/+page.svelte
- src/lib/types.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-22T08:37:27.996Z
Learnt from: mrh1997
Repo: themesberg/flowbite-svelte PR: 1442
File: src/lib/utils/Popper.svelte:144-145
Timestamp: 2024-11-22T08:37:27.996Z
Learning: In `src/lib/utils/Popper.svelte`, inconsistencies in event handling (e.g., using both `mouseenter`/`mouseleave` and `pointerenter`/`pointerleave`) may be intentional to minimize modifications and reduce the risk of new problems.
Applied to files:
src/lib/scroll-spy/ScrollSpy.svelte
🧬 Code graph analysis (1)
src/lib/scroll-spy/theme.ts (3)
src/lib/scroll-spy/index.ts (1)
scrollspy(2-2)src/lib/theme/themes.ts (1)
scrollspy(86-86)src/lib/theme/themeUtils.ts (1)
Classes(12-14)
🪛 ESLint
src/lib/scroll-spy/ScrollSpy.svelte
[error] 29-29: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 235-235: Unexpected href link without resolve().
(svelte/no-navigation-without-resolve)
🔇 Additional comments (2)
src/lib/theme/themes.ts (1)
86-86: LGTM!The export follows the established pattern and correctly integrates the ScrollSpy theme into the library's public API.
src/lib/scroll-spy/ScrollSpy.svelte (1)
235-235: ESLint warning is a false positive.The
svelte/no-navigation-without-resolvewarning can be ignored here. These are fragment identifiers (#section-id) for same-page navigation, not route changes. Theonclickhandler prevents default navigation and usesscrollToSection, making thehrefprimarily for accessibility.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/lib/scroll-spy/ScrollSpy.svelte (2)
34-49: Remove commented-out code.Lines 35 and 41 contain commented code that appears to be leftover from development or copied from another component. Please remove these comments to keep the codebase clean.
Apply this diff:
const theme = $derived(() => { - // const isStickyAllowed = sticky && position === 'top'; const { base, container, list, link, li } = scrollspy({ position, sticky, isSticky }); - // class="{styles.overlay({ class: clsx(theme?.overlay, classes?.overlay) })} z-[9998]" return {
154-156: Consider reducing the number of thresholds for better performance.Creating 101 threshold values (line 156) results in very frequent IntersectionObserver callbacks as elements scroll. For typical scroll spy use cases, 10-20 thresholds would provide smooth updates while being more performant.
If you'd like to optimize, consider:
- // Use full threshold range 0.0 → 1.0 in 0.01 steps for smooth updates - const thresholds = Array.from({ length: 101 }, (_, i) => i / 100); + // Use threshold range 0.0 → 1.0 in 0.1 steps for smooth updates with better performance + const thresholds = Array.from({ length: 11 }, (_, i) => i / 10);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/lib/scroll-spy/ScrollSpy.svelte(1 hunks)src/lib/scroll-spy/theme.ts(1 hunks)src/routes/docs-examples/extend/scroll-spy/+page.svelte(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/routes/docs-examples/extend/scroll-spy/+page.svelte
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-22T08:37:27.996Z
Learnt from: mrh1997
Repo: themesberg/flowbite-svelte PR: 1442
File: src/lib/utils/Popper.svelte:144-145
Timestamp: 2024-11-22T08:37:27.996Z
Learning: In `src/lib/utils/Popper.svelte`, inconsistencies in event handling (e.g., using both `mouseenter`/`mouseleave` and `pointerenter`/`pointerleave`) may be intentional to minimize modifications and reduce the risk of new problems.
Applied to files:
src/lib/scroll-spy/ScrollSpy.svelte
🧬 Code graph analysis (1)
src/lib/scroll-spy/theme.ts (3)
src/lib/scroll-spy/index.ts (1)
scrollspy(2-2)src/lib/theme/themes.ts (1)
scrollspy(86-86)src/lib/theme/themeUtils.ts (1)
Classes(12-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Playwright and Lint
🔇 Additional comments (3)
src/lib/scroll-spy/theme.ts (1)
1-71: LGTM! Theme configuration is well-structured.The ScrollSpy theme follows tailwind-variants patterns correctly and the previous sticky class issue has been properly resolved. The compound variant (lines 64-70) now conditionally applies the
stickyclass only whenposition="top"andsticky={true}, which allows thestickyprop to work as intended.src/lib/scroll-spy/ScrollSpy.svelte (2)
198-222: LGTM! Lifecycle management is properly implemented.The effect correctly sets up the scroll element reference, creates a local IntersectionObserver instance (avoiding the previous global state issue), and attaches event listeners with appropriate cleanup. The use of
passive: truefor the scroll listener is good for performance.
225-245: LGTM! Template has excellent accessibility support.The markup uses semantic HTML with proper ARIA attributes (
aria-label,aria-current,role="list"), includes keyboard navigation support, and follows accessibility best practices.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/lib/scroll-spy/ScrollSpy.svelte (1)
124-135: Optimize sorting performance by caching item indices.The sort comparator calls
findIndexfor every comparison, resulting in O(m × n × log m) complexity where m is the number of intersecting sections and n is the total number of items. For large item lists, this becomes inefficient.Apply this diff to cache the indices:
// Sort by intersection ratio descending, then by items order ascending + const itemIndexMap = new Map(items.map((item, index) => [item.id, index])); const sorted = Array.from(intersectingSections.entries()).sort((a, b) => { const ratioA = a[1].intersectionRatio; const ratioB = b[1].intersectionRatio; if (Math.abs(ratioA - ratioB) > 0.01) { return ratioB - ratioA; } - const indexA = items.findIndex((item) => item.id === a[0]); - const indexB = items.findIndex((item) => item.id === b[0]); + const indexA = itemIndexMap.get(a[0]) ?? -1; + const indexB = itemIndexMap.get(b[0]) ?? -1; return indexA - indexB; });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/scroll-spy/ScrollSpy.svelte(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-22T08:37:27.996Z
Learnt from: mrh1997
Repo: themesberg/flowbite-svelte PR: 1442
File: src/lib/utils/Popper.svelte:144-145
Timestamp: 2024-11-22T08:37:27.996Z
Learning: In `src/lib/utils/Popper.svelte`, inconsistencies in event handling (e.g., using both `mouseenter`/`mouseleave` and `pointerenter`/`pointerleave`) may be intentional to minimize modifications and reduce the risk of new problems.
Applied to files:
src/lib/scroll-spy/ScrollSpy.svelte
🪛 ESLint
src/lib/scroll-spy/ScrollSpy.svelte
[error] 230-230: Unexpected href link without resolve().
(svelte/no-navigation-without-resolve)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Playwright and Lint
🔇 Additional comments (1)
src/lib/scroll-spy/ScrollSpy.svelte (1)
229-240: Dismiss this review comment - the code pattern is correct.The
hrefattribute withoutresolve()is intentional here. ThescrollToSectionfunction properly callsevent.preventDefault()(line 62), and the component useshreffor accessibility and keyboard navigation while handling scroll behavior manually. This is the correct pattern for scroll spy functionality and is not a SvelteKit router bypass. Additionally, thesvelte/no-navigation-without-resolverule is not explicitly enabled in the ESLint configuration, so the flagged warning may not actually exist.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/scroll-spy/ScrollSpy.svelte(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-22T08:37:27.996Z
Learnt from: mrh1997
Repo: themesberg/flowbite-svelte PR: 1442
File: src/lib/utils/Popper.svelte:144-145
Timestamp: 2024-11-22T08:37:27.996Z
Learning: In `src/lib/utils/Popper.svelte`, inconsistencies in event handling (e.g., using both `mouseenter`/`mouseleave` and `pointerenter`/`pointerleave`) may be intentional to minimize modifications and reduce the risk of new problems.
Applied to files:
src/lib/scroll-spy/ScrollSpy.svelte
🪛 ESLint
src/lib/scroll-spy/ScrollSpy.svelte
[error] 231-231: Unexpected href link without resolve().
(svelte/no-navigation-without-resolve)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Playwright and Lint
🔇 Additional comments (7)
src/lib/scroll-spy/ScrollSpy.svelte (7)
1-32: LGTM! Clean setup and state initialization.The imports, props destructuring, and state initialization follow Svelte 5 best practices. The browser guard and properly typed
scrollElementprevent SSR issues and type errors.
50-57: Class composition logic is sound.The function correctly determines the active state, retrieves themed styles, and merges custom classes. Once the
$derived.by()pattern is applied (per the previous comment), remember to update line 56 fromtheme().linktotheme.link.
59-104: LGTM! Robust scroll navigation logic.The function correctly handles both window and container scrolling contexts, applies offsets, respects the smoothScroll setting, and includes appropriate error handling. The type assertions are safe given the
isWindowchecks.
106-144: LGTM! Keyboard navigation and active section logic are well-implemented.The keyboard handler correctly supports Enter and Space keys for accessibility. The
updateActiveSectionfunction uses a deterministic algorithm: it prioritizes sections by intersection ratio (with a sensible 0.01 threshold for floating-point comparison) and falls back to item order for ties. The logic gracefully handles edge cases like no visible sections.
146-184: LGTM! Intersection observer setup is robust.The observer configuration uses sensible defaults (11 thresholds for smooth updates, rootMargin incorporating the offset), handles missing containers and sections with warnings, and correctly maintains the
intersectingSectionsmap. Returning the observer enables proper cleanup in the effect.
186-222: LGTM! Effect setup and scroll handling are correct.The
handleScrollfunction appropriately updates sticky state and active section based on scroll position. The effect properly resolves the scroll container, sets up the intersection observer, attaches a passive scroll listener for performance, and returns a cleanup function that disconnects and removes listeners. Reactive dependencies are correctly tracked.
225-245: Template structure and accessibility are well-implemented.The markup uses appropriate ARIA attributes (
aria-label,aria-current,role="list"), proper event handlers for both mouse and keyboard interaction, and provides fallbackhrefvalues for progressive enhancement.Regarding the ESLint warning on line 231: the
hrefattribute is intentionally combined withonclickthat callspreventDefault()(line 62). This is a standard pattern for scroll spy components—hash URLs serve as fallbacks while JavaScript provides smooth scrolling. The warning is a false positive.Once the
$derived.by()refactor is applied, remember to update lines 225, 226, 227, and 229 fromtheme().propertytotheme.property.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/scroll-spy/ScrollSpy.svelte(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-22T08:37:27.996Z
Learnt from: mrh1997
Repo: themesberg/flowbite-svelte PR: 1442
File: src/lib/utils/Popper.svelte:144-145
Timestamp: 2024-11-22T08:37:27.996Z
Learning: In `src/lib/utils/Popper.svelte`, inconsistencies in event handling (e.g., using both `mouseenter`/`mouseleave` and `pointerenter`/`pointerleave`) may be intentional to minimize modifications and reduce the risk of new problems.
Applied to files:
src/lib/scroll-spy/ScrollSpy.svelte
🪛 ESLint
src/lib/scroll-spy/ScrollSpy.svelte
[error] 231-231: Unexpected href link without resolve().
(svelte/no-navigation-without-resolve)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Playwright and Lint
| href={item.href || `#${item.id}`} | ||
| class={getItemClass(item.id)} | ||
| aria-current={activeId === item.id ? "location" : undefined} | ||
| onclick={(e) => { | ||
| scrollToSection(item.id, e); | ||
| }} | ||
| onkeydown={(e) => handleKeydown(e, item.id)} |
There was a problem hiding this comment.
Fix the svelte/no-navigation-without-resolve lint error on the anchor.
pnpm check is currently failing because this href assignment isn’t wrapped in a resolve(...) or otherwise acknowledged. Please update this block so it satisfies the rule—either resolve internal URLs via SvelteKit’s resolve helper (with an appropriate fallback for non-SvelteKit consumers) or add an explicit, justified svelte-ignore if the rule can’t be met here. As it stands, the lint error will keep the pipeline red.
🧰 Tools
🪛 ESLint
[error] 231-231: Unexpected href link without resolve().
(svelte/no-navigation-without-resolve)
🤖 Prompt for AI Agents
In src/lib/scroll-spy/ScrollSpy.svelte around lines 231-237, the anchor href is
assigned directly which triggers svelte/no-navigation-without-resolve; fix by
either (A) importing and using SvelteKit's resolve helper when available (e.g.,
import { resolve } from '$app/navigation' or appropriate module) and wrap the
href assignment like href={resolve(item.href || `#${item.id}`)} with a small
runtime fallback that returns the original string if resolve is undefined, or
(B) if this component must remain framework-agnostic, add a focused
svelte-ignore comment above the anchor (// svelte-ignore
a11y-no-distracting-elements or the exact rule name) with a one-line
justification that links are internal hash navigation and safe; implement one of
these two options to satisfy the linter.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/scroll-spy/ScrollSpy.svelte (1)
50-57: Consider caching active/inactive link variants for efficiency.Creating a new
scrollspy({ active: isActive })call per item on every render is less efficient than pre-computing both active and inactive variants once.You could compute both variants in the derived theme:
const theme = $derived.by(() => { - const { base, container, list, link, li } = scrollspy({ + const { base, container, list, li } = scrollspy({ position, sticky, isSticky }); + const { link: activeLink } = scrollspy({ active: true }); + const { link: inactiveLink } = scrollspy({ active: false }); return { base: base({ class: clsx(styles?.base, className) }), container: container({ class: clsx(styles?.container, classes?.container) }), list: list({ class: clsx(styles?.list, classes?.list) }), - link: link({ class: clsx(styles?.link, classes?.link) }), + activeLink: activeLink({ class: clsx(styles?.link, classes?.link) }), + inactiveLink: inactiveLink({ class: clsx(styles?.link, classes?.link) }), li: li({ class: clsx(styles?.li, classes?.li) }) }; }); function getItemClass(itemId: string): string { const isActive = activeId === itemId; - const { link } = scrollspy({ active: isActive }); - + const link = isActive ? theme.activeLink : theme.inactiveLink; const customClass = isActive ? activeClass : inactiveClass; - return clsx(theme.link, link(), customClass, classes?.link); + return clsx(link, customClass); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/scroll-spy/ScrollSpy.svelte(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-11-22T08:37:27.996Z
Learnt from: mrh1997
Repo: themesberg/flowbite-svelte PR: 1442
File: src/lib/utils/Popper.svelte:144-145
Timestamp: 2024-11-22T08:37:27.996Z
Learning: In `src/lib/utils/Popper.svelte`, inconsistencies in event handling (e.g., using both `mouseenter`/`mouseleave` and `pointerenter`/`pointerleave`) may be intentional to minimize modifications and reduce the risk of new problems.
Applied to files:
src/lib/scroll-spy/ScrollSpy.svelte
📚 Learning: 2024-11-12T10:36:34.807Z
Learnt from: LahTeuto
Repo: themesberg/flowbite-svelte PR: 1484
File: src/lib/navbar/NavLi.svelte:34-34
Timestamp: 2024-11-12T10:36:34.807Z
Learning: When enhancing ARIA roles in Svelte components, avoid setting `role="presentation"` on elements that should retain the `link` role, as it can diminish ARIA role coverage and negatively affect accessibility.
Applied to files:
src/lib/scroll-spy/ScrollSpy.svelte
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Playwright and Lint
🔇 Additional comments (5)
src/lib/scroll-spy/ScrollSpy.svelte (5)
146-184: LGTM!The IntersectionObserver setup is well-implemented with appropriate SSR guards, error handling, and a good balance of threshold granularity (11 steps) for smooth updates without excessive callbacks.
186-196: LGTM!The scroll handler properly guards dependencies, updates sticky state, and triggers active section updates. The type assertion is minimal and necessary.
198-223: LGTM!The effect lifecycle management is solid: resolves scroll container, sets up observer, adds passive scroll listener for performance, and properly cleans up by disconnecting the observer, clearing intersection state, and removing listeners.
235-242: LGTM!The accessibility implementation is solid:
aria-current="location"properly indicates the active navigation item, and both click and keyboard handlers are correctly bound to enable full keyboard navigation.
231-233: Review comment resolved—no lint errors detected.The recent verification confirms no lint errors in the ScrollSpy component. The ignore comment
a11y_invalid_attributeand the href assignment with fallback ({item.href || \#${item.id}`}) are functioning correctly. The past review flaggingsvelte/no-navigation-without-resolvewas likely addressed: fragment URLs (with#`) are exempt from the resolve() requirement, which aligns with the fallback behavior in the code.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/lib/scroll-spy/ScrollSpy.svelte (3)
133-133: Extract magic number to a named constant.The
0.01threshold for comparing intersection ratios is a magic number that affects the sensitivity of active section detection. Extract it to a named constant for clarity and easier tuning.+ const INTERSECTION_RATIO_EPSILON = 0.01; + function updateActiveSection() { // ... - if (Math.abs(ratioA - ratioB) > 0.01) { + if (Math.abs(ratioA - ratioB) > INTERSECTION_RATIO_EPSILON) { return ratioB - ratioA; }
158-158: Document or extract the hardcoded bottom margin.The
-40%bottom margin inrootMargindirectly affects when sections become active. Consider extracting it as a named constant with a comment explaining the choice, or making it configurable via props if users need different sensitivity.+ // Bottom margin determines how far up the viewport a section must scroll before becoming inactive + const ROOT_MARGIN_BOTTOM = "-40%"; + function setupIntersectionObserver() { // ... - const rootMargin = `-${offset}px 0px -40% 0px`; + const rootMargin = `-${offset}px 0px ${ROOT_MARGIN_BOTTOM} 0px`;
234-236: Remove unnecessary svelte-ignore directive.The
svelte-ignore a11y_invalid_attributedirective appears unnecessary since thehrefon line 236 always has a valid value (eitheritem.hrefor a hash link#${item.id}). Unless there's a specific undocumented reason, this directive should be removed to avoid suppressing legitimate warnings elsewhere.- <!-- svelte-ignore a11y_invalid_attribute --> <a href={item.href || `#${item.id}`}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/scroll-spy/ScrollSpy.svelte(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-11-22T08:37:27.996Z
Learnt from: mrh1997
Repo: themesberg/flowbite-svelte PR: 1442
File: src/lib/utils/Popper.svelte:144-145
Timestamp: 2024-11-22T08:37:27.996Z
Learning: In `src/lib/utils/Popper.svelte`, inconsistencies in event handling (e.g., using both `mouseenter`/`mouseleave` and `pointerenter`/`pointerleave`) may be intentional to minimize modifications and reduce the risk of new problems.
Applied to files:
src/lib/scroll-spy/ScrollSpy.svelte
📚 Learning: 2024-11-12T10:36:34.807Z
Learnt from: LahTeuto
Repo: themesberg/flowbite-svelte PR: 1484
File: src/lib/navbar/NavLi.svelte:34-34
Timestamp: 2024-11-12T10:36:34.807Z
Learning: When enhancing ARIA roles in Svelte components, avoid setting `role="presentation"` on elements that should retain the `link` role, as it can diminish ARIA role coverage and negatively affect accessibility.
Applied to files:
src/lib/scroll-spy/ScrollSpy.svelte
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Playwright and Lint
🔇 Additional comments (1)
src/lib/scroll-spy/ScrollSpy.svelte (1)
1-250: Solid implementation with good accessibility and proper cleanup.The component demonstrates several strengths:
- Proper cleanup in the
$effectteardown (observer disconnect, event listener removal, intersection map cleared)- Good accessibility:
aria-labelon navigation,aria-currenton active links, and keyboard support viahandleKeydown- Robust active section detection using intersection ratios with fallback to item order
- Handles both window and custom scroll containers correctly
All previously flagged critical and major issues (global observer state, type safety, reactivity patterns) have been successfully addressed.
…e hardcoded bottom margin, Remove unnecessary svelte-ignore directive
1 participant
🔗 Related issue (optional)
Closes #
📑 Description
🔍 PR Type
🚦 PR Status
✅ Checklist
pnpm check && pnpm test:e2eand all tests passapi-checkdirectory if a component API changedmainbranch (not the published npm version)🧪 Screenshots / Demos (optional)
ℹ️ Additional Information
Summary by CodeRabbit
New Features
Bug Fixes / Layout
Documentation
Tests
Chores