Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

Ginger/GingerCoreNET/ActionsLib/UI/Web/ActAccessibilityTesting.cs (3)

66-68: 🛠️ Refactor suggestion

Use an enum instead of magic strings for CurrentRuleType

The string property invites typos and makes switch/if statements brittle. Define a strongly-typed enum to represent the platform type.

Define an enum and update the property:

+public enum RulePlatformType
+{
+    Web,
+    Mobile
+}
+
 // Public property to set the type of rules to fetch
-public string CurrentRuleType { get; set; } = "Web"; // Default to "Web"
+public RulePlatformType CurrentRuleType { get; set; } = RulePlatformType.Web;

Then update all string comparisons throughout the file to use the enum.

---

918-925: 🛠️ Refactor suggestion

Confusing builder usage in mobile analyzer invocation

The method creates a MobileAccessibilityAnalyzer instance named axeBuilder and then passes this same instance into its own AnalyzerMobileAccessibility method, which is confusing.

Rename the variable and simplify the call:

 public void AnalyzerMobileAccessibility(IWebDriver driver, IWebElement elementXPath = null)
 {
     try
     {
         Artifacts = [];
-        MobileAccessibilityAnalyzer axeBuilder = new MobileAccessibilityAnalyzer(driver, ActiveRulesForAnalysis);
-        axeBuilder.AnalyzerMobileAccessibility(driver, elementXPath, currentAct: this, axeBuilder);
+        var analyzer = new MobileAccessibilityAnalyzer(driver, ActiveRulesForAnalysis);
+        analyzer.AnalyzerMobileAccessibility(driver, elementXPath, currentAct: this);
     }
     catch (Exception ex)
     {
         Error = "Error during mobile accessibility testing: " + ex.Message;
         Reporter.ToLog(eLogLevel.ERROR, "Error during mobile accessibility testing", ex);
     }
 }

Note: This assumes the MobileAccessibilityAnalyzer.AnalyzerMobileAccessibility method signature will be updated to not require passing the analyzer instance to itself.

---

934-936: 🛠️ Refactor suggestion

platformType parameter is unused

AnalyzerAccessibility(IWebDriver Driver, IWebElement element, ePlatformType platformType) ignores platformType, risking divergence between caller intent and behaviour.

Either remove the unused parameter:

-public void AnalyzerAccessibility(IWebDriver Driver, IWebElement element, ePlatformType platformType)
+public void AnalyzerAccessibility(IWebDriver Driver, IWebElement element)

Or implement platform-specific logic if the parameter is intended for future use.

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

Ginger/Ginger/SolutionWindows/AccessibilityRulePage.xaml.cs (1)

51-52: Replace ternary with the idiomatic null-coalescing assignment

The same list instance can be preserved more succinctly and safely:

-            mAccessibilityConfiguration.ExcludedRules = mAccessibilityConfiguration.ExcludedRules != null ? mAccessibilityConfiguration.ExcludedRules : [];
+            mAccessibilityConfiguration.ExcludedRules ??= new();

Benefits: shorter, clearer, avoids evaluating the property twice.

Ginger/Ginger/SolutionWindows/AccessibilityRulePage.xaml.cs (1)

101-105: UI elements should live in XAML, not be spawned in code-behind
This segment repeats the pattern previously flagged – constructing radio buttons and spacer labels imperatively. Moving these controls to the .xaml keeps presentation separate from logic, simplifies styling, and eliminates noisy constructor code.

Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (3)

50-50: Remove unsupported namespace OpenQA.Selenium.Common
This namespace does not exist in Selenium 4.x and will break compilation. Remove it entirely or replace with the correct support namespace (OpenQA.Selenium.Support.UI) if needed.

---

405-405: Specify generic type for AppiumDriver
AppiumDriver is a generic abstract class (AppiumDriver<TWebElement>). Update the field declaration to include the generic parameter for compatibility and to avoid CS0308.

Apply this diff:

-protected AppiumDriver MobDriver;
+protected AppiumDriver<IWebElement> MobDriver;

