From 25ad2d5146d3fb09607555b546fff31fa5c4b07d Mon Sep 17 00:00:00 2001 From: Jim Bethancourt Date: Fri, 25 Sep 2026 13:54:31 -0500 Subject: [PATCH 1/7] Fix menu links --- lib/report-view.js | 66 +++++++++++++++ tests/e2e/user-journeys.spec.js | 23 ++++++ tests/unit/report-view.test.js | 138 +++++++++++++++++++++++++++++++- 3 files changed, 226 insertions(+), 1 deletion(-) diff --git a/lib/report-view.js b/lib/report-view.js index 573680e..025b3d9 100644 --- a/lib/report-view.js +++ b/lib/report-view.js @@ -268,6 +268,66 @@ export function bindPopupHandlers(root) { }); } +// --------------------------------------------------------------------------- +// Section navigation ("Report sections" menu). The report is rendered +// asynchronously, so a fragment in the URL (#CLASSMAP, #GOD, ...) has no +// target when the browser tries to scroll at load time; and clicking a menu +// link whose fragment already matches the URL is a native no-op. Scrolling +// is therefore handled explicitly: a delegated listener scrolls the target +// into view and enhanceReport honours the initial fragment after rendering. +// --------------------------------------------------------------------------- + +function findSectionTarget(hash) { + if (!hash || hash === '#') return null; + return document.getElementById(hash.slice(1)); +} + +export function scrollToSectionHash(hash) { + const target = findSectionTarget(hash); + if (target) target.scrollIntoView(); +} + +// Native fragment navigation moves the sequential focus starting point to +// the target; mirror that so keyboard users continue from the section they +// jumped to instead of the menu. +function focusSectionTarget(target) { + if (!target.hasAttribute('tabindex') && !/^(A|AREA|BUTTON|INPUT|SELECT|TEXTAREA)$/.test(target.tagName)) { + target.setAttribute('tabindex', '-1'); + } + target.focus({ preventScroll: true }); +} + +export function bindSectionNavLinks(root) { + const scope = root || document; + // Delegate on the persistent container: enhanced-table interactions + // re-render the report with innerHTML, which would drop per-link + // listeners; one delegated listener survives those swaps. The guard + // keeps effect re-runs from stacking duplicate listeners. + const guard = scope === document ? scope.documentElement : scope; + if (guard.hasAttribute('data-rf-nav-bound')) return; + guard.setAttribute('data-rf-nav-bound', ''); + scope.addEventListener('click', event => { + // Only intercept unmodified primary-button clicks that target the + // current tab; Ctrl/Cmd/Shift clicks must keep their browser defaults + // (open in new tab/window etc.). + if (event.defaultPrevented || event.button !== 0 || + event.metaKey || event.ctrlKey || event.shiftKey || event.altKey) return; + const link = typeof event.target?.closest === 'function' + ? event.target.closest('a[href^="#"]') + : null; + if (!link || (scope !== document && !scope.contains(link))) return; + const href = link.getAttribute('href'); + const target = findSectionTarget(href); + if (!target) return; // leave unresolved fragments to default navigation + event.preventDefault(); + if (window.location.hash !== href) { + history.pushState(null, '', href); + } + target.scrollIntoView(); + focusSectionTarget(target); + }); +} + // --------------------------------------------------------------------------- // Chart.js bubble charts per disharmony. // --------------------------------------------------------------------------- @@ -493,6 +553,12 @@ export async function enhanceReport(root, data) { } bindPopupHandlers(root); + // Menu links: explicit scrolling because the report renders too late for + // the browser's own fragment navigation (deep links) and re-clicking the + // current fragment is otherwise a no-op. + bindSectionNavLinks(root); + scrollToSectionHash(window.location.hash); + // Reload the GitHub buttons script so dynamically injected buttons render. if (!document.querySelector('script[src*="buttons.github.io"]')) { const script = document.createElement('script'); diff --git a/tests/e2e/user-journeys.spec.js b/tests/e2e/user-journeys.spec.js index e29b223..e1d0451 100644 --- a/tests/e2e/user-journeys.spec.js +++ b/tests/e2e/user-journeys.spec.js @@ -118,6 +118,29 @@ test('report renders all sections from repository JSON with fallback template', await expect(page.locator('#publishDate')).toContainText('Last Published:'); }); +test('report menu links scroll the referenced section into view', async ({ page }) => { + await page.goto('/refactorfirst/refactorfirst'); + const section = page.getByRole('heading', { name: 'Code Disharmonies', level: 2 }); + await expect(section).toBeAttached(); + await expect(section).not.toBeInViewport(); + + await page.locator('.rf-report nav a[href="#DISHARMONIES"]').click(); + await expect(page).toHaveURL(/#DISHARMONIES$/); + await expect(section).toBeInViewport(); + + // Re-clicking the same link (URL fragment already current) still scrolls. + await page.evaluate(() => window.scrollTo(0, 0)); + await expect(section).not.toBeInViewport(); + await page.locator('.rf-report nav a[href="#DISHARMONIES"]').click(); + await expect(section).toBeInViewport(); +}); + +test('deep link with a section fragment scrolls the section into view on load', async ({ page }) => { + await page.goto('/refactorfirst/refactorfirst/#GOD'); + const section = page.getByRole('heading', { name: 'God Classes', level: 3 }); + await expect(section).toBeInViewport(); +}); + test('branch fallback shows report from master', async ({ page }) => { let requested = []; await page.unroute('**/raw.githubusercontent.com/**'); diff --git a/tests/unit/report-view.test.js b/tests/unit/report-view.test.js index 0affd99..6c1668c 100644 --- a/tests/unit/report-view.test.js +++ b/tests/unit/report-view.test.js @@ -1,4 +1,4 @@ -import { describe, it, expect, beforeEach } from 'bun:test'; +import { describe, it, expect, beforeEach, afterEach } from 'bun:test'; import { exposeGraphDots, initBubbleChart, @@ -7,6 +7,8 @@ import { showPopup, hidePopup, bindPopupHandlers, + bindSectionNavLinks, + scrollToSectionHash, enhanceReport, statefulElementIds, stashStatefulDom, @@ -252,6 +254,111 @@ describe('stateful DOM preservation across table-state re-renders', () => { }); }); +describe('report section menu links', () => { + const originalScrollIntoView = window.Element.prototype.scrollIntoView; + let scrolledTo; + + beforeEach(() => { + scrolledTo = []; + window.Element.prototype.scrollIntoView = function () { scrolledTo.push(this.id); }; + document.body.innerHTML = ` +
+ Outside the report root +
+ +

