From d9ccb20ebd0de447b97e91da559e6ece232f8b6a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Edgars=20Egl=C4=ABtis?= <37242620+eglitise@users.noreply.github.com> Date: Sun, 18 Aug 2024 20:00:04 +0300 Subject: [PATCH] feat: improve UX for attaching to existing session (#1607) * fix: do not set attachSessId to first session if multiple are found * feat: auto-refresh session list when switching to attach to session tab also remove session retrieval anywhere outside this tab * chore: remove unused import * fix: add tooltip to button for reloading session discovery * chore: run prettier * feat: show more details for discovered sessions also reverse the list to show most recent sessions first * docs: update attach to session docs * chore: revert session discovery reload button changes * fix: do not add platformVersion if there is no platformName * chore: rename vars for readability * fix: reset attachSessId for Sauce or error * chore: address review comments * chore: use an OOP approach * chore: add consts for session builder tabs * chore: add consts for server types * chore: move session info assembly to utils * test: add unit tests for session info assembly * chore: refactor super() call --- app/common/renderer/actions/Session.js | 106 +++++------ .../Session/AdvancedServerParams.jsx | 9 +- .../components/Session/AttachToSession.jsx | 65 ++----- .../components/Session/CloudProviders.jsx | 173 ++++++------------ .../components/Session/SavedSessions.jsx | 4 +- .../renderer/components/Session/Session.jsx | 22 ++- .../renderer/constants/session-builder.js | 25 +++ app/common/renderer/reducers/Session.js | 64 ++----- .../renderer/utils/attaching-to-session.js | 68 +++++++ .../attach-to-session/attach-to-session.png | Bin 44003 -> 44409 bytes .../attach-to-session/found-session.png | Bin 10075 -> 0 bytes .../attach-to-session/found-sessions.png | Bin 0 -> 24232 bytes docs/session-builder/attach-to-session.md | 16 +- test/unit/utils-attaching-to-session.spec.js | 62 +++++++ 14 files changed, 319 insertions(+), 295 deletions(-) create mode 100644 app/common/renderer/utils/attaching-to-session.js delete mode 100644 docs/session-builder/assets/images/attach-to-session/found-session.png create mode 100644 docs/session-builder/assets/images/attach-to-session/found-sessions.png create mode 100644 test/unit/utils-attaching-to-session.spec.js diff --git a/app/common/renderer/actions/Session.js b/app/common/renderer/actions/Session.js index dd4d980268..69b4ef1761 100644 --- a/app/common/renderer/actions/Session.js +++ b/app/common/renderer/actions/Session.js @@ -1,15 +1,6 @@ import {notification} from 'antd'; import axios from 'axios'; -import { - debounce, - includes, - isPlainObject, - isUndefined, - keys, - toPairs, - union, - without, -} from 'lodash'; +import {includes, isPlainObject, isUndefined, toPairs, union, values, without} from 'lodash'; import moment from 'moment'; import {v4 as UUID} from 'uuid'; import {Web2Driver} from 'web2driver'; @@ -21,8 +12,8 @@ import { SESSION_SERVER_TYPE, VISIBLE_PROVIDERS, } from '../../shared/setting-defs'; +import {SERVER_TYPES, SESSION_BUILDER_TABS} from '../constants/session-builder'; import {APP_MODE} from '../constants/session-inspector'; -import CloudProviders from '../components/Session/CloudProviders.jsx'; import i18n from '../i18next'; import {fs, ipcRenderer, util, getSetting, setSetting} from '../polyfills'; import {log} from '../utils/logger'; @@ -106,15 +97,6 @@ const NEW_COMMAND_TIMEOUT_SEC = 3600; let isFirstRun = true; // we only want to auto start a session on a first run -const serverTypes = {}; -for (const key of keys(CloudProviders)) { - serverTypes[key] = key; -} -serverTypes.local = 'local'; -serverTypes.remote = 'remote'; - -export const ServerTypes = serverTypes; - export const DEFAULT_SERVER_PATH = '/'; export const DEFAULT_SERVER_HOST = '127.0.0.1'; export const DEFAULT_SERVER_PORT = 4723; @@ -255,7 +237,7 @@ export function newSession(caps, attachSessId = null) { desiredCapabilities = addCustomCaps(desiredCapabilities); switch (session.serverType) { - case ServerTypes.local: + case SERVER_TYPES.LOCAL: host = session.server.local.hostname; if (host === '0.0.0.0') { // if we're on windows, we won't be able to connect directly to '0.0.0.0' @@ -265,13 +247,13 @@ export function newSession(caps, attachSessId = null) { } port = session.server.local.port; break; - case ServerTypes.remote: + case SERVER_TYPES.REMOTE: host = session.server.remote.hostname; port = session.server.remote.port; path = session.server.remote.path; https = session.server.remote.ssl; break; - case ServerTypes.sauce: + case SERVER_TYPES.SAUCE: path = '/wd/hub'; host = `ondemand.${session.server.sauce.dataCenter}.saucelabs.com`; port = 80; @@ -294,7 +276,7 @@ export function newSession(caps, attachSessId = null) { desiredCapabilities[SAUCE_OPTIONS_CAP].name = `Appium Desktop Session -- ${dateTime}`; } break; - case ServerTypes.headspin: { + case SERVER_TYPES.HEADSPIN: { let headspinUrl; try { headspinUrl = new URL(session.server.headspin.webDriverUrl); @@ -310,7 +292,7 @@ export function newSession(caps, attachSessId = null) { headspinUrl.port === '' ? (https ? 443 : 80) : headspinUrl.port; break; } - case ServerTypes.perfecto: + case SERVER_TYPES.PERFECTO: host = session.server.perfecto.hostname; port = session.server.perfecto.port || (session.server.perfecto.ssl ? 443 : 80); token = session.server.perfecto.token || process.env.PERFECTO_TOKEN; @@ -322,7 +304,7 @@ export function newSession(caps, attachSessId = null) { desiredCapabilities['perfecto:options'] = {securityToken: token}; https = session.server.perfecto.ssl; break; - case ServerTypes.browserstack: + case SERVER_TYPES.BROWSERSTACK: host = session.server.browserstack.hostname = process.env.BROWSERSTACK_HOST || 'hub-cloud.browserstack.com'; port = session.server.browserstack.port = process.env.BROWSERSTACK_PORT || 443; @@ -339,7 +321,7 @@ export function newSession(caps, attachSessId = null) { } https = session.server.browserstack.ssl = parseInt(port, 10) === 443; break; - case ServerTypes.lambdatest: + case SERVER_TYPES.LAMBDATEST: host = session.server.lambdatest.hostname = process.env.LAMBDATEST_HOST || 'mobile-hub.lambdatest.com'; port = session.server.lambdatest.port = process.env.LAMBDATEST_PORT || 443; @@ -369,7 +351,7 @@ export function newSession(caps, attachSessId = null) { } https = session.server.lambdatest.ssl = parseInt(port, 10) === 443; break; - case ServerTypes.bitbar: + case SERVER_TYPES.BITBAR: host = process.env.BITBAR_HOST || 'appium.bitbar.com'; port = session.server.bitbar.port = 443; path = session.server.bitbar.path = '/wd/hub'; @@ -384,7 +366,7 @@ export function newSession(caps, attachSessId = null) { }; https = session.server.bitbar.ssl = true; break; - case ServerTypes.kobiton: + case SERVER_TYPES.KOBITON: host = process.env.KOBITON_HOST || 'api.kobiton.com'; port = session.server.kobiton.port = 443; path = session.server.kobiton.path = '/wd/hub'; @@ -398,7 +380,7 @@ export function newSession(caps, attachSessId = null) { } https = session.server.kobiton.ssl = true; break; - case ServerTypes.pcloudy: + case SERVER_TYPES.PCLOUDY: host = session.server.pcloudy.hostname; port = session.server.pcloudy.port = 443; path = session.server.pcloudy.path = '/objectspy/wd/hub'; @@ -415,7 +397,7 @@ export function newSession(caps, attachSessId = null) { }; https = session.server.pcloudy.ssl = true; break; - case ServerTypes.testingbot: + case SERVER_TYPES.TESTINGBOT: host = session.server.testingbot.hostname = process.env.TB_HOST || 'hub.testingbot.com'; port = session.server.testingbot.port = 443; path = session.server.testingbot.path = '/wd/hub'; @@ -433,7 +415,7 @@ export function newSession(caps, attachSessId = null) { } https = session.server.testingbot.ssl = true; break; - case ServerTypes.experitest: { + case SERVER_TYPES.EXPERITEST: { if (!session.server.experitest.url || !session.server.experitest.accessKey) { showError(new Error(i18n.t('experitestAccessKeyURLRequired'))); return false; @@ -455,7 +437,7 @@ export function newSession(caps, attachSessId = null) { experitestUrl.port === '' ? (https ? 443 : 80) : experitestUrl.port; break; } - case ServerTypes.roboticmobi: { + case SERVER_TYPES.ROBOTQA: { host = 'remote.robotqa.com'; path = '/'; port = 443; @@ -467,7 +449,7 @@ export function newSession(caps, attachSessId = null) { } break; } - case ServerTypes.remotetestkit: { + case SERVER_TYPES.REMOTETESTKIT: { host = 'gwjp.appkitbox.com'; path = '/wd/hub'; port = 443; @@ -477,7 +459,7 @@ export function newSession(caps, attachSessId = null) { session.server.remotetestkit.token; break; } - case ServerTypes.mobitru: { + case SERVER_TYPES.MOBITRU: { const webDriverUrl = session.server.mobitru.webDriverUrl || process.env.MOBITRU_WEBDRIVER_URL || @@ -713,11 +695,15 @@ export function getSavedSessions() { } /** - * Switch to a different tab + * Switch to a different Session Builder tab */ export function switchTabs(key) { - return (dispatch) => { + return (dispatch, getState) => { dispatch({type: SWITCHED_TABS, key}); + // if switching to Attach to Session tab, also retrieve the running sessions + if (key === SESSION_BUILDER_TABS.ATTACH_TO_SESSION) { + getRunningSessions()(dispatch, getState); + } }; } @@ -775,11 +761,9 @@ export function setAttachSessId(attachSessId) { * Change the server type */ export function changeServerType(serverType) { - return async (dispatch, getState) => { + return async (dispatch) => { await setSetting(SESSION_SERVER_TYPE, serverType); dispatch({type: CHANGE_SERVER_TYPE, serverType}); - const action = getRunningSessions(); - action(dispatch, getState); }; } @@ -787,12 +771,10 @@ export function changeServerType(serverType) { * Set a server parameter (host, port, etc...) */ export function setServerParam(name, value, serverType) { - const debounceGetRunningSessions = debounce(getRunningSessions(), 5000); return async (dispatch, getState) => { serverType = serverType || getState().session.serverType; await setSetting(SESSION_SERVER_TYPE, serverType); dispatch({type: SET_SERVER_PARAM, serverType, name, value}); - debounceGetRunningSessions(dispatch, getState); }; } @@ -808,26 +790,26 @@ export function setLocalServerParams() { if (serverArgs) { dispatch({ type: SET_SERVER_PARAM, - serverType: ServerTypes.local, + serverType: SERVER_TYPES.LOCAL, name: 'port', value: serverArgs.port, }); dispatch({ type: SET_SERVER_PARAM, - serverType: ServerTypes.local, + serverType: SERVER_TYPES.LOCAL, name: 'hostname', value: 'localhost', }); } else { dispatch({ type: SET_SERVER_PARAM, - serverType: ServerTypes.local, + serverType: SERVER_TYPES.LOCAL, name: 'port', value: undefined, }); dispatch({ type: SET_SERVER_PARAM, - serverType: ServerTypes.local, + serverType: SERVER_TYPES.LOCAL, name: 'hostname', value: undefined, }); @@ -852,8 +834,8 @@ export function setSavedServerParams() { if (server) { // if we have a cloud provider as a saved server, but for some reason the // cloud provider is no longer in the list, revert server type to remote - if (keys(CloudProviders).includes(serverType) && !currentProviders.includes(serverType)) { - serverType = ServerTypes.remote; + if (values(SERVER_TYPES).includes(serverType) && !currentProviders.includes(serverType)) { + serverType = SERVER_TYPES.REMOTE; } dispatch({type: SET_SERVER, server, serverType}); } @@ -917,26 +899,26 @@ function getSaveableState(reduxState) { }; } +/** + * Retrieve all running sessions for the currently configured server details + */ export function getRunningSessions() { return async (dispatch, getState) => { const avoidServerTypes = ['sauce']; - // Get currently running sessions for this server const state = getState().session; - const {server, serverType} = state; - const serverInfo = server[serverType]; - - let {hostname, port, path, ssl, username, accessKey} = serverInfo; + const {server, serverType, attachSessId} = state; + let {hostname, port, path, ssl, username, accessKey} = server[serverType]; // if we have a standard remote server, fill out connection info based on placeholder defaults // in case the user hasn't adjusted those fields - if (serverType === ServerTypes.remote) { + if (serverType === SERVER_TYPES.REMOTE) { hostname = hostname || DEFAULT_SERVER_HOST; port = port || DEFAULT_SERVER_PORT; path = path || DEFAULT_SERVER_PATH; } + // no need to get sessions if we don't have complete server info if (!hostname || !port || !path) { - // no need to get sessions if we don't have complete server info return; } @@ -958,7 +940,19 @@ export function getRunningSessions() { : {}), }, }); - dispatch({type: GET_SESSIONS_DONE, sessions: res.data.value}); + const sessions = res.data.value; + dispatch({type: GET_SESSIONS_DONE, sessions}); + + // set attachSessId if only one session found + if (sessions.length === 1) { + dispatch({type: SET_ATTACH_SESS_ID, attachSessId: sessions[0].id}); + } else if (attachSessId) { + // clear attachSessId if it is no longer present in the found session list + const attachSessIdFound = sessions.find((session) => session.id === attachSessId); + if (!attachSessIdFound) { + dispatch({type: SET_ATTACH_SESS_ID, attachSessId: null}); + } + } } catch (err) { log.warn(`Ignoring error in getting list of active sessions: ${err}`); dispatch({type: GET_SESSIONS_DONE}); diff --git a/app/common/renderer/components/Session/AdvancedServerParams.jsx b/app/common/renderer/components/Session/AdvancedServerParams.jsx index 491c50f52c..4aa78cf9d2 100644 --- a/app/common/renderer/components/Session/AdvancedServerParams.jsx +++ b/app/common/renderer/components/Session/AdvancedServerParams.jsx @@ -1,6 +1,7 @@ import {Checkbox, Col, Collapse, Form, Input, Row} from 'antd'; import React from 'react'; +import {SERVER_TYPES} from '../../constants/session-builder'; import styles from './Session.module.css'; const AdvancedServerParams = ({server, setServerParam, serverType, t}) => ( @@ -16,7 +17,7 @@ const AdvancedServerParams = ({server, setServerParam, serverType, t}) => ( - setServerParam('allowUnauthorized', e.target.checked, 'advanced') + setServerParam('allowUnauthorized', e.target.checked, SERVER_TYPES.ADVANCED) } > {t('allowUnauthorizedCerts')} @@ -28,7 +29,9 @@ const AdvancedServerParams = ({server, setServerParam, serverType, t}) => ( setServerParam('useProxy', e.target.checked, 'advanced')} + onChange={(e) => + setServerParam('useProxy', e.target.checked, SERVER_TYPES.ADVANCED) + } > {t('Use Proxy')} @@ -38,7 +41,7 @@ const AdvancedServerParams = ({server, setServerParam, serverType, t}) => ( setServerParam('proxy', e.target.value, 'advanced')} + onChange={(e) => setServerParam('proxy', e.target.value, SERVER_TYPES.ADVANCED)} placeholder={t('Proxy URL')} value={server.advanced.proxy} /> diff --git a/app/common/renderer/components/Session/AttachToSession.jsx b/app/common/renderer/components/Session/AttachToSession.jsx index 4efa0ce748..c3e2e5b555 100644 --- a/app/common/renderer/components/Session/AttachToSession.jsx +++ b/app/common/renderer/components/Session/AttachToSession.jsx @@ -1,46 +1,10 @@ import {ReloadOutlined} from '@ant-design/icons'; -import {Button, Card, Col, Form, Row, Select} from 'antd'; +import {Button, Card, Col, Form, Row, Select, Tooltip} from 'antd'; import React from 'react'; -import {ServerTypes} from '../../actions/Session'; +import {getSessionInfo} from '../../utils/attaching-to-session'; import SessionStyles from './Session.module.css'; -const formatCaps = (caps) => { - let importantCaps = [caps.app, caps.platformName, caps.deviceName]; - if (caps.automationName) { - importantCaps.push(caps.automationName); - } - return importantCaps.join(', ').trim(); -}; - -const formatCapsBrowserstack = (caps) => { - let importantCaps = formatCaps(caps).split(', '); - if (caps.sessionName) { - importantCaps.push(caps.sessionName); - } - return importantCaps.join(', ').trim(); -}; - -const formatCapsLambdaTest = (caps) => { - if (caps.hasOwnProperty.call(caps, 'capabilities')) { - caps = caps.capabilities; - } - const deviceName = caps.desired ? caps.desired.deviceName : caps.deviceName; - const importantCaps = [deviceName, caps.platformName, caps.platformVersion]; - return importantCaps.join(', ').trim(); -}; - -const getSessionInfo = (session, serverType) => { - switch (serverType) { - case ServerTypes.browserstack: - return `${session.id} — ${formatCapsBrowserstack(session.capabilities)}`; - case ServerTypes.lambdatest: - return `${session.id} - ${formatCapsLambdaTest(session.capabilities)}`; - default: - return `${session.id} — ${formatCaps(session.capabilities)}`; - } -}; - const AttachToSession = ({ serverType, attachSessId, @@ -69,17 +33,26 @@ const AttachToSession = ({ value={attachSessId || undefined} onChange={(value) => setAttachSessId(value)} > - {runningAppiumSessions.map((session) => ( - -
{getSessionInfo(session, serverType)}
- - ))} + {runningAppiumSessions + .slice() + .reverse() + .map((session) => ( + // list is reversed in order to place the most recent sessions at the top + // slice() is added because reverse() mutates the original array + +
{getSessionInfo(session, serverType)}
+ + ))} -
-
+ +