fix: Refactor inspector.js into ES modules#161
Conversation
There was a problem hiding this comment.
Pull request overview
This PR modularizes the MageForge Inspector frontend by refactoring the monolithic inspector.js into ES module mixins, while adding/expanding UI behavior (tabs, draggable badge/connector) and accessibility/performance inspection features. It also updates the template to load the inspector as an ES module and adjusts Alpine.js CDN fallback timing to reduce Hyvä/Alpine conflicts.
Changes:
- Split Inspector logic into dedicated ES modules (DOM block detection, UI rendering, picker/shortcuts, tabs, a11y analysis, performance/vitals, draggable connector).
- Switched Inspector script loading to
type="module"and refined Alpine.js CDN fallback timing. - Added richer Accessibility/Cache/Core Web Vitals panels and draggable pinned badge UX.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/view/frontend/web/js/inspector.js | Converts the Alpine component to a small core that mixes in module method groups via object spread. |
| src/view/frontend/templates/inspector.phtml | Updates Inspector loading to ES module and refines Alpine CDN fallback scheduling. |
| src/view/frontend/web/js/inspector/accessibility.js | Accessibility tab rendering + accessible-name/role/focusability analysis utilities. |
| src/view/frontend/web/js/inspector/dom.js | DOM traversal and comment-marker parsing to map elements to MageForge block metadata. |
| src/view/frontend/web/js/inspector/draggable.js | Draggable pinned badge behavior plus SVG connector drawing logic. |
| src/view/frontend/web/js/inspector/performance.js | Cache tab rendering and element-scoped browser/perf metrics + image/resource analysis. |
| src/view/frontend/web/js/inspector/picker.js | Keyboard shortcuts, inspector toggle, hover/click picking, pin/unpin behavior. |
| src/view/frontend/web/js/inspector/tabs.js | Tab system construction and Structure tab rendering flow. |
| src/view/frontend/web/js/inspector/ui.js | DOM overlay creation, badge positioning, info-section creation, and footer rendering. |
| src/view/frontend/web/js/inspector/vitals.js | Web Vitals observers + performance/resource/DOM complexity helper utilities. |
| // Ctrl+Shift+I or Cmd+Option+I | ||
| if ((e.ctrlKey || e.metaKey) && e.shiftKey && e.key === 'I') { | ||
| e.preventDefault(); | ||
| this.toggleInspector(); | ||
| } | ||
|
|
||
| // ESC to close | ||
| if (e.key === 'Escape' && this.isOpen) { |
There was a problem hiding this comment.
Keyboard shortcut handling doesn’t match the comment (“Cmd+Option+I”) and is currently gated on shiftKey + uppercase 'I'. This prevents Cmd+Option+I on macOS and can be brittle across keyboard layouts/case. Update the condition to support (metaKey && altKey) and normalize e.key (e.g., case-insensitive) so the documented shortcuts actually work.
| // Ctrl+Shift+I or Cmd+Option+I | |
| if ((e.ctrlKey || e.metaKey) && e.shiftKey && e.key === 'I') { | |
| e.preventDefault(); | |
| this.toggleInspector(); | |
| } | |
| // ESC to close | |
| if (e.key === 'Escape' && this.isOpen) { | |
| const key = String(e.key).toLowerCase(); | |
| // Ctrl+Shift+I or Cmd+Option+I | |
| if ( | |
| key === 'i' && | |
| ((e.ctrlKey && e.shiftKey) || (e.metaKey && e.altKey)) | |
| ) { | |
| e.preventDefault(); | |
| this.toggleInspector(); | |
| } | |
| // ESC to close | |
| if (key === 'escape' && this.isOpen) { |
| // Check if click is outside badge | ||
| if (!this.infoBadge.contains(e.target) && !this.floatingButton.contains(e.target)) { |
There was a problem hiding this comment.
this.floatingButton.contains(e.target) will throw if floatingButton is null (e.g., if creation failed or the component is partially torn down). Guard this check with a null-safe condition before calling .contains() to avoid runtime errors when handling click-outside while pinned.
| // Check if click is outside badge | |
| if (!this.infoBadge.contains(e.target) && !this.floatingButton.contains(e.target)) { | |
| // Check if click is outside badge and floating button | |
| if ( | |
| !this.infoBadge.contains(e.target) && | |
| (!this.floatingButton || | |
| !this.floatingButton.contains(e.target)) | |
| ) { |
| const lcpObserver = new PerformanceObserver((list) => { | ||
| const entries = list.getEntries(); | ||
| const lastEntry = entries[entries.length - 1]; | ||
| this.webVitals.lcp = { | ||
| element: lastEntry.element, | ||
| value: lastEntry.renderTime || lastEntry.loadTime, | ||
| time: lastEntry.startTime | ||
| }; |
There was a problem hiding this comment.
The LCP PerformanceObserver callback assumes list.getEntries() is non-empty; entries[entries.length - 1] will be undefined if the observer fires with an empty list, causing an exception when accessing .element. Add a guard for entries.length === 0 before reading lastEntry.
| const ariaLabelledBy = element.getAttribute('aria-labelledby'); | ||
| if (ariaLabelledBy) { | ||
| const labelElement = document.getElementById(ariaLabelledBy); | ||
| return labelElement ? labelElement.textContent.trim() : ariaLabelledBy; | ||
| } |
There was a problem hiding this comment.
aria-labelledby can legally contain multiple IDs separated by whitespace. Using getElementById(ariaLabelledBy) on the full string will fail in that case. Consider splitting the attribute value on whitespace, resolving each ID, and concatenating the referenced text to derive a correct accessible name.
| renderRenderTimeMetric(container, element) { | ||
| const blockData = this.getBlockMetaData(element); | ||
| if (blockData && blockData.performance) { | ||
| const renderTime = parseFloat(blockData.performance.renderTime); |
There was a problem hiding this comment.
parseFloat(blockData.performance.renderTime) can mis-parse values formatted with thousands separators (e.g. PHP number_format yields "1,234.56", and parseFloat returns 1). Sanitize the string (remove commas) or store/consume this metric as a numeric value to ensure correct thresholds/colors.
| const renderTime = parseFloat(blockData.performance.renderTime); | |
| const rawRenderTime = String(blockData.performance.renderTime); | |
| const normalizedRenderTime = rawRenderTime.replace(/,/g, ''); | |
| const renderTime = Number.parseFloat(normalizedRenderTime); |
| const textSpan = document.createElement('span'); | ||
| textSpan.className = 'mageforge-info-value'; | ||
| textSpan.textContent = text; | ||
| textSpan.title = 'Click to copy'; | ||
|
|
There was a problem hiding this comment.
The copy-to-clipboard control is implemented as a <span> with an onclick handler. As-is it’s not keyboard focusable/operable, which is an accessibility issue for this UI feature. Use a semantic <button> (or add tabindex="0", role="button", and key handlers for Enter/Space) so keyboard users can trigger copy.
| while (node && node !== endComment) { | ||
| if (node.nodeType === Node.ELEMENT_NODE) { | ||
| elements.push(node); | ||
| // Also add all descendants | ||
| elements.push(...node.querySelectorAll('*')); | ||
| } |
There was a problem hiding this comment.
getElementsBetweenComments pushes each sibling element plus all descendants via querySelectorAll('*'), which can balloon block.elements and duplicate nodes across blocks. This can become very expensive on large DOMs. Consider a more efficient containment check (e.g., using Range comparisons) or building a WeakMap<Element, Block> once during parsing instead of storing every descendant in arrays.
| // Cache blocks for performance | ||
| if (!this.cachedBlocks || Date.now() - this.lastBlocksCacheTime > 1000) { | ||
| this.cachedBlocks = this.findAllMageForgeBlocks(); | ||
| this.lastBlocksCacheTime = Date.now(); | ||
| } | ||
|
|
||
| let closestBlock = null; | ||
| let closestDepth = -1; | ||
|
|
||
| // Find the deepest (most specific) block containing this element | ||
| for (const block of this.cachedBlocks) { | ||
| if (block.elements.includes(element)) { | ||
| // Calculate depth (how many ancestors between element and body) | ||
| let depth = 0; | ||
| let node = element; | ||
| while (node && node !== document.body) { | ||
| depth++; | ||
| node = node.parentElement; | ||
| } | ||
|
|
||
| if (depth > closestDepth) { | ||
| closestBlock = block; | ||
| closestDepth = depth; | ||
| } | ||
| } |
There was a problem hiding this comment.
findBlockForElement does a linear scan of every cached block and calls block.elements.includes(element) (also linear) while recomputing DOM depth inside the loop. This is O(blocks * elementsPerBlock) per lookup and will be noticeable while hovering. Consider precomputing a WeakMap from element->deepest block when caching blocks, and compute element depth once outside the loop (or track nesting depth during parsing).
| const tabContainer = document.createElement('div'); | ||
| tabContainer.className = 'mageforge-tabs-container'; | ||
|
|
||
| // Tab header | ||
| const tabHeader = document.createElement('div'); | ||
| tabHeader.className = 'mageforge-tabs-header'; | ||
|
|
||
| // Define tabs | ||
| const tabs = [ | ||
| { id: 'structure', label: 'Structure', icon: '🏰' }, | ||
| { id: 'accessibility', label: 'Accessibility', icon: '♿' }, | ||
| { id: 'performance', label: 'Cache', icon: '💾' }, | ||
| { id: 'core-web-vitals', label: 'Core Web Vitals', icon: '🌐' } | ||
| ]; | ||
|
|
||
| // Tab content container | ||
| const tabContentContainer = document.createElement('div'); | ||
|
|
||
| // Create tab buttons | ||
| tabs.forEach(tab => { | ||
| const button = document.createElement('button'); | ||
| button.type = 'button'; | ||
| button.className = 'mageforge-tab-button' + (this.activeTab === tab.id ? ' active' : ''); | ||
|
|
||
| // Label text | ||
| const textSpan = document.createElement('span'); | ||
| textSpan.textContent = tab.label; | ||
| button.appendChild(textSpan); | ||
|
|
||
| // Show "New" badge for Performance and Core Web Vitals if seen < 5 times | ||
| if (['performance', 'core-web-vitals'].includes(tab.id) && | ||
| (this.featureViews[tab.id] || 0) < this.MAX_NEW_BADGE_VIEWS) { | ||
| const badge = document.createElement('span'); | ||
| badge.className = 'mageforge-badge-new'; | ||
| badge.textContent = 'NEW'; | ||
| button.appendChild(badge); | ||
| } | ||
|
|
||
| button.onclick = (e) => { | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| this.switchTab(tab.id, data, element); | ||
| }; | ||
|
|
||
| tabHeader.appendChild(button); | ||
| }); |
There was a problem hiding this comment.
The tab UI is built from plain <div> + <button> elements without tablist semantics (role="tablist"/"tab"), aria-selected, aria-controls, or keyboard navigation (ArrowLeft/ArrowRight). For accessibility, add proper ARIA roles/attributes and keyboard handling so the tab interface is navigable via keyboard and announced correctly by screen readers.
This pull request introduces significant enhancements to the MageForge Inspector frontend, focusing on modularizing JavaScript logic for maintainability and adding new accessibility and UI features. The main changes include extracting major functionality into separate modules for accessibility analysis, DOM traversal, and draggable UI components, as well as improving Alpine.js integration and script loading behavior.
Frontend JavaScript modularization and new features:
accessibility.jsmodule to encapsulate accessibility analysis and rendering logic for the Inspector's Accessibility tab, providing detailed checks for ARIA attributes, element roles, focusability, and more.dom.jsmodule to handle DOM traversal and MageForge block detection, including parsing comment markers, finding block regions, and efficiently mapping DOM elements to their respective blocks.draggable.jsmodule to implement draggable functionality for the Inspector badge, including SVG connector drawing between the badge and the selected element, and robust event handling for drag operations.Improvements to script loading and Alpine.js integration:
inspector.phtmlto defer loading until all deferred and module scripts have executed, reducing conflicts with Hyvä themes and improving compatibility. [1] [2]defertotype="module"for better ES module support and modern browser compatibility.