Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
fairlighteth
changed the title
Contributor
|
Hey @fairlighteth , all is mostly great! |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughNew network switching system with Coinbase-specific mobile guidance, in-flight state tracking, and connection flow analytics. Includes new hooks for network switching with guidance snackbars and target chain confirmation, integrated into wallet activation and trade state sync flows. Changes
Sequence DiagramsequenceDiagram
actor User
participant Modal as Network Selector
participant Hook as useSwitchNetworkWithGuidance
participant Snackbar as Guidance Snackbar
participant Wallet as Wallet Provider
participant Chain as Chain Monitor
participant Analytics as Coinbase Analytics
User->>Modal: Select Network
Modal->>Hook: switchNetworkWithGuidance(targetChain)
Hook->>Analytics: sendCoinbaseConnectionFlowEvent(switchStart)
alt Coinbase Mobile
Hook->>Snackbar: Show guidance message
end
Hook->>Wallet: switchNetwork(targetChain)
par Parallel Operations
Wallet-->>Hook: Switch initiated
Hook->>Chain: waitForTargetChain(targetChain)
and
Wallet->>Chain: Update chain
Chain-->>Hook: Chain changed, resolve wait
end
Hook->>Analytics: sendCoinbaseConnectionFlowEvent(switchSuccess)
alt Coinbase Mobile
Hook->>Snackbar: Remove guidance snackbar
end
Hook-->>Modal: Promise resolves
rect rgba(255, 0, 0, 0.5)
alt Switch In Progress (concurrent call)
Hook->>Analytics: sendCoinbaseConnectionFlowEvent(switchBlockedInFlight)
Hook-->>Modal: Throw SwitchInProgressError
end
end
rect rgba(255, 165, 0, 0.5)
alt Timeout or Error
Hook->>Analytics: sendCoinbaseConnectionFlowEvent(switchError)
Hook->>Snackbar: Remove guidance snackbar
Hook-->>Modal: Throw error
end
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes The pull request introduces multiple interconnected features with significant logic density: a new network switching hook with timeout handling, in-flight promise tracking, mobile-specific UI guidance, and analytics integration; plus updates to wallet activation context and trade state setup. While test coverage is comprehensive, the heterogeneous changes across different modules and the stateful complexity of concurrent switch prevention require careful review of both happy and error paths. Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
…e-flow-telemetry # Conflicts: # apps/cowswap-frontend/src/modules/wallet/containers/WalletModal/index.tsx
Contributor
Author
|
@elena-zh Fixed both issues:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/cowswap-frontend/src/modules/trade/hooks/setupTradeState/useSetupTradeState.ts (1)
67-81:⚠️ Potential issue | 🟠 MajorPreserve
rememberedUrlStateRefwhen the switch is only blocked.
SwitchInProgressErrormeans another Coinbase switch is still running, not that this flow has completed. Clearing the ref at Lines 80-81 can drop the pending URL state beforeonProviderNetworkChanges()gets a chance to replay it, so the user can fall back togetDefaultTradeRawState()when the original switch eventually succeeds.Only clear
rememberedUrlStateRefafter an actual completion/failure of this switch attempt; a blocked-in-flight branch should leave the remembered URL state intact.As per coding guidelines, "Cancel or skip follow-up work if a previous request is still running".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cowswap-frontend/src/modules/trade/hooks/setupTradeState/useSetupTradeState.ts` around lines 67 - 81, The code currently clears rememberedUrlStateRef.current unconditionally at the end of the switch attempt, which drops the pending URL state when the error is a SwitchInProgressError; change the logic so rememberedUrlStateRef.current is only cleared when this switch attempt actually completed or failed (i.e., not when error instanceof SwitchInProgressError). Concretely, update the block around the SwitchInProgressError check in useSetupTradeState so that if the caught error is a SwitchInProgressError you skip clearing rememberedUrlStateRef.current (leaving it for onProviderNetworkChanges to replay), but on other errors or on successful completion you still clear it; keep tradeNavigate(currentProviderChainId, getDefaultTradeRawState(...)) behavior for non-SwitchInProgressError failures.
🧹 Nitpick comments (3)
apps/cowswap-frontend/src/common/hooks/useWaitForTargetChain.ts (1)
47-51: Return object should be memoized withuseMemo.Per coding guidelines, "Hooks returning objects must use
useMemo" to maintain referential stability for consumers.♻️ Proposed fix to memoize the return object
+import { useCallback, useEffect, useMemo, useRef } from 'react' -import { useCallback, useEffect, useRef } from 'react'- return { - clearPendingChainConfirmation, - waitForTargetChain, - } + return useMemo( + () => ({ + clearPendingChainConfirmation, + waitForTargetChain, + }), + [clearPendingChainConfirmation, waitForTargetChain], + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cowswap-frontend/src/common/hooks/useWaitForTargetChain.ts` around lines 47 - 51, The hook useWaitForTargetChain currently returns a plain object { clearPendingChainConfirmation, waitForTargetChain } which breaks referential stability for callers; wrap that return object in React.useMemo so the same object identity is preserved unless clearPendingChainConfirmation or waitForTargetChain change (i.e., return useMemo(() => ({ clearPendingChainConfirmation, waitForTargetChain }), [clearPendingChainConfirmation, waitForTargetChain])).apps/cowswap-frontend/src/modules/advancedOrders/containers/AdvancedOrdersWidget/index.tsx (1)
66-68: Good cleanup of ESLint directive, but scaffolding remains.Removing
max-lines-per-functionfrom the disabled rules is a positive step. However, the TODO comments and remainingeslint-disablefor@typescript-eslint/explicit-function-return-typestill constitute linter scaffolding that should be addressed before shipping, per coding guidelines.Consider tracking these TODOs as follow-up work to fully comply with the guideline to "Remove linter scaffolding (
// TODO,eslint-disable) by supplying the correct types before shipping."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cowswap-frontend/src/modules/advancedOrders/containers/AdvancedOrdersWidget/index.tsx` around lines 66 - 68, The file contains linter-scaffolding TODOs and an eslint-disable for `@typescript-eslint/explicit-function-return-type` around the large function that implements the AdvancedOrdersWidget component; remove the eslint-disable and TODO comments by giving the component an explicit return type (e.g., annotate the AdvancedOrdersWidget function signature with React.FC<Props> or : JSX.Element and add proper types for its props/state) and refactor long logical blocks inside the AdvancedOrdersWidget function into smaller helper functions (extract helpers with clear names and types) so the original function is shorter; after extracting helpers and typing everything, delete the remaining TODOs and the eslint-disable line.apps/cowswap-frontend/src/common/hooks/useSwitchNetworkWithGuidance.tsx (1)
18-61: Split the switch runner/telemetry helpers out of this hook file.This new file already crosses the repo's TSX size cap and now mixes the public hook, module-scoped coordination state, snackbar UI, telemetry emission, and a test-only reset export. Pulling the runner/guard/telemetry pieces into sibling utilities would keep the hook focused and avoid growing another high-friction file.
As per coding guidelines, "Keep TypeScript/TSX sources around 200 LOC; anything over 200 needs active justification, and non-generated files must stay <= 250 LOC".
Also applies to: 69-270
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cowswap-frontend/src/common/hooks/useSwitchNetworkWithGuidance.tsx` around lines 18 - 61, The hook file mixes module-scoped coordination (inFlightPromise), runner/guard logic and telemetry types — extract those into a sibling utility module: move inFlightPromise, the types SwitchNetworkWithGuidanceMeta, SwitchNetworkTelemetryEvent, SwitchExecutionParams, SwitchRequestParams, SwitchPromiseParams, the runner/guard functions that call waitForTargetChain/switchNetwork and the test-only _resetInFlightState export into a new file (e.g., switchNetworkRunner.ts), export the necessary functions and types, and update useSwitchNetworkWithGuidance to import them; ensure the new module exposes the in-flight coordination API used by the hook and tests (including _resetInFlightState) and keep all UI/snackbar and hook-specific logic inside the original hook file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/cowswap-frontend/src/common/analytics/coinbaseConnectionFlow.ts`:
- Around line 44-59: The getErrorDetails function currently only handles Error
and string types and should also normalize object-shaped provider errors (e.g.,
{ code, message } or { name, message }) so errorName and errorMessage are
populated; update getErrorDetails to detect plain objects with a message
property and set errorMessage to that message and errorName to the name or code
(falling back to 'Error' if neither exists). Also add a unit test in
coinbaseConnectionFlow.test.ts covering a provider-style error object like {
code: 'SOME_CODE', message: 'details' } to assert both errorName (from code) and
errorMessage are emitted. Ensure you reference the existing function
getErrorDetails and the test file coinbaseConnectionFlow.test.ts when making the
changes.
In `@apps/cowswap-frontend/src/common/hooks/useWaitForTargetChain.ts`:
- Around line 34-45: waitForTargetChain can overwrite an existing pending
promise leaving the previous caller hanging; before assigning
pendingChainConfirmationRef.current you should check if a pending entry exists
and reject it (e.g., call its.reject(new Error('waitForTargetChain
superseded'))) then create the new pending object that stores both resolve and
reject handlers, or alternatively return the existing pending promise when
targetChain matches the same pending target; update the
pendingChainConfirmationRef usage and the promise constructor in
waitForTargetChain so the pending object includes a reject function and the
previous pending is rejected before overwriting.
In
`@apps/cowswap-frontend/src/modules/twap/containers/TwapConfirmModal/index.tsx`:
- Around line 73-76: The exported component TwapConfirmModal currently
suppresses the explicit return-type rule and leaves TODO scaffolding; remove the
eslint-disable and TODO comments and add an explicit return type to the function
signature (for example "export function TwapConfirmModal(): JSX.Element" or "():
React.ReactElement"), ensuring React is imported if needed; keep the
implementation unchanged but replace the scaffolding with the proper typed
signature so linting passes.
---
Outside diff comments:
In
`@apps/cowswap-frontend/src/modules/trade/hooks/setupTradeState/useSetupTradeState.ts`:
- Around line 67-81: The code currently clears rememberedUrlStateRef.current
unconditionally at the end of the switch attempt, which drops the pending URL
state when the error is a SwitchInProgressError; change the logic so
rememberedUrlStateRef.current is only cleared when this switch attempt actually
completed or failed (i.e., not when error instanceof SwitchInProgressError).
Concretely, update the block around the SwitchInProgressError check in
useSetupTradeState so that if the caught error is a SwitchInProgressError you
skip clearing rememberedUrlStateRef.current (leaving it for
onProviderNetworkChanges to replay), but on other errors or on successful
completion you still clear it; keep tradeNavigate(currentProviderChainId,
getDefaultTradeRawState(...)) behavior for non-SwitchInProgressError failures.
---
Nitpick comments:
In `@apps/cowswap-frontend/src/common/hooks/useSwitchNetworkWithGuidance.tsx`:
- Around line 18-61: The hook file mixes module-scoped coordination
(inFlightPromise), runner/guard logic and telemetry types — extract those into a
sibling utility module: move inFlightPromise, the types
SwitchNetworkWithGuidanceMeta, SwitchNetworkTelemetryEvent,
SwitchExecutionParams, SwitchRequestParams, SwitchPromiseParams, the
runner/guard functions that call waitForTargetChain/switchNetwork and the
test-only _resetInFlightState export into a new file (e.g.,
switchNetworkRunner.ts), export the necessary functions and types, and update
useSwitchNetworkWithGuidance to import them; ensure the new module exposes the
in-flight coordination API used by the hook and tests (including
_resetInFlightState) and keep all UI/snackbar and hook-specific logic inside the
original hook file.
In `@apps/cowswap-frontend/src/common/hooks/useWaitForTargetChain.ts`:
- Around line 47-51: The hook useWaitForTargetChain currently returns a plain
object { clearPendingChainConfirmation, waitForTargetChain } which breaks
referential stability for callers; wrap that return object in React.useMemo so
the same object identity is preserved unless clearPendingChainConfirmation or
waitForTargetChain change (i.e., return useMemo(() => ({
clearPendingChainConfirmation, waitForTargetChain }),
[clearPendingChainConfirmation, waitForTargetChain])).
In
`@apps/cowswap-frontend/src/modules/advancedOrders/containers/AdvancedOrdersWidget/index.tsx`:
- Around line 66-68: The file contains linter-scaffolding TODOs and an
eslint-disable for `@typescript-eslint/explicit-function-return-type` around the
large function that implements the AdvancedOrdersWidget component; remove the
eslint-disable and TODO comments by giving the component an explicit return type
(e.g., annotate the AdvancedOrdersWidget function signature with React.FC<Props>
or : JSX.Element and add proper types for its props/state) and refactor long
logical blocks inside the AdvancedOrdersWidget function into smaller helper
functions (extract helpers with clear names and types) so the original function
is shorter; after extracting helpers and typing everything, delete the remaining
TODOs and the eslint-disable line.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5f52c543-03d2-4033-8c69-bdabb9bd9506
📒 Files selected for processing (23)
apps/cowswap-frontend/src/common/analytics/coinbaseConnectionFlow.test.tsapps/cowswap-frontend/src/common/analytics/coinbaseConnectionFlow.tsapps/cowswap-frontend/src/common/hooks/useOnSelectNetwork.test.tsxapps/cowswap-frontend/src/common/hooks/useOnSelectNetwork.tsxapps/cowswap-frontend/src/common/hooks/useSwitchNetworkWithGuidance.test.tsxapps/cowswap-frontend/src/common/hooks/useSwitchNetworkWithGuidance.tsxapps/cowswap-frontend/src/common/hooks/useWaitForTargetChain.tsapps/cowswap-frontend/src/common/pure/NetworksList/ActiveRowLinks/ActiveRowLinks.styled.tsapps/cowswap-frontend/src/modules/advancedOrders/containers/AdvancedOrdersWidget/index.tsxapps/cowswap-frontend/src/modules/erc20Approve/pure/ApproveConfirmation/AdvancedApprove.tsxapps/cowswap-frontend/src/modules/hooksStore/containers/HooksStoreWidget/index.tsxapps/cowswap-frontend/src/modules/hooksStore/pure/AppliedHookItem/index.tsxapps/cowswap-frontend/src/modules/limitOrders/containers/LimitOrdersWarnings/index.tsxapps/cowswap-frontend/src/modules/limitOrders/containers/RateInput/styled.tsapps/cowswap-frontend/src/modules/ordersTable/pure/OrderFillsAtWithDistance/OrderFillsAtWithDistance.pure.tsxapps/cowswap-frontend/src/modules/ordersTable/pure/OrdersTable/Row/Group/OrdersTableRowGroup.pure.tsxapps/cowswap-frontend/src/modules/swap/containers/SwapWidget/index.tsxapps/cowswap-frontend/src/modules/trade/hooks/setupTradeState/useSetupTradeState.test.tsapps/cowswap-frontend/src/modules/trade/hooks/setupTradeState/useSetupTradeState.tsapps/cowswap-frontend/src/modules/twap/containers/TwapConfirmModal/index.tsxapps/cowswap-frontend/src/modules/wallet/containers/WalletModal/index.tsxapps/explorer/src/components/orders/DetailsTable/VerboseDetails.tsxlibs/wallet/src/web3-react/hooks/useActivateConnector.ts
💤 Files with no reviewable changes (3)
- apps/cowswap-frontend/src/modules/erc20Approve/pure/ApproveConfirmation/AdvancedApprove.tsx
- apps/cowswap-frontend/src/modules/hooksStore/containers/HooksStoreWidget/index.tsx
- apps/explorer/src/components/orders/DetailsTable/VerboseDetails.tsx
…cs in coinbaseConnectionFlow
elena-zh
approved these changes
Apr 7, 2026
Contributor
elena-zh
left a comment
elena-zh
left a comment
There was a problem hiding this comment.
Looks good now, thank you
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/cowswap-frontend/src/modules/trade/hooks/setupTradeState/useSetupTradeState.ts (1)
62-78: Consider guardingerror.nameaccess for type safety.The catch block accesses
error.namewithout first verifying thaterroris an object with anameproperty. While rare, non-Error objects can be thrown.🛡️ Optional defensive check
} catch (error) { // We are ignoring Gnosis safe context error // Because it's a normal situation when we are not in Gnosis safe App - if (error.name === 'NoSafeContext') return + if (error instanceof Error && error.name === 'NoSafeContext') return // Benign in-flight retries can happen while the original switch is still settling. if (error instanceof SwitchInProgressError) return - console.error('Network switching error: ', error) + console.error('Network switching error:', error)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cowswap-frontend/src/modules/trade/hooks/setupTradeState/useSetupTradeState.ts` around lines 62 - 78, In the catch block inside useSetupTradeState (useSetupTradeState.ts) guard access to error.name by first ensuring error is an object and not null and that it has a name property (e.g., typeof error === 'object' && error !== null && 'name' in error) before comparing to 'NoSafeContext'; apply the same defensive check before treating error as a SwitchInProgressError or logging it so you avoid runtime exceptions when non-Error values are thrown.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/cowswap-frontend/src/modules/trade/hooks/setupTradeState/useSetupTradeState.ts`:
- Around line 62-78: In the catch block inside useSetupTradeState
(useSetupTradeState.ts) guard access to error.name by first ensuring error is an
object and not null and that it has a name property (e.g., typeof error ===
'object' && error !== null && 'name' in error) before comparing to
'NoSafeContext'; apply the same defensive check before treating error as a
SwitchInProgressError or logging it so you avoid runtime exceptions when
non-Error values are thrown.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a8f7824f-e1b1-4903-ba02-9ccbe35706f9
📒 Files selected for processing (4)
apps/cowswap-frontend/src/common/analytics/coinbaseConnectionFlow.test.tsapps/cowswap-frontend/src/common/analytics/coinbaseConnectionFlow.tsapps/cowswap-frontend/src/modules/trade/hooks/setupTradeState/useSetupTradeState.test.tsapps/cowswap-frontend/src/modules/trade/hooks/setupTradeState/useSetupTradeState.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/cowswap-frontend/src/common/analytics/coinbaseConnectionFlow.ts
Collaborator
|
This fix probably is not relevant due to migration to Reown |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds stage-level Coinbase connection telemetry so we can measure where users fail in connect/switch flows (instead of inferring from anecdotal reports).
It introduces a dedicated analytics event and wires it through key Coinbase interaction points.
Included changes:
Files touched include:
To Test
useSetupTradeState.test.ts
Background
We have multiple Coinbase entry points and asynchronous handoffs (modal connect, app switch, network switch, URL-sync path).
Without structured stage telemetry, failures look similar in user reports but have different root causes.
This PR provides the instrumentation needed to:
Summary by CodeRabbit
Release Notes
New Features
Improvements
Tests