feat: drop down of apps from app management #113

lenareed · 2024-09-09T22:07:23Z

Description

pull names of apps from user's app management. Remove platform option as this is included with app name from API. If history instance uses app no longer in app management, app name is greyed out.

Additional:
Added optional to tunnel name and owner as these are optional fields to avoid confusion.

…ce error needs to be fixed

…nagement

…nd likely app payloads uploaded by saucectl

mhan83

One issue I noticed: when I load a test record for the first time, I see the correct app name selected, but it switches to the wrong app name after a brief pause. Only happens for the first test record I load, however.

Screen.Recording.2024-09-11.at.2.44.58.PM.mov

mhan83 · 2024-09-11T19:56:29Z

webview-ui/test-generation/src/App.tsx

+  for (let i = 0; i < loadedAppNames.length; i++) {
+    optionElements.push(<VSCodeOption>{loadedAppNames[i]}</VSCodeOption>);
+    if (loadedAppNames[i] == appName) {
+      seenHistoryAppName = true;
+    }
+  }


It's more idiomatic to do this declaratively by mapping the items directly to a component. For example:

{steps.map((step) => ( <TestStep key={`${step.testRecordId}-${step.index}`} dispatch={dispatch} language={state.language} step={step} /> ))}

from https://github.com/saucelabs/vscode-scriptiq/blob/main/webview-ui/test-generation/src/App.tsx#L373-L380

mhan83 · 2024-09-11T20:11:44Z

webview-ui/test-generation/src/App.scss

@@ -68,6 +68,10 @@
  row-gap: 8px;
 }

