feat/CUS-7732-Added support for edge browser and changed it into factory design pattern #237
Conversation
WalkthroughThis PR introduces a browser-specific Excel utilities abstraction layer with Chrome and Edge implementations, replaces PdfAndDocUtilities across multiple action classes with the new ExcelUtilities interface via factory pattern, updates project dependencies, and consolidates Excel counting logic into StoreNonEmptyCellCount. Changes
Sequence DiagramsequenceDiagram
participant Action as Action Class
participant Factory as ExcelUtilitiesFactory
participant Utils as ExcelUtilities<br/>(Chrome/Edge)
participant Browser as Browser<br/>(chrome:// or edge://)
participant LocalFS as Local<br/>Filesystem
Action->>Factory: create(driver, logger)
Factory->>Factory: detect browser type
alt Chrome
Factory->>Utils: new ChromeExcelUtilities()
else Edge
Factory->>Utils: new EdgeExcelUtilities()
end
Factory-->>Action: ExcelUtilities instance
Action->>Utils: copyFileFromDownloads(format, name)
Utils->>Utils: isFileDownloaded()
Utils->>Browser: navigate to downloads page
Utils->>Browser: inspect downloads UI (JS)
Browser-->>Utils: file path + status
Utils->>Utils: getDownloadedFileLocalPath()
Browser-->>Utils: encoded file path
Utils->>Browser: read file as Base64 data URL
Browser-->>Utils: data content
Utils->>LocalFS: write temp file with format
LocalFS-->>Utils: local File object
Utils-->>Action: File (local copy)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The changes involve consistent refactoring across 11 action classes following a single pattern (replacing PdfAndDocUtilities with ExcelUtilities via factory), but introduce two new browser-specific implementations with moderate complexity (DOM inspection, JavaScript execution, Base64 encoding/decoding). StoreNonEmptyCellCount adds focused Excel counting logic with new helper methods. Dependency updates are straightforward. The homogeneous refactoring pattern reduces review friction, but the new Chrome and Edge implementations require careful verification of browser-specific logic. Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
excelActions_cloud/src/main/java/com/testsigma/addons/web/ExtractCompleteColumnValuesWithSheetIndex.java (1)
150-150: Fix typo in log message."Extractrd" should be "Extracted".
Apply this diff:
- logger.info("Extractrd column values: " + entireFieldValues); + logger.info("Extracted column values: " + entireFieldValues);
🧹 Nitpick comments (4)
excelActions_cloud/src/main/java/com/testsigma/addons/util/ExcelUtilities.java (2)
5-10: Add documentation for the public interface.This is a new public API that would benefit from Javadoc comments explaining:
- The purpose of each method
- Parameter descriptions
- Return value details
- Expected exceptions and when they occur
Example:
+/** + * Utilities for handling Excel file downloads in browser automation. + * Implementations provide browser-specific handling for Chrome and Edge. + */ public interface ExcelUtilities { + /** + * Copies a downloaded file from the browser's download location. + * @param fileFormat the expected file format extension (e.g., "xlsx") + * @param fileName optional specific file name to search for, or null for latest + * @return File object pointing to the local copy + * @throws Exception if file is still downloading, not found, or copy fails + */ File copyFileFromDownloads(String fileFormat, String fileName) throws Exception; + /** + * Checks if any files are currently downloading. + * @return true if no downloads in progress, false otherwise + */ boolean isFileDownloaded(); + /** + * Gets the local file path of the most recent download. + * @return the absolute file path as a string + */ String getDownloadedFileLocalPath(); + /** + * Searches for a file by name in downloads. + * @param desiredFileName the file name or partial name to search for + * @return the absolute file path if found + */ String getFilePathByFileNameInDownloads(String desiredFileName); }
6-6: Consider more specific exception types for better error handling.Using
throws Exceptionis too broad and makes it difficult for callers to handle specific failure scenarios appropriately. Consider defining custom exceptions or using more specific types likeIOException,FileNotFoundException, orIllegalStateException.Example custom exceptions:
public class DownloadInProgressException extends Exception { public DownloadInProgressException(String message) { super(message); } } public class FileNotFoundInDownloadsException extends Exception { public FileNotFoundInDownloadsException(String message) { super(message); } }Then update the interface:
- File copyFileFromDownloads(String fileFormat, String fileName) throws Exception; + File copyFileFromDownloads(String fileFormat, String fileName) + throws DownloadInProgressException, FileNotFoundInDownloadsException, IOException;excelActions_cloud/src/main/java/com/testsigma/addons/util/ChromeExcelUtilities.java (1)
24-36: Improve error context when throwing RuntimeException.Lines 28 and 34 throw
RuntimeExceptionwith a message but lose the calling context. The caller won't know which file operation failed or have access to the stack trace for debugging.Consider creating more specific exception types or at least including more context:
public File copyFileFromDownloads(String fileFormat, String fileName) throws Exception { driver.navigate().to("chrome://downloads/"); if (!isFileDownloaded()) { - throw new RuntimeException("File is still downloading."); + throw new RuntimeException("File is still downloading. Filename: " + fileName); } String remoteFilePath = fileName != null ? getFilePathByFileNameInDownloads(fileName) : getDownloadedFileLocalPath(); if (remoteFilePath != null) { return createLocalFileFromDownloadsCopy(remoteFilePath, fileFormat); } else { - throw new RuntimeException("File path not found."); + throw new RuntimeException("File path not found for filename: " + fileName + ", format: " + fileFormat); } }excelActions_cloud/src/main/java/com/testsigma/addons/util/ExcelUtilitiesFactory.java (1)
9-20: Add null safety checks for parameters.The factory method doesn't validate that
driverandloggerare non-null before using them.Apply this diff to add null checks:
public static ExcelUtilities create(WebDriver driver, Logger logger) { + if (driver == null) { + throw new IllegalArgumentException("WebDriver cannot be null"); + } + if (logger == null) { + throw new IllegalArgumentException("Logger cannot be null"); + } String browserName = ((RemoteWebDriver) driver).getCapabilities().getBrowserName().toLowerCase();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
excelActions_cloud/pom.xml(3 hunks)excelActions_cloud/src/main/java/com/testsigma/addons/util/ChromeExcelUtilities.java(1 hunks)excelActions_cloud/src/main/java/com/testsigma/addons/util/EdgeExcelUtilities.java(1 hunks)excelActions_cloud/src/main/java/com/testsigma/addons/util/ExcelUtilities.java(1 hunks)excelActions_cloud/src/main/java/com/testsigma/addons/util/ExcelUtilitiesFactory.java(1 hunks)excelActions_cloud/src/main/java/com/testsigma/addons/web/ExtractCompleteColumnValueForGivenFile.java(2 hunks)excelActions_cloud/src/main/java/com/testsigma/addons/web/ExtractCompleteColumnValues.java(2 hunks)excelActions_cloud/src/main/java/com/testsigma/addons/web/ExtractCompleteColumnValuesWithSheetIndex.java(2 hunks)excelActions_cloud/src/main/java/com/testsigma/addons/web/ExtractCompleteFieldValuesForGivenFile.java(3 hunks)excelActions_cloud/src/main/java/com/testsigma/addons/web/ExtractCompleteRowValues.java(2 hunks)excelActions_cloud/src/main/java/com/testsigma/addons/web/ExtractCompleteRowValuesWithSheetIndex.java(2 hunks)excelActions_cloud/src/main/java/com/testsigma/addons/web/ExtractUniqueColumnValuesWithSheetIndex.java(2 hunks)excelActions_cloud/src/main/java/com/testsigma/addons/web/ExtractUniqueRowValuesWithSheetIndex.java(2 hunks)excelActions_cloud/src/main/java/com/testsigma/addons/web/StoreNonEmptyCellCount.java(4 hunks)excelActions_cloud/src/main/java/com/testsigma/addons/web/VerifyExcel.java(2 hunks)excelActions_cloud/src/main/java/com/testsigma/addons/web/WriteCellvalueWithSheet.java(2 hunks)excelActions_cloud/src/main/java/com/testsigma/addons/web/cellValue.java(3 hunks)
🔇 Additional comments (23)
excelActions_cloud/pom.xml (2)
9-9: LGTM! Dependency updates align with the new Excel utilities framework.The version bumps are appropriate for supporting the new browser-specific Excel handling capabilities. The Selenium and Appium updates provide the necessary WebDriver features, and the commons-lang3 update ensures access to the latest utility functions.
Also applies to: 16-16, 51-51, 57-57, 72-72
51-51: Verify that no deprecated Selenium timeout methods are used in the codebase.Selenium 4.33.0 removes deprecated Java timeout methods (
setScriptTimeout,pageLoadTimeout). Ensure the codebase has migrated to the newer timeout configuration APIs before this upgrade.Based on learnings.
Run the following script to search for deprecated timeout method usage:
excelActions_cloud/src/main/java/com/testsigma/addons/util/ChromeExcelUtilities.java (1)
44-45: Complex JavaScript query may be fragile across Chrome versions.The JavaScript code accesses Chrome's internal downloads UI structure through shadow DOM. This internal structure is not a stable API and could break in future Chrome versions. Consider documenting the supported Chrome versions and monitoring for breakages.
Additionally, consider adding defensive checks:
JavascriptExecutor js = (JavascriptExecutor) driver; +// Check if downloads-manager exists first +Object managerExists = js.executeScript( + "return document.querySelector('downloads-manager') != null;"); +if (managerExists == null || !Boolean.TRUE.equals(managerExists)) { + logger.warn("Chrome downloads UI not found. This may indicate an incompatible Chrome version."); + return true; // Assume complete if UI not available +} + Object obj = js.executeScript("return document.querySelector('downloads-manager').shadowRoot.querySelector('#downloadsList')" + ".items.filter(e => e.state === 'IN_PROGRESS').map(e => e.filePath || e.file_path || e.fileUrl || e.file_url); ");excelActions_cloud/src/main/java/com/testsigma/addons/web/ExtractCompleteRowValuesWithSheetIndex.java (1)
3-4: LGTM! Clean refactoring to use the factory pattern.The replacement of
PdfAndDocUtilitieswithExcelUtilitiesFactory.create(driver, logger)correctly implements the factory pattern for browser-specific Excel handling. The rest of the file's logic remains unchanged and correct.Also applies to: 44-47
excelActions_cloud/src/main/java/com/testsigma/addons/web/cellValue.java (1)
3-4: LGTM! Consistent application of the factory pattern.The refactoring correctly replaces the old utility with the new
ExcelUtilitiesobtained viaExcelUtilitiesFactory.create(driver, logger). The implementation follows the same pattern as other files in this PR.Also applies to: 47-51
excelActions_cloud/src/main/java/com/testsigma/addons/web/ExtractCompleteColumnValueForGivenFile.java (1)
3-4: LGTM! Proper use of the factory with filename parameter.The refactoring correctly uses
ExcelUtilitiesFactory.create(driver, logger)and passes the processed filename tocopyFileFromDownloads. The filename processing logic (removing .xlsx extension) is appropriate for the file search functionality.Also applies to: 45-52
excelActions_cloud/src/main/java/com/testsigma/addons/web/ExtractCompleteColumnValues.java (1)
3-4: LGTM! Consistent refactoring pattern applied.The change correctly implements the factory pattern by replacing
PdfAndDocUtilitieswithExcelUtilitiesFactory.create(driver, logger). The implementation is consistent with the other files in this PR.Also applies to: 42-45
excelActions_cloud/src/main/java/com/testsigma/addons/web/VerifyExcel.java (2)
3-4: LGTM! Final usage file correctly refactored.The refactoring is complete and correct, replacing
PdfAndDocUtilitieswith the factory-createdExcelUtilities. All usage files in this PR now consistently use the factory pattern for browser-specific Excel handling.Also applies to: 42-46
42-42: Verify that EdgeExcelUtilities implementation exists and is tested.The PR description mentions adding Edge browser support, and the factory is expected to return
EdgeExcelUtilitiesfor Edge browsers. However, theEdgeExcelUtilitiesimplementation file was not included in this review. Please confirm that:
- The
EdgeExcelUtilitiesclass exists and implements theExcelUtilitiesinterface- The
ExcelUtilitiesFactorycorrectly detects and returnsEdgeExcelUtilitiesfor Edge browsers- Both Chrome and Edge implementations have been tested
Run the following script to locate the EdgeExcelUtilities implementation and factory logic:
excelActions_cloud/src/main/java/com/testsigma/addons/web/ExtractCompleteColumnValuesWithSheetIndex.java (2)
4-5: LGTM!The import changes correctly introduce the ExcelUtilities abstraction via the factory pattern.
45-48: LGTM!The factory pattern is correctly applied to create browser-specific ExcelUtilities instances.
excelActions_cloud/src/main/java/com/testsigma/addons/web/WriteCellvalueWithSheet.java (2)
3-4: LGTM!The import changes align with the ExcelUtilities abstraction introduced in this PR.
50-54: LGTM!The factory pattern is correctly applied to obtain browser-specific Excel utilities.
excelActions_cloud/src/main/java/com/testsigma/addons/web/ExtractCompleteFieldValuesForGivenFile.java (2)
3-4: LGTM!The import changes correctly introduce the ExcelUtilities abstraction.
44-54: LGTM!The factory pattern is correctly applied, and the file name handling is preserved.
excelActions_cloud/src/main/java/com/testsigma/addons/web/ExtractUniqueColumnValuesWithSheetIndex.java (2)
3-4: LGTM!The import changes align with the factory pattern refactor.
48-51: LGTM!The factory pattern is correctly implemented for browser-specific Excel utilities.
excelActions_cloud/src/main/java/com/testsigma/addons/web/ExtractUniqueRowValuesWithSheetIndex.java (2)
3-4: LGTM!The import changes are consistent with the ExcelUtilities abstraction.
48-51: LGTM!The factory pattern is correctly applied.
excelActions_cloud/src/main/java/com/testsigma/addons/web/StoreNonEmptyCellCount.java (2)
11-12: LGTM!The new imports support the extracted counting helper methods.
70-127: Well-structured refactor with clear separation of concerns.The extraction of counting logic into dedicated helper methods improves maintainability and testability.
excelActions_cloud/src/main/java/com/testsigma/addons/web/ExtractCompleteRowValues.java (2)
3-4: LGTM!The import changes align with the factory pattern refactor.
39-42: LGTM!The factory pattern is correctly applied to create browser-specific Excel utilities.
| public ChromeExcelUtilities(WebDriver driver, Logger logger) { | ||
| this.driver = driver; | ||
| this.logger = logger; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Validate that the driver is actually a Chrome browser.
The constructor accepts any WebDriver but the implementation only works with Chrome (uses chrome://downloads). Add browser validation to fail fast with a clear error message.
Apply this diff:
public ChromeExcelUtilities(WebDriver driver, Logger logger) {
+ if (driver == null) {
+ throw new IllegalArgumentException("WebDriver cannot be null");
+ }
+ if (logger == null) {
+ throw new IllegalArgumentException("Logger cannot be null");
+ }
+
+ // Validate that this is a Chrome browser
+ String browserName = ((RemoteWebDriver) driver).getCapabilities().getBrowserName();
+ if (!"chrome".equalsIgnoreCase(browserName)) {
+ throw new IllegalArgumentException(
+ "ChromeExcelUtilities requires Chrome browser, but got: " + browserName);
+ }
+
this.driver = driver;
this.logger = logger;
}Don't forget to add the import:
+import org.openqa.selenium.remote.RemoteWebDriver;📝 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.
| public ChromeExcelUtilities(WebDriver driver, Logger logger) { | |
| this.driver = driver; | |
| this.logger = logger; | |
| } | |
| public ChromeExcelUtilities(WebDriver driver, Logger logger) { | |
| if (driver == null) { | |
| throw new IllegalArgumentException("WebDriver cannot be null"); | |
| } | |
| if (logger == null) { | |
| throw new IllegalArgumentException("Logger cannot be null"); | |
| } | |
| // Validate that this is a Chrome browser | |
| String browserName = ((RemoteWebDriver) driver).getCapabilities().getBrowserName(); | |
| if (!"chrome".equalsIgnoreCase(browserName)) { | |
| throw new IllegalArgumentException( | |
| "ChromeExcelUtilities requires Chrome browser, but got: " + browserName); | |
| } | |
| this.driver = driver; | |
| this.logger = logger; | |
| } |
🤖 Prompt for AI Agents
In
excelActions_cloud/src/main/java/com/testsigma/addons/util/ChromeExcelUtilities.java
around lines 19 to 22, the constructor currently accepts any WebDriver but the
class relies on Chrome-specific functionality; update the constructor to
validate the incoming driver is a Chrome browser and throw an
IllegalArgumentException with a clear message if not. Implement this by checking
either (driver instanceof org.openqa.selenium.chrome.ChromeDriver) or by casting
to org.openqa.selenium.remote.RemoteWebDriver and verifying
getCapabilities().getBrowserName().equalsIgnoreCase("chrome"), and import
org.openqa.selenium.chrome.ChromeDriver (and
org.openqa.selenium.remote.RemoteWebDriver if using capabilities). Ensure the
validation runs at construction time so the class fails fast when used with
non-Chrome drivers.
| public String getFilePathByFileNameInDownloads(String desiredFileName) { | ||
| try { | ||
| String script = "return Array.from(document.querySelector('downloads-manager').shadowRoot.querySelector('#downloadsList').items)" + | ||
| " .find(item => item.fileName && item.fileName.includes('" + desiredFileName + "')).filePath;"; | ||
| JavascriptExecutor js = (JavascriptExecutor) driver; | ||
| Object obj = js.executeScript(script); | ||
| return obj.toString(); | ||
| } catch(Exception e){ | ||
| logger.info("There is no file with name:"+desiredFileName); | ||
| throw new RuntimeException("There is no file with name:"+desiredFileName); | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical security issue: JavaScript injection vulnerability via string concatenation.
Line 68 concatenates desiredFileName directly into the JavaScript code without sanitization. A malicious filename could inject arbitrary JavaScript, potentially accessing sensitive data or manipulating the browser.
Apply this diff to properly escape the filename:
public String getFilePathByFileNameInDownloads(String desiredFileName) {
try {
- String script = "return Array.from(document.querySelector('downloads-manager').shadowRoot.querySelector('#downloadsList').items)" +
- " .find(item => item.fileName && item.fileName.includes('" + desiredFileName + "')).filePath;";
+ // Escape single quotes in filename to prevent injection
+ String escapedFileName = desiredFileName.replace("'", "\\'");
+ String script = "return Array.from(document.querySelector('downloads-manager').shadowRoot.querySelector('#downloadsList').items)" +
+ " .find(item => item.fileName && item.fileName.includes('" + escapedFileName + "')).filePath;";
JavascriptExecutor js = (JavascriptExecutor) driver;
Object obj = js.executeScript(script);
return obj.toString();Even better, pass the filename as a script argument to avoid string concatenation entirely:
public String getFilePathByFileNameInDownloads(String desiredFileName) {
try {
- String script = "return Array.from(document.querySelector('downloads-manager').shadowRoot.querySelector('#downloadsList').items)" +
- " .find(item => item.fileName && item.fileName.includes('" + desiredFileName + "')).filePath;";
+ String script = "var desiredName = arguments[0]; " +
+ "return Array.from(document.querySelector('downloads-manager').shadowRoot.querySelector('#downloadsList').items)" +
+ " .find(item => item.fileName && item.fileName.includes(desiredName)).filePath;";
JavascriptExecutor js = (JavascriptExecutor) driver;
- Object obj = js.executeScript(script);
+ Object obj = js.executeScript(script, desiredFileName);
return obj.toString();📝 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.
| public String getFilePathByFileNameInDownloads(String desiredFileName) { | |
| try { | |
| String script = "return Array.from(document.querySelector('downloads-manager').shadowRoot.querySelector('#downloadsList').items)" + | |
| " .find(item => item.fileName && item.fileName.includes('" + desiredFileName + "')).filePath;"; | |
| JavascriptExecutor js = (JavascriptExecutor) driver; | |
| Object obj = js.executeScript(script); | |
| return obj.toString(); | |
| } catch(Exception e){ | |
| logger.info("There is no file with name:"+desiredFileName); | |
| throw new RuntimeException("There is no file with name:"+desiredFileName); | |
| } | |
| } | |
| public String getFilePathByFileNameInDownloads(String desiredFileName) { | |
| try { | |
| - String script = "return Array.from(document.querySelector('downloads-manager').shadowRoot.querySelector('#downloadsList').items)" + | |
| String script = "var desiredName = arguments[0]; " + | |
| "return Array.from(document.querySelector('downloads-manager').shadowRoot.querySelector('#downloadsList').items)" + | |
| " .find(item => item.fileName && item.fileName.includes(desiredName)).filePath;"; | |
| JavascriptExecutor js = (JavascriptExecutor) driver; | |
| Object obj = js.executeScript(script, desiredFileName); | |
| return obj.toString(); | |
| } catch(Exception e){ | |
| logger.info("There is no file with name:"+desiredFileName); | |
| throw new RuntimeException("There is no file with name:"+desiredFileName); | |
| } | |
| } |
🤖 Prompt for AI Agents
In
excelActions_cloud/src/main/java/com/testsigma/addons/util/ChromeExcelUtilities.java
around lines 65 to 76, the current method builds JavaScript by concatenating
desiredFileName which allows JavaScript injection; instead, pass the filename as
an argument to JavascriptExecutor.executeScript and reference it via
arguments[0] in the script (no string concatenation), check the returned Object
for null before calling toString (and throw a clear RuntimeException if not
found), and narrow the catch to Exception types you expect while logging the
filename in the error message for context.
| File f = new File(path); | ||
| String fileName = f.getName(); | ||
| byte[] decodedBytes = Base64.getDecoder().decode(base64String); | ||
| File downloadedFile = File.createTempFile(fileName, "." + fileFormat); | ||
| Files.write(Paths.get(downloadedFile.getAbsolutePath()), decodedBytes); |
There was a problem hiding this comment.
Potential file naming issue with double extensions.
Line 109 creates a temp file with fileName as the prefix and then adds "." + fileFormat as the suffix. If fileName already contains an extension (e.g., "report.xlsx"), the resulting file could be "report.xlsx.xlsx".
Apply this diff to strip any existing extension:
String base64String = result.toString().substring(result.toString().indexOf("base64,") + 7);
File f = new File(path);
- String fileName = f.getName();
+ String fileName = f.getName();
+ // Remove extension if present to avoid duplication
+ int lastDot = fileName.lastIndexOf('.');
+ if (lastDot > 0) {
+ fileName = fileName.substring(0, lastDot);
+ }
byte[] decodedBytes = Base64.getDecoder().decode(base64String);
File downloadedFile = File.createTempFile(fileName, "." + fileFormat);📝 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.
| File f = new File(path); | |
| String fileName = f.getName(); | |
| byte[] decodedBytes = Base64.getDecoder().decode(base64String); | |
| File downloadedFile = File.createTempFile(fileName, "." + fileFormat); | |
| Files.write(Paths.get(downloadedFile.getAbsolutePath()), decodedBytes); | |
| File f = new File(path); | |
| String fileName = f.getName(); | |
| // Remove extension if present to avoid duplication | |
| int lastDot = fileName.lastIndexOf('.'); | |
| if (lastDot > 0) { | |
| fileName = fileName.substring(0, lastDot); | |
| } | |
| byte[] decodedBytes = Base64.getDecoder().decode(base64String); | |
| File downloadedFile = File.createTempFile(fileName, "." + fileFormat); | |
| Files.write(Paths.get(downloadedFile.getAbsolutePath()), decodedBytes); |
🤖 Prompt for AI Agents
In
excelActions_cloud/src/main/java/com/testsigma/addons/util/ChromeExcelUtilities.java
around lines 106 to 110, the temp file is created using the original file name
as the prefix which can cause double extensions (e.g., "report.xlsx.xlsx");
strip any existing extension from fileName before passing it as the prefix to
File.createTempFile (if the stripped name is shorter than 3 characters, use a
safe default like "tmpfile"), then create the temp file with "." + fileFormat as
the suffix and write the decoded bytes to it.
| String script = "return (function(targetFileName) {" + | ||
| " let excelPath = null;" + | ||
| " try {" + | ||
| " let downloadItems = Array.from(document.querySelector(\"body > downloads-app\")?.shadowRoot?.querySelectorAll(\"edge-card\") || []);" + | ||
| " for (const downloadItem of downloadItems) {" + | ||
| " const fileNameElement = downloadItem.querySelector(\".downloads_itemTitle\");" + | ||
| " if (fileNameElement) {" + | ||
| " const fileName = fileNameElement.textContent.trim();" + | ||
| " if (fileName.toLowerCase() === targetFileName.toLowerCase() && fileName.toLowerCase().endsWith('.xlsx')) {" + | ||
| " const filePath = downloadItem.querySelector('.downloads_itemIconContainer > img')?.getAttribute('src');" + | ||
| " if (filePath) {" + | ||
| " try {" + | ||
| " const decodedPath = decodeURIComponent(filePath);" + | ||
| " const match = decodedPath.match(/path=(.*)&scale/);" + | ||
| " if (match && match[1]) {" + | ||
| " excelPath = match[1].replace(/\\+/g, ' ');" + | ||
| " excelPath = excelPath.replace(/%20/g, ' ');" + | ||
| " return excelPath;" + | ||
| " }" + | ||
| " } catch (decodeError) {" + | ||
| " console.error('Error decoding file path:', decodeError);" + | ||
| " }" + | ||
| " }" + | ||
| " }" + | ||
| " }" + | ||
| " }" + | ||
| " let downloadItemsLegacy = document.querySelectorAll('div[role=\"listitem\"]');" + | ||
| " for (const item of downloadItemsLegacy) {" + | ||
| " let fileNameElement = item.querySelector('button[aria-label]');" + | ||
| " if (fileNameElement) {" + | ||
| " let fileName = fileNameElement.getAttribute('aria-label');" + | ||
| " if (fileName && fileName.toLowerCase() === targetFileName.toLowerCase() && fileName.toLowerCase().endsWith('.xlsx')) {" + | ||
| " let img = item.querySelector('img');" + | ||
| " if (img) {" + | ||
| " let src = img.getAttribute('src');" + | ||
| " if (src) {" + | ||
| " try {" + | ||
| " excelPath = decodeURIComponent(src.split('path=')[1].split('&')[0]).replace(/\\+/g, ' ');" + | ||
| " excelPath = excelPath.replace(/%20/g, ' '); " + | ||
| " return excelPath;" + | ||
| " } catch (e) {" + | ||
| " console.error('Error extracting or decoding file path:', e);" + | ||
| " }" + | ||
| " }" + | ||
| " }" + | ||
| " }" + | ||
| " }" + | ||
| " }" + | ||
| " } catch (error) {" + | ||
| " console.error('Error during excel path retrieval:', error);" + | ||
| " }" + | ||
| " return excelPath || null;" + | ||
| "})('" + desiredFileName + "');"; | ||
|
|
There was a problem hiding this comment.
In comment, subject line bold.
Let's craft.
Also provide diff snippet to fix.
Let's craft final answer.
Ensure final message: include <!-- file_start: ... -->, scratchpad, comment, file_end. After scratchpad, we need actual comment.
Let's craft final message.
We must ensure final comment appended with ``.
Also mention reason: unescaped input results in JS syntax error when file names have '.
Let's produce.
Need to mention hooking to use script arguments to avoid string concatenation.
Also mention to adjust getDownloadedFileLocalPath? That method uses .endsWith('.xlsx')? But no major? maybe not.
But we may want to mention the same issue there? They don't pass desiredFileName but they search for .xlsx. They decode path. But no injection.
Focus on comment.
Need to produce diff with fix.
Proposed fix:
String script = "return (function(targetFileName) {...})(arguments[0]);";
Object obj = js.executeScript(script, desiredFileName);
But inside script they refer to targetFileName. We need to remove string concatenation and rely on argument.
Also the script uses '.
Let's craft diff.
Also emphasise to encode.
Let's produce final message accordingly.
Ensure final comment short.
Also mention degrade.
Let's craft final response with proper structure.
Need to include scratchpad at top.
Let's craft final.
Note: scratchpad should include tasks as earlier? Already good.
But we need to update scratchpad to mark done? maybe no but we can.
Maybe we should mark done? But instructions said keep tasks, status etc. Provide next steps? Already.
Maybe we should update to show done? maybe lighten. Provide in scratchpad [done], Reviewed EdgeExcelUtilities. Do so.
Update scratchpad.
Let's craft final message. Need to update scratchpad to include done etc.
Let's produce.
🤖 Prompt for AI Agents
In
excelActions_cloud/src/main/java/com/testsigma/addons/util/EdgeExcelUtilities.java
around lines 181-234: the JS snippet is built by concatenating desiredFileName
into the string which breaks when the filename contains single quotes or other
special chars; instead, stop inlining the filename into the script, change the
IIFE to accept its parameter from the WebDriver/JS executor (use arguments[0] or
pass the value as a script argument) and call executeScript with the script and
desiredFileName as a separate parameter so the browser runtime receives a
properly escaped string; update the Java call site to pass desiredFileName as an
argument to executeScript and remove the string concatenation of the filename
from the script.
|
|
||
| public static ExcelUtilities create(WebDriver driver, Logger logger) { | ||
| String browserName = ((RemoteWebDriver) driver).getCapabilities().getBrowserName().toLowerCase(); | ||
| System.out.println("browserName: " + browserName); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace System.out.println with logger.
Using System.out.println bypasses the logging framework and won't be captured in log files.
Apply this diff to use the logger instead:
- System.out.println("browserName: " + browserName);
+ logger.info("Browser detected: " + browserName);📝 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.
| System.out.println("browserName: " + browserName); | |
| logger.info("Browser detected: " + browserName); |
🤖 Prompt for AI Agents
In
excelActions_cloud/src/main/java/com/testsigma/addons/util/ExcelUtilitiesFactory.java
around line 11, replace the System.out.println("browserName: " + browserName)
call with the class logger: remove the println, ensure a logger exists (e.g.,
private static final Logger logger =
LoggerFactory.getLogger(ExcelUtilitiesFactory.class) or use the existing logger
field), and log the message via logger.info or logger.debug
(logger.info("browserName: {}", browserName)) so the output goes through the
logging framework and is captured in log files.
|
|
||
| String countType = testData.getValue().toString(); | ||
| logger.info("Count type: " + countType); | ||
| // int calculatedCount = PdfAndDocUtilities.calculateExcelDataCount("non-empty cells", sheet); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove commented-out code.
The commented line referencing the old PdfAndDocUtilities.calculateExcelDataCount should be deleted.
Apply this diff:
-// int calculatedCount = PdfAndDocUtilities.calculateExcelDataCount("non-empty cells", sheet);
int calculatedCount = calculateExcelDataCount(countType, sheet);📝 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.
| // int calculatedCount = PdfAndDocUtilities.calculateExcelDataCount("non-empty cells", sheet); | |
| int calculatedCount = calculateExcelDataCount(countType, sheet); |
🤖 Prompt for AI Agents
In
excelActions_cloud/src/main/java/com/testsigma/addons/web/StoreNonEmptyCellCount.java
around line 53, remove the commented-out line "// int calculatedCount
= PdfAndDocUtilities.calculateExcelDataCount("non-empty cells", sheet);" so the
dead/commented legacy code is deleted; commit the file with that single-line
removal and ensure no extra whitespace or leftover comment markers remain.
Addon Name: ExcelActions_cloud
Jarvis Link: https://jarvis.testsigma.com/ui/tenants/2817/addons
Jira : https://testsigma.atlassian.net/browse/CUS-7732
Added support for edge browser and changed it into factory design pattern
Summary by CodeRabbit
Release Notes
New Features
Chores