chore: add defensive code and better analytics for broadcast errors, … #5375
Conversation
WalkthroughThe recent updates focus on enhancing error tracking and refining prop handling across various components. Key changes include the removal of unnecessary prop spreading, conditional rendering of components, and the addition of analytics tracking for error events. The version of Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
pete-watters
force-pushed
the
fix/5118/broadcast-error
branch
from
997aa53 to
1fab7d8
Compare
| @@ -122,6 +122,7 @@ export function useCurrentAccountNativeSegwitAddressIndexZero() { | |||
| */ | |||
| export function useNativeSegwitAccountIndexAddressIndexZero(accountIndex: number) { | |||
| const signer = useNativeSegwitSigner(accountIndex)?.(0); | |||
| // could it be this? | |||
There was a problem hiding this comment.
Adding this note as the error thrown was Cannot read property of undefined reading .address" .
I see the deprecation warning above this function but I don't understand what it means.
/**
* @deprecated Use signer.address instead
*/
Does that mean I can refactor this to use signer?.address instead of signer?.payment.address?
| @@ -129,7 +130,15 @@ function LedgerSignStacksTxContainer() { | |||
| signedTx, | |||
| }); | |||
| } catch (e) { | |||
| ledgerNavigate.toBroadcastErrorStep(isError(e) ? e.message : 'Unknown error'); | |||
| const error = isError(e) ? e.message : 'Unknown error'; | |||
| void analytics.track('ledger_transaction_publish_error', { | |||
There was a problem hiding this comment.
I added analytics to errors that bring us to the broadcast error screen. I am unsure what we should use analytics for though.
- should only use them for events and leave errors to sentry?
- should I have access to segment?
| address: string; | ||
| } | ||
| export function FormAddressDisplayer({ address, ...rest }: FormAddressDisplayerProps) { | ||
| export function FormAddressDisplayer({ address }: FormAddressDisplayerProps) { |
There was a problem hiding this comment.
I refactored this on my travels as we don't need the ...rest and in case address here is undefined and the cause of the runtime error
|
@kyranjamie : I was still unable to reproduce this issue, see comment here Opening this as a draft as I have some Q's and in case adding this tracking and defensive code helps either identify / prevent this issue. |
| > | ||
| <AddressDisplayer address={address} /> | ||
| {address && <AddressDisplayer address={address} />} |
There was a problem hiding this comment.
Why would address be undefined if it isn't an optional prop?
If it is undefined sometimes, then our types are wrong, and this treats a symptom rather than the root cause of the problem
There was a problem hiding this comment.
It's more guesswork/ clutching at straws so I can roll it back?
I added this as sometimes we map and pass recipient.address.
I was searching for the cause of a possible "Cannot read property of undefined reading .address" error. It should throw a type error but I'm not sure if that will help if we are mapping through recipients with an undefined address.
There was a problem hiding this comment.
Would prefer to revert this change. This would really confuse me reading this code.
If investigating, suggesting put a analytics event here if undefined? Or, to do a runtime check on this, do it where the component is used vs inside it.
![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
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- src/app/components/address-displayer/form-address-displayer.tsx (2 hunks)
- src/app/features/ledger/flows/stacks-tx-signing/ledger-sign-stacks-tx-container.tsx (2 hunks)
- src/app/pages/send/ordinal-inscription/send-inscription-review.tsx (1 hunks)
- src/app/pages/send/send-crypto-asset-form/form/brc20/brc20-send-form-confirmation.tsx (1 hunks)
- src/app/pages/send/send-crypto-asset-form/form/btc/btc-send-form-confirmation.tsx (1 hunks)
- src/app/query/stacks/bns/bns.utils.ts (1 hunks)
- src/app/store/accounts/blockchain/bitcoin/native-segwit-account.hooks.ts (1 hunks)
- src/app/ui/components/address-displayer/address-displayer.tsx (1 hunks)
Files skipped from review due to trivial changes (1)
- src/app/store/accounts/blockchain/bitcoin/native-segwit-account.hooks.ts
Additional comments not posted (9)
src/app/ui/components/address-displayer/address-displayer.tsx (2)
6-7: Simplified theAddressDisplayerPropsinterface by removing unnecessary dependencies.
9-13: Refactored theAddressDisplayerfunction to directly use theaddressproperty without spreading other props.src/app/components/address-displayer/form-address-displayer.tsx (2)
6-7: Simplified theFormAddressDisplayerPropsinterface by removing unnecessary dependencies.
Line range hint
9-19: Refactored theFormAddressDisplayerfunction to directly use theaddressproperty without spreading other props and added a conditional rendering check for theaddressprop.src/app/pages/send/ordinal-inscription/send-inscription-review.tsx (1)
67-67: Added error tracking forbroadcast_ordinal_transaction_errorin theonErrorcallback.src/app/pages/send/send-crypto-asset-form/form/brc20/brc20-send-form-confirmation.tsx (1)
101-103: Added error tracking forbroadcast_brc20_transaction_errorin theonErrorcallback.src/app/query/stacks/bns/bns.utils.ts (1)
143-143: Good defensive coding practice by checking ifres.addressis undefined before proceeding.src/app/features/ledger/flows/stacks-tx-signing/ledger-sign-stacks-tx-container.tsx (1)
134-139: Adding analytics tracking forledger_transaction_publish_erroris a good practice for monitoring and diagnosing issues.src/app/pages/send/send-crypto-asset-form/form/btc/btc-send-form-confirmation.tsx (1)
141-143: Adding analytics tracking forbtc_transaction_broadcast_erroris a good practice for monitoring and diagnosing issues.
|
@pete-watters mind adding details about the new analytics (e.g. new events?) to the description? 🙏 |
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!pnpm-lock.yaml
Files selected for processing (4)
- package.json (1 hunks)
- src/app/pages/send/ordinal-inscription/send-inscription-review.tsx (1 hunks)
- src/app/pages/send/send-crypto-asset-form/form/brc20/brc20-send-form-confirmation.tsx (1 hunks)
- src/app/pages/send/send-crypto-asset-form/form/btc/btc-send-form-confirmation.tsx (1 hunks)
Files skipped from review due to trivial changes (1)
- package.json
Files skipped from review as they are similar to previous changes (3)
- src/app/pages/send/ordinal-inscription/send-inscription-review.tsx
- src/app/pages/send/send-crypto-asset-form/form/brc20/brc20-send-form-confirmation.tsx
- src/app/pages/send/send-crypto-asset-form/form/btc/btc-send-form-confirmation.tsx
pete-watters
force-pushed
the
fix/5118/broadcast-error
branch
from
82ce4a1 to
5446e92
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!pnpm-lock.yaml
Files selected for processing (9)
- package.json (1 hunks)
- src/app/components/address-displayer/form-address-displayer.tsx (2 hunks)
- src/app/features/ledger/flows/stacks-tx-signing/ledger-sign-stacks-tx-container.tsx (2 hunks)
- src/app/pages/send/ordinal-inscription/send-inscription-review.tsx (1 hunks)
- src/app/pages/send/send-crypto-asset-form/form/brc20/brc20-send-form-confirmation.tsx (1 hunks)
- src/app/pages/send/send-crypto-asset-form/form/btc/btc-send-form-confirmation.tsx (1 hunks)
- src/app/query/stacks/bns/bns.utils.ts (1 hunks)
- src/app/store/accounts/blockchain/bitcoin/native-segwit-account.hooks.ts (1 hunks)
- src/app/ui/components/address-displayer/address-displayer.tsx (1 hunks)
Files skipped from review due to trivial changes (1)
- src/app/features/ledger/flows/stacks-tx-signing/ledger-sign-stacks-tx-container.tsx
Files skipped from review as they are similar to previous changes (8)
- package.json
- src/app/components/address-displayer/form-address-displayer.tsx
- src/app/pages/send/ordinal-inscription/send-inscription-review.tsx
- src/app/pages/send/send-crypto-asset-form/form/brc20/brc20-send-form-confirmation.tsx
- src/app/pages/send/send-crypto-asset-form/form/btc/btc-send-form-confirmation.tsx
- src/app/query/stacks/bns/bns.utils.ts
- src/app/store/accounts/blockchain/bitcoin/native-segwit-account.hooks.ts
- src/app/ui/components/address-displayer/address-displayer.tsx
| > | ||
| <AddressDisplayer address={address} /> | ||
| {address && <AddressDisplayer address={address} />} |
There was a problem hiding this comment.
Would prefer to revert this change. This would really confuse me reading this code.
If investigating, suggesting put a analytics event here if undefined? Or, to do a runtime check on this, do it where the component is used vs inside it.
pete-watters
force-pushed
the
fix/5118/broadcast-error
branch
from
5446e92 to
bf36652
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!pnpm-lock.yaml
Files selected for processing (7)
- package.json (1 hunks)
- src/app/features/ledger/flows/stacks-tx-signing/ledger-sign-stacks-tx-container.tsx (2 hunks)
- src/app/pages/send/ordinal-inscription/send-inscription-review.tsx (1 hunks)
- src/app/pages/send/send-crypto-asset-form/form/brc20/brc20-send-form-confirmation.tsx (1 hunks)
- src/app/pages/send/send-crypto-asset-form/form/btc/btc-send-form-confirmation.tsx (1 hunks)
- src/app/query/stacks/bns/bns.utils.ts (1 hunks)
- src/app/store/accounts/blockchain/bitcoin/native-segwit-account.hooks.ts (1 hunks)
Files skipped from review as they are similar to previous changes (6)
- package.json
- src/app/features/ledger/flows/stacks-tx-signing/ledger-sign-stacks-tx-container.tsx
- src/app/pages/send/ordinal-inscription/send-inscription-review.tsx
- src/app/pages/send/send-crypto-asset-form/form/brc20/brc20-send-form-confirmation.tsx
- src/app/pages/send/send-crypto-asset-form/form/btc/btc-send-form-confirmation.tsx
- src/app/store/accounts/blockchain/bitcoin/native-segwit-account.hooks.ts
Additional comments not posted (1)
src/app/query/stacks/bns/bns.utils.ts (1)
143-143: Added null checks forres.addressenhance robustness by preventing potential runtime errors when the address is undefined or an empty string.
This PR adds some extra analytics and defensive code to try and gather more insight into and prevent #5118
The analytics added are:
ledger_transaction_publish_errorbroadcast_ordinal_errorbroadcast_brc20_errorbroadcast_btc_errorSummary by CodeRabbit
New Features
Bug Fixes
Chores
@leather-wallet/modelsdependency version from 0.4.4 to 0.4.5 for better compatibility and performance.Documentation
useNativeSegwitAccountIndexAddressIndexZerofunction for clarity on potential scenarios.