fix: transaction issue from social wallets #467
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Warning Rate limit exceeded@chalabi2 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 13 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Modal
participant useClientReset
participant Web3AuthContext
participant ChainProvider
User->>Modal: Clicks to connect/disconnect social wallet
Modal->>useClientReset: Calls forceCompleteReset()
useClientReset->>Web3AuthContext: Calls resetWeb3AuthClients()
Web3AuthContext-->>useClientReset: Resets Web3Auth state
useClientReset->>ChainProvider: Triggers forceChainProviderReset()
ChainProvider-->>useClientReset: Remounts and clears cached state
useClientReset-->>Modal: Reset complete
Modal->>User: Proceeds with wallet connection or disconnection
Assessment against linked issues
Poem
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #467 +/- ##
==========================================
+ Coverage 56.07% 58.46% +2.39%
==========================================
Files 235 236 +1
Lines 16179 16106 -73
==========================================
+ Hits 9073 9417 +344
+ Misses 7106 6689 -417 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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 comments (1)
components/react/modal.tsx (1)
268-283:⚠️ Potential issueMissing implementation for social wallet reset logic.
The comments indicate steps 2-3 should check if this is a social wallet and reset clients if already connected, but the actual implementation is missing from the
onWalletClickedfunction body.Based on the PR objectives to fix transaction issues from social wallets, add the missing reset logic:
// Step 2: Check if this is a social wallet and if we should reset clients + const isSocialWallet = wallet?.walletInfo.name?.includes('web3auth'); + if (isSocialWallet && current?.address) { + await forceCompleteReset(); + } + // Step 3: Special case for keplr - check immediately🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 268-270: components/react/modal.tsx#L268-L270
Added lines #L268 - L270 were not covered by tests
[warning] 273-273: components/react/modal.tsx#L273
Added line #L273 was not covered by tests
[warning] 282-283: components/react/modal.tsx#L282-L283
Added lines #L282 - L283 were not covered by tests
🧹 Nitpick comments (1)
hooks/useTx.tsx (1)
27-49: Improved error message extraction with comprehensive patterns.The multiple regex patterns provide better coverage for different error message formats, making error messages more user-friendly.
Fix the optional chaining issue flagged by Biome on line 39:
- if (match && match[1]) { + if (match?.[1]) {🧰 Tools
🪛 Biome (1.9.4)
[error] 39-39: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 GitHub Check: codecov/patch
[warning] 27-41: hooks/useTx.tsx#L27-L41
Added lines #L27 - L41 were not covered by tests
[warning] 44-47: hooks/useTx.tsx#L44-L47
Added lines #L44 - L47 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
components/react/modal.tsx(6 hunks)contexts/manifestAppProviders.tsx(3 hunks)contexts/web3AuthContext.tsx(5 hunks)hooks/useClientReset.ts(1 hunks)hooks/useTx.tsx(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
hooks/useClientReset.ts (1)
contexts/web3AuthContext.tsx (1)
Web3AuthContext(29-38)
components/react/modal.tsx (1)
hooks/useClientReset.ts (1)
useClientReset(10-34)
hooks/useTx.tsx (2)
contexts/web3AuthContext.tsx (1)
Web3AuthContext(29-38)contexts/toastContext.tsx (1)
useToast(34-40)
🪛 GitHub Check: codecov/patch
hooks/useClientReset.ts
[warning] 10-32: hooks/useClientReset.ts#L10-L32
Added lines #L10 - L32 were not covered by tests
components/react/modal.tsx
[warning] 86-86: components/react/modal.tsx#L86
Added line #L86 was not covered by tests
[warning] 268-270: components/react/modal.tsx#L268-L270
Added lines #L268 - L270 were not covered by tests
[warning] 273-273: components/react/modal.tsx#L273
Added line #L273 was not covered by tests
[warning] 282-283: components/react/modal.tsx#L282-L283
Added lines #L282 - L283 were not covered by tests
[warning] 294-294: components/react/modal.tsx#L294
Added line #L294 was not covered by tests
[warning] 302-302: components/react/modal.tsx#L302
Added line #L302 was not covered by tests
[warning] 308-308: components/react/modal.tsx#L308
Added line #L308 was not covered by tests
[warning] 424-427: components/react/modal.tsx#L424-L427
Added lines #L424 - L427 were not covered by tests
hooks/useTx.tsx
[warning] 27-41: hooks/useTx.tsx#L27-L41
Added lines #L27 - L41 were not covered by tests
[warning] 44-47: hooks/useTx.tsx#L44-L47
Added lines #L44 - L47 were not covered by tests
[warning] 74-79: hooks/useTx.tsx#L74-L79
Added lines #L74 - L79 were not covered by tests
[warning] 82-84: hooks/useTx.tsx#L82-L84
Added lines #L82 - L84 were not covered by tests
contexts/web3AuthContext.tsx
[warning] 139-171: contexts/web3AuthContext.tsx#L139-L171
Added lines #L139 - L171 were not covered by tests
[warning] 181-189: contexts/web3AuthContext.tsx#L181-L189
Added lines #L181 - L189 were not covered by tests
🪛 Biome (1.9.4)
hooks/useTx.tsx
[error] 39-39: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (15)
components/react/modal.tsx (4)
26-26: LGTM! Clean import of the client reset hook.
86-86: LGTM! Proper destructuring of the reset function.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 86-86: components/react/modal.tsx#L86
Added line #L86 was not covered by tests
273-273: LGTM! Function correctly converted to async.The async conversion enables the use of
awaitfor the client reset operations.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 273-273: components/react/modal.tsx#L273
Added line #L273 was not covered by tests
424-427: LGTM! Client reset on disconnect prevents stale state.Calling
forceCompleteReset()after disconnection ensures clean state for subsequent connections.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 424-427: components/react/modal.tsx#L424-L427
Added lines #L424 - L427 were not covered by testscontexts/manifestAppProviders.tsx (4)
40-40: LGTM! State for managing ChainProvider resets.
80-83: LGTM! Clean implementation of force reset function.The
useCallbackensures stable reference and the key increment pattern is a standard React technique for forcing component remounts.
85-92: LGTM! Proper context value extension.Using
useMemowith correct dependencies ensures the context value is stable unless the underlying values change.
95-122: LGTM! ChainProvider setup with reset capability.The key prop on ChainProvider ensures complete remount when
chainProviderKeychanges, effectively clearing all cached state. The Web3AuthContext.Provider wrapper makes the reset function available to consumers.hooks/useTx.tsx (3)
53-54: LGTM! Extended destructuring for wallet connection management.Adding
disconnectandconnectaligns with the broader wallet connection management improvements in this PR.
57-57: LGTM! Address tracking for switch detection.
74-83: Smart address change handling with delay.The 200ms delay allows ongoing wallet state changes to complete before acquiring the signing client, which should prevent issues with rapid account switching.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 74-79: hooks/useTx.tsx#L74-L79
Added lines #L74 - L79 were not covered by testscontexts/web3AuthContext.tsx (4)
25-27: LGTM! Clean interface extension for reset functionality.
36-37: LGTM! Consistent default values in context.
139-170: LGTM! Comprehensive Web3Auth client reset implementation.The function properly:
- Clears login hints for all Web3Auth wallets
- Handles concurrent disconnection with
Promise.all- Includes proper error handling and logging
- Checks wallet types and connection status before operations
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 139-171: contexts/web3AuthContext.tsx#L139-L171
Added lines #L139 - L171 were not covered by tests
181-189: LGTM! Context value properly includes reset function.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 181-189: contexts/web3AuthContext.tsx#L181-L189
Added lines #L181 - L189 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
hooks/__tests__/useClientReset.test.tsx (1)
78-86: Consider adding tests for other potential error scenarios.While the current error test covers
resetWeb3AuthClientsfailures, you might want to consider adding tests for scenarios wherequeryClient.clear()orforceChainProviderReset()throw errors to ensure complete error handling coverage.Example additional test cases:
test('forceCompleteReset handles queryClient.clear errors gracefully', async () => { mockQueryClient.clear.mockImplementation(() => { throw new Error('Query client clear failed'); }); const { result } = renderHook(() => useClientReset(), { wrapper }); await expect(result.current.forceCompleteReset()).rejects.toThrow('Query client clear failed'); expect(mockResetWeb3AuthClients).toHaveBeenCalledTimes(1); expect(mockQueryClient.clear).toHaveBeenCalledTimes(1); }); test('forceCompleteReset handles forceChainProviderReset errors gracefully', async () => { mockForceChainProviderReset.mockImplementation(() => { throw new Error('Chain provider reset failed'); }); const { result } = renderHook(() => useClientReset(), { wrapper }); await expect(result.current.forceCompleteReset()).rejects.toThrow('Chain provider reset failed'); expect(mockResetWeb3AuthClients).toHaveBeenCalledTimes(1); expect(mockQueryClient.clear).toHaveBeenCalledTimes(1); expect(mockForceChainProviderReset).toHaveBeenCalledTimes(1); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contexts/__tests__/web3AuthContext.test.tsx(1 hunks)hooks/__tests__/useClientReset.test.tsx(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
contexts/__tests__/web3AuthContext.test.tsx (2)
tests/mock.ts (2)
mockModule(23-60)clearAllMocks(11-15)contexts/web3AuthContext.tsx (2)
Web3AuthContext(29-38)Web3AuthProvider(40-194)
hooks/__tests__/useClientReset.test.tsx (3)
tests/mock.ts (2)
mockModule(23-60)clearAllMocks(11-15)contexts/web3AuthContext.tsx (1)
Web3AuthContext(29-38)hooks/useClientReset.ts (1)
useClientReset(10-34)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (14)
hooks/__tests__/useClientReset.test.tsx (8)
1-8: LGTM: Clean imports and proper setup.The imports are well-organized and include all necessary testing utilities and dependencies.
10-36: LGTM: Comprehensive mock setup.The mock setup properly covers all dependencies of the
useClientResethook, including the Web3Auth context functions and React Query client. The mocks are appropriately configured to match the expected behavior.
38-46: LGTM: Proper cleanup and wrapper setup.The cleanup strategy is thorough, clearing all mocks and performing React Testing Library cleanup. The wrapper component correctly provides the mocked context.
48-53: LGTM: Basic functionality test.This test correctly verifies that the hook returns the expected
forceCompleteResetfunction.
55-64: LGTM: Comprehensive happy path test.This test thoroughly verifies the complete reset sequence when all dependencies are available, ensuring all three operations are called in the correct order.
66-76: LGTM: Edge case handling.This test correctly verifies the behavior when
forceChainProviderResetis undefined, ensuring the function returnsfalseand doesn't attempt to call the reset functions.
78-86: LGTM: Error handling test.This test properly verifies that errors from
resetWeb3AuthClientspropagate correctly. The implementation correctly stops execution when the first async operation fails.
88-98: LGTM: Memoization verification.This test correctly verifies that the
forceCompleteResetcallback is properly memoized usinguseCallback, which is important for performance in React applications.contexts/__tests__/web3AuthContext.test.tsx (6)
1-8: LGTM: Clean imports and setup.The imports are well-organized and appropriate for a React component test using Bun test framework.
10-53: Comprehensive mocking setup looks good.The mock configuration correctly covers all the required dependencies:
@cosmos-kit/web3authwith realistic mock classes@web3auth/authfor network typescosmos-kitand extension wallets- Environment configuration
The mock classes provide the necessary methods and properties that match the real implementation based on the context from
contexts/web3AuthContext.tsx.
55-59: Proper cleanup implementation.Good use of multiple cleanup methods to ensure test isolation.
61-73: Good verification of resetWeb3AuthClients function availability.This test correctly verifies that the new
resetWeb3AuthClientsfunction is provided by the context as expected.
75-100: Comprehensive context value validation.Excellent coverage of all context properties and their expected types. The test properly validates the context interface.
102-122: Good async function testing.The test correctly verifies that
resetWeb3AuthClientscan be called without throwing errors, which is important for the reset functionality.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
components/react/__tests__/modal.test.tsx (3)
14-14: Replaceanywith proper types for better type safety.Using
anytype defeats TypeScript's type checking. Consider creating proper type definitions or using the actual types from cosmos-kit.- let mockWalletRepo: any; + let mockWalletRepo: WalletRepository;
29-59: Consider extracting window mock setup to a helper function.The window mock setup is extensive and could be reused across test files. Extracting it would improve maintainability and reduce duplication.
Create a helper function:
// In tests/helpers/windowMocks.ts export function createMockWindow(overrides?: Partial<Window>) { return { keplr: undefined, open: jest.fn(), addEventListener: jest.fn(), removeEventListener: jest.fn(), localStorage: { getItem: jest.fn(), setItem: jest.fn(), removeItem: jest.fn(), clear: jest.fn(), }, matchMedia: jest.fn(() => ({ matches: false, addListener: jest.fn(), removeListener: jest.fn(), addEventListener: jest.fn(), removeEventListener: jest.fn(), dispatchEvent: jest.fn(), })), navigator: { platform: 'MacIntel', userAgent: 'test', maxTouchPoints: 0, }, document: { body: { style: {}, }, }, ...overrides, }; }Then use it in the test:
- (global as any).window = { - ...originalWindow, - keplr: undefined, - open: jest.fn(), - // ... rest of the mock - }; + (global as any).window = createMockWindow({ ...originalWindow });
349-349: Consider a more robust approach for mocking Web3AuthClient instance.Using
Object.setPrototypeOfto satisfyinstanceofchecks is fragile and could break with library updates. Consider using a proper mock instance or adjusting the implementation to not rely oninstanceof.- Object.setPrototypeOf(mockClient, Web3AuthClient.prototype); + // Create a proper mock instance that extends Web3AuthClient + const mockClient = Object.create(Web3AuthClient.prototype); + mockClient.setLoginHint = jest.fn();Or better yet, mock the entire Web3AuthClient class:
jest.mock('@cosmos-kit/web3auth', () => ({ Web3AuthClient: jest.fn().mockImplementation(() => ({ setLoginHint: jest.fn(), })), }));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/react/__tests__/modal.test.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (1)
components/react/__tests__/modal.test.tsx (1)
307-335: Good test coverage for the client reset functionality.This test properly validates the new
forceCompleteResetfunctionality that addresses the stale data issue mentioned in the PR objectives. The test correctly mocks the hook and verifies the reset is called on disconnect.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
components/react/__tests__/__snapshots__/ModalDialog.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (1)
tests/format.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
| icons: [], | ||
| <Web3AuthContext.Provider value={contextValue}> | ||
| <ChainProvider | ||
| key={chainProviderKey} // This forces complete re-mount and reset of cosmos-kit |
There was a problem hiding this comment.
Cute. Internal React mechanism. The key parameter doesn't belong to ChainProvider.
| // Check if the address has changed since last use (indicating a potential account switch) | ||
| if (lastUsedAddress.current && lastUsedAddress.current !== address) { | ||
| // Add a small delay to allow any ongoing wallet state changes to complete | ||
| await new Promise(resolve => setTimeout(resolve, 200)); |
There was a problem hiding this comment.
[nit] I'm not a big fan of those kinds of delays.
2 participants
fixes #450
The issue was that the wallet was caching stale data in the ChainProvider for web3auth wallets
This PR does not meet the target coverage because it does not contain tests for the useTx hook but #466 does.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Chores