fix: resolve 16 of 54 lint warnings #2652
Conversation
unrenamed
requested review from
perkinsjr,
chronark,
mcstepp and
MichaelUnkey
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@unrenamed is attempting to deploy a commit to the Unkey Team on Vercel. A member of the Team first needs to authorize it. |
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request includes several modifications across multiple components in the dashboard and web application. Key changes involve refactoring state management for copy functionality in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![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
🧹 Outside diff range and nitpick comments (11)
apps/dashboard/components/dashboard/visible-button.tsx (1)
20-22: Consider extracting the timeout duration.The 10000ms timeout value could be extracted to a named constant or made configurable via props for better maintainability and reusability.
+const VISIBILITY_TIMEOUT_MS = 10000; + export function VisibleButton({ className, isVisible, setIsVisible, + timeoutMs = VISIBILITY_TIMEOUT_MS, ...props }: VisibleButtonProps) { useEffect(() => { if (!isVisible) { return; } const timer = setTimeout(() => { setIsVisible(false); - }, 10000); + }, timeoutMs); return () => clearTimeout(timer); }, [setIsVisible, isVisible]);apps/dashboard/components/dashboard/copy-button.tsx (1)
21-28: Consider using a custom hook for copy feedback.While the current implementation is correct, this copy-with-timeout pattern appears in multiple components (as mentioned in the AI summary). This is an opportunity to reduce code duplication.
Consider extracting this logic into a custom hook:
function useCopyFeedback(duration = 2000) { const [copied, setCopied] = React.useState(false); React.useEffect(() => { if (!copied) { return; } const timer = setTimeout(() => { setCopied(false); }, duration); return () => clearTimeout(timer); }, [copied, duration]); return [copied, setCopied] as const; }Usage:
-const [copied, setCopied] = React.useState(false); - -React.useEffect(() => { - if (!copied) { - return; - } - const timer = setTimeout(() => { - setCopied(false); - }, 2000); - return () => clearTimeout(timer); -}, [copied]); +const [copied, setCopied] = useCopyFeedback();apps/www/components/copy-button.tsx (2)
19-27: Consider extracting the timeout duration as a constantThe effect implementation is correct and includes proper cleanup. However, the magic number
2000could be extracted as a named constant for better maintainability.+const COPY_FEEDBACK_DURATION = 2000; // milliseconds + useEffect(() => { if (!copied) { return; } const timer = setTimeout(() => { setCopied(false); - }, 2000); + }, COPY_FEEDBACK_DURATION); return () => clearTimeout(timer); }, [copied]);
41-41: Consider handling clipboard write errorsThe
copyToClipboardWithMetafunction is async but not awaited. Consider handling potential clipboard write errors to provide better user feedback.- onClick={() => { - copyToClipboardWithMeta(value, { + onClick={async () => { + try { + await copyToClipboardWithMeta(value, { + component: src, + }); + setCopied(true); + } catch (error) { + console.error('Failed to copy:', error); + // Optionally show user feedback about the failure + } - component: src, - }); - setCopied(true); }}apps/www/components/ui/meteorLines.tsx (1)
Line range hint
1-99: Consider combining components to reduce code duplicationThe
MeteorLinesandMeteorLinesAngularcomponents share significant logic and structure. Consider combining them into a single component to improve maintainability.Here's a suggested approach:
type MeteorType = 'linear' | 'angular'; interface MeteorsProps { type?: MeteorType; number?: number; xPos?: number; speed?: number; delay?: number; direction?: "left" | "right"; className?: string; } const Meteors = ({ type = 'linear', number = 20, xPos = 60, speed = 10, delay = 0, direction = "left", className, }: MeteorsProps) => { const [windowWidth, setWindowWidth] = useState(window.innerWidth); const [meteorStyles, setMeteorStyles] = useState<Array<React.CSSProperties>>([]); useEffect(() => { const handleResize = () => setWindowWidth(window.innerWidth); window.addEventListener('resize', handleResize); return () => window.removeEventListener('resize', handleResize); }, []); useEffect(() => { const pos = type === 'angular' ? windowWidth / 2 + xPos : direction === "left" ? xPos : windowWidth - (xPos + 75); const styles = [...new Array(number)].map(() => ({ top: type === 'angular' ? -100 : -50, left: pos, animationDelay: delay ? `${delay}s` : `${Math.random() * 1 + 0.2}s`, animationDuration: speed ? `${speed}s` : `${Math.floor(Math.random() * 10 + 2)}s`, })); setMeteorStyles(styles); }, [type, number, direction, xPos, speed, delay, windowWidth]); const meteorClassName = type === 'angular' ? "pointer-events-none absolute left-1/2 top-0 h-[.75px] w-20 rotate-[300deg] animate-meteorAngle rounded-[9999px] bg-gradient-to-r from-white/90 to-transparent shadow-[0_0_0_1px_#ffffff10]" : "-z-20 pointer-events-none absolute left-0 top-0 h-[.75px] w-20 rotate-[90deg] opacity-0 animate-meteor rounded-[9999px] bg-gradient-to-r from-white/90 to-transparent shadow-[0_0_0_1px_#ffffff10]"; return ( <> {meteorStyles.map((style, idx) => ( <span key={idx.toString()} className={clsx(className, meteorClassName)} style={style} > <div className="-z-20 pointer-events-none absolute top-1/2 h-[.75px] w-[500px] -translate-y-1/2 bg-gradient-to-r from-white/10 to-transparent" /> </span> ))} </> ); }; // For backward compatibility const MeteorLines = (props: Omit<MeteorsProps, 'type'>) => <Meteors {...props} type="linear" />; const MeteorLinesAngular = (props: Omit<MeteorsProps, 'type'>) => <Meteors {...props} type="angular" />; export { Meteors, MeteorLines, MeteorLinesAngular };apps/www/components/shiny-card.tsx (2)
38-43: LGTM: Good optimization ofinitContainer.The
useCallbackoptimization is appropriate here since this function is used in multiple effects and event listeners. The empty dependency array is correct as the function only uses refs.Consider adding a type annotation for better maintainability:
-const initContainer = useCallback(() => { +const initContainer = useCallback((): void => {
Line range hint
45-63: Fix missing dependency and consider performance optimization.
- The
boxesstate is used insideonMouseMovebut not included in the dependencies array. This could lead to stale closures.- The function performs expensive DOM operations (
getBoundingClientRect) on every mouse move.Add the missing dependency:
-}, [mousePosition.x, mousePosition.y]); +}, [mousePosition.x, mousePosition.y, boxes]);Consider debouncing or throttling the mouse move handler to reduce the frequency of DOM operations:
import { debounce } from 'lodash'; const debouncedOnMouseMove = useCallback( debounce(() => { // existing onMouseMove logic }, 16), // ~60fps [mousePosition.x, mousePosition.y, boxes] );apps/www/components/ui/copy-code-button.tsx (2)
11-19: Extract timeout duration as a constant.The implementation looks good and follows React best practices. Consider extracting the magic number
2000as a named constant at the top of the file for better maintainability.+const COPY_FEEDBACK_DURATION = 2000; // milliseconds + export function CopyCodeSnippetButton(props: Props) { // ... useEffect(() => { if (!copied) return; const timer = setTimeout(() => { setCopied(false); - }, 2000); + }, COPY_FEEDBACK_DURATION); return () => clearTimeout(timer); }, [copied]);
Line range hint
22-31: Add error handling and improve accessibility for clipboard operations.The current implementation could be enhanced with proper error handling and better user feedback:
- The clipboard operation could fail
- The clipboard API might not be available in all browsers
- No loading state during the copy operation
Consider implementing this improved version:
return ( <button type="button" aria-label="Copy code snippet" + aria-pressed={copied} className={props.className} - onClick={() => { - navigator.clipboard.writeText(props.textToCopy); - setCopied(true); - }} + onClick={async () => { + try { + await navigator.clipboard.writeText(props.textToCopy); + setCopied(true); + } catch (error) { + console.error('Failed to copy:', error); + // Optionally show user feedback about the failure + } + }} > {copied ? <CheckmarkCircle /> : <CopyIcon />} </button> );apps/www/components/particles.tsx (2)
68-72: Consider consolidating canvas initialization logic.There are two effects that call
initCanvas:
- The mount effect (around line 45)
- This refresh effect
This could lead to duplicate initialization if
refreshis true on mount. Consider consolidating the initialization logic into a single effect.useEffect(() => { if (canvasRef.current) { context.current = canvasRef.current.getContext("2d"); } - initCanvas(); animate(); window.addEventListener("resize", initCanvas); return () => { window.removeEventListener("resize", initCanvas); }; }, []); useEffect(() => { - if (!refresh) { - return; - } initCanvas(); -}, [initCanvas, refresh]); +}, [initCanvas, refresh, canvasRef]);
Line range hint
74-86: Consider simplifying mouse movement logic.The
onMouseMovefunction handles multiple responsibilities:
- Getting canvas bounds
- Calculating mouse position relative to canvas
- Checking if mouse is inside canvas
- Updating mouse ref
Consider breaking this down into smaller, focused functions for better maintainability.
+const isInsideCanvas = (x: number, y: number, w: number, h: number) => + x < w / 2 && x > -w / 2 && y < h / 2 && y > -h / 2; +const getRelativeMousePosition = ( + mouseX: number, + mouseY: number, + rect: DOMRect, + canvasSize: { w: number; h: number } +) => ({ + x: mouseX - rect.left - canvasSize.w / 2, + y: mouseY - rect.top - canvasSize.h / 2, +}); const onMouseMove = useCallback(() => { if (canvasRef.current) { const rect = canvasRef.current.getBoundingClientRect(); - const { w, h } = canvasSize.current; - const x = mousePosition.x - rect.left - w / 2; - const y = mousePosition.y - rect.top - h / 2; - const inside = x < w / 2 && x > -w / 2 && y < h / 2 && y > -h / 2; + const pos = getRelativeMousePosition( + mousePosition.x, + mousePosition.y, + rect, + canvasSize.current + ); + const inside = isInsideCanvas( + pos.x, + pos.y, + canvasSize.current.w, + canvasSize.current.h + ); if (inside) { - mouse.current.x = x; - mouse.current.y = y; + mouse.current = pos; } } }, [mousePosition.x, mousePosition.y]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
apps/dashboard/components/dashboard/copy-button.tsx(2 hunks)apps/dashboard/components/dashboard/visible-button.tsx(2 hunks)apps/www/components/copy-button.tsx(3 hunks)apps/www/components/particles.tsx(3 hunks)apps/www/components/shiny-card.tsx(3 hunks)apps/www/components/ui/copy-code-button.tsx(2 hunks)apps/www/components/ui/meteorLines.tsx(2 hunks)
🔇 Additional comments (10)
apps/dashboard/components/dashboard/visible-button.tsx (2)
3-3: LGTM: Import statement is correctly added.
The useEffect import is properly placed and necessary for the hook usage.
16-24: LGTM: Improved useEffect implementation with proper cleanup.
The refactored implementation includes several improvements:
- Early return pattern prevents unnecessary timer setup
- Proper cleanup using clearTimeout
- Correct dependency array ensuring the effect runs when needed
apps/dashboard/components/dashboard/copy-button.tsx (1)
18-18: LGTM! Clear and concise state naming.
The rename from hasCopied to copied improves readability while maintaining the boolean nature of the state.
apps/www/components/copy-button.tsx (3)
5-5: LGTM: Optimized imports
The change to import specific hooks instead of the entire React namespace is a good optimization that follows React best practices.
17-17: LGTM: Improved state variable naming
The rename from hasCopied to copied maintains clarity while being more concise. This change also improves consistency with the dashboard component.
Line range hint 46-50: LGTM: Clean conditional rendering
The conditional rendering is implemented correctly and consistently uses the renamed state variable.
apps/www/components/shiny-card.tsx (3)
5-5: LGTM: Import changes are appropriate.
The addition of useCallback to the imports is necessary for the performance optimizations.
65-67: LGTM: Effect is correctly implemented.
The effect properly reacts to changes in the memoized onMouseMove function.
69-74: LGTM: Clean implementation of conditional container initialization.
The effect correctly handles the refresh prop with a clean guard clause, preventing unnecessary initialization calls.
apps/www/components/particles.tsx (1)
5-5: LGTM: Proper memoization of initCanvas.
The initCanvas function is correctly memoized with empty dependencies since it only uses ref-based functions that are stable across renders.
Also applies to: 62-65
unrenamed
changed the title
|
@chronark I'm ready to submit two more PRs: one for the |
What does this PR do?
Inspired by @harshbhat04’s initiative to address lint warnings in #2648, I’m submitting this PR as the first step toward fully resolving these issues. This PR tackles 16 out of the 54 existing warnings, with 38 remaining.
While this isn't urgent, addressing these warnings gradually helps maintain code quality by preventing "broken windows" from accumulating. Additionally, some warnings reveal potential inefficiencies and minor issues in the components that could benefit from refactoring for better performance and adherence to React best practices.
Partially fixes #2605
Type of change
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes