Conversation
📝 WalkthroughWalkthroughLarge-scale refactoring removing unused React imports across components, eliminating unused prop parameters from component signatures (profile, parentContainerRef, setToasts, thresholdTimeMs, etc.), cleaning up unused type definitions from entity files, and simplifying function signatures and formatting consistency. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (13)
components/memelab/MemeLabCollection.tsx (1)
167-177: Add key prop to mapped fragments.Each element in the mapped array should have a unique key to avoid React warnings and ensure proper reconciliation.
🔎 Proposed fix
- {website.split(" ").map((w) => ( - <> + {website.split(" ").map((w, index) => ( + <span key={index}> <a href={addProtocol(w)} target="_blank" rel="noopener noreferrer" > {w} </a> - </> + </span> ))}Note: Changed from React fragment to
<span>since fragments with keys are less common. If the list is truly stable, using index as key is acceptable here.components/waves/leaderboard/gallery/WaveLeaderboardGalleryItem.tsx (1)
33-34: Component correctly implements React and Next.js conventions; ESLint verification required before merge.The component properly uses named React imports (
useState,useEffect,useRef,memofrom line 3), appliesreadonlymodifiers on all interface props, and follows functional component patterns with memo optimization. Navigation uses Next.jsLinkcomponent instead of raw<a>tags. Code aligns with React 19.2 and Next.js 16 conventions. Verify ESLint compliance by runningnpm run lintto confirm the changes pass Next.js Core Web Vitals and React Hooks rules.components/waves/drops/LightDrop.tsx (1)
2-6: Remove unused import and interface.The
ApiLightDropimport andLightDropPropsinterface are no longer used since the component doesn't consume any props. This creates a misleading API where callers must pass adropprop that is completely ignored.🔎 Proposed fix to complete the refactor
-import { FC } from "react"; -import { ApiLightDrop } from "@/generated/models/ApiLightDrop"; - -interface LightDropProps { - readonly drop: ApiLightDrop; -} +import { FC } from "react";components/waves/drops/winner/DefaultWinnerDrop.tsx (1)
72-84: Fix typo in interface name.The interface name
DefautWinnerDropPropshas a typo—it should beDefaultWinnerDropProps(with an 'l') to match the component nameDefaultWinnerDrop.🔎 Proposed fix
-interface DefautWinnerDropProps { +interface DefaultWinnerDropProps { readonly drop: ExtendedDrop; readonly showWaveInfo: boolean; readonly activeDrop: ActiveDropState | null; readonly showReplyAndQuote: boolean; readonly location: DropLocation; readonly dropViewDropId: string | null; readonly onReply: (param: DropInteractionParams) => void; readonly onQuote: (param: DropInteractionParams) => void; readonly onReplyClick: (serialNo: number) => void; readonly onQuoteClick: (drop: ApiDrop) => void; readonly onDropContentClick?: ((drop: ExtendedDrop) => void) | undefined; } const DefaultWinnerDrop = ({ drop, showWaveInfo, activeDrop, location, dropViewDropId, onReply, onQuote, onReplyClick, onQuoteClick, onDropContentClick, showReplyAndQuote, -}: DefautWinnerDropProps) => { +}: DefaultWinnerDropProps) => {components/waves/memes/traits/DropdownTrait.tsx (1)
27-32: Refactor to fully controlled component pattern.This Effect manually syncs external state into an uncontrolled component (line 67 uses
defaultValue), which mixes controlled and uncontrolled patterns—a React anti-pattern. This approach fights React's declarative model and can cause subtle bugs.Based on learnings, remove unnecessary Effects that only sync state. Convert this to a fully controlled component by removing the ref and Effect, then using
valueinstead ofdefaultValue.🔎 Proposed refactor to controlled component
Remove the ref (line 24):
- const selectRef = useRef<HTMLSelectElement>(null);Remove the Effect (lines 27-32):
- // Update select value when traits change - React.useEffect(() => { - const value = (traits[field] as string) || ""; - if (selectRef.current && value !== selectRef.current.value) { - selectRef.current.value = value; - } - }, [traits, field]);Update the select element to use
valueinstead ofdefaultValueand remove the ref (lines 65-69):<select - ref={selectRef} - defaultValue={(traits[field] as string) || ""} + value={(traits[field] as string) || ""} onChange={handleChange} onBlur={handleBlur}components/nextGen/collections/NextGenArtists.tsx (1)
42-44: Fix missing Effect dependency to satisfy React Hooks ESLint rule.The
fetchResultsfunction is called inside the Effect but not listed in the dependency array, violating thereact-hooks/exhaustive-depsrule.🔎 Proposed fixes
Option 1 (Recommended): Move
fetchResultsinside the EffectuseEffect(() => { + function fetchResults() { + let url = `${publicEnv.API_ENDPOINT}/api/nextgen/collections`; + fetchUrl(url).then((response: DBResponse) => { + setArtistCollections( + response.data.reduce((acc, collection) => { + if ( + !acc.find((a: any) => + areEqualAddresses(a.address, collection.artist_address) + ) + ) { + acc.push({ + address: collection.artist_address, + collections: response.data + .filter((c) => + areEqualAddresses(c.artist_address, collection.artist_address) + ) + .sort((a, b) => a.id - b.id), + }); + } + return acc; + }, []) + ); + }); + } fetchResults(); }, []);Then remove the old
fetchResultsfunction definition (lines 17-40).Option 2: Wrap with
useCallback+ import { useEffect, useState, useCallback } from "react";- function fetchResults() { + const fetchResults = useCallback(() => { let url = `${publicEnv.API_ENDPOINT}/api/nextgen/collections`; fetchUrl(url).then((response: DBResponse) => { setArtistCollections( response.data.reduce((acc, collection) => { if ( !acc.find((a: any) => areEqualAddresses(a.address, collection.artist_address) ) ) { acc.push({ address: collection.artist_address, collections: response.data .filter((c) => areEqualAddresses(c.artist_address, collection.artist_address) ) .sort((a, b) => a.id - b.id), }); } return acc; }, []) ); }); - } + }, []);useEffect(() => { fetchResults(); - }, []); + }, [fetchResults]);Based on coding guidelines: "Code must satisfy ESLint with Next's Core Web Vitals and React Hooks rules."
components/waves/memes/file-upload/utils/constants.ts (1)
12-15: Update the comment to reflect 200MB limit.The comment states "100MB" but the actual value is
200 * 1024 * 1024(200MB). This creates confusion between documentation and implementation.🔎 Proposed fix
/** - * Maximum file size allowed (100MB) + * Maximum file size allowed (200MB) */ export const FILE_SIZE_LIMIT: number = 200 * 1024 * 1024;components/user/stats/activity/tdh-history/UserPageStatsActivityTDHHistoryCharts.tsx (2)
136-141: Critical: Replace random key with stable identifier.Using
getRandomObjectId()as a React key is a critical anti-pattern. Keys must be stable across renders to allow React's reconciliation algorithm to function correctly. Random keys cause React to destroy and recreate components on every render, leading to:
- Loss of component state
- Unnecessary DOM operations and poor performance
- Broken animations and transitions
Since you removed the index parameter in this PR, use
dataSet.titleinstead, which is unique per CHART_CONFIGS.🔎 Proposed fix
- {dataSets.map((dataSet) => ( + {dataSets.map((dataSet) => ( <UserPageStatsActivityTDHHistoryChart - key={getRandomObjectId()} + key={dataSet.title} data={dataSet} /> ))}
79-132: Replace Effect with useMemo for derived state.The Effect computes derived state from
tdhHistorywithout performing any side effects. Per coding guidelines, derive this data during render usinguseMemoinstead ofuseState+useEffect.As per coding guidelines, "Remove unnecessary Effects; if the Effect only derives state, compute during render instead."
🔎 Proposed refactor
- const [dataSets, setDataSets] = useState<ChartProps[]>([]); + const dataSets = useMemo<ChartProps[]>(() => { + if (!tdhHistory.length) { + return []; + } + const tdhHistoryReversed = tdhHistory.toReversed(); + const tdhLabels = tdhHistoryReversed.map((t) => t.date); + return getDataSets({ tdh: tdhHistoryReversed, labels: tdhLabels }); + }, [tdhHistory]); const isNumber = (n: any): n is number => typeof n === "number"; const getData = ({ tdh, labels, field, }: { tdh: TDHHistory[]; labels: Date[]; field: keyof TDHHistory; }): number[] => { return labels.map((l) => { const tdhItem = tdh.find((t) => t.date === l); if (!tdhItem) { return 0; } const value = tdhItem[field]; return tdhItem && isNumber(value) ? value : 0; }); }; const getDataSets = ({ tdh, labels, }: { tdh: TDHHistory[]; labels: Date[]; }): ChartProps[] => { return CHART_CONFIGS.map((config) => { return { title: config.title, labels, datasets: config.datasets.map((d) => ({ label: d.label, data: getData({ tdh, labels, field: d.field }), backgroundColor: d.color, borderColor: d.color, })), }; }).filter((c) => c.datasets.some((d) => d.data.some((n) => n > 0))); }; - - useEffect(() => { - if (!tdhHistory.length) { - setDataSets([]); - return; - } - - const tdhHistoryReversed = tdhHistory.toReversed(); - const tdhLabels = tdhHistoryReversed.map((t) => t.date); - setDataSets(getDataSets({ tdh: tdhHistoryReversed, labels: tdhLabels })); - }, [tdhHistory]);Note: You'll need to import
useMemofrom React.components/waves/create-wave/outcomes/winners/rows/cic/CreateWaveOutcomesRowCICApprove.tsx (1)
4-8: Incomplete refactor: Remove button is now non-functional.The
removeOutcomeprop was removed from the component signature, but the Remove button at lines 48-68 still exists with noonClickhandler. This creates a broken user experience where the button appears interactive but does nothing when clicked.Either:
- Add back the
removeOutcomecallback and wire it to the button'sonClick, or- Remove the entire button element if removal functionality is no longer needed
components/waves/create-wave/outcomes/winners/rows/cic/CreateWaveOutcomesRowCICRank.tsx (1)
4-8: Incomplete refactor: Remove button is now non-functional.The
removeOutcomeprop was removed from the component signature, but the Remove button at lines 48-68 still exists with noonClickhandler. This creates a broken user experience where the button appears interactive but does nothing when clicked.Either:
- Add back the
removeOutcomecallback and wire it to the button'sonClick, or- Remove the entire button element if removal functionality is no longer needed
components/waves/winners/drops/header/WaveWinnersDropHeaderAuthorHandle.tsx (1)
14-24: Apply consistent fallback handling forwinner.drop.author.handleacross all three usages.The multiline formatting change is good. However,
author.handleis typed asstring | null(from ApiProfileMin), so the three usages are inconsistent:
- Line 15:
user={winner.drop.author.handle ?? winner.drop.author.id}— has fallback- Line 18:
href={/${winner.drop.author.handle}}— no fallback, would create/nullif handle is null- Line 22:
{winner.drop.author.handle}— no fallback, would render undefined textApply the same fallback pattern to lines 18 and 22 as used in line 15, or conditionally render the entire component if
handleis null.components/nextGen/collections/collectionParts/NextGenCollectionDetails.tsx (1)
123-323: The ABOUT view displays hardcoded Pebbles content for all collections.The removal of the unused
collectionprop is correct. However,NextGenCollectionDetailsAboutrenders hardcoded content specific to the Pebbles collection (lines 128, 149, 164, 240, etc.) regardless of which collection is being viewed. This component is used for all collections via the dynamic routeapp/nextgen/collection/[collection]/[[...view]]/page.tsx, so any user navigating to the ABOUT tab for any NextGen collection will see Pebbles-specific information instead of collection-specific content.Other views (OVERVIEW, PROVENANCE, TOP_TRAIT_SETS) properly pass and use the
collectionprop. Either restrict this view to the Pebbles collection, make it collection-aware with dynamic content, or return null for non-Pebbles collections.
🧹 Nitpick comments (28)
components/waves/winners/MemesWaveWinnerDropSmall.tsx (1)
126-133: Replace<img>with<Image />fromnext/image.The
<img>element violates Next.js's@next/next/no-img-elementESLint rule. Since this file is already being updated as part of the cleanup, consider migrating to theImagecomponent fromnext/imageto ensure full ESLint compliance.🔎 Proposed fix using next/image
+import Image from "next/image"; ... - {drop.author.pfp ? ( - <img - src={getScaledImageUri( - drop.author.pfp, - ImageScale.W_AUTO_H_50 - )} - alt={`${drop.author.handle}'s profile`} - className="tw-size-7 tw-rounded-lg tw-ring-1 tw-ring-white/10 tw-object-cover" - /> + {drop.author.pfp ? ( + <Image + src={getScaledImageUri( + drop.author.pfp, + ImageScale.W_AUTO_H_50 + )} + alt={`${drop.author.handle}'s profile`} + width={28} + height={28} + className="tw-rounded-lg tw-ring-1 tw-ring-white/10 tw-object-cover" + />Note: You may need to adjust
widthandheightbased on the actual image dimensions or usefillwith a container if dynamic sizing is required.components/waves/winners/drops/DropContentSmall.tsx (1)
26-29: Consider extracting noop callbacks to preserve referential stability.The inline empty arrow functions (
() => {}) create new function references on each render. IfWaveDropContentis memoized, these changing references could defeat its memoization.🔎 Suggested improvement
+const noop = () => {}; + export const DropContentSmall = memo<DropContentSmallProps>( ({ drop, onDropClick }) => { const [activePartIndex, setActivePartIndex] = useState(0); const handleDropClick = useCallback(() => { onDropClick(drop); }, [drop, onDropClick]); return ( <div> <WaveDropContent drop={drop} activePartIndex={activePartIndex} setActivePartIndex={setActivePartIndex} - onLongPress={() => {}} + onLongPress={noop} onDropContentClick={handleDropClick} - onQuoteClick={() => {}} - setLongPressTriggered={() => {}} + onQuoteClick={noop} + setLongPressTriggered={noop} /> </div> ); } );components/home/HomeFeed.tsx (1)
47-49: Remove unnecessary Effect that only initializes state.The
useEffectwith an empty dependency array is redundant—it only setsactiveDroptonull, which is already its initial value from line 23. According to coding guidelines, Effects whose only job is to set state should be removed. This also allows removing the unuseduseEffectimport.🔎 Proposed fix
Remove the Effect (lines 47–49) and update the import on line 3:
-import { useState, useEffect, useCallback } from "react"; +import { useState, useCallback } from "react";And delete:
- useEffect(() => { - setActiveDrop(null); - }, []); -Based on learnings, unnecessary Effects should be removed when they only derive or sync internal state. Aligning with this guidance and the PR's goal of cleaning up unused code.
components/utils/icons/ClockIcon.tsx (1)
3-35: Consider migrating to FontAwesome for icon consistency.While the current implementation is correct, the coding guidelines specify using FontAwesome for icons in React components. Consider migrating this custom SVG icon to FontAwesome in a future refactor to align with project standards.
As per coding guidelines, this would improve consistency across the icon system.
components/waves/drops/MediaThumbnail.tsx (1)
25-25: LGTM! React import removal aligns with JSX automatic runtime.The removal of the explicit React import is correct and follows modern React 19.2 conventions.
Pre-existing issue:
<img>element should use Next.js<Image />Lines 12-16 use a raw
<img>element, which violates the Next.js ESLint rule@next/next/no-img-element. Would you like me to open a separate issue to migrate this to<Image />fromnext/image?Based on coding guidelines requiring Next.js
<Image />for all images.components/waves/create-wave/voting/components/TimeUnitSelector.tsx (2)
22-25: Move constant array outside component scope.The
timeUnitOptionsarray is static but gets recreated on every render. Define it outside the component to avoid unnecessary allocations.🔎 Suggested refactor
+const TIME_UNIT_OPTIONS: { value: TimeUnit; label: string }[] = [ + { value: "minutes", label: "Minutes" }, + { value: "hours", label: "Hours" }, +]; + const TimeUnitSelector = memo( ({ value, onUnitChange, id, ariaLabel }: TimeUnitSelectorProps) => { - // Options for the time unit dropdown - const timeUnitOptions: { value: TimeUnit; label: string }[] = [ - { value: "minutes", label: "Minutes" }, - { value: "hours", label: "Hours" }, - ]; return ( <select id={id} aria-label={ariaLabel} className="tw-px-3 tw-py-2 tw-bg-iron-800 tw-text-iron-50 tw-border tw-border-iron-700 tw-rounded-md tw-ml-2" value={value} onChange={(e) => { - // Type assertion needed here as TypeScript can't infer the value is TimeUnit const selectedUnit = e.target.value as TimeUnit; onUnitChange(selectedUnit); }} data-testid="time-unit-selector" > - {timeUnitOptions.map((option) => ( + {TIME_UNIT_OPTIONS.map((option) => ( <option key={option.value} value={option.value}> {option.label} </option> ))} </select> ); } );
21-21: Remove comments per coding guidelines.The coding guidelines specify that code should be self-explanatory without comments. Both comments can be safely removed as the code is clear without them.
As per coding guidelines, comments should not be included in the code.
Also applies to: 34-34
components/brain/notifications/NotificationsWrapper.tsx (1)
16-30: Consider simplifying redundant optional type syntax.The type definition uses
| undefinedafter optional properties (e.g.,chat?: ... | undefined), which is redundant since the?operator already permitsundefined. While functionally correct, simplifying would improve readability.🔎 Simplified type definition
type WaveWithChatScope = ExtendedDrop["wave"] & { - chat?: - | { - scope?: - | { - group?: - | { - is_direct_message?: boolean | undefined; - } - | undefined; - } - | undefined; - } - | undefined; + chat?: { + scope?: { + group?: { + is_direct_message?: boolean; + }; + }; + }; };components/memes/drops/meme-participation-drop/MemeDropActions.tsx (1)
29-31: Remove explanatory comments per coding guidelines.The comments explain why
showVotingis not set. Per the project's coding guidelines, code should be self-explanatory without comments.As per coding guidelines, code in this project should be self-explanatory and not include comments.
components/profile-activity/filter/ProfileActivityLogsFilterListItem.tsx (1)
17-23: Remove unnecessary Effect and compute state during render.The
useEffectonly derivesisSelectedfromselectedItemsanditemType. This violates the guideline to avoid Effects for state derivation. Compute directly during render instead.Based on learnings, remove unnecessary Effects and calculate during render.
🔎 Proposed refactor
- const [isSelected, setIsSelected] = useState( - selectedItems.includes(itemType) - ); - - useEffect(() => { - setIsSelected(selectedItems.includes(itemType)); - }, [selectedItems]); + const isSelected = selectedItems.includes(itemType);components/memes/drops/MemeWinnerArtistInfo.tsx (1)
20-21: Consider removing the comment per coding guidelines.The code is self-explanatory, and the coding guidelines specify that comments should be avoided.
As per coding guidelines, code should not include comments.
Proposed change
- // Get the decision time from the winning context const decisionTime = drop.winning_context?.decision_time;components/nextGen/collections/NextGenArtists.tsx (2)
53-66: LGTM: Unused parameter correctly removed.The removal of the unused index parameter aligns with the PR objective and improves code clarity.
Optional: Remove redundant type annotation
TypeScript can infer the parameter type from
artistCollections, making the explicit annotation unnecessary:- (ac: { address: string; collections: NextGenCollection[] }) => { + (ac) => {
21-37: Replaceanytype with specific type for better type safety.The accumulator type can be explicitly defined instead of using
any.🔎 Proposed fix
setArtistCollections( response.data.reduce((acc, collection) => { if ( - !acc.find((a: any) => + !acc.find((a) => areEqualAddresses(a.address, collection.artist_address) ) ) { acc.push({ address: collection.artist_address, collections: response.data .filter((c) => areEqualAddresses(c.artist_address, collection.artist_address) ) .sort((a, b) => a.id - b.id), }); } return acc; - }, []) + }, [] as { address: string; collections: NextGenCollection[] }[]) );components/drops/view/item/content/nft-tag/DropListItemContentNftDetails.tsx (1)
16-20: Consider replacing<img>with Next.js<Image />.Per coding guidelines,
<img>elements should be replaced with<Image />fromnext/imageto comply with the@next/next/no-img-elementESLint rule. While this is outside the scope of the current PR, it would improve consistency with Next.js best practices.🔎 Proposed refactor
+import Image from "next/image"; + import { ReferencedNft } from "@/entities/IDrop";<div className="tw-gap-x-2 tw-flex tw-items-center"> {image && ( - <img + <Image alt="Seize" src={getScaledImageUri(image, ImageScale.W_AUTO_H_50)} className="tw-flex-shrink-0 tw-h-4 tw-w-4 tw-object-contain" + width={16} + height={16} /> )}As per coding guidelines, Next.js
<Image />component should be used for images.components/waves/winners/DefaultWaveWinnerDropSmall.tsx (1)
123-131: Consider migrating to Next.js<Image>component.The component uses an
<img>tag for the author profile picture. Per coding guidelines, Next.js<Image>should be used instead to comply with the@next/next/no-img-elementESLint rule and gain automatic optimization benefits.Note: This is pre-existing code, not introduced in this PR, so it can be addressed separately if desired.
🔎 Proposed refactor to use Next.js Image
Add the Image import at the top of the file:
+import Image from "next/image";Then replace the img tag:
- {drop.author.pfp ? ( - <img - src={getScaledImageUri( - drop.author.pfp, - ImageScale.W_AUTO_H_50 - )} - alt={`${drop.author.handle}'s profile`} - className="tw-size-7 tw-rounded-lg tw-ring-1 tw-ring-white/10 tw-object-cover" - /> - ) : ( - <div className="tw-size-7 tw-rounded-lg tw-ring-1 tw-ring-white/10 tw-bg-iron-800" /> - )} + {drop.author.pfp ? ( + <Image + src={getScaledImageUri( + drop.author.pfp, + ImageScale.W_AUTO_H_50 + )} + alt={`${drop.author.handle}'s profile`} + className="tw-size-7 tw-rounded-lg tw-ring-1 tw-ring-white/10 tw-object-cover" + width={28} + height={28} + /> + ) : ( + <div className="tw-size-7 tw-rounded-lg tw-ring-1 tw-ring-white/10 tw-bg-iron-800" /> + )}components/manifoldMinting/ManifoldMintingWidget.tsx (1)
83-91: LGTM! Unused index parameter correctly removed.The index parameter was not referenced in the map body, so its removal is appropriate and aligns with the PR objective.
Optional: Consider using
.forEach()instead of.map()Since the iteration is used purely for side effects (pushing to
params) and the returned array is discarded,.forEach()would be more semantically appropriate than.map():-merkleProofs.map((mp) => { +merkleProofs.forEach((mp) => { params.push({ address: props.proxy as `0x${string}`, abi: props.abi, chainId: MANIFOLD_NETWORK.id, functionName: "checkMintIndex", args: [props.contract, props.claim.instanceId, mp.value], }); });components/waves/drops/DropNotFound.tsx (1)
1-3: Consider removing the JSDoc comment.The component name
DropNotFoundis self-explanatory. As per coding guidelines, comments should be avoided when code is self-documenting.Proposed refactor
-/** - * Component to show when a drop is not found - */ export default function DropNotFound() {components/waves/winners/WaveWinnersSmall.tsx (1)
62-67: Unnecessary non-null assertion on a guarded value.On line 65, the non-null assertion (
!) is redundant since the condition on line 64 already guaranteesdecisionPoints.length > 0, and optional chaining (?.) already handles the case. Consider removing the!for cleaner code:🔎 Suggested fix
useEffect(() => { if (decisionPoints?.length > 0 && !activeDecisionPoint) { - setActiveDecisionPoint(decisionPoints[0]?.id!); + setActiveDecisionPoint(decisionPoints[0].id); } }, [decisionPoints, activeDecisionPoint]);components/memes/drops/meme-participation-drop/MemeDropArtistInfo.tsx (2)
48-51: Minor redundancy in fallback logic.Since line 48 already checks that
drop.author?.handleis truthy, the fallback?? drop.author.idon line 50 will never be executed. Consider simplifying to justuser={drop.author.handle}.🔎 Proposed simplification
- <UserProfileTooltipWrapper - user={drop.author.handle ?? drop.author.id} - > + <UserProfileTooltipWrapper user={drop.author.handle}>
96-96: Remove comment per coding guidelines.As per coding guidelines, comments should be avoided when code is self-explanatory. The
ArtistPreviewModalcomponent name clearly indicates its purpose.🔎 Proposed fix
- - {/* Artist Preview Modal */} <ArtistPreviewModalBased on coding guidelines: "Do not include any comments in the code; it should be self-explanatory"
components/waves/drops/participation/ratings/ParticipationDropRatingsVoterSection.tsx (1)
33-40: Consider migrating to Next.js<Image />component.The standard HTML
<img>tag should be replaced with Next.js<Image />to comply with ESLint rule@next/next/no-img-elementand leverage automatic image optimization.📦 Suggested migration to Next.js Image
Add the Image import at the top of the file:
+import Image from "next/image";Then replace the
<img>tag:- <img - src={getScaledImageUri( - rater.profile.pfp, - ImageScale.W_AUTO_H_50 - )} - alt={`${rater.profile.handle}'s avatar`} - className={`tw-h-5 tw-w-5 tw-rounded-md tw-ring-1 tw-object-cover ${theme.ring} tw-bg-iron-900`} - /> + <Image + src={getScaledImageUri( + rater.profile.pfp, + ImageScale.W_AUTO_H_50 + )} + alt={`${rater.profile.handle}'s avatar`} + width={20} + height={20} + className={`tw-h-5 tw-w-5 tw-rounded-md tw-ring-1 tw-object-cover ${theme.ring} tw-bg-iron-900`} + />Based on coding guidelines and learnings.
components/rememes/Rememes.tsx (2)
142-145: Consider using Next.js<Link>component for internal navigation.The
<a>tag causes full page reloads. Replace with<Link>fromnext/linkfor client-side navigation and better performance, per Next.js best practices.Proposed refactor
Import
Linkat the top of the file:import Image from "next/image"; +import Link from "next/link"; import { usePathname, useRouter, useSearchParams } from "next/navigation";Then replace the anchor tag:
- <a - href={`/rememes/${rememe.contract}/${rememe.id}`} + <Link + href={`/rememes/${rememe.contract}/${rememe.id}`} className="decoration-none scale-hover" > <Container fluid> ... </Container> - </a> + </Link>
254-259: Preferrouter.push()overwindow.location.hreffor internal navigation.Since
routerfromuseRouter()is already available (line 44), userouter.push("/rememes/add")instead ofwindow.location.hreffor client-side navigation without full page reload.Proposed refactor
<Button className="seize-btn btn-white d-flex align-items-center justify-content-center gap-2 w-100 w-sm-auto" onClick={() => { - window.location.href = "/rememes/add"; + router.push("/rememes/add"); }} >components/distribution-plan-tool/review-distribution-plan/table/ReviewDistributionPlanTableHeader.tsx (1)
124-150: LGTM!The className formatting changes improve readability without affecting functionality.
components/waves/drops/winner/WinnerDropBadge.tsx (2)
1-1: Consider using direct FC import instead of React namespace.Per coding guidelines, prefer direct named imports from React over namespace usage.
🔎 Proposed refactor
-import React from "react"; +import { FC } from "react"; import { faClock } from "@fortawesome/free-regular-svg-icons"; import { faTrophy } from "@fortawesome/free-solid-svg-icons"; import { FontAwesomeIcon } from "@fortawesome/react-fontawesome";-const WinnerDropBadge: React.FC<WinnerDropBadgeProps> = ({ +const WinnerDropBadge: FC<WinnerDropBadgeProps> = ({ rank, position = 1, decisionTime,Based on learnings, prefer direct named imports.
Also applies to: 13-13
19-25: Optional: String type guard is unreachable code.The type guard on lines 23-24 checks for string type, but based on the interface definitions (
rank: number | null,position: number),effectiveRankcan only ever be a number. This check will never be true and could be simplified.🔎 Proposed simplification
const effectiveRank = rank !== null && rank !== undefined ? rank : position; if (!effectiveRank) return null; - const rankNumber = - typeof effectiveRank === "string" - ? parseInt(effectiveRank, 10) - : effectiveRank; + const rankNumber = effectiveRank;components/rememes/RememeAddPage.tsx (2)
357-360: Good removal of unused parameter, but consider using a stable key.The removal of the unused
indexparameter aligns with the PR objective. However, the code usesgetRandomObjectId()as a React key, which generates a new ID on every render. This causes React to recreate DOM elements unnecessarily.🔎 Suggested refactor for stable key
Since the index was already available (before this PR removed it), consider using it for the key, or use the error message itself if errors are unique:
-submissionResult.errors.map((e) => ( - <Col xs={12} className="pt-2" key={getRandomObjectId()}> +submissionResult.errors.map((e, index) => ( + <Col xs={12} className="pt-2" key={`error-${index}`}> {e} </Col> ))}
63-108: Consider refactoring effect to useMemo.This effect only derives the
checkListstate fromaddRememeanduserTDHwithout performing side effects. Per React best practices and learnings, derived state should be computed during render usinguseMemorather than in an effect.Based on learnings, effects should be reserved for synchronization with external systems, not for deriving state.
🔎 Suggested refactor using useMemo
-useEffect(() => { +const checkList = useMemo(() => { const mychecklist: CheckList[] = []; if (addRememe) { if (!seizeSettings?.rememes_submission_tdh_threshold) { mychecklist.push({ status: false, note: "Something went wrong fetching global settings", }); } else { const isDeployer = areEqualAddresses( addRememe.contract.contractDeployer, address ); if (isDeployer) { mychecklist.push({ status: true, note: "Rememe can be added (Rememe Contract Deployer)", }); } else { if (!userTDH) { mychecklist.push({ status: false, note: "You need to have some TDH before you can add Rememes", }); } else { mychecklist.push({ status: userTDH.boosted_tdh >= seizeSettings.rememes_submission_tdh_threshold, note: `You need ${numberWithCommas( seizeSettings.rememes_submission_tdh_threshold )} TDH to add ${ addRememe.nfts.length > 1 ? `these Rememes` : `this Rememe` }${ userTDH ? ` (you have ${numberWithCommas(userTDH.boosted_tdh)} TDH)` : `` }`, }); } } } } - setCheckList(mychecklist); + return mychecklist; }, [addRememe, userTDH]);Then remove the
const [checkList, setCheckList] = useState<CheckList[]>([]);line and anysetCheckList([])calls, as the value is now computed.
2 participants
Summary by CodeRabbit
Refactor
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.