Class Map

+

God Classes

+
+
`; + history.replaceState(null, '', '/refactorfirst/refactorfirst/'); + }); + + afterEach(() => { + window.Element.prototype.scrollIntoView = originalScrollIntoView; + history.replaceState(null, '', '/'); + }); + + it('scrolls the referenced section into view and moves focus when a menu link is clicked', () => { + bindSectionNavLinks(document.getElementById('report-root')); + document.querySelector('a[href="#GOD"]').click(); + expect(scrolledTo).toEqual(['GOD']); + expect(window.location.hash).toBe('#GOD'); + // Keyboard users continue from the selected section (native fragment + // navigation moves the sequential focus starting point too). + const target = document.getElementById('GOD'); + expect(document.activeElement).toBe(target); + expect(target.getAttribute('tabindex')).toBe('-1'); + }); + + it('scrolls again when the link is clicked while its hash is already current', () => { + // Native fragment navigation is a no-op when the URL already has the + // fragment, so the handler must scroll explicitly every time. + history.replaceState(null, '', '/refactorfirst/refactorfirst/#GOD'); + bindSectionNavLinks(document.getElementById('report-root')); + document.querySelector('a[href="#GOD"]').click(); + expect(scrolledTo).toEqual(['GOD']); + }); + + it('survives report re-renders and binds only once', () => { + // Table interactions re-render the report with innerHTML; the delegated + // listener on the persistent container must keep working on fresh nodes. + const root = document.getElementById('report-root'); + bindSectionNavLinks(root); + bindSectionNavLinks(root); // re-render/effect re-run must not double-bind + root.innerHTML = ` + +

Class Map

