U/sgriffin/rules2 #1657
U/sgriffin/rules2 #1657
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request introduces a comprehensive rules-based diagnostics system for email header analysis, featuring a unified styling framework and violation detection engine. The changes enable consistent display of rule violations across classic desktop, new desktop, and mobile interfaces.
Key Changes:
- Implements a rules engine with local JSON-based rule definitions for email header validation
- Adds shared CSS styling system (rulesCommon.css) for violation badges, cards, and highlights across all UIs
- Integrates violation display components with inline badges, cards, and accordion-based diagnostics sections
Reviewed changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| webpack.config.js | Adds rules.json to copy patterns and watch paths for hot reload |
| src/data/rules.json | Defines validation rules for email headers (SPF, DKIM, spam filters) |
| src/Scripts/rules/* | Implements rules engine with types, loaders, validation logic, and service |
| src/Scripts/ui/ViolationUI.ts | Creates UI components for displaying violations as badges and cards |
| src/Scripts/ui/*.ts | Integrates violation display into existing UI frames |
| src/Scripts/HeaderModel.ts | Adds rules analysis integration to header processing |
| src/Content/rulesCommon.css | Defines shared violation styling with severity-based theming |
| src/Content/themeColors.css | Adds diagnostic color variables for violation states |
| src/Content/*Frame.css | Implements UI-specific violation display styles |
| src/Pages/*.html | Adds HTML templates for violation badges, cards, and accordions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@stephenegriffin I've opened a new pull request, #1658, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 37 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 37 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export function highlightContent(content: string, violationGroups: ViolationGroup[]): string { | ||
| if (!content || !violationGroups || violationGroups.length === 0) { | ||
| return content; | ||
| } | ||
|
|
||
| interface Match { | ||
| start: number; | ||
| end: number; | ||
| text: string; | ||
| } | ||
|
|
||
| // Collect all matches first without modifying content | ||
| const allMatches: Match[] = []; | ||
|
|
||
| violationGroups.forEach(group => { | ||
| group.violations.forEach(violation => { | ||
| if (violation.highlightPattern) { | ||
| const patterns = violation.highlightPattern.split("|"); | ||
|
|
||
| patterns.forEach(pattern => { | ||
| if (pattern && pattern.trim()) { | ||
| const escapedPattern = pattern.trim().replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); | ||
|
|
||
| try { | ||
| const regex = new RegExp(escapedPattern, "gi"); | ||
| let match; | ||
| while ((match = regex.exec(content)) !== null) { | ||
| allMatches.push({ | ||
| start: match.index, | ||
| end: match.index + match[0].length, | ||
| text: match[0] | ||
| }); | ||
| } | ||
| } catch (error) { | ||
| console.warn("Invalid regex pattern:", pattern, error); | ||
| } | ||
| } | ||
| }); | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| if (allMatches.length === 0) { | ||
| return content; | ||
| } | ||
|
|
||
| // Sort by position and merge overlapping matches | ||
| allMatches.sort((a, b) => a.start - b.start); | ||
|
|
||
| const mergedMatches: Match[] = []; | ||
| let current = allMatches[0]!; | ||
|
|
||
| for (let i = 1; i < allMatches.length; i++) { | ||
| const next = allMatches[i]!; | ||
|
|
||
| if (next.start < current.end) { | ||
| // Overlapping - extend current if needed | ||
| if (next.end > current.end) { | ||
| current = { | ||
| start: current.start, | ||
| end: next.end, | ||
| text: content.slice(current.start, next.end) | ||
| }; | ||
| } | ||
| } else { | ||
| // Non-overlapping - save current and move to next | ||
| mergedMatches.push(current); | ||
| current = next; | ||
| } | ||
| } | ||
| mergedMatches.push(current); | ||
|
|
||
| // Build result by inserting spans from end to start (preserves positions) | ||
| let result = content; | ||
| for (let i = mergedMatches.length - 1; i >= 0; i--) { | ||
| const match = mergedMatches[i]!; | ||
| result = | ||
| result.slice(0, match.start) + | ||
| `<span class="highlight-violation">${match.text}</span>` + | ||
| result.slice(match.end); | ||
| } | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
The function highlightContent in ViolationUtils.ts has a critical bug that creates nested/broken HTML when patterns match content inside previously-inserted HTML tags. The function collects all matches from the original content, but then inserts highlight spans from end to start. If a second pattern matches text that appears in the first pattern's inserted HTML (like "class" matching inside <span class="highlight-violation">), it will corrupt the HTML structure.
The test at line 237 documents this issue but the implementation doesn't prevent it. The function needs to either:
- Track inserted spans and skip matches within them, or
- Apply all matches in a single pass before any HTML insertion, or
- Work on a DOM tree instead of string manipulation
This pull request introduces a unified and accessible styling system for rule violation diagnostics across all UI variants (classic desktop, new desktop, and mobile). It adds a new shared CSS file for rule violation styles, updates color variables for diagnostics, and implements new HTML templates and UI elements for displaying diagnostics in a consistent and user-friendly way. The changes also include accessibility improvements and specific layout/styling enhancements for diagnostics dialogs and popovers.
Core theme: Shared diagnostics styling and UI components
rulesCommon.csswith common styles for rule violation badges, cards, and details, and imported it into all major UI CSS files for consistency. [1] [2] [3] [4]themeColors.csswith new color variables for warning, info, and error states, as well as highlight backgrounds for diagnostics.UI/UX improvements for diagnostics display
classicDesktopFrame.html,newDesktopFrame.html,mha.html). Also added dedicated diagnostics content containers to the layouts. [1] [2] [3] [4] [5] [6] [7]Accessibility and high contrast support
Diagnostics dialog and popover enhancements