fix: resolve 15 of 16 remaining lint warnings #2662
Conversation
unrenamed
requested review from
perkinsjr,
chronark,
mcstepp and
MichaelUnkey
as code owners
|
|
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. |
📝 WalkthroughWalkthroughThe pull request introduces several modifications across multiple components, primarily focusing on enhancing form state management and submission logic. Key changes include the addition of Changes
Possibly related PRs
Suggested labels
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! 🙏 |
![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 (8)
apps/dashboard/app/auth/sign-in/email-signin.tsx (2)
Line range hint
24-52: Prevent memory leaks in async operations.The async operation might continue after component unmount, potentially causing memory leaks or state updates on unmounted component.
Add cleanup logic:
React.useEffect(() => { + let mounted = true; const signUpOrgUser = async () => { if (!signInLoaded) { return; } const ticket = new URL(window.location.href).searchParams.get(param); if (!ticket) { return; } - setIsLoading(true); + if (mounted) setIsLoading(true); await signIn .create({ strategy: "ticket", ticket, }) .then((result) => { + if (!mounted) return; if (result.status === "complete" && result.createdSessionId) { setActive({ session: result.createdSessionId }).then(() => { + if (mounted) router.push("/apis"); }); } }) .catch((err) => { - setIsLoading(false); + if (mounted) setIsLoading(false); console.error(err); }); }; signUpOrgUser(); + return () => { + mounted = false; + }; }, [signInLoaded, router, signIn, setActive]);
Line range hint
54-85: Improve error handling and prevent memory leaks in signInWithCode.The function has several areas for improvement:
- Potential memory leaks in async operations
- Error handling could be more type-safe
- Generic error message could be more specific
Consider these improvements:
const signInWithCode = async (e: React.FormEvent<HTMLFormElement>) => { + let mounted = true; e.preventDefault(); const email = new FormData(e.currentTarget).get("email"); if (!signInLoaded || typeof email !== "string") { return null; } - setIsLoading(true); + if (mounted) setIsLoading(true); await signIn .create({ identifier: email, }) .then(async () => { + if (!mounted) return; const firstFactor = signIn.supportedFirstFactors.find((f) => f.strategy === "email_code") as | { emailAddressId: string } | undefined; if (firstFactor) { await signIn.prepareFirstFactor({ strategy: "email_code", emailAddressId: firstFactor.emailAddressId, }); - setIsLoading(false); + if (mounted) setIsLoading(false); props.verification(true); } setLastUsed("email"); }) .catch((err: { errors: Array<{ code: string; message: string }> }) => { - setIsLoading(false); + if (mounted) setIsLoading(false); if (err.errors[0].code === "form_identifier_not_found") { props.setAccountNotFound(true); props.email(email); } else { - props.setError("We couldn't sign you in. Please try again later"); + props.setError(`Sign-in failed: ${err.errors[0].message || 'Please try again later'}`); } }); + return () => { + mounted = false; + }; };apps/dashboard/app/auth/sign-up/email-signup.tsx (1)
Line range hint
24-77: Refactor useEffect to improve state management and error handlingThe current implementation has several areas for improvement:
- Duplicate state reset
- Incomplete error handling
- Multiple state mutations that could lead to race conditions
Consider applying these changes:
React.useEffect(() => { const signUpFromParams = async () => { if (!signUpLoaded) { return; } 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) { const result = await signUp?.create({ strategy: "ticket", ticket, }); if (result?.status === "complete" && result.createdSessionId) { await setActive({ session: result.createdSessionId }); router.push("/apis"); } } if (emailParam) { await signUp?.create({ emailAddress: emailParam, }); await signUp.prepareEmailAddressVerification(); setVerification(true); } - } catch (err) { + } catch (err: any) { if (err.errors?.[0]?.code === "form_identifier_exists") { toast.error("It looks like you have an account. Please use sign in"); + } else { + toast.error("An error occurred during sign up"); + console.error(err); } + } finally { + setTransferLoading(false); } }; signUpFromParams(); - setTransferLoading(false); }, [signUpLoaded]);apps/dashboard/app/(app)/settings/root-keys/new/client.tsx (1)
Line range hint
96-117: Consider revising the useEffect dependency handlingWhile the performance optimization using Set is excellent, ignoring the exhaustive dependencies warning might lead to bugs. The current implementation won't react to changes in
apisorselectedPermissions.Consider this alternative approach that properly handles dependencies while maintaining performance:
- // biome-ignore lint/correctness/useExhaustiveDependencies: effect must be called once to set initial cards state useEffect(() => { const initialSelectedApiSet = new Set<string>(); selectedPermissions.forEach((permission) => { const apiId = permission.split(".")[1] ?? ""; // Extract API ID if (apiId.length) { initialSelectedApiSet.add(apiId); } }); const initialCardStates: Record<string, boolean> = {}; apis.forEach((api) => { initialCardStates[api.id] = initialSelectedApiSet.has(api.id); // O(1) check }); - // We use a Set to gather unique API IDs, enabling O(1) membership checks. - // This avoids the O(m * n) complexity of repeatedly iterating over selectedPermissions - // for each API, reducing the overall complexity to O(n + m) and improving performance - // for large data sets. setCardStatesMap(initialCardStates); - }, []); // Execute ones on the first load + }, [apis, selectedPermissions]);This ensures the card states stay in sync with both the APIs and selected permissions while maintaining the O(n + m) performance characteristics.
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/client.tsx (4)
Line range hint
262-267: Consider refactoring the useEffect hook to properly handle dependencies.While the biome-ignore comment explains the intent, it's better to follow React's best practices for handling effect dependencies.
Consider this alternative implementation:
- // biome-ignore lint/correctness/useExhaustiveDependencies: reset is only required on mount - useEffect(() => { - // React hook form + zod doesn't play nice with nested objects, so we need to reset them on load. - resetRateLimit(); - resetLimited(); - }, []); + useEffect(() => { + const initializeForm = () => { + resetRateLimit(); + resetLimited(); + }; + initializeForm(); + }, [resetRateLimit, resetLimited]);Also, consider memoizing the reset functions using useCallback to prevent unnecessary re-renders:
const resetRateLimit = useCallback(() => { form.resetField("ratelimit.duration", undefined); form.resetField("ratelimit.limit", undefined); form.resetField("ratelimit", undefined); }, [form]);
Line range hint
4-4: Address the TODO comment for test coverage.This component handles complex form state and validation logic. It's crucial to have comprehensive test coverage for:
- Form validation scenarios
- State management
- API integration
- Error handling
Would you like me to help generate test cases for this component? I can create a new GitHub issue with:
- Unit tests for form validation
- Integration tests for API interactions
- Snapshot tests for UI states
Line range hint
262-267: Fix the refill day handling and improve object manipulation.There are several issues in the form submission logic:
- The refill day assignment has a bug
- The values object is being mutated directly
Apply these fixes:
- const refill = values.limit?.refill; - if (refill?.interval === "daily") { - refill?.refillDay === undefined; - } - if (refill?.interval === "monthly" && !refill.refillDay) { - refill.refillDay = 1; - } + const formValues = { ...values }; + if (formValues.limit?.refill?.interval === "daily") { + delete formValues.limit.refill.refillDay; + } else if (formValues.limit?.refill?.interval === "monthly" && !formValues.limit.refill.refillDay) { + formValues.limit.refill.refillDay = 1; + }
Line range hint
32-38: Improve form error handling consistency.The form has several areas where error handling could be improved:
- Duplicate error message displays
- Inconsistent error message placement
Consider centralizing error handling:
const ErrorMessage: React.FC<{ name: string }> = ({ name }) => { const error = form.formState.errors[name]; return error ? ( <p className="text-xs text-content-alert">{error.message}</p> ) : null; };Then replace duplicate error displays with the centralized component:
-{form.formState.errors.ratelimit && ( - <p className="text-xs text-center text-content-alert"> - {form.formState.errors.ratelimit.message} - </p> -)} +<ErrorMessage name="ratelimit" />Also applies to: 262-267
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/client.tsx(1 hunks)apps/dashboard/app/(app)/settings/root-keys/new/client.tsx(1 hunks)apps/dashboard/app/auth/sign-in/email-signin.tsx(1 hunks)apps/dashboard/app/auth/sign-up/email-signup.tsx(2 hunks)apps/www/components/shiny-card.tsx(1 hunks)
🔇 Additional comments (5)
apps/www/components/shiny-card.tsx (2)
63-63: LGTM! Dependency array fix addresses potential stale closure issues.
The addition of boxes to the dependency array is correct and resolves the ESLint exhaustive-deps warning.
63-63: Consider memoizing boxes array for performance optimization.
While the dependency array fix is necessary, it could cause unnecessary re-renders when boxes change. Consider memoizing the boxes array if performance becomes an issue:
- const [boxes, setBoxes] = useState<Array<HTMLElement>>([]);
+ const [boxes, setBoxes] = useState<Array<HTMLElement>>([]);
+ const memoizedBoxes = useMemo(() => boxes, []);
useEffect(() => {
containerRef.current &&
setBoxes(Array.from(containerRef.current.children).map((el) => el as HTMLElement));
}, []);
const onMouseMove = useCallback(() => {
// ... existing code ...
- }, [boxes, mousePosition.x, mousePosition.y]);
+ }, [memoizedBoxes, mousePosition.x, mousePosition.y]);apps/dashboard/app/auth/sign-up/email-signup.tsx (2)
27-29: LGTM: Good defensive programming practice
The early return when signUpLoaded is false is a good practice as it prevents unnecessary execution and follows the fail-fast principle.
Line range hint 23-77: Verify consistent error handling across auth components
Let's ensure that error handling and loading states are consistent across all authentication components.
✅ Verification successful
Error handling and loading states are consistently implemented across auth components
After analyzing the error handling patterns across authentication components:
- All components properly manage loading states with
setIsLoading/setTransferLoading - Error handling is consistent:
- OAuth flows use toast notifications for generic errors
- Email flows properly handle specific error cases (e.g., "form_identifier_exists")
- Loading states are properly reset in both success and error cases
- All errors are logged via
console.error
The implementation in email-signup.tsx follows these same patterns and is consistent with the rest of the authentication flow.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar auth components and their error handling patterns
echo "Searching for auth components..."
rg -t tsx "useSignUp|useSignIn" --glob "apps/dashboard/app/auth/**"
echo "Checking error handling patterns..."
rg -t tsx "catch \(.*\) {" --glob "apps/dashboard/app/auth/**" -A 5
echo "Checking loading state usage..."
rg -t tsx "setIsLoading|setTransferLoading" --glob "apps/dashboard/app/auth/**" -A 2
Length of output: 603
Script:
#!/bin/bash
# Search for similar auth components and their error handling patterns
echo "Searching for auth components..."
rg "useSignUp|useSignIn" apps/dashboard/app/auth/
echo "Checking error handling patterns..."
rg "catch.*{" apps/dashboard/app/auth/ -A 5
echo "Checking loading state usage..."
rg "setIsLoading|setTransferLoading" apps/dashboard/app/auth/ -A 2
Length of output: 9560
Script:
#!/bin/bash
# Let's check the error handling patterns more specifically
echo "Checking error handling in sign-up components..."
rg "catch.*\(" apps/dashboard/app/auth/sign-up/ -B 2 -A 5
echo "Checking error handling in sign-in components..."
rg "catch.*\(" apps/dashboard/app/auth/sign-in/ -B 2 -A 5
echo "Checking toast error patterns..."
rg "toast\.error" apps/dashboard/app/auth/ -B 2 -A 2
Length of output: 9415
apps/dashboard/app/(app)/settings/root-keys/new/client.tsx (1)
Line range hint 1-95: LGTM! The implementation demonstrates solid practices
The code shows good attention to:
- Error handling in TRPC mutations
- Accessibility in UI components
- Clean state management
- Proper cleanup on dialog close
Also applies to: 118-324
| @@ -20,13 +20,14 @@ export function EmailSignIn(props: { | |||
| const router = useRouter(); | |||
| const [lastUsed, setLastUsed] = useLastUsed(); | |||
|
|
|||
| // biome-igmore lint: this works | |||
| // biome-ignore lint/correctness/useExhaustiveDependencies: effect must be called once if sign-in is loaded | |||
There was a problem hiding this comment.
Remove unnecessary lint ignore comment and fix dependencies.
The lint ignore comment is masking a real issue. The effect has implicit dependencies on router, signIn, and setActive that should be included in the dependency array.
Remove the ignore comment and fix the dependencies:
- // biome-ignore lint/correctness/useExhaustiveDependencies: effect must be called once if sign-in is loaded
- React.useEffect(() => {
+ React.useEffect(() => {Then add the missing dependencies:
- }, [signInLoaded]);
+ }, [signInLoaded, router, signIn, setActive]);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
see my comment below 👇
There was a problem hiding this comment.
@unrenamed, understood. Thanks for the clarification.
Why use
|
unrenamed
changed the title
What does this PR do?
Resolves all lint warnings except the final one, which is more complex and may require input from a core team member for optimal resolution, 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
These updates aim to enhance user experience by optimizing performance and ensuring smoother interactions across the application.