`; + root.querySelector('a[href="#CLASSMAP"]').click(); + expect(scrolledTo).toEqual(['CLASSMAP']); + }); + + it('ignores clicks outside the report root', () => { + bindSectionNavLinks(document.getElementById('report-root')); + document.querySelector('a[href="#outside"]').click(); + expect(scrolledTo).toEqual([]); + }); + + it('leaves links pointing at missing sections to default navigation', () => { + bindSectionNavLinks(document.getElementById('report-root')); + const link = document.querySelector('a[href="#MISSING"]'); + const event = new MouseEvent('click', { bubbles: true, cancelable: true }); + link.dispatchEvent(event); + expect(event.defaultPrevented).toBe(false); + expect(scrolledTo).toEqual([]); + }); + + it('preserves modified clicks (Ctrl/Cmd/Shift) so new-tab gestures still work', () => { + bindSectionNavLinks(document.getElementById('report-root')); + const link = document.querySelector('a[href="#GOD"]'); + for (const mods of [{ ctrlKey: true }, { metaKey: true }, { shiftKey: true }]) { + const event = new MouseEvent('click', { bubbles: true, cancelable: true, ...mods }); + link.dispatchEvent(event); + expect(event.defaultPrevented).toBe(false); + } + expect(scrolledTo).toEqual([]); + expect(window.location.hash).toBe(''); + }); + + it('scrolls to the section referenced by the initial URL hash', () => { + // The report renders asynchronously, so the browser cannot honour the + // fragment at load time; enhanceReport must scroll once rendered. + history.replaceState(null, '', '/refactorfirst/refactorfirst/#CLASSMAP'); + scrollToSectionHash(window.location.hash); + expect(scrolledTo).toEqual(['CLASSMAP']); + }); + + it('does nothing for an empty, bare or unknown hash', () => { + scrollToSectionHash(''); + scrollToSectionHash('#'); + scrollToSectionHash('#NOPE'); + expect(scrolledTo).toEqual([]); + }); +}); + describe('enhanceReport', () => { it('exposes dots, wires popup globals and initializes charts', async () => { document.body.innerHTML = ` @@ -280,4 +387,33 @@ describe('enhanceReport', () => { expect(created.length).toBe(1); delete window.Chart; }); + + it('binds section menu links and scrolls to the initial URL hash', async () => { + const originalScrollIntoView = window.Element.prototype.scrollIntoView; + const scrolledTo = []; + window.Element.prototype.scrollIntoView = function () { scrolledTo.push(this.id); }; + try { + document.body.innerHTML = ` +
+
+ +

Class Map

+
+
`; + history.replaceState(null, '', '/refactorfirst/refactorfirst/#CLASSMAP'); + + await enhanceReport(document.getElementById('report-root'), { project: { name: 'Demo' } }); + expect(scrolledTo).toContain('CLASSMAP'); + + document.querySelector('a[href="#CLASSMAP"]').click(); + expect(scrolledTo.filter(id => id === 'CLASSMAP').length).toBe(2); + expect(window.location.hash).toBe('#CLASSMAP'); + expect(document.activeElement).toBe(document.getElementById('CLASSMAP')); + } finally { + window.Element.prototype.scrollIntoView = originalScrollIntoView; + history.replaceState(null, '', '/'); + } + }); }); From fd79f81161ef9855e4c5a330582e8cda8be5c95d Mon Sep 17 00:00:00 2001 From: Jim Bethancourt Date: Fri, 25 Sep 2026 14:23:16 -0500 Subject: [PATCH 2/7] Apply fix proposed by CodeRabbit to honor a deep-link fragment only once per report root --- lib/report-view.js | 9 ++++++++- tests/unit/report-view.test.js | 35 ++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/lib/report-view.js b/lib/report-view.js index 025b3d9..e612563 100644 --- a/lib/report-view.js +++ b/lib/report-view.js @@ -557,7 +557,14 @@ export async function enhanceReport(root, data) { // the browser's own fragment navigation (deep links) and re-clicking the // current fragment is otherwise a no-op. bindSectionNavLinks(root); - scrollToSectionHash(window.location.hash); + // Honor a deep-link fragment only on the first enhancement of this + // report root: later enhanceReport runs (retry, refetch, branch switch) + // must not yank the user's scroll position back to the fragment. + const navGuard = root && typeof root.hasAttribute === 'function' ? root : document.documentElement; + if (!navGuard.hasAttribute('data-rf-nav-scrolled')) { + navGuard.setAttribute('data-rf-nav-scrolled', ''); + scrollToSectionHash(window.location.hash); + } // Reload the GitHub buttons script so dynamically injected buttons render. if (!document.querySelector('script[src*="buttons.github.io"]')) { diff --git a/tests/unit/report-view.test.js b/tests/unit/report-view.test.js index 6c1668c..dba3d46 100644 --- a/tests/unit/report-view.test.js +++ b/tests/unit/report-view.test.js @@ -416,4 +416,39 @@ describe('enhanceReport', () => { history.replaceState(null, '', '/'); } }); + + it('scrolls to the initial URL hash only once per report root', async () => { + const originalScrollIntoView = window.Element.prototype.scrollIntoView; + const scrolledTo = []; + window.Element.prototype.scrollIntoView = function () { scrolledTo.push(this.id); }; + try { + document.body.innerHTML = ` +
+
+ +

Class Map

+
+
`; + history.replaceState(null, '', '/refactorfirst/refactorfirst/#CLASSMAP'); + const root = document.getElementById('report-root'); + + await enhanceReport(root, { project: { name: 'Demo' } }); + expect(scrolledTo).toEqual(['CLASSMAP']); + + // Later enhancements (retry, refetch, branch switch) must preserve + // the user's scroll position instead of jumping back to the fragment. + scrolledTo.length = 0; + await enhanceReport(root, { project: { name: 'Demo' } }); + expect(scrolledTo).toEqual([]); + + // ...while the menu links keep navigating to sections afterwards. + document.querySelector('a[href="#CLASSMAP"]').click(); + expect(scrolledTo).toEqual(['CLASSMAP']); + } finally { + window.Element.prototype.scrollIntoView = originalScrollIntoView; + history.replaceState(null, '', '/'); + } + }); }); From 9d59d30ba9dae08277bd3045a8d6d58a6f7b3ff1 Mon Sep 17 00:00:00 2001 From: Jim Bethancourt Date: Fri, 25 Sep 2026 14:30:04 -0500 Subject: [PATCH 3/7] Update sponsor information link to reflect new sponsor name --- assets/refactor-first-report.mustache | 4 ++-- public/assets/refactor-first-report.mustache | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/assets/refactor-first-report.mustache b/assets/refactor-first-report.mustache index 2d075d5..d30f4b8 100644 --- a/assets/refactor-first-report.mustache +++ b/assets/refactor-first-report.mustache @@ -336,8 +336,8 @@ Issue - Sponsor + Sponsor