fix(connect): do not release device when device is going to be discon… #16843
Conversation
mroz22
requested review from
szymonlesisz,
martykan and
marekrjpolak
as code owners
WalkthroughThe changes focus on updating the Possibly related PRs
Suggested labels
Suggested reviewers
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
🪧 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (1)
packages/connect/src/core/onCallFirmwareUpdate.ts (1)
515-532: Consider adding error handling for language update failures.While the language update failure is logged, it's silently caught. Consider:
- Notifying the user about the language update failure
- Allowing retry of the language update
try { log.info( 'onCallFirmwareUpdate', 'changing language for fresh device to: ', params.language, ); await reconnectedDevice.changeLanguage({ language: params.language }); } catch (err) { log.error('onCallFirmwareUpdate', 'changeLanguage failed silently: ', err); + postMessage( + createUiMessage(UI.FIRMWARE_PROGRESS, { + device: reconnectedDevice.toMessageObject(), + operation: 'language-update-failed', + error: err.message, + }), + ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/connect/src/core/onCallFirmwareUpdate.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: build-deploy
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: prepare_android_test_app
- GitHub Check: Setup and Cache Dependencies
🔇 Additional comments (3)
packages/connect/src/core/onCallFirmwareUpdate.ts (3)
487-487: LGTM! Enhanced logging improves traceability.The added log statements provide clear markers for tracking the firmware update process, making it easier to debug issues.
Also applies to: 490-491
566-567: Verify device release timing.The device is only released after firmware validation is complete or when validation is omitted. This is the correct approach as it ensures all necessary operations are completed before releasing the device.
Also applies to: 578-579
480-508: Verify firmware upload sequence.The removal of
await reconnectedDevice.release()after firmware upload is a good fix. Previously, releasing the device immediately after upload could cause issues when:
- An intermediary firmware needs to be installed
- The device needs to be reconnected for further operations
This change aligns with the PR objective to fix device disconnection issues.
Let's verify if there are any other instances in the codebase where devices are prematurely released during firmware updates:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for device release calls near firmware upload operations ast-grep --pattern 'uploadFirmware($$$).then($$$release()$$$)' # Search for any direct release calls after firmware upload rg -B 5 -A 5 'uploadFirmware.*\n.*release\(\)'Length of output: 305
🏁 Script executed:
#!/bin/bash echo "Searching for device release calls using ast-grep (with multiline flag):" ast-grep -U --pattern 'uploadFirmware($$$).then($$$release()$$$)' echo -e "\nSearching for direct release calls after uploadFirmware within surrounding lines using rg (with multiline enabled):" rg -U -nH -C10 'await\s+uploadFirmware(.*\n){1,3}\s*await\s+.*release\(' . echo -e "\nSearching for explicit release calls on reconnectedDevice:" rg -U -nH -C5 'await\s+reconnectedDevice\.release\(' .Length of output: 2978
Firmware Update Sequence Verified.
The shell script output confirms that the only remaining calls to
await reconnectedDevice.release()occur later in the firmware update sequence (after performing the firmware hash check), rather than immediately after the initial firmware upload. This aligns with the intended fix of removing an early release that could disrupt intermediary firmware updates.• No premature device release calls are found near the firmware upload operation.
• The remaining release calls (at lines 566 and 578) occur in the appropriate control flow for post-update validation.
|
QA OK bridge 2.x - 1 of 1 OK /worked before/ Info:
|
mroz22
added
the
release
|
cc @MiroslavProchazka and @komret - cherrypick candidate |
|
QA OK |
this could be a solution to this #15013 (comment)