feat(suite): non-canonical bridge versions deprecation. phase 1 #17272
Conversation
|
🚀 Expo preview is ready!
|
|
That's great. We should tell users how to uninstall the bridge in the tooltip. Even I don't know how to uninstall it. |
|
I guess @Ondra-Zik-SL could put together some materials? |
mroz22
force-pushed
the
bridge-deprecation
branch
2 times, most recently
from
333dfc3 to
f032db6
Compare
mroz22
changed the title
mroz22
requested review from
MiroslavProchazka,
marekrjpolak,
szymonlesisz and
martykan
as code owners
WalkthroughThis pull request implements a series of changes to deprecate the current Bridge functionality. It removes a test suite for ✨ Finishing Touches
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. 🪧 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 (3)
suite-common/suite-config/src/routes.ts (1)
45-51: New route added for Bridge deprecation noticesThis route configuration correctly sets up the new Bridge deprecation page as a foreground app with modal parameters, which will enable displaying warnings to users about deprecated Bridge versions.
The pattern '/bridge-bridge' seems a bit unusual. Consider renaming to '/bridge-deprecated' for consistency with the app name and better clarity.
packages/suite/src/views/suite/bridge-deprecated/index.tsx (2)
35-45: Unnecessary Fragment wrapper.The Fragment (
<>...</>) around the Button component is redundant as it contains only a single child element.- bottomBarComponents={ - <> - <Button - icon="caretLeft" - variant="tertiary" - onClick={() => goToWallet()} - data-testid="@bridge/goto/wallet-index" - > - <Translation id="TR_TAKE_ME_BACK_TO_WALLET" /> - </Button> - </> - } + bottomBarComponents={ + <Button + icon="caretLeft" + variant="tertiary" + onClick={() => goToWallet()} + data-testid="@bridge/goto/wallet-index" + > + <Translation id="TR_TAKE_ME_BACK_TO_WALLET" /> + </Button> + }🧰 Tools
🪛 Biome (1.9.4)
[error] 35-44: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
39-39: Simplify onClick handler.The current implementation creates an anonymous function that calls
goToWallet(). SincegoToWalletis already a function that doesn't require any arguments, you can pass it directly to the onClick handler.- onClick={() => goToWallet()} + onClick={goToWallet}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/connect/src/data/__tests__/transportInfo.test.ts(0 hunks)packages/connect/src/data/transportInfo.ts(1 hunks)packages/connect/src/device/TransportList.ts(1 hunks)packages/suite/src/components/suite/banners/SuiteBanners/BridgeDeprecatedBanner.tsx(2 hunks)packages/suite/src/components/suite/banners/SuiteBanners/SuiteBanners.tsx(2 hunks)packages/suite/src/components/suite/modals/ModalSwitcher/ForegroundAppModal.tsx(2 hunks)packages/suite/src/hooks/suite/usePreferredModal.ts(1 hunks)packages/suite/src/support/messages.ts(1 hunks)packages/suite/src/views/suite/bridge-deprecated/index.tsx(1 hunks)packages/transport/src/transports/bridge.ts(3 hunks)packages/urls/src/urls.ts(1 hunks)suite-common/suite-config/src/routes.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/connect/src/data/tests/transportInfo.test.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/suite/src/views/suite/bridge-deprecated/index.tsx
[error] 35-44: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Unit Tests
- GitHub Check: Type Checking
- GitHub Check: Linting and formatting
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: build-deploy
- GitHub Check: prepare_android_test_app
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (17)
packages/connect/src/device/TransportList.ts (1)
34-34: BridgeTransport instantiation simplified.The instantiation of BridgeTransport has been simplified by removing the
latestVersionparameter, which is consistent with the changes in the BridgeTransport implementation.packages/connect/src/data/transportInfo.ts (1)
44-44: Changed getBridgeInfo to a local function.The
getBridgeInfofunction is now a local function rather than an exported one, which aligns with the deprecation strategy for non-canonical bridge versions. This encapsulates the bridge version information within this module.packages/transport/src/transports/bridge.ts (2)
79-82: Removed latestVersion from constructor parameters.The
latestVersionparameter has been removed from the constructor destructuring, which is consistent with the changes in the type definition and the overall plan to simplify bridge version handling.
103-109: Implemented explicit version checking for non-canonical bridges.The code now explicitly checks for supported bridge versions instead of using dynamic version comparison. This implementation directly supports the PR objective of deprecating non-canonical bridge versions (other than 2.0.27 and 2.0.33).
The comment clearly explains which versions are supported, and the code correctly handles version 3.x as a forward-compatibility measure.
packages/urls/src/urls.ts (1)
168-168: New URL added for Bridge uninstallation guideThis URL will be used to direct users to documentation about uninstalling deprecated Bridge versions, which aligns with the PR objective of providing guidance for users to remove non-canonical bridges.
packages/suite/src/hooks/suite/usePreferredModal.ts (1)
19-19: Added 'bridge-deprecated' route to high-priority modalsThis ensures that the Bridge deprecation warning will have high priority and will be displayed prominently to users, similar to other important notices like firmware updates. This properly supports the goal of notifying users about non-canonical bridges.
packages/suite/src/components/suite/modals/ModalSwitcher/ForegroundAppModal.tsx (2)
12-12: Added import for BridgeDeprecated componentProperly imports the new component that will display the Bridge deprecation warnings.
30-30: Added BridgeDeprecated component to foreground app mappingThis correctly maps the 'bridge-deprecated' route to the BridgeDeprecated component, ensuring the deprecation notice will be displayed when the route is accessed.
packages/suite/src/components/suite/banners/SuiteBanners/SuiteBanners.tsx (2)
19-19: Import updated for deprecation notice implementation.The import has been updated to use the new
BridgeDeprecatedcomponent which aligns with the PR objective of displaying warnings for non-canonical bridge versions.
83-85: Simplified condition and updated banner for bridge deprecation.The code now directly checks
bridge?.outdatedproperty and displays theBridgeDeprecatedbanner instead of the previousUpdateBridgecomponent. This change correctly implements phase 1 of the bridge deprecation plan as outlined in the PR objectives - showing warnings to users about non-canonical bridge versions.packages/suite/src/support/messages.ts (1)
9565-9573: The new message entries look good for bridge deprecation notification.These two new message entries properly support the first phase of deprecating non-canonical bridge versions. The messages provide clear warning to users about the upcoming deprecation and include instructions on uninstalling outdated bridge versions with a "Learn more" link for additional guidance. This implementation aligns with the PR objectives to notify users of non-canonical bridges with instructions for removal.
packages/suite/src/components/suite/banners/SuiteBanners/BridgeDeprecatedBanner.tsx (3)
7-7: Component name change aligns with the deprecation strategy.The renaming from
UpdateBridgetoBridgeDeprecatedclearly indicates the shift in focus from updating to deprecating, which aligns with the PR objectives for phase 1 of the non-canonical bridge deprecation plan.
16-20: Navigation and messaging appropriately updated for deprecation workflow.The changes to the navigation path, test ID, and button text are consistent with the new deprecation-focused approach:
- Navigation now directs to the new
suite-bridge-deprecatedroute- Test ID updated to match the new component purpose
- Button text changed to "TR_LEARN_MORE" which is more appropriate for a deprecation notice
These updates maintain a cohesive user experience while implementing the requested deprecation warnings.
23-23: Banner message accurately reflects deprecation status.The message now clearly communicates that the bridge version will soon be deprecated rather than suggesting an update is available, which properly informs users of the upcoming change as specified in the PR objectives.
packages/suite/src/views/suite/bridge-deprecated/index.tsx (3)
8-10: Clear documentation of component purpose.The JSDoc comment effectively explains the purpose of this component, which helps maintain code clarity for future developers.
21-32: Uninstallation guidance addresses user feedback.The implementation of uninstallation instructions with a link to detailed guidance directly addresses the feedback from user Hannsek in the PR comments, who suggested including instructions on how to uninstall non-canonical bridges.
1-51: Excellent implementation of deprecation warnings (Phase 1).This new component effectively implements Phase 1 of the deprecation plan by:
- Clearly informing users that their bridge version will soon be deprecated
- Providing uninstallation instructions with a link to detailed guidance
- Maintaining good UX with a way to return to the wallet
The component structure is clean, well-documented, and follows the established patterns in the codebase.
🧰 Tools
🪛 Biome (1.9.4)
[error] 35-44: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
|
Do we need the modal? Maybe it would be best to send them to KB where there can be a proper detailed article. |
Dunno, product question (@Hannsek) my gutfeeling was that it would be nice to be more descriptive already in the app and that it might be useful once we will start forcing users to uninstall canonical bridges as well. |
|
Tagging our Suite @trezor/suite-engagement team as this PR affects SuiteBanners file |
|
Let's keep the modal and let's edit the KB article |
mroz22
force-pushed
the
bridge-deprecation
branch
from
f032db6 to
7ff74e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/suite/src/views/suite/bridge-deprecated/index.tsx (2)
18-33: Consider adding more detailed instructions in the modal.Per Hannsek's comment in the PR discussion, users may not know how to uninstall the bridge. While you've provided a link to instructions, consider adding more direct guidance in the modal text itself.
<Modal heading={<Translation id="TR_YOUR_BRIDGE_VERSION_WILL_SOON_BE_DEPRECATED" />} description={ <Translation id="TR_BRIDGE_UNINSTALL_INSTRUCTIONS" values={{ a: chunks => ( <Link href={UNINSTALL_BRIDGE_URL} typographyStyle="hint"> {chunks} </Link> ), + // Consider adding additional structured content here, such as OS-specific instructions }} /> } isHeadingCentered
34-45: Remove unnecessary Fragment wrapper.The Fragment (
<>...</>) around the Button is unnecessary since there's only a single child element.bottomBarComponents={ - <> <Button icon="caretLeft" variant="tertiary" onClick={() => goToWallet()} data-testid="@bridge/goto/wallet-index" > <Translation id="TR_TAKE_ME_BACK_TO_WALLET" /> </Button> - </> }🧰 Tools
🪛 Biome (1.9.4)
[error] 35-44: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
packages/suite/src/components/suite/banners/SuiteBanners/SuiteBanners.tsx (1)
83-85: Consider adding logging to track user exposure to deprecation banner.Since this is phase 1 of a multi-phase approach to deprecating bridges, it would be valuable to track how many users are seeing this banner to understand the scope of the impact.
} else if (bridge?.outdated) { + // Log analytics event to track exposure to deprecation banner + // analytics.logEvent('bridge_deprecation_banner_shown'); banner = <BridgeDeprecated />; priority = 30; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/connect/src/data/__tests__/transportInfo.test.ts(0 hunks)packages/connect/src/data/transportInfo.ts(1 hunks)packages/connect/src/device/TransportList.ts(1 hunks)packages/suite/src/components/suite/banners/SuiteBanners/BridgeDeprecatedBanner.tsx(2 hunks)packages/suite/src/components/suite/banners/SuiteBanners/SuiteBanners.tsx(2 hunks)packages/suite/src/components/suite/modals/ModalSwitcher/ForegroundAppModal.tsx(2 hunks)packages/suite/src/hooks/suite/usePreferredModal.ts(1 hunks)packages/suite/src/support/messages.ts(1 hunks)packages/suite/src/views/suite/bridge-deprecated/index.tsx(1 hunks)packages/transport/src/transports/bridge.ts(3 hunks)packages/urls/src/urls.ts(1 hunks)suite-common/suite-config/src/routes.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/connect/src/data/tests/transportInfo.test.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/suite/src/hooks/suite/usePreferredModal.ts
- packages/connect/src/data/transportInfo.ts
- packages/suite/src/components/suite/modals/ModalSwitcher/ForegroundAppModal.tsx
- packages/urls/src/urls.ts
- packages/connect/src/device/TransportList.ts
- suite-common/suite-config/src/routes.ts
- packages/suite/src/components/suite/banners/SuiteBanners/BridgeDeprecatedBanner.tsx
- packages/transport/src/transports/bridge.ts
- packages/suite/src/support/messages.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/suite/src/views/suite/bridge-deprecated/index.tsx
[error] 35-44: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- 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 / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: EAS Update
- GitHub Check: prepare_android_test_app
- GitHub Check: transport-e2e-test
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (9)
packages/suite/src/views/suite/bridge-deprecated/index.tsx (5)
1-7: LGTM - Appropriate imports for the new component.You've imported all necessary components and constants for this new modal interface. The
UNINSTALL_BRIDGE_URLimport will provide the necessary link for users to learn how to uninstall deprecated bridge versions.
8-10: Good documentation.The JSDoc comment clearly explains the purpose of this component, which aligns well with the PR objectives for phase 1 of the bridge deprecation plan.
11-17: LGTM - Component setup with proper hooks.The component is properly set up with the necessary hooks. The
useLayout('Bridge')call ensures the component renders with the appropriate layout template.
46-50: LGTM - Metadata for SEO is properly included.Good job adding the metadata for SEO purposes. This helps with proper page titles when the modal is displayed.
1-50:❓ Verification inconclusive
Verify user experience around navigation.
Since this is an important deprecation notice, verify that the user experience around dismissing this modal is appropriate. Consider these questions:
- Will users be able to easily return to this information if they dismiss it?
- Should there be a "Don't show again" option, or should this warning persist until action is taken?
- Should there be a more prominent call-to-action for uninstalling the deprecated bridge?
🌐 Web query:
What are best practices for deprecation notices in software applications?💡 Result:
Deprecating software features effectively requires a structured approach to minimize user disruption while maintaining transparency. Best practices focus on clear communication, phased timelines, and providing actionable alternatives.
Communication Strategies
- Multi-channel notifications: Use emails, in-app banners, blog posts, and social media to announce deprecations. Directly contact affected users via analytics tools (e.g., Moesif) to identify active users and notify them[1][4][6].
- Early and repeated alerts: Start notifications well in advance (6–12 months) and send periodic reminders. Slack Design recommends non-dismissible in-product warnings as the deprecation deadline approaches[6][4].
- Documentation updates: Mark deprecated features in API docs, changelogs, and help guides. The USGS Astrogeology team mandates adding deprecation warnings to documentation and code comments[2][7].
Phased Timelines
- Minimum 6-month deprecation period: Allow users time to migrate by enforcing a buffer between announcement and removal. IATI and Totara require at least six months before final removal, often aligning with major release cycles[5][7].
- Brownouts for inactive users: Temporarily disable deprecated features during low-usage periods to alert unresponsive users, as suggested by Moesif[1].
Technical Implementation
- Code-level warnings: Embed deprecation notices in API responses, console logs, or UI elements. The USGS team uses
coutstatements, while Google employs static analysis tools like Tricorder to flag deprecated code usage[2][3].- Versioning alignment: Remove deprecated features only in major releases to avoid breaking changes in minor updates[7].
User Support
- Provide alternatives: Offer migration guides, updated code samples, or replacement APIs. Techtarget emphasizes that clear alternatives reduce friction and improve retention[4][10].
- Dedicated support channels: Link deprecation notices to help articles, FAQs, or customer support contact options[6][10].
Enforcement and Feedback
- Compulsory deprecation: Google advises setting enforceable deadlines with mechanisms to break noncompliant integrations after sufficient warning[3].
- Community feedback loops: During the deprecation period, gather user input to adjust timelines or improve documentation, as outlined in IATI’s process[5].
By combining proactive communication, user-centric timelines, and technical safeguards, teams can deprecate features while maintaining trust and reducing operational disruptions.
Citations:
- 1: https://www.moesif.com/blog/best-practices/api-analytics/API-Best-Practices-For-Feature-Deprecation/
- 2: https://astrogeology.usgs.gov/docs/how-to-guides/software-management/deprecation/
- 3: https://abseil.io/resources/swe-book/html/ch15.html
- 4: https://www.techtarget.com/searchsoftwarequality/tip/How-to-deprecate-software-features-without-bothering-users
- 5: https://iatistandard.org/documents/10614/IATI_Software_Deprecation_Process.pdf
- 6: https://slack.design/articles/on-writing-for-deprecation/
- 7: https://totara.atlassian.net/wiki/spaces/DEV/pages/121187267/Deprecation+guidelines
- 8: https://blog.invgate.com/deprecated-software
- 9: https://www.invosolutions.com/customer-resources/software-deprecation-guide
- 10: https://document360.com/blog/api-deprecation/
Please verify the modal’s navigation flow and its persistence in the context of deprecation messaging.
Based on current best practices for deprecation notices, please consider the following:
Modal Persistence & Accessibility:
Ensure that if the modal is dismissed (e.g., via the "Take me back to wallet" button), users have an accessible way to revisit the deprecation information. If the warning is critical, it may be preferable to keep the deprecation alert non-dismissible or provide alternate paths (like a dedicated menu or documentation link) to re-access the information.Actionability of the Warning:
Evaluate if the current navigation, which directs users back to the wallet, might be overshadowing the core purpose of prompting an uninstallation of the legacy bridge. Consider whether incorporating a more prominent or direct call-to-action (for example, an "Uninstall Bridge" button) would better support user remediation.Dismissal Options:
Reflect on whether a "Don't show again" option is appropriate. While such an option can reduce annoyance, best practices suggest that deprecation warnings—especially ones with long lead times—should persist until the necessary action is taken.Please review these points and ensure that the user experience aligns with proactive deprecation communication strategies.
🧰 Tools
🪛 Biome (1.9.4)
[error] 35-44: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
packages/suite/src/components/suite/banners/SuiteBanners/SuiteBanners.tsx (4)
19-19: Component rename from UpdateBridge to BridgeDeprecated.You've updated the component import to use the new
BridgeDeprecatedcomponent instead ofUpdateBridge. This aligns with the PR objective of deprecating non-canonical bridge versions.
1-107: Verify that priority level is appropriate for the deprecation notice.The bridge deprecation banner has a priority of 30, which is lower than several other banners like firmware issues, backup issues, and safety checks. Please verify that this is the appropriate priority level given the importance of this deprecation notice.
Given your PR description states this is just phase 1 of the deprecation plan (showing warnings), the lower priority might be appropriate. In phase 2, when you implement transport error responses, you might want to increase this priority.
83-85:❓ Verification inconclusive
Ensure comprehensive testing of the bridge?.outdated condition.
Ensure proper testing of this condition with various states of the bridge transport:
- When bridge is null or undefined
- When bridge exists but outdated is false
- When bridge exists and outdated is true
This will help prevent unexpected behavior when displaying the deprecation banner.
🏁 Script executed:
#!/bin/bash # Search for tests related to the bridge outdated flag rg -A 5 "bridge.*outdated" --glob "*test*"Length of output: 44
Action Required: Confirm Test Coverage for the 'bridge?.outdated' Condition
Our test search did not yield evidence of explicit tests for the following scenarios:
- When
bridgeisnullorundefined- When
bridgeexists butoutdatedisfalse- When
bridgeexists andoutdatedistrue(triggering the deprecation banner)Please verify manually (or add tests if missing) that these cases are covered to prevent unexpected behavior when displaying the deprecation banner.
83-85:❓ Verification inconclusive
Simplified bridge version checking.
You've simplified the condition for showing the Bridge deprecation notice by using just
bridge?.outdated. This is cleaner than the previous approach which likely had more complex version checking logic.However, please verify that
bridge?.outdatedis set correctly for all the versions you want to deprecate (all except 2.0.27 and 2.0.33 as mentioned in the PR objectives).
🏁 Script executed:
#!/bin/bash # Search for where bridge.outdated is set to understand the condition rg -A 5 -B 5 "bridge\.outdated" --glob "*.{ts,tsx,js,jsx}"Length of output: 60
Action Required: Manual Verification of
bridge?.outdatedFlag Assignment
The refactoring to usebridge?.outdatedfor showing the deprecation banner is indeed cleaner. However, our initial search for wherebridge.outdatedis set did not return any results. This means we couldn’t automatically confirm that the flag is correctly initialized to deprecate all bridge versions except 2.0.27 and 2.0.33. Please verify manually that:
- The
bridge?.outdatedflag is being set in the codebase and correctly reflects the intended version exclusions.- There is no hidden logic elsewhere that might override or bypass this simplified check.
mroz22
force-pushed
the
bridge-deprecation
branch
from
7ff74e6 to
3011f3b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/suite/src/views/suite/bridge-deprecated/index.tsx (1)
34-45: Unnecessary Fragment can be removed.The Fragment wrapping the Button component is unnecessary since there's only a single child element.
- bottomBarComponents={ - <> - <Button - icon="caretLeft" - variant="tertiary" - onClick={() => goToWallet()} - data-testid="@bridge/goto/wallet-index" - > - <Translation id="TR_TAKE_ME_BACK_TO_WALLET" /> - </Button> - </> - } + bottomBarComponents={ + <Button + icon="caretLeft" + variant="tertiary" + onClick={() => goToWallet()} + data-testid="@bridge/goto/wallet-index" + > + <Translation id="TR_TAKE_ME_BACK_TO_WALLET" /> + </Button> + }🧰 Tools
🪛 Biome (1.9.4)
[error] 35-44: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
packages/suite/src/components/suite/banners/SuiteBanners/SuiteBanners.tsx (1)
1-107: Consider adding tests for bridge deprecation functionality.Given that this is part of a phased approach to bridge deprecation (as mentioned in the PR objectives), it would be valuable to add tests to verify:
- That the correct banner is shown when outdated bridge versions are detected
- That the navigation to uninstallation instructions works properly
This would help ensure smooth transition through the different phases of deprecation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/connect/src/data/__tests__/transportInfo.test.ts(0 hunks)packages/connect/src/data/transportInfo.ts(1 hunks)packages/connect/src/device/TransportList.ts(1 hunks)packages/suite/src/components/suite/banners/SuiteBanners/BridgeDeprecatedBanner.tsx(2 hunks)packages/suite/src/components/suite/banners/SuiteBanners/SuiteBanners.tsx(2 hunks)packages/suite/src/components/suite/modals/ModalSwitcher/ForegroundAppModal.tsx(2 hunks)packages/suite/src/hooks/suite/usePreferredModal.ts(1 hunks)packages/suite/src/support/messages.ts(1 hunks)packages/suite/src/views/suite/bridge-deprecated/index.tsx(1 hunks)packages/transport/src/transports/bridge.ts(3 hunks)packages/urls/src/urls.ts(1 hunks)suite-common/suite-config/src/routes.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/connect/src/data/tests/transportInfo.test.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/urls/src/urls.ts
- packages/suite/src/hooks/suite/usePreferredModal.ts
- packages/connect/src/device/TransportList.ts
- packages/suite/src/components/suite/modals/ModalSwitcher/ForegroundAppModal.tsx
- packages/connect/src/data/transportInfo.ts
- suite-common/suite-config/src/routes.ts
- packages/transport/src/transports/bridge.ts
- packages/suite/src/support/messages.ts
- packages/suite/src/components/suite/banners/SuiteBanners/BridgeDeprecatedBanner.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
packages/suite/src/views/suite/bridge-deprecated/index.tsx
[error] 35-44: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: PR-check / web-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: build-deploy
- GitHub Check: EAS Update
- GitHub Check: prepare_android_test_app
- GitHub Check: transport-e2e-test
- GitHub Check: build-web
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (10)
packages/suite/src/views/suite/bridge-deprecated/index.tsx (6)
1-7: Dependencies and hooks are properly organized.The imports are structured well, separating external dependencies from internal components and hooks.
8-10: Documentation clearly states the purpose of the component.The comment effectively communicates the component's purpose - showing users how to uninstall the old standalone bridge.
11-17: Component setup is correct and follows best practices.The component uses hooks appropriately -
useDispatchfor actions anduseLayoutfor setting the layout context.
19-33: Modal content is properly localized and includes help resources.The modal effectively:
- Uses translation keys for internationalization
- Provides a link to uninstallation instructions
- Formats content appropriately with centered heading
This implementation aligns with the PR objective to notify users about non-canonical bridges.
47-50: SEO metadata is included for the modal.The component correctly includes a Metadata component for SEO purposes.
1-50:❓ Verification inconclusive
Verify the uninstallation instructions URL is accessible.
Ensure that the
UNINSTALL_BRIDGE_URLpoints to comprehensive instructions for users to uninstall non-canonical bridge versions, as mentioned in the PR comments by Hannsek regarding the need for clear uninstallation guidance.
🏁 Script executed:
#!/bin/bash # Check if the UNINSTALL_BRIDGE_URL constant is properly defined and its value rg -A 1 "UNINSTALL_BRIDGE_URL" --type tsLength of output: 581
Action: Verify Uninstallation Instructions URL Content
The constant
UNINSTALL_BRIDGE_URLis correctly imported from@trezor/urlsand defined inpackages/urls/src/urls.tswith the valuehttps://trezor.io/learn/a/what-is-trezor-bridge. This URL is used in the deprecated Bridge component as expected.
- Location:
- Definition in
packages/urls/src/urls.ts- Usage in
packages/suite/src/views/suite/bridge-deprecated/index.tsxPlease ensure that the content at this URL provides the comprehensive uninstallation guidance required for users to remove non-canonical bridge versions, as highlighted in PR comments by Hannsek. If the page does not include detailed uninstallation instructions, consider updating the URL to point to the correct documentation.
🧰 Tools
🪛 Biome (1.9.4)
[error] 35-44: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
packages/suite/src/components/suite/banners/SuiteBanners/SuiteBanners.tsx (4)
19-19: Update of import reflects the new bridge deprecation approach.The import has been updated from
UpdateBridgetoBridgeDeprecated, reflecting the change in approach from updating bridges to deprecating non-canonical versions.
83-85: Simplified condition for showing bridge deprecation banner.The condition has been simplified to directly check
bridge?.outdatedinstead of using a more complexshowUpdateBridgefunction (as mentioned in the AI summary). This makes the code more maintainable and easier to understand.The banner's priority (30) is appropriately set within the priority hierarchy of the banners system.
1-107:❓ Verification inconclusive
Confirm implementation of bridge?.outdated property.
To ensure that the
bridge?.outdatedproperty is correctly implemented and will accurately identify non-canonical bridge versions as mentioned in the PR objectives, we should verify its definition.
🏁 Script executed:
#!/bin/bash # Check how bridge.outdated is determined/set in the transport layer rg "outdated.*bridge|bridge.*outdated" --type ts | grep -v "SuiteBanners.tsx" # Look for non-canonical bridge version definitions rg "canonical.*bridge|bridge.*canonical" --type tsLength of output: 479
Confirm Bridge Outdated Implementation in Transport Layer
The grep results show references to “bridge outdated” in the connect-ui files and a comment in the transport abstraction, but the actual definition or assignment of the
bridge?.outdatedproperty isn’t clearly evident. Please manually verify that the Bridge transport implementation (especially in the transport layer such as inpackages/transport/src/transports/abstract.ts) correctly sets and exposes theoutdatedproperty to flag non-canonical versions as intended.
- Verification locations:
packages/transport/src/transports/abstract.ts(for comments/implementation details)packages/connect-ui/src/components/Notification.tsx&packages/connect-ui/src/index.tsx(usage references)
18-19:✅ Verification successful
Verify the BridgeDeprecatedBanner component implementation.
The import path for
BridgeDeprecatedis./BridgeDeprecatedBanner, but the component we reviewed is defined inpackages/suite/src/views/suite/bridge-deprecated/index.tsx. Let's ensure that there's proper connection between these files.
🏁 Script executed:
#!/bin/bash # Check the implementation of BridgeDeprecatedBanner cat packages/suite/src/components/suite/banners/SuiteBanners/BridgeDeprecatedBanner.tsx 2>/dev/null || echo "File not found" # Check if BridgeDeprecated is re-exported from another file rg -r1 "export.*BridgeDeprecated" --type tsLength of output: 1101
BridgeDeprecated Banner Connection Verified
We verified that the file at
packages/suite/src/components/suite/banners/SuiteBanners/BridgeDeprecatedBanner.tsx
exists and correctly exports theBridgeDeprecatedcomponent. The component’s implementation (including routing viagoto('suite-bridge-deprecated')) matches the expected behavior. Additionally, the similar export found in
packages/suite/src/views/suite/bridge-deprecated/index.tsx
confirms that the connection between these files is intentional and consistent.No further changes are necessary.
| @@ -42,6 +42,13 @@ export const routes = [ | |||
| isForegroundApp: true, | |||
| params: modalAppParams, | |||
| }, | |||
| { | |||
| name: 'suite-bridge-deprecated', | |||
| pattern: '/bridge-bridge', | |||
There was a problem hiding this comment.
Should the pattern really be bridge-bridge?
There was a problem hiding this comment.
that feels like a mistake
…nstall standalone bridge instead
mroz22
force-pushed
the
bridge-deprecation
branch
from
3011f3b to
3198d91
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/suite/src/views/suite/bridge-deprecated/index.tsx (1)
34-45: Remove unnecessary Fragment wrapper.The Fragment wrapper around the Button component is unnecessary since it only contains one child element.
bottomBarComponents={ - <> <Button icon="caretLeft" variant="tertiary" onClick={() => goToWallet()} data-testid="@bridge/goto/wallet-index" > <Translation id="TR_TAKE_ME_BACK_TO_WALLET" /> </Button> - </> }🧰 Tools
🪛 Biome (1.9.4)
[error] 35-44: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/suite/src/components/suite/banners/SuiteBanners/BridgeDeprecatedBanner.tsx(2 hunks)packages/suite/src/components/suite/banners/SuiteBanners/SuiteBanners.tsx(2 hunks)packages/suite/src/components/suite/modals/ModalSwitcher/ForegroundAppModal.tsx(2 hunks)packages/suite/src/hooks/suite/usePreferredModal.ts(1 hunks)packages/suite/src/support/messages.ts(1 hunks)packages/suite/src/views/suite/bridge-deprecated/index.tsx(1 hunks)packages/urls/src/urls.ts(1 hunks)suite-common/suite-config/src/routes.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/urls/src/urls.ts
- packages/suite/src/hooks/suite/usePreferredModal.ts
- suite-common/suite-config/src/routes.ts
- packages/suite/src/components/suite/banners/SuiteBanners/SuiteBanners.tsx
- packages/suite/src/components/suite/banners/SuiteBanners/BridgeDeprecatedBanner.tsx
- packages/suite/src/components/suite/modals/ModalSwitcher/ForegroundAppModal.tsx
- packages/suite/src/support/messages.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/suite/src/views/suite/bridge-deprecated/index.tsx
[error] 35-44: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: PR-check / web-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: build-deploy
- GitHub Check: EAS Update
- GitHub Check: prepare_android_test_app
- GitHub Check: transport-e2e-test
- GitHub Check: build-web
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (6)
packages/suite/src/views/suite/bridge-deprecated/index.tsx (6)
1-7: Imports organization looks good.The component correctly imports necessary dependencies and internal utilities.
8-10: Good documentation practice.The JSDoc comment clearly explains the component's purpose, which is important for maintainability.
11-16: Component setup follows best practices.The component follows React best practices by using hooks appropriately and setting up the layout with useLayout. The goToWallet function is correctly implemented using dispatch.
18-33: Well-structured modal with proper internationalization.The modal is well-structured with appropriate heading and description. The Translation component is correctly used for internationalization, and the link to uninstall instructions is properly implemented.
46-50: Good use of Metadata for SEO.Using the Metadata component to set the page title is a good practice for SEO.
1-50:❓ Verification inconclusive
Ensure the component is properly connected to routing.
This new component needs to be connected to the application's routing system to be accessible. Verify that it's properly registered in the route configuration.
🏁 Script executed:
#!/bin/bash # Check if this component is registered in the routes configuration echo "Checking for BridgeDeprecated component in routes configuration..." rg "BridgeDeprecated" --type=ts | grep -i routeLength of output: 190
Action Required: Verify BridgeDeprecated Routing Registration
It looks like our search didn't return any references to the component in the routing configuration. Please ensure that the
BridgeDeprecatedcomponent is explicitly registered within the application's route definitions. You might need to verify that the route is added (e.g., in a file likesrc/config/routes.tsor a similar routing file) so that the component becomes accessible.
- Check the routing configuration file(s) for an entry linking to
BridgeDeprecated.- Confirm that the route path (commonly something like
/bridge-deprecated) is registered.- If the component isn’t currently integrated into the routes, please add the necessary configuration.
🧰 Tools
🪛 Biome (1.9.4)
[error] 35-44: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
| import { Button, Link } from '@trezor/components'; | ||
| import { UNINSTALL_BRIDGE_URL } from '@trezor/urls'; | ||
|
|
||
| import { goto } from 'src/actions/suite/routerActions'; | ||
| import { Metadata, Modal, Translation } from 'src/components/suite'; | ||
| import { useDispatch, useLayout } from 'src/hooks/suite'; | ||
|
|
||
| /** | ||
| * modal that should show users basic information how to uninstall old standalone bridge | ||
| */ | ||
| export const BridgeDeprecated = () => { | ||
| const dispatch = useDispatch(); | ||
|
|
||
| const goToWallet = () => dispatch(goto('wallet-index')); | ||
|
|
||
| useLayout('Bridge'); | ||
|
|
||
| return ( | ||
| <Modal | ||
| heading={<Translation id="TR_YOUR_BRIDGE_VERSION_WILL_SOON_BE_DEPRECATED" />} | ||
| description={ | ||
| <Translation | ||
| id="TR_BRIDGE_UNINSTALL_INSTRUCTIONS" | ||
| values={{ | ||
| a: chunks => ( | ||
| <Link href={UNINSTALL_BRIDGE_URL} typographyStyle="hint"> | ||
| {chunks} | ||
| </Link> | ||
| ), | ||
| }} | ||
| /> | ||
| } | ||
| isHeadingCentered | ||
| bottomBarComponents={ | ||
| <> | ||
| <Button | ||
| icon="caretLeft" | ||
| variant="tertiary" | ||
| onClick={() => goToWallet()} | ||
| data-testid="@bridge/goto/wallet-index" | ||
| > | ||
| <Translation id="TR_TAKE_ME_BACK_TO_WALLET" /> | ||
| </Button> | ||
| </> | ||
| } | ||
| > | ||
| <Metadata title="Bridge | Trezor Suite" /> | ||
| </Modal> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify translation key existence and proper messaging.
Since this is a new component for bridge deprecation, it's important to ensure that the translation keys (TR_YOUR_BRIDGE_VERSION_WILL_SOON_BE_DEPRECATED and TR_BRIDGE_UNINSTALL_INSTRUCTIONS) exist in the translation files and that the messaging aligns with the PR objectives of properly instructing users about the non-canonical bridge deprecation.
🏁 Script executed:
#!/bin/bash
# Search for the translation keys to make sure they exist in the translation files
echo "Checking for translation keys in the codebase..."
rg "TR_YOUR_BRIDGE_VERSION_WILL_SOON_BE_DEPRECATED" --type=json
rg "TR_BRIDGE_UNINSTALL_INSTRUCTIONS" --type=json
# Also check for the UNINSTALL_BRIDGE_URL constant
echo "Checking for UNINSTALL_BRIDGE_URL constant..."
rg "UNINSTALL_BRIDGE_URL" --type=tsLength of output: 732
Action Required: Validate and Add Missing Translation Keys
- Our search confirmed that the
UNINSTALL_BRIDGE_URLconstant is correctly located inpackages/urls/src/urls.ts. - However, the translation keys
TR_YOUR_BRIDGE_VERSION_WILL_SOON_BE_DEPRECATEDandTR_BRIDGE_UNINSTALL_INSTRUCTIONSwere not found in the JSON translation files. Please verify that these keys exist. - Ensure that the messaging for these translation keys is in line with the PR objectives to clearly instruct users on the deprecation of the standalone bridge.
🧰 Tools
🪛 Biome (1.9.4)
[error] 35-44: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
There could be standalone bridges other than 2.0.27, 2.0.33 or built-in node bridge in the wild. The idea is to deprecate these non-canonical bridge versions to reduce our debugging surface.
Here are some figures from about 2 years ago. It would be nic to check current usage with the data team.
The plan is as follows:
This PR executes phase 1. Any non-cannonical bridge gets a notification prompting user to get rid of it.
cc @Hannsek and @MiroslavProchazka