Reviewing files that changed from the base of the PR and between 645945c and 704f8ff.

• Ginger/Ginger/Drivers/DriversConfigsEditPages/WebAgentConfigEditPage.xaml (1 hunks)
• Ginger/Ginger/Drivers/DriversConfigsEditPages/WebAgentConfigEditPage.xaml.cs (7 hunks)
• Ginger/Ginger/ValueExpression/UCValueExpression.xaml.cs (1 hunks)
• Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowser.cs (3 hunks)
• Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs (1 hunks)
• Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs (2 hunks)
• Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightNonPersistentBrowser.cs (4 hunks)
• Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightPersistentBrowser.cs (3 hunks)

Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightNonPersistentBrowser.cs (2)

77-88: Good implementation of dependency handling for video recording.

The code correctly adds functionality to install the ffmpeg dependency when video recording is enabled. The error handling is robust - if the installation fails, it appropriately logs the error and falls back by disabling video recording rather than allowing the entire process to fail.

The code appropriately leverages the existing ExecutePlaywrightInstallation method from PlaywrightBrowser.cs.

---

144-158:

Details

❓ Verification inconclusive

Video recording configuration properly implemented.

This implementation correctly initializes and applies the video recording settings to the browser context options. The conditional check ensures video recording is only configured when enabled.

One suggestion: consider handling the case where RecordVideoDir is empty but video recording is enabled, as this might cause runtime issues.

Run the following script to check if there are any validation checks for empty video directory elsewhere in the codebase:

---

🏁 Script executed:

#!/bin/bash
# Check if there's validation for empty video directory in related files
rg -A 3 -B 3 "RecordVideoDir" --glob "*.cs"

Length of output: 13609

---

Video Recording Directory Validation in Non-Persistent Browser

• The video recording settings are correctly wired in the browser context options.
• UI flow in the codebase (e.g., in WebAgentConfigEditPage.xaml.cs) already ensures that an empty video directory value is replaced with the default (~\Documents\VideoRecordings).
• However, since PlaywrightNonPersistentBrowser.cs simply assigns _options.RecordVideoDir without an extra check, please verify that all instantiations of _options (including any programmatic ones outside the UI) always provide a valid non-empty directory. If there's any chance of missing validation, consider adding a safeguard here.

Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightPersistentBrowser.cs (1)

85-96: Proper implementation of dependency installation for video recording.

The code correctly checks if video recording is enabled, attempts to install the required ffmpeg dependency, and gracefully handles failures by disabling video recording and logging an error. This is a robust implementation that prevents failures in the dependency installation from breaking the entire functionality.

Ginger/Ginger/ValueExpression/UCValueExpression.xaml.cs (2)

121-127: Enhanced folder selection feature with path expansion.

The implementation successfully adds the ability to use relative paths starting with ~\ when browsing for folders, which is expanded to the solution's directory. This provides better user experience by allowing users to specify relative paths that will be correctly resolved.

Good defensive programming with the null check before performing string operations on ValueTextBox.Text.

---

132-134: Improved folder browser configuration.

The implementation now sets the initial directory based on the user input or the expanded path, which enhances usability by opening the dialog in the expected location. Also, enabling the "New Folder" button is a good addition for scenarios where users need to create directories on the fly.

Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs (2)

114-119: Good implementation of path resolution for video recording directory.

The code correctly handles relative paths for video recording by replacing the tilde prefix with the actual solution folder path. This ensures that video files are stored in a consistent location relative to the project, making it more portable across different environments.

---

126-128: LGTM: Video recording configuration properly set in browser options.

These lines correctly configure the video recording settings in the PlaywrightBrowser options object, ensuring that the user's preferences are properly applied when starting the browser.

Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowser.cs (3)

27-27: LGTM: Added helpful error message constant.

Adding this constant helps maintain consistency in error messages throughout the codebase.

---

39-41: LGTM: Well-defined properties for video recording configuration.

These properties properly encapsulate the video recording settings needed by the PlaywrightBrowser.

---

77-86: Good refactoring of installation logic.

Extracting the installation logic into a separate static method improves code reusability and readability. The improved error message now includes the component name and exit code, which will help with troubleshooting.

Ginger/Ginger/Drivers/DriversConfigsEditPages/WebAgentConfigEditPage.xaml.cs (3)

68-103: Good implementation of path normalization for video directory.

The code effectively handles path normalization, ensuring that paths are represented consistently using the "~" prefix for paths relative to the solution directory. This improves portability and readability.

---

398-402: LGTM: Proper event handling setup for the video directory textbox.

Event handlers are correctly set up to manage the behavior of the video directory text box, and the initial enabled state of controls is set based on the checkbox state.

---

532-535: LGTM: Good UI responsiveness for the video recording checkbox.

The event handler properly enables/disables the video recording controls based on the checkbox state, providing immediate feedback to the user.

Reviewing files that changed from the base of the PR and between 704f8ff and 9bbf0a2.

• Ginger/Ginger/Drivers/DriversConfigsEditPages/WebAgentConfigEditPage.xaml.cs (7 hunks)
• Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs (2 hunks)

Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs (1)

