push latest fixes#4404
Conversation
WalkthroughThis PR modifies Excel action handling to expand cell address pulling to ReadData operations alongside ReadCellData, refines sheet detail extraction with header-aware row counting and new address outputs, and adjusts cell address calculations with updated indexing. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (2)**/{Ginger,GingerCore}/**/{*Page,*Window}.xaml.cs📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.cs📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
🧠 Learnings (2)📚 Learning: 2025-12-18T05:20:03.536ZApplied to files:
📚 Learning: 2025-12-18T05:29:42.896ZApplied to files:
🧬 Code graph analysis (2)Ginger/Ginger/Actions/ActionEditPages/ActExcelEditPage.xaml.cs (1)
Ginger/GingerCoreNET/ActionsLib/ActExcel.cs (1)
🔇 Additional comments (4)
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: 2
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
Ginger/Ginger/Actions/ActionEditPages/ActExcelEditPage.xaml.csGinger/GingerCoreNET/ActionsLib/ActExcel.cs
🧰 Additional context used
📓 Path-based instructions (2)
**/{Ginger,GingerCore}/**/{*Page,*Window}.xaml.cs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Name pages and windows following the pattern:
{Feature}Pageor{Feature}Window
Files:
Ginger/Ginger/Actions/ActionEditPages/ActExcelEditPage.xaml.cs
**/*.cs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
Reporter.ToLog(eLogLevel.ERROR, message)for logging errors
Files:
Ginger/Ginger/Actions/ActionEditPages/ActExcelEditPage.xaml.csGinger/GingerCoreNET/ActionsLib/ActExcel.cs
🧠 Learnings (3)
📚 Learning: 2025-07-16T14:42:32.229Z
Learnt from: prashelke
Repo: Ginger-Automation/Ginger PR: 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/Actions/ActionEditPages/ActExcelEditPage.xaml.cs
📚 Learning: 2025-12-18T05:20:03.536Z
Learnt from: AmanPrasad43
Repo: Ginger-Automation/Ginger PR: 4385
File: Ginger/Ginger/Actions/ActionEditPages/ActExcelEditPage.xaml:0-0
Timestamp: 2025-12-18T05:20:03.536Z
Learning: In the Ginger codebase, WPF DataTriggers can use string values (e.g., "ReadCellByIndex", "GetSheetDetails") to compare against enum properties (e.g., eExcelActionType) because WPF automatically handles enum-to-string conversion during DataTrigger evaluation. This pattern is confirmed to work correctly and is acceptable in XAML files.
Applied to files:
Ginger/Ginger/Actions/ActionEditPages/ActExcelEditPage.xaml.cs
📚 Learning: 2025-12-18T05:29:42.896Z
Learnt from: AmanPrasad43
Repo: Ginger-Automation/Ginger PR: 4385
File: Ginger/GingerCoreNET/Run/Excels/ExcelNPOIOperations.cs:468-475
Timestamp: 2025-12-18T05:29:42.896Z
Learning: In the Ginger codebase, assume file extensions are lowercase (e.g., .xlsx, .xls). Do not perform case-insensitive extension checks (such as StringComparison.OrdinalIgnoreCase) when using EndsWith() or similar methods to verify file extensions; rely on lowercase comparisons or convert to lowercase first. This ensures consistency and avoids unnecessary case-insensitive comparisons across C# source files.
Applied to files:
Ginger/Ginger/Actions/ActionEditPages/ActExcelEditPage.xaml.csGinger/GingerCoreNET/ActionsLib/ActExcel.cs
🧬 Code graph analysis (2)
Ginger/Ginger/Actions/ActionEditPages/ActExcelEditPage.xaml.cs (1)
Ginger/GingerCoreNET/ActionsLib/ActExcel.cs (3)
ActExcel(37-639)ReadCellData(489-523)ReadData(370-468)
Ginger/GingerCoreNET/ActionsLib/ActExcel.cs (1)
Ginger/GingerCoreCommon/Actions/Act.cs (1)
AddOrUpdateReturnParamActual(1308-1335)
🔇 Additional comments (1)
Ginger/GingerCoreNET/ActionsLib/ActExcel.cs (1)
297-356: LGTM! Row counting and address range logic are well-implemented.The changes correctly:
- Count only non-empty data rows
- Parse and validate the header row number with safe defaults
- Calculate total row count including header rows
- Generate accurate cell address ranges for Excel operations
The defensive checks for invalid header row numbers (defaulting to 1 if < 1) prevent potential calculation errors.
| { | ||
| if (!mAct.CheckMandatoryFieldsExists([nameof(mAct.CalculatedFileName), nameof(mAct.CalculatedSheetName)])) return null; | ||
| if (!isViewAllData && (mAct.ExcelActionType == ActExcel.eExcelActionType.ReadCellData || mAct.ExcelActionType == ActExcel.eExcelActionType.ReadData) && !string.IsNullOrWhiteSpace(mAct.CalculatedFilter)) | ||
| if (!isViewAllData && (mAct.ExcelActionType == ActExcel.eExcelActionType.ReadCellData || (mAct.ExcelActionType == ActExcel.eExcelActionType.ReadData)) && mAct.DataSelectionMethod == ActExcel.eDataSelectionMethod.ByCellAddress && !string.IsNullOrWhiteSpace(mAct.CalculatedFilter)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for ReadCellData usage patterns with DataSelectionMethod checks
echo "=== Checking Execute method for DataSelectionMethod usage with ReadCellData ==="
rg -n 'case.*ReadCellData' --type=cs -A 10
echo -e "\n=== Checking if DataSelectionMethod is checked in ReadCellData execution path ==="
rg -n 'ReadCellData' --type=cs -B 2 -A 5 | head -100
echo -e "\n=== Checking ActExcel.cs specifically for Execute method ==="
fd 'ActExcel.cs' --type f --exec cat -n {} \; | sed -n '250,290p'
echo -e "\n=== Checking UpdateVisibility logic in ActExcelEditPage ==="
fd 'ActExcelEditPage.xaml.cs' --type f --exec cat -n {} \; | sed -n '80,120p'Repository: Ginger-Automation/Ginger
Length of output: 15825
🏁 Script executed:
#!/bin/bash
# Get the full ReadCellData method implementation to understand the else path
echo "=== Full ReadCellData method implementation in ActExcel.cs ==="
fd 'ActExcel.cs' --path '*/ActionsLib/*' --type f --exec cat -n {} \; | sed -n '489,530p'
echo -e "\n=== Checking what happens after the ByCellAddress check ==="
rg -n 'if.*DataSelectionMethod.*ByCellAddress' --type=cs -A 20 | head -50
echo -e "\n=== Searching for ReadData method to understand parameter-based approach ==="
rg -n 'private void ReadData\(\)' --type=cs -A 15Repository: Ginger-Automation/Ginger
Length of output: 5250
🏁 Script executed:
#!/bin/bash
# Get specific lines from ActExcel.cs to view the complete ReadCellData method
echo "=== Getting lines 489-550 to see full ReadCellData method ==="
fd 'ActExcel.cs' -x wc -l {} \;
fd 'ActExcel.cs' -x cat -n {} \; | sed -n '489,550p'
echo -e "\n=== Check if there's else clause or return in the method ==="
fd 'ActExcel.cs' -x cat -n {} \; | sed -n '374,420p'Repository: Ginger-Automation/Ginger
Length of output: 6296
🏁 Script executed:
#!/bin/bash
# Verify if there are any other guards or validations for ReadCellData with ByParameters
echo "=== Check if there's validation when executing ReadCellData with ByParameters ==="
rg -n 'ExcelActionType.*ReadCellData' --type=cs -B 3 -A 5 | head -40
echo -e "\n=== Check if CalculatedFilter property handles ReadCellData specifically ==="
sed -n '170,190p' <(fd 'ActExcel.cs' -x cat -n {})Repository: Ginger-Automation/Ginger
Length of output: 5541
ReadCellData() method (line 489) should validate DataSelectionMethod before execution.
The ReadCellData() method unconditionally calls excelOperator.ReadCellData() regardless of DataSelectionMethod. However, CalculatedFilter returns different values based on the mode: cell addresses for ByCellAddress or SQL filters for ByParameters.
Passing a SQL filter to excelOperator.ReadCellData() (which expects cell addresses) will fail. Align this method with ReadData() (line 374), which properly checks DataSelectionMethod and routes to either ReadCellData() or ReadData() accordingly. If DataSelectionMethod == ByParameters, call excelOperator.ReadData() instead.
🤖 Prompt for AI Agents
In Ginger/Ginger/Actions/ActionEditPages/ActExcelEditPage.xaml.cs around line
323 (method ReadCellData at ~line 489, ReadData referenced at ~line 374), the
code unconditionally calls excelOperator.ReadCellData() even when
DataSelectionMethod == ByParameters which passes an SQL-style CalculatedFilter
to a method expecting cell addresses; modify ReadCellData() to first check
mAct.DataSelectionMethod and if it equals ByParameters call
excelOperator.ReadData(...) with the same parameters used in ReadData(),
otherwise proceed to call excelOperator.ReadCellData(...) (ensuring
CalculatedFilter is used as cell address only for ByCellAddress). Ensure method
signatures and any returned values match the chosen operator call.
| int headerRow = 1; | ||
| int.TryParse(CalculatedHeaderRowNum,out headerRow); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add validation for headerRow to ensure consistency.
The header row parsing lacks the validation present in GetSheetDetails (lines 312-315). If a user explicitly provides an invalid value like "0" or "-1", TryParse will succeed but the value will be invalid, leading to incorrect cell address calculations on line 411.
🔎 Proposed fix to add validation
int headerRow = 1;
int.TryParse(CalculatedHeaderRowNum,out headerRow);
+if (headerRow < 1)
+{
+ headerRow = 1;
+}🤖 Prompt for AI Agents
In Ginger/GingerCoreNET/ActionsLib/ActExcel.cs around lines 403-404, the
headerRow parsed from CalculatedHeaderRowNum is not validated (TryParse can
yield 0 or negative values); change the logic to parse into headerRow and then
ensure headerRow >= 1 (matching GetSheetDetails validation) — if the parsed
value is less than 1, reset headerRow to 1 (or apply the same fallback used in
GetSheetDetails) and optionally emit a warning/log so downstream cell address
calculations remain correct.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Ginger/GingerCoreNETUnitTest/LinuxTransformationTests/ActExcelTests.cs (1)
207-225: Consider adding Path verification for comprehensive test coverage.For consistency with
ReadCellDataExcelAllCellTest, consider adding assertions to verify thePathproperty in these single-cellReadCellDatatests. Both should return "A2" as the Path value.🔎 Suggested enhancement
For
ReadCellDataExcelOneCellTest(after line 224):Assert.AreEqual(actExcel.ActReturnValues[0].Actual, "1"); Assert.AreEqual(actExcel.ActReturnValues[0].Param, "ID"); +Assert.AreEqual(actExcel.ActReturnValues[0].Path, "A2");For
ReadCellDataExcelOneCellWithLongDigitsTest(after line 341):Assert.AreEqual(actExcel.ActReturnValues[0].Actual, "30465673520871400000"); Assert.AreEqual(actExcel.ActReturnValues[0].Param, "SIM"); +Assert.AreEqual(actExcel.ActReturnValues[0].Path, "A2");Also applies to: 324-342
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
Ginger/GingerCoreNETUnitTest/LinuxTransformationTests/ActExcelTests.cs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
Reporter.ToLog(eLogLevel.ERROR, message)for logging errors
Files:
Ginger/GingerCoreNETUnitTest/LinuxTransformationTests/ActExcelTests.cs
🧠 Learnings (1)
📚 Learning: 2025-12-18T05:29:42.896Z
Learnt from: AmanPrasad43
Repo: Ginger-Automation/Ginger PR: 4385
File: Ginger/GingerCoreNET/Run/Excels/ExcelNPOIOperations.cs:468-475
Timestamp: 2025-12-18T05:29:42.896Z
Learning: In the Ginger codebase, assume file extensions are lowercase (e.g., .xlsx, .xls). Do not perform case-insensitive extension checks (such as StringComparison.OrdinalIgnoreCase) when using EndsWith() or similar methods to verify file extensions; rely on lowercase comparisons or convert to lowercase first. This ensures consistency and avoids unnecessary case-insensitive comparisons across C# source files.
Applied to files:
Ginger/GingerCoreNETUnitTest/LinuxTransformationTests/ActExcelTests.cs
🔇 Additional comments (1)
Ginger/GingerCoreNETUnitTest/LinuxTransformationTests/ActExcelTests.cs (1)
245-245: LGTM! Test expectation correctly updated to validate explicit cell addresses.The assertion now verifies that
ReadCellDatareturns explicit Excel cell addresses (B2, C2, D2, etc.) in the Path property, which aligns with the implementation changes described in the AI summary.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Ginger/GingerCoreNET/ActionsLib/ActExcel.cs (2)
411-426: Minor:currentRowNumis computed unconditionally but only used whenPullCellAddressis true.Moving the calculation inside the
if (PullCellAddress)block (lines 428-433) would avoid unnecessary computation.🔎 Proposed optimization
for (int currentRow = 0; currentRow < excelDataTable.Rows.Count; currentRow++) { DataRow r = excelDataTable.Rows[currentRow]; - int currentRowNum = headerRow + 1 + currentRow; for (int currentCol = 0; currentCol < excelDataTable.Columns.Count; currentCol++) { if (SelectAllRows) { AddOrUpdateReturnParamActualWithPath(excelDataTable.Columns[currentCol].ColumnName, r[currentCol].ToString(), "" + (currentRow + 1).ToString()); } else { AddOrUpdateReturnParamActual(excelDataTable.Columns[currentCol].ColumnName, r[currentCol].ToString()); } if (PullCellAddress) { + int currentRowNum = headerRow + 1 + currentRow; string cellAddress = GetColumnName(currentCol + 1) + currentRowNum; AddOrUpdateReturnParamActual($"{excelDataTable.Columns[currentCol].ColumnName}_CellAddress", cellAddress); } } }
515-526: Path format uses numeric row+column indices instead of cell address for clarity.The path on line 519 concatenates numeric indices (e.g.,
"11","12") rather than using the calculatedcellAddress(e.g.,"A5"). This pattern is consistent elsewhere in the codebase (e.g., ActDBValidation.cs line 589 uses identical concatenation), but it can be ambiguous—for example,"111"could represent row 1/col 11, row 11/col 1, or row 111/col 1. Since the calculatedcellAddressis already available and added as a separate_CellAddressparameter, consider using it as the path for explicit clarity.if (PullCellAddress) { // Calculate proper address (e.g. A5) string cellAddress = GetColumnName(startCoord.Col + i) + currentRowNum; - AddOrUpdateReturnParamActualWithPath(excelDataTable.Columns[i].ColumnName, r[i].ToString(), (j + 1).ToString() + (i + 1).ToString()); + AddOrUpdateReturnParamActualWithPath(excelDataTable.Columns[i].ColumnName, r[i].ToString(), cellAddress); AddOrUpdateReturnParamActual($"{excelDataTable.Columns[i].ColumnName}_CellAddress", cellAddress); }
♻️ Duplicate comments (1)
Ginger/GingerCoreNET/ActionsLib/ActExcel.cs (1)
403-404: Add validation forheaderRowto prevent incorrect cell address calculations.Unlike
GetSheetDetails(lines 312-315), this parsing doesn't validate thatheaderRow >= 1. IfCalculatedHeaderRowNumis"0"or negative,TryParsesucceeds but produces invalid row numbers forcellAddresson line 430.🔎 Proposed fix
int headerRow = 1; int.TryParse(CalculatedHeaderRowNum,out headerRow); +if (headerRow < 1) +{ + headerRow = 1; +}
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
Ginger/Ginger/Actions/ActionEditPages/ActExcelEditPage.xaml.csGinger/GingerCoreNET/ActionsLib/ActExcel.cs
🧰 Additional context used
📓 Path-based instructions (2)
**/{Ginger,GingerCore}/**/{*Page,*Window}.xaml.cs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Name pages and windows following the pattern:
{Feature}Pageor{Feature}Window
Files:
Ginger/Ginger/Actions/ActionEditPages/ActExcelEditPage.xaml.cs
**/*.cs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
Reporter.ToLog(eLogLevel.ERROR, message)for logging errors
Files:
Ginger/Ginger/Actions/ActionEditPages/ActExcelEditPage.xaml.csGinger/GingerCoreNET/ActionsLib/ActExcel.cs
🧠 Learnings (2)
📚 Learning: 2025-12-18T05:20:03.536Z
Learnt from: AmanPrasad43
Repo: Ginger-Automation/Ginger PR: 4385
File: Ginger/Ginger/Actions/ActionEditPages/ActExcelEditPage.xaml:0-0
Timestamp: 2025-12-18T05:20:03.536Z
Learning: In the Ginger codebase, WPF DataTriggers can use string values (e.g., "ReadCellByIndex", "GetSheetDetails") to compare against enum properties (e.g., eExcelActionType) because WPF automatically handles enum-to-string conversion during DataTrigger evaluation. This pattern is confirmed to work correctly and is acceptable in XAML files.
Applied to files:
Ginger/Ginger/Actions/ActionEditPages/ActExcelEditPage.xaml.cs
📚 Learning: 2025-12-18T05:29:42.896Z
Learnt from: AmanPrasad43
Repo: Ginger-Automation/Ginger PR: 4385
File: Ginger/GingerCoreNET/Run/Excels/ExcelNPOIOperations.cs:468-475
Timestamp: 2025-12-18T05:29:42.896Z
Learning: In the Ginger codebase, assume file extensions are lowercase (e.g., .xlsx, .xls). Do not perform case-insensitive extension checks (such as StringComparison.OrdinalIgnoreCase) when using EndsWith() or similar methods to verify file extensions; rely on lowercase comparisons or convert to lowercase first. This ensures consistency and avoids unnecessary case-insensitive comparisons across C# source files.
Applied to files:
Ginger/Ginger/Actions/ActionEditPages/ActExcelEditPage.xaml.csGinger/GingerCoreNET/ActionsLib/ActExcel.cs
🧬 Code graph analysis (2)
Ginger/Ginger/Actions/ActionEditPages/ActExcelEditPage.xaml.cs (1)
Ginger/GingerCoreNET/ActionsLib/ActExcel.cs (3)
ActExcel(37-648)ReadData(370-468)ReadCellData(489-532)
Ginger/GingerCoreNET/ActionsLib/ActExcel.cs (1)
Ginger/GingerCoreCommon/Actions/Act.cs (2)
AddOrUpdateReturnParamActual(1308-1335)AddOrUpdateReturnParamActualWithPath(1359-1387)
🔇 Additional comments (4)
Ginger/Ginger/Actions/ActionEditPages/ActExcelEditPage.xaml.cs (2)
119-126: Expanded visibility for "Pull Cell Address" checkbox looks correct.The condition now correctly shows the checkbox for both
ReadDataandReadCellDataaction types when in "By Parameters" mode, aligning with thePullCellAddresshandling added to theReadData()method inActExcel.cs(lines 428-433).
323-326: Good fix: ByCellAddress guard prevents filter type mismatch.Adding the
DataSelectionMethod == ByCellAddresscheck correctly ensures thatReadCellDatais only called when the filter is a cell address (e.g.,"A1:D10"), not an SQL-style filter (e.g.,"ColName='value'"). This addresses the data type mismatch concern.Ginger/GingerCoreNET/ActionsLib/ActExcel.cs (2)
408-433: Cell address logic is correct, contingent onheaderRowvalidation.The formula
currentRowNum = headerRow + 1 + currentRowcorrectly offsets past the header row. ForheaderRow = 1andcurrentRow = 0, the first data row is at Excel row 2, which is correct.Note: This logic depends on the
headerRowvalidation fix mentioned above.
316-328: Row counting logic is consistent and correct.The calculation properly accounts for header rows and data rows:
rowCountcounts only non-empty data rows (excluding header)totalRowCount = rowCount + headerRowNumincludes the header in the totalUsedRangeAddressrange (rows 1 tostartRowIndex + rowCount) equalstotalRowCountFor example, with 5 data rows and
headerRowNum=1:totalRowCount=6and range spans rows 1–6 (6 rows), which is consistent. The method also properly handles empty sheets and follows the coding guideline by usingReporter.ToLog(eLogLevel.ERROR, ...)for error logging.
2 participants
Description
Type of Change
Checklist
[IsSerializedForLocalRepository]where neededReporter.ToLog()patternSummary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.