fix: clean up lint warnings #2648
Conversation
harshsbhat
requested review from
perkinsjr,
chronark,
mcstepp and
MichaelUnkey
as code owners
|
|
@harshsbhat is attempting to deploy a commit to the Unkey Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThis pull request introduces various modifications across multiple components in the application. Key changes include updates to dependency arrays in Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CreateKey
participant Server
User->>CreateKey: Submit Key Creation
CreateKey->>CreateKey: Validate Input
CreateKey->>Server: Send Data
Server-->>CreateKey: Response
CreateKey->>User: Show Result
Assessment against linked issues
Possibly related PRs
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! 🙏 |
|
I am not 1000% sure about one or two of them. I apologize in advance if something is messed up. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (20)
apps/dashboard/components/dashboard/visible-button.tsx (1)
Line range hint
16-22: Consider resetting the timeout when visibility changesThe current implementation only starts a single timeout when the component mounts. This means if a user shows/hides multiple times, the original timeout continues running regardless of user interactions. This could lead to unexpected behavior where the content hides 10 seconds after mount rather than 10 seconds after being shown.
Consider this implementation instead:
React.useEffect(() => { - setTimeout(() => { - setIsVisible(false); - }, 10000); + let timeoutId: NodeJS.Timeout; + if (isVisible) { + timeoutId = setTimeout(() => { + setIsVisible(false); + }, 10000); + } + return () => clearTimeout(timeoutId); - }, [setIsVisible]); + }, [isVisible, setIsVisible]);This will:
- Reset the timer when visibility changes
- Clean up previous timeouts
- Only start the timer when content is visible
apps/dashboard/components/dashboard/copy-button.tsx (1)
Line range hint
11-14: Improve error handling and typing in copyToClipboardWithMetaThe clipboard operation should handle potential errors and properly utilize the async/await pattern.
Consider this improved implementation:
-async function copyToClipboardWithMeta(value: string, _meta?: Record<string, unknown>) { - navigator.clipboard.writeText(value); +async function copyToClipboardWithMeta( + value: string, + meta?: { component?: string } +): Promise<boolean> { + try { + await navigator.clipboard.writeText(value); + return true; + } catch (error) { + console.error('Failed to copy:', error); + return false; + } }Then update the onClick handler to handle potential failures:
- onClick={() => { - copyToClipboardWithMeta(value, { + onClick={async () => { + const success = await copyToClipboardWithMeta(value, { component: src, }); - setHasCopied(true); + setHasCopied(success); }}apps/www/components/copy-button.tsx (1)
Line range hint
11-13: Add error handling for clipboard operations.The clipboard operation could fail due to permissions or API availability. Currently, these errors are silently ignored, which could mislead users into thinking the copy succeeded when it didn't.
Consider this more robust implementation:
-async function copyToClipboardWithMeta(value: string, _meta?: Record<string, unknown>) { - navigator.clipboard.writeText(value); +async function copyToClipboardWithMeta( + value: string, + _meta?: Record<string, unknown> +): Promise<boolean> { + try { + await navigator.clipboard.writeText(value); + return true; + } catch (error) { + console.error('Failed to copy:', error); + return false; + } } // In the onClick handler: - onClick={() => { - copyToClipboardWithMeta(value, { + onClick={async () => { + const success = await copyToClipboardWithMeta(value, { component: src, }); - setHasCopied(true); + setHasCopied(success); }}Also applies to: 34-37
apps/www/app/accelerate/components/rive.tsx (2)
Line range hint
40-47: Replace console.error with proper error handling.Given the PR's objective to clean up lint warnings and ensure no console logs remain, consider implementing proper error handling instead of using
console.error.function onClickDay(day: number) { let el = document.getElementById(`day_${day}`); if (!el) { el = document.getElementById("day_1"); } if (!el) { - console.error("Could not find anchor element"); - return; + return; // Silent fail is acceptable for UI enhancement features } el.scrollIntoView(); }
Line range hint
63-84: Simplify event handling logic.The current implementation uses repetitive if conditions for handling day clicks. Consider refactoring to a more maintainable approach.
const onRiveEventReceived = (riveEvent: any) => { const eventData = riveEvent.data; if (eventData.type === RiveEventType.General) { - if (eventData.name === "click_d1") { - onClickDay(1); - } - if (eventData.name === "click_d2") { - onClickDay(2); - } - if (eventData.name === "click_d3") { - onClickDay(3); - } - if (eventData.name === "click_d4") { - onClickDay(4); - } - if (eventData.name === "click_d5") { - onClickDay(5); - } - if (eventData.name === "click_d6") { - onClickDay(6); - } + const dayMatch = eventData.name.match(/^click_d(\d+)$/); + if (dayMatch) { + onClickDay(Number(dayMatch[1])); + } } };apps/www/components/ui/meteorLines.tsx (2)
Line range hint
68-81: Simplify width handling in useEffectThe window width is only used locally within the effect and doesn't need to be maintained in state. We can simplify this by removing the state and using the width variable directly.
- const [_windowWidth, setWindowWidth] = useState<number>(0); useEffect(() => { const width = window.innerWidth; - setWindowWidth(width); const pos = width / 2 + xPos; const styles = [...new Array(number)].map(() => ({ top: -100, 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); }, [number, xPos, speed, delay]);
Line range hint
8-10: Remove commented-out codeThe
getRandomFromSetfunction is commented out and unused. It should be removed to improve code clarity.-// function getRandomFromSet(set: number[]) { -// return set[Math.floor(Math.random() * set.length)]; -// }apps/www/components/shiny-card.tsx (1)
Line range hint
13-40: Consider implementing proper mouse position trackingThe current implementation might benefit from a more robust approach to mouse position tracking:
- Consider using a debounced mouse position update to prevent excessive re-renders
- Implement proper cleanup in useEffect hooks for mouse position tracking
- Consider using
useCallbackfor the mouse movement handlerExample implementation:
import { debounce } from 'lodash'; const debouncedMouseMove = useCallback( debounce((x: number, y: number) => { // Update mouse position logic here }, 16), // ~60fps [] );apps/dashboard/components/multi-select.tsx (1)
42-63: LGTM! Consider improving the comment about input behavior.The
handleKeyDownimplementation and its dependency array look correct. The function properly handles keyboard events for deleting selections and managing input focus.The comment on line 56 could be more specific about which default behavior is being referenced. Consider:
-// This is not a default behaviour of the <input /> field +// Override default behavior: Escape key normally doesn't blur input fieldsapps/dashboard/app/(app)/settings/user/update-user-image.tsx (1)
Line range hint
22-142: Consider consolidating duplicate file validation logic.The file validation logic is duplicated between the
onChangePicturehandler and the drop handler. Consider extracting it into a reusable function to improve maintainability.Here's a suggested refactor:
+ const validateAndProcessFile = useCallback((file: File) => { + if (file.size / 1024 / 1024 > 2) { + toast.error("File size too big (max 2MB)"); + return false; + } + if (file.type !== "image/png" && file.type !== "image/jpeg") { + toast.error("File type not supported (.png or .jpg only)"); + return false; + } + + const reader = new FileReader(); + reader.onload = (e) => { + setImage(e.target?.result as string); + }; + reader.readAsDataURL(file); + return true; + }, []); const onChangePicture = useCallback( (e: ChangeEvent<HTMLInputElement>) => { toast("Uploading image..."); const file = e.target.files?.[0]; if (!file) { toast.error("No image selected"); return; } - if (file.size / 1024 / 1024 > 2) { - toast.error("File size too big (max 2MB)"); - return; - } - if (file.type !== "image/png" && file.type !== "image/jpeg") { - toast.error("File type not supported (.png or .jpg only)"); - return; - } - - const reader = new FileReader(); - reader.onload = (e) => { - setImage(e.target?.result as string); - }; - reader.readAsDataURL(file); + if (!validateAndProcessFile(file)) { + return; + } if (!user) { toast.error("Only allowed for orgs"); return; } // ... rest of the code }, [user, validateAndProcessFile], ); // In the onDrop handler: onDrop={(e) => { e.preventDefault(); e.stopPropagation(); setDragActive(false); const file = e.dataTransfer.files?.[0]; if (file) { - if (file.size / 1024 / 1024 > 2) { - toast.error("File size too big (max 2MB)"); - } else if (file.type !== "image/png" && file.type !== "image/jpeg") { - toast.error("File type not supported (.png or .jpg only)"); - } else { - const reader = new FileReader(); - reader.onload = (e) => { - setImage(e.target?.result as string); - }; - reader.readAsDataURL(file); - } + validateAndProcessFile(file); } }}apps/dashboard/lib/charts/time-series-chart.tsx (1)
93-93: LGTM with a minor suggestion.The dependency array optimization for the scale calculation is accurate. However, since
paddinghas default values and is not optional, the optional chaining operators (??) in the scale calculation are unnecessary.Consider removing the optional chaining:
- domain: [minY - rangeY * (padding.bottom ?? 0), maxY + rangeY * (padding.top ?? 0)], + domain: [minY - rangeY * padding.bottom, maxY + rangeY * padding.top],apps/dashboard/app/(app)/settings/general/update-workspace-image.tsx (2)
Line range hint
22-142: Consider extracting duplicate file validation logic.The file validation and reading logic is duplicated between the
onChangePictureandonDrophandlers. Consider extracting this into a shared utility function to improve maintainability.Here's a suggested refactor:
const validateAndReadFile = (file: File): Promise<string> => { return new Promise((resolve, reject) => { if (file.size / 1024 / 1024 > 2) { reject(new Error("File size too big (max 2MB)")); return; } if (file.type !== "image/png" && file.type !== "image/jpeg") { reject(new Error("File type not supported (.png or .jpg only)")); return; } const reader = new FileReader(); reader.onload = (e) => resolve(e.target?.result as string); reader.onerror = () => reject(new Error("Error reading file")); reader.readAsDataURL(file); }); }; // Usage in onChangePicture const onChangePicture = useCallback( async (e: ChangeEvent<HTMLInputElement>) => { const file = e.target.files?.[0]; if (!file) { toast.error("No image selected"); return; } try { const imageData = await validateAndReadFile(file); setImage(imageData); if (!organization) { toast.error("Only allowed for orgs"); return; } await organization.setLogo({ file }); toast.success("Image uploaded"); } catch (error) { toast.error(error instanceof Error ? error.message : "Error processing file"); } }, [organization] ); // Similar update for onDrop handler
Line range hint
22-35: Improve user feedback during image upload.The current implementation shows "Uploading image..." toast before validation, which could lead to confusing feedback if validation fails. Consider:
- Moving the upload notification after validation
- Adding loading state UI feedback
Here's a suggested improvement:
const [isUploading, setIsUploading] = useState(false); const onChangePicture = useCallback( async (e: ChangeEvent<HTMLInputElement>) => { const file = e.target.files?.[0]; if (!file) { toast.error("No image selected"); return; } try { setIsUploading(true); // Validate file first if (file.size / 1024 / 1024 > 2) { throw new Error("File size too big (max 2MB)"); } if (file.type !== "image/png" && file.type !== "image/jpeg") { throw new Error("File type not supported (.png or .jpg only)"); } // Then show upload toast toast("Uploading image..."); // Rest of the implementation... } catch (error) { toast.error(error instanceof Error ? error.message : "Error uploading image"); } finally { setIsUploading(false); } }, [organization] ); // Add loading indicator to UI <UploadCloud className={`${ dragActive ? "scale-110" : "scale-100" } h-5 w-5 ${isUploading ? "animate-spin" : ""} text-gray-500 transition-all duration-75 group-hover:scale-110 group-active:scale-95`} />apps/dashboard/app/auth/sign-up/email-signup.tsx (3)
Line range hint
56-58: Remove empty else block.The catch block contains an empty else statement that serves no purpose. Consider either implementing proper error handling or removing the else block entirely.
if (err.errors[0].code === "form_identifier_exists") { toast.error("It looks like you have an account. Please use sign in"); - } else { - } + }
Line range hint
21-74: Consider splitting the useEffect for better maintainability.The current useEffect handles multiple responsibilities (ticket-based and email-based sign-up) and contains complex async logic. Consider splitting it into smaller, focused effects for better maintainability and testing.
Here's a suggested refactor:
// Split into separate effects React.useEffect(() => { const handleTicketSignUp = async (ticket: string) => { try { const result = await signUp?.create({ strategy: "ticket", ticket, }); if (result?.status === "complete" && result.createdSessionId) { await setActive({ session: result.createdSessionId }); router.push("/apis"); } } catch (err) { setTransferLoading(false); setError((err as Error).message); console.error(err); } }; const ticket = new URL(window.location.href).searchParams.get("__clerk_ticket"); if (ticket) { handleTicketSignUp(ticket); } setTransferLoading(false); }, [router, setActive, setError, signUp]); React.useEffect(() => { const handleEmailSignUp = async (email: string) => { try { setVerification(true); await signUp?.create({ emailAddress: email }); await signUp.prepareEmailAddressVerification(); setVerification(true); } catch (err) { if (err.errors[0].code === "form_identifier_exists") { toast.error("It looks like you have an account. Please use sign in"); } } finally { setTransferLoading(false); } }; const email = new URL(window.location.href).searchParams.get("email"); if (email) { handleEmailSignUp(email); } }, [setVerification, signUp]);This refactor:
- Separates concerns into two effects
- Improves error handling with try/catch/finally
- Makes the code more testable
- Reduces cognitive complexity
Line range hint
21-74: Add cleanup function to prevent memory leaks.The useEffect contains async operations but lacks a cleanup function. This could lead to memory leaks if the component unmounts before the operations complete.
Add a cleanup function:
React.useEffect(() => { let isSubscribed = true; const signUpFromParams = async () => { const ticket = new URL(window.location.href).searchParams.get("__clerk_ticket"); const emailParam = new URL(window.location.href).searchParams.get("email"); if (!ticket && !emailParam) { return; } try { if (ticket && isSubscribed) { // ... ticket sign-up logic } if (emailParam && isSubscribed) { // ... email sign-up logic } } finally { if (isSubscribed) { setTransferLoading(false); } } }; signUpFromParams(); return () => { isSubscribed = false; }; }, [router, setActive, setError, setVerification, signUp]);apps/www/components/particles.tsx (1)
Line range hint
89-196: Consider performance and type safety improvements.The particle animation logic could benefit from several improvements:
- The
circleParamsandremapValuefunctions could be memoized since they don't depend on changing values.- The circles array uses
any[]type which could be replaced with the definedCircletype.- The animation logic could be extracted into a custom hook for better separation of concerns.
Example improvements:
- const circles = useRef<any[]>([]); + const circles = useRef<Circle[]>([]); + const circleParams = useMemo(() => () => { const x = Math.floor(Math.random() * canvasSize.current.w); // ... rest of the function return { x, y, translateX, translateY, size, alpha, targetAlpha, dx, dy, magnetism }; + }, [canvasSize.current.w, canvasSize.current.h]); + const remapValue = useMemo(() => ( value: number, start1: number, end1: number, start2: number, end2: number, ): number => { const remapped = ((value - start1) * (end2 - start2)) / (end1 - start1) + start2; return remapped > 0 ? remapped : 0; + }, []);apps/dashboard/app/(app)/settings/root-keys/new/client.tsx (1)
116-116: LGTM! The dependency array update fixes the lint warning while maintaining correctness.The addition of
apisandselectedPermissionsto the dependency array is correct as these values are used within the effect. This change ensures the card states are properly updated when either the APIs list or selected permissions change.Consider adding a brief inline comment about the Set-based optimization:
useEffect(() => { const initialSelectedApiSet = new Set<string>(); + // Using Set for O(1) membership checks to optimize performance selectedPermissions.forEach((permission) => {apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/client.tsx (2)
Line range hint
249-264: Simplify form reset logicThe current implementation uses multiple individual resetField calls, which could be simplified and made more maintainable.
Consider using form.reset with default values:
const resetRateLimit = () => { - form.resetField("ratelimit.duration", undefined); - form.resetField("ratelimit.limit", undefined); - form.resetField("ratelimit", undefined); + form.reset({ + ...form.getValues(), + ratelimit: undefined + }); }; const resetLimited = () => { - form.resetField("limit.refill.amount", undefined); - form.resetField("limit.refill.interval", undefined); - form.resetField("limit.refill", undefined); - form.resetField("limit.remaining", undefined); - form.resetField("limit", undefined); + form.reset({ + ...form.getValues(), + limit: undefined + }); };
Line range hint
92-94: Fix refill day assignment bugThere's a critical bug in the refill day logic. The current code uses
===for comparison instead of=for assignment, which means the refill day is never cleared for daily intervals.Fix the assignment operator:
const refill = values.limit?.refill; if (refill?.interval === "daily") { - refill?.refillDay === undefined; + refill.refillDay = undefined; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (21)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/client.tsx(1 hunks)apps/dashboard/app/(app)/settings/general/update-workspace-image.tsx(1 hunks)apps/dashboard/app/(app)/settings/root-keys/[keyId]/permissions/add-permission-for-api.tsx(1 hunks)apps/dashboard/app/(app)/settings/root-keys/new/client.tsx(1 hunks)apps/dashboard/app/(app)/settings/user/update-user-image.tsx(1 hunks)apps/dashboard/app/auth/sign-in/email-signin.tsx(1 hunks)apps/dashboard/app/auth/sign-up/email-signup.tsx(1 hunks)apps/dashboard/components/dashboard/copy-button.tsx(1 hunks)apps/dashboard/components/dashboard/visible-button.tsx(1 hunks)apps/dashboard/components/multi-select.tsx(1 hunks)apps/dashboard/lib/charts/sparkline.tsx(1 hunks)apps/dashboard/lib/charts/time-series-chart.tsx(3 hunks)apps/dashboard/lib/charts/useTooltip.ts(1 hunks)apps/dashboard/lib/charts/y-axis.tsx(2 hunks)apps/play/app/page-bk.tsx(2 hunks)apps/www/app/accelerate/components/rive.tsx(1 hunks)apps/www/components/copy-button.tsx(1 hunks)apps/www/components/particles.tsx(1 hunks)apps/www/components/shiny-card.tsx(2 hunks)apps/www/components/ui/hover-border-gradient.tsx(1 hunks)apps/www/components/ui/meteorLines.tsx(4 hunks)
🔇 Additional comments (17)
apps/dashboard/lib/charts/useTooltip.ts (1)
53-63: LGTM! Dependency array is now complete and accurate.
The added dependencies (snapToY, snapToX, margin.left) are correctly included as they are used within the callback function. This change ensures proper reactivity when these values change, preventing stale closure bugs.
Let's verify that these dependencies are indeed used within the callback:
✅ Verification successful
Dependencies are correctly included in the dependency array
The verification confirms that all added dependencies are actively used within the callback:
snapToXandsnapToYare used for tooltip positioning logicmargin.leftis used in x-coordinate calculation withconst x = lp.x - margin.left
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of dependencies within the callback
# Expected: Each dependency should be referenced in the callback implementation
# Test: Search for snapToX, snapToY usage in tooltip positioning
rg -A 2 'snapToX.*\?.*:' apps/dashboard/lib/charts/useTooltip.ts
rg -A 2 'snapToY.*\?.*:' apps/dashboard/lib/charts/useTooltip.ts
# Test: Search for margin.left usage in x-coordinate calculation
rg -A 2 'margin\.left' apps/dashboard/lib/charts/useTooltip.ts
Length of output: 700
apps/www/app/accelerate/components/rive.tsx (1)
101-101: LGTM! Correct addition of 'day' to useEffect dependencies.
The addition of 'day' to the dependency array is necessary since it's used within the effect to update animation state (smVarUnlockedUntil.value and smVarHighlight.value). This change ensures the effect properly re-runs when the day prop changes.
apps/www/components/ui/hover-border-gradient.tsx (1)
58-58: LGTM! Proper exhaustive dependencies added to useEffect.
The dependency array update correctly includes all values used within the effect (rotateDirection and duration), fixing potential stale closure issues and ensuring proper reactivity to prop changes. This change aligns with React's exhaustive dependencies rule and addresses lint warnings.
apps/www/components/ui/meteorLines.tsx (1)
38-38: LGTM! Improved useEffect dependencies
The dependency array now correctly includes all props that affect the meteor styles calculation (delay, xPos, direction, speed), ensuring the effect updates when these values change.
apps/dashboard/lib/charts/y-axis.tsx (2)
63-63: LGTM! Added missing dependency to useMemo.
The addition of minY to the dependency array is correct as it's used in the filter condition for tick values.
72-72: LGTM! Added missing dependencies to useEffect.
The addition of tickFormat and setLeftAxisMargin to the dependency array is correct. However, be aware that including setLeftAxisMargin might cause unnecessary re-runs of the effect if the setter function's identity changes between renders. This is generally fine as React guarantees stable setter function identities from useState.
Let's verify if there are any potential infinite loops or performance issues:
✅ Verification successful
The dependency array in useEffect is correct and safe
After analyzing the codebase, I can confirm that the dependency array is properly implemented and safe. The setLeftAxisMargin function is created by useState in the TimeSeriesChartInner component and passed down through the ChartContext. React guarantees the stability of this setter function, and the context value is properly structured. The effect's logic is sound, only updating the margin when necessary to prevent infinite loops.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential re-render issues in parent components
# Look for components that might cause setter function identity changes
# Test: Search for useState declarations that include 'AxisMargin'
rg -A 2 'useState.*AxisMargin'
# Test: Search for components that render YAxis with dynamic props
ast-grep --pattern 'const $_ = $$$; return (<$$$><YAxis $$$tickFormat={$_}/>)'
Length of output: 113
Script:
#!/bin/bash
# Let's check the component implementation to understand the context better
# First, let's look at the file content
cat "apps/dashboard/lib/charts/y-axis.tsx"
# Also search for any imports or usage of this component
rg -l "YAxis"
# And check for any useState declarations in the file
rg "useState.*Margin" "apps/dashboard/lib/charts/y-axis.tsx"
Length of output: 3384
Script:
#!/bin/bash
# Let's check the chart context implementation to see how setLeftAxisMargin is managed
rg -g '!node_modules' -A 5 'const.*ChartContext'
# Also check where the context is being used
rg -g '!node_modules' 'useChartContext'
# And look for any memoization of the context value
ast-grep --pattern 'const $_ = useMemo(() => ({ $$$setLeftAxisMargin$$$}), [$$$]);'
Length of output: 3240
Script:
#!/bin/bash
# Let's check how leftAxisMargin and setLeftAxisMargin are managed in the TimeSeriesChart component
cat "apps/dashboard/lib/charts/time-series-chart.tsx"
# Also check the ChartContextType definition
cat "apps/dashboard/lib/charts/chart-context.ts"
Length of output: 6374
apps/dashboard/app/auth/sign-in/email-signin.tsx (1)
50-50: LGTM! The useEffect dependency array is now complete.
The addition of router, setActive, and signIn to the dependency array is correct as these dependencies are used within the effect's callback. This change properly addresses the lint warning about missing dependencies while maintaining the component's intended behavior.
Let's verify that these dependencies are stable references to prevent unnecessary effect re-runs:
✅ Verification successful
Based on the shell script results, I can now verify the dependencies in the useEffect:
routeris obtained fromuseRouter()hook from Next.js, which is stable across renderssignInandsetActivecome fromuseSignIn()hook from Clerk, which is also stablesignInLoadedis also from the sameuseSignIn()hook
The dependencies are stable and the dependency array is correctly implemented.
The hooks are properly initialized at the top level of the component:
const router = useRouter();
const { signIn, isLoaded: signInLoaded, setActive } = useSignIn();All dependencies are stable references provided by their respective libraries (Next.js and Clerk), so they won't cause unnecessary effect re-runs. The review comment was correct in approving this change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the dependencies are stable references
# Look for proper hook initialization to ensure stable references
# Check router initialization
rg "useRouter" -A 2
# Check clerk hook initialization
rg "useSignIn" -A 2
Length of output: 41330
apps/dashboard/app/(app)/settings/root-keys/[keyId]/permissions/add-permission-for-api.tsx (1)
43-43: LGTM! Important fix for useMemo dependencies.
The addition of props.apis to the dependency array is correct and necessary. This ensures that the selectedApi value is properly recalculated when either the selected ID or the list of available APIs changes, preventing potential stale selections.
Let's verify there are no other useMemo hooks with missing API dependencies:
✅ Verification successful
The useMemo dependency array change is correct and complete
After reviewing the complete file, I can confirm that:
- There is only one useMemo hook in this component
- The dependency array correctly includes both
selectedApiIdandprops.apissince the memoized value depends on both - No other instances of potentially incorrect useMemo dependencies were found in this file
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for useMemo hooks that might reference apis prop but miss it in dependencies
ast-grep --pattern 'useMemo(() => {
$$$
apis
$$$
}, [$$$])'
Length of output: 67
Script:
#!/bin/bash
# Let's try a different approach to find useMemo hooks with potential missing dependencies
# First, let's find the specific file to understand the context better
cat "apps/dashboard/app/(app)/settings/root-keys/[keyId]/permissions/add-permission-for-api.tsx"
Length of output: 4566
apps/dashboard/app/(app)/settings/user/update-user-image.tsx (1)
60-60: LGTM! Correct removal of setImage from dependencies.
The removal of setImage from the dependency array is correct as React state setters are stable and don't need to be included in dependency arrays.
apps/dashboard/lib/charts/sparkline.tsx (1)
92-92: LGTM! Dependency array correctly updated.
The addition of padding.bottom and padding.top to the useMemo dependency array is correct as these values are used in the scale calculations. This change ensures proper recalculation of scales when padding values change, fixing the exhaustive-deps lint warning.
apps/dashboard/lib/charts/time-series-chart.tsx (2)
65-65: LGTM! Correct dependency optimization.
The dependency array optimization for the date range calculation is accurate. The useMemo hook only needs to recalculate when data changes since it's the only value used in the calculation.
77-77: LGTM! Correct dependency optimization.
The dependency array optimization for the Y-axis range calculation is accurate. The useMemo hook correctly depends on both data and series since these are the only values used in calculating the min and max Y values.
apps/dashboard/app/(app)/settings/general/update-workspace-image.tsx (1)
60-60: LGTM! Dependency array correctly simplified.
The removal of setImage from the dependency array is correct as setState functions are stable and don't need to be included. The organization dependency is properly maintained.
Let's verify there are no other instances where we might need to apply similar cleanup:
✅ Verification successful
Dependency array change is correct and safe
After reviewing the complete file context, the removal of setImage from the dependency array of onChangePicture useCallback is correct because:
setImageis from useState and is guaranteed to be stable- The only external dependency used in the callback is
organization, which is properly included - The change aligns with React's hooks lint rules without compromising functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other useCallback hooks with setState in deps
ast-grep --pattern 'useCallback($$$, [$$$setImage$$$])'
Length of output: 57
Script:
#!/bin/bash
# Let's try a broader search for useCallback hooks with set* functions in dependency arrays
ast-grep --pattern 'useCallback($$$, [$$$set$$$])'
# Also search specifically for the current file to understand the context better
cat apps/dashboard/app/\(app\)/settings/general/update-workspace-image.tsx
Length of output: 5981
apps/dashboard/app/auth/sign-up/email-signup.tsx (1)
73-73: LGTM! Dependency array update follows React's exhaustive-deps rule.
The updated dependency array now correctly includes all variables used within the effect: router, setActive, setError, setVerification, and signUp. This change aligns with React's exhaustive-deps lint rule and ensures proper reactivity.
apps/play/app/page-bk.tsx (2)
Line range hint 20-24: LGTM! Correct optimization of useEffect dependencies.
The empty dependency array is appropriate here since the effect only needs to run once on mount to set up the scroll behavior. The scrollRef is correctly accessed via the current property, making it safe to omit from dependencies.
Line range hint 26-37: LGTM! Proper optimization of useCallback dependencies.
The dependency array correctly includes only apiId since the other values (timeStamp, keyId, keyName) are refs whose identity remains stable across renders. This follows React's best practices for hooks and ref usage.
apps/www/components/particles.tsx (1)
66-67: Verify if the refresh prop is still needed.
The refresh prop is defined in the interface but appears unused after removing it from the useEffect dependencies. If manual refresh capability is needed, the dependency should be restored.
| @@ -18,7 +18,7 @@ export function VisibleButton({ | |||
| setTimeout(() => { | |||
| setIsVisible(false); | |||
| }, 10000); | |||
| }, [isVisible]); | |||
| }, [setIsVisible]); | |||
There was a problem hiding this comment.
💡 Codebase verification
The dependency array should include both isVisible and setIsVisible
The current implementation has a useEffect that sets isVisible to false after 10 seconds, but only includes setIsVisible in the dependency array. Since the effect uses setIsVisible to update the visibility state, this is technically correct from a lint perspective (as shown by Biome's useExhaustiveDependencies rule). However, the effect should also react to changes in isVisible to properly reset the timeout when the visibility state changes.
- Add
isVisibleto the dependency array to ensure the timeout is reset whenever visibility changes:
React.useEffect(() => {
setTimeout(() => {
setIsVisible(false);
}, 10000);
}, [setIsVisible, isVisible]);🔗 Analysis chain
Verify the lint rule that triggered this change
Let's verify which lint rule suggested changing the dependency array from [isVisible] to [setIsVisible], as this change might not align with the component's intended behavior.
Let me gather more context about the specific component and its usage of useEffect to better understand the change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for lint configurations and similar patterns in the codebase
# Check for lint configurations
echo "Checking for lint configurations..."
fd -t f "biome.json|.eslintrc" -x cat {}
# Look for similar useEffect patterns with setTimeout
echo -e "\nChecking for similar patterns..."
ast-grep --pattern 'useEffect(() => {
setTimeout($$$)
}, [$_])'
Length of output: 1836
Script:
#!/bin/bash
# Check the implementation of visible-button.tsx
echo "Checking visible-button.tsx implementation..."
fd "visible-button.tsx" -x cat {}
# Look for similar patterns with isVisible state
echo -e "\nChecking for similar isVisible state patterns..."
ast-grep --pattern 'const [isVisible, setIsVisible] = useState'
Length of output: 1350
There was a problem hiding this comment.
This was giving Lint warning
There was a problem hiding this comment.
@harshsbhat, I understand that including isVisible in the dependency array was giving a lint warning.
Including isVisible ensures that the useEffect resets the timeout whenever isVisible changes, which may be important for the intended behavior. If the linter (Biome) is flagging isVisible as an unnecessary dependency, you might consider adjusting the linter configuration to ignore this specific case.
Alternatively, if the component functions correctly without isVisible in the dependency array and the visibility state behaves as expected, it's acceptable to keep the dependency array as [setIsVisible] to avoid the lint warning.
Let me know if you'd like assistance with adjusting the linter settings.
There was a problem hiding this comment.
The bot's right—isVisible is vital for the effect's logic. Without it, the effect only triggers once or when setIsVisible changes, which isn’t the expected behavior. The bot's suggestion from the previous file applies here too.
React.useEffect(() => {
if (!isVisible) return;
const timer = setTimeout(() => { setIsVisible(false); }, 10000);
return () => clearTimeout(timer);
}, [setIsVisible, isVisible]);There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| @@ -21,7 +21,7 @@ export function CopyButton({ value, className, src, ...props }: CopyButtonProps) | |||
| setTimeout(() => { | |||
| setHasCopied(false); | |||
| }, 2000); | |||
| }, [hasCopied]); | |||
| }, []); | |||
There was a problem hiding this comment.
Critical: Incorrect useEffect dependency array implementation
The empty dependency array causes the timer to only run once when the component mounts, which breaks the copy feedback functionality. If a user clicks the copy button multiple times quickly, the visual feedback will be incorrect as the timer won't reset.
Apply this fix to maintain proper functionality while addressing lint warnings:
React.useEffect(() => {
+ if (!hasCopied) return;
const timer = setTimeout(() => {
setHasCopied(false);
}, 2000);
+ return () => clearTimeout(timer);
- }, []);
+ }, [hasCopied]);This solution:
- Only starts the timer when
hasCopiedis true - Cleans up the timer on unmount or when
hasCopiedchanges - Properly resets the timer on each copy action
- Addresses the lint warning by including the dependency
📝 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.
| }, []); | |
| React.useEffect(() => { | |
| if (!hasCopied) return; | |
| const timer = setTimeout(() => { | |
| setHasCopied(false); | |
| }, 2000); | |
| return () => clearTimeout(timer); | |
| }, [hasCopied]); |
| @@ -20,7 +20,7 @@ export function CopyButton({ value, className, src, children, ...props }: CopyBu | |||
| setTimeout(() => { | |||
| setHasCopied(false); | |||
| }, 2000); | |||
| }, [hasCopied]); | |||
| }, []); | |||
There was a problem hiding this comment.
Critical: Empty dependency array breaks copy feedback behavior.
The empty dependency array causes the timeout to only be set once when the component mounts, rather than resetting on each copy action. This leads to inconsistent "copied" state duration and potential race conditions if users click multiple times.
Apply this fix to ensure consistent behavior:
React.useEffect(() => {
- setTimeout(() => {
+ const timeoutId = setTimeout(() => {
setHasCopied(false);
}, 2000);
+ return () => clearTimeout(timeoutId);
- }, []);
+ }, [hasCopied]);This change:
- Properly resets the timeout on each copy action
- Cleans up previous timeouts to prevent memory leaks
- Ensures consistent 2-second feedback duration
📝 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.
| }, []); | |
| React.useEffect(() => { | |
| const timeoutId = setTimeout(() => { | |
| setHasCopied(false); | |
| }, 2000); | |
| return () => clearTimeout(timeoutId); | |
| }, [hasCopied]); |
There was a problem hiding this comment.
the bot forgot about the if statement to check if hasCopied is not false — if it's, the effect makes no sense, does it?
There was a problem hiding this comment.
@unrenamed, you're absolutely correct. Including the if statement to check if hasCopied is true makes sense. Here's the updated code:
React.useEffect(() => {
+ if (!hasCopied) return;
const timeoutId = setTimeout(() => {
setHasCopied(false);
}, 2000);
return () => clearTimeout(timeoutId);
}, [hasCopied]);This ensures the effect only runs when hasCopied is true, preventing unnecessary executions when it's false. Thank you for pointing that out!
| @@ -21,7 +21,7 @@ const MeteorLines = ({ | |||
| className, | |||
| }: MeteorsProps) => { | |||
| const [meteorStyles, setMeteorStyles] = useState<Array<React.CSSProperties>>([]); | |||
| const [windowWidth, _setWindowWidth] = useState<number>(0); | |||
| const [_windowWidth, _setWindowWidth] = useState<number>(0); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused state variable
The _windowWidth state variable is declared but never used in the component. The underscore prefix suggests it was renamed to silence a lint warning, but it would be better to remove it entirely.
- const [_windowWidth, _setWindowWidth] = useState<number>(0);📝 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.
| const [_windowWidth, _setWindowWidth] = useState<number>(0); |
| className = "", | ||
| refresh = false, | ||
| }) => { | ||
| export const ShinyCardGroup: React.FC<ShinyCardGroupProps> = ({ children, className = "" }) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clean up unused prop type
The refresh prop has been removed from the component parameters, but it's still defined in the ShinyCardGroupProps type. This creates an inconsistency that should be addressed.
Remove the unused prop from the type definition:
type ShinyCardGroupProps = {
children: React.ReactNode;
className?: string;
- refresh?: boolean;
};Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
I suggest you don't rush with this one. see the comment below. the prop may be still required
There was a problem hiding this comment.
@unrenamed, understood. We'll keep the refresh prop in the type definition as it may still be required.
| }, []); | ||
|
|
||
| useEffect(() => { | ||
| onMouseMove(); | ||
| }, [mousePosition]); | ||
| }, []); | ||
|
|
||
| useEffect(() => { | ||
| initContainer(); | ||
| }, [refresh]); | ||
| }, []); |
There was a problem hiding this comment.
💡 Codebase verification
Confirmed: useEffect dependencies need fixing and duplicate initialization should be removed
The code inspection reveals:
onMouseMovefunction directly usesmousePositionstate but the useEffect has empty deps array, which will cause stale values- There are indeed two identical
initContaineruseEffect calls onMouseMoveis used to update UI based on mouse position, so it should react to position changes
The original review comment's suggestions are correct and should be implemented:
- Merge the duplicate
initContaineruseEffect calls - Add
mousePositionto the dependency array of theonMouseMoveuseEffect - Keep the resize listener implementation
🔗 Analysis chain
Fix useEffect dependencies and remove duplication
There are several issues with the current useEffect implementations:
- Duplicate initialization: Two identical useEffect hooks calling
initContainer - Missing dependencies:
onMouseMoveusesmousePositionbut it's not in the deps array - Potential stale closure issues with mouse position tracking
Suggested fixes:
- useEffect(() => {
- initContainer();
- window.addEventListener("resize", initContainer);
-
- return () => {
- window.removeEventListener("resize", initContainer);
- };
- }, []);
-
- useEffect(() => {
- onMouseMove();
- }, []);
-
- useEffect(() => {
- initContainer();
- }, []);
+ useEffect(() => {
+ initContainer();
+ window.addEventListener("resize", initContainer);
+
+ return () => {
+ window.removeEventListener("resize", initContainer);
+ };
+ }, []);
+
+ useEffect(() => {
+ onMouseMove();
+ }, [mousePosition]);This change:
- Removes duplicate initialization
- Properly tracks mousePosition changes
- Maintains the resize listener
Let me analyze the component's implementation and dependencies to verify the concerns about useEffect hooks and mousePosition state.
Let me analyze the component implementation to verify the concerns about useEffect hooks and mousePosition dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for similar patterns in other components
rg "useEffect.*\[\]" --type tsx
Length of output: 61
Script:
#!/bin/bash
# Search for useEffect patterns in TypeScript/React files
rg "useEffect.*\[\]" -t ts -t tsx -t jsx
# Get the full context of the ShinyCard component
rg -B 10 -A 10 "ShinyCard" -t ts -t tsx -t jsx
# Look for mousePosition usage
ast-grep --pattern 'const mousePosition = $_'
Length of output: 1963
Script:
#!/bin/bash
# Try with different file extension patterns
rg "useEffect.*\[\]" --type-add 'tsx:*.tsx' --type tsx
# Get the content of shiny-card.tsx
cat apps/www/components/shiny-card.tsx
# Look for onMouseMove implementation
ast-grep --pattern 'function onMouseMove() {
$$$
}'
Length of output: 3525
There was a problem hiding this comment.
looks like my comment just duplicates what the bot already figured out—it's pretty smart, haha!
yeah, they're right, by the way
There was a problem hiding this comment.
@unrenamed, glad you found the analysis helpful! Let me know if there's anything else I can assist with.
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove empty lines to address lint warnings.
Given this PR's objective to clean up lint warnings, these empty lines might be triggering formatting rules.
Apply this diff to maintain consistent spacing:
});
}
}
-
-
// This is not a default behaviour of the <input /> field
if (e.key === "Escape") {
input.blur();
}
}
-
},
[setSelected],Also applies to: 60-60
| onMouseMove(); | ||
| }, [mousePosition.x, mousePosition.y]); | ||
| }, []); |
There was a problem hiding this comment.
Critical: Restore mousePosition dependencies in useEffect.
Removing mousePosition dependencies breaks the interactive nature of the particle animation. The particles won't respond to mouse movement after the initial render since the effect only runs once on mount.
Restore the dependencies to maintain interactivity:
useEffect(() => {
onMouseMove();
- }, []);
+ }, [mousePosition.x, mousePosition.y]);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
the bot is right. the same logic as in apps/www/components/shiny-card.tsx
There was a problem hiding this comment.
@unrenamed, thanks for confirming! I'll keep this in mind for future reviews.
| @@ -263,7 +263,7 @@ export const CreateKey: React.FC<Props> = ({ apiId, keyAuthId, defaultBytes, def | |||
| // React hook form + zod doesn't play nice with nested objects, so we need to reset them on load. | |||
| resetRateLimit(); | |||
| resetLimited(); | |||
| }, []); | |||
| }, [resetRateLimit, resetLimited]); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize useEffect dependencies with useCallback
While adding the dependencies is correct, the reset functions should be memoized to prevent unnecessary effect re-runs.
Wrap the reset functions with useCallback:
- const resetRateLimit = () => {
+ const resetRateLimit = useCallback(() => {
// set them to undefined so the form resets properly.
form.resetField("ratelimit.duration", undefined);
form.resetField("ratelimit.limit", undefined);
form.resetField("ratelimit", undefined);
- };
+ }, [form]);
- const resetLimited = () => {
+ const resetLimited = useCallback(() => {
form.resetField("limit.refill.amount", undefined);
form.resetField("limit.refill.interval", undefined);
form.resetField("limit.refill", undefined);
form.resetField("limit.remaining", undefined);
form.resetField("limit", undefined);
- };
+ }, [form]);Committable suggestion skipped: line range outside the PR's diff.
| @@ -65,7 +65,7 @@ const MeteorLinesAngular = ({ | |||
| className, | |||
| }: MeteorsProps) => { | |||
| const [meteorStyles, setMeteorStyles] = useState<Array<React.CSSProperties>>([]); | |||
| const [windowWidth, setWindowWidth] = useState<number>(0); | |||
| const [_windowWidth, setWindowWidth] = useState<number>(0); | |||
There was a problem hiding this comment.
alternatively, we can just const [, setWindowWidth] = useState<number>(0);
on the other hand, if _windowWidth is no longer used, is there any reason to keep updating it? This state now seems redundant.
| useEffect(() => { | ||
| onMouseMove(); | ||
| }, [mousePosition]); | ||
| }, []); |
There was a problem hiding this comment.
I haven't tested it myself, but from reading the code, it seems incorrect. With this change, the hook calls the onMouseMove event only on initial load. So when the mouse actually moves afterward, the event doesn't get triggered—right?
| }, []); | ||
|
|
||
| useEffect(() => { | ||
| onMouseMove(); | ||
| }, [mousePosition]); | ||
| }, []); | ||
|
|
||
| useEffect(() => { | ||
| initContainer(); | ||
| }, [refresh]); | ||
| }, []); |
There was a problem hiding this comment.
looks like my comment just duplicates what the bot already figured out—it's pretty smart, haha!
yeah, they're right, by the way
| @@ -18,7 +18,7 @@ export function VisibleButton({ | |||
| setTimeout(() => { | |||
| setIsVisible(false); | |||
| }, 10000); | |||
| }, [isVisible]); | |||
| }, [setIsVisible]); | |||
There was a problem hiding this comment.
The bot's right—isVisible is vital for the effect's logic. Without it, the effect only triggers once or when setIsVisible changes, which isn’t the expected behavior. The bot's suggestion from the previous file applies here too.
React.useEffect(() => {
if (!isVisible) return;
const timer = setTimeout(() => { setIsVisible(false); }, 10000);
return () => clearTimeout(timer);
}, [setIsVisible, isVisible]);| @@ -20,7 +20,7 @@ export function CopyButton({ value, className, src, children, ...props }: CopyBu | |||
| setTimeout(() => { | |||
| setHasCopied(false); | |||
| }, 2000); | |||
| }, [hasCopied]); | |||
| }, []); | |||
There was a problem hiding this comment.
the bot forgot about the if statement to check if hasCopied is not false — if it's, the effect makes no sense, does it?
| onMouseMove(); | ||
| }, [mousePosition.x, mousePosition.y]); | ||
| }, []); |
There was a problem hiding this comment.
the bot is right. the same logic as in apps/www/components/shiny-card.tsx
|
|
||
| useEffect(() => { | ||
| initCanvas(); | ||
| }, [refresh]); | ||
| }, []); |
There was a problem hiding this comment.
I'd suggest checking how it affected the component behaviour. smth tells me this effect re-initiates canvas on refresh, hence refresh might be necessary here. the same logic as in apps/www/components/shiny-card.tsx
| @@ -113,7 +113,7 @@ export const Client: React.FC<Props> = ({ apis }) => { | |||
| // for large data sets. | |||
|
|
|||
| setCardStatesMap(initialCardStates); | |||
| }, []); // Execute ones on the first load | |||
| }, [apis, selectedPermissions]); // Execute ones on the first load | |||
There was a problem hiding this comment.
the comment explicitly states that the effect should only run once. Adding dependencies breaks this intended behavior.
| @@ -78,7 +78,7 @@ const MeteorLinesAngular = ({ | |||
| animationDuration: speed ? `${speed}s` : `${Math.floor(Math.random() * 10 + 2)}s`, | |||
| })); | |||
| setMeteorStyles(styles); | |||
| }, [number, windowWidth]); | |||
There was a problem hiding this comment.
the new change doesn’t seem to break existing functionality, but it looks like it wasn’t working as expected before. windowWidth isn’t updated on window resize—it’s set once by the effect and never changes, leading to issues like the meteor lines staying on the left when resizing from tablet to full HD (see screenshot).
cc @MichaelUnkey, Git blame shows you last modified this component. I think the core team should decide whether to fix this. It does not seem critical to me.
| @@ -55,7 +55,7 @@ export function HoverBorderGradient({ | |||
| }, duration * 1000); | |||
| return () => clearInterval(interval); | |||
| } | |||
| }, [hovered]); | |||
| }, [hovered, rotateDirection, duration]); | |||
There was a problem hiding this comment.
the HoverBorderGradient component doesn't seem to be used anymore. Should we remove it entirely? cc @chronark
| @@ -40,7 +40,7 @@ export function DialogAddPermissionsForAPI( | |||
| const [selectedApiId, setSelectedApiId] = useState<string>(""); | |||
| const selectedApi = useMemo( | |||
| () => props.apis.find((api) => api.id === selectedApiId), | |||
| [selectedApiId], | |||
| [selectedApiId, props.apis], | |||
There was a problem hiding this comment.
frequent changes to props.apis can hurt performance, as it's an array of objects and shallow comparison is used for dependencies. Ensure props.apis doesn't change often.
There was a problem hiding this comment.
@unrenamed you are right. See my comment. It was pretty weird to see these things show a lint warning. I will refactor this entirely. Archiving for now
|
Lint warnings aren’t always style issues; they can indicate that components (in the React case) are poorly written and need refactoring to align with React design principles. Some of your changes actually confirmed that so it's great that you started addressing the lint warnings.
Okay, but leaving "broken windows" is not ideal. This can be tackled in stages: either within this PR (just not addressing all points at once) or in future PRs where you're fixing other bugs. |
|
@unrenamed I think I messed up pretty badly here. Closing this for now. Feel free to take over or create a new PR. Sorry once again for a bad PR. cc: @chronark |
What does this PR do?
Fixes #2605
Since the Lint rules were made strict, many new warnings appeared. This PR cleans up those warnings
Type of change
How should this be tested?
pnpm fmtChecklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
Chores