fix: Ledger - cancel properly and don't allow user to close non cancellable sections #5353
Conversation
WalkthroughThe recent updates primarily focus on enhancing the user experience during Ledger interactions by introducing a cancellable action mechanism. This involves importing and utilizing the 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 (
|
|
@coderabbitai review |
Actions PerformedReview triggered.
|
pete-watters
changed the title
| @@ -8,11 +8,10 @@ import { ArrowLeftIcon, CloseIcon } from '@app/ui/icons'; | |||
| import { BigTitleHeader } from './components/big-title-header'; | |||
| import { HeaderActionButton } from './components/header-action-button'; | |||
|
|
|||
| type HeaderVariants = 'page' | 'home' | 'onboarding' | 'dialog' | 'bigTitle' | 'fund'; | |||
| type HeaderVariants = 'page' | 'home' | 'onboarding' | 'bigTitle' | 'fund'; | |||
There was a problem hiding this comment.
I just made a new header for Dialog - DialogHeader to remove this extra variant here and isWaitingOnPerformedAction
| } | ||
| interface RadixDialogProps extends DialogProps { | ||
| children: ReactNode; | ||
| footer?: ReactNode; | ||
| header: ReactElement<any, string | JSXElementConstructor<any>>; | ||
| header?: ReactElement<any, string | JSXElementConstructor<any>>; |
There was a problem hiding this comment.
I tried changing the type of this to ReactNode but I need these types as I am using cloneElement to clone the header passed to the Dialog and pass on the onClose action
| const { chain } = useCurrentNetwork(); | ||
|
|
||
| const [isLoading, setIsLoading] = useState(false); |
There was a problem hiding this comment.
I added this code as if users:
- start signing this message
- hit
Xto cancel and go back - when they go back to the correct place now
signBip322MessageIsLoadingstays stuck as loading so they cannot retry
![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
Out of diff range and nitpick comments (4)
src/app/ui/components/containers/headers/components/header-action-button.tsx (1)
7-7: EnsureonActionis properly documented in the interface.Adding a brief comment describing the purpose and usage of
onActionwould improve code readability and maintainability.src/app/features/ledger/flows/stacks-message-signing/ledger-stacks-sign-msg-container.tsx (3)
Line range hint
7-7: Remove unused importgetfrom 'lodash.get'.- import get from 'lodash.get';
Line range hint
53-53: The variablenavigateis declared but never used. Consider removing it if it's not needed.- const navigate = useNavigate();
Line range hint
55-55: The variablelocationis declared but never used. Consider removing it if it's not needed.- const location = useLocation();
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (35)
- src/app/components/broadcast-error-dialog/broadcast-error-dialog.tsx (2 hunks)
- src/app/features/dialogs/edit-nonce-dialog/edit-nonce-dialog.tsx (2 hunks)
- src/app/features/dialogs/high-fee-dialog/high-fee-dialog.tsx (2 hunks)
- src/app/features/dialogs/increase-fee-dialog/increase-btc-fee-dialog.tsx (2 hunks)
- src/app/features/dialogs/increase-fee-dialog/increase-fee-sent-dialog.tsx (2 hunks)
- src/app/features/dialogs/increase-fee-dialog/increase-stx-fee-dialog.tsx (2 hunks)
- src/app/features/dialogs/switch-account-dialog/components/account-list-unavailable.tsx (1 hunks)
- src/app/features/dialogs/switch-account-dialog/switch-account-dialog.tsx (3 hunks)
- src/app/features/ledger/flows/bitcoin-tx-signing/ledger-bitcoin-sign-tx-container.tsx (3 hunks)
- src/app/features/ledger/flows/jwt-signing/ledger-sign-jwt-container.tsx (2 hunks)
- src/app/features/ledger/flows/request-bitcoin-keys/ledger-request-bitcoin-keys.tsx (1 hunks)
- src/app/features/ledger/flows/request-stacks-keys/ledger-request-stacks-keys.tsx (1 hunks)
- src/app/features/ledger/flows/stacks-message-signing/ledger-stacks-sign-msg-container.tsx (2 hunks)
- src/app/features/ledger/flows/stacks-tx-signing/ledger-sign-stacks-tx-container.tsx (3 hunks)
- src/app/features/ledger/generic-flows/request-keys/request-keys-flow.tsx (1 hunks)
- src/app/features/ledger/generic-flows/tx-signing/tx-signing-flow.tsx (1 hunks)
- src/app/features/ledger/generic-steps/connect-device/connect-ledger-error.layout.tsx (1 hunks)
- src/app/features/ledger/generic-steps/connect-device/connect-ledger-start.tsx (2 hunks)
- src/app/features/ledger/generic-steps/unsupported-browser/unsupported-browser.layout.tsx (2 hunks)
- src/app/features/ledger/hooks/use-ledger-navigate.ts (1 hunks)
- src/app/features/ledger/utils/stacks-ledger-utils.ts (1 hunks)
- src/app/features/retrieve-taproot-to-native-segwit/components/retrieve-taproot-to-native-segwit.layout.tsx (2 hunks)
- src/app/features/settings/network/network.tsx (2 hunks)
- src/app/features/settings/sign-out/sign-out.tsx (2 hunks)
- src/app/features/settings/theme/theme-dialog.tsx (2 hunks)
- src/app/pages/rpc-sign-bip322-message/rpc-sign-bip322-message.tsx (2 hunks)
- src/app/pages/send/ordinal-inscription/send-inscription-choose-fee.tsx (2 hunks)
- src/app/pages/send/ordinal-inscription/send-inscription-form.tsx (2 hunks)
- src/app/pages/send/ordinal-inscription/send-inscription-review.tsx (2 hunks)
- src/app/pages/send/ordinal-inscription/sent-inscription-summary.tsx (2 hunks)
- src/app/pages/send/send-crypto-asset-form/components/recipient-accounts-dialog/recipient-accounts-dialog.tsx (2 hunks)
- src/app/ui/components/containers/dialog/dialog.tsx (1 hunks)
- src/app/ui/components/containers/headers/components/header-action-button.tsx (1 hunks)
- src/app/ui/components/containers/headers/dialog-header.tsx (1 hunks)
- src/app/ui/components/containers/headers/header.tsx (4 hunks)
Files skipped from review due to trivial changes (9)
- src/app/features/dialogs/high-fee-dialog/high-fee-dialog.tsx
- src/app/features/dialogs/increase-fee-dialog/increase-btc-fee-dialog.tsx
- src/app/features/dialogs/increase-fee-dialog/increase-fee-sent-dialog.tsx
- src/app/features/dialogs/switch-account-dialog/components/account-list-unavailable.tsx
- src/app/features/ledger/flows/jwt-signing/ledger-sign-jwt-container.tsx
- src/app/features/settings/theme/theme-dialog.tsx
- src/app/pages/send/ordinal-inscription/send-inscription-choose-fee.tsx
- src/app/pages/send/send-crypto-asset-form/components/recipient-accounts-dialog/recipient-accounts-dialog.tsx
- src/app/ui/components/containers/dialog/dialog.tsx
Additional Context Used
GitHub Check Runs (1)
typecheck failure (5)
src/app/features/ledger/flows/stacks-message-signing/ledger-stacks-sign-msg-container.tsx: [failure] 7-7:
'get' is declared but its value is never read.
src/app/features/ledger/flows/stacks-message-signing/ledger-stacks-sign-msg-container.tsx: [failure] 53-53:
'navigate' is declared but its value is never read.
src/app/features/ledger/flows/stacks-message-signing/ledger-stacks-sign-msg-container.tsx: [failure] 55-55:
'location' is declared but its value is never read.
Additional comments not posted (39)
src/app/ui/components/containers/headers/components/header-action-button.tsx (1)
15-16: Good use of hover styles to enhance user interaction.src/app/features/ledger/generic-flows/tx-signing/tx-signing-flow.tsx (2)
5-5: Correct update of the import path forDialogHeader.
11-17: EnsurecloseActionis properly handled when undefined.src/app/features/ledger/generic-flows/request-keys/request-keys-flow.tsx (2)
5-5: Correct update of the import path forDialogHeader.
14-25: Proper handling of action cancellability based onisActionCancellableByUser.src/app/ui/components/containers/headers/dialog-header.tsx (2)
11-12: Proper addition of optional props to enhance component flexibility.
15-40: Effective use of conditional rendering fortitleandonCloseprops inDialogHeader.src/app/features/ledger/generic-steps/unsupported-browser/unsupported-browser.layout.tsx (1)
7-7: Correct update of the import path forDialogHeader.src/app/components/broadcast-error-dialog/broadcast-error-dialog.tsx (1)
10-10: Correct update of the import path forDialogHeader.src/app/features/ledger/generic-steps/connect-device/connect-ledger-start.tsx (1)
9-9: Correct update of the import path forDialogHeader.src/app/features/dialogs/edit-nonce-dialog/edit-nonce-dialog.tsx (2)
12-12: Correct update of the import path forDialogHeader.
51-51: Effective use of thetitleprop to provide context in theDialogHeader.src/app/features/retrieve-taproot-to-native-segwit/components/retrieve-taproot-to-native-segwit.layout.tsx (2)
8-8: Updated import to useDialogHeaderaligns with the project-wide renaming effort.
24-24: Correct usage ofDialogHeaderin theDialogcomponent. This change ensures consistency in dialog headers across the application.src/app/features/ledger/generic-steps/connect-device/connect-ledger-error.layout.tsx (1)
41-43: Responsive design adjustments toLedgerTitleare appropriate for improving user experience across different devices.src/app/features/settings/network/network.tsx (2)
15-15: Updated import to useDialogHeaderaligns with the project-wide renaming effort.
46-46: Proper use ofDialogHeaderwith a title in theDialogcomponent enhances clarity and consistency in UI.src/app/pages/rpc-sign-bip322-message/rpc-sign-bip322-message.tsx (2)
1-2: ImportinguseStateanduseEffectis essential for managing component state and side effects, which is seen in the use of these hooks further down in the file.
41-50: The logic to manage the loading state based on navigation changes is sound. It ensures that the loading indicator is reset when the user navigates back, allowing them to retry the operation.src/app/features/ledger/flows/request-bitcoin-keys/ledger-request-bitcoin-keys.tsx (1)
70-70: Adjusting the condition for action cancellability based on the connection status is a prudent enhancement. It prevents user actions during critical operations, enhancing the robustness of the flow.src/app/features/ledger/flows/request-stacks-keys/ledger-request-stacks-keys.tsx (1)
82-82: Consistent with the previous file, adjusting the condition for action cancellability based on the connection status is a good practice. It ensures that user actions are blocked during sensitive operations.src/app/features/dialogs/switch-account-dialog/switch-account-dialog.tsx (2)
12-14: Updated import to useDialogHeaderaligns with the project-wide renaming effort.
56-56: Proper use ofDialogHeaderwith a title in theDialogcomponent enhances clarity and consistency in UI, especially in a critical feature like account switching.src/app/pages/send/ordinal-inscription/send-inscription-form.tsx (2)
15-15: Updated import statement to useDialogHeaderreflects the renaming of the component for consistency across the application.
45-45: Usage ofDialogHeaderin theDialogcomponent aligns with the new component structure and enhances readability and maintainability of the dialog-related code.src/app/features/settings/sign-out/sign-out.tsx (2)
10-10: Updated import statement to useDialogHeaderreflects the renaming of the component for consistency across the application.
31-31: Usage ofDialogHeaderin theDialogcomponent aligns with the new component structure and enhances readability and maintainability of the dialog-related code.src/app/pages/send/ordinal-inscription/sent-inscription-summary.tsx (2)
19-19: Updated import statement to useDialogHeaderreflects the renaming of the component for consistency across the application.
63-63: Usage ofDialogHeaderin theDialogcomponent aligns with the new component structure and enhances readability and maintainability of the dialog-related code.src/app/features/ledger/utils/stacks-ledger-utils.ts (1)
91-93: The addition ofuseActionCancellableByUserfunction enhances the application's ability to determine if an action can be cancelled based on the current route, improving user experience by providing appropriate UI feedback and options.src/app/pages/send/ordinal-inscription/send-inscription-review.tsx (2)
19-19: Updated import statement to useDialogHeaderreflects the renaming of the component for consistency across the application.
79-79: Usage ofDialogHeaderin theDialogcomponent aligns with the new component structure and enhances readability and maintainability of the dialog-related code.src/app/features/ledger/hooks/use-ledger-navigate.ts (1)
144-150: Modification of thecancelLedgerActionfunction to usebaseUrlfor determining the navigation destination enhances the predictability and robustness of the navigation logic, aligning with the PR's objectives to fix navigation issues.src/app/features/ledger/flows/bitcoin-tx-signing/ledger-bitcoin-sign-tx-container.tsx (1)
29-29: The use ofuseActionCancellableByUserin the Bitcoin transaction signing flow allows the application to dynamically determine if the current action can be cancelled, enhancing user experience by providing appropriate options based on the context.src/app/features/ledger/flows/stacks-tx-signing/ledger-sign-stacks-tx-container.tsx (1)
21-21: The use ofuseActionCancellableByUserin the Stacks transaction signing flow allows the application to dynamically determine if the current action can be cancelled, enhancing user experience by providing appropriate options based on the context.src/app/features/ledger/flows/stacks-message-signing/ledger-stacks-sign-msg-container.tsx (2)
24-24: Updated import to useDialogHeaderinstead ofHeader. Ensure all references to the oldHeadercomponent are updated across the project.
156-156: TheonCloselogic now conditionally allows the dialog to be closed based oncanClose. Ensure this change aligns with the intended user flow, especially in non-cancellable ledger actions.Verification successful
The search results from the script provide a comprehensive overview of how the
onCloseproperty is used across various ledger-related components in the application. The specific line from the fileledger-stacks-sign-msg-container.tsxshows that theonClosecallback is conditionally set based on thecanClosevariable, which implies that the dialog's ability to close is contingent on certain conditions being met.From the output, it's evident that similar conditional logic for
onCloseis also used in other ledger-related components, such as:
ledger-sign-jwt-container.tsxuses a similar conditional approach.request-keys-flow.tsxalso conditions theonClosebased onisActionCancellableByUser.This pattern suggests that the application consistently handles the ability to close dialogs based on specific conditions, which likely aligns with the intended user flow for handling non-cancellable ledger actions. Therefore, the implementation in
ledger-stacks-sign-msg-container.tsxappears to be consistent with other parts of the application.Final Assessment:
- The conditional
onCloseimplementation inledger-stacks-sign-msg-container.tsxaligns with similar patterns found in other ledger-related components, suggesting a consistent approach across the application.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new onClose logic aligns with user flow requirements. # Test: Search for the usage of `onClose` in ledger related components. Expect: Consistent handling of non-cancellable actions. rg --type tsx $'onClose'Length of output: 51
Script:
#!/bin/bash # Description: Re-verify that the new onClose logic aligns with user flow requirements. # Test: Search for the usage of `onClose` in ledger related components. Expect: Consistent handling of non-cancellable actions. rg --glob '*.tsx' 'onClose'Length of output: 12803
src/app/features/dialogs/increase-fee-dialog/increase-stx-fee-dialog.tsx (2)
27-27: Updated import to useDialogHeaderinstead ofHeader. Ensure all references to the oldHeadercomponent are updated across the project.
106-106: TheDialogcomponent now includes atitlein theDialogHeader. Ensure this title is appropriate and consistent with UI guidelines.
| const backLocation = location.pathname.match(RouteUrls.SendOrdinalInscription) | ||
| ? RouteUrls.Home | ||
| : '..'; | ||
| // Use baseUrl to determine where to go on close |
There was a problem hiding this comment.
This is the main fix for 5143.
I tried making changes to our routing to remove /* and restructure things but had no luck and hit old bugs from the past.
For Ledger, we have a base route that we then append dialog routes to.
Here, when the user rejects we take the base url from location and send them back there.
- So if they hit:
/sign-bip322-message - Then go to
/sign-bip322-message/connect-your-ledger - Then
/sign-bip322-message/awaiting-approval - On close we now go back to
/sign-bip322-message
| @@ -140,27 +140,20 @@ function LedgerSignStacksMsg({ account, unsignedMessage }: LedgerSignMsgProps) { | |||
| } | |||
| } | |||
|
|
|||
| const allowUserToGoBack = get(location.state, 'goBack'); | |||
There was a problem hiding this comment.
I removed this allowUserToGoBack as I can't find where 'goBack' is set in the state and I don't think it's doing anything
| @@ -88,8 +88,9 @@ export function signStacksTransactionWithSignature(transaction: string, signatur | |||
| export function useActionCancellableByUser() { | |||
| const { pathname } = useLocation(); | |||
| return ( | |||
| pathname.includes(RouteUrls.DeviceBusy) || | |||
| pathname.includes(RouteUrls.ConnectLedger) || | |||
There was a problem hiding this comment.
I made a change here to allow users to close the dialog if they are on:
ConnectLedgerConnectLedgerError
I stopped allowing close if they are on the DeviceBusy route as this is when we are loading their keys so I don't think they should be able to close it
pete-watters
force-pushed
the
fix/5143/ledger-routing
branch
from
f28b981 to
85328a9
Compare
|
@kyranjamie : Opening this as a DRAFT as I made some more changes I am not sure you will agree are improvements. I can pick out the change to fix this specific bug as needed. I was using this as an exercise to help me get up to speed with our Ledger code. When working on changing the dialogs/containers, we agreed to remove the text that was saying I have made changes so that when we are awaiting specific actions, the Dialog is not closable and we don't show a disabled Area.mp4It's not perfect though as users can still close from this interim screen before accounts are loaded: If they do that, it continues to work but the UI flashes until the accounts are loaded trying to show |
| @@ -148,11 +150,9 @@ function LedgerSignStacksTxContainer() { | |||
| hasUserSkippedBuggyAppWarning, | |||
| }; | |||
|
|
|||
| const canClose = !awaitingDeviceConnection && canUserCancelAction; | |||
There was a problem hiding this comment.
Any way we can avoid the repetition of this expression?
There was a problem hiding this comment.
Sure, I will refactor it 👍
There was a problem hiding this comment.
@kyranjamie: I did a refactor to improve this here.
I added some more notes here.
I'm going to do some more testing but let me know if you think the code looks OK or if I should keep going with the refactor
Thanks, no rush, just wanted to share WIP so far |
pete-watters
force-pushed
the
fix/5143/ledger-routing
branch
from
53a3b71 to
e2c7e19
Compare
pete-watters
force-pushed
the
fix/5143/ledger-routing
branch
3 times, most recently
from
d848d81 to
01dbb33
Compare
| @@ -120,3 +122,19 @@ export function checkLockedDeviceError(e: Error) { | |||
| e?.message === LedgerConnectionErrors.DeviceLocked | |||
| ); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
I moved this logic to generic as it's for both Stacks and BTC.
I noticed that we often return awaitingDeviceConnection from the ledger hooks and then we need to feed it into my new hook useCancelLedgerAction.
For exampleuseLedgerSignTx returns awaitingDeviceConnection which gets passed into useCancelLedgerAction.
I experimented with instead pushing that logic into the hooks so that they return canCancelLedgerAction instead of awaitingDeviceConnection but going that route and I will need to also update BaseLedgerOperationContext which could be going a bit too far:
export interface BaseLedgerOperationContext {
latestDeviceResponse: LatestDeviceResponse;
awaitingDeviceConnection: boolean;
}
pete-watters
force-pushed
the
fix/5143/ledger-routing
branch
from
01dbb33 to
ff5aba0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (36)
- src/app/components/broadcast-error-dialog/broadcast-error-dialog.tsx (2 hunks)
- src/app/features/dialogs/edit-nonce-dialog/edit-nonce-dialog.tsx (2 hunks)
- src/app/features/dialogs/high-fee-dialog/high-fee-dialog.tsx (2 hunks)
- src/app/features/dialogs/increase-fee-dialog/increase-btc-fee-dialog.tsx (2 hunks)
- src/app/features/dialogs/increase-fee-dialog/increase-fee-sent-dialog.tsx (2 hunks)
- src/app/features/dialogs/increase-fee-dialog/increase-stx-fee-dialog.tsx (2 hunks)
- src/app/features/dialogs/switch-account-dialog/components/account-list-unavailable.tsx (1 hunks)
- src/app/features/dialogs/switch-account-dialog/switch-account-dialog.tsx (3 hunks)
- src/app/features/ledger/flows/bitcoin-tx-signing/ledger-bitcoin-sign-tx-container.tsx (2 hunks)
- src/app/features/ledger/flows/jwt-signing/ledger-sign-jwt-container.tsx (3 hunks)
- src/app/features/ledger/flows/request-bitcoin-keys/ledger-request-bitcoin-keys.tsx (3 hunks)
- src/app/features/ledger/flows/request-stacks-keys/ledger-request-stacks-keys.tsx (3 hunks)
- src/app/features/ledger/flows/stacks-message-signing/ledger-stacks-sign-msg-container.tsx (3 hunks)
- src/app/features/ledger/flows/stacks-tx-signing/ledger-sign-stacks-tx-container.tsx (2 hunks)
- src/app/features/ledger/generic-flows/request-keys/request-keys-flow.tsx (1 hunks)
- src/app/features/ledger/generic-flows/tx-signing/tx-signing-flow.tsx (1 hunks)
- src/app/features/ledger/generic-steps/connect-device/connect-ledger-error.layout.tsx (1 hunks)
- src/app/features/ledger/generic-steps/connect-device/connect-ledger-start.tsx (2 hunks)
- src/app/features/ledger/generic-steps/unsupported-browser/unsupported-browser.layout.tsx (2 hunks)
- src/app/features/ledger/hooks/use-ledger-navigate.ts (1 hunks)
- src/app/features/ledger/utils/generic-ledger-utils.ts (2 hunks)
- src/app/features/ledger/utils/stacks-ledger-utils.ts (3 hunks)
- src/app/features/retrieve-taproot-to-native-segwit/components/retrieve-taproot-to-native-segwit.layout.tsx (2 hunks)
- src/app/features/settings/network/network.tsx (2 hunks)
- src/app/features/settings/sign-out/sign-out.tsx (2 hunks)
- src/app/features/settings/theme/theme-dialog.tsx (2 hunks)
- src/app/pages/rpc-sign-bip322-message/rpc-sign-bip322-message.tsx (2 hunks)
- src/app/pages/send/ordinal-inscription/send-inscription-choose-fee.tsx (2 hunks)
- src/app/pages/send/ordinal-inscription/send-inscription-form.tsx (2 hunks)
- src/app/pages/send/ordinal-inscription/send-inscription-review.tsx (2 hunks)
- src/app/pages/send/ordinal-inscription/sent-inscription-summary.tsx (2 hunks)
- src/app/pages/send/send-crypto-asset-form/components/recipient-accounts-dialog/recipient-accounts-dialog.tsx (2 hunks)
- src/app/ui/components/containers/dialog/dialog.tsx (1 hunks)
- src/app/ui/components/containers/headers/components/header-action-button.tsx (1 hunks)
- src/app/ui/components/containers/headers/dialog-header.tsx (1 hunks)
- src/app/ui/components/containers/headers/header.tsx (4 hunks)
Files skipped from review due to trivial changes (1)
- src/app/features/dialogs/increase-fee-dialog/increase-btc-fee-dialog.tsx
Files skipped from review as they are similar to previous changes (34)
- src/app/components/broadcast-error-dialog/broadcast-error-dialog.tsx
- src/app/features/dialogs/edit-nonce-dialog/edit-nonce-dialog.tsx
- src/app/features/dialogs/high-fee-dialog/high-fee-dialog.tsx
- src/app/features/dialogs/increase-fee-dialog/increase-fee-sent-dialog.tsx
- src/app/features/dialogs/increase-fee-dialog/increase-stx-fee-dialog.tsx
- src/app/features/dialogs/switch-account-dialog/components/account-list-unavailable.tsx
- src/app/features/dialogs/switch-account-dialog/switch-account-dialog.tsx
- src/app/features/ledger/flows/bitcoin-tx-signing/ledger-bitcoin-sign-tx-container.tsx
- src/app/features/ledger/flows/jwt-signing/ledger-sign-jwt-container.tsx
- src/app/features/ledger/flows/request-bitcoin-keys/ledger-request-bitcoin-keys.tsx
- src/app/features/ledger/flows/request-stacks-keys/ledger-request-stacks-keys.tsx
- src/app/features/ledger/flows/stacks-message-signing/ledger-stacks-sign-msg-container.tsx
- src/app/features/ledger/flows/stacks-tx-signing/ledger-sign-stacks-tx-container.tsx
- src/app/features/ledger/generic-flows/request-keys/request-keys-flow.tsx
- src/app/features/ledger/generic-flows/tx-signing/tx-signing-flow.tsx
- src/app/features/ledger/generic-steps/connect-device/connect-ledger-error.layout.tsx
- src/app/features/ledger/generic-steps/connect-device/connect-ledger-start.tsx
- src/app/features/ledger/generic-steps/unsupported-browser/unsupported-browser.layout.tsx
- src/app/features/ledger/hooks/use-ledger-navigate.ts
- src/app/features/ledger/utils/stacks-ledger-utils.ts
- src/app/features/retrieve-taproot-to-native-segwit/components/retrieve-taproot-to-native-segwit.layout.tsx
- src/app/features/settings/network/network.tsx
- src/app/features/settings/sign-out/sign-out.tsx
- src/app/features/settings/theme/theme-dialog.tsx
- src/app/pages/rpc-sign-bip322-message/rpc-sign-bip322-message.tsx
- src/app/pages/send/ordinal-inscription/send-inscription-choose-fee.tsx
- src/app/pages/send/ordinal-inscription/send-inscription-form.tsx
- src/app/pages/send/ordinal-inscription/send-inscription-review.tsx
- src/app/pages/send/ordinal-inscription/sent-inscription-summary.tsx
- src/app/pages/send/send-crypto-asset-form/components/recipient-accounts-dialog/recipient-accounts-dialog.tsx
- src/app/ui/components/containers/dialog/dialog.tsx
- src/app/ui/components/containers/headers/components/header-action-button.tsx
- src/app/ui/components/containers/headers/dialog-header.tsx
- src/app/ui/components/containers/headers/header.tsx
Additional comments not posted (1)
src/app/features/ledger/utils/generic-ledger-utils.ts (1)
2-2: The import ofuseLocationis correct and necessary for the new hooks.
pete-watters
force-pushed
the
fix/5143/ledger-routing
branch
from
ff5aba0 to
649a1ec
Compare
src/app/features/ledger/flows/request-bitcoin-keys/ledger-request-bitcoin-keys.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- src/app/features/ledger/flows/bitcoin-tx-signing/ledger-bitcoin-sign-tx-container.tsx (2 hunks)
- src/app/features/ledger/flows/jwt-signing/ledger-sign-jwt-container.tsx (3 hunks)
- src/app/features/ledger/flows/request-bitcoin-keys/ledger-request-bitcoin-keys.tsx (3 hunks)
- src/app/features/ledger/flows/request-stacks-keys/ledger-request-stacks-keys.tsx (3 hunks)
- src/app/features/ledger/flows/stacks-message-signing/ledger-stacks-sign-msg-container.tsx (3 hunks)
- src/app/features/ledger/flows/stacks-tx-signing/ledger-sign-stacks-tx-container.tsx (2 hunks)
- src/app/features/ledger/hooks/use-ledger-navigate.ts (2 hunks)
- src/app/features/ledger/utils/generic-ledger-utils.ts (2 hunks)
- src/app/features/ledger/utils/stacks-ledger-utils.ts (3 hunks)
Files skipped from review as they are similar to previous changes (9)
- src/app/features/ledger/flows/bitcoin-tx-signing/ledger-bitcoin-sign-tx-container.tsx
- src/app/features/ledger/flows/jwt-signing/ledger-sign-jwt-container.tsx
- src/app/features/ledger/flows/request-bitcoin-keys/ledger-request-bitcoin-keys.tsx
- src/app/features/ledger/flows/request-stacks-keys/ledger-request-stacks-keys.tsx
- src/app/features/ledger/flows/stacks-message-signing/ledger-stacks-sign-msg-container.tsx
- src/app/features/ledger/flows/stacks-tx-signing/ledger-sign-stacks-tx-container.tsx
- src/app/features/ledger/hooks/use-ledger-navigate.ts
- src/app/features/ledger/utils/generic-ledger-utils.ts
- src/app/features/ledger/utils/stacks-ledger-utils.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Out of diff range and nitpick comments (1)
src/app/features/ledger/flows/request-bitcoin-keys/request-bitcoin-keys.utils.ts (1)
Line range hint
81-86: Ensure consistent handling ofdefaultNumberOfKeysToPullFromLedgerDevice.The loop structure is correct, but consider extracting the repeated logic into a separate function to adhere to the DRY principle.
- for ( - let accountIndex = 0; - accountIndex < defaultNumberOfKeysToPullFromLedgerDevice; - accountIndex++ - ) { - onRequestKey?.(accountIndex + defaultNumberOfKeysToPullFromLedgerDevice); - const { path, policy } = await getTaprootExtendedPublicKey({ - bitcoinApp, - fingerprint, - network, - accountIndex, - }); - keys.push({ id: createWalletIdDecoratedPath(path, 'default'), path, policy, targetId }); - } + async function addKeys(getExtendedPublicKey, startIndex = 0) { + for ( + let accountIndex = 0; + accountIndex < defaultNumberOfKeysToPullFromLedgerDevice; + accountIndex++ + ) { + onRequestKey?.(accountIndex + startIndex); + const { path, policy } = await getExtendedPublicKey({ + bitcoinApp, + fingerprint, + network, + accountIndex, + }); + keys.push({ id: createWalletIdDecoratedPath(path, 'default'), path, policy, targetId }); + } + } + await addKeys(getNativeSegwitExtendedPublicKey); + await addKeys(getTaprootExtendedPublicKey, defaultNumberOfKeysToPullFromLedgerDevice);
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/app/features/ledger/flows/request-bitcoin-keys/ledger-request-bitcoin-keys.tsx (3 hunks)
- src/app/features/ledger/flows/request-bitcoin-keys/request-bitcoin-keys.utils.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/app/features/ledger/flows/request-bitcoin-keys/ledger-request-bitcoin-keys.tsx
|
|
||
| for ( | ||
| let accountIndex = 0; | ||
| accountIndex < defaultNumberOfKeysToPullFromLedgerDevice; | ||
| accountIndex++ | ||
| ) { |
There was a problem hiding this comment.
Ensure consistent handling of defaultNumberOfKeysToPullFromLedgerDevice.
The loop structure is correct, but consider extracting the repeated logic into a separate function to adhere to the DRY principle.
- for (
- let accountIndex = 0;
- accountIndex < defaultNumberOfKeysToPullFromLedgerDevice;
- accountIndex++
- ) {
- onRequestKey?.(accountIndex);
- const { path, policy } = await getNativeSegwitExtendedPublicKey({
- bitcoinApp,
- fingerprint,
- network,
- accountIndex,
- });
- keys.push({ id: createWalletIdDecoratedPath(path, 'default'), path, policy, targetId });
- }
+ async function addKeys(getExtendedPublicKey, startIndex = 0) {
+ for (
+ let accountIndex = 0;
+ accountIndex < defaultNumberOfKeysToPullFromLedgerDevice;
+ accountIndex++
+ ) {
+ onRequestKey?.(accountIndex + startIndex);
+ const { path, policy } = await getExtendedPublicKey({
+ bitcoinApp,
+ fingerprint,
+ network,
+ accountIndex,
+ });
+ keys.push({ id: createWalletIdDecoratedPath(path, 'default'), path, policy, targetId });
+ }
+ }
+ await addKeys(getNativeSegwitExtendedPublicKey);
+ await addKeys(getTaprootExtendedPublicKey, defaultNumberOfKeysToPullFromLedgerDevice);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.
| for ( | |
| let accountIndex = 0; | |
| accountIndex < defaultNumberOfKeysToPullFromLedgerDevice; | |
| accountIndex++ | |
| ) { | |
| async function addKeys(getExtendedPublicKey, startIndex = 0) { | |
| for ( | |
| let accountIndex = 0; | |
| accountIndex < defaultNumberOfKeysToPullFromLedgerDevice; | |
| accountIndex++ | |
| ) { | |
| onRequestKey?.(accountIndex + startIndex); | |
| const { path, policy } = await getExtendedPublicKey({ | |
| bitcoinApp, | |
| fingerprint, | |
| network, | |
| accountIndex, | |
| }); | |
| keys.push({ id: createWalletIdDecoratedPath(path, 'default'), path, policy, targetId }); | |
| } | |
| } | |
| await addKeys(getNativeSegwitExtendedPublicKey); | |
| await addKeys(getTaprootExtendedPublicKey, defaultNumberOfKeysToPullFromLedgerDevice); |
This PR:
DialogHeader-s to their own simpler implementationSummary by CodeRabbit
New Features
Improvements
Bug Fixes
Refactor