feat: reorganize HTML report structure - implements issue #49 #50
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
WalkthroughThe changes overhaul the HTML report generation for API differences by grouping changes under their containing types rather than by change category. Supporting code, templates, JavaScript, and CSS are refactored or extended to implement collapsible, type-centric sections, update summary cards, and adjust test assertions to the new structure. Tests and templates are updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant HtmlFormatterScriban
participant Templates
participant Browser
User->>CLI: Run API diff with HTML output
CLI->>HtmlFormatterScriban: Generate HTML report
HtmlFormatterScriban->>HtmlFormatterScriban: GroupChangesByType(differences)
HtmlFormatterScriban->>Templates: Render grouped changes (per type)
Templates-->>CLI: HTML report (type-centric)
CLI->>User: Outputs HTML report
User->>Browser: Opens HTML report
Browser->>Browser: Renders collapsible type sections, badges, etc.
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related issues
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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: 1
🔭 Outside diff range comments (1)
src/DotNetApiDiff/Reporting/HtmlTemplates/change-group.scriban (1)
1-56: Consider consolidating duplicate template code.This template appears to duplicate much of the structure from
type-group.scriban. Both templates render type groups with collapsible sections and signature details.To improve maintainability:
- Consider using a single template with conditional logic
- Or extract common parts into shared partial templates
This would follow the DRY principle and make future updates easier to maintain.
🧹 Nitpick comments (3)
src/DotNetApiDiff/Reporting/HtmlTemplates/scripts.js (2)
175-213: Review the type group ID extraction approach.The
initializeTypeGroupsfunction uses regex to extract type group IDs from onclick attributes, which could be fragile if the HTML structure changes. Consider using data attributes instead for more robust element identification.Consider using data attributes for type group IDs instead of parsing onclick attributes:
// Instead of parsing onclick, use a data attribute - const onclickAttr = typeHeader.getAttribute('onclick'); - const match = onclickAttr?.match(/toggleTypeGroup\('([^']+)'\)/); - const typeGroupId = match ? match[1] : null; + const typeGroupId = typeHeader.getAttribute('data-type-group-id');Then update the HTML template to include
data-type-group-idattributes on type headers.
175-213: Consider using data attributes instead of parsing onclick.The function works correctly and follows the established pattern, but the regex extraction from onclick attributes is fragile and tightly coupled to HTML structure.
Consider refactoring to use data attributes for better maintainability:
// Instead of extracting from onclick: -const onclickAttr = typeHeader.getAttribute('onclick'); -const match = onclickAttr?.match(/toggleTypeGroup\('([^']+)'\)/); -const typeGroupId = match ? match[1] : null; // Use a data attribute: +const typeGroupId = typeGroup.getAttribute('data-type-group-id');This would require updating the HTML template to include
data-type-group-idattributes but would make the code more robust and easier to maintain.src/DotNetApiDiff/Reporting/HtmlFormatterScriban.cs (1)
235-254: Consider removing unused GroupChanges method.The original
GroupChangesmethod is no longer used after the migration to type-based grouping. Consider removing it to reduce code maintenance burden and eliminate confusion.Since
GroupChangesis no longer used, consider removing it:- private object[] GroupChanges(List<ApiDifference> changes) - { - return changes.GroupBy(c => c.ElementType) - .OrderBy(g => g.Key) - .Select(g => new - { - key = g.Key, - count = g.Count(), - changes = g.OrderBy(c => c.ElementName).Select(c => new - { - element_name = c.ElementName, - description = c.Description, - is_breaking_change = c.IsBreakingChange, - has_signatures = !string.IsNullOrEmpty(c.OldSignature) || !string.IsNullOrEmpty(c.NewSignature), - old_signature = HtmlEscape(c.OldSignature), - new_signature = HtmlEscape(c.NewSignature), - details_id = $"details-{Guid.NewGuid():N}" - }).ToArray() - }).ToArray(); - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
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 comments (32)
tests/DotNetApiDiff.Tests/Reporting/HtmlFormatterScribanTests.cs (3)
74-74: LGTM!Test assertion correctly updated to validate the new type-centric grouping with folder icon.
93-93: LGTM!Using fully qualified element names improves clarity by showing the containing type.
108-108: LGTM!Test correctly validates type-based grouping for removed items.
tests/DotNetApiDiff.Tests/Integration/CliWorkflowTests.cs (1)
141-142: Exit code standardization looks good, but consider documenting this breaking change.The change from exit code 2 to 1 for breaking changes aligns with standard CLI conventions where exit code 1 indicates general errors/failures. However, this is a breaking change for existing CI/CD pipelines that may be checking for exit code 2.
Consider:
- Documenting this change prominently in release notes
- Adding a migration guide for users who need to update their CI/CD scripts
Also applies to: 166-167, 201-202, 236-237, 367-368, 425-426, 448-449, 471-472, 539-540, 617-618
src/DotNetApiDiff/Reporting/HtmlTemplates/type-group.scriban (1)
1-13: LGTM!Type header section is well-structured with proper pluralization and clear visual indicators.
src/DotNetApiDiff/Reporting/HtmlTemplates/main-layout.scriban (3)
59-83: LGTM!Simplified summary cards with consistent clickability improve user experience.
155-156: Ensure collision-resistant ID generation for type groupsThe current sanitization in
src/DotNetApiDiff/Reporting/HtmlTemplates/main-layout.scriban:155-156
uses only.Replace(" ", "-"),.Replace(".", "-"), and.Replace("`", "-")to build IDs fromsection.change_typeandgroup.key. This can lead to:
- Collisions (e.g. “Namespace.Type” and “Namespace-Type” both → “Namespace-Type”)
- Broken IDs for other characters (
<,>,,,[], etc.)- No uniqueness guarantee across groups
Please review and verify:
- All possible
group.keyvalues (including generic names likeList<T>)- Whether there’s any existing utility for sanitizing or hashing these IDs
- The feasibility of introducing a shared helper (e.g. hash-based or collision-checked) to generate and track unique IDs
102-107: Potential double-click event issue.Having onclick handlers on both the parent div (line 102) and the child button (lines 104-106) could cause the toggle to fire twice when clicking the button.
Consider either:
- Remove the onclick from the button since the parent div handles it
- Add
event.stopPropagation()to the button's click handler- Remove the button element entirely and style the parent div as clickable
-<div class="section-header" onclick="toggleSection('breaking-changes')"> +<div class="section-header" onclick="toggleSection('breaking-changes')" role="button" tabindex="0"> <h2>⚠️ Breaking Changes</h2> - <button class="section-toggle collapsed"> - <span class="toggle-icon">▶</span> - </button> + <span class="section-toggle collapsed"> + <span class="toggle-icon">▶</span> + </span> </div>Likely an incorrect or invalid review comment.
src/DotNetApiDiff/Reporting/HtmlTemplates/scripts.js (6)
73-94: LGTM! Type group toggle functionality follows established patterns.The
toggleTypeGroupfunction correctly mirrors the existing section toggle pattern with proper DOM manipulation, CSS class management, and session storage persistence. The function handles edge cases by checking for null elements before proceeding.
96-111: LGTM! Session storage helpers are consistent.The
setTypeGroupStateandgetTypeGroupStatefunctions follow the same pattern as the existing section storage helpers, using appropriate key prefixes ("type-group-" vs "section-") to avoid conflicts and properly handling storage exceptions.
170-173: Good integration of type groups initialization.The addition of
initializeTypeGroups()call to the existinginitializeSections()function ensures type groups are properly initialized when the page loads.
73-94: LGTM! Type group toggle logic is well-implemented.The function correctly mirrors the section toggle pattern with proper state management and defensive programming. The early return when elements aren't found prevents runtime errors.
96-111: LGTM! Consistent session storage implementation.The type group storage helpers follow the same pattern as section storage with appropriate key prefixes and error handling.
170-173: LGTM! Proper integration of type group initialization.Adding the call to
initializeTypeGroups()ensures type groups are properly initialized on page load, following the same pattern as sections.src/DotNetApiDiff/Reporting/HtmlFormatterScriban.cs (9)
175-175: LGTM! Consistent migration to type-based grouping.All change sections (added, removed, modified, moved, excluded) have been consistently updated to use
GroupChangesByTypeinstead of the previousGroupChangesmethod, ensuring uniform type-centric organization across all change categories.Also applies to: 188-188, 201-201, 214-214, 228-228
256-283: Well-structured type-based grouping with comprehensive metadata.The
GroupChangesByTypemethod provides excellent organization and rich metadata:
- Groups changes by containing type with proper sorting
- Includes breaking change counts and flags for each type
- Provides both display names and full names for flexibility
- Orders changes within each type group logically
The data structure supports the enhanced UI requirements effectively.
285-312: Robust type extraction with proper edge case handling.The
ExtractContainingTypemethod correctly handles various scenarios:
- Type-level changes return the type name directly
- Member-level changes extract the containing type from the full name
- Nested types (with '+' separator) are preserved
- Malformed names fall back gracefully
The logic appears sound for typical .NET naming conventions.
314-333: Clean display name extraction methods.Both
GetTypeDisplayNameandGetMemberDisplayNamemethods properly extract simple names from fully qualified names by finding the last dot separator. The logic is straightforward and handles edge cases appropriately.
175-175: LGTM! Systematic migration to type-based grouping.All change sections consistently use the new
GroupChangesByTypemethod, ensuring uniform presentation across all change categories.Also applies to: 188-188, 201-201, 214-214, 228-228
256-283: LGTM! Well-structured type-based grouping implementation.The method correctly implements the core requirement of grouping changes by containing type. The rich metadata structure including breaking change counts and display names provides excellent data for the templates.
285-312: LGTM! Robust type extraction with good fallback handling.The method correctly handles the common cases for extracting containing types, including nested types and reasonable fallbacks for edge cases. The logic appropriately handles both type-level and member-level changes.
314-319: LGTM! Simple and effective type name extraction.The method correctly extracts the simple type name for display purposes with appropriate handling of types without namespaces.
321-333: LGTM! Consistent member name extraction logic.The method correctly handles both type-level and member-level changes, providing appropriate display names for each context.
src/DotNetApiDiff/Reporting/HtmlTemplates/styles.css (9)
66-72: Good enhancement to section header interactivity.The addition of cursor pointer and hover effects to section headers improves user experience by providing clear visual feedback for interactive elements. The transition effect is smooth and professional.
672-691: Well-designed type section styling.The type section styles provide excellent visual hierarchy with:
- Gradient background for headers creating depth
- Consistent left border using brand color (#007bff)
- Proper typography and spacing for type summaries
- Clear visual distinction for breaking change indicators
693-791: Comprehensive and consistent type group styling.The type group styles effectively support the new functionality:
- Proper interactive states with hover effects and transitions
- Consistent toggle button styling matching section toggles
- Smooth collapse/expand animations using CSS transitions
- Appropriate padding and spacing for readability
The CSS follows established patterns from the existing section styling.
793-862: Excellent visual categorization and change item styling.The change category and item styles provide clear visual organization:
- Distinct background colors for each change type (added: green tint, removed: red tint, etc.)
- Consistent border-left color coding matching the summary cards
- Proper spacing and typography hierarchy
- Clean element type badges with appropriate sizing
The color scheme aligns well with the existing design system.
743-749: CSS & JS Toggle Icon Consistency ConfirmedI’ve verified that
scripts.jsalways sets the icon to ▶ andstyles.cssappliesrotate(0deg)when collapsed androtate(90deg)when expanded for both.section-toggleand.type-toggle. The visual state changes purely via CSS rotation, so everything is aligned and no code changes are needed.
66-72: LGTM! Good UX enhancement for interactive headers.The cursor pointer and hover transition clearly indicate the interactive nature of section headers, improving user experience.
672-692: LGTM! Well-designed foundation for type-centric layout.The type-section styles establish clear visual hierarchy with appropriate spacing, gradients, and color accents that align with the existing design system.
693-772: LGTM! Comprehensive and accessible type group styling.The styling provides excellent visual hierarchy, interactive feedback, and accessibility considerations. The consistency with existing toggle patterns and proper focus handling demonstrates attention to both design and UX principles.
773-862: LGTM! Complete and well-organized change item styling.The styling provides excellent organization for changes within type groups, with appropriate color coding, smooth transitions, and clear visual hierarchy. The element type badges add helpful context while maintaining clean design.
| <button class="toggle-btn" onclick="toggleSignature('{{ change.details_id }}')"> | ||
| <span class="toggle-icon">▼</span> View Signature Details |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure proper HTML escaping and improve accessibility.
Two concerns to address:
-
XSS Prevention: Ensure that
change.details_id,change.old_signature, andchange.new_signatureare properly HTML-escaped in the Scriban template engine to prevent XSS attacks. -
Accessibility: The toggle button should include ARIA attributes for screen readers.
Consider updating the toggle button:
-<button class="toggle-btn" onclick="toggleSignature('{{ change.details_id }}')">
+<button class="toggle-btn" onclick="toggleSignature('{{ change.details_id }}')" aria-expanded="false" aria-controls="{{ change.details_id }}">Also applies to: 43-43, 49-49
🤖 Prompt for AI Agents
In src/DotNetApiDiff/Reporting/HtmlTemplates/type-group.scriban around lines
35-36, 43, and 49, ensure that the variables change.details_id,
change.old_signature, and change.new_signature are properly HTML-escaped using
Scriban's escape functions to prevent XSS vulnerabilities. Additionally, enhance
the toggle button's accessibility by adding appropriate ARIA attributes such as
aria-expanded and aria-controls to indicate the toggle state and the controlled
element for screen readers.
|
This was included as part of PR #51 |
Overview
This PR implements issue #49 by reorganizing the HTML report structure from type-first to change-category-first organization with collapsible type groups.
Changes Made
🔧 Core Implementation
PrepareChangeSectionsto useGroupChangesByTypemethod for hierarchical organizationtoggleTypeGroupfunctionality with session storage persistence🎨 User Experience Improvements
🧪 Testing & Quality
Before & After
Before: Types were primary groupings with change types as sub-sections
After: Change categories are primary with collapsible type groups within each category
Technical Details
Testing
Resolves #49
Summary by CodeRabbit
New Features
Style
Bug Fixes
Tests