Feature/uft lab app selection#4472
Conversation
WalkthroughAdds UFT Mobile app discovery and selection to the UFT Credentials Dialog, introduces an Apps tab alongside Devices, and wires selected app details into AppiumDriverEditPage to update Appium capabilities (bundleId / appPackage / appActivity) with platform validation. Changes
Sequence DiagramsequenceDiagram
participant ADE as AppiumDriverEditPage
participant Dialog as UFTCredentialsDialog
participant API as UFT Mobile API
ADE->>Dialog: Open dialog (initialAppId, initialPlatform)
activate Dialog
Dialog->>API: GET /rest/devices
API-->>Dialog: devices list
Dialog->>Dialog: DisplayDevices() / ApplyDeviceFilters()
Dialog->>API: GET /rest/apps/getAplicationsLastVersion
API-->>Dialog: apps payload
Dialog->>Dialog: ParseApps(json) / DisplayApps() / ApplyAppFilters(initialPlatform)
note over Dialog: User selects device and app
Dialog->>Dialog: Validate platform match on Ok
Dialog-->>ADE: DialogResult = true (with selected device & app)
deactivate Dialog
ADE->>ADE: ApplySelectedPhoneDetails(dialog)
ADE->>ADE: ApplySelectedAppDetails(dialog)
ADE->>ADE: Update Appium capabilities (bundleId / appPackage / appActivity)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Ginger/Ginger/Drivers/DriversConfigsEditPages/AppiumDriverEditPage.xaml.cs`:
- Around line 867-904: The ApplySelectedAppDetails method can delete
cross-platform capabilities even when the selected platform's identifier is
empty, potentially clearing all app identifiers; update ApplySelectedAppDetails
to guard deletions/update logic: before calling
DeleteCapabilityIfExist("appium:appPackage")/DeleteCapabilityIfExist("appium:appActivity")
for iOS, check if dialog.SelectedAppBundleId is non-empty (or else skip deletion
and show a user warning); likewise for Android, only call
DeleteCapabilityIfExist("appium:bundleId") when dialog.SelectedAppPackage or
SelectedAppActivity is non-empty (or show a warning) — adjust the branches
around
UpdateCapabilityValue("appium:bundleId"/"appium:appPackage"/"appium:appActivity")
and the deletion calls, and use ApplyPlatformFromSelection and
xCapabilitiesGrid.Grid?.Items.Refresh as before to refresh only when actual
changes occur.
In `@Ginger/Ginger/Drivers/DriversConfigsEditPages/UFTCredentialsDialog.xaml`:
- Around line 326-335: The stacked Image and ImageMakerControl currently rely on
layering for fallback but lack an explicit visibility swap when IconSource is
null; update the XAML around Image (binding IconSource) and ImageMakerControl
(binding IconType/SetAsFontImageWithSize) so Image.Visibility is collapsed when
IconSource is null (use TargetNullValue or a converter) and
ImageMakerControl.Visibility is Visible as the fallback (or use a DataTrigger
that toggles their Visibility based on IconSource); also ensure the
ImageMakerControl has a default style/foreground set so its fallback icon
renders consistently.
In `@Ginger/Ginger/Drivers/DriversConfigsEditPages/UFTCredentialsDialog.xaml.cs`:
- Line 509: The code currently reads bool instrumented =
obj.TryGetValue("instrumented", out JToken instToken) &&
instToken.Value<bool>(); which can throw if "instrumented" is a non-boolean
type; change the logic in the method that performs this parse to defensively
handle types: first call obj.TryGetValue("instrumented", out JToken instToken),
then if instToken != null check instToken.Type == JTokenType.Boolean and use
instToken.Value<bool>(); else attempt bool.TryParse(instToken.ToString(), out
var parsed) and use parsed; otherwise set instrumented = false (or a safe
default). Update the parse around the symbols obj, instToken and the
instrumented variable accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ce396f40-6979-4a41-bf85-22ade6cc185c
📒 Files selected for processing (3)
Ginger/Ginger/Drivers/DriversConfigsEditPages/AppiumDriverEditPage.xaml.csGinger/Ginger/Drivers/DriversConfigsEditPages/UFTCredentialsDialog.xamlGinger/Ginger/Drivers/DriversConfigsEditPages/UFTCredentialsDialog.xaml.cs
| private void ApplySelectedAppDetails(UFTCredentialsDialog dialog) | ||
| { | ||
| if (dialog == null || string.IsNullOrWhiteSpace(dialog.SelectedAppId)) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| bool wasUpdated = false; | ||
|
|
||
| if (string.Equals(dialog.SelectedAppType, "IOS", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| if (!string.IsNullOrWhiteSpace(dialog.SelectedAppBundleId)) | ||
| { | ||
| wasUpdated |= UpdateCapabilityValue("appium:bundleId", dialog.SelectedAppBundleId); | ||
| } | ||
| DeleteCapabilityIfExist("appium:appPackage"); | ||
| DeleteCapabilityIfExist("appium:appActivity"); | ||
| } | ||
| else | ||
| { | ||
| if (!string.IsNullOrWhiteSpace(dialog.SelectedAppPackage)) | ||
| { | ||
| wasUpdated |= UpdateCapabilityValue("appium:appPackage", dialog.SelectedAppPackage); | ||
| } | ||
| if (!string.IsNullOrWhiteSpace(dialog.SelectedAppActivity)) | ||
| { | ||
| wasUpdated |= UpdateCapabilityValue("appium:appActivity", dialog.SelectedAppActivity); | ||
| } | ||
| DeleteCapabilityIfExist("appium:bundleId"); | ||
| } | ||
|
|
||
| bool platformUpdated = ApplyPlatformFromSelection(dialog.SelectedAppType); | ||
|
|
||
| if (wasUpdated || platformUpdated) | ||
| { | ||
| xCapabilitiesGrid.Grid?.Items.Refresh(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Edge case: App with missing identifiers may leave capabilities in unclear state.
If an iOS app is selected but SelectedAppBundleId is empty, the Android capabilities (appPackage/appActivity) are deleted without setting any iOS capability. Similarly for Android apps with empty SelectedAppPackage. This could leave the configuration without any app identifier.
Consider either:
- Not deleting cross-platform capabilities when the target platform's capability is empty, or
- Showing a warning to the user that the selected app has incomplete configuration.
🛡️ Proposed guard to avoid clearing capabilities without replacement
if (string.Equals(dialog.SelectedAppType, "IOS", StringComparison.OrdinalIgnoreCase))
{
if (!string.IsNullOrWhiteSpace(dialog.SelectedAppBundleId))
{
wasUpdated |= UpdateCapabilityValue("appium:bundleId", dialog.SelectedAppBundleId);
+ DeleteCapabilityIfExist("appium:appPackage");
+ DeleteCapabilityIfExist("appium:appActivity");
}
- DeleteCapabilityIfExist("appium:appPackage");
- DeleteCapabilityIfExist("appium:appActivity");
}
else
{
- if (!string.IsNullOrWhiteSpace(dialog.SelectedAppPackage))
+ bool hasAndroidCap = false;
+ if (!string.IsNullOrWhiteSpace(dialog.SelectedAppPackage))
{
wasUpdated |= UpdateCapabilityValue("appium:appPackage", dialog.SelectedAppPackage);
+ hasAndroidCap = true;
}
if (!string.IsNullOrWhiteSpace(dialog.SelectedAppActivity))
{
wasUpdated |= UpdateCapabilityValue("appium:appActivity", dialog.SelectedAppActivity);
+ hasAndroidCap = true;
+ }
+ if (hasAndroidCap)
+ {
+ DeleteCapabilityIfExist("appium:bundleId");
}
- DeleteCapabilityIfExist("appium:bundleId");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private void ApplySelectedAppDetails(UFTCredentialsDialog dialog) | |
| { | |
| if (dialog == null || string.IsNullOrWhiteSpace(dialog.SelectedAppId)) | |
| { | |
| return; | |
| } | |
| bool wasUpdated = false; | |
| if (string.Equals(dialog.SelectedAppType, "IOS", StringComparison.OrdinalIgnoreCase)) | |
| { | |
| if (!string.IsNullOrWhiteSpace(dialog.SelectedAppBundleId)) | |
| { | |
| wasUpdated |= UpdateCapabilityValue("appium:bundleId", dialog.SelectedAppBundleId); | |
| } | |
| DeleteCapabilityIfExist("appium:appPackage"); | |
| DeleteCapabilityIfExist("appium:appActivity"); | |
| } | |
| else | |
| { | |
| if (!string.IsNullOrWhiteSpace(dialog.SelectedAppPackage)) | |
| { | |
| wasUpdated |= UpdateCapabilityValue("appium:appPackage", dialog.SelectedAppPackage); | |
| } | |
| if (!string.IsNullOrWhiteSpace(dialog.SelectedAppActivity)) | |
| { | |
| wasUpdated |= UpdateCapabilityValue("appium:appActivity", dialog.SelectedAppActivity); | |
| } | |
| DeleteCapabilityIfExist("appium:bundleId"); | |
| } | |
| bool platformUpdated = ApplyPlatformFromSelection(dialog.SelectedAppType); | |
| if (wasUpdated || platformUpdated) | |
| { | |
| xCapabilitiesGrid.Grid?.Items.Refresh(); | |
| } | |
| } | |
| private void ApplySelectedAppDetails(UFTCredentialsDialog dialog) | |
| { | |
| if (dialog == null || string.IsNullOrWhiteSpace(dialog.SelectedAppId)) | |
| { | |
| return; | |
| } | |
| bool wasUpdated = false; | |
| if (string.Equals(dialog.SelectedAppType, "IOS", StringComparison.OrdinalIgnoreCase)) | |
| { | |
| if (!string.IsNullOrWhiteSpace(dialog.SelectedAppBundleId)) | |
| { | |
| wasUpdated |= UpdateCapabilityValue("appium:bundleId", dialog.SelectedAppBundleId); | |
| DeleteCapabilityIfExist("appium:appPackage"); | |
| DeleteCapabilityIfExist("appium:appActivity"); | |
| } | |
| } | |
| else | |
| { | |
| bool hasAndroidCap = false; | |
| if (!string.IsNullOrWhiteSpace(dialog.SelectedAppPackage)) | |
| { | |
| wasUpdated |= UpdateCapabilityValue("appium:appPackage", dialog.SelectedAppPackage); | |
| hasAndroidCap = true; | |
| } | |
| if (!string.IsNullOrWhiteSpace(dialog.SelectedAppActivity)) | |
| { | |
| wasUpdated |= UpdateCapabilityValue("appium:appActivity", dialog.SelectedAppActivity); | |
| hasAndroidCap = true; | |
| } | |
| if (hasAndroidCap) | |
| { | |
| DeleteCapabilityIfExist("appium:bundleId"); | |
| } | |
| } | |
| bool platformUpdated = ApplyPlatformFromSelection(dialog.SelectedAppType); | |
| if (wasUpdated || platformUpdated) | |
| { | |
| xCapabilitiesGrid.Grid?.Items.Refresh(); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Ginger/Ginger/Drivers/DriversConfigsEditPages/AppiumDriverEditPage.xaml.cs`
around lines 867 - 904, The ApplySelectedAppDetails method can delete
cross-platform capabilities even when the selected platform's identifier is
empty, potentially clearing all app identifiers; update ApplySelectedAppDetails
to guard deletions/update logic: before calling
DeleteCapabilityIfExist("appium:appPackage")/DeleteCapabilityIfExist("appium:appActivity")
for iOS, check if dialog.SelectedAppBundleId is non-empty (or else skip deletion
and show a user warning); likewise for Android, only call
DeleteCapabilityIfExist("appium:bundleId") when dialog.SelectedAppPackage or
SelectedAppActivity is non-empty (or show a warning) — adjust the branches
around
UpdateCapabilityValue("appium:bundleId"/"appium:appPackage"/"appium:appActivity")
and the deletion calls, and use ApplyPlatformFromSelection and
xCapabilitiesGrid.Grid?.Items.Refresh as before to refresh only when actual
changes occur.
| <Grid Width="28" Height="28" Margin="0,0,6,0" HorizontalAlignment="Left"> | ||
| <UserControls:ImageMakerControl Width="28" | ||
| Height="28" | ||
| ImageType="{Binding IconType}" | ||
| SetAsFontImageWithSize="24" /> | ||
| <Image Source="{Binding IconSource}" | ||
| Width="28" | ||
| Height="28" | ||
| Stretch="Uniform" /> | ||
| </Grid> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using a fallback for missing app icons.
The Image and ImageMakerControl are stacked in a Grid, but when IconSource is null, the Image will be invisible and only ImageMakerControl shows. This layering approach works, but ensure ImageMakerControl is styled to be visible as the fallback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Ginger/Ginger/Drivers/DriversConfigsEditPages/UFTCredentialsDialog.xaml`
around lines 326 - 335, The stacked Image and ImageMakerControl currently rely
on layering for fallback but lack an explicit visibility swap when IconSource is
null; update the XAML around Image (binding IconSource) and ImageMakerControl
(binding IconType/SetAsFontImageWithSize) so Image.Visibility is collapsed when
IconSource is null (use TargetNullValue or a converter) and
ImageMakerControl.Visibility is Visible as the fallback (or use a DataTrigger
that toggles their Visibility based on IconSource); also ensure the
ImageMakerControl has a default style/foreground set so its fallback icon
renders consistently.
| string fileName = GetStringValue(obj, "fileName"); | ||
| string instrumentationStatus = GetStringValue(obj, "instrumentationStatus"); | ||
| string iconBase64 = GetStringValue(obj, "icon"); | ||
| bool instrumented = obj.TryGetValue("instrumented", out JToken instToken) && instToken.Value<bool>(); |
There was a problem hiding this comment.
Potential exception when parsing non-boolean instrumented field.
If the API returns instrumented as a string (e.g., "true") or an incompatible type, Value<bool>() could throw an exception or return unexpected results.
🛡️ Proposed defensive parsing
- bool instrumented = obj.TryGetValue("instrumented", out JToken instToken) && instToken.Value<bool>();
+ bool instrumented = false;
+ if (obj.TryGetValue("instrumented", out JToken instToken))
+ {
+ if (instToken.Type == JTokenType.Boolean)
+ {
+ instrumented = instToken.Value<bool>();
+ }
+ else if (instToken.Type == JTokenType.String)
+ {
+ bool.TryParse(instToken.ToString(), out instrumented);
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bool instrumented = obj.TryGetValue("instrumented", out JToken instToken) && instToken.Value<bool>(); | |
| bool instrumented = false; | |
| if (obj.TryGetValue("instrumented", out JToken instToken)) | |
| { | |
| if (instToken.Type == JTokenType.Boolean) | |
| { | |
| instrumented = instToken.Value<bool>(); | |
| } | |
| else if (instToken.Type == JTokenType.String) | |
| { | |
| bool.TryParse(instToken.ToString(), out instrumented); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Ginger/Ginger/Drivers/DriversConfigsEditPages/UFTCredentialsDialog.xaml.cs`
at line 509, The code currently reads bool instrumented =
obj.TryGetValue("instrumented", out JToken instToken) &&
instToken.Value<bool>(); which can throw if "instrumented" is a non-boolean
type; change the logic in the method that performs this parse to defensively
handle types: first call obj.TryGetValue("instrumented", out JToken instToken),
then if instToken != null check instToken.Type == JTokenType.Boolean and use
instToken.Value<bool>(); else attempt bool.TryParse(instToken.ToString(), out
var parsed) and use parsed; otherwise set instrumented = false (or a safe
default). Update the parse around the symbols obj, instToken and the
instrumented variable accordingly.
…e button to Select Lab Device/App
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Ginger/Ginger/Drivers/DriversConfigsEditPages/UFTCredentialsDialog.xaml`:
- Around line 325-340: The ListBox.ItemContainerStyle currently disables
keyboard focus visuals via FocusVisualStyle="{x:Null}", which harms
accessibility; instead restore a subtle focus visual: create a reusable
FocusVisualStyle resource (or a minimal Setter on the same Style) that adds a
non-intrusive border/outline or background change for ListBoxItem when focused,
and reference it from the ListBox.ItemContainerStyle (replace the
FocusVisualStyle="{x:Null}" Setter). Keep other behavior
(PreviewMouseLeftButtonDown handler AppItem_PreviewMouseLeftButtonDown and the
ControlTemplate/ContentPresenter) unchanged.
In `@Ginger/Ginger/Drivers/DriversConfigsEditPages/UFTCredentialsDialog.xaml.cs`:
- Around line 1565-1589: The ConvertBase64ToImage method currently swallows all
exceptions in its catch block; update that catch to capture the Exception (e.g.,
catch (Exception ex)) and write a debug-level log entry including the exception
message/stack and minimal context (such as whether base64 was null/empty or its
length) using the project's logging facility (or System.Diagnostics.Debug/Trace
if no logger exists) before returning null so failed icon conversions are
recorded for troubleshooting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b042e858-04bc-4f7e-aa7c-53ca7e58cb29
📒 Files selected for processing (3)
Ginger/Ginger/Drivers/DriversConfigsEditPages/AppiumDriverEditPage.xamlGinger/Ginger/Drivers/DriversConfigsEditPages/UFTCredentialsDialog.xamlGinger/Ginger/Drivers/DriversConfigsEditPages/UFTCredentialsDialog.xaml.cs
| <ListBox.ItemContainerStyle> | ||
| <Style TargetType="ListBoxItem"> | ||
| <Setter Property="Padding" Value="0" /> | ||
| <Setter Property="Margin" Value="0" /> | ||
| <Setter Property="Background" Value="Transparent" /> | ||
| <Setter Property="BorderThickness" Value="0" /> | ||
| <Setter Property="FocusVisualStyle" Value="{x:Null}" /> | ||
| <EventSetter Event="PreviewMouseLeftButtonDown" Handler="AppItem_PreviewMouseLeftButtonDown" /> | ||
| <Setter Property="Template"> | ||
| <Setter.Value> | ||
| <ControlTemplate TargetType="ListBoxItem"> | ||
| <ContentPresenter /> | ||
| </ControlTemplate> | ||
| </Setter.Value> | ||
| </Setter> | ||
| </Style> |
There was a problem hiding this comment.
Consider preserving keyboard focus visual for accessibility.
Setting FocusVisualStyle="{x:Null}" removes the keyboard focus indicator, which may impact users who navigate using keyboard. This matches the Devices tab pattern, but consider adding a subtle focus visual or border change for accessibility compliance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Ginger/Ginger/Drivers/DriversConfigsEditPages/UFTCredentialsDialog.xaml`
around lines 325 - 340, The ListBox.ItemContainerStyle currently disables
keyboard focus visuals via FocusVisualStyle="{x:Null}", which harms
accessibility; instead restore a subtle focus visual: create a reusable
FocusVisualStyle resource (or a minimal Setter on the same Style) that adds a
non-intrusive border/outline or background change for ListBoxItem when focused,
and reference it from the ListBox.ItemContainerStyle (replace the
FocusVisualStyle="{x:Null}" Setter). Keep other behavior
(PreviewMouseLeftButtonDown handler AppItem_PreviewMouseLeftButtonDown and the
ControlTemplate/ContentPresenter) unchanged.
| private static ImageSource ConvertBase64ToImage(string base64) | ||
| { | ||
| if (string.IsNullOrWhiteSpace(base64)) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| try | ||
| { | ||
| byte[] bytes = Convert.FromBase64String(base64); | ||
| using var ms = new System.IO.MemoryStream(bytes); | ||
| var bitmap = new BitmapImage(); | ||
| bitmap.BeginInit(); | ||
| bitmap.CacheOption = BitmapCacheOption.OnLoad; | ||
| bitmap.StreamSource = ms; | ||
| bitmap.DecodePixelWidth = 56; | ||
| bitmap.EndInit(); | ||
| bitmap.Freeze(); | ||
| return bitmap; | ||
| } | ||
| catch | ||
| { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider logging failed icon conversions for troubleshooting.
The bare catch block silently swallows all exceptions without logging. While the fallback behavior (returning null) is appropriate for UI resilience, consider adding debug-level logging to aid troubleshooting when icons fail to load.
🔧 Suggested improvement
catch
{
+ // Optionally log at DEBUG level for troubleshooting
+ // Reporter.ToLog(eLogLevel.DEBUG, $"Failed to decode app icon from base64");
return null;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Ginger/Ginger/Drivers/DriversConfigsEditPages/UFTCredentialsDialog.xaml.cs`
around lines 1565 - 1589, The ConvertBase64ToImage method currently swallows all
exceptions in its catch block; update that catch to capture the Exception (e.g.,
catch (Exception ex)) and write a debug-level log entry including the exception
message/stack and minimal context (such as whether base64 was null/empty or its
length) using the project's logging facility (or System.Diagnostics.Debug/Trace
if no logger exists) before returning null so failed icon conversions are
recorded for troubleshooting.
| } | ||
| catch | ||
| { | ||
| return null; |
There was a problem hiding this comment.
add debug log to catch exception
in the uft lab phone selection (in the phone agent config)
added another tab to the phone selection for selecting apps
Summary by CodeRabbit
New Features
Improvements