+.app-not-loaded {
+  color: grey;


Is there a --vscode-* color that's better for this? This grey doesn't stand out in a light theme.

mhan83 · 2024-09-11T20:17:11Z

webview-ui/test-generation/src/App.tsx

+            className={
+              optionElements.some(
+                (option) => option.props.className === 'app-not-loaded',
+              )
+                ? 'app-not-loaded'
+                : ''
            }


You can use the :has() pseudo-class instead of this.

css like this:

.app-list:has(.app-not-loaded) { color: gray; }

Then just add the .app-list class:

Suggested change

className={

optionElements.some(

(option) => option.props.className === 'app-not-loaded',

)

? 'app-not-loaded'

: ''

}

className="app-list"

This doesn't quite have the functionality I want, instead of only greying out the "bad" app, when I'm on that history instance, it greys out every app I selected, and the "bad" app is not greyed out in the drop down list

😕 the two implementations are behaving identically for me...

mhan83 · 2024-09-11T20:32:48Z

src/api/llm/http.ts

+  const data = (await response.json()) as { items: AppInfo[] };
+
+  if (data && Array.isArray(data.items) && data.items.every(isAppInfo)) {


This is a strange mix of being defensive about the response data while making assumptions about it. Why assume the response has an items property but then check its items are the correct type? I'd think you'd want to verify the entire shape of the response if you're going to at all. And then have a single type guard instead of checking isAppInfo on all the items.

mhan83 · 2024-09-11T20:34:49Z

src/api/llm/http.ts

+  );
+
+  if (!response.ok) {
+    throw new Error('Network response was not ok');


Could include the status and/or statusText in the error.

mhan83 · 2024-09-11T20:40:34Z

src/editors/TestGenerationPanel.ts

+    TestGenerationPanel.currentPanel.loadAppNames();
+


Every time I load a test record from the history list, the same list of apps needs to be fetched...is that intended?

It was intentional, I didn't want users to be confused why when they reload the screen, even for history, the app's don't match what's in app management. For example, if they load a history instance and realize the app has been deleted, they might re-upload their app and expect to see the app name list reflect that when re-opening the history instance. But if the downsides of calling that API isn't worth that scenario, I can only have it load when it isn't loading history.

mhan83 · 2024-09-11T20:53:49Z

src/types.ts

@@ -162,7 +162,7 @@ export function isStoppedResponse(data: unknown): data is StoppedResponse {
  );
 }

-export type Platform = 'Android' | 'iOS';
+export type Platform = 'android' | 'ios';


Is this change safe? Platform is used in a few places: the test generation request and its also serialized as part of the test record. There'd be a mismatch here between test records stored before and after this change...

I could update it so both upper and lower case are accepted. I tested all the functionality and nothing appeared broken. On the server side, I added a .lower() so that even platform types that remain upper case would work.

Changing the type for data that gets serialized/deserialized seems at best unnecessary at worst a foot gun. The type is the type, if you have data that doesn't conform to it, change the data not the type, imo. Adding to the type union is non-breaking so is safer than this change.

Ok, I added to the union. I viewed it more as the type we had was incorrect as when we get the platform from the API call it is all lower case, and I thought having to change it each time was inefficient. Especially since each word has a different uppercase structure so we can't just uppercase the first letter, which is why I didn't change the data. But I can change that if that's preferred.

mhan83 · 2024-09-11T20:55:52Z

webview-ui/test-generation/src/state.ts

@@ -78,6 +78,8 @@ export interface State {
  language: 'python' | 'java';

  tunnel: Tunnel;
+
+  loadedAppInfo: AppInfo[];


nit: maybe just call it apps for brevity?

alexplischke · 2024-09-11T21:00:31Z

src/api/llm/http.ts

+  );
+
+  if (!response.ok) {
+    throw new Error('Network response was not ok');


I don't think this will be informative enough for the user or for us if get an error report. You see it in the error message at

vscode-scriptiq/src/editors/TestGenerationPanel.ts

Line 179 in f70bbdb

'Failed to load app names. Please check your credentials and try again.',

as well, where you have to make a guess about what could be wrong. We'd definitely know if we get an unauthorized response back. I wouldn't hide the error, just show it to the user.

alexplischke · 2024-09-11T21:02:51Z

webview-ui/test-generation/src/App.tsx

+  const appNameToPlatform = new Map<string, string>();
+  loadedAppInfo.forEach((item) => appNameToPlatform.set(item.name, item.kind));
+  let seenHistoryAppName = appName == ''; // if appName is empty, we don't need to add appName later
+  const optionElements: JSX.Element[] = [];


Would be great if the app names were sorted. I had a bit of trouble navigating through our relatively large list!

Good to know! I'll add that

alexplischke

Some selections don't appear to be working 🤔

alexplischke · 2024-09-11T21:15:13Z

src/api/llm/http.ts

@@ -72,3 +72,28 @@ export async function sendUserRating(
    throw new Error('Unexpected status code: ' + resp.status);
  }
 }
+
+export async function fetchAppNames(creds: Credentials) {


Imho a misnomer, since you return more than just the names. Whenever I look at this, I expect a list of strings to be returned. Would recommend a different name 🙏

Won't this return both VDC and RDC type apps? Are we fully supporting VDC now?

@alexplischke are there apps which are RDC or VDC specific? Do you have an example app that is VDC only? I'd be happy to test uploading and filtering that out.

Ah, zips are simulators only. I'll filter those out for now, with a comment to remove for when VDC is enabled.

…i call

mhan83 · 2024-09-13T15:38:53Z

webview-ui/test-generation/src/App.tsx

+                  type: 'setAppName',
+                  value: {
+                    appName: appName as string,
+                    platformName: appInfo.kind as 'ios' | 'android',


~~If appName isn't in the list of apps, the platform needs to come from state?~~

nvm, it'll error anyways, I guess

Yeah, when I test it running an app no longer in app management I get an error that says the app isn't in app storage, so the error statement is properly informative.

mhan83 · 2024-09-13T15:41:57Z

src/api/llm/http.ts

+    return data.items.filter(
+      (item) =>
+        !('is_simulator' in item.metadata) ||
+        item.metadata.is_simulator === false,
+    );


I think it'd be better to have this logic live outside the api definition. The api layer should simply model the api itself (its parameters, expected responses, etc).

mhan83 · 2024-09-13T15:55:58Z

src/types.ts

+  id: string;
+  name: string;
+  kind: string;
+  metadata: Map<unknown, unknown>;


The defined type doesn't match the logic in the type guard; its a Map here, but in the type guard function below, it checks that metadata is an object.

Could define it a couple ways. Just define the metadata you want:

Suggested change

metadata: Map<unknown, unknown>;

metadata: {

is_simulator?: boolean;

};

The AppInfo type is already a truncated version of the actual server response, may as well make metadata the same and add as needed.

Or, if you want something as flexible as Map:

Suggested change

metadata: Map<unknown, unknown>;

metadata: { [key: string]: any };

I prefer the former.

Also keep in mind, metadata can be null. We probably won't encounter that case, but getting the types wrong defeats whatever safety we may get from having them in the first place.

alexplischke · 2024-09-13T16:37:55Z

src/api/llm/http.ts

+        item.metadata.is_simulator === false,
+    );
+  } else {
+    throw new Error('Unexpected data format from API');


We should log out what the response in that case was. Else I'd have to modify the code just to be able to troubleshoot what came back from the server 🔍

alexplischke · 2024-09-13T16:48:31Z

src/editors/TestGenerationPanel.ts

+        toast.showError(
+          'Failed to load app names. Please check your credentials and try again.',
+        );


It's better to surface the error to the user. That way they can act on it (wrong credentials, in which case they'd have an auth error) or file an issue with the error message. Otherwise they'd simply see a templated response but not that's wrong.

Suggested change

toast.showError(

'Failed to load app names. Please check your credentials and try again.',

);

toast.showError(

`Failed to load app names: ${errMsg(error)}.`,

);

alexplischke · 2024-09-13T17:08:36Z

webview-ui/test-generation/src/state.ts


 export interface Assertion {
  value: string;
  key: string;
 }

 export interface Platform {
-  name: 'iOS' | 'Android';
+  name: 'ios' | 'android' | 'iOS' | 'Android';


I think this may be hiding some relevant historic information.

Am I correct to assume that the previously stored records are using iOS and Android?

If I was someone new to code, and I see this new definition now, I don't know why we have the case differences. If you intend to keep these changes, it'd be helpful to the future-us to have an inline explanation 🙏

💡 I think there's a more important gotcha here though that introduces some future pitfalls though and worth a re-evaluation. You can see it in

vscode-scriptiq/webview-ui/test-generation/src/state.ts

Line 399 in ff56521

name: testRecord.platform.toLowerCase() as 'android' | 'ios',

for instance, forcing you to potentially lie to the compiler.

Food for thought:

Do we need to change our platform definition just because App Storage defines it differently? Or should we transform what's coming from App Storage to our case standard? This change would be more isolated and require no backend change on our side either.

Should we change our definition to lower case? In which case the correct way would be to do a full data migration (storage.ts has a placeholder exactly for this use case). Is it a annoying? Yes, but that's the consequence of not massaging App Storage responses to our needs.

We're in beta and can use it to our advantage! Let's say, much like above, that maybe we do want to lowercase our platform definition eventually. We can transform the data coming from App Storage to our needs now and schedule the changes for a lowercase switch for a v1.0.0 release, not needing the data migration change.

I will update this PR to modify the API data to be upper case and add a new ticket to update our platform definitions to match the API and be lower case, as both are acceptable inputs as platform names into appium and that will allow for greater consistency.

…a to also be uppercase. Adding clearer error statements, moving filtering of simulator only apps

mhan83

One more thought: should we validate the app selection before generating the test? Let's say I load history test record and the app it tested is not in app storage. I modify the record, leaving the app as is, and try to generate a test. What error will the user see in this case? Will it be easily understandable/actionable? If not, since we know on the client side the generation will fail, should we validate and present a nice error message right away?

webview-ui/test-generation/src/App.tsx

alexplischke · 2024-09-16T15:01:46Z

webview-ui/test-generation/src/state.ts

+      let apps = action.value;
+      apps = apps.filter(
+        (item) =>
+          item.metadata === null ||


I'm a wee bit confused, since according to the AppInfo interface, metadata is always defined. Which is it?

Since metadata might be null, though rare, I'll allow metadata to be null in the type.

I'd vote for adding it to the interface. The downside is a few extra null checks, upside is an accurate and reliable type.

alexplischke · 2024-09-16T15:03:44Z

webview-ui/test-generation/src/state.ts

+          !('is_simulator' in item.metadata) ||
+          item.metadata.is_simulator === false,


Can be simplified:

Suggested change

!('is_simulator' in item.metadata) ||

item.metadata.is_simulator === false,

!item.metadata.is_simulator

alexplischke · 2024-09-16T15:10:59Z

webview-ui/test-generation/src/state.ts

+          item.metadata.is_simulator === false,
+      );
+      apps.forEach((app) => {
+        app.kind = app.kind === 'android' ? 'Android' : 'iOS';


See my other comments w/r/t/ massaging the data at the HTTP client level or type guarding.

alexplischke · 2024-09-16T15:28:24Z

webview-ui/test-generation/src/App.tsx

+                  type: 'setAppName',
+                  value: {
+                    appName: appName as string,
+                    platformName: appInfo.kind as 'iOS' | 'Android',


We are still forcing types onto the compiler though 😅 If there's a bug in

vscode-scriptiq/webview-ui/test-generation/src/state.ts

Line 377 in 7b7798b

app.kind = app.kind === 'android' ? 'Android' : 'iOS';

, this line will be complicit in the deception of the compiler.

Options

One technique that we use in saucectl quite often, is to massage the data that we receive at the HTTP client level, which would be in fetchApps() in this case. You could return a type of AppInfo that's internal to us, with platformName being a native field of type Platform.

Add a function for type guarding. AppKindToPlatform() or some such. Might result in more 🍝 code over time though.

alexplischke · 2024-09-16T15:29:28Z

webview-ui/test-generation/src/App.tsx

+                dispatch({
+                  type: 'setAppName',
+                  value: {
+                    appName: appName as string,


vscode-scriptiq/webview-ui/test-generation/src/App.tsx

Line 137 in 7b7798b

const appName: string = e.target.value as string;

is already doing a cast.

alexplischke · 2024-09-16T15:31:22Z

I believe that the title can be renamed from feat!: drop down of apps from app management to feat: drop down of apps from app management, since this implementation is fully backwards compatible now.

lenareed · 2024-09-16T17:27:07Z

@mhan83 for the question about the error message, you get a device connection failed message very quickly, it looks like this
Device Connection Failed: Message: Unable to find file 'petals.apk' in app storage Build info: version: 'unknown', revision: 'unknown', time: 'unknown' System info: host: 'Sauce Labs RDC', ip: 'N/A', os.name: 'Linux', os.arch: 'amd64', os.version: '6.1.85+', java.version: '17.0.12' Driver info: driver.version: unknown
I think that is an informative error, though we could make the format nicer if we want

first attempt at adding app names from user's app management, referen…

a944a50

…ce error needs to be fixed

lenareed requested a review from a team as a code owner September 9, 2024 22:07

lenareed changed the title ~~Draft: drop down of apps from app management~~ Draft: feat: drop down of apps from app management Sep 9, 2024

lenareed added 3 commits September 10, 2024 14:58

properly loads when click + and history, greys out apps not in app ma…

fb00c75

…nagement

removed unnecessary imports/types

12f4927

Merge remote-tracking branch 'origin/main' into DEVX-2886

5be9760

lenareed changed the title ~~Draft: feat: drop down of apps from app management~~ feat!: drop down of apps from app management Sep 10, 2024

lenareed changed the title ~~feat!: drop down of apps from app management~~ Draft: feat!: drop down of apps from app management Sep 10, 2024

lenareed added 2 commits September 10, 2024 16:06

removed apps without kind, cannot be run because not android or ios a…

1f31469

…nd likely app payloads uploaded by saucectl

moved filter to api call

f70bbdb

lenareed changed the title ~~Draft: feat!: drop down of apps from app management~~ feat!: drop down of apps from app management Sep 11, 2024

mhan83 reviewed Sep 11, 2024

View reviewed changes

alexplischke reviewed Sep 11, 2024

View reviewed changes

lenareed added 10 commits September 11, 2024 17:24

use vscode native color

69b9fdf

used react to load data

6fb6d9c

more informative error, update method names, single type guard for ap…

501ce47

…i call

fixed dropdown so loads on new window, saves platform kind

7117b70

removing log lines

64c078e

moved sort to loadAppNames so saved in state

792b8f7

filter by explicitly by is_simulator

f23b594

fix case issue by making platform name always lowercase

bc38521

updated css so grey when visible

f472a10

waiting till items to populate dropdown

ff56521

mhan83 reviewed Sep 13, 2024

View reviewed changes

alexplischke reviewed Sep 13, 2024

View reviewed changes

lenareed added 3 commits September 13, 2024 16:47

reverted platform type to uppercase platform names, modifying api dat…

e99dd58

…a to also be uppercase. Adding clearer error statements, moving filtering of simulator only apps

updating missed platforms from lower case back to uppercase

f2508ad

remove unnecessary type declaration

7b7798b

mhan83 reviewed Sep 16, 2024

View reviewed changes

webview-ui/test-generation/src/App.tsx Show resolved Hide resolved

alexplischke reviewed Sep 16, 2024

View reviewed changes

lenareed changed the title ~~feat!: drop down of apps from app management~~ feat: drop down of apps from app management Sep 16, 2024

metadata can be null, move modifying API input to method with fetch

47444a3

mhan83 approved these changes Sep 17, 2024

View reviewed changes

alexplischke approved these changes Sep 17, 2024

View reviewed changes

lenareed merged commit 2e6d327 into main Sep 17, 2024
1 check passed

lenareed deleted the DEVX-2886 branch September 17, 2024 18:50

		const data = (await response.json()) as { items: AppInfo[] };

		if (data && Array.isArray(data.items) && data.items.every(isAppInfo)) {

-  metadata: Map<unknown, unknown>;
+  metadata: {
+    is_simulator?: boolean;
+  };

	metadata: Map<unknown, unknown>;
	metadata: { [key: string]: any };

		!('is_simulator' in item.metadata) \|\|
		item.metadata.is_simulator === false,

	!('is_simulator' in item.metadata) \|\|
	item.metadata.is_simulator === false,
	!item.metadata.is_simulator

feat: drop down of apps from app management #113

feat: drop down of apps from app management #113

Conversation

lenareed commented Sep 9, 2024 • edited Loading

Description

mhan83 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexplischke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhan83 Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhan83 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lenareed Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexplischke commented Sep 16, 2024

lenareed commented Sep 16, 2024

lenareed commented Sep 9, 2024 •

edited

Loading

mhan83 Sep 13, 2024 •

edited

Loading

lenareed Sep 16, 2024 •

edited

Loading