Conversation
cruzdanilo
requested review from
dieguezguille,
franm91,
itofarina and
nfmelendez
as code owners
🦋 Changeset detectedLatest commit: 514e147 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Caution Review failedThe pull request is closed. WalkthroughLarge-scale migration consolidating ESLint configurations to flat config format, upgrading major dependencies (Expo 52→54, React 18→19, React Native 0.76→0.81, Wagmi 2→3, pnpm 9→10), migrating from Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (133)
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 |
Summary of ChangesHello @cruzdanilo, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a comprehensive overhaul of the application's foundational technologies and user interface. It modernizes the development stack by upgrading critical libraries and adopting a streamlined ESLint setup. The changes aim to enhance the application's robustness, responsiveness, and overall user experience by resolving existing issues and introducing refined component interactions. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| entryPoint, | ||
| source: "WebauthnAccount" as const, | ||
| getAccountInitCode: () => Promise.resolve(accountInitCode({ factory, x, y })), | ||
| getDummySignature: () => "0x", | ||
| getDummySignature: () => DUMMY_SIGNATURE, | ||
| signUserOperationHash: async (uoHash) => { | ||
| try { | ||
| if (queryClient.getQueryData<AuthMethod>(["method"]) === "siwe" && getAccount(ownerConfig).address) { | ||
| if (queryClient.getQueryData<AuthMethod>(["method"]) === "siwe" && getConnection(ownerConfig).address) { | ||
| return wrapSignature(0, await signMessage(ownerConfig, { message: { raw: uoHash } })); |
There was a problem hiding this comment.
Bug: The code accesses a non-existent property isDisconnected on the object from getConnection. The correct property is status, which will break the SIWE authentication flow.
Severity: CRITICAL
🔍 Detailed Analysis
In the signIn mutation function within src/utils/useAuth.ts, the code attempts to check the connection status using getConnection(ownerConfig).isDisconnected. However, the object returned by Wagmi's getConnection function does not have an isDisconnected property. The correct property to check is status. Accessing a non-existent property will evaluate to undefined, causing the conditional check to fail. This will break the Sign-In With Ethereum (SIWE) authentication flow, preventing it from triggering a new connection when one is needed.
💡 Suggested Fix
In src/utils/useAuth.ts, update the conditional check to use the status property. Change if (method === "siwe" && getConnection(ownerConfig).isDisconnected) to if (method === "siwe" && getConnection(ownerConfig).status === 'disconnected').
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/utils/accountClient.ts#L62-L69
Potential issue: In the `signIn` mutation function within `src/utils/useAuth.ts`, the
code attempts to check the connection status using
`getConnection(ownerConfig).isDisconnected`. However, the object returned by Wagmi's
`getConnection` function does not have an `isDisconnected` property. The correct
property to check is `status`. Accessing a non-existent property will evaluate to
`undefined`, causing the conditional check to fail. This will break the Sign-In With
Ethereum (SIWE) authentication flow, preventing it from triggering a new connection when
one is needed.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 8445513
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a major undertaking, upgrading numerous dependencies including Expo, React, React Native, Wagmi, and Tamagui. The migration to ESLint flat config and pnpm catalogs is a significant improvement for maintainability. The core logic changes to support EIP-5792 are well-implemented. My review focuses on a few subtle issues related to React's ref system that might have been missed during the large-scale refactoring. Overall, this is an excellent and crucial upgrade for the project.
I am having trouble creating individual review comments. Click here to see my feedback.
src/components/activity/Activity.tsx (96-97)
Using a plain object literal { current: null } for a ref is not recommended. While it might work in some cases, it relies on React's implementation details and is not the idiomatic way to create refs. You should use React.createRef() for module-level refs to ensure they are correctly handled by React. This change breaks the "scroll to top on tab press" functionality. This issue is also present in other files like Card.tsx, DeFi.tsx, Home.tsx, PayMode.tsx, and Swaps.tsx.
export const activityScrollReference = React.createRef<FlatList<ActivityItemType>>();
export const activityRefreshControlReference = React.createRef<RefreshControl>();
src/components/card/exa-card/ExaCard.tsx (48-57)
Similar to AnimatedView, the forwardedRef pattern is implemented incorrectly here. Animated.createAnimatedComponent passes a ref prop that is not received by the YStackWithForwardedReference function component. To fix this and allow refs to be passed to the animated YStack, you should use React.forwardRef.
const YStackWithForwardedReference = React.forwardRef<
React.ComponentRef<typeof YStack>,
ComponentPropsWithoutRef<typeof YStack>
>((properties, ref) => {
return <YStack ref={ref} {...properties} />;
});
YStackWithForwardedReference.displayName = "YStackWithForwardedRef";
const AnimatedYStack = Animated.createAnimatedComponent(YStackWithForwardedReference);
src/components/shared/AnimatedView.tsx (5-14)
The forwardedRef pattern is implemented incorrectly here. Animated.createAnimatedComponent will pass a ref prop, but a standard function component cannot receive it. This means the ref will be lost. To correctly forward the ref to the underlying View component, you should wrap your component in React.forwardRef.
const ViewWithForwardedReference = React.forwardRef<
React.ComponentRef<typeof View>,
ComponentPropsWithoutRef<typeof View>
>((properties, ref) => {
return <View ref={ref} {...properties} />;
});
ViewWithForwardedReference.displayName = "ViewWithForwardedRef";
export default Animated.createAnimatedComponent(ViewWithForwardedReference);
src/components/shared/Text.tsx (28-42)
The ref is not being forwarded correctly here. The ref prop is special in React and is not passed down as a normal prop. To forward a ref to a function component, you must wrap it in React.forwardRef. The current implementation will not forward the ref to the underlying StyledText component, which can cause issues when a parent component tries to get a ref to this Text component.
const TextComponent = React.forwardRef<
React.ComponentRef<typeof StyledText>,
ComponentPropsWithoutRef<typeof StyledText> & { sensitive?: boolean }
>(({ children, sensitive, ...rest }, reference) => {
const { data: hidden } = useQuery<boolean>({ queryKey: ["settings", "sensitive"] });
return (
<StyledText ref={reference} {...rest}>
{sensitive && hidden ? "***" : children}
</StyledText>
);
});
1 participant
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.