649-668: New video recording properties look consistent.

These property definitions correctly associate user-configurable attributes, default values, and descriptive text for video recording. The descriptive text for width and height is correct (no mismatch).

Ginger/Ginger/Drivers/DriversConfigsEditPages/WebAgentConfigEditPage.xaml.cs (11)

22-22: Import statement is appropriate.

This addition aligns with the usage of PlaywrightDriver references in this file.

---

50-50: Initialization call is fine.

Calling InitializeComponent() in the constructor is standard practice for WPF pages.

---

68-71: LostFocus event handler looks good.

This method simply delegates to ReplaceSolutionDirPathInVideoDirPath, which helps maintain relative paths.

---

72-93: Path normalization logic is sensible.

Using "~\\" as a placeholder for the solution folder and restoring the default path when empty is a good approach. Ensure edge cases around partial matches of solutionFolder remain acceptable.

---

95-103: TextChanged event smoothly updates the path.

Replacing the solution path on larger edits helps maintain consistent relative paths. The condition (textChange.AddedLength > 1 || textChange.RemovedLength > 1) is a reasonable heuristic.

---

321-338: Binding video-recording parameters is correct.

These additions allow the UI to manage new Playwright-specific properties (EnableVideoRecording, RecordVideoDir, VideoHeight, and VideoWidth).

---

387-387: Advanced settings visibility adjustment is aligned with Playwright usage.

No concerns here.

---

389-389: Hiding RemoteWebDriverUrl for Playwright.

Makes sense since remote WebDriver setups are more Selenium-centric.

---

398-403: Displaying video recording panel & attaching events.

Enabling the panel only when video recording is checked is a logical UI flow.

---

404-407: Hiding video panel for non-Playwright drivers is consistent.

Collapsing the panel ensures a clean UI for drivers lacking video-recording features.

---

531-535: Checkbox click toggles video-panel controls.

This short method cleanly updates the IsEnabled state based on the checkbox.

Reviewing files that changed from the base of the PR and between 9bbf0a2 and 29c68b4.

• Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs (2 hunks)

Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs (2)

126-128: Ensure video recording options are properly configured.

The code correctly adds video recording settings to the browser options, including the recording directory, whether recording is enabled, and the video dimensions.

---

653-657: LGTM! Enable video recording property correctly implemented.

The property is properly configured with a default value of "false" which is a safe default requiring explicit opt-in for video recording.

Reviewing files that changed from the base of the PR and between 29c68b4 and 740d123.

• Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs (2 hunks)

Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs (5)

114-119: Ensure updated path reflects in final options.

Within this block, the local variable recordVideoDir is changed if it starts with ~\, but that updated value is never reassigned back to RecordVideoDir. As a result, the line in the returned PlaywrightBrowser.Options that sets RecordVideoDir = recordVideoDir is effectively ignoring the transformed path.

A possible fix:

string recordVideoDir = RecordVideoDir;
if (recordVideoDir != null && recordVideoDir.StartsWith(@"~\"))
{
    string solutionFolder = amdocs.ginger.GingerCoreNET.WorkSpace.Instance.Solution.Folder;
    recordVideoDir = recordVideoDir.Replace(@"~\", solutionFolder, StringComparison.InvariantCultureIgnoreCase);
}

+ // Reassign the property to capture the updated path
+ RecordVideoDir = recordVideoDir;

---

132-134: LGTM - Video recording configuration is correctly set in options.

The implementation properly sets the video recording options in the browser options object.

---

655-658: Consider adding path validation for video directory.

The RecordVideoDir is properly configured with a default path, but there's no validation to ensure the path is accessible and writable. Consider adding runtime validation when video recording is enabled.

Example validation:

if (EnableVideoRecording && !string.IsNullOrEmpty(RecordVideoDir))
{
    string dirPath = RecordVideoDir;
    if (dirPath.StartsWith(@"~\"))
    {
        string solutionFolder = amdocs.ginger.GingerCoreNET.WorkSpace.Instance.Solution.Folder;
        dirPath = dirPath.Replace(@"~\", solutionFolder, StringComparison.InvariantCultureIgnoreCase);
    }
    
    try
    {
        Directory.CreateDirectory(dirPath);
    }
    catch (Exception ex)
    {
        Reporter.ToLog(eLogLevel.ERROR, $"Failed to create video recording directory: {dirPath}", ex);
        EnableVideoRecording = false;
    }
}

---

667-668: Fix inconsistency in the VideoHeight property description.

The description for VideoHeight incorrectly states "The height in pixels" instead of "The width in pixels".

-[UserConfiguredDescription("The height in pixels of video recording")]
+[UserConfiguredDescription("The height in pixels of video recording")]
public int VideoHeight { get; set; }

---

665-668: Add a default value for VideoHeight.

The VideoHeight property has an empty string default value, which is unusual for an integer property. Consider setting a reasonable default dimension like 720p.

[UserConfigured]
-[UserConfiguredDefault("")]
+[UserConfiguredDefault("720")]
[UserConfiguredDescription("The height in pixels of video recording")]
public int VideoHeight { get; set; }