feat: Add detailed validation error messages to ApiComparison #52
Conversation
- Implement hierarchical change category organization with collapsible type groups - Change from type-first to change-category-first organization - Add collapsible type groups that default to collapsed state - Fix summary navigation links to properly navigate to sections - Update HTML template structure: * Modify HtmlFormatterScriban.cs to use GroupChangesByType method * Update main-layout.scriban with collapsible type groups and navigation * Enhance styles.css with type group collapsibility and hover effects * Add scripts.js toggle functionality with session storage persistence - Make section headers fully clickable (not just arrow buttons) - Add consistent hover effects for section and type headers - Fix CLI workflow tests to expect correct exit codes - All 425 tests passing Resolves #49
- Add InvalidMessage property to ApiComparison class for detailed error reporting - Enhanced IsValid() method to provide specific validation failure details - Updated ExitCodeManager to include InvalidMessage in log output - Improves debugging experience when API comparisons fail validation Resolves #51
WalkthroughThe changes introduce detailed validation error reporting for API comparisons. An Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ApiComparison
participant ExitCodeManager
participant Logger
User->>ApiComparison: IsValid()
ApiComparison->>ApiComparison: Validate changes, set InvalidMessage if invalid
ApiComparison-->>User: Return true/false
User->>ExitCodeManager: GetExitCode(apiComparison)
ExitCodeManager->>ApiComparison: Check IsValid()
alt Invalid
ExitCodeManager->>Logger: Log InvalidMessage (if present)
ExitCodeManager-->>User: Return ComparisonError
else Valid
ExitCodeManager-->>User: Return appropriate exit code
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (9)
src/DotNetApiDiff/Models/ApiComparison.cs (1)
84-113: Deduplicate the four “wrong-collection” checksThe four almost-identical LINQ blocks differ only by the collection & expected
ChangeType.
Extract into a small local function to cut repetition and reduce future maintenance risk.bool CheckWrongType(IEnumerable<ApiChange> set, ChangeType expected, string setName) { var wrong = set.Where(c => c.Type != expected).ToList(); if (!wrong.Any()) return true; messages.Add($"{setName} contains {wrong.Count} invalid item(s): " + string.Join(", ", wrong.Select(c => $"'{c.Description}' ({c.Type})"))); return false; }src/DotNetApiDiff/Reporting/HtmlFormatterScriban.cs (2)
235-254:GroupChangesnow appears unused – safe to removeAfter the switch to
GroupChangesByType, the oldGroupChangesmethod is dead code.
Keeping it around inflates the file and invites drift. Consider deleting it (or markingObsoleteif public).
285-333: Edge-cases missing inExtractContainingType& friendsString parsing fails for some legal CLR names, e.g. generic arity/back-tick (
TypeName\1.Method) or operators (op_Addition). Relying on the Roslyn symbol model (or at leastMetadataName) would be safer than manualLastIndexOf`.
If that’s out of scope, add unit tests to cover:
- Generic types & members (
List<T>.Add)- Nested generic types (
Outer\1+Inner`)- Overloaded operators (
MyType.op_Addition)src/DotNetApiDiff/Reporting/HtmlTemplates/type-group.scriban (1)
24-31: Consider adding accessible text for the visual-only glyphsThe template relies on emoji (📁,
⚠️ , BREAKING) to convey key information. Screen-reader users will not get that context. Add visually-hidden text (e.g.,<span class="sr-only">Folder</span>) or usearia-labelon the elements so the semantics aren’t lost.src/DotNetApiDiff/Reporting/HtmlTemplates/scripts.js (1)
140-170: Section initialisation list is hard-coded
initializeSections()enumerates only four section IDs. If additional categories (e.g.,moved-items) are added later they will not get the collapsed/expanded behaviour. Iterate over.changes-sectionelements instead of a static array.src/DotNetApiDiff/Reporting/HtmlTemplates/styles.css (1)
107-118: Duplicate.toggle-iconrotation rules
.toggle-iconrotation logic is now duplicated for section and type toggles. Consider using a single shared rule:.toggle-control.collapsed .toggle-icon { transform: rotate(0deg); } .toggle-control:not(.collapsed) .toggle-icon { transform: rotate(90deg); }and add
toggle-controlto both.section-toggleand.type-toggleto DRY the stylesheet.tests/DotNetApiDiff.Tests/Integration/CliWorkflowTests.cs (1)
425-426: Reduce duplication: extract an exit-code assertion helper (optional)The “0 or 1” assertion is repeated in many places. Consider a small helper to centralize this logic and message format.
Example helper within this test class:
private static void AssertExitCodeIsOk(int exitCode, string contextMessage) { Assert.Contains(exitCode, new[] { 0, 1 }); }Then replace usages like:
Assert.True(result.ExitCode == 0 || result.ExitCode == 1, $"CLI should succeed with namespace filtering. Exit code: {result.ExitCode}");with:
AssertExitCodeIsOk(result.ExitCode, "CLI should succeed with namespace filtering");tests/DotNetApiDiff.Tests/Reporting/HtmlFormatterScribanTests.cs (2)
74-74: Emoji-based assertion may be brittle (optional)Asserting on the “📁” glyph can be fragile across encodings/renderers. Consider adding a structural assertion too (e.g., for a group header element/class) to reduce coupling to the icon.
93-93: ElementName includes containing type — ensure output reflects itGood update to align with grouping by containing type. Consider asserting the fully-qualified member name appears to strengthen the test.
Assert.Contains("Removed Items", report); Assert.Contains("OldMethod", report); Assert.Contains("Removed method", report); + Assert.Contains("OldClass.OldMethod", report); Assert.Contains("📁 OldClass", report); // Type group header should show type name
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/DotNetApiDiff/ExitCodes/ExitCodeManager.cs(1 hunks)src/DotNetApiDiff/Models/ApiComparison.cs(2 hunks)src/DotNetApiDiff/Reporting/HtmlFormatterScriban.cs(6 hunks)src/DotNetApiDiff/Reporting/HtmlTemplates/change-group.scriban(1 hunks)src/DotNetApiDiff/Reporting/HtmlTemplates/main-layout.scriban(2 hunks)src/DotNetApiDiff/Reporting/HtmlTemplates/scripts.js(2 hunks)src/DotNetApiDiff/Reporting/HtmlTemplates/styles.css(2 hunks)src/DotNetApiDiff/Reporting/HtmlTemplates/type-group.scriban(1 hunks)tests/DotNetApiDiff.Tests/Integration/CliWorkflowTests.cs(10 hunks)tests/DotNetApiDiff.Tests/Reporting/HtmlFormatterScribanTests.cs(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/DotNetApiDiff.Tests/Integration/CliWorkflowTests.cs (1)
tests/DotNetApiDiff.Tests/Integration/TypeMappingIntegrationTests.cs (1)
ExitCode(79-132)
🪛 GitHub Actions: Code Quality
src/DotNetApiDiff/ExitCodes/ExitCodeManager.cs
[error] 137-137: dotnet format: Fix whitespace formatting. Replace 22 characters with '\n '.
[error] 138-138: dotnet format: Fix whitespace formatting. Replace 22 characters with '\n '.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build on windows-latest
🔇 Additional comments (11)
src/DotNetApiDiff/Reporting/HtmlTemplates/main-layout.scriban (1)
155-170: ID slugging misses several unsafe characters
group.keyis only stripped of space, dot and back-tick. Type names may include+,<,>,,, etc. Those produce invalid IDs and breakdocument.getElementById. Consider a dedicated slug helper that removes/escapes everything except[A-Za-z0-9_-].tests/DotNetApiDiff.Tests/Integration/CliWorkflowTests.cs (9)
141-142: Exit code 1 for breaking changes — aligned and OKAccepting 0 or 1 here matches the updated semantics. Looks good.
166-167: Consistent exit code expectation — OKAssertion updated to 0 or 1 (was 0 or 2). Aligned with new behavior.
201-202: Consistent exit code expectation — OKMatches the new “breaking changes => exit code 1” mapping.
236-237: Consistent exit code expectation — OKAligned with updated ExitCodeManager behavior.
367-368: Consistent exit code expectation — OKThe change reads correctly and aligns with the rest.
448-449: Consistent exit code expectation — OKKeeps test intent clear with updated codes.
471-472: Consistent exit code expectation — OKAligned with new behavior and consistent with other tests.
617-618: Consistent exit code expectation — OKMatches new semantics; assertion reads well.
539-540: No remaining references to exit code 2
I searched the entire test suite and codebase for any expectations or comments around exit code 2 and found none. Restricting the CLI to exit codes 0 or 1 is safe.• tests/DotNetApiDiff.Tests/Integration/CliWorkflowTests.cs (lines 539–540)
csharp // Exit code 1 is expected when there are breaking changes Assert.True(result.ExitCode == 0 || result.ExitCode == 1, $"CLI should execute successfully. Exit code: {result.ExitCode}");tests/DotNetApiDiff.Tests/Reporting/HtmlFormatterScribanTests.cs (1)
108-108: Type group header assertion — OKChecks the expected group header after the grouping refactor. Looks good.
… accessible toggles, robust JS) Signed-off-by: jbrinkman <joe.brinkman@improving.com>
jbrinkman
deleted the
feature/issue-51-api-comparison-validation-diagnostics
branch
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/DotNetApiDiff/Reporting/HtmlTemplates/main-layout.scriban (2)
102-107: Reintroduced nested interactive controls and non-keyboard-activatable headerClickable div wrapping a button is an a11y anti-pattern; only the button should be interactive. Also add proper ARIA and type attributes.
Apply:
- <div class="section-header" onclick="toggleSection('breaking-changes')"> + <div class="section-header"> <h2>⚠️ Breaking Changes</h2> - <button class="section-toggle collapsed"> - <span class="toggle-icon">▶</span> - </button> + <button + class="section-toggle collapsed" + type="button" + aria-controls="breaking-changes-content" + aria-expanded="false" + aria-label="Toggle breaking changes section" + onclick="toggleSection('breaking-changes')"> + <span class="toggle-icon">▶</span> + </button> </div>
142-147: Same a11y issue in other change sections — make the button the only interactive controlRemove onclick from the header div; move it to the button and add ARIA.
- <div class="section-header" onclick="toggleSection('{{ section.change_type }}-items')"> + <div class="section-header"> <h2>{{ section.icon }} {{ section.title }} ({{ section.count }})</h2> - <button class="section-toggle collapsed"> - <span class="toggle-icon">▶</span> - </button> + <button + class="section-toggle collapsed" + type="button" + aria-controls="{{ section.change_type }}-items-content" + aria-expanded="false" + aria-label="Toggle {{ section.title }} section" + onclick="toggleSection('{{ section.change_type }}-items')"> + <span class="toggle-icon">▶</span> + </button> </div>
🧹 Nitpick comments (1)
src/DotNetApiDiff/Reporting/HtmlTemplates/main-layout.scriban (1)
154-172: ID generation: strengthen slugging to avoid selector issues and collisionsThe current replace chain handles spaces, dots, and backticks only. Type identifiers can include other characters (e.g.,
<T>,+,[],,). Prefer a stable slug (or short hash) computed once in the view model and reused for:
- data-type-group-id
- the corresponding "-content" id
This will reduce risk of invalid IDs and simplify template code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/DotNetApiDiff/Commands/CompareCommand.cs(1 hunks)src/DotNetApiDiff/ExitCodes/ExitCodeManager.cs(2 hunks)src/DotNetApiDiff/Models/ApiComparison.cs(2 hunks)src/DotNetApiDiff/Reporting/HtmlTemplates/change-group.scriban(1 hunks)src/DotNetApiDiff/Reporting/HtmlTemplates/main-layout.scriban(2 hunks)src/DotNetApiDiff/Reporting/HtmlTemplates/scripts.js(5 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/DotNetApiDiff/ExitCodes/ExitCodeManager.cs
- src/DotNetApiDiff/Commands/CompareCommand.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/DotNetApiDiff/Reporting/HtmlTemplates/change-group.scriban
- src/DotNetApiDiff/Models/ApiComparison.cs
- src/DotNetApiDiff/Reporting/HtmlTemplates/scripts.js
🔇 Additional comments (1)
src/DotNetApiDiff/Reporting/HtmlTemplates/main-layout.scriban (1)
178-185: Ensure HTML-escaping of dynamic contentUser-controlled fields may include markup or malicious content. In
src/DotNetApiDiff/Reporting/HtmlTemplates/main-layout.scribanaround lines 178–185, wrap bothchange.element_nameandchange.descriptionin an HTML-escape filter:• At line ~178:
- <code>{{ change.element_name }}</code> + <code>{{ change.element_name | html.escape }}</code>• At line ~184:
- <div class="change-description">{{ change.description }}</div> + <div class="change-description">{{ change.description | html.escape }}</div>If Scriban doesn’t provide a built-in
html.escapefilter, please add one or pre-encode these values upstream before rendering.
| <div class="type-group" data-type-group-id="{{ section.change_type }}-{{ group.key | string.replace " " "-" | string.replace "." "-" | string.replace "`" "-" }}"> | ||
| <div class="type-header"> | ||
| <h3 class="type-name"> | ||
| 📁 {{ group.key }} | ||
| <span class="type-summary"> | ||
| ({{ group.count }} change{{ if group.count != 1 }}s{{ end }}{{ if group.has_breaking_changes }}, {{ group.breaking_changes_count }} breaking{{ end }}) | ||
| </span> | ||
| </h3> | ||
| <div class="type-header-controls"> | ||
| {{if group.has_breaking_changes}} | ||
| <span class="breaking-type-badge">⚠️ Breaking Changes</span> | ||
| {{end}} | ||
| <button class="type-toggle collapsed" onclick="toggleTypeGroup(this.closest('.type-group').dataset.typeGroupId)"> | ||
| <span class="toggle-icon">▶</span> | ||
| </button> | ||
| </div> | ||
| </div> | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Type-group toggle button lacks ARIA and type; add attributes and keep state in sync
Add type, aria-controls, aria-expanded, and an accessible label. Ensure the toggle script updates aria-expanded accordingly.
- <button class="type-toggle collapsed" onclick="toggleTypeGroup(this.closest('.type-group').dataset.typeGroupId)">
+ <button
+ class="type-toggle collapsed"
+ type="button"
+ aria-controls="{{ section.change_type }}-{{ group.key | string.replace " " "-" | string.replace "." "-" | string.replace "`" "-" }}-content"
+ aria-expanded="false"
+ aria-label="Toggle type group: {{ group.key }}"
+ onclick="toggleTypeGroup(this.closest('.type-group').dataset.typeGroupId)">
<span class="toggle-icon">▶</span>
</button>Additionally, consider computing a pre-slugged, safe group ID in the view model (or a helper) rather than ad-hoc replaces to avoid edge cases with characters like <, >, +, [, ], ,, : and to guarantee uniqueness.
Add ARIA attributes and explicit button type to the toggle control
To make the type-group toggle accessible and prevent unintended form submissions, the <button> needs an explicit type="button", aria-controls pointing at its content region, aria-expanded to reflect its state, and a descriptive aria-label. Ensure your toggleTypeGroup script updates aria-expanded when opening/closing.
Locations:
- File:
src/DotNetApiDiff/Reporting/HtmlTemplates/main-layout.scriban - Around lines 154–171, update the
<button class="type-toggle collapsed">element.
Suggested diff:
- <button class="type-toggle collapsed" onclick="toggleTypeGroup(this.closest('.type-group').dataset.typeGroupId)">
+ <button
+ class="type-toggle collapsed"
+ type="button"
+ aria-controls="{{ section.change_type }}-{{ group.key | string.replace " " "-" | string.replace "." "-" | string.replace "`" "-" }}-content"
+ aria-expanded="false"
+ aria-label="Toggle type group: {{ group.key }}"
+ onclick="toggleTypeGroup(this.closest('.type-group').dataset.typeGroupId)">
<span class="toggle-icon">▶</span>
</button>Additionally, consider generating a sanitized ID for each group in your view-model or via a helper (instead of ad-hoc string.replace chains) to guard against special characters (< > + [ ] , : etc.) and ensure global uniqueness.
📝 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.
| <div class="type-group" data-type-group-id="{{ section.change_type }}-{{ group.key | string.replace " " "-" | string.replace "." "-" | string.replace "`" "-" }}"> | |
| <div class="type-header"> | |
| <h3 class="type-name"> | |
| 📁 {{ group.key }} | |
| <span class="type-summary"> | |
| ({{ group.count }} change{{ if group.count != 1 }}s{{ end }}{{ if group.has_breaking_changes }}, {{ group.breaking_changes_count }} breaking{{ end }}) | |
| </span> | |
| </h3> | |
| <div class="type-header-controls"> | |
| {{if group.has_breaking_changes}} | |
| <span class="breaking-type-badge">⚠️ Breaking Changes</span> | |
| {{end}} | |
| <button class="type-toggle collapsed" onclick="toggleTypeGroup(this.closest('.type-group').dataset.typeGroupId)"> | |
| <span class="toggle-icon">▶</span> | |
| </button> | |
| </div> | |
| </div> | |
| <div class="type-header-controls"> | |
| {{if group.has_breaking_changes}} | |
| <span class="breaking-type-badge">⚠️ Breaking Changes</span> | |
| {{end}} | |
| <button | |
| class="type-toggle collapsed" | |
| type="button" | |
| aria-controls="{{ section.change_type }}-{{ group.key | string.replace " " "-" | string.replace "." "-" | string.replace "`" "-" }}-content" | |
| aria-expanded="false" | |
| aria-label="Toggle type group: {{ group.key }}" | |
| onclick="toggleTypeGroup(this.closest('.type-group').dataset.typeGroupId)"> | |
| <span class="toggle-icon">▶</span> | |
| </button> | |
| </div> |
🤖 Prompt for AI Agents
In src/DotNetApiDiff/Reporting/HtmlTemplates/main-layout.scriban around lines
154 to 171, the toggle button lacks accessibility attributes and an explicit
button type. Add type="button" to prevent form submission, include aria-controls
referencing the ID of the collapsible content, aria-expanded reflecting the
toggle state, and a descriptive aria-label for screen readers. Modify the
toggleTypeGroup script to update aria-expanded on toggle. Also, replace the
current string.replace chain for generating IDs with a more robust sanitization
method or helper to ensure valid, unique IDs free of problematic characters.
| <div class="type-changes collapsed" id="{{ section.change_type }}-{{ group.key | string.replace " " "-" | string.replace "." "-" | string.replace "`" "-" }}-content"> | ||
| <div class="category-changes"> | ||
| {{for change in group.changes}} | ||
| <div class="change-item {{ section.change_type }}"> | ||
| <div class="change-header"> | ||
| <div class="change-name"> | ||
| <span class="element-type">{{ change.element_type }}</span> | ||
| <code>{{ change.element_name }}</code> | ||
| {{if change.is_breaking_change}} | ||
| <span class="breaking-badge">BREAKING</span> | ||
| {{end}} | ||
| </div> | ||
| <div class="change-description">{{ change.description }}</div> | ||
| </div> | ||
| {{if change.has_signatures}} | ||
| <div class="signature-toggle"> | ||
| <button class="toggle-btn" onclick="toggleSignature('{{ change.details_id }}')"> | ||
| <span class="toggle-icon">▼</span> View Signature Details | ||
| </button> | ||
| </div> | ||
| <div id="{{ change.details_id }}" class="signature-details" style="display: none;"> | ||
| {{if change.old_signature}} | ||
| <div class="signature-section"> | ||
| <h5>Old Signature:</h5> | ||
| <pre><code class="csharp">{{ change.old_signature }}</code></pre> | ||
| </div> | ||
| {{end}} | ||
| {{if change.new_signature}} | ||
| <div class="signature-section"> | ||
| <h5>New Signature:</h5> | ||
| <pre><code class="csharp">{{ change.new_signature }}</code></pre> | ||
| </div> | ||
| {{end}} | ||
| </div> | ||
| {{end}} | ||
| </div> | ||
| {{end}} | ||
| </div> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Signature details toggle: missing ARIA on button; region should be associated; ensure signatures are HTML-escaped
- Add type and ARIA to the toggle button; keep aria-expanded in sync in toggleSignature.
- Ensure signature content is HTML-escaped to prevent markup injection within code blocks.
- <button class="toggle-btn" onclick="toggleSignature('{{ change.details_id }}')">
- <span class="toggle-icon">▼</span> View Signature Details
- </button>
+ <button
+ class="toggle-btn"
+ type="button"
+ aria-controls="{{ change.details_id }}"
+ aria-expanded="false"
+ aria-label="Toggle signature details for {{ change.element_name }}"
+ onclick="toggleSignature('{{ change.details_id }}')">
+ <span class="toggle-icon">▼</span> View Signature Details
+ </button>- <div id="{{ change.details_id }}" class="signature-details" style="display: none;">
+ <div id="{{ change.details_id }}" class="signature-details" style="display: none;">- <pre><code class="csharp">{{ change.old_signature }}</code></pre>
+ <pre><code class="csharp">{{ change.old_signature | html.escape }}</code></pre>- <pre><code class="csharp">{{ change.new_signature }}</code></pre>
+ <pre><code class="csharp">{{ change.new_signature | html.escape }}</code></pre>If html.escape isn’t available in your Scriban setup, please pre-encode these values in the view model before rendering.
🏁 Script executed:
#!/bin/bash
# Search for the toggleSignature implementation to verify ARIA-expanded handling
rg "toggleSignature" -n .
# Check whether an html.escape filter (or equivalent) is already available in Scriban templates
rg "html\.escape" -n .
rg "html\.encode" -n .Length of output: 695
🏁 Script executed:
#!/bin/bash
# Show the implementation of toggleSignature to verify ARIA-expanded toggling
sed -n '1,200p' src/DotNetApiDiff/Reporting/HtmlTemplates/scripts.jsLength of output: 6981
🌐 Web query:
Does Scriban provide a built-in html.escape or html.encode filter/function?
💡 Result:
Yes, Scriban provides a built-in html.escape function that escapes a string for safe inclusion in HTML. This function replaces special HTML characters with their corresponding HTML entities, ensuring that the string can be safely embedded within HTML content.
Usage:
{{ "<p>This is a paragraph</p>" | html.escape }}
Output:
<p>This is a paragraph</p>
In this example, the < and > characters are replaced with < and >, respectively, to prevent them from being interpreted as HTML tags.
For more details on Scriban's built-in functions, you can refer to the official documentation. (github.com)
Citations:
Enhance signature-toggle accessibility and escape signature content
To meet ARIA best practices and prevent markup injection, please:
- Update the toggle
<button>in all signature‐details templates (main-layout.scriban,change-group.scriban,type-group.scriban):- Add
type="button",aria-controls="{{ change.details_id }}",aria-expanded="false", and a descriptivearia-label.
- Add
- Escape user‐provided signature text using Scriban’s built-in
html.escapefilter on bothold_signatureandnew_signature. - In
src/DotNetApiDiff/Reporting/HtmlTemplates/scripts.js, keeparia-expandedin sync when you expand or collapse:- Set
button.setAttribute('aria-expanded', 'true')on expand and'false'on collapse.
- Set
Example template diff:
--- a/.../main-layout.scriban
+++ b/.../main-layout.scriban
@@ -185,7 +185,14 @@
-<button class="toggle-btn" onclick="toggleSignature('{{ change.details_id }}')">
- <span class="toggle-icon">▼</span> View Signature Details
-</button>
+<button
+ class="toggle-btn"
+ type="button"
+ aria-controls="{{ change.details_id }}"
+ aria-expanded="false"
+ aria-label="Toggle signature details for {{ change.element_name }}"
+ onclick="toggleSignature('{{ change.details_id }}')">
+ <span class="toggle-icon">▼</span> View Signature Details
+</button>
@@ -195,7 +202,7 @@
-<pre><code class="csharp">{{ change.old_signature }}</code></pre>
+<pre><code class="csharp">{{ change.old_signature | html.escape }}</code></pre>
@@ -200,7 +207,7 @@
-<pre><code class="csharp">{{ change.new_signature }}</code></pre>
+<pre><code class="csharp">{{ change.new_signature | html.escape }}</code></pre>Example scripts.js diff:
--- a/src/DotNetApiDiff/Reporting/HtmlTemplates/scripts.js
+++ b/src/DotNetApiDiff/Reporting/HtmlTemplates/scripts.js
function toggleSignature(detailsId) {
@@ -6,9 +6,13 @@
if (details.style.display === 'none') {
details.style.display = 'block';
button.classList.add('expanded');
+ button.setAttribute('aria-expanded', 'true');
button.innerHTML = '<span class="toggle-icon">▲</span> Hide Signature Details';
} else {
details.style.display = 'none';
button.classList.remove('expanded');
+ button.setAttribute('aria-expanded', 'false');
button.innerHTML = '<span class="toggle-icon">▼</span> View Signature Details';
}
}📝 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.
| <div class="type-changes collapsed" id="{{ section.change_type }}-{{ group.key | string.replace " " "-" | string.replace "." "-" | string.replace "`" "-" }}-content"> | |
| <div class="category-changes"> | |
| {{for change in group.changes}} | |
| <div class="change-item {{ section.change_type }}"> | |
| <div class="change-header"> | |
| <div class="change-name"> | |
| <span class="element-type">{{ change.element_type }}</span> | |
| <code>{{ change.element_name }}</code> | |
| {{if change.is_breaking_change}} | |
| <span class="breaking-badge">BREAKING</span> | |
| {{end}} | |
| </div> | |
| <div class="change-description">{{ change.description }}</div> | |
| </div> | |
| {{if change.has_signatures}} | |
| <div class="signature-toggle"> | |
| <button class="toggle-btn" onclick="toggleSignature('{{ change.details_id }}')"> | |
| <span class="toggle-icon">▼</span> View Signature Details | |
| </button> | |
| </div> | |
| <div id="{{ change.details_id }}" class="signature-details" style="display: none;"> | |
| {{if change.old_signature}} | |
| <div class="signature-section"> | |
| <h5>Old Signature:</h5> | |
| <pre><code class="csharp">{{ change.old_signature }}</code></pre> | |
| </div> | |
| {{end}} | |
| {{if change.new_signature}} | |
| <div class="signature-section"> | |
| <h5>New Signature:</h5> | |
| <pre><code class="csharp">{{ change.new_signature }}</code></pre> | |
| </div> | |
| {{end}} | |
| </div> | |
| {{end}} | |
| </div> | |
| {{end}} | |
| </div> | |
| </div> | |
| </div> | |
| <div class="type-changes collapsed" id="{{ section.change_type }}-{{ group.key | string.replace " " "-" | string.replace "." "-" | string.replace "`" "-" }}-content"> | |
| <div class="category-changes"> | |
| {{for change in group.changes}} | |
| <div class="change-item {{ section.change_type }}"> | |
| <div class="change-header"> | |
| <div class="change-name"> | |
| <span class="element-type">{{ change.element_type }}</span> | |
| <code>{{ change.element_name }}</code> | |
| {{if change.is_breaking_change}} | |
| <span class="breaking-badge">BREAKING</span> | |
| {{end}} | |
| </div> | |
| <div class="change-description">{{ change.description }}</div> | |
| </div> | |
| {{if change.has_signatures}} | |
| <div class="signature-toggle"> | |
| <button | |
| class="toggle-btn" | |
| type="button" | |
| aria-controls="{{ change.details_id }}" | |
| aria-expanded="false" | |
| aria-label="Toggle signature details for {{ change.element_name }}" | |
| onclick="toggleSignature('{{ change.details_id }}')"> | |
| <span class="toggle-icon">▼</span> View Signature Details | |
| </button> | |
| </div> | |
| <div id="{{ change.details_id }}" class="signature-details" style="display: none;"> | |
| {{if change.old_signature}} | |
| <div class="signature-section"> | |
| <h5>Old Signature:</h5> | |
| <pre><code class="csharp">{{ change.old_signature | html.escape }}</code></pre> | |
| </div> | |
| {{end}} | |
| {{if change.new_signature}} | |
| <div class="signature-section"> | |
| <h5>New Signature:</h5> | |
| <pre><code class="csharp">{{ change.new_signature | html.escape }}</code></pre> | |
| </div> | |
| {{end}} | |
| </div> | |
| {{end}} | |
| </div> | |
| {{end}} | |
| </div> | |
| </div> | |
| </div> |
🤖 Prompt for AI Agents
In src/DotNetApiDiff/Reporting/HtmlTemplates/main-layout.scriban lines 172 to
211, improve accessibility by adding type="button", aria-controls referencing
the signature details ID, aria-expanded="false", and a descriptive aria-label to
the toggle button for signature details. Also, apply the html.escape filter to
both old_signature and new_signature to prevent markup injection. Additionally,
in src/DotNetApiDiff/Reporting/HtmlTemplates/scripts.js, update the
toggleSignature function to set the button's aria-expanded attribute to "true"
when expanding and "false" when collapsing the signature details, keeping ARIA
state in sync with visibility.
Summary
This PR enhances the diagnostic capabilities of the
ApiComparisonclass by adding detailed validation error messages. Previously, when API comparisons failed validation, users only received a generic "API comparison is invalid" message with no indication of the specific problem.Changes Made
1. Added
InvalidMessageProperty toApiComparison2. Enhanced
IsValid()MethodInvalidMessageat start of each validation run3. Updated
ExitCodeManagerLoggingInvalidMessagein log output when availableInvalidMessageis emptyExample Error Messages
Before
After
Or:
Benefits
Testing
Files Changed
src/DotNetApiDiff/Models/ApiComparison.cs- Added InvalidMessage property and enhanced validationsrc/DotNetApiDiff/ExitCodes/ExitCodeManager.cs- Updated logging to include detailed error messagesRelated Issues
Resolves #51
Type of Change
This enhancement significantly improves the debugging experience when API comparisons fail validation, making it much easier to identify and resolve issues.
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Tests