Skip to content

fix: clean up lint warnings #2648

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.

Sign up for GitHub

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

Closed
harshsbhat wants to merge 2 commits into from
Closed

fix: clean up lint warnings #2648

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2426fb2
fix: clean up lint warnings
harshsbhat Nov 6, 2024
a9ca96e
Merge branch 'main' into harshbhat/lint-warnings
harshsbhat Nov 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/client.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]);
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.


return (
<>
Expand Down
2 changes: 1 addition & 1 deletion apps/dashboard/app/(app)/settings/general/update-workspace-image.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ export const UpdateWorkspaceImage: React.FC = () => {
toast.error("Error uploading image");
});
},
[setImage, organization],
[organization],
);

if (!organization) {
Expand Down
2 changes: 1 addition & 1 deletion apps/dashboard/app/(app)/settings/root-keys/[keyId]/permissions/add-permission-for-api.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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],
Copy link
Copy Markdown
Contributor

@unrenamed unrenamed Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@harshsbhat harshsbhat Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

);

const isSelectionDisabled =
Expand Down
2 changes: 1 addition & 1 deletion apps/dashboard/app/(app)/settings/root-keys/new/client.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Copy Markdown
Contributor

@unrenamed unrenamed Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the comment explicitly states that the effect should only run once. Adding dependencies breaks this intended behavior.


return (
<div className="flex flex-col gap-4">
Expand Down
2 changes: 1 addition & 1 deletion apps/dashboard/app/(app)/settings/user/update-user-image.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ export const UpdateUserImage: React.FC = () => {
toast.error("Error uploading image");
});
},
[setImage, user],
[user],
);

return (
Expand Down
2 changes: 1 addition & 1 deletion apps/dashboard/app/auth/sign-in/email-signin.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ export function EmailSignIn(props: {
});
};
signUpOrgUser();
}, [signInLoaded]);
}, [router, setActive, signIn, signInLoaded]);

const signInWithCode = async (e: React.FormEvent<HTMLFormElement>) => {
e.preventDefault();
Expand Down
2 changes: 1 addition & 1 deletion apps/dashboard/app/auth/sign-up/email-signup.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ export const EmailSignUp: React.FC<Props> = ({ setError, setVerification }) => {
};
signUpFromParams();
setTransferLoading(false);
}, [signUpLoaded]);
}, [router, setActive, setError, setVerification, signUp]);

const signUpWithCode = async (e: React.FormEvent<HTMLFormElement>) => {
e.preventDefault();
Expand Down
2 changes: 1 addition & 1 deletion apps/dashboard/components/dashboard/copy-button.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ export function CopyButton({ value, className, src, ...props }: CopyButtonProps)
setTimeout(() => {
setHasCopied(false);
}, 2000);
}, [hasCopied]);
}, []);
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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:

  1. Only starts the timer when hasCopied is true
  2. Cleans up the timer on unmount or when hasCopied changes
  3. Properly resets the timer on each copy action
  4. 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.

Suggested change
}, []);
React.useEffect(() => {
if (!hasCopied) return;
const timer = setTimeout(() => {
setHasCopied(false);
}, 2000);
return () => clearTimeout(timer);
}, [hasCopied]);


return (
<button
Expand Down
2 changes: 1 addition & 1 deletion apps/dashboard/components/dashboard/visible-button.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ export function VisibleButton({
setTimeout(() => {
setIsVisible(false);
}, 10000);
}, [isVisible]);
}, [setIsVisible]);
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Nov 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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 isVisible to 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

Copy link
Copy Markdown
Contributor Author

@harshsbhat harshsbhat Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was giving Lint warning

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Copy Markdown
Contributor

@unrenamed unrenamed Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]);

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!


return (
<button
Expand Down
35 changes: 19 additions & 16 deletions apps/dashboard/components/multi-select.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,25 +39,28 @@ export const MultiSelect: React.FC<Props> = ({ options, placeholder, selected, s
setSelected((prev) => prev.filter((s) => s.value !== o.value));
};

const handleKeyDown = React.useCallback((e: React.KeyboardEvent<HTMLDivElement>) => {
const input = inputRef.current;
if (input) {
if (e.key === "Delete" || e.key === "Backspace") {
if (input.value === "") {
setSelected((prev) => {
const newSelected = [...prev];
newSelected.pop();
return newSelected;
});
const handleKeyDown = React.useCallback(
(e: React.KeyboardEvent<HTMLDivElement>) => {
const input = inputRef.current;
if (input) {
if (e.key === "Delete" || e.key === "Backspace") {
if (input.value === "") {
setSelected((prev) => {
const newSelected = [...prev];
newSelected.pop();
return newSelected;
});
}
}
}

Comment on lines 54 to 55
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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

// This is not a default behaviour of the <input /> field
if (e.key === "Escape") {
input.blur();
// This is not a default behaviour of the <input /> field
if (e.key === "Escape") {
input.blur();
}
}
}
}, []);
},
[setSelected],
);

const selectables = options.filter((o) => !selected.includes(o));

Expand Down
2 changes: 1 addition & 1 deletion apps/dashboard/lib/charts/sparkline.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ function SparkLineInner<T extends Datum>({
range: [0, width],
}),
};
}, [startDate, endDate, minY, maxY, height, width]);
}, [startDate, endDate, minY, maxY, height, width, padding.bottom, padding.top]);

