ParsingImprovements #361
ParsingImprovements #361
Conversation
…options - Added `cellTextFormat` parameter to control how cell text is extracted (options: `Compact`, `Lines`, `Markdown`). - Updated methods in `HtmlParserFromTable` to support new formatting options. - Improved handling of header rows by selecting the best candidate based on effective column span. - Enhanced data extraction from rows, including support for `data-category` and `data-severity` attributes. - Refactored `CmdletConvertFromHtmlTable` to utilize the new parsing capabilities and return `HtmlTableResult` directly.
… installation on Linux * Introduced environment variable `HTMLTINKERX_PLAYWRIGHT_WITH_DEPS` to control dependency installation. * Updated `HtmlBrowser` to check for this variable and adjust installation behavior accordingly. * Added tests to verify installation behavior when the variable is enabled or disabled.
|
Claude finished @PrzemyslawKlys's task —— View job PR Review: ParsingImprovementsThis pull request introduces significant enhancements to HTML table parsing and Playwright dependency management. Here's my comprehensive review: ✅ Strengths1. Playwright Installation Improvements
2. HTML Table Parsing Enhancements
3. Test Coverage
🔄 Areas for Improvement1. Code Quality IssuesThread Safety Concerns: // HtmlBrowser.Installer.cs:495-496
[DllImport("libc")]
private static extern uint geteuid();
Error Handling: // HtmlBrowser.Installer.cs:503-507
try {
return geteuid() == 0;
} catch {
// ignore
}
2. Performance ConsiderationsString Operations in Loops: // HtmlParserFromTable.cs:203-207
foreach (var kv in replaceContent) {
value = ReplaceCaseInsensitive(value, kv.Key, kv.Value);
}
Regex Usage:
3. Security ConsiderationsFile System Operations: // HtmlBrowser.Installer.cs:379-380
if (Directory.Exists(baseDir))
Directory.Delete(baseDir, true);
HTTP Client Usage:
4. Documentation and MaintainabilityXML Documentation:
Magic Numbers: var buffer = new byte[81920]; // HtmlBrowser.Installer.cs:352
🧪 Test Coverage AssessmentExcellent Coverage Areas:
Missing Test Coverage:
🚀 Performance ImplicationsPositive:
Potential Concerns:
📋 RecommendationsHigh Priority:
Medium Priority:
Low Priority:
📊 Overall AssessmentThis is a high-quality pull request that adds valuable functionality while maintaining good architectural patterns. The Playwright dependency management improvements are particularly well-designed, and the HTML table parsing enhancements provide needed flexibility. Recommendation: Approve with minor improvements - The code is production-ready but would benefit from addressing the P/Invoke safety issue and adding tests for the new text formatting features. |
There was a problem hiding this comment.
Pull request overview
This PR introduces HTML table parsing improvements along with enhanced control over Playwright browser installation dependencies. The title "ParsingImprovements" focuses on the primary feature additions, while the description highlights the Playwright dependency control mechanism.
Key changes:
- Added a new
CellTextFormatparameter to control how cell text is extracted from HTML tables (Compact, Lines, or Markdown) - Introduced the
HTMLTINKERX_PLAYWRIGHT_WITH_DEPSenvironment variable to control whether Playwright installs OS-level dependencies on Linux - Refactored table parsing logic to support data attributes for Category/Severity fields and improved header row selection
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| Sources/PSParseHTML.PowerShell/CmdletConvertFromHtmlTable.cs | Simplified cmdlet logic by extracting table retrieval into a helper method; changed OutputType from PSObject[] to HtmlTableResult; added CellTextFormat parameter |
| Sources/HtmlTinkerX/Playwright/HtmlBrowser.Installer.cs | Added environment variable check for controlling --with-deps flag during Playwright installation on Linux; includes fallback logic to detect root user |
| Sources/HtmlTinkerX/HtmlParserFromTable.cs | Extensive enhancements to table parsing including new cell text formatting, improved header row selection, data attribute support for Category/Severity fields, and helper methods for text extraction |
| Sources/HtmlTinkerX/HtmlParser.cs | Updated public API methods to include the new cellTextFormat parameter with default value |
| Sources/HtmlTinkerX/Enums/HtmlCellTextFormat.cs | New enum defining three text extraction modes: Compact, Lines, and Markdown |
| Sources/HtmlTinkerX.Tests/HtmlParserListTests.cs | Added null-forgiving operator to fix nullable reference warning |
| Sources/HtmlTinkerX.Tests/HtmlParserFromTableHelpersTests.cs | Updated test to pass new required HtmlCellTextFormat parameter |
| Sources/HtmlTinkerX.Tests/HtmlParserFormTests.cs | Added null-forgiving operator to fix nullable reference warning |
| Sources/HtmlTinkerX.Tests/HtmlOpenGraphTests.cs | Added null-forgiving operator to fix nullable reference warning |
| Sources/HtmlTinkerX.Tests/HtmlHttpClientFactoryTests.cs | Added nullable annotation to cookie variable |
| Sources/HtmlTinkerX.Tests/HtmlBrowserInstallerTests.cs | Added new test for disabled --with-deps behavior; updated existing test to explicitly enable the feature via environment variable |
| README.MD | Added documentation section explaining the new HTMLTINKERX_PLAYWRIGHT_WITH_DEPS environment variable and its usage on Linux |
| PSParseHTML.psd1 | Updated copyright year to 2026 |
Comments suppressed due to low confidence (1)
Sources/HtmlTinkerX/HtmlParserFromTable.cs:960
- This variable is manually disposed in a finally block - consider a C# using statement as a preferable resource management technique.
HttpClient http;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| int categoryIndex = headers.FindIndex(h => h.Equals("Category", StringComparison.OrdinalIgnoreCase)); | ||
| int severityIndex = headers.FindIndex(h => h.Equals("Severity", StringComparison.OrdinalIgnoreCase)); |
There was a problem hiding this comment.
The variables categoryIndex and severityIndex are computed but never used in this method. These variables are used in other similar methods (like ParseTablesWithAngleSharp and ParseTablesWithHtmlAgilityPackDetailed) to handle data attributes, but here they're calculated and then ignored. Either remove these unused variables or add the missing logic to use them for handling Category/Severity data attributes consistently with the other methods.
| int categoryIndex = headers.FindIndex(h => h.Equals("Category", StringComparison.OrdinalIgnoreCase)); | |
| int severityIndex = headers.FindIndex(h => h.Equals("Severity", StringComparison.OrdinalIgnoreCase)); |
| Dictionary<string, string?> dict = new(); | ||
| for (int i = 0; i < headers.Count; i++) { | ||
| string header = headers[i]; | ||
| dict[string.IsNullOrEmpty(header) ? i.ToString() : header] = rowValues[i]; | ||
| } | ||
| if (categoryIndex >= 0) { | ||
| string key = (row.GetAttribute("data-category") ?? string.Empty).Trim(); | ||
| if (key.Length > 0) { | ||
| string val = key; | ||
| if (dataValueLookup != null && dataValueLookup.TryGetValue(key, out var label)) { | ||
| val = label; | ||
| } | ||
| dict["Category"] = val; | ||
| } | ||
| } | ||
| if (severityIndex >= 0) { | ||
| string key = (row.GetAttribute("data-severity") ?? string.Empty).Trim(); | ||
| if (key.Length > 0) { | ||
| string val = key; | ||
| if (dataValueLookup != null && dataValueLookup.TryGetValue(key, out var label)) { | ||
| val = label; | ||
| } | ||
| dict["Severity"] = val; | ||
| } | ||
| } | ||
| if (dict.TryGetValue("Category", out var catVal) && string.IsNullOrWhiteSpace(catVal)) { | ||
| dict["Category"] = FillFromDataAttributes("Category", catVal, row, dataValueLookup); | ||
| } | ||
| if (dict.TryGetValue("Severity", out var sevVal) && string.IsNullOrWhiteSpace(sevVal)) { | ||
| dict["Severity"] = FillFromDataAttributes("Severity", sevVal, row, dataValueLookup); | ||
| } | ||
| if (dict.Count > 0) { | ||
| tableRows.Add(dict); | ||
| } |
There was a problem hiding this comment.
The code block from lines 391-424 appears to be incorrectly indented. These lines should be indented one additional level as they are part of the foreach (var row in rows.Skip(startIndex)) loop that begins at line 341. The current indentation makes the code structure unclear and difficult to read.
| Dictionary<string, string?> dict = new(); | |
| for (int i = 0; i < headers.Count; i++) { | |
| string header = headers[i]; | |
| dict[string.IsNullOrEmpty(header) ? i.ToString() : header] = rowValues[i]; | |
| } | |
| if (categoryIndex >= 0) { | |
| string key = (row.GetAttribute("data-category") ?? string.Empty).Trim(); | |
| if (key.Length > 0) { | |
| string val = key; | |
| if (dataValueLookup != null && dataValueLookup.TryGetValue(key, out var label)) { | |
| val = label; | |
| } | |
| dict["Category"] = val; | |
| } | |
| } | |
| if (severityIndex >= 0) { | |
| string key = (row.GetAttribute("data-severity") ?? string.Empty).Trim(); | |
| if (key.Length > 0) { | |
| string val = key; | |
| if (dataValueLookup != null && dataValueLookup.TryGetValue(key, out var label)) { | |
| val = label; | |
| } | |
| dict["Severity"] = val; | |
| } | |
| } | |
| if (dict.TryGetValue("Category", out var catVal) && string.IsNullOrWhiteSpace(catVal)) { | |
| dict["Category"] = FillFromDataAttributes("Category", catVal, row, dataValueLookup); | |
| } | |
| if (dict.TryGetValue("Severity", out var sevVal) && string.IsNullOrWhiteSpace(sevVal)) { | |
| dict["Severity"] = FillFromDataAttributes("Severity", sevVal, row, dataValueLookup); | |
| } | |
| if (dict.Count > 0) { | |
| tableRows.Add(dict); | |
| } | |
| Dictionary<string, string?> dict = new(); | |
| for (int i = 0; i < headers.Count; i++) { | |
| string header = headers[i]; | |
| dict[string.IsNullOrEmpty(header) ? i.ToString() : header] = rowValues[i]; | |
| } | |
| if (categoryIndex >= 0) { | |
| string key = (row.GetAttribute("data-category") ?? string.Empty).Trim(); | |
| if (key.Length > 0) { | |
| string val = key; | |
| if (dataValueLookup != null && dataValueLookup.TryGetValue(key, out var label)) { | |
| val = label; | |
| } | |
| dict["Category"] = val; | |
| } | |
| } | |
| if (severityIndex >= 0) { | |
| string key = (row.GetAttribute("data-severity") ?? string.Empty).Trim(); | |
| if (key.Length > 0) { | |
| string val = key; | |
| if (dataValueLookup != null && dataValueLookup.TryGetValue(key, out var label)) { | |
| val = label; | |
| } | |
| dict["Severity"] = val; | |
| } | |
| } | |
| if (dict.TryGetValue("Category", out var catVal) && string.IsNullOrWhiteSpace(catVal)) { | |
| dict["Category"] = FillFromDataAttributes("Category", catVal, row, dataValueLookup); | |
| } | |
| if (dict.TryGetValue("Severity", out var sevVal) && string.IsNullOrWhiteSpace(sevVal)) { | |
| dict["Severity"] = FillFromDataAttributes("Severity", sevVal, row, dataValueLookup); | |
| } | |
| if (dict.Count > 0) { | |
| tableRows.Add(dict); | |
| } |
| [DllImport("libc")] | ||
| private static extern uint geteuid(); | ||
|
|
||
| private static bool IsRunningAsRoot() { | ||
| if (!RuntimeInformation.IsOSPlatform(OSPlatform.Linux)) { | ||
| return false; | ||
| } | ||
|
|
||
| try { | ||
| return geteuid() == 0; | ||
| } catch { | ||
| // ignore | ||
| } | ||
|
|
||
| try { |
There was a problem hiding this comment.
Replace this call with a call to managed code if possible.
| [DllImport("libc")] | |
| private static extern uint geteuid(); | |
| private static bool IsRunningAsRoot() { | |
| if (!RuntimeInformation.IsOSPlatform(OSPlatform.Linux)) { | |
| return false; | |
| } | |
| try { | |
| return geteuid() == 0; | |
| } catch { | |
| // ignore | |
| } | |
| try { | |
| private static bool IsRunningAsRoot() { | |
| if (!RuntimeInformation.IsOSPlatform(OSPlatform.Linux)) { | |
| return false; | |
| } | |
| try { |
| [DllImport("libc")] | ||
| private static extern uint geteuid(); | ||
|
|
||
| private static bool IsRunningAsRoot() { | ||
| if (!RuntimeInformation.IsOSPlatform(OSPlatform.Linux)) { | ||
| return false; | ||
| } | ||
|
|
||
| try { | ||
| return geteuid() == 0; | ||
| } catch { | ||
| // ignore | ||
| } | ||
|
|
||
| try { |
There was a problem hiding this comment.
Minimise the use of unmanaged code.
| [DllImport("libc")] | |
| private static extern uint geteuid(); | |
| private static bool IsRunningAsRoot() { | |
| if (!RuntimeInformation.IsOSPlatform(OSPlatform.Linux)) { | |
| return false; | |
| } | |
| try { | |
| return geteuid() == 0; | |
| } catch { | |
| // ignore | |
| } | |
| try { | |
| private static bool IsRunningAsRoot() { | |
| if (!RuntimeInformation.IsOSPlatform(OSPlatform.Linux)) { | |
| return false; | |
| } | |
| try { |
| if (rows == null || rows.Count == 0) { | ||
| continue; | ||
| } | ||
| var dataValueLookup = BuildDataValueLookup(table); |
There was a problem hiding this comment.
This assignment to dataValueLookup is useless, since its value is never read.
| var dataValueLookup = BuildDataValueLookup(table); |
| if (!string.IsNullOrEmpty(key)) { | ||
| if (!dict.ContainsKey(key)) { | ||
| dict[key] = text; | ||
| } |
There was a problem hiding this comment.
These 'if' statements can be combined.
| if (!string.IsNullOrEmpty(key)) { | |
| if (!dict.ContainsKey(key)) { | |
| dict[key] = text; | |
| } | |
| if (!string.IsNullOrEmpty(key) && !dict.ContainsKey(key)) { | |
| dict[key] = text; |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (headerRow == null) { | ||
| return (metadata, rows, 0); |
There was a problem hiding this comment.
The new early return when headerRow is null means tables that have no <thead> and no <th> cells are now skipped entirely (because callers later drop results when metadata.Headers is empty). Previously, these tables were still parsed with default Column1..N headers based on the first row, so HTML like <table><tr><td>A</td></tr></table> would still yield data. With this change, those tables return nothing, which is a regression for users parsing headerless tables. Consider restoring the fallback to the first row or generating default headers instead of returning early.
Useful? React with 👍 / 👎.
| ValidateProxy(Proxy, ProxyCredential); | ||
| if (IncludeMetadata.IsPresent) { | ||
| List<HtmlTableResult> tables; | ||
| if (ParameterSetName == ParameterSetUrl) { | ||
| using HttpClient client = HttpClientHelper.Create(Proxy, ProxyCredential); | ||
| if (Engine == HtmlParserEngine.AngleSharp && !ReverseTable.IsPresent) { | ||
| string content = (await HtmlParser.ParseUrlWithAngleSharpAsync(Url.ToString(), client).ConfigureAwait(false)).DocumentElement.OuterHtml; | ||
| tables = HtmlParser.ParseTablesWithAngleSharpDetailed(content, Cast(ReplaceContent), Cast(ReplaceHeaders), AllProperties.IsPresent, SkipFooter.IsPresent, CleanHeaders.IsPresent, EmptyValuePlaceholder); | ||
| } else { | ||
| var doc = await HtmlParser.ParseUrlWithHtmlAgilityPackAsync(Url.ToString(), client).ConfigureAwait(false); | ||
| tables = HtmlParser.ParseTablesWithHtmlAgilityPackDetailed(doc.DocumentNode.OuterHtml, ReverseTable.IsPresent, Cast(ReplaceContent), Cast(ReplaceHeaders), AllProperties.IsPresent, SkipFooter.IsPresent, CleanHeaders.IsPresent, EmptyValuePlaceholder); | ||
| } | ||
| } else { | ||
| if (Engine == HtmlParserEngine.AngleSharp && !ReverseTable.IsPresent) { | ||
| tables = HtmlParser.ParseTablesWithAngleSharpDetailed(Content, Cast(ReplaceContent), Cast(ReplaceHeaders), AllProperties.IsPresent, SkipFooter.IsPresent, CleanHeaders.IsPresent, EmptyValuePlaceholder); | ||
| } else { | ||
| tables = HtmlParser.ParseTablesWithHtmlAgilityPackDetailed(Content, ReverseTable.IsPresent, Cast(ReplaceContent), Cast(ReplaceHeaders), AllProperties.IsPresent, SkipFooter.IsPresent, CleanHeaders.IsPresent, EmptyValuePlaceholder); | ||
| } | ||
| } | ||
|
|
||
| foreach (var tableResult in tables) { | ||
| WriteObject(CreateTableObject(tableResult)); | ||
| } | ||
| } else { | ||
| // Use the detailed parsing methods but extract only the Data part | ||
| List<HtmlTableResult> detailedTables; | ||
| if (ParameterSetName == ParameterSetUrl) { | ||
| using HttpClient client = HttpClientHelper.Create(Proxy, ProxyCredential); | ||
| if (Engine == HtmlParserEngine.AngleSharp && !ReverseTable.IsPresent) { | ||
| string content = (await HtmlParser.ParseUrlWithAngleSharpAsync(Url.ToString(), client).ConfigureAwait(false)).DocumentElement.OuterHtml; | ||
| detailedTables = HtmlParser.ParseTablesWithAngleSharpDetailed(content, Cast(ReplaceContent), Cast(ReplaceHeaders), AllProperties.IsPresent, SkipFooter.IsPresent, CleanHeaders.IsPresent, EmptyValuePlaceholder); | ||
| } else { | ||
| var doc = await HtmlParser.ParseUrlWithHtmlAgilityPackAsync(Url.ToString(), client).ConfigureAwait(false); | ||
| detailedTables = HtmlParser.ParseTablesWithHtmlAgilityPackDetailed(doc.DocumentNode.OuterHtml, ReverseTable.IsPresent, Cast(ReplaceContent), Cast(ReplaceHeaders), AllProperties.IsPresent, SkipFooter.IsPresent, CleanHeaders.IsPresent, EmptyValuePlaceholder); | ||
| } | ||
| } else { | ||
| if (Engine == HtmlParserEngine.AngleSharp && !ReverseTable.IsPresent) { | ||
| detailedTables = HtmlParser.ParseTablesWithAngleSharpDetailed(Content, Cast(ReplaceContent), Cast(ReplaceHeaders), AllProperties.IsPresent, SkipFooter.IsPresent, CleanHeaders.IsPresent, EmptyValuePlaceholder); | ||
| } else { | ||
| detailedTables = HtmlParser.ParseTablesWithHtmlAgilityPackDetailed(Content, ReverseTable.IsPresent, Cast(ReplaceContent), Cast(ReplaceHeaders), AllProperties.IsPresent, SkipFooter.IsPresent, CleanHeaders.IsPresent, EmptyValuePlaceholder); | ||
| } | ||
| } | ||
|
|
||
| // Return array of arrays - each table as a separate array (extract Data from detailed results) | ||
| var tableArrays = new List<PSObject[]>(); | ||
| foreach (var tableResult in detailedTables) { | ||
| tableArrays.Add(ConvertRows(tableResult.Data)); | ||
| } | ||
| var detailedTables = await GetTablesDetailedAsync(); | ||
|
|
||
| if (tableArrays.Count == 1) { | ||
| WriteObject(tableArrays[0], false); | ||
| } else { | ||
| if (tableArrays.Count > 1) { | ||
| WriteWarning($"{tableArrays.Count} tables found. Returning array of tables."); | ||
| } | ||
| WriteObject(tableArrays.ToArray(), false); | ||
| } | ||
| foreach (var tableResult in detailedTables) { | ||
| WriteObject(tableResult, false); |
There was a problem hiding this comment.
ProcessRecordAsync now always emits HtmlTableResult objects regardless of the IncludeMetadata switch. That makes -IncludeMetadata a no-op and also changes the default output shape (previously arrays of rows) for callers that don’t pass it, which can break existing PowerShell pipelines. Either keep the old branching behavior or drop the parameter and update docs/tests so callers don’t silently get metadata they didn’t request.
Useful? React with 👍 / 👎.
* Enhanced handling of tables without <thead> or <th> by using the first non-empty row to determine column count. * Updated output type to `PSObject[]` for better compatibility with PowerShell. * Added methods to convert rows into `PSObject` format and create structured table objects with metadata.
|
Claude finished @PrzemyslawKlys's task —— View job PR Review: ParsingImprovementsComprehensive review of parsing improvements and Playwright dependency management changes. ✅ Strengths1. Well-Designed Text Formatting SystemThe new
The implementation in 2. Robust Environment Variable Parsing
3. Enhanced Data AttributesCategory/Severity data attribute support with lookup fallbacks provides valuable metadata handling for specialized tables. 🚨 Critical Security IssuesHIGH PRIORITY: P/Invoke SafetyFile: [DllImport("libc")]
private static extern uint geteuid();Problem: This P/Invoke declaration can cause runtime crashes on non-Linux platforms. JIT processes Impact: Application crashes, potential security vulnerabilities Recommendation: Move to platform-conditional compilation: #if LINUX
[DllImport("libc")]
private static extern uint geteuid();
#endifException Handling IssueFile: } catch {
// ignore
}Silent exception swallowing makes debugging impossible. Log exceptions or catch specific types.
|
* Improved the dynamic HTML structure by adding a visual counter. * Adjusted the timeout duration in the video recording test based on the operating system.
|
Claude finished @PrzemyslawKlys's task —— View job PR Review: ParsingImprovementsComprehensive review of parsing improvements and Playwright dependency management changes.
🚨 Critical Security IssueHIGH PRIORITY: Unsafe P/Invoke DeclarationFile: [DllImport("libc")]
private static extern uint geteuid();Problem: This P/Invoke declaration will cause runtime crashes on non-Linux platforms. The JIT processes Impact: Application crashes, potential security vulnerabilities Recommendation: Move to platform-conditional compilation: #if LINUX
[DllImport("libc")]
private static extern uint geteuid();
#endif
|
… severity handling * Enhanced handling of `data-category` and `data-severity` attributes. * Simplified header generation logic for tables without explicit headers. * Improved error handling and code readability in `IsRunningAsRoot` method.
|
Claude finished @PrzemyslawKlys's task —— View job PR Review: ParsingImprovementsComprehensive review of parsing improvements and Playwright dependency management changes.
✅ Strengths1. Well-Designed Text Formatting SystemThe new
The implementation in 2. Robust Environment Variable Parsing
3. Enhanced Data AttributesCategory/Severity data attribute support with lookup fallbacks provides valuable metadata handling for specialized tables. 4. Comprehensive Test Coverage for PlaywrightThe new tests thoroughly cover both enabled/disabled dependency scenarios with proper mocking and environment cleanup. 🚨 Critical Security IssueHIGH PRIORITY: Safe P/Invoke Implementation ✅File: Excellent Fix: The P/Invoke safety issue from previous reviews has been completely resolved. The code now uses a fully managed approach:
This is a significant security improvement that eliminates potential platform crashes while maintaining functionality.
|
HTMLTINKERX_PLAYWRIGHT_WITH_DEPSto control dependency installation.HtmlBrowserto check for this variable and adjust installation behavior accordingly.