---

3511-3518: Correct exception message and improve JS attribute setting
Fix the typo in the exception text and switch to setAttribute for robust handling of all attribute names (including those with hyphens).

Use this patch:

-    if (vals.Length != 2)
-    {
-        throw new Exception(@"Inot string should be in the format : attribute=value");
-    } ((IJavaScriptExecutor)Driver).ExecuteScript("arguments[0]." + vals[0] + "=arguments[1]", e, vals[1]);
+    if (vals.Length != 2)
+    {
+        throw new Exception(@"Input string should be in the format: attribute=value");
+    }
+    ((IJavaScriptExecutor)Driver)
+        .ExecuteScript(
+            "arguments[0].setAttribute(arguments[1], arguments[2]);",
+            e,
+            vals[0].Trim(),
+            vals[1]
+        );

Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/MobileAccessibilityAnalyzer.cs (4)

26-29: Hard-coded viewport size still present – breaks off-screen detection
Constants SCREEN_WIDTH_PX/SCREEN_HEIGHT_PX remain fixed at 1080×2220. Any device with a different resolution will mis-report Element_OffScreen/Element_PartiallyOffScreen. Retrieve the size once from the driver (((IHasSize)_driver).Size) and use those values.

---

455-468: Logic re-uses the same hard-coded dimensions
The off-screen check here relies on the constants above, so it inherits the same inaccuracy. Once the constants are removed, update this block to use the runtime size variables.

---

760-766: Dynamic strings injected into HTML without encoding
issue.Description, issue.ElementIdentifier, and issue.SuggestedFix are interpolated directly, risking broken or malicious HTML. Run each through HttpUtility.HtmlEncode (or equivalent) before appending.

---

624-632: Redundant mobileAnalyzer parameter still here
AnalyzerMobileAccessibility is an instance method but still receives a MobileAccessibilityAnalyzer parameter that is always this. Drop the parameter and fix callers to remove the self-passing.

Ginger/GingerCoreNET/ActionsLib/UI/Web/ActAccessibilityTesting.cs (3)

66-68: Use an enum instead of string for CurrentRuleType
Storing "Web" / "Mobile" as raw strings invites typos. Replace with a strongly-typed enum (e.g., RulePlatformType) and update usages.

---

534-544: Filename typo & brittle string comparison remain
"MobileAccessiblityRules.json" is still misspelled; it should be "MobileAccessibilityRules.json".
Also, string comparisons on platType are brittle—switch to the suggested enum and switch/case.

---

907-915: Self-passing of analyzer instance makes the call confusing
axeBuilder.AnalyzerMobileAccessibility(driver, elementXPath, currentAct: this, axeBuilder); passes the analyzer into its own method. After removing the redundant parameter, the call should simply be analyzer.AnalyzerMobileAccessibility(driver, elementXPath, currentAct: this);.

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

Learnt from: prashelke
PR: Ginger-Automation/Ginger#4232
File: Ginger/GingerCoreNET/ActionsLib/UI/VisualTesting/VRTAnalyzer.cs:19-22
Timestamp: 2025-06-16T10:37:13.024Z
Learning: In Ginger codebase, both `using amdocs.ginger.GingerCoreNET;` and `using Amdocs.Ginger.CoreNET;` are valid and serve different purposes. The first (lowercase) contains the WorkSpace class and related workspace functionality, while the second (proper case) contains drivers like GenericAppiumDriver and other core functionality. Both may be required in files that use types from both namespaces.

Learnt from: prashelke
PR: Ginger-Automation/Ginger#4232
File: Ginger/GingerCoreNET/ActionsLib/UI/VisualTesting/VRTAnalyzer.cs:19-22
Timestamp: 2025-06-16T09:52:40.854Z
Learning: The `GenericAppiumDriver` class is located in the `Amdocs.Ginger.CoreNET` namespace, not in a namespace that matches its file path structure. File paths don't always correspond to namespace declarations in this codebase.