const chartContext: ChartContextType<T> = {
width,
Expand Down
6 changes: 3 additions & 3 deletions apps/dashboard/lib/charts/time-series-chart.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ function TimeSeriesChartInner<T extends Datum>({
startDate: dates[times.indexOf(Math.min(...times))],
endDate: dates[times.indexOf(Math.max(...times))],
};
}, data);
}, [data]);

const { minY, maxY } = useMemo(() => {
const values = series
Expand All @@ -74,7 +74,7 @@ function TimeSeriesChartInner<T extends Datum>({
minY: Math.min(...values),
maxY: Math.max(...values),
};
}, [data, series, padding?.bottom, padding?.top]);
}, [data, series]);

const { yScale, xScale } = useMemo(() => {
const rangeY = maxY - minY;
Expand All @@ -90,7 +90,7 @@ function TimeSeriesChartInner<T extends Datum>({
range: [0, width],
}),
};
}, [startDate, endDate, minY, maxY, height, width, margin]);
}, [startDate, endDate, minY, maxY, height, width, padding.bottom, padding.top]);

const chartContext: ChartContextType<T> = {
width,
Expand Down
12 changes: 11 additions & 1 deletion apps/dashboard/lib/charts/useTooltip.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,17 @@ export function useTooltip<T extends Datum>({
tooltipTop: snapToY ? yScale(series.find((s) => s.id === seriesId)!.valueAccessor(d)) : 0,
});
},
[seriesId, data, xScale, yScale, series, visxTooltip.showTooltip],
[
seriesId,
data,
xScale,
yScale,
series,
visxTooltip.showTooltip,
snapToY,
snapToX,
margin.left,
],
);

const TooltipWrapper = renderInPortal ? visxTooltipInPortal.TooltipInPortal : TooltipWithBounds;
Expand Down
4 changes: 2 additions & 2 deletions apps/dashboard/lib/charts/y-axis.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ export default function YAxis({
// Both of these reduce the number of ticks farther below numTicks, but this should only affect small ranges
value >= minY && integerTicks ? Number.isInteger(value) : true,
);
}, [tickValuesProp, numTicksProp, height, yScale, integerTicks]);
}, [tickValuesProp, numTicksProp, height, yScale, integerTicks, minY]);

useEffect(() => {
const maxWidth =
Expand All @@ -69,7 +69,7 @@ export default function YAxis({
if ((leftAxisMargin ?? 0) < maxWidth) {
setLeftAxisMargin(maxWidth);
}
}, [tickValues, tickAxisSpacing, leftAxisMargin]);
}, [tickValues, tickAxisSpacing, leftAxisMargin, tickFormat, setLeftAxisMargin]);

return (
<>
Expand Down
4 changes: 2 additions & 2 deletions apps/play/app/page-bk.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ export default function Home() {

useEffect(() => {
scrollRef?.current?.scrollIntoView({ behavior: "smooth" });
}, [scrollRef]);
}, []);

const parseCurlCommand = useCallback(
(stepString: string) => {
Expand All @@ -34,7 +34,7 @@ export default function Home() {
tempString = keyName.current ? tempString.replace("<key>", keyName.current) : tempString;
return tempString;
},
[apiId, keyId, keyName, timeStamp],
[apiId],
);
function handleSubmit(cmd: string) {
postNewLine(cmd, "text-violet-500");
Expand Down
2 changes: 1 addition & 1 deletion apps/www/app/accelerate/components/rive.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ const RiveAccelerateAsset = ({ day, stateMachines }: { day: number; stateMachine
r.rive.on(EventType.RiveEvent, onRiveEventReceived);

setDone(true);
}, [done, r.rive, smVarHighlight, smVarUnlockedUntil]);
}, [done, r.rive, smVarHighlight, smVarUnlockedUntil, day]);

return <r.RiveComponent />;
};
2 changes: 1 addition & 1 deletion apps/www/components/copy-button.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ export function CopyButton({ value, className, src, children, ...props }: CopyBu
setTimeout(() => {
setHasCopied(false);
}, 2000);
}, [hasCopied]);
}, []);
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Nov 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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:

  1. Properly resets the timeout on each copy action
  2. Cleans up previous timeouts to prevent memory leaks
  3. 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.

Suggested change
}, []);
React.useEffect(() => {
const timeoutId = setTimeout(() => {
setHasCopied(false);
}, 2000);
return () => clearTimeout(timeoutId);
}, [hasCopied]);

Copy link
Copy Markdown
Contributor

@unrenamed unrenamed Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the bot forgot about the if statement to check if hasCopied is not false — if it's, the effect makes no sense, does it?

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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!


