Connect settings in Suite #17313
Connect settings in Suite #17313
Conversation
|
🚀 Expo preview is ready!
|
martykan
force-pushed
the
feat/walletconnect-mobile
branch
from
d1bac8e to
17c808d
Compare
martykan
force-pushed
the
feat/connect-settings-section
branch
from
579853e to
c839865
Compare
martykan
force-pushed
the
feat/walletconnect-mobile
branch
from
17c808d to
dd4f07b
Compare
martykan
force-pushed
the
feat/connect-settings-section
branch
from
c839865 to
d0849aa
Compare
martykan
force-pushed
the
feat/walletconnect-mobile
branch
4 times, most recently
from
8b7a734 to
24c0203
Compare
martykan
force-pushed
the
feat/connect-settings-section
branch
from
d0849aa to
9faaa65
Compare
martykan
force-pushed
the
feat/walletconnect-mobile
branch
from
24c0203 to
2c80ff6
Compare
martykan
force-pushed
the
feat/connect-settings-section
branch
from
9faaa65 to
f81d509
Compare
martykan
force-pushed
the
feat/walletconnect-mobile
branch
5 times, most recently
from
7b8fbf4 to
9d1158e
Compare
martykan
force-pushed
the
feat/connect-settings-section
branch
from
f81d509 to
551be44
Compare
martykan
force-pushed
the
feat/connect-settings-section
branch
from
551be44 to
fd7141a
Compare
WalkthroughThe changes implement a new settings section for connected apps across the application. A new component, 🪧 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: 3
🧹 Nitpick comments (11)
suite-common/walletconnect/src/walletConnectTypes.ts (1)
23-23: Good addition of validation field to WalletConnectSession interfaceThe new
validationfield is a good addition that will enable tracking the validation state of WalletConnect sessions, which aligns with the PR objective of creating a "Connected apps" section to manage connections.However, I noticed that the order of union values differs from the similar field in the
PendingConnectionProposalinterface (line 55):
- Here:
'VALID' | 'INVALID' | 'UNKNOWN'- Line 55:
'UNKNOWN' | 'VALID' | 'INVALID'Consider standardizing the order of union values across both interfaces for consistency and better maintainability:
- validation: 'VALID' | 'INVALID' | 'UNKNOWN'; + validation: 'UNKNOWN' | 'VALID' | 'INVALID';packages/suite/src/views/settings/SettingsConnectedApps/WalletConnectButton.tsx (2)
22-54: Accessible modal implementationThe modal implementation is well-structured with appropriate translations, but consider adding autofocus to the input field for better user experience.
<Input + autoFocus value={connectionUrl} onChange={e => setConnectionUrl(e.target.value)} placeholder={translationString( 'TR_WALLETCONNECT_ADD_CONNECTION_PLACEHOLDER', )} />
45-51: Consider adding basic URL validationWhile the WalletConnect URI format validation might be handled in the thunk, adding basic validation in the UI can provide immediate feedback to users.
<Input value={connectionUrl} onChange={e => setConnectionUrl(e.target.value)} placeholder={translationString( 'TR_WALLETCONNECT_ADD_CONNECTION_PLACEHOLDER', )} + state={connectionUrl && !connectionUrl.startsWith('wc:') ? 'error' : undefined} + bottomText={connectionUrl && !connectionUrl.startsWith('wc:') + ? translationString('TR_WALLETCONNECT_INVALID_URL') + : undefined} />You would need to add the "TR_WALLETCONNECT_INVALID_URL" translation string to your translation files.
packages/suite/src/views/settings/SettingsConnectedApps/SettingsConnectedApps.tsx (2)
26-26: Verify tab state initializationThere's a small typo in the state variable name
activeItemdId(should beactiveItemId). While it works as is since the same variable name is used consistently, it would be cleaner to fix this.- const [activeItemdId, setActiveItemId] = useState(tabs[0].id); + const [activeItemId, setActiveItemId] = useState(tabs[0].id);
52-52: Verify tab component renderingWhen using
find()to locate the active tab component, consider adding a fallback in case no matching tab is found. While unlikely in this controlled environment, it would improve robustness.- {tabs.find(tab => tab.id === activeItemdId)?.component} + {tabs.find(tab => tab.id === activeItemdId)?.component || tabs[0].component}packages/suite/src/views/settings/SettingsConnectedApps/ConnectPermissions.tsx (2)
40-40: Consider improving permission display format.The permissions are currently displayed as a comma-separated list, which might not be user-friendly if the permission identifiers are technical terms. Consider mapping these to human-readable descriptions or adding translations.
-<Text variant="tertiary">Permissions: {app.types.join(', ')}</Text> +<Text variant="tertiary"> + <Translation id="TR_PERMISSIONS" />: {app.types.map(type => ( + <Translation key={type} id={`TR_PERMISSION_${type}`} /> + )).reduce((prev, curr) => [prev, ', ', curr])} +</Text>
43-61: Add confirmation dialog before forgetting permissions.Consider implementing a confirmation dialog before removing app permissions to prevent accidental removals, especially since this action could disrupt the user's workflow with external applications.
<Dropdown placement={{ position: 'bottom', alignment: 'end' }} items={[ { key: 'group1', options: [ { icon: 'xCircle', label: <Translation id="TR_FORGET" />, onClick: () => { + // Show confirmation dialog before forgetting + if (window.confirm( + `Are you sure you want to forget permissions for ${app.origin}?` + )) { dispatch( connectPopupActions.forgetAppPermissions(app), ); + } }, }, ], }, ]} />packages/suite/src/views/settings/SettingsConnectedApps/WalletConnectList.tsx (1)
59-79: Add confirmation dialog before disconnecting sessions.Consider implementing a confirmation dialog before disconnecting a session to prevent accidental disconnections, as this would disrupt the user's connection to external applications.
<Dropdown placement={{ position: 'bottom', alignment: 'end' }} items={[ { key: 'group1', options: [ { icon: 'xCircle', label: <Translation id="TR_DISCONNECT" />, onClick: () => { + // Show confirmation dialog before disconnecting + if (window.confirm( + `Are you sure you want to disconnect from ${session.peer.metadata.name}?` + )) { dispatch( walletConnectDisconnectThunk({ topic: session.topic, }), ); + } }, }, ], }, ]} />packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/ConnectPopupModal.tsx (2)
18-29: Consider additional validation for permission types.The current implementation correctly checks for
isRemembered && processName && originbefore dispatching the remember permissions action. However, there's no validation thatpermissionTypesexists or is non-empty.- const onConfirm = () => { - if (isRemembered && processName && origin) + const onConfirm = () => { + if (isRemembered && processName && origin && permissionTypes?.length) dispatch( connectPopupActions.rememberAppPermissions({ processName, origin, types: permissionTypes, }), ); dispatch(connectPopupActions.approveCall()); };
72-79: Consider showing more details about permissions being remembered.The checkbox allows users to remember permissions, but it doesn't show what specific permissions will be remembered. Consider adding a tooltip or expanding the UI to display the specific permissions being granted.
Also, consider adding a conditional check to only show the checkbox when both
processNameandoriginare present, since these are required for remembering permissions.-{processName !== 'WalletConnect' && ( +{processName !== 'WalletConnect' && processName && origin && ( <Checkbox isChecked={isRemembered} onClick={() => setIsRemembered(!isRemembered)} > <Translation id="TR_CONNECT_MODAL_REMEMBER" /> + {permissionTypes?.length > 0 && ( + <Tooltip content={permissionTypes.join(', ')}> + <InfoIcon size="tiny" /> + </Tooltip> + )} </Checkbox> )}suite-common/connect-popup/src/connectPopupThunks.ts (1)
74-91: Consider adding permission expiration mechanism.While the implementation correctly skips confirmation for remembered apps, there's no mechanism for expiring permissions after a certain time period or number of uses. This could be a security concern for sensitive operations.
Consider adding an expiration timestamp or usage counter to the
AppRememberedPermissiontype and checking it here before skipping the confirmation dialog.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
packages/suite-desktop-ui/src/support/Router.tsx(2 hunks)packages/suite-web/src/support/Router.tsx(1 hunks)packages/suite/src/actions/suite/storageActions.ts(1 hunks)packages/suite/src/components/settings/SettingsLayout.tsx(1 hunks)packages/suite/src/components/suite/StatusLight.tsx(3 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/MobileMenu/MobileMenuActions.tsx(1 hunks)packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Navigation.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/ConnectPopupModal.tsx(2 hunks)packages/suite/src/middlewares/wallet/storageMiddleware.ts(2 hunks)packages/suite/src/storage/definitions.ts(2 hunks)packages/suite/src/support/messages.ts(4 hunks)packages/suite/src/views/settings/SettingsConnectedApps/ConnectPermissions.tsx(1 hunks)packages/suite/src/views/settings/SettingsConnectedApps/SettingsConnectedApps.tsx(1 hunks)packages/suite/src/views/settings/SettingsConnectedApps/WalletConnectButton.tsx(1 hunks)packages/suite/src/views/settings/SettingsConnectedApps/WalletConnectList.tsx(1 hunks)packages/suite/src/views/settings/SettingsDebug/SettingsDebug.tsx(0 hunks)packages/suite/src/views/settings/SettingsDebug/WalletConnect.tsx(0 hunks)suite-common/connect-popup/src/connectPopupActions.ts(2 hunks)suite-common/connect-popup/src/connectPopupReducer.ts(3 hunks)suite-common/connect-popup/src/connectPopupThunks.ts(2 hunks)suite-common/connect-popup/src/connectPopupTypes.ts(3 hunks)suite-common/suite-config/src/routes.ts(1 hunks)suite-common/walletconnect/src/walletConnectThunks.ts(1 hunks)suite-common/walletconnect/src/walletConnectTypes.ts(1 hunks)
💤 Files with no reviewable changes (2)
- packages/suite/src/views/settings/SettingsDebug/SettingsDebug.tsx
- packages/suite/src/views/settings/SettingsDebug/WalletConnect.tsx
🧰 Additional context used
🧬 Code Definitions (10)
packages/suite-web/src/support/Router.tsx (1)
packages/suite/src/views/settings/SettingsConnectedApps/SettingsConnectedApps.tsx (1) (1)
SettingsConnectedApps(13-58)
packages/suite-desktop-ui/src/support/Router.tsx (1)
packages/suite/src/views/settings/SettingsConnectedApps/SettingsConnectedApps.tsx (1) (1)
SettingsConnectedApps(13-58)
packages/suite/src/middlewares/wallet/storageMiddleware.ts (1)
suite-common/connect-popup/src/connectPopupActions.ts (1) (1)
connectPopupActions(33-40)
packages/suite/src/views/settings/SettingsConnectedApps/WalletConnectList.tsx (1)
suite-common/walletconnect/src/walletConnectThunks.ts (1) (1)
walletConnectDisconnectThunk(371-377)
suite-common/connect-popup/src/connectPopupActions.ts (1)
suite-common/connect-popup/src/connectPopupTypes.ts (1) (1)
AppRememberedPermission(28-34)
packages/suite/src/views/settings/SettingsConnectedApps/SettingsConnectedApps.tsx (4)
packages/suite/src/views/settings/SettingsConnectedApps/ConnectPermissions.tsx (1) (1)
ConnectPermissions(8-67)packages/suite/src/views/settings/SettingsConnectedApps/WalletConnectList.tsx (1) (1)
WalletConnectList(19-85)packages/suite/src/components/settings/SettingsLayout.tsx (1) (1)
SettingsLayout(79-94)packages/suite/src/views/settings/SettingsConnectedApps/WalletConnectButton.tsx (1) (1)
WalletConnectButton(9-66)
suite-common/connect-popup/src/connectPopupReducer.ts (2)
suite-common/connect-popup/src/connectPopupTypes.ts (2) (2)
ConnectPopupCall(5-26)AppRememberedPermission(28-34)suite-common/connect-popup/src/connectPopupActions.ts (1) (1)
connectPopupActions(33-40)
suite-common/connect-popup/src/connectPopupThunks.ts (2)
suite-common/connect-popup/src/connectPopupReducer.ts (1) (1)
selectConnectAppPermissions(52-53)suite-common/connect-popup/src/connectPopupActions.ts (1) (1)
connectPopupActions(33-40)
packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/ConnectPopupModal.tsx (2)
suite-common/connect-popup/src/connectPopupReducer.ts (1) (1)
selectConnectPopupCall(49-50)suite-common/connect-popup/src/connectPopupActions.ts (1) (1)
connectPopupActions(33-40)
packages/suite/src/views/settings/SettingsConnectedApps/WalletConnectButton.tsx (1)
suite-common/walletconnect/src/walletConnectThunks.ts (1) (1)
walletConnectPairThunk(353-369)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: run-e2e-suite-web-tests (@group=metadata2, trezor-user-env-unix)
- GitHub Check: Linting and formatting
- GitHub Check: Type Checking
- GitHub Check: Unit Tests
- GitHub Check: run-e2e-suite-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: prepare_android_test_app
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (38)
suite-common/walletconnect/src/walletConnectThunks.ts (1)
223-223:❓ Verification inconclusive
Validation state correctly propagated from proposal to session
This change correctly propagates the validation state from the pending proposal to the saved session, which is consistent with the interface change in
walletConnectTypes.ts.I notice that when populating active sessions (lines 337-344), there's no explicit handling for the
validationproperty. This could result in undefined validation values for existing sessions that were created before this property was added.
🏁 Script executed:
#!/bin/bash # Check how active sessions are saved and verify if validation is handled for existing sessions # Find where sessions are saved or updated in the codebase rg -A 3 -B 3 "walletConnectActions.saveSession" --type tsLength of output: 1975
Attention: Active sessions may lack explicit validation handling
- The change correctly propagates the
validationproperty from pending proposals when saving new sessions (line 223).- However, when populating active sessions (lines 337-344 in
walletConnectThunks.ts), the action is invoked with a simple spread ({ ...sessions[topic] }) and does not explicitly assign thevalidationproperty.- As a result, sessions that were created before the addition of the validation property could end up with an undefined
validationvalue.- Please verify if these legacy sessions should default to a specific validation state or if additional handling is necessary when loading active sessions.
packages/suite/src/components/suite/layouts/SuiteLayout/MobileMenu/MobileMenuActions.tsx (1)
104-104: Adding new route to active settings check - looks goodThe addition of 'settings-connected-apps' to the array of routes checked for active status is consistent with the existing pattern and ensures the settings menu item will be properly highlighted when navigating to the connected apps section.
suite-common/suite-config/src/routes.ts (1)
100-104: New route for connected apps settings - LGTMThe new route follows the established pattern for settings routes, with the appropriate name, pattern, and app association. The placement within the routes array is logical, positioned among other settings routes.
packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/Navigation.tsx (1)
27-27: Addition to SETTINGS_ROUTES - looks goodThe new route has been properly added to the SETTINGS_ROUTES constant, ensuring that the Settings navigation item will be marked as active when viewing the connected apps section.
packages/suite/src/storage/definitions.ts (2)
6-6: Imported AppRememberedPermission type - LGTMThe import is correctly added to support the new database schema property.
142-147: New database schema for storing connect permissions - LGTMThe new
connectproperty in the SuiteDBSchema follows the established pattern of other schema properties and provides appropriate structure for storing app permissions. This enables persistence of connected app permissions across sessions.packages/suite-desktop-ui/src/support/Router.tsx (2)
10-10: Clean import of the SettingsConnectedApps componentThe import statement is correctly added, following the same pattern as other settings components in the file.
75-75: Successfully registered the SettingsConnectedApps routeThe new route is properly added to the components object, maintaining consistency with the existing route registration pattern.
packages/suite-web/src/support/Router.tsx (1)
151-155: Well-implemented lazy loading for the SettingsConnectedApps componentThe implementation correctly uses lazy loading with the appropriate webpack chunk name, following the same pattern as other settings components. This ensures the component is only loaded when needed, improving initial load performance.
packages/suite/src/middlewares/wallet/storageMiddleware.ts (2)
4-5: Added necessary import for connect popup actionsThe import is correctly added to access the required action types.
220-227:✅ Verification successful
Proper persistence implementation for app permissions
The middleware correctly detects when app permissions are modified and triggers storage actions to persist these changes, following the same pattern as other persistent data in the application.
🏁 Script executed:
#!/bin/bash # Check if saveConnectSettings function is properly implemented rg -A 10 "saveConnectSettings" --glob "*.ts" --glob "*.tsx"Length of output: 2173
Actionable: Verify and confirm persistence for app permissions.
The middleware now correctly detects changes in app permissions by calling
storageActions.saveConnectSettings(), and the implementation of this function (located inpackages/suite/src/actions/suite/storageActions.ts) properly persists the app permissions data to the database. There are no issues identified with the persistence mechanism, as the same pattern is used consistently across the application.packages/suite/src/views/settings/SettingsConnectedApps/WalletConnectButton.tsx (4)
1-8: Well-structured importsThe imports are organized logically, separating React, external dependencies, and internal components.
9-14: Clean component setup with proper state managementThe component correctly uses React hooks for state management and accesses the dispatch function and translations.
15-21: Well-implemented connection handlingThe connection handler dispatches the appropriate action, clears the input, and closes the modal. The cancel function is also implemented correctly.
56-64: Well-implemented button for opening the modalThe button implementation is clean and follows the design pattern of the application.
packages/suite/src/actions/suite/storageActions.ts (1)
467-472: New storage action to persist connect permissionsThe new
saveConnectSettingsfunction follows the established pattern for database operations in this file. It appropriately checks database accessibility before attempting to store data and retrieves the necessary state from Redux.suite-common/connect-popup/src/connectPopupTypes.ts (3)
2-2: LGTM - Required import for type safetyThe import of
MethodPermissiontype from Trezor Connect's core module is necessary for the permission management feature.
13-13: LGTM - Permission types tracking enhances securityAdding the
permissionTypesfield to track what permissions are requested by the app is a good security enhancement.
28-34: Well-structured type for app permissions storageThis new type provides a clear structure for storing application permission data, including app metadata and granted permissions. The types are comprehensive and support both Trezor Connect and WalletConnect implementations.
packages/suite/src/components/suite/StatusLight.tsx (3)
6-6: LGTM - Added info variant for status indicatorsAdding the 'info' variant extends the component's capabilities to represent different states of connected apps.
18-18: LGTM - Consistent background color for info variantThe background color mapping for the 'info' variant follows the established pattern of using semantic color tokens.
29-29: LGTM - Inner color mapping for info variantThe inner color mapping for the 'info' variant is appropriately defined, completing the visual styling.
packages/suite/src/views/settings/SettingsConnectedApps/SettingsConnectedApps.tsx (3)
13-26: Well-structured tabbed interface for connected appsThe tabs implementation cleanly separates the two different types of connections (Trezor Connect and WalletConnect) with appropriate components for each. This maintains a consistent UI pattern with other settings pages.
29-36: LGTM - Clean header with appropriate actionsThe header layout with the title and the WalletConnect button is well-organized. The button placement provides good discoverability for the add connection action.
37-54: LGTM - Well-structured card and tabs layoutThe card layout with nested columns and tabs follows the established UI patterns. The tabs implementation with click handlers and conditional content rendering is clean and effective.
packages/suite/src/components/settings/SettingsLayout.tsx (1)
51-58: LGTM: New Connected Apps settings menu entry.The addition of the Connected Apps navigation item is implemented correctly, following the established pattern for settings pages. It's notable that this feature is currently gated behind the debug mode flag, which is consistent with the existing pattern for testing features before broader release.
packages/suite/src/views/settings/SettingsConnectedApps/ConnectPermissions.tsx (1)
8-20: LGTM: Empty state handling looks good.The component properly handles the case when no connected apps are present by displaying an appropriate message.
suite-common/connect-popup/src/connectPopupActions.ts (3)
3-3: LGTM: Appropriate import of the AppRememberedPermission type.The action creators now correctly import the type needed for the payload.
19-31: LGTM: Well-structured action creators.The new action creators for managing app permissions follow the existing pattern and are well-implemented using Redux Toolkit's createAction.
38-39: LGTM: Updated exports.The actions are properly exported in the connectPopupActions object.
packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/ConnectPopupModal.tsx (2)
12-13: Good implementation of permission persistence functionality.Adding the state variable for remembering app permissions is a good approach as it allows users to avoid having to repeatedly approve permissions for the same app.
50-70: Improved layout with Column component.Using the Column component for layout and structuring the modal content improves readability and maintainability.
suite-common/connect-popup/src/connectPopupThunks.ts (3)
7-7: Added imports for permission handling.The new imports for
MethodPermissionandselectConnectAppPermissionsare necessary to support the permission checking functionality.Also applies to: 12-12
63-72: Well-implemented permission checking logic.The permission checking logic is thorough, verifying not only if the app (by origin and processName) is remembered but also if all required permissions are already granted.
87-87: LGTM: Adding permissionTypes to initiateCall.Adding the
permissionTypesto the initiateCall action payload is a good change that allows the modal to display and remember the specific permissions being requested.suite-common/connect-popup/src/connectPopupReducer.ts (2)
4-4: State structure updated to support permission storage.The
ConnectPopupStatetype and initial state have been correctly updated to include thepermissionsarray for storing remembered app permissions.Also applies to: 8-8, 17-17
52-53: LGTM: Added selector for app permissions.The added selector function
selectConnectAppPermissionsis a clean and simple way to access the permissions array from the state.packages/suite/src/support/messages.ts (1)
3544-3547: New message definitions for connected apps feature look good.These message definitions properly support the new "Connected apps" section being implemented, providing localized strings for UI elements related to Trezor Connect and WalletConnect functionality. The additions follow the established pattern in the codebase for defining localized messages.
Also applies to: 9602-9605, 9643-9655, 9677-9688
packages/suite/src/views/settings/SettingsConnectedApps/WalletConnectList.tsx
Show resolved
Hide resolved
packages/suite/src/views/settings/SettingsConnectedApps/WalletConnectList.tsx
Show resolved
Hide resolved
martykan
force-pushed
the
feat/connect-settings-section
branch
from
fd7141a to
d491354
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (11)
packages/suite/src/views/settings/SettingsConnectedApps/WalletConnectButton.tsx (3)
15-19: Consider adding loading state feedback during connectionThe connection process might take some time, but there's no loading indicator to inform the user that something is happening after they click connect.
const handleConnect = () => { + const connectionInProgress = true; // Add loading state dispatch(walletConnectPairThunk({ uri: connectionUrl })); setConnectionUrl(''); // Clear input after attempt setModalOpened(false); };Consider tracking a loading state and displaying a spinner or disabling the button during connection to improve user experience.
45-51: Add validation for WalletConnect URI formatThe input field accepts any string, but WalletConnect URIs typically follow a specific format starting with "wc:".
<Input value={connectionUrl} onChange={e => setConnectionUrl(e.target.value)} + error={connectionUrl && !connectionUrl.startsWith('wc:') ? 'Invalid WalletConnect URI format' : undefined} placeholder={translationString( 'TR_WALLETCONNECT_ADD_CONNECTION_PLACEHOLDER', )} />
1-66: Add accessibility attributes to improve inclusivityThe component lacks important accessibility attributes which are essential for users relying on screen readers or keyboard navigation.
Consider adding:
aria-labelto the Input fieldaria-describedbyfor error messages- Keyboard handling (Enter key submission in the input field)
Example implementation for the Input component:
<Input value={connectionUrl} onChange={e => setConnectionUrl(e.target.value)} + aria-label={translationString('TR_WALLETCONNECT_ADD_CONNECTION_PLACEHOLDER')} + onKeyDown={e => e.key === 'Enter' && connectionUrl && handleConnect()} placeholder={translationString( 'TR_WALLETCONNECT_ADD_CONNECTION_PLACEHOLDER', )} />packages/suite/src/views/settings/SettingsConnectedApps/SettingsConnectedApps.tsx (4)
26-26: Fix typo in variable nameactiveItemdIdThere's a typo in the variable name - "d" is duplicated between "Item" and "Id".
-const [activeItemdId, setActiveItemId] = useState(tabs[0].id); +const [activeItemId, setActiveItemId] = useState(tabs[0].id);
40-40: Update reference to the activeItemId variableThis needs to match the corrected variable name from the previous fix.
-<Tabs size="large" activeItemId={activeItemdId} hasBorder={false}> +<Tabs size="large" activeItemId={activeItemId} hasBorder={false}>
52-52: Update reference to the activeItemId variableThis also needs to match the corrected variable name.
-{tabs.find(tab => tab.id === activeItemdId)?.component} +{tabs.find(tab => tab.id === activeItemId)?.component}
13-58: Consider persisting tab selection across user sessionsCurrently, the component always defaults to the first tab when mounted. For better user experience, consider persisting the user's last selected tab.
You could use local storage or the Redux store to remember the user's preference:
+import { useEffect } from 'react'; +import { analytics, EventType } from '@trezor/suite-analytics'; export const SettingsConnectedApps = () => { const tabs = [ // ... tabs definition ]; - const [activeItemId, setActiveItemId] = useState(tabs[0].id); + const [activeItemId, setActiveItemId] = useState(() => { + const savedTab = localStorage.getItem('settingsConnectedAppsTab'); + return savedTab && tabs.some(tab => tab.id === savedTab) ? savedTab : tabs[0].id; + }); + + useEffect(() => { + localStorage.setItem('settingsConnectedAppsTab', activeItemId); + }, [activeItemId]); + + const handleTabChange = (tabId: string) => { + setActiveItemId(tabId); + analytics.report({ + type: EventType.SettingsConnectedAppsTabChange, + payload: { tab: tabId }, + }); + } // ... in the JSX update the onClick handler - onClick={() => setActiveItemId(tab.id)} + onClick={() => handleTabChange(tab.id)}packages/suite/src/views/settings/SettingsConnectedApps/ConnectPermissions.tsx (4)
40-40: Use translation component for "Permissions:" textThe "Permissions:" text is hardcoded instead of using the Translation component like other text in the file.
-<Text variant="tertiary">Permissions: {app.types.join(', ')}</Text> +<Text variant="tertiary"> + <Translation id="TR_PERMISSIONS" />: {app.types.join(', ')} +</Text>Make sure to add the "TR_PERMISSIONS" translation key to the messages file.
12-20: Enhance empty state with guidance for usersThe empty state shows that there are no connected apps but doesn't guide users on how to connect one.
if (apps.length === 0) { return ( - <Row padding={spacings.xl} justifyContent="center"> - <Text align="center" variant="tertiary"> - <Translation id="TR_NO_CONNECTED_APPS" /> - </Text> + <Row padding={spacings.xl} justifyContent="center" flexDirection="column" gap={spacings.md}> + <Text align="center" variant="tertiary"> + <Translation id="TR_NO_CONNECTED_APPS" /> + </Text> + <Text align="center" variant="secondary"> + <Translation id="TR_CONNECT_APPS_INSTRUCTION" /> + </Text> </Row> ); }You'll need to add the new translation key "TR_CONNECT_APPS_INSTRUCTION" with appropriate instructions for users.
49-58: Add user feedback when forgetting app permissionsCurrently, there's no feedback when a user forgets an app's permissions. Consider showing a toast notification.
onClick: () => { dispatch( connectPopupActions.forgetAppPermissions(app), ); + dispatch( + notificationsActions.addToast({ + type: 'success', + message: `Permissions for ${app.origin} were removed successfully.`, + }), + ); },Remember to import
notificationsActionsfrom the appropriate location.
25-63: Improve display of permission types for better readabilityThe current implementation displays permission types as a simple comma-separated list, which might not be very user-friendly for non-technical users.
Consider using a more descriptive format or even individual badges for each permission type:
-<Text variant="tertiary">Permissions: {app.types.join(', ')}</Text> +<Text variant="tertiary"><Translation id="TR_PERMISSIONS" />:</Text> +<Row gap={spacings.xs} flexWrap="wrap" marginTop={spacings.xs}> + {app.types.map(type => ( + <Badge key={type} variant="secondary"> + {mapPermissionTypeToReadableName(type)} + </Badge> + ))} +</Row>You would also need to implement a
mapPermissionTypeToReadableNamefunction that converts technical permission types to user-friendly names.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/suite/src/components/settings/SettingsLayout.tsx(2 hunks)packages/suite/src/components/suite/StatusLight.tsx(3 hunks)packages/suite/src/support/messages.ts(4 hunks)packages/suite/src/views/settings/SettingsConnectedApps/ConnectPermissions.tsx(1 hunks)packages/suite/src/views/settings/SettingsConnectedApps/SettingsConnectedApps.tsx(1 hunks)packages/suite/src/views/settings/SettingsConnectedApps/WalletConnectButton.tsx(1 hunks)packages/suite/src/views/settings/SettingsConnectedApps/WalletConnectList.tsx(1 hunks)suite-common/connect-popup/src/connectPopupTypes.ts(3 hunks)suite-common/walletconnect/src/walletConnectThunks.ts(1 hunks)suite-common/walletconnect/src/walletConnectTypes.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- suite-common/walletconnect/src/walletConnectThunks.ts
- suite-common/walletconnect/src/walletConnectTypes.ts
- suite-common/connect-popup/src/connectPopupTypes.ts
- packages/suite/src/views/settings/SettingsConnectedApps/WalletConnectList.tsx
- packages/suite/src/components/suite/StatusLight.tsx
- packages/suite/src/components/settings/SettingsLayout.tsx
🧰 Additional context used
🧬 Code Definitions (2)
packages/suite/src/views/settings/SettingsConnectedApps/SettingsConnectedApps.tsx (4)
packages/suite/src/views/settings/SettingsConnectedApps/ConnectPermissions.tsx (1) (1)
ConnectPermissions(8-67)packages/suite/src/views/settings/SettingsConnectedApps/WalletConnectList.tsx (1) (1)
WalletConnectList(19-85)packages/suite/src/components/settings/SettingsLayout.tsx (1) (1)
SettingsLayout(80-95)packages/suite/src/views/settings/SettingsConnectedApps/WalletConnectButton.tsx (1) (1)
WalletConnectButton(9-66)
packages/suite/src/views/settings/SettingsConnectedApps/WalletConnectButton.tsx (1)
suite-common/walletconnect/src/walletConnectThunks.ts (1) (1)
walletConnectPairThunk(353-369)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: prepare_android_test_app
- GitHub Check: EAS Update
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (8)
packages/suite/src/support/messages.ts (8)
3544-3547: LGTM! New message added for Connected apps section.The message key
TR_CONNECTED_APPShas been appropriately added with a clear default message, which will be used for the new Connected apps section mentioned in the PR objectives.
3602-3605: LGTM! Message added for the Connect modal's "remember" checkbox.This message will be used in the Connect popup modal to provide users with an option to remember app permissions, as mentioned in the PR summary.
3643-3646: LGTM! New message for WalletConnect connection action.The message key
TR_WALLETCONNECT_ADD_CONNECTIONproperly replaces the previousTR_WALLETCONNECT_NEW_CONNECTION_URLmentioned in the summary, with a clearer action-oriented label.
3647-3651: LGTM! Descriptive message for WalletConnect connection.This message provides helpful context to users when manually adding a WalletConnect connection.
3652-3655: LGTM! Placeholder message for WalletConnect input.This message will be used as placeholder text in the input field when adding a WalletConnect connection.
3677-3680: LGTM! Empty state message added.This message will be shown when there are no connected apps, providing a clear empty state indication for users.
3681-3684: LGTM! Feature name message added.Adding a dedicated message for the "WalletConnect" feature name allows for consistent labeling throughout the application.
3685-3688: LGTM! Action button message added.The message key
TR_FORGETprovides a clear action label for removing connected apps or sessions.
martykan
force-pushed
the
feat/connect-settings-section
branch
from
b7de581 to
d491354
Compare
|
QA OK
Info:
|
Description
New "Connected apps" section in Suite for Trezor Connect and WalletConnect.
Screenshots: