feat: use AbortController to remove/abort events #1136
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughThe pull request introduces a consistent approach to managing event listeners across multiple components and libraries using Changes
Sequence DiagramsequenceDiagram
participant Component
participant AbortController
participant EventListener
participant Window/Document
Component->>AbortController: Create controller
Component->>Window/Document: Add event listener with controller.signal
Component->>AbortController: Abort when unmounting
AbortController->>EventListener: Cancel listener
The sequence diagram illustrates the typical flow of event listener management using ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 1
🧹 Nitpick comments (13)
docs/src/app/providers.tsx (1)
32-34: Consider adding passive option for better performance.While the AbortController implementation is correct, consider adding the
passive: trueoption to the event listener for better scroll performance, similar to how it's implemented in other components.media.addEventListener("change", onMediaChange, { signal: controller.signal, + passive: true, });packages/svelte/src/lib/component/UploadDropzone.svelte (1)
Line range hint
161-181: Consider extracting paste handler to a separate function.The implementation of AbortController is correct, but the code organization could be improved for better maintainability.
Consider extracting the paste handler to a separate function:
+const createPasteHandler = (rootRef: HTMLElement, onChange: ((files: File[]) => void) | undefined) => (event: ClipboardEvent) => { + if (document.activeElement !== rootRef) return; + const pastedFiles = getFilesFromClipboardEvent(event); + if (!pastedFiles) return; + files = [...files, ...pastedFiles]; + onChange?.(files); + if (mode === "auto") uploadFiles(files); +}; onMount(() => { if (!appendOnPaste) return; const controller = new AbortController() - const handlePaste = (event: ClipboardEvent) => { - if (document.activeElement !== rootRef) return; - const pastedFiles = getFilesFromClipboardEvent(event); - if (!pastedFiles) return; - files = [...files, ...pastedFiles]; - onChange?.(files); - if (mode === "auto") uploadFiles(files); - }; + const handlePaste = createPasteHandler(rootRef, onChange); document.addEventListener("paste", handlePaste, { signal: controller.signal }); return () => controller.abort(); });packages/svelte/src/lib/component/create-dropzone.ts (1)
283-309: LGTM! Comprehensive event management for root element.The implementation properly handles all root element event listeners with AbortController.
Consider grouping related event listeners for better readability:
const dropzoneRoot: Action<HTMLElement> = (node) => { const controller = new AbortController(); rootRef.set(node); node.setAttribute("role", "presentation"); if (!get(props).disabled) { node.setAttribute("tabindex", "0"); + // Keyboard events node.addEventListener("keydown", onKeyDown, { signal: controller.signal }); node.addEventListener("focus", onFocus, { signal: controller.signal }); node.addEventListener("blur", onBlur, { signal: controller.signal }); + // Mouse events node.addEventListener("click", onClick, { signal: controller.signal }); + // Drag events node.addEventListener("dragenter", onDragEnter, { signal: controller.signal }); node.addEventListener("dragover", onDragOver, { signal: controller.signal }); node.addEventListener("dragleave", onDragLeave, { signal: controller.signal }); node.addEventListener("drop", onDropCb, { signal: controller.signal }); } // ... };docs/src/components/Search.tsx (1)
300-314: LGTM! Clean implementation of AbortController.The event listener management is well-implemented with proper cleanup.
Consider these minor improvements:
- window.addEventListener("keydown", onKeyDown, { - signal: controller.signal, - }); + window.addEventListener("keydown", onKeyDown, { signal: controller.signal });packages/solid/src/components/dropzone.tsx (3)
Line range hint
203-221: Consider debouncing the paste handler.While the AbortController implementation is correct, the paste handler immediately modifies state and triggers an upload. Consider debouncing these operations to prevent rapid consecutive paste events from causing performance issues.
+ const debouncedUpload = debounce((filesToUpload: File[]) => { + if (mode === "auto") uploadFiles(filesToUpload); + }, 300); const pasteHandler = (e: ClipboardEvent) => { if (document.activeElement !== rootRef) return; const pastedFiles = getFilesFromClipboardEvent(e); if (!pastedFiles) return; setFiles((prevFiles) => [...prevFiles, ...pastedFiles]); $props.onChange?.(files()); - if (mode === "auto") uploadFiles(files()); + debouncedUpload(files()); };
Line range hint
377-398: Consider extracting the timeout delay to a constant.The implementation is correct, but the setTimeout delay is hardcoded. Consider extracting it to a named constant for better maintainability.
+ const FILE_DIALOG_CHECK_DELAY = 300; const onWindowFocus = () => { if (state.isFileDialogActive) { - setTimeout(() => { + setTimeout(() => { const input = inputRef(); if (input) { const { files } = input; if (!files?.length) { setState({ isFileDialogActive: false }); } } - }, 300); + }, FILE_DIALOG_CHECK_DELAY); } }; - window.addEventListener("focus", onWindowFocus, { - capture: false, - signal: controller.signal, - }); + window.addEventListener("focus", onWindowFocus, { signal: controller.signal });
Line range hint
403-427: LGTM! Clean implementation of drag event handlers.The event listeners are well-managed with proper cleanup.
Consider simplifying the event listener options:
- document.addEventListener("dragover", onDocumentDragOver, { - capture: false, - signal: controller.signal, - }); - document.addEventListener("drop", onDocumentDrop, { - capture: false, - signal: controller.signal, - }); + document.addEventListener("dragover", onDocumentDragOver, { signal: controller.signal }); + document.addEventListener("drop", onDocumentDrop, { signal: controller.signal });packages/react/src/components/dropzone.tsx (2)
453-458: Maintain consistency with timeout delays across implementations.For consistency with other implementations, consider extracting the timeout delay to a constant.
+ const FILE_DIALOG_CHECK_DELAY = 300; // Same value used in Solid implementation if (state.isFileDialogActive) { setTimeout(() => { if (inputRef.current) { const { files } = inputRef.current; if (!files?.length) { dispatch({ type: "closeDialog" }); } } - }, 300); + }, FILE_DIALOG_CHECK_DELAY); }
472-484: LGTM! Clean implementation of drag event handlers.The event listeners are well-managed with proper cleanup.
Consider simplifying the event listener options:
- document.addEventListener("dragover", onDocumentDragOver, { - capture: false, - signal: controller.signal, - }); - document.addEventListener("drop", onDocumentDrop, { - capture: false, - signal: controller.signal, - }); + document.addEventListener("dragover", onDocumentDragOver, { signal: controller.signal }); + document.addEventListener("drop", onDocumentDrop, { signal: controller.signal });packages/vue/src/components/dropzone.tsx (1)
Line range hint
437-505: LGTM! Clean implementation with a single controller.The Vue implementation efficiently uses a single AbortController for all event listeners with proper cleanup in onUnmounted.
Consider these improvements for consistency with other implementations:
+ const FILE_DIALOG_CHECK_DELAY = 300; // Same value used in React and Solid implementations if (state.isFileDialogActive) { setTimeout(() => { if (inputRef.value) { const { files } = inputRef.value; if (!files?.length) { state.isFileDialogActive = false; } } - }, 300); + }, FILE_DIALOG_CHECK_DELAY); } - window.addEventListener("focus", onWindowFocus, { - capture: false, - signal: controller.signal, - }); - document.addEventListener("dragover", onDocumentDragOver, { - capture: false, - signal: controller.signal, - }); - document.addEventListener("drop", onDocumentDrop, { - capture: false, - signal: controller.signal, - }); + window.addEventListener("focus", onWindowFocus, { signal: controller.signal }); + document.addEventListener("dragover", onDocumentDragOver, { signal: controller.signal }); + document.addEventListener("drop", onDocumentDrop, { signal: controller.signal });packages/svelte/src/lib/component/UploadButton.svelte (3)
Line range hint
111-127: Consider storing the AbortController in component state.While the AbortController implementation is correct, consider storing it in component state to ensure consistent access across component lifecycle events. This would allow for manual abortion if needed before component destruction.
- const controller = new AbortController() + $: controller = $: controller ?? new AbortController()
Line range hint
111-130: Add error handling for paste events.The paste event handler should include error handling to prevent potential crashes and improve debugging capabilities.
const handlePaste = (event: ClipboardEvent) => { + try { if (!appendOnPaste) return; if (document.activeElement !== fileInputRef) return; const pastedFiles = getFilesFromClipboardEvent(event); if (!pastedFiles) return; files = [...files, ...pastedFiles]; onChange?.(files); if (mode === "auto") uploadFiles(files); + } catch (error) { + console.error("Error handling paste event:", error); + } };
127-127: Consider using passive event listener for performance.Since the paste event handler doesn't use
preventDefault(), you can optimize performance by adding thepassiveoption.- document.addEventListener("paste", handlePaste, { signal: controller.signal }); + document.addEventListener("paste", handlePaste, { signal: controller.signal, passive: true });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
docs/src/app/providers.tsx(2 hunks)docs/src/components/Search.tsx(1 hunks)docs/src/components/SectionProvider.tsx(1 hunks)packages/react/src/components/dropzone.tsx(2 hunks)packages/react/src/utils/usePaste.ts(1 hunks)packages/solid/src/components/button.tsx(2 hunks)packages/solid/src/components/dropzone.tsx(5 hunks)packages/svelte/src/lib/component/UploadButton.svelte(2 hunks)packages/svelte/src/lib/component/UploadDropzone.svelte(2 hunks)packages/svelte/src/lib/component/create-dropzone.ts(7 hunks)packages/vue/src/components/dropzone.tsx(2 hunks)packages/vue/src/components/shared.tsx(1 hunks)
🔇 Additional comments (6)
packages/react/src/utils/usePaste.ts (1)
10-15: Clean implementation of AbortController!The implementation correctly manages the paste event listener lifecycle using AbortController, following React best practices with proper cleanup in the useEffect hook.
docs/src/components/SectionProvider.tsx (1)
110-122: Excellent implementation of event listener management!The implementation demonstrates best practices by:
- Using AbortController for both scroll and resize events
- Including the passive option for scroll performance
- Properly cleaning up requestAnimationFrame
packages/solid/src/components/button.tsx (1)
Line range hint
159-176: LGTM! Consistent implementation of AbortController.The implementation aligns well with the PR's objective and maintains consistency with other framework implementations. The cleanup is properly handled using
onCleanup(() => controller.abort()).packages/svelte/src/lib/component/create-dropzone.ts (3)
Line range hint
55-77: LGTM! Clean implementation of AbortController for window focus events.The implementation properly manages the window focus event listener lifecycle.
Line range hint
81-105: LGTM! Well-structured implementation for document drag events.The implementation effectively manages document-level drag and drop event listeners.
Line range hint
318-346: LGTM! Proper event management for input element.The implementation correctly handles input element event listeners and cleanup.
Following Theo's video about AbortController, this PR introduces it to manage events and abort them all together instead of using
removeEventListener.Summary by CodeRabbit
Refactor
AbortControllerPerformance