fix: refine next 22 of 38 lint warnings#2660
Conversation
unrenamed
requested review from
MichaelUnkey,
chronark,
mcstepp and
perkinsjr
as code owners
|
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request includes modifications across several components in the dashboard and chart libraries. Key changes involve the simplification of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@unrenamed is attempting to deploy a commit to the Unkey Team on Vercel. A member of the Team first needs to authorize it. |
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (18)
apps/www/app/accelerate/components/rive.tsx (1)
Line range hint
73-101: Consider splitting the useEffect hook's responsibilities.The current useEffect hook handles both event listener setup and state machine value updates. Consider splitting these into separate effects for better maintainability and clearer responsibility boundaries:
+ // Handle state machine updates + React.useEffect(() => { + if (!r.rive || smVarHighlight == null || smVarUnlockedUntil == null) { + return; + } + + if (day > 0) { + smVarUnlockedUntil.value = day; + smVarHighlight.value = day; + } + }, [day, r.rive, smVarHighlight, smVarUnlockedUntil]); + + // Handle event listener setup + React.useEffect(() => { + if (done || !r.rive) { + return; + } + const onRiveEventReceived = (riveEvent: any) => { const eventData = riveEvent.data; if (eventData.type === RiveEventType.General) { if (eventData.name === "click_d1") { onClickDay(1); } // ... rest of the click handlers } }; - if (day > 0 && smVarHighlight !== null && smVarUnlockedUntil !== null) { - smVarUnlockedUntil.value = day; - smVarHighlight.value = day; - } - r.rive.on(EventType.RiveEvent, onRiveEventReceived); setDone(true); + + return () => { + r.rive.off(EventType.RiveEvent, onRiveEventReceived); + }; - }, [done, day, r.rive, smVarHighlight, smVarUnlockedUntil]); + }, [done, r.rive]);apps/www/components/ui/meteorLines.tsx (1)
37-37: LGTM! Consider reordering dependencies.The addition of
directionto the dependency array is correct as it's used within the effect to calculate the position. Consider reordering the dependencies to match their usage order in the effect for better maintainability.- }, [number, direction, delay, xPos, speed]); + }, [direction, xPos, number, delay, speed]);apps/dashboard/components/multi-select.tsx (1)
42-63: Add defensive check before removing itemsThe changes to include
setSelectedin the dependency array are correct and necessary to prevent stale closures. However, consider adding a defensive check before removing items.Consider this improvement:
const handleKeyDown = React.useCallback( (e: React.KeyboardEvent<HTMLDivElement>) => { const input = inputRef.current; if (input) { if (e.key === "Delete" || e.key === "Backspace") { - if (input.value === "") { + if (input.value === "" && selected.length > 0) { setSelected((prev) => { const newSelected = [...prev]; newSelected.pop(); return newSelected; }); } }apps/dashboard/app/(app)/settings/user/update-user-image.tsx (3)
Line range hint
65-77: Remove duplicate image upload logic from form submission.The form submission handler performs an additional image upload that's already handled in
onChangePicture. This could lead to duplicate uploads.Consider simplifying to:
<form onSubmit={async (e) => { e.preventDefault(); - if (!image) { - toast.error("No image selected"); - return; - } - - await user?.setProfileImage({ file: image }); - await user?.reload(); - toast.success("Image uploaded"); }} >
Line range hint
22-60: Improve consistency in error handling and loading states.The error handling and loading states differ between the file input and drag-drop handlers:
- File input shows "Uploading image..." before validation
- Drag-drop doesn't show upload progress
- No loading state during upload
Consider extracting a common upload handler:
const handleFileUpload = async (file: File) => { if (!user) { toast.error("Only allowed for orgs"); 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); toast("Uploading image..."); try { await user.setProfileImage({ file }); toast.success("Image uploaded"); } catch (error) { toast.error("Error uploading image"); } };Then use it in both handlers:
const onChangePicture = useCallback( (e: ChangeEvent<HTMLInputElement>) => { const file = e.target.files?.[0]; if (!file) { toast.error("No image selected"); return; } handleFileUpload(file); }, [user] ); // In onDrop: if (file) { handleFileUpload(file); }
Line range hint
89-95: Enhance accessibility for image upload interactions.The component could benefit from improved accessibility:
- Add a visually hidden label text for screen readers
- Add proper ARIA attributes for the drag-drop zone
Consider these improvements:
<label htmlFor="image" + aria-label="Upload profile picture" className="relative flex flex-col items-center justify-center w-24 h-24 mt-1 transition-all border rounded-full shadow-sm cursor-pointer border-border bg-background group hover:bg-background-subtle" > <div className="absolute z-[5] h-full w-full rounded-full" + role="button" + aria-label="Drag and drop zone for profile picture" + tabIndex={0} onDragOver={...} >apps/dashboard/lib/charts/sparkline.tsx (2)
Line range hint
106-121: Consider memoizing the chartContext object.The
chartContextobject is recreated on every render. Consider wrapping it inuseMemoto optimize performance, especially since it's used in the context provider and passed down to children.- const chartContext: ChartContextType<T> = { + const chartContext = useMemo<ChartContextType<T>>(() => ({ width, height, data, series, startDate, endDate, xScale, yScale, minY, maxY, margin, padding, tooltipContent, leftAxisMargin, setLeftAxisMargin, - }; + }), [width, height, data, series, startDate, endDate, xScale, yScale, minY, maxY, margin, padding, tooltipContent, leftAxisMargin]);
Line range hint
164-171: Add type safety for tooltip data access.The tooltip rendering assumes
tooltipDatais always valid when accessing properties. Consider adding type guards or optional chaining for more robust error handling.- {tooltipData && ( + {tooltipData?.date && ( <Line - x1={xScale(tooltipData.date)} - x2={xScale(tooltipData.date)} + x1={xScale(tooltipData.date)} + x2={xScale(tooltipData.date)} y1={height} y2={0}apps/dashboard/lib/charts/time-series-chart.tsx (1)
Line range hint
94-108: Consider additional performance optimizations.While the dependency changes look good, here are two suggestions to further optimize performance:
- Consider memoizing the tooltipContent callback to prevent unnecessary rerenders:
const memoizedTooltipContent = useCallback( (d: T) => tooltipContent?.(d) ?? series[0].valueAccessor(d), [tooltipContent, series] );
- Consider using a reducer for margin state management instead of useState to make the logic more maintainable:
const [margin, dispatchMargin] = useReducer( (state: typeof marginProp, action: { type: 'SET_LEFT_AXIS'; payload: number }) => { switch (action.type) { case 'SET_LEFT_AXIS': return { ...state, left: marginProp.left + action.payload }; } }, marginProp );apps/dashboard/app/(app)/authorization/roles/create-new-role.tsx (3)
Line range hint
28-31: Clean up unused type definitionsThe
permissionsprop is defined in thePropstype but is no longer used in the component implementation. Consider removing it to maintain code clarity.type Props = { trigger: React.ReactNode; - permissions?: Permission[]; };
Line range hint
89-95: Clean up unused permission handling in form submissionThe
onSubmitfunction still processespermissionOptionseven though the UI for setting permissions has been commented out. Consider simplifying the mutation call.async function onSubmit(values: z.infer<typeof formSchema>) { createRole.mutate({ name: values.name, description: values.description, - permissionIds: values.permissionOptions?.map((o) => o.value), }); }
Line range hint
146-146: Remove or properly document commented codeThere's a large block of commented code for permissions UI with a note indicating it's "Broken". If the permissions are intentionally handled on a separate page after role creation, consider:
- Removing the commented code entirely
- Or, if this is a temporary state, creating a GitHub issue to track the proper implementation and adding a TODO comment with the issue reference
apps/dashboard/app/(app)/settings/general/update-workspace-image.tsx (4)
Line range hint
91-101: Fix type mismatch in form submission.The form submission handler passes the
imagestate (which is a string) directly tosetLogo({ file: image }), but thesetLogoAPI expects a File object. This will likely cause runtime errors.Consider storing both the File object and the preview URL:
- const [image, setImage] = useState<string | null>(organization?.imageUrl ?? null); + const [imagePreview, setImagePreview] = useState<string | null>(organization?.imageUrl ?? null); + const [imageFile, setImageFile] = useState<File | null>(null); // In onChangePicture: reader.onload = (e) => { - setImage(e.target?.result as string); + setImagePreview(e.target?.result as string); + setImageFile(file); }; // In form submission: - if (!image) { + if (!imageFile) { toast.error("No image selected"); return; } - await organization?.setLogo({ file: image }); + await organization?.setLogo({ file: imageFile });
Line range hint
134-156: Reduce code duplication in file validation.The file validation logic is duplicated between
onChangePictureand the drop handler. Consider extracting it into a reusable function.Here's a suggested refactor:
+ const MAX_FILE_SIZE_MB = 2; + const ALLOWED_FILE_TYPES = ["image/png", "image/jpeg"] as const; + + const validateAndProcessFile = (file: File) => { + if (file.size / 1024 / 1024 > MAX_FILE_SIZE_MB) { + toast.error(`File size too big (max ${MAX_FILE_SIZE_MB}MB)`); + return false; + } + if (!ALLOWED_FILE_TYPES.includes(file.type as typeof ALLOWED_FILE_TYPES[number])) { + toast.error("File type not supported (.png or .jpg only)"); + return false; + } + return true; + }; // In onChangePicture: 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; - } + if (!validateAndProcessFile(file)) { + return; + } // In drop handler: 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 { + if (validateAndProcessFile(file)) { const reader = new FileReader(); // ... } }
Line range hint
22-23: Improve upload status messaging.The "Uploading image..." toast is shown before file validation, which could lead to a confusing sequence of messages if validation fails.
Consider showing the upload status toast only after validation passes:
const onChangePicture = useCallback( (e: ChangeEvent<HTMLInputElement>) => { - toast("Uploading image..."); const file = e.target.files?.[0]; if (!file) { toast.error("No image selected"); return; } // ... validation ... + toast("Uploading image..."); const reader = new FileReader();
Line range hint
108-112: Enhance accessibility for screen readers.The image upload interface could be more accessible to screen reader users.
Consider adding ARIA attributes and improving the input label:
<label htmlFor="image" + role="button" + aria-label="Upload workspace avatar. Accepted files: PNG or JPEG up to 2MB" className="relative flex flex-col items-center justify-center w-24 h-24 mt-1 transition-all border rounded-full shadow-sm cursor-pointer bg-background border-border group hover:bg-background-subtle" >Also consider adding appropriate ARIA attributes for the drag and drop zone:
<div className="absolute z-[5] h-full w-full rounded-full" + role="region" + aria-label="Drop zone for workspace avatar" + aria-describedby="upload-instructions" onDragOver={...} >apps/play/app/page-bk.tsx (2)
Line range hint
51-53: Fix invalid JSON error message format.The current error message uses invalid JSON syntax.
Apply this diff to fix the format:
- postNewLine('{"Error", "Invalid Curl Command"}', "text-red-500"); + postNewLine('{"error": "Invalid Curl Command"}', "text-red-500");
Line range hint
45-49: Optimize history state updates.The current implementation mutates the state directly before updating it, which could lead to unexpected behavior.
Apply this diff to fix the state update:
function postNewLine(input: string, color: string) { - const temp = historyItems; - temp.push({ content: input, color: color }); - setHistoryItems([...temp]); + setHistoryItems(prev => [...prev, { content: input, color }]); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
apps/dashboard/app/(app)/authorization/roles/create-new-role.tsx(1 hunks)apps/dashboard/app/(app)/settings/general/update-workspace-image.tsx(1 hunks)apps/dashboard/app/(app)/settings/user/update-user-image.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/x-axis.tsx(0 hunks)apps/dashboard/lib/charts/y-axis.tsx(0 hunks)apps/play/app/page-bk.tsx(2 hunks)apps/www/app/accelerate/components/rive.tsx(1 hunks)apps/www/components/ui/hover-border-gradient.tsx(0 hunks)apps/www/components/ui/meteorLines.tsx(1 hunks)
💤 Files with no reviewable changes (3)
- apps/dashboard/lib/charts/x-axis.tsx
- apps/dashboard/lib/charts/y-axis.tsx
- apps/www/components/ui/hover-border-gradient.tsx
🔇 Additional comments (11)
apps/dashboard/lib/charts/useTooltip.ts (1)
53-63: LGTM! Important dependency array fixes.
The updated dependency array now correctly includes all values used within the handleTooltip callback. This fixes potential stale closure bugs where the tooltip position wouldn't update properly when margin.left, snapToX, snapToY, or showTooltip changed.
Let's verify there aren't any other missing dependencies in the callback:
apps/www/app/accelerate/components/rive.tsx (1)
101-101: LGTM! Correct fix for the lint warning.
Adding day to the dependency array is necessary since it's used within the effect to update the state machine inputs. This change ensures the animation state updates properly when the day prop changes.
apps/dashboard/app/(app)/settings/user/update-user-image.tsx (1)
60-60: LGTM! Correct dependency array optimization.
The dependency array has been correctly optimized to only include the user object. This is appropriate because:
setImageis a state setter which is stable and doesn't need to be includedtoastis a stable imported utilityuseris correctly included as it's used within the callback
apps/dashboard/lib/charts/sparkline.tsx (1)
92-92: LGTM! Improved scale recalculation on padding changes.
The addition of padding.bottom and padding.top to the useMemo dependencies is correct and necessary. This ensures that the chart scales are properly recalculated when padding values change, leading to more accurate visualizations.
apps/dashboard/lib/charts/time-series-chart.tsx (3)
65-65: LGTM! Optimized useMemo dependencies for date calculations.
The dependency array now correctly includes only data since the start and end dates are derived solely from the data array.
77-77: LGTM! Optimized useMemo dependencies for min/max calculations.
The dependency array now correctly includes only data and series since the min/max values depend on the filtered active series values from the data.
93-93: Verify the scale recalculations with padding changes.
The addition of padding.bottom and padding.top to the dependency array is correct as they affect the yScale domain. However, let's verify that this doesn't cause unnecessary recalculations.
apps/dashboard/app/(app)/authorization/roles/create-new-role.tsx (2)
61-61: LGTM! Clean component signature update
The removal of the unused permissions prop simplifies the component interface while maintaining all necessary functionality.
Line range hint 146-146: Verify the role-permission management flow
The comment suggests permissions are managed on a separate page after role creation. Let's verify this flow is properly implemented and documented.
✅ Verification successful
Role-permission management flow is properly implemented and documented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related role-permission management components and documentation
# Look for role edit/update components
echo "Searching for role management components..."
fd -e tsx -e ts --full-path '.*roles.*' -x rg -l 'permission'
# Check for documentation about the role creation flow
echo "Checking for flow documentation..."
fd -e md -e mdx --full-path '.*roles.*|.*authorization.*' -x rg -l 'role.*permission|permission.*role'
Length of output: 1773
apps/dashboard/app/(app)/settings/general/update-workspace-image.tsx (1)
60-60: LGTM! Improved dependency array.
The simplified dependency array is correct. Since setImage is a stable setState function, it doesn't need to be included in the dependencies.
apps/play/app/page-bk.tsx (1)
37-37: LGTM! Correct dependency array optimization.
The dependency array is now properly optimized:
- Keeps
apiIdas it's a regular variable - Removes ref values (
keyId,keyName,timeStamp) as they're mutable and don't require re-creation of the callback
| useEffect(() => { | ||
| scrollRef?.current?.scrollIntoView({ behavior: "smooth" }); | ||
| }, [scrollRef]); | ||
| }, []); |
There was a problem hiding this comment.
Restore scrollRef dependency to ensure proper scrolling behavior.
The empty dependency array means the scroll effect only runs once on mount. This could cause issues if:
- scrollRef is initialized after the initial mount
- New messages are added and need to be scrolled into view
Apply this diff to restore the proper behavior:
- }, []);
+ }, [scrollRef]);📝 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.
| }, []); | |
| }, [scrollRef]); |
There was a problem hiding this comment.
scrollRef is a React component ref created with useRef hook thus it does not change between component re-renders. This means scrollRef being in a dependency array of a useEffect hooks has no effect.
There was a problem hiding this comment.
@unrenamed, thank you for the clarification. You're correct that refs created with useRef do not change between renders, so including scrollRef in the dependency array does not have an effect. I'll keep this in mind for future reviews.
✏️ Learnings added
Learnt from: unrenamed
PR: unkeyed/unkey#2660
File: apps/play/app/page-bk.tsx:24-24
Timestamp: 2024-11-13T14:58:01.054Z
Learning: In React, refs created with useRef do not change between renders, so including them in the dependency array of useEffect hooks is unnecessary.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
scrollRef?. the first optional chaining operator makes no sense here though, since current property always exist in a component ref — it just may be null sometimes. I'll change this in a separate commit.
There was a problem hiding this comment.
@unrenamed, you're absolutely right. The optional chaining operator on scrollRef is unnecessary since scrollRef created with useRef will always be defined. Thanks for catching that.
✏️ Learnings added
Learnt from: unrenamed
PR: unkeyed/unkey#2660
File: apps/play/app/page-bk.tsx:24-24
Timestamp: 2024-11-13T15:01:50.195Z
Learning: In React, refs created with `useRef` are always defined, even if their `current` property may be `null`. Therefore, it's unnecessary to use optional chaining on the ref object itself (e.g., `scrollRef?.`).
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
2 participants
What does this PR do?
Refines lint warning fixes, building on #2652. Progresses towards resolving #2605.
Type of change
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Removals
XAxisandYAxiscomponents to streamline chart rendering.HoverBorderGradientcomponent due to redundancy.Improvements