Feat/upgrade to bridge #16780
Feat/upgrade to bridge #16780
Conversation
WalkthroughThe pull request introduces updates across multiple packages to improve transport management and debugging functionality. In the connection component, the 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 (
|
marekrjpolak
force-pushed
the
feat/upgrade-to-bridge
branch
2 times, most recently
from
26c0a1a to
e4bfffd
Compare
marekrjpolak
force-pushed
the
feat/upgrade-to-bridge
branch
from
e4bfffd to
95eb1d7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/suite/src/views/settings/SettingsDebug/Transport.tsx (2)
22-31: Verify completeness of transport lists and descriptions.The newly added
TRANSPORTS_WEB,TRANSPORTS_DESKTOP, andTRANSPORT_DESCRIPTIONSseem accurate. Double-check that no internal or third-party transport type is missing, particularly if additional environments might require specialized transport entries.
63-80: Double-check XOR logic when filtering nextTransports.The statement
(t.name === transport.name) !== t.checkedcan be confusing at first glance. It effectively toggles the item. Consider extracting a helper to enhance readability. Otherwise, the dispatch calls (setDebugMode,TrezorConnect.setTransports) are consistent with the suite’s approach.packages/transport/src/transports/abstract.ts (1)
146-148: Document or implement thepingmethod’s intended behavior.Currently,
pingalways resolves tofalse. If this is just a placeholder, adding a brief comment highlighting that it’s a no-op would clarify its purpose for future maintainers.packages/connect/src/device/DeviceList.ts (1)
405-423: Consider adding documentation for the upgrade check mechanism.The
scheduleUpgradeCheckmethod implements an important feature but lacks documentation explaining its purpose and behavior.Add JSDoc comments to explain:
- The purpose of the upgrade check
- The checking interval
- The upgrade process
- The cleanup mechanism
+/** + * Schedules periodic checks for newer transports of the same API type. + * If a newer transport is found and successfully pinged, it triggers + * a transport upgrade process. + * + * @param apiType - The transport API type to check + * @param initParams - Transport initialization parameters + * @private + */ private scheduleUpgradeCheck(apiType: TransportApiType, initParams: InitParams) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/connect/src/device/DeviceList.ts(4 hunks)packages/suite/src/views/settings/SettingsDebug/Transport.tsx(2 hunks)packages/transport/src/transports/abstract.ts(1 hunks)packages/transport/src/transports/bridge.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
🔇 Additional comments (7)
packages/suite/src/views/settings/SettingsDebug/Transport.tsx (3)
13-19: Ensure consistency for newly introduced types.Defining a dedicated alias (
Transport) and extending it withinTransportMenuItemis a neat approach. Just make sure all future references, comments, and imports internally echo these names for clarity. The definition ofnameandcheckedfields appears coherent.
33-47: Memoize usage looks fine; watch out for repeated updates.
useMemoover[transports, activeTransports, debugTransports]is appropriate. If the data changes very frequently (e.g., each render), the memo might be recalculated often. Otherwise, this approach is optimal for performance.
49-52: Conditional desktop vs. web transport selection is well-structured.Dynamic assignment based on the environment is clear and maintainable.
packages/transport/src/transports/bridge.ts (1)
90-94: Ensure error handling covers connection edge cases.
pingattempts a POST to'/', returning a boolean based on success. This is concise, but if an unexpected network error arises (e.g., partial or slow connectivity), confirm that returningfalseis the desired fallback behavior. Otherwise, the method aligns with the abstraction fromAbstractTransport.packages/connect/src/device/DeviceList.ts (3)
206-206: LGTM! Property declaration is well-typed.The
scheduledUpgradeChecksproperty is correctly typed usingApiTypeMapto store timeout identifiers for each transport type.
383-386: LGTM! Transport upgrade check scheduling is properly guarded.The upgrade check is only scheduled when a transport is successfully started or retained, which is the correct behavior.
571-572: LGTM! Proper cleanup of scheduled upgrade checks.The
stopTransportmethod correctly cleans up the scheduled upgrade check for the transport being stopped.
| private scheduleUpgradeCheck(apiType: TransportApiType, initParams: InitParams) { | ||
| clearTimeout(this.scheduledUpgradeChecks[apiType]); | ||
| this.scheduledUpgradeChecks[apiType] = setTimeout(async () => { | ||
| const transport = this.transport[apiType]; | ||
| if (!transport) return; | ||
| const transports = this.transports.filter(t => t.apiType === apiType); | ||
| for (const t of transports) { | ||
| if (t === transport) break; | ||
| if (await t.ping()) { | ||
| this.transportLock(apiType, 'Upgrading', signal => | ||
| this.createInitPromise(apiType, initParams, signal), | ||
| ).catch(() => {}); | ||
|
|
||
| return; | ||
| } | ||
| } | ||
| this.scheduleUpgradeCheck(apiType, initParams); | ||
| }, 1000); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling and cleanup in the upgrade check.
The upgrade check implementation could be improved in a few areas:
- The timeout callback should handle potential errors during transport ping
- The recursive scheduling could lead to memory leaks if the transport is stopped during the ping operation
Consider this implementation:
private scheduleUpgradeCheck(apiType: TransportApiType, initParams: InitParams) {
clearTimeout(this.scheduledUpgradeChecks[apiType]);
this.scheduledUpgradeChecks[apiType] = setTimeout(async () => {
const transport = this.transport[apiType];
- if (!transport) return;
+ if (!transport || !this.scheduledUpgradeChecks[apiType]) return;
const transports = this.transports.filter(t => t.apiType === apiType);
for (const t of transports) {
if (t === transport) break;
- if (await t.ping()) {
- this.transportLock(apiType, 'Upgrading', signal =>
- this.createInitPromise(apiType, initParams, signal),
- ).catch(() => {});
-
- return;
+ try {
+ if (await t.ping()) {
+ this.transportLock(apiType, 'Upgrading', signal =>
+ this.createInitPromise(apiType, initParams, signal),
+ ).catch(() => {});
+ return;
+ }
+ } catch (error) {
+ // Log error but continue checking other transports
+ console.error(`Error pinging transport: ${error}`);
}
}
this.scheduleUpgradeCheck(apiType, initParams);
}, 1000);
}📝 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.
| private scheduleUpgradeCheck(apiType: TransportApiType, initParams: InitParams) { | |
| clearTimeout(this.scheduledUpgradeChecks[apiType]); | |
| this.scheduledUpgradeChecks[apiType] = setTimeout(async () => { | |
| const transport = this.transport[apiType]; | |
| if (!transport) return; | |
| const transports = this.transports.filter(t => t.apiType === apiType); | |
| for (const t of transports) { | |
| if (t === transport) break; | |
| if (await t.ping()) { | |
| this.transportLock(apiType, 'Upgrading', signal => | |
| this.createInitPromise(apiType, initParams, signal), | |
| ).catch(() => {}); | |
| return; | |
| } | |
| } | |
| this.scheduleUpgradeCheck(apiType, initParams); | |
| }, 1000); | |
| } | |
| private scheduleUpgradeCheck(apiType: TransportApiType, initParams: InitParams) { | |
| clearTimeout(this.scheduledUpgradeChecks[apiType]); | |
| this.scheduledUpgradeChecks[apiType] = setTimeout(async () => { | |
| const transport = this.transport[apiType]; | |
| if (!transport || !this.scheduledUpgradeChecks[apiType]) return; | |
| const transports = this.transports.filter(t => t.apiType === apiType); | |
| for (const t of transports) { | |
| if (t === transport) break; | |
| try { | |
| if (await t.ping()) { | |
| this.transportLock(apiType, 'Upgrading', signal => | |
| this.createInitPromise(apiType, initParams, signal), | |
| ).catch(() => {}); | |
| return; | |
| } | |
| } catch (error) { | |
| // Log error but continue checking other transports | |
| console.error(`Error pinging transport: ${error}`); | |
| } | |
| } | |
| this.scheduleUpgradeCheck(apiType, initParams); | |
| }, 1000); | |
| } |
|
QA OK tested auto upgrade
Info:
|
|
from now on we will see these error in console when we use only webUSB ? 
|
|
I would like to see only unexpected errors in console and this one is in fact expected. But I am afraid we can't do anything about it. :( |
|
@bosomt If you really want to use only webusb and never upgrade (unlike production web suite), it should be enough to turn Bridge transport off in debug menu. |
Description
When any transport is active,
DeviceListis periodically pinging to all transports with higher priority (currently only Bridge ping is implemented) and initiate reconnection as soon as any of them becomes accessible.