return (
<button
Expand Down
5 changes: 2 additions & 3 deletions apps/www/components/particles.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ export const Particles: React.FC<ParticlesProps> = ({
quantity = 30,
staticity = 50,
ease = 50,
refresh = false,
color = "#ffffff",
vx = 0,
vy = 0,
Expand Down Expand Up @@ -61,11 +60,11 @@ export const Particles: React.FC<ParticlesProps> = ({

useEffect(() => {
onMouseMove();
}, [mousePosition.x, mousePosition.y]);
}, []);
Comment on lines 62 to +63
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Nov 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Copy link
Copy Markdown
Contributor

@unrenamed unrenamed Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the bot is right. the same logic as in apps/www/components/shiny-card.tsx

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@unrenamed, thanks for confirming! I'll keep this in mind for future reviews.


useEffect(() => {
initCanvas();
}, [refresh]);
}, []);
Copy link
Copy Markdown
Contributor

@unrenamed unrenamed Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


const initCanvas = () => {
resizeCanvas();
Expand Down
12 changes: 4 additions & 8 deletions apps/www/components/shiny-card.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,7 @@ type ShinyCardGroupProps = {
refresh?: boolean;
};

export const ShinyCardGroup: React.FC<ShinyCardGroupProps> = ({
children,
className = "",
refresh = false,
}) => {
export const ShinyCardGroup: React.FC<ShinyCardGroupProps> = ({ children, className = "" }) => {
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Nov 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Copy link
Copy Markdown
Contributor

@unrenamed unrenamed Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest you don't rush with this one. see the comment below. the prop may be still required

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@unrenamed, understood. We'll keep the refresh prop in the type definition as it may still be required.

const containerRef = useRef<HTMLDivElement>(null);
const mousePosition = useMousePosition();
const mouse = useRef<{ x: number; y: number }>({ x: 0, y: 0 });
Expand All @@ -33,15 +29,15 @@ export const ShinyCardGroup: React.FC<ShinyCardGroupProps> = ({
return () => {
window.removeEventListener("resize", initContainer);
};
}, [setBoxes]);
}, []);

useEffect(() => {
onMouseMove();
}, [mousePosition]);
}, []);
Comment on lines 34 to +36
Copy link
Copy Markdown
Contributor

@unrenamed unrenamed Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(() => {
initContainer();
}, [refresh]);
}, []);
Comment on lines +32 to +40
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Nov 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

⚠️ Potential issue

Confirmed: useEffect dependencies need fixing and duplicate initialization should be removed

The code inspection reveals:

  1. onMouseMove function directly uses mousePosition state but the useEffect has empty deps array, which will cause stale values
  2. There are indeed two identical initContainer useEffect calls
  3. onMouseMove is 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 initContainer useEffect calls
  • Add mousePosition to the dependency array of the onMouseMove useEffect
  • Keep the resize listener implementation
🔗 Analysis chain

Fix useEffect dependencies and remove duplication

There are several issues with the current useEffect implementations:

  1. Duplicate initialization: Two identical useEffect hooks calling initContainer
  2. Missing dependencies: onMouseMove uses mousePosition but it's not in the deps array
  3. 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

Copy link
Copy Markdown
Contributor

@unrenamed unrenamed Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like my comment just duplicates what the bot already figured out—it's pretty smart, haha!

yeah, they're right, by the way

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@unrenamed, glad you found the analysis helpful! Let me know if there's anything else I can assist with.


const initContainer = () => {
if (containerRef.current) {
Expand Down
2 changes: 1 addition & 1 deletion apps/www/components/ui/hover-border-gradient.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ export function HoverBorderGradient({
}, duration * 1000);
return () => clearInterval(interval);
}
}, [hovered]);
}, [hovered, rotateDirection, duration]);
Copy link
Copy Markdown
Contributor

@unrenamed unrenamed Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the HoverBorderGradient component doesn't seem to be used anymore. Should we remove it entirely? cc @chronark

return (
<Tag
onMouseEnter={() => {
Expand Down
8 changes: 4 additions & 4 deletions apps/www/components/ui/meteorLines.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
const [_windowWidth, _setWindowWidth] = useState<number>(0);

useEffect(() => {
const width = window.innerWidth;
const pos = direction === "left" ? xPos : width - (xPos + 75);
Expand All @@ -35,7 +35,7 @@ const MeteorLines = ({
}));

setMeteorStyles(styles);
}, [number, windowWidth]);
}, [number, delay, xPos, direction, speed]);

return (
<>
Expand Down Expand Up @@ -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);
Copy link
Copy Markdown
Contributor

@unrenamed unrenamed Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(() => {
const width = window.innerWidth;
Expand All @@ -78,7 +78,7 @@ const MeteorLinesAngular = ({
animationDuration: speed ? `${speed}s` : `${Math.floor(Math.random() * 10 + 2)}s`,
}));
setMeteorStyles(styles);
}, [number, windowWidth]);
Copy link
Copy Markdown
Contributor

@unrenamed unrenamed Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

image

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.

}, [number, xPos, speed, delay]);
return (
<>
{[...meteorStyles].map((style, idx) => (
Expand Down