develop to master #32
Conversation
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Summary of ChangesHello @976520, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a series of enhancements across the Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📊 Test Coverage Report (vitest)
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request primarily focuses on refactoring and optimizing the scrolloop library, particularly its core virtualizer logic and documentation. Key changes include updating the README.md with separate installation instructions for React and React Native, removing InfiniteList examples, and introducing a 'Packages' section. The core virtualizer (Virtualizer.ts) was refactored to use private class fields (#) for better encapsulation and introduced memoization for virtualItems to prevent unnecessary re-renders. Integer conversions in layout and range calculations now use the bitwise OR operator (| 0) for conciseness. The SlotMachine.vue component in the documentation was significantly refactored to animate digit strips instead of individual digits, improving animation smoothness, and its watch logic was updated for better cleanup and animation handling. Build configurations (tsup.config.ts files) were updated with more aggressive Terser compression settings and adjusted target ES versions. The InfiniteList.tsx component saw a major refactor to simplify SSR data handling, remove several useMemo and useCallback definitions by inlining styles and render functions, and optimize prefetching logic. Review comments highlighted the need to re-memoize the renderItem prop in InfiniteList to avoid performance issues, to use more descriptive variable names in domPruner.ts for readability, to use a more specific type for cleanupTimer in SlotMachine.vue, and to revert a change in Virtualizer.ts that replaced Math.max/Math.min with less readable conditional clamping for renderRange.
| renderItem={(index, itemStyle) => | ||
| renderItem(mergedAllItems[index], index, itemStyle) | ||
| } |
There was a problem hiding this comment.
Creating a new function for the renderItem prop on every render of InfiniteList will break the memoization of the VirtualList component, causing it to re-render unnecessarily. This can lead to performance degradation, especially in a scrolling component.
You should memoize this callback using useCallback as it was before this change. You can re-introduce the memoized callback before the return statement:
const virtualListRenderItem = useCallback(
(index: number, itemStyle: CSSProperties) => {
return renderItem(mergedAllItems[index], index, itemStyle);
},
[mergedAllItems, renderItem]
);| renderItem={(index, itemStyle) => | |
| renderItem(mergedAllItems[index], index, itemStyle) | |
| } | |
| renderItem={virtualListRenderItem} |
| export function pruneOffscreenDOMIdle( | ||
| container: HTMLElement, | ||
| visibleRange: { start: number; end: number }, | ||
| onPrune: (index: number) => void | ||
| ): () => void { | ||
| let cancelled = false; | ||
| let requestId: ReturnType<typeof requestIdleCallback> | null = null; | ||
|
|
||
| const pruneChunk = () => { | ||
| range: { start: number; end: number }, | ||
| onPrune: (idx: number) => void | ||
| ) { | ||
| let id: any, | ||
| cancelled = false; | ||
| const prune = () => { | ||
| if (cancelled) return; | ||
|
|
||
| const items = container.querySelectorAll("[data-item-index]"); | ||
| let pruned = 0; | ||
| const maxPrunePerFrame = 5; | ||
|
|
||
| for (const item of items) { | ||
| if (pruned >= maxPrunePerFrame) break; | ||
|
|
||
| const index = parseInt(item.getAttribute("data-item-index") || "-1", 10); | ||
| if (index < 0) continue; | ||
|
|
||
| if (index < visibleRange.start || index > visibleRange.end) { | ||
| onPrune(index); | ||
| pruned++; | ||
| } | ||
| } | ||
|
|
||
| if (pruned > 0 && !cancelled) { | ||
| requestId = requestIdleCallback(pruneChunk); | ||
| } | ||
| let count = 0; | ||
| container.querySelectorAll("[data-item-index]").forEach((el) => { | ||
| if (count++ > 5) return; | ||
| const i = parseInt(el.getAttribute("data-item-index") || "-1", 10); | ||
| if (i >= 0 && (i < range.start || i > range.end)) onPrune(i); | ||
| }); | ||
| if (count > 0 && !cancelled) id = RIC(prune); | ||
| }; | ||
|
|
||
| requestId = requestIdleCallback(pruneChunk); | ||
|
|
||
| id = RIC(prune); | ||
| return () => { | ||
| cancelled = true; | ||
| if (requestId !== null) { | ||
| cancelIdleCallback(requestId); | ||
| } | ||
| CIC(id); | ||
| }; | ||
| } | ||
|
|
||
| export function pruneOffscreenDOMChunk( | ||
| container: HTMLElement, | ||
| visibleRange: { start: number; end: number }, | ||
| chunkSize: number, | ||
| onPrune: (index: number) => void | ||
| ): () => void { | ||
| let cancelled = false; | ||
| let timeoutId: number | null = null; | ||
|
|
||
| const pruneChunk = () => { | ||
| range: { start: number; end: number }, | ||
| chunk: number, | ||
| onPrune: (idx: number) => void | ||
| ) { | ||
| let id: any, | ||
| cancelled = false; | ||
| const prune = () => { | ||
| if (cancelled) return; | ||
|
|
||
| const items = Array.from(container.querySelectorAll("[data-item-index]")); | ||
| const offscreenItems = items.filter((item) => { | ||
| const index = parseInt(item.getAttribute("data-item-index") || "-1", 10); | ||
| return ( | ||
| index >= 0 && (index < visibleRange.start || index > visibleRange.end) | ||
| ); | ||
| }); | ||
|
|
||
| const chunk = offscreenItems.slice(0, chunkSize); | ||
| chunk.forEach((item) => { | ||
| const index = parseInt(item.getAttribute("data-item-index") || "-1", 10); | ||
| if (index >= 0) { | ||
| onPrune(index); | ||
| const items = [...container.querySelectorAll("[data-item-index]")].filter( | ||
| (el) => { | ||
| const i = parseInt(el.getAttribute("data-item-index") || "-1", 10); | ||
| return i >= 0 && (i < range.start || i > range.end); | ||
| } | ||
| }); | ||
|
|
||
| if (offscreenItems.length > chunkSize && !cancelled) { | ||
| timeoutId = window.setTimeout(pruneChunk, 16) as unknown as number; | ||
| } | ||
| ); | ||
| items | ||
| .slice(0, chunk) | ||
| .forEach((el) => | ||
| onPrune(parseInt(el.getAttribute("data-item-index")!, 10)) | ||
| ); | ||
| if (items.length > chunk && !cancelled) id = setTimeout(prune, 16); | ||
| }; | ||
|
|
||
| timeoutId = window.setTimeout(pruneChunk, 16) as unknown as number; | ||
|
|
||
| id = setTimeout(prune, 16); | ||
| return () => { | ||
| cancelled = true; | ||
| if (timeoutId !== null) { | ||
| window.clearTimeout(timeoutId as any); | ||
| } | ||
| clearTimeout(id); | ||
| }; | ||
| } |
There was a problem hiding this comment.
Variable and parameter names throughout this file have been shortened to the point of being cryptic (e.g., range, idx, prune, id, i, chunk). This significantly reduces code readability and makes it difficult to maintain. Please use more descriptive names like visibleRange, index, pruneTask, etc., to improve code clarity.
| const prevValue = ref(props.value); | ||
| const lastChangeTime = ref(Date.now()); | ||
| const animationId = ref(0); | ||
| const cleanupTimer = ref<any>(null); |
There was a problem hiding this comment.
Using any for the cleanupTimer ref is not type-safe and bypasses TypeScript's type checking. It's better to provide a more specific type for better safety and code clarity.
const cleanupTimer = ref<ReturnType<typeof setTimeout> | null>(null);
| let renderRange = { | ||
| startIndex: Math.max(0, visibleRange.startIndex - this.overscan), | ||
| endIndex: Math.min(this.count - 1, visibleRange.endIndex + this.overscan), | ||
| startIndex: (visibleRange.startIndex - this.#overscan) | 0, | ||
| endIndex: (visibleRange.endIndex + this.#overscan) | 0, | ||
| }; | ||
|
|
||
| for (const plugin of this.plugins) { | ||
|
|
||
| if (renderRange.startIndex < 0) renderRange.startIndex = 0; | ||
| if (renderRange.endIndex > this.#count - 1) | ||
| renderRange.endIndex = this.#count - 1; |
There was a problem hiding this comment.
This block of code is a more verbose and less clear way of achieving what was previously done with Math.max and Math.min. The | 0 bitwise OR is redundant here as the operands are integers. Using Math.max and Math.min is more idiomatic and readable for clamping values within a range.
| let renderRange = { | |
| startIndex: Math.max(0, visibleRange.startIndex - this.overscan), | |
| endIndex: Math.min(this.count - 1, visibleRange.endIndex + this.overscan), | |
| startIndex: (visibleRange.startIndex - this.#overscan) | 0, | |
| endIndex: (visibleRange.endIndex + this.#overscan) | 0, | |
| }; | |
| for (const plugin of this.plugins) { | |
| if (renderRange.startIndex < 0) renderRange.startIndex = 0; | |
| if (renderRange.endIndex > this.#count - 1) | |
| renderRange.endIndex = this.#count - 1; | |
| let renderRange = { | |
| startIndex: Math.max(0, visibleRange.startIndex - this.#overscan), | |
| endIndex: Math.min(this.#count - 1, visibleRange.endIndex + this.#overscan), | |
| }; |
No description provided.