-
Notifications
You must be signed in to change notification settings - Fork 0
Fix ESLint warnings for fast refresh and hook dependencies #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- Move UI variant definitions to separate files to fix fast refresh warnings - Create centralized toast import to avoid component export issues - Add missing dependencies to React useEffect hooks - Improve code organization and maintainability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR addresses ESLint warnings related to React Fast Refresh and hook dependencies by centralizing toast imports, extracting UI variant definitions to separate files, and adding missing dependencies to React hooks.
Key Changes:
- Created a centralized toast wrapper (
@/lib/toast) to avoid component export issues with Fast Refresh - Extracted
badgeVariantsandtoggleVariantsto separate variant files following a pattern to fix Fast Refresh warnings - Added missing dependencies to
useEffecthooks inSpotifyPlaylistSyncandSpotifyPlayer
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/toast.ts | New centralized toast re-export wrapper |
| src/pages/Rooms.tsx | Updated toast import to use centralized wrapper |
| src/pages/Login.tsx | Updated toast import to use centralized wrapper |
| src/pages/Index.tsx | Updated toast import to use centralized wrapper |
| src/hooks/useSocket.ts | Updated toast import to use centralized wrapper |
| src/components/SpotifyPlaylistSync.tsx | Updated toast import and added url to useEffect dependencies |
| src/components/SpotifyPlayer.tsx | Added currentTrack to useEffect dependencies |
| src/components/ui/sonner.tsx | Removed toast export to fix Fast Refresh warnings |
| src/components/ui/badge.tsx | Refactored to import variants from separate file |
| src/components/ui/badge-variants.ts | New file containing extracted badge variants |
| src/components/ui/toggle.tsx | Refactored to import variants from separate file, removed variants export |
| src/components/ui/toggle-variants.ts | New file containing extracted toggle variants |
| src/components/ui/button-variants.ts | New unused file with incomplete button variant definitions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Toggle.displayName = TogglePrimitive.Root.displayName; | ||
|
|
||
| export { Toggle, toggleVariants }; | ||
| export { Toggle }; |
Copilot
AI
Dec 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The toggleVariants export was removed, but it's still being imported by src/components/ui/toggle-group.tsx on line 6. This will cause a build error. Either re-export toggleVariants here, or update toggle-group.tsx to import from ./toggle-variants instead.
| export { Toggle }; | |
| export { Toggle }; | |
| export { toggleVariants, ToggleVariants }; |
| setUrl(playlistUrl); | ||
| } | ||
| }, [playlistUrl]); | ||
| }, [playlistUrl, url]); |
Copilot
AI
Dec 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding url to the dependency array is problematic because this effect sets url via setUrl(playlistUrl). While the condition if (playlistUrl && !url) prevents infinite loops, including url in dependencies means the effect will re-run whenever url changes for any reason (e.g., when a playlist is created in handleCreatePlaylist), potentially causing unexpected behavior. The dependency array should only include playlistUrl since that's the external prop being tracked.
| }, [playlistUrl, url]); | |
| }, [playlistUrl]); |
| play(spotifyUri, startPosition); | ||
| } | ||
| }, [currentTrack?.id, isReady, isPremium, play, playbackState, playbackMode]); | ||
| }, [currentTrack?.id, isReady, isPremium, play, playbackState, playbackMode, currentTrack]); |
Copilot
AI
Dec 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding the entire currentTrack object to the dependency array is redundant and inefficient since currentTrack?.id is already included. The effect should only re-run when the track ID changes, not when other properties of the currentTrack object change. This could cause unnecessary re-renders and re-executions of the effect. Remove currentTrack from the dependency array.
| }, [currentTrack?.id, isReady, isPremium, play, playbackState, playbackMode, currentTrack]); | |
| }, [currentTrack?.id, isReady, isPremium, play, playbackState, playbackMode]); |
| import { cva, type VariantProps } from "class-variance-authority"; | ||
|
|
||
| export const buttonVariants = cva( | ||
| "inline-flex items-center justify-center whitespace-nowrap rounded-md text-sm font-medium ring-offset-background transition-colors focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2 disabled:pointer-events-none disabled:opacity-50", | ||
| { | ||
| variants: { | ||
| variant: { | ||
| default: "bg-primary text-primary-foreground hover:bg-primary/90", | ||
| destructive: | ||
| "bg-destructive text-destructive-foreground hover:bg-destructive/90", | ||
| outline: | ||
| "border border-input bg-background hover:bg-accent hover:text-accent-foreground", | ||
| secondary: | ||
| "bg-secondary text-secondary-foreground hover:bg-secondary/80", | ||
| ghost: "hover:bg-accent hover:text-accent-foreground", | ||
| link: "text-primary underline-offset-4 hover:underline", | ||
| }, | ||
| size: { | ||
| default: "h-10 px-4 py-2", | ||
| sm: "h-9 rounded-md px-3", | ||
| lg: "h-11 rounded-md px-8", | ||
| icon: "h-10 w-10", | ||
| }, | ||
| }, | ||
| defaultVariants: { | ||
| variant: "default", | ||
| size: "default", | ||
| }, | ||
| } | ||
| ); | ||
|
|
||
| export type ButtonVariants = VariantProps<typeof buttonVariants>; No newline at end of file |
Copilot
AI
Dec 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The button-variants.ts file was created but is not being used anywhere. The button.tsx file still defines its own buttonVariants and exports it (which is needed by alert-dialog.tsx, calendar.tsx, and pagination.tsx). Additionally, the variant definitions are out of sync - button.tsx includes gap-2 [&_svg]:pointer-events-none [&_svg]:size-4 [&_svg]:shrink-0 in the base classes (line 8), but button-variants.ts does not (line 4). Either complete the refactoring by updating button.tsx to import from this file, or remove this unused file to avoid confusion and maintenance issues.
| import { cva, type VariantProps } from "class-variance-authority"; | |
| export const buttonVariants = cva( | |
| "inline-flex items-center justify-center whitespace-nowrap rounded-md text-sm font-medium ring-offset-background transition-colors focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2 disabled:pointer-events-none disabled:opacity-50", | |
| { | |
| variants: { | |
| variant: { | |
| default: "bg-primary text-primary-foreground hover:bg-primary/90", | |
| destructive: | |
| "bg-destructive text-destructive-foreground hover:bg-destructive/90", | |
| outline: | |
| "border border-input bg-background hover:bg-accent hover:text-accent-foreground", | |
| secondary: | |
| "bg-secondary text-secondary-foreground hover:bg-secondary/80", | |
| ghost: "hover:bg-accent hover:text-accent-foreground", | |
| link: "text-primary underline-offset-4 hover:underline", | |
| }, | |
| size: { | |
| default: "h-10 px-4 py-2", | |
| sm: "h-9 rounded-md px-3", | |
| lg: "h-11 rounded-md px-8", | |
| icon: "h-10 w-10", | |
| }, | |
| }, | |
| defaultVariants: { | |
| variant: "default", | |
| size: "default", | |
| }, | |
| } | |
| ); | |
| export type ButtonVariants = VariantProps<typeof buttonVariants>; |
| import { useAuth } from '@/contexts/AuthContext'; | ||
| import { Room, Track, SpotifyUser } from '@/types/wejay'; | ||
| import { toast } from 'sonner'; | ||
| import { toast } from '@/lib/toast'; |
Copilot
AI
Dec 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused import toast.
| import { toast } from '@/lib/toast'; |
- Complete button-variants.ts with proper variant definitions - Update button.tsx to use separate variants file - Remove buttonVariants export to fix fast refresh warning - Addresses Copilot review comment about incomplete button-variants file
Summary
Changes
badgeVariants,toggleVariantsto separate files@/lib/toastwrapper to centralize toast importsTesting