Mobile POM Fine tune with AI Added#4313
Merged
Merged
Conversation
Contributor
WalkthroughExposes POMUtils publicly and attaches it to Selenium and GenericAppium drivers with processing state/events; moves AI fine-tune batching into POMUtils (device-platform-aware); updates drivers and wizard to subscribe/unsubscribe POMUtils, adjusts WizardWindow.ShowWizard default width, extends OpenAI call to accept device platform, and expands POM learn UI gating. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Wizard as WizardWindow
participant Page as POMLearnPage
participant Driver as Selenium/GenericAppium
participant POM as POMUtils
participant AI as GenAI
User->>Wizard: Open POM Learn Wizard
Wizard->>Driver: Subscribe (Selenium or Appium)
Note over Driver,POM: Driver exposes public POMUtils and wires ProcessingStatusChanged
Page->>Driver: Start discovery (GetVisibleControls)
loop per found element
Driver->>POM: TriggerFineTuneWithAI(element, category, devicePlatform)
end
POM->>AI: Send batched payload (includes platform)
AI-->>POM: Response
POM->>Driver: Update elements / mark processed
POM-->>Wizard: ProcessingStatusChanged(start/stop)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
Ginger/GingerCoreNET/GeneralLib/General.cs (3)
1405-1421: Don’t set Authorization header when token is empty; simplify local mode.Avoid sending “Bearer ” with an empty token; keep the header off in local runs if token missing.
Apply this diff:
- string bearerToken = GetLocalSetup() == "True" ? GetLocalToken() : gingerPlayAPITokenManager.GetValidToken(); - if (!string.IsNullOrEmpty(bearerToken)) - { - client.DefaultRequestHeaders.Authorization = new System.Net.Http.Headers.AuthenticationHeaderValue("Bearer", bearerToken); - } - else - { - if(GetLocalSetup() == "True") - { - client.DefaultRequestHeaders.Authorization = new System.Net.Http.Headers.AuthenticationHeaderValue("Bearer", bearerToken); - } - else - { - Reporter.ToLog(eLogLevel.DEBUG, $"Response: Invalid token"); - Response = $"Response: Invalid token"; - } - } + string bearerToken = GetLocalSetup() == "True" ? GetLocalToken() : gingerPlayAPITokenManager.GetValidToken(); + if (!string.IsNullOrEmpty(bearerToken)) + { + client.DefaultRequestHeaders.Authorization = + new System.Net.Http.Headers.AuthenticationHeaderValue("Bearer", bearerToken); + } + else if (GetLocalSetup() == "True") + { + // Local mode without token: skip Authorization header entirely + Reporter.ToLog(eLogLevel.DEBUG, "Local setup without token: skipping Authorization header"); + } + else + { + Reporter.ToLog(eLogLevel.DEBUG, "Response: Invalid token"); + Response = "Response: Invalid token"; + }
1430-1439: Improve error detail on non‑success responses.Consider logging response.StatusCode and body when available before returning a generic failure. Also consider a shared HttpClient or explicit timeouts for resilience.
1385-1389: Add back-compat overload to preserve old (jsonstring, url) signature
Insert an overload forwarding(string jsonstring, string url = null)to the new method to avoid breaking any positional callers that expect the two-arg signature:public class General { @@ + // Back-compat overload: preserves old (jsonstring, url) signature + public static Task<string> GetResponseForprocess_extracted_elementsByOpenAI(string jsonstring, string url = null) + => GetResponseForprocess_extracted_elementsByOpenAI(jsonstring, null, url); + public static async Task<string> GetResponseForprocess_extracted_elementsByOpenAI( string jsonstring, eDevicePlatformType? DevicePlatformType = null, string url = null)Ginger/Ginger/ApplicationModelsLib/POMModels/POMWizardLib/LearnWizard/POMLearnConfigWizardPage.xaml.cs (2)
115-121: Rename for clarity: method now subscribes to any driver, not just Selenium.Consider renaming to SubscribeToCurrentDriver to reflect expanded logic.
227-241: Gate AI POM controls by both BetaFeatures and configuration. RequireWorkSpace.Instance.BetaFeatures.ShowPOMForAIalongsideGingerPlayConfiguration.IsGingerPlayConfigured()(and preserve thexAgentControlUC.AgentIsRunningcheck for enabling) when settingxLearnPOMByAI.VisibilityandxLearnPOMByAI.IsEnabled.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
Ginger/Ginger/ApplicationModelsLib/POMModels/POMWizardLib/LearnWizard/POMLearnConfigWizardPage.xaml.cs(3 hunks)Ginger/Ginger/SolutionWindows/TreeViewItems/AgentsFolderTreeItem.cs(1 hunks)Ginger/Ginger/WizardLib/WizardWindow.xaml.cs(5 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs(5 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs(7 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs(4 hunks)Ginger/GingerCoreNET/GeneralLib/General.cs(3 hunks)
🧰 Additional context used
🧠 Learnings (11)
📚 Learning: 2025-06-16T10:37:13.073Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4232
File: Ginger/GingerCoreNET/ActionsLib/UI/VisualTesting/VRTAnalyzer.cs:19-22
Timestamp: 2025-06-16T10:37:13.073Z
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.
Applied to files:
Ginger/GingerCoreNET/GeneralLib/General.csGinger/Ginger/WizardLib/WizardWindow.xaml.csGinger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.csGinger/Ginger/ApplicationModelsLib/POMModels/POMWizardLib/LearnWizard/POMLearnConfigWizardPage.xaml.cs
📚 Learning: 2025-04-25T13:29:45.059Z
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4188
File: Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs:22-23
Timestamp: 2025-04-25T13:29:45.059Z
Learning: The `using Amdocs.Ginger.Repository;` statement in Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs is necessary as it provides access to the `ObservableList<>` class which is used throughout the file for collections of RunSetConfig, AnalyzerItemBase, and ApplicationPOMModel objects.
Applied to files:
Ginger/GingerCoreNET/GeneralLib/General.csGinger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.csGinger/Ginger/WizardLib/WizardWindow.xaml.csGinger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.csGinger/Ginger/ApplicationModelsLib/POMModels/POMWizardLib/LearnWizard/POMLearnConfigWizardPage.xaml.cs
📚 Learning: 2025-04-25T13:29:45.059Z
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4188
File: Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs:22-23
Timestamp: 2025-04-25T13:29:45.059Z
Learning: The `using Amdocs.Ginger.Repository;` statement in Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs is necessary as it imports the ObservableList<> class which is used throughout the file.
Applied to files:
Ginger/GingerCoreNET/GeneralLib/General.csGinger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.csGinger/Ginger/WizardLib/WizardWindow.xaml.csGinger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.csGinger/Ginger/ApplicationModelsLib/POMModels/POMWizardLib/LearnWizard/POMLearnConfigWizardPage.xaml.cs
📚 Learning: 2025-06-16T09:52:40.873Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4232
File: Ginger/GingerCoreNET/ActionsLib/UI/VisualTesting/VRTAnalyzer.cs:19-22
Timestamp: 2025-06-16T09:52:40.873Z
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.
Applied to files:
Ginger/GingerCoreNET/GeneralLib/General.csGinger/Ginger/WizardLib/WizardWindow.xaml.csGinger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.csGinger/Ginger/ApplicationModelsLib/POMModels/POMWizardLib/LearnWizard/POMLearnConfigWizardPage.xaml.cs
📚 Learning: 2025-03-20T11:10:30.816Z
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4137
File: Ginger/Ginger/SourceControl/SourceControlProjectsPage.xaml.cs:21-21
Timestamp: 2025-03-20T11:10:30.816Z
Learning: The `Amdocs.Ginger.Common.SourceControlLib` namespace is required in files that reference the `GingerSolution` class for source control operations in the Ginger automation framework.
Applied to files:
Ginger/GingerCoreNET/GeneralLib/General.csGinger/Ginger/WizardLib/WizardWindow.xaml.csGinger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.csGinger/Ginger/ApplicationModelsLib/POMModels/POMWizardLib/LearnWizard/POMLearnConfigWizardPage.xaml.cs
📚 Learning: 2025-08-22T16:31:58.569Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4280
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs:171-174
Timestamp: 2025-08-22T16:31:58.569Z
Learning: In GingerCoreNET, the canonical FilterProperties list lives in Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs and should not be duplicated in other classes (e.g., SeleniumDriver.cs).
Applied to files:
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.csGinger/Ginger/WizardLib/WizardWindow.xaml.csGinger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.csGinger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs
📚 Learning: 2025-07-16T14:42:32.229Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4254
File: Ginger/Ginger/ApplicationModelsLib/POMModels/POMWizardLib/LearnWizard/POMLearnConfigWizardPage.xaml.cs:70-73
Timestamp: 2025-07-16T14:42:32.229Z
Learning: In Ginger/Ginger/ApplicationModelsLib/POMModels/POMWizardLib/LearnWizard/POMLearnConfigWizardPage.xaml.cs, prashelke prefers to keep the current `!WorkSpace.Instance.BetaFeatures.ShowPOMForAI` logic for binding and showing AI POM controls, rather than changing it to positive logic.
Applied to files:
Ginger/Ginger/WizardLib/WizardWindow.xaml.csGinger/Ginger/ApplicationModelsLib/POMModels/POMWizardLib/LearnWizard/POMLearnConfigWizardPage.xaml.cs
📚 Learning: 2025-09-04T15:43:57.789Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4294
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs:9146-9291
Timestamp: 2025-09-04T15:43:57.789Z
Learning: In SeleniumDriver.cs, prefer using the C# fallback GenerateEnhancedSvgXPath(IWebElement) for SVG XPath generation instead of injecting/validating a JavaScript-based XPath builder. This was accepted in PR Ginger-Automation/Ginger#4294.
Applied to files:
Ginger/Ginger/ApplicationModelsLib/POMModels/POMWizardLib/LearnWizard/POMLearnConfigWizardPage.xaml.csGinger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs
📚 Learning: 2025-06-19T05:08:45.192Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4222
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs:2645-2656
Timestamp: 2025-06-19T05:08:45.192Z
Learning: In the Ginger mobile automation framework (GenericAppiumDriver.cs), XPath checks for "android" and "XCUIElement" should remain case-sensitive as currently implemented. The team prefers not to make these checks case-insensitive.
Applied to files:
Ginger/Ginger/ApplicationModelsLib/POMModels/POMWizardLib/LearnWizard/POMLearnConfigWizardPage.xaml.cs
📚 Learning: 2025-09-04T11:19:44.719Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4294
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs:10651-10658
Timestamp: 2025-09-04T11:19:44.719Z
Learning: In Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs, maintainer prefers not to keep the helper IsSvgElementByWebElement(IWebElement); rely on the existing enum-based IsSvgElement(eElementType) helper instead. Future reviews should not reintroduce IsSvgElementByWebElement.
Applied to files:
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs
📚 Learning: 2025-09-04T10:00:39.311Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4294
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs:5731-5734
Timestamp: 2025-09-04T10:00:39.311Z
Learning: In SeleniumDriver.ProcessSvgChildElements, always search SVG children within the current ISearchContext (frame/shadow) instead of using Driver.FindElement, and pass parentContext from FindAllElementsFromPOM.
Applied to files:
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs
🧬 Code graph analysis (7)
Ginger/GingerCoreNET/GeneralLib/General.cs (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs (5)
Task(130-189)Task(191-195)Task(368-396)Task(398-413)Task(415-418)
Ginger/Ginger/SolutionWindows/TreeViewItems/AgentsFolderTreeItem.cs (1)
Ginger/Ginger/WizardLib/WizardWindow.xaml.cs (4)
WizardWindow(40-608)WizardWindow(76-104)WizardWindow(120-123)ShowWizard(51-74)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs (4)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (31)
ElementInfo(6599-6664)ElementInfo(6745-6761)ElementInfo(6763-6774)ElementInfo(8150-8153)ElementInfo(8155-8214)ElementInfo(8236-8296)ElementInfo(9477-9498)ElementInfo(9557-9608)ElementInfo(11784-11795)ElementInfo(11797-11801)ElementInfo(11803-11836)ElementInfo(11931-11938)ElementInfo(11940-12001)ElementInfo(12075-12100)ElementInfo(12102-12127)ElementInfo(12312-12333)Task(5424-5489)Task(8403-8440)Task(11412-11500)Task(12458-12482)Task(12692-12722)Task(12724-12742)Task(12749-12780)Task(12782-12805)Task(12887-12891)ObservableList(5682-5773)ObservableList(7344-7368)ObservableList(7687-7846)ObservableList(7961-8053)ObservableList(9974-9977)ObservableList(12396-12399)Ginger/GingerCoreNET/GeneralLib/General.cs (10)
Task(634-655)Task(1079-1162)Task(1181-1212)Task(1214-1217)Task(1331-1383)Task(1385-1449)ObservableList(318-323)ObservableList(845-876)ObservableList(997-1078)General(60-1488)Ginger/GingerCoreNET/GeneralLib/ElementWrapperInfo.cs (1)
ElementWrapperInfo(29-32)Ginger/GingerCoreCommon/Repository/ApplicationModelLib/POMModelLib/PomSetting.cs (1)
PomSetting(27-68)
Ginger/Ginger/WizardLib/WizardWindow.xaml.cs (2)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs (1)
POMUtils(40-430)Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs (2)
GenericAppiumDriver(73-5495)GenericAppiumDriver(279-282)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs (3)
POMUtils(40-430)TiggerFineTuneWithAI(328-341)TiggerDelayProcessingfinetuneWithAI(343-352)
Ginger/Ginger/ApplicationModelsLib/POMModels/POMWizardLib/LearnWizard/POMLearnConfigWizardPage.xaml.cs (5)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (5)
SeleniumDriver(85-12910)SeleniumDriver(504-507)SeleniumDriver(509-515)SeleniumDriver(517-520)SeleniumDriver(522-525)Ginger/Ginger/WizardLib/WizardWindow.xaml.cs (5)
WizardWindow(40-608)WizardWindow(76-104)WizardWindow(120-123)SubscribeToSeleniumDriver(521-534)SubscribeToAppiumDriver(560-574)Ginger/GingerCoreCommon/ReporterLib/Reporter.cs (1)
Reporter(28-357)Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs (2)
GenericAppiumDriver(73-5495)GenericAppiumDriver(279-282)Ginger/GingerCoreCommon/External/Configurations/GingerPlayConfiguration.cs (2)
GingerPlayConfiguration(23-240)IsGingerPlayConfigured(232-238)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs (3)
POMUtils(40-430)TiggerFineTuneWithAI(328-341)TiggerDelayProcessingfinetuneWithAI(343-352)
🔇 Additional comments (5)
Ginger/Ginger/SolutionWindows/TreeViewItems/AgentsFolderTreeItem.cs (2)
84-85: Good: migrate to default wizard sizing.Removing explicit width/height aligns with the new ShowWizard defaults and centralizes sizing.
125-129: Remove explicit wizard size in AddTreeItem
WizardWindow.ShowWizard defaults to 1000×800; drop the1100, 1100parameters for consistency:- WizardWindow.ShowWizard(new AddAgentWizard(mAgentsFolder), 1100, 1100); + WizardWindow.ShowWizard(new AddAgentWizard(mAgentsFolder));Was the
1100×1100override intentional for this path?Ginger/GingerCoreNET/GeneralLib/General.cs (2)
24-26: Namespace import is appropriate.Brings in eDevicePlatformType used by the new API.
1394-1399: Verifyelementsfield type with AI service API
Please confirm that the AI service endpoint (returned byGetAIServicePOMProcessExtractedElementsPath) expectselementsas a JSON array/object rather than a string before applying the suggested change.Ginger/Ginger/ApplicationModelsLib/POMModels/POMWizardLib/LearnWizard/POMLearnConfigWizardPage.xaml.cs (1)
128-155: Good: unified subscription to Selenium and Appium POMUtils events.Covers both drivers and avoids duplicate subscriptions via WizardWindow’s internal unsubscribe.
Please confirm WizardWindow.SubscribeToAppiumDriver/SubscribeToSeleniumDriver are always called on the UI thread, as they update UI state via Dispatcher.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs (2)
216-225: Null-conditional bug parsing AI response (possible NRE).
jObject["data"]?["genai_result"].ToString()will throw whendataorgenai_resultare missing because.ToString()is called on null and the second indexer isn’t null-safe. Use SelectToken and null-conditional ToString:- var jObject = JObject.Parse(cleanedResponse); - var genaiResultToken = jObject["data"]?["genai_result"].ToString(); - if (genaiResultToken != null) - { - responseElements = JsonConvert.DeserializeObject<List<ElementWrapper>>((string)genaiResultToken); - } + var jObject = JObject.Parse(cleanedResponse); + var genaiResult = jObject.SelectToken("data.genai_result")?.ToString(); + if (!string.IsNullOrWhiteSpace(genaiResult)) + { + responseElements = JsonConvert.DeserializeObject<List<ElementWrapper>>(genaiResult); + }
281-291: Guard locators list and skip unknown locate types.
- Ensure
existingElement.Locatorsis not null beforeAdd.- Skipping unknown types is safer than defaulting to ByRelXPath.
- if (!Enum.TryParse(kvp.Key, true, out locateBy)) - { - Reporter.ToLog(eLogLevel.DEBUG, $"Unknown locator type '{kvp.Key}', defaulting to ByRelXPath"); - locateBy = eLocateBy.ByRelXPath; - } + if (!Enum.TryParse(kvp.Key, true, out locateBy)) + { + Reporter.ToLog(eLogLevel.DEBUG, $"Unknown locator type '{kvp.Key}', skipping unknown locator '{kvp.Key}'"); + continue; + } @@ - existingElement.Locators.Add(locator); + if (existingElement.Locators == null) + { + existingElement.Locators = new ObservableList<ElementLocator>(); + } + existingElement.Locators.Add(locator);Also applies to: 270-277
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs(7 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-22T16:31:58.569Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4280
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs:171-174
Timestamp: 2025-08-22T16:31:58.569Z
Learning: In GingerCoreNET, the canonical FilterProperties list lives in Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs and should not be duplicated in other classes (e.g., SeleniumDriver.cs).
Applied to files:
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs
📚 Learning: 2025-04-25T13:29:45.059Z
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4188
File: Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs:22-23
Timestamp: 2025-04-25T13:29:45.059Z
Learning: The `using Amdocs.Ginger.Repository;` statement in Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs is necessary as it provides access to the `ObservableList<>` class which is used throughout the file for collections of RunSetConfig, AnalyzerItemBase, and ApplicationPOMModel objects.
Applied to files:
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs
📚 Learning: 2025-04-25T13:29:45.059Z
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4188
File: Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs:22-23
Timestamp: 2025-04-25T13:29:45.059Z
Learning: The `using Amdocs.Ginger.Repository;` statement in Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs is necessary as it imports the ObservableList<> class which is used throughout the file.
Applied to files:
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs
🧬 Code graph analysis (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs (4)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (25)
ElementInfo(6599-6664)ElementInfo(6745-6761)ElementInfo(6763-6774)ElementInfo(8150-8153)ElementInfo(8155-8214)ElementInfo(8236-8296)ElementInfo(9477-9498)ElementInfo(9557-9608)ElementInfo(11784-11795)ElementInfo(11797-11801)ElementInfo(11803-11836)ElementInfo(11931-11938)ElementInfo(11940-12001)ElementInfo(12075-12100)ElementInfo(12102-12127)ElementInfo(12312-12333)Task(5424-5489)Task(8403-8440)Task(11412-11500)Task(12458-12482)Task(12692-12722)Task(12724-12742)Task(12749-12780)Task(12782-12805)Task(12887-12891)Ginger/GingerCoreNET/GeneralLib/General.cs (7)
Task(634-655)Task(1079-1162)Task(1181-1212)Task(1214-1217)Task(1331-1383)Task(1385-1449)General(60-1488)Ginger/GingerCoreNET/GeneralLib/ElementWrapperInfo.cs (1)
ElementWrapperInfo(29-32)Ginger/GingerCoreCommon/Repository/ApplicationModelLib/POMModelLib/PomSetting.cs (1)
PomSetting(27-68)
🔇 Additional comments (2)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs (2)
43-47: Good: canonical FilterProperties kept in POMUtils.This aligns with the repo convention to centralize the filter list here. Based on learnings
341-350: Fix delayed processing: set IsProcessing, pass DevicePlatformType, and lock.Delayed path still never flips IsProcessing and drops the platform, so UI never shows spinner and mobile batches default to "web". Apply:
-public void TiggerDelayProcessingfinetuneWithAI(PomSetting pomSetting, ePomElementCategory? PomCategory) +public void TiggerDelayProcessingfinetuneWithAI(PomSetting pomSetting, ePomElementCategory? PomCategory, eDevicePlatformType? DevicePlatformType = null) { if (pomSetting.LearnPOMByAI) { - if (processingQueue.Count < 10 && !IsProcessing) - { - _ = Task.Run(() => TriggerDelayedProcessing(pomSetting,PomCategory)); - } + if (processingQueue.Count < 10) + { + lock (lockObj) + { + if (!IsProcessing) + { + IsProcessing = true; + _ = Task.Run(() => TriggerDelayedProcessing(pomSetting, PomCategory, DevicePlatformType)); + } + } + } } }Run to update and check call sites after signature change:
#!/bin/bash rg -nP --type=cs 'TiggerDelayProcessingfinetuneWithAI\s*\(' -C3
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (2)
499-530: Wire POMUtils events from every constructorOnly the parameterless ctor attaches
POMUtils_ProcessingStatusChanged, but most call sites usenew SeleniumDriver(eBrowserType …)ornew SeleniumDriver(object driver). Those ctors skip the subscription, soIsProcessingnever updates for the UI and the busy spinner regresses. Either chain the other ctors tothis()or call the wiring helper there as well.
5751-5780: Pass the device platform into TriggerFineTuneWithAI
POMUtils.TriggerFineTuneWithAI(...)now expects the device platform so mobile sessions can reach the new tuning flow, but both calls here supplynull. For Appium-backed runs this routes everything down the web prompts and defeats the “Mobile Fine tune with AI” work. Please resolve the platform (e.g., frompomSetting?.DevicePlatformTypeorisAppiumSession/MobDriver) and pass it through on both trigger sites.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
Ginger/Ginger/ApplicationModelsLib/POMModels/POMWizardLib/LearnWizard/POMLearnConfigWizardPage.xaml.cs(3 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs(6 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs(10 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs(9 hunks)
🧰 Additional context used
🧠 Learnings (24)
📚 Learning: 2025-07-16T14:42:32.229Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4254
File: Ginger/Ginger/ApplicationModelsLib/POMModels/POMWizardLib/LearnWizard/POMLearnConfigWizardPage.xaml.cs:70-73
Timestamp: 2025-07-16T14:42:32.229Z
Learning: In Ginger/Ginger/ApplicationModelsLib/POMModels/POMWizardLib/LearnWizard/POMLearnConfigWizardPage.xaml.cs, prashelke prefers to keep the current `!WorkSpace.Instance.BetaFeatures.ShowPOMForAI` logic for binding and showing AI POM controls, rather than changing it to positive logic.
Applied to files:
Ginger/Ginger/ApplicationModelsLib/POMModels/POMWizardLib/LearnWizard/POMLearnConfigWizardPage.xaml.cs
📚 Learning: 2025-08-22T14:57:03.711Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4280
File: Ginger/Ginger/ApplicationModelsLib/POMModels/POMWizardLib/LearnWizard/POMLearnConfigWizardPage.xaml:71-103
Timestamp: 2025-08-22T14:57:03.711Z
Learning: In Ginger POM wizard, prashelke prefers to show AI-powered features as visible to users but disabled by default (Visibility="Visible", IsEnabled="False") to promote feature discoverability rather than hiding them completely. Users should be able to see what AI capabilities are available even when not yet enabled.
Applied to files:
Ginger/Ginger/ApplicationModelsLib/POMModels/POMWizardLib/LearnWizard/POMLearnConfigWizardPage.xaml.cs
📚 Learning: 2025-09-04T09:34:04.299Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4294
File: Ginger/GingerCoreCommon/UIElement/ElementInfo.cs:555-569
Timestamp: 2025-09-04T09:34:04.299Z
Learning: In Ginger/GingerCoreCommon/UIElement/ElementInfo.cs, the eLearnedType.AI enum value should be retained as it's required for future AI development features, as confirmed by prashelke.
Applied to files:
Ginger/Ginger/ApplicationModelsLib/POMModels/POMWizardLib/LearnWizard/POMLearnConfigWizardPage.xaml.cs
📚 Learning: 2025-09-04T15:42:00.330Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4294
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs:8334-8350
Timestamp: 2025-09-04T15:42:00.330Z
Learning: In PR Ginger-Automation/Ginger#4294, SeleniumDriver.cs: maintainer requested to keep the WebElement-based helper IsSvgElementByWebElement(IWebElement) and its current usages for now, deferring the replacement with GetElementTypeEnum(..) + IsSvgElement(eElementType) to a later PR.
Applied to files:
Ginger/Ginger/ApplicationModelsLib/POMModels/POMWizardLib/LearnWizard/POMLearnConfigWizardPage.xaml.csGinger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.csGinger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.csGinger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs
📚 Learning: 2025-06-19T05:08:45.192Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4222
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs:2645-2656
Timestamp: 2025-06-19T05:08:45.192Z
Learning: In the Ginger mobile automation framework (GenericAppiumDriver.cs), XPath checks for "android" and "XCUIElement" should remain case-sensitive as currently implemented. The team prefers not to make these checks case-insensitive.
Applied to files:
Ginger/Ginger/ApplicationModelsLib/POMModels/POMWizardLib/LearnWizard/POMLearnConfigWizardPage.xaml.csGinger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs
📚 Learning: 2025-09-04T15:43:57.789Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4294
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs:9146-9291
Timestamp: 2025-09-04T15:43:57.789Z
Learning: In SeleniumDriver.cs, prefer using the C# fallback GenerateEnhancedSvgXPath(IWebElement) for SVG XPath generation instead of injecting/validating a JavaScript-based XPath builder. This was accepted in PR Ginger-Automation/Ginger#4294.
Applied to files:
Ginger/Ginger/ApplicationModelsLib/POMModels/POMWizardLib/LearnWizard/POMLearnConfigWizardPage.xaml.csGinger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.csGinger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.csGinger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs
📚 Learning: 2025-04-25T13:29:45.059Z
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4188
File: Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs:22-23
Timestamp: 2025-04-25T13:29:45.059Z
Learning: The `using Amdocs.Ginger.Repository;` statement in Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs is necessary as it provides access to the `ObservableList<>` class which is used throughout the file for collections of RunSetConfig, AnalyzerItemBase, and ApplicationPOMModel objects.
Applied to files:
Ginger/Ginger/ApplicationModelsLib/POMModels/POMWizardLib/LearnWizard/POMLearnConfigWizardPage.xaml.csGinger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.csGinger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs
📚 Learning: 2025-04-25T13:29:45.059Z
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4188
File: Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs:22-23
Timestamp: 2025-04-25T13:29:45.059Z
Learning: The `using Amdocs.Ginger.Repository;` statement in Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs is necessary as it imports the ObservableList<> class which is used throughout the file.
Applied to files:
Ginger/Ginger/ApplicationModelsLib/POMModels/POMWizardLib/LearnWizard/POMLearnConfigWizardPage.xaml.csGinger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs
📚 Learning: 2025-06-16T10:37:13.073Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4232
File: Ginger/GingerCoreNET/ActionsLib/UI/VisualTesting/VRTAnalyzer.cs:19-22
Timestamp: 2025-06-16T10:37:13.073Z
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.
Applied to files:
Ginger/Ginger/ApplicationModelsLib/POMModels/POMWizardLib/LearnWizard/POMLearnConfigWizardPage.xaml.csGinger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs
📚 Learning: 2025-03-20T11:10:30.816Z
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4137
File: Ginger/Ginger/SourceControl/SourceControlProjectsPage.xaml.cs:21-21
Timestamp: 2025-03-20T11:10:30.816Z
Learning: The `Amdocs.Ginger.Common.SourceControlLib` namespace is required in files that reference the `GingerSolution` class for source control operations in the Ginger automation framework.
Applied to files:
Ginger/Ginger/ApplicationModelsLib/POMModels/POMWizardLib/LearnWizard/POMLearnConfigWizardPage.xaml.csGinger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs
📚 Learning: 2025-06-16T09:52:40.873Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4232
File: Ginger/GingerCoreNET/ActionsLib/UI/VisualTesting/VRTAnalyzer.cs:19-22
Timestamp: 2025-06-16T09:52:40.873Z
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.
Applied to files:
Ginger/Ginger/ApplicationModelsLib/POMModels/POMWizardLib/LearnWizard/POMLearnConfigWizardPage.xaml.csGinger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs
📚 Learning: 2025-09-04T16:40:09.511Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4294
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs:6019-6061
Timestamp: 2025-09-04T16:40:09.511Z
Learning: In Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (AddSvgSpecificLocators, around lines ~6000–6065) for PR Ginger-Automation/Ginger#4294, maintainer prashelke indicated that adjusting XPath to remove outer quotes around EscapeXPathString(...) is not required. Do not suggest changing the quoting around EscapeXPathString for SVG-specific locators in future reviews unless maintainers request it.
Applied to files:
Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.csGinger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs
📚 Learning: 2025-08-22T16:31:58.569Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4280
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs:171-174
Timestamp: 2025-08-22T16:31:58.569Z
Learning: In GingerCoreNET, the canonical FilterProperties list lives in Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs and should not be duplicated in other classes (e.g., SeleniumDriver.cs).
Applied to files:
Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.csGinger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.csGinger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs
📚 Learning: 2025-09-02T07:51:28.822Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4289
File: Ginger/GingerCoreNET/Application Models/Delta/PomDelta/PomDeltaUtils.cs:99-103
Timestamp: 2025-09-02T07:51:28.822Z
Learning: In Ginger/GingerCoreNET/Application Models/Delta/PomDelta/PomDeltaUtils.cs around line 99, prashelke prefers to keep the unused variable `selectedElementList` from GetSelectedElementList() call rather than removing it, despite it being unused in the current logic.
Applied to files:
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs
📚 Learning: 2025-07-16T14:17:56.429Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4254
File: Ginger/GingerCoreNET/GeneralLib/ElementWrapperInfo.cs:89-93
Timestamp: 2025-07-16T14:17:56.429Z
Learning: In Ginger/GingerCoreNET/GeneralLib/ElementWrapperInfo.cs, the user prashelke prefers to keep the current casing for properties `insideIframe` and `insideShadowDOM` in the `ElementwrapperContext` class rather than changing them to PascalCase.
Applied to files:
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs
📚 Learning: 2025-08-22T16:22:20.870Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4280
File: Ginger/GingerCoreNET/GeneralLib/ElementWrapperInfo.cs:41-41
Timestamp: 2025-08-22T16:22:20.870Z
Learning: In Ginger/GingerCoreNET/GeneralLib/ElementWrapperInfo.cs, the user prashelke prefers to keep the current lowercase naming for properties like `elementGuid` rather than changing them to PascalCase, likely for consistency within the ElementWrapperInfo data model classes.
Applied to files:
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs
📚 Learning: 2024-07-26T22:04:12.930Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#3429
File: Ginger/Ginger/AutomatePageLib/AddActionMenu/WindowExplorer/Common/WindowExplorerPage.xaml.cs:1139-1142
Timestamp: 2024-07-26T22:04:12.930Z
Learning: The user has updated the code by removing or addressing the commented code as suggested in the review.
Applied to files:
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs
📚 Learning: 2025-09-04T11:19:44.719Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4294
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs:10651-10658
Timestamp: 2025-09-04T11:19:44.719Z
Learning: In Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs, maintainer prefers not to keep the helper IsSvgElementByWebElement(IWebElement); rely on the existing enum-based IsSvgElement(eElementType) helper instead. Future reviews should not reintroduce IsSvgElementByWebElement.
Applied to files:
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs
📚 Learning: 2025-09-05T09:30:10.074Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4294
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs:5942-5960
Timestamp: 2025-09-05T09:30:10.074Z
Learning: In Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (around SVG helpers, approx. lines 5942–5960 and 5983–5984), maintainer prefers keeping ToLower() for tag/name normalization; do not suggest replacing with ToLowerInvariant() in future reviews for this area.
Applied to files:
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs
📚 Learning: 2025-09-04T09:55:20.543Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4294
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs:5789-5796
Timestamp: 2025-09-04T09:55:20.543Z
Learning: In Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (around line 5786), maintainer prashelke prefers to keep the helper method `IsSvgElement(eElementType)` as-is (no inlining or renaming), even though it currently checks only for `eElementType.Svg`. Future reviews should not suggest removing or renaming this helper.
Applied to files:
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs
📚 Learning: 2025-09-04T16:40:33.092Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4294
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs:9182-9223
Timestamp: 2025-09-04T16:40:33.092Z
Learning: In PR Ginger-Automation/Ginger#4294 (SeleniumDriver.cs GenerateSvgAttributeBasedXPath), maintainer declined changing XPath to remove extra quotes around EscapeXPathString(...) results; keep current quoting as-is in this area for now.
Applied to files:
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs
📚 Learning: 2025-07-10T07:11:28.974Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4249
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs:4684-4695
Timestamp: 2025-07-10T07:11:28.974Z
Learning: In Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs, attribute values used in CSS selectors (e.g., ByTitle, ByAriaLabel, ByDataTestId, ByPlaceholder) should be escaped using a helper method (such as EscapeCssAttributeValue) to prevent selector breakage due to special characters.
Applied to files:
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs
📚 Learning: 2025-07-10T07:12:52.786Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4249
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs:7199-7202
Timestamp: 2025-07-10T07:12:52.786Z
Learning: In Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs, text values used in XPath expressions should be escaped using a helper method (such as EscapeXPathString) to handle single quotes and ensure valid XPath syntax.
Applied to files:
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs
📚 Learning: 2025-09-04T10:00:39.311Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4294
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs:5731-5734
Timestamp: 2025-09-04T10:00:39.311Z
Learning: In SeleniumDriver.ProcessSvgChildElements, always search SVG children within the current ISearchContext (frame/shadow) instead of using Driver.FindElement, and pass parentContext from FindAllElementsFromPOM.
Applied to files:
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs
🧬 Code graph analysis (4)
Ginger/Ginger/ApplicationModelsLib/POMModels/POMWizardLib/LearnWizard/POMLearnConfigWizardPage.xaml.cs (3)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (6)
SeleniumDriver(85-12910)SeleniumDriver(499-503)SeleniumDriver(505-511)SeleniumDriver(522-525)SeleniumDriver(527-530)ePlatformType(12572-12575)Ginger/Ginger/WizardLib/WizardWindow.xaml.cs (5)
WizardWindow(40-612)WizardWindow(76-104)WizardWindow(120-123)SubscribeToSeleniumDriver(521-534)SubscribeToAppiumDriver(564-578)Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs (3)
GenericAppiumDriver(73-5488)GenericAppiumDriver(274-277)ePlatformType(4190-4193)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs (2)
Ginger/GingerCoreNET/RunLib/DriverBase.cs (1)
DriverBase(38-424)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs (3)
POMUtils(37-420)TriggerFineTuneWithAI(323-336)TriggerDelayProcessingfinetuneWithAI(338-347)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs (5)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (31)
ElementInfo(6599-6664)ElementInfo(6745-6761)ElementInfo(6763-6774)ElementInfo(8150-8153)ElementInfo(8155-8214)ElementInfo(8236-8296)ElementInfo(9477-9498)ElementInfo(9557-9608)ElementInfo(11784-11795)ElementInfo(11797-11801)ElementInfo(11803-11836)ElementInfo(11931-11938)ElementInfo(11940-12001)ElementInfo(12075-12100)ElementInfo(12102-12127)ElementInfo(12312-12333)Task(5434-5499)Task(8403-8440)Task(11412-11500)Task(12458-12482)Task(12692-12722)Task(12724-12742)Task(12749-12780)Task(12782-12805)Task(12887-12891)ObservableList(5692-5782)ObservableList(7344-7368)ObservableList(7687-7846)ObservableList(7961-8053)ObservableList(9974-9977)ObservableList(12396-12399)Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs (13)
Task(2315-2331)Task(2608-2718)Task(2731-2772)Task(2837-2840)Task(2846-2885)Task(3716-3881)Task(3893-3903)ObservableList(2969-3094)ObservableList(3104-3146)ObservableList(3210-3213)ObservableList(3481-3484)ObservableList(4423-4426)eDevicePlatformType(3491-3494)Ginger/GingerCoreNET/GeneralLib/ElementWrapperInfo.cs (1)
ElementWrapperInfo(29-32)Ginger/GingerCoreNET/GeneralLib/General.cs (4)
ObservableList(318-323)ObservableList(845-876)ObservableList(997-1078)General(60-1488)Ginger/GingerCoreCommon/Repository/ApplicationModelLib/POMModelLib/PomSetting.cs (1)
PomSetting(27-68)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs (3)
POMUtils(37-420)TriggerFineTuneWithAI(323-336)TriggerDelayProcessingfinetuneWithAI(338-347)
🔇 Additional comments (5)
Ginger/Ginger/ApplicationModelsLib/POMModels/POMWizardLib/LearnWizard/POMLearnConfigWizardPage.xaml.cs (5)
25-25: LGTM!The import addition for
Amdocs.Ginger.CoreNETis appropriate for accessing theGenericAppiumDriverclass used in the multi-driver subscription logic.
129-153: Enhanced multi-driver subscription logic looks good.The implementation correctly handles both SeleniumDriver and GenericAppiumDriver subscriptions through the WizardWindow, with appropriate null checks and logging for each driver type.
227-236: The agent running state logic is correctly implemented.Line 233 properly sets
xLearnPOMByAI.IsEnabled = xAgentControlUC.AgentIsRunning, ensuring the AI feature is only enabled when both GingerPlay is configured and the agent is running. This addresses the previous review concern about enabling the feature prematurely.
158-158: LGTM!The error message change from "Error subscribing to SeleniumDriver events" to "Error subscribing to Driver events" appropriately reflects the generic driver handling approach.
439-439: LGTM!The
UpdateConfigsBasedOnAgentStatusmethod correctly combines both conditions (xAgentControlUC.AgentIsRunning && GingerPlayConfiguration.IsGingerPlayConfigured()) for enabling the AI POM feature, maintaining consistency with the platform-specific logic.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs (5)
211-218: NullReference on missing data/genai_result.
.ToString()is called even when the token chain is null.- var genaiResultToken = jObject["data"]?["genai_result"].ToString(); - if (genaiResultToken != null) + var genaiResultToken = jObject["data"]?["genai_result"]?.ToString(); + if (!string.IsNullOrEmpty(genaiResultToken)) { responseElements = JsonConvert.DeserializeObject<List<ElementWrapper>>((string)genaiResultToken); }
240-251: Clear dedupe guard when an element is processed.Ensure future reprocessing is possible if needed and keep the set bounded.
existingElement.ElementName = enhancedName ?? existingElement.ElementName; existingElement.Description = enhancedDescription ?? existingElement.Description; existingElement.IsProcessed = true; + // allow future re-enqueue if needed + _enqueuedIds.TryRemove(existingElement.Guid, out _);
87-101: Property mapping misses names with spaces (e.g., "Platform Element Type").Reflection cannot find C# properties containing spaces. Add a whitespace-insensitive fallback.
- var propertyInfo = typeof(ElementwrapperProperties) - .GetProperty(prop.Name, BindingFlags.IgnoreCase | BindingFlags.Public | BindingFlags.Instance); + var propertyInfo = typeof(ElementwrapperProperties) + .GetProperty(prop.Name, BindingFlags.IgnoreCase | BindingFlags.Public | BindingFlags.Instance) + ?? typeof(ElementwrapperProperties) + .GetProperty(prop.Name.Replace(" ", ""), BindingFlags.IgnoreCase | BindingFlags.Public | BindingFlags.Instance);
276-289: Prevent duplicate locators.AI may return the same locator more than once. Avoid duplicates.
- if (!string.IsNullOrEmpty(kvp.Value)) + if (!string.IsNullOrEmpty(kvp.Value)) { var locator = new ElementLocator { LocateBy = locateBy, LocateValue = kvp.Value, IsAutoLearned = true, Category = PomCategory, IsAIGenerated = true, Active = true }; - existingElement.Locators.Add(locator); + if (!existingElement.Locators.Any(l => l.LocateBy == locator.LocateBy && l.LocateValue == locator.LocateValue)) + { + existingElement.Locators.Add(locator); + } }
129-131: Harden batch size parsing.Avoid FormatException and enforce sane bounds.
- batchSize = !string.IsNullOrEmpty(GingerCoreNET.GeneralLib.General.GetAIBatchsize()) ? Convert.ToInt32(GingerCoreNET.GeneralLib.General.GetAIBatchsize()) : batchSize; + if (int.TryParse(GingerCoreNET.GeneralLib.General.GetAIBatchsize(), out var configured) && configured > 0) + { + batchSize = configured; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs(10 hunks)
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-09-02T07:51:28.822Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4289
File: Ginger/GingerCoreNET/Application Models/Delta/PomDelta/PomDeltaUtils.cs:99-103
Timestamp: 2025-09-02T07:51:28.822Z
Learning: In Ginger/GingerCoreNET/Application Models/Delta/PomDelta/PomDeltaUtils.cs around line 99, prashelke prefers to keep the unused variable `selectedElementList` from GetSelectedElementList() call rather than removing it, despite it being unused in the current logic.
Applied to files:
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs
📚 Learning: 2025-07-16T14:17:56.429Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4254
File: Ginger/GingerCoreNET/GeneralLib/ElementWrapperInfo.cs:89-93
Timestamp: 2025-07-16T14:17:56.429Z
Learning: In Ginger/GingerCoreNET/GeneralLib/ElementWrapperInfo.cs, the user prashelke prefers to keep the current casing for properties `insideIframe` and `insideShadowDOM` in the `ElementwrapperContext` class rather than changing them to PascalCase.
Applied to files:
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs
📚 Learning: 2025-08-22T16:22:20.870Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4280
File: Ginger/GingerCoreNET/GeneralLib/ElementWrapperInfo.cs:41-41
Timestamp: 2025-08-22T16:22:20.870Z
Learning: In Ginger/GingerCoreNET/GeneralLib/ElementWrapperInfo.cs, the user prashelke prefers to keep the current lowercase naming for properties like `elementGuid` rather than changing them to PascalCase, likely for consistency within the ElementWrapperInfo data model classes.
Applied to files:
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs
📚 Learning: 2025-09-04T15:42:00.330Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4294
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs:8334-8350
Timestamp: 2025-09-04T15:42:00.330Z
Learning: In PR Ginger-Automation/Ginger#4294, SeleniumDriver.cs: maintainer requested to keep the WebElement-based helper IsSvgElementByWebElement(IWebElement) and its current usages for now, deferring the replacement with GetElementTypeEnum(..) + IsSvgElement(eElementType) to a later PR.
Applied to files:
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs
📚 Learning: 2025-09-04T15:43:57.789Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4294
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs:9146-9291
Timestamp: 2025-09-04T15:43:57.789Z
Learning: In SeleniumDriver.cs, prefer using the C# fallback GenerateEnhancedSvgXPath(IWebElement) for SVG XPath generation instead of injecting/validating a JavaScript-based XPath builder. This was accepted in PR Ginger-Automation/Ginger#4294.
Applied to files:
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs
📚 Learning: 2024-07-26T22:04:12.930Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#3429
File: Ginger/Ginger/AutomatePageLib/AddActionMenu/WindowExplorer/Common/WindowExplorerPage.xaml.cs:1139-1142
Timestamp: 2024-07-26T22:04:12.930Z
Learning: The user has updated the code by removing or addressing the commented code as suggested in the review.
Applied to files:
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs
📚 Learning: 2025-08-22T16:31:58.569Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4280
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs:171-174
Timestamp: 2025-08-22T16:31:58.569Z
Learning: In GingerCoreNET, the canonical FilterProperties list lives in Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs and should not be duplicated in other classes (e.g., SeleniumDriver.cs).
Applied to files:
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs
📚 Learning: 2025-04-25T13:29:45.059Z
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4188
File: Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs:22-23
Timestamp: 2025-04-25T13:29:45.059Z
Learning: The `using Amdocs.Ginger.Repository;` statement in Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs is necessary as it provides access to the `ObservableList<>` class which is used throughout the file for collections of RunSetConfig, AnalyzerItemBase, and ApplicationPOMModel objects.
Applied to files:
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs
🧬 Code graph analysis (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs (5)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (31)
ElementInfo(6599-6664)ElementInfo(6745-6761)ElementInfo(6763-6774)ElementInfo(8150-8153)ElementInfo(8155-8214)ElementInfo(8236-8296)ElementInfo(9477-9498)ElementInfo(9557-9608)ElementInfo(11784-11795)ElementInfo(11797-11801)ElementInfo(11803-11836)ElementInfo(11931-11938)ElementInfo(11940-12001)ElementInfo(12075-12100)ElementInfo(12102-12127)ElementInfo(12312-12333)Task(5434-5499)Task(8403-8440)Task(11412-11500)Task(12458-12482)Task(12692-12722)Task(12724-12742)Task(12749-12780)Task(12782-12805)Task(12887-12891)ObservableList(5692-5782)ObservableList(7344-7368)ObservableList(7687-7846)ObservableList(7961-8053)ObservableList(9974-9977)ObservableList(12396-12399)Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs (13)
Task(2315-2331)Task(2608-2718)Task(2731-2772)Task(2837-2840)Task(2846-2885)Task(3716-3881)Task(3893-3903)ObservableList(2969-3094)ObservableList(3104-3146)ObservableList(3210-3213)ObservableList(3481-3484)ObservableList(4423-4426)eDevicePlatformType(3491-3494)Ginger/GingerCoreNET/GeneralLib/ElementWrapperInfo.cs (1)
ElementWrapperInfo(29-32)Ginger/GingerCoreNET/GeneralLib/General.cs (4)
ObservableList(318-323)ObservableList(845-876)ObservableList(997-1078)General(60-1488)Ginger/GingerCoreCommon/Repository/ApplicationModelLib/POMModelLib/PomSetting.cs (1)
PomSetting(27-68)
🔇 Additional comments (4)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs (4)
351-361: Start condition now guarded atomically. LGTM.
185-189: Forwarding url to AI call. LGTM.
338-349: Delayed runner never sets IsProcessing; race and missing UI status.The delayed path enqueues work while IsProcessing stays false. UI won’t show processing and another worker can start concurrently.
Apply under-lock state flip before Task.Run:
public void TriggerDelayProcessingfinetuneWithAI(PomSetting pomSetting, ePomElementCategory? PomCategory, eDevicePlatformType? DevicePlatformType = null) { if (pomSetting.LearnPOMByAI) { - lock (lockObj) - { - if (processingQueue.Count < 10 && !IsProcessing) - { - _ = Task.Run(() => TriggerDelayedProcessing(pomSetting, PomCategory, DevicePlatformType)); - } - } + lock (lockObj) + { + if (processingQueue.Count < 10 && !IsProcessing) + { + IsProcessing = true; + _ = Task.Run(() => TriggerDelayedProcessing(pomSetting, PomCategory, DevicePlatformType)); + } + } } }
37-38: No staleTigger*usages detected – publicizing POMUtils poses no internal breakages.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs(10 hunks)
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-09-02T07:51:28.822Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4289
File: Ginger/GingerCoreNET/Application Models/Delta/PomDelta/PomDeltaUtils.cs:99-103
Timestamp: 2025-09-02T07:51:28.822Z
Learning: In Ginger/GingerCoreNET/Application Models/Delta/PomDelta/PomDeltaUtils.cs around line 99, prashelke prefers to keep the unused variable `selectedElementList` from GetSelectedElementList() call rather than removing it, despite it being unused in the current logic.
Applied to files:
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs
📚 Learning: 2025-07-16T14:17:56.429Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4254
File: Ginger/GingerCoreNET/GeneralLib/ElementWrapperInfo.cs:89-93
Timestamp: 2025-07-16T14:17:56.429Z
Learning: In Ginger/GingerCoreNET/GeneralLib/ElementWrapperInfo.cs, the user prashelke prefers to keep the current casing for properties `insideIframe` and `insideShadowDOM` in the `ElementwrapperContext` class rather than changing them to PascalCase.
Applied to files:
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs
📚 Learning: 2025-08-22T16:22:20.870Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4280
File: Ginger/GingerCoreNET/GeneralLib/ElementWrapperInfo.cs:41-41
Timestamp: 2025-08-22T16:22:20.870Z
Learning: In Ginger/GingerCoreNET/GeneralLib/ElementWrapperInfo.cs, the user prashelke prefers to keep the current lowercase naming for properties like `elementGuid` rather than changing them to PascalCase, likely for consistency within the ElementWrapperInfo data model classes.
Applied to files:
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs
📚 Learning: 2025-09-04T15:42:00.330Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4294
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs:8334-8350
Timestamp: 2025-09-04T15:42:00.330Z
Learning: In PR Ginger-Automation/Ginger#4294, SeleniumDriver.cs: maintainer requested to keep the WebElement-based helper IsSvgElementByWebElement(IWebElement) and its current usages for now, deferring the replacement with GetElementTypeEnum(..) + IsSvgElement(eElementType) to a later PR.
Applied to files:
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs
📚 Learning: 2025-09-04T15:43:57.789Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4294
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs:9146-9291
Timestamp: 2025-09-04T15:43:57.789Z
Learning: In SeleniumDriver.cs, prefer using the C# fallback GenerateEnhancedSvgXPath(IWebElement) for SVG XPath generation instead of injecting/validating a JavaScript-based XPath builder. This was accepted in PR Ginger-Automation/Ginger#4294.
Applied to files:
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs
📚 Learning: 2024-07-26T22:04:12.930Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#3429
File: Ginger/Ginger/AutomatePageLib/AddActionMenu/WindowExplorer/Common/WindowExplorerPage.xaml.cs:1139-1142
Timestamp: 2024-07-26T22:04:12.930Z
Learning: The user has updated the code by removing or addressing the commented code as suggested in the review.
Applied to files:
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs
📚 Learning: 2025-08-22T16:31:58.569Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4280
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs:171-174
Timestamp: 2025-08-22T16:31:58.569Z
Learning: In GingerCoreNET, the canonical FilterProperties list lives in Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs and should not be duplicated in other classes (e.g., SeleniumDriver.cs).
Applied to files:
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs
📚 Learning: 2025-04-25T13:29:45.059Z
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4188
File: Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs:22-23
Timestamp: 2025-04-25T13:29:45.059Z
Learning: The `using Amdocs.Ginger.Repository;` statement in Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs is necessary as it provides access to the `ObservableList<>` class which is used throughout the file for collections of RunSetConfig, AnalyzerItemBase, and ApplicationPOMModel objects.
Applied to files:
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs
🧬 Code graph analysis (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs (4)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (25)
ElementInfo(6599-6664)ElementInfo(6745-6761)ElementInfo(6763-6774)ElementInfo(8150-8153)ElementInfo(8155-8214)ElementInfo(8236-8296)ElementInfo(9477-9498)ElementInfo(9557-9608)ElementInfo(11784-11795)ElementInfo(11797-11801)ElementInfo(11803-11836)ElementInfo(11931-11938)ElementInfo(11940-12001)ElementInfo(12075-12100)ElementInfo(12102-12127)ElementInfo(12312-12333)Task(5434-5499)Task(8403-8440)Task(11412-11500)Task(12458-12482)Task(12692-12722)Task(12724-12742)Task(12749-12780)Task(12782-12805)Task(12887-12891)Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs (8)
Task(2315-2331)Task(2608-2718)Task(2731-2772)Task(2837-2840)Task(2846-2885)Task(3716-3881)Task(3893-3903)eDevicePlatformType(3491-3494)Ginger/GingerCoreNET/GeneralLib/General.cs (1)
General(60-1488)Ginger/GingerCoreCommon/Repository/ApplicationModelLib/POMModelLib/PomSetting.cs (1)
PomSetting(27-68)
🔇 Additional comments (9)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs (9)
19-34: LGTM: Unused usings removed.The cleanup of unused
usingdirectives (System.ComponentModel and Microsoft.Graph) has been successfully completed. The remaining usings are all referenced in the file.
37-37: LGTM: Public API surface change noted.The class has been changed from internal to public sealed, exposing the AI workflow orchestration APIs. This aligns with the PR objective to integrate mobile POM fine-tuning with AI. The sealed modifier prevents inheritance, which is appropriate for a utility class.
54-68: LGTM: Thread-safe processing state management.The volatile flag and
SetProcessinghelper correctly separate lock acquisition (for state change) from event invocation (outside lock), preventing potential deadlocks from event handler re-entrance. Well implemented.
190-190: LGTM: URL parameter now forwarded to AI call.The
urlparameter is now correctly passed toGetResponseForprocess_extracted_elementsByOpenAI, allowing downstream code to use custom endpoints.
416-417: LGTM: Delay added to TriggerDelayedProcessing.The 750ms delay has been added, making the method name consistent with its behavior. This provides a small debounce window for tail elements.
423-424: LGTM: Using current instance methods.The
UpdateAndMarkElementsAsyncmethod now correctly calls instance methods directly instead of creating a newPOMUtilsinstance, preserving event wiring and state consistency.
357-364: LGTM: Batch processing trigger properly locked.The
TriggerBatchProcessingmethod correctly acquires the lock before checking both the queue count and processing state, then usesSetProcessingto safely update the flag. This ensures atomic decision-making for starting batch processing.
326-339: Good: Deduplication prevents multiple enqueues.The use of
_enqueuedIds.TryAddat line 331 correctly prevents the same element from being enqueued multiple times before it's processed. However, see the separate comment about cleaning up the dictionary to prevent memory leaks.
345-352: Minor race: Count check still outside meaningful atomicity window.While the lock is acquired at line 345, the
processingQueue.Count < 10check at line 347 reads the queue count, but the queue itself is aConcurrentQueuethat can be modified by other threads. The lock only protects theIsProcessingflag decision, not the queue operations.This is a minor issue because the worst case is an extra task gets started when count is near the threshold. However, for stricter correctness, consider checking the condition once inside the lock.
For perfect atomicity, re-check the count after acquiring the lock (though this is a minor refinement):
lock (lockObj) { - if (processingQueue.Count < 10 && !IsProcessing) + int currentCount = processingQueue.Count; + if (currentCount < 10 && !IsProcessing) { SetProcessing(true); _ = Task.Run(() => TriggerDelayedProcessing(pomSetting, PomCategory, DevicePlatformType)); } }Or accept the minor race as the impact is negligible.
Likely an incorrect or invalid review comment.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs (1)
341-354: Critical: Nested lock causes deadlock.Lines 345-352 acquire
lockObj, then line 349 callsSetProcessing(true)which attempts to acquire the samelockObjagain at line 59. C#'slockstatement usesMonitor.Enter, which is not reentrant for the same lock object—this will deadlock immediately.Fix by directly setting the flag instead of calling the helper:
lock (lockObj) { if (processingQueue.Count < 10 && !IsProcessing) { - SetProcessing(true); + _isProcessing = true; _ = Task.Run(() => TriggerDelayedProcessing(pomSetting, PomCategory, DevicePlatformType)); } } + // Notify outside the lock to avoid deadlock in handlers + if (_isProcessing) + { + ProcessingStatusChanged?.Invoke(this, true); + }Alternatively, restructure
SetProcessingto accept a lock-already-held parameter, but the direct approach above is simpler.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs(10 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-22T16:31:58.569Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4280
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs:171-174
Timestamp: 2025-08-22T16:31:58.569Z
Learning: In GingerCoreNET, the canonical FilterProperties list lives in Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs and should not be duplicated in other classes (e.g., SeleniumDriver.cs).
Applied to files:
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs
🧬 Code graph analysis (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs (5)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (31)
ElementInfo(6599-6664)ElementInfo(6745-6761)ElementInfo(6763-6774)ElementInfo(8150-8153)ElementInfo(8155-8214)ElementInfo(8236-8296)ElementInfo(9477-9498)ElementInfo(9557-9608)ElementInfo(11784-11795)ElementInfo(11797-11801)ElementInfo(11803-11836)ElementInfo(11931-11938)ElementInfo(11940-12001)ElementInfo(12075-12100)ElementInfo(12102-12127)ElementInfo(12312-12333)Task(5434-5499)Task(8403-8440)Task(11412-11500)Task(12458-12482)Task(12692-12722)Task(12724-12742)Task(12749-12780)Task(12782-12805)Task(12887-12891)ObservableList(5692-5782)ObservableList(7344-7368)ObservableList(7687-7846)ObservableList(7961-8053)ObservableList(9974-9977)ObservableList(12396-12399)Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs (13)
Task(2315-2331)Task(2608-2718)Task(2731-2772)Task(2837-2840)Task(2846-2885)Task(3716-3881)Task(3893-3903)ObservableList(2969-3094)ObservableList(3104-3146)ObservableList(3210-3213)ObservableList(3481-3484)ObservableList(4423-4426)eDevicePlatformType(3491-3494)Ginger/GingerCoreNET/GeneralLib/General.cs (4)
ObservableList(318-323)ObservableList(845-876)ObservableList(997-1078)General(60-1488)Ginger/GingerCoreCommon/UIElement/ElementInfo.cs (2)
ObservableList(503-506)ObservableList(508-511)Ginger/GingerCoreCommon/Repository/ApplicationModelLib/POMModelLib/PomSetting.cs (1)
PomSetting(27-68)
🔇 Additional comments (11)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/POM/POMUtils.cs (11)
46-50: LGTM!Field declarations correctly implement thread-safe collections and synchronization primitives for the batching logic. The
_enqueuedIdsdeduplication dictionary properly prevents duplicate enqueueing.
52-68: LGTM!The processing state management correctly implements the lock-then-notify pattern. Setting
_isProcessingvolatile (line 54) ensures visibility, and theSetProcessinghelper (lines 56-68) properly acquires the lock only for the state change, then raisesProcessingStatusChangedoutside the lock to prevent reentrancy deadlocks.
127-186: LGTM!The
SendInBatchesListmethod correctly accepts and forwards theDevicePlatformTypeparameter (line 127) to theGetResponseFromGenAIcalls (lines 146, 170). This ensures mobile platform context is preserved through the batching logic.
188-192: LGTM!The method now correctly forwards both
DevicePlatformTypeandurlparameters to the AI endpoint (line 190), ensuring mobile platform context and custom endpoint URLs are properly propagated.
193-304: LGTM!Response processing includes proper defensive checks: null/whitespace validation for
enhanceLocatorsJson(lines 261-263) and locator values (line 279), plus fallback parsing for malformed AI responses (lines 227-236). The nested try-catch handles both primary and fallback deserialization gracefully.
326-339: LGTM!The deduplication logic (line 331) correctly prevents duplicate enqueueing using
TryAddon the_enqueuedIdsdictionary. The corresponding cleanup inProcessBatchAsync(line 380) andFlushRemainingAsync(line 404) ensures the dictionary doesn't leak memory.
367-393: LGTM!The batch processing loop correctly dequeues items, processes them in batches of 10, and cleans up the
_enqueuedIdsdictionary (line 380). Thefinallyblock (lines 389-392) properly callsSetProcessing(false)without holding any locks, avoiding deadlock.
395-411: LGTM!The flush logic correctly drains the queue and cleans up
_enqueuedIds(line 404) for each dequeued item, ensuring no memory leaks and allowing re-enqueueing of elements in future operations.
413-417: LGTM!The method name now matches behavior—750ms delay (line 415) allows tail elements to accumulate before processing. This prevents premature batch submission for near-boundary scenarios.
420-424: LGTM!The method correctly invokes instance methods (lines 422-423) rather than creating a new
POMUtilsinstance, preserving theProcessingStatusChangedevent wiring and shared state.
326-354: Remove typo comment – no 'Tigger' typo present
The methods are correctly spelledTriggerFineTuneWithAIandTriggerDelayProcessingfinetuneWithAI. There’s no “Tigger” typo in the public API.Likely an incorrect or invalid review comment.
Maheshkale447
approved these changes
Sep 30, 2025
15 tasks
This was referenced Nov 21, 2025
15 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thank you for your contribution.
Before submitting this PR, please make sure:
Summary by CodeRabbit
New Features
Style
Refactor
API