Feature: New UB Components #7354
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/thirdweb/src/react/core/hooks/wallets/useAutoConnectCore.test.tsx (1)
16-18: Avoid shared state between tests by recreatingMockStorageandConnectionManagerper test
mockStorageandmanagerare instantiated once at module scope and then reused across all test cases.
Both of these objects are stateful (e.g.useAutoConnectCoremay persistlastConnectedWalletIdsinmockStorage).
A mutation in one test can therefore leak into the next test and cause flaky, order-dependent results.- const mockStorage = new MockStorage(); - const manager = createConnectionManager(mockStorage); +let mockStorage: MockStorage; +let manager: ReturnType<typeof createConnectionManager>; + +beforeEach(() => { + mockStorage = new MockStorage(); + manager = createConnectionManager(mockStorage); +});The wrapper defined below will keep referencing the freshly created
managerbecause it’s closed over a mutable variable.
This small refactor guarantees isolation and makes future additions (or reinstating the timeout test) safer.
🧹 Nitpick comments (1)
packages/thirdweb/src/react/core/hooks/wallets/useAutoConnectCore.test.tsx (1)
94-99: Redundant explicit timeout
waitForalready defaults to a 1000 ms timeout, so passing{ timeout: 1000 }is unnecessary noise.
Dropping the option keeps the test concise and avoids the impression that the value is significant.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/thirdweb/src/react/core/hooks/wallets/useAutoConnectCore.test.tsx(1 hunks)packages/thirdweb/src/wallets/connection/autoConnectCore.test.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/thirdweb/src/wallets/connection/autoConnectCore.test.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Size
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
packages/thirdweb/src/react/core/hooks/wallets/useAutoConnectCore.test.tsx (1)
1-3:onTimeoutbehaviour is no longer covered – regression in test scopeThe previous commit included a test asserting that
useAutoConnectCorecalls theonTimeoutcallback; that entire scenario has been removed.
If the timeout branch is still part of the hook’s contract, please add a replacement test (or confirm that the branch was deleted and can be safely untested).
This keeps critical error-handling logic from silently regressing.
gregfromstl
force-pushed
the
greg/new-ub-components
branch
from
1a7e067 to
cfc7c79
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/dashboard/src/app/(app)/(dashboard)/(chain)/[chain_id]/(chainPage)/components/client/PayModal.tsx(1 hunks)apps/dashboard/src/app/pay/components/client/PayPageEmbed.client.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/dashboard/src/app/(app)/(dashboard)/(chain)/[chain_id]/(chainPage)/components/client/PayModal.tsx
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Size
- GitHub Check: Unit Tests
- GitHub Check: Lint Packages
- GitHub Check: Analyze (javascript)
| @@ -63,7 +63,7 @@ export function PayPageEmbed({ | |||
| onPurchaseSuccess: (result) => { | |||
| if (!redirectUri) return; | |||
| const url = new URL(redirectUri); | |||
| switch (result.type) { | |||
| switch (result?.type) { | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against undefined before the switch to preserve discriminated-union narrowing
Using switch (result?.type) widens the expression to 'crypto' | 'fiat' | 'transaction' | undefined, preventing TypeScript from narrowing result inside each case. This forces result.status / result.transactionHash to be typed as any, defeating the benefit of the discriminated union and risking silent type regressions.
Proposed fix: short-circuit when result is falsy, then switch on the guaranteed, non-optional result.type.
- switch (result?.type) {
+ if (!result) {
+ return window.open(url.toString());
+ }
+ switch (result.type) {Optionally add a default branch to future-proof against new result types.
📝 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.
| switch (result?.type) { | |
| if (!result) { | |
| return window.open(url.toString()); | |
| } | |
| switch (result.type) { |
🤖 Prompt for AI Agents
In apps/dashboard/src/app/pay/components/client/PayPageEmbed.client.tsx at line
66, the switch statement uses optional chaining on result.type, causing
TypeScript to widen the type and lose discriminated union narrowing. To fix
this, add a guard clause before the switch to return early or handle the case
when result is undefined, ensuring that inside the switch statement result is
always defined. Then switch on result.type without optional chaining.
Optionally, add a default case in the switch to handle any future result types
safely.
cfc7c79 to
1b600b3
Compare
gregfromstl
force-pushed
the
greg/new-ub-components
branch
from
1b600b3 to
74717d7
Compare
PR-Codex overview
This PR introduces new components for payment processing and enhances existing functionalities, including the transition from
PayEmbedtoCheckoutWidgetandBuyWidget. It also improves error handling and adds utility functions for token management.Detailed summary
PayEmbedcomponent and replaced it withCheckoutWidgetandBuyWidget.TransactionWidgetfor executing transactions.ErrorBannerandUnsupportedTokenScreen.Summary by CodeRabbit
New Features
Enhancements
Documentation
Bug Fixes / Style
Tests