From a280615a2cd6b62526bdaa07a9bf582d4dad7881 Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Tue, 28 Jul 2026 16:11:57 -0400 Subject: [PATCH 1/2] visitAllLinks: crawl with production-faithful navigation, linearly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three fixes, found crawling a 265-page documentation app: 1. Relative hrefs: the click navigates by element.href, which the browser resolves against the TEST PAGE's url (e.g. /tests), not the app's current route the way a production visit would (there, the address bar is the current route). Verified: '../data-schema/index.md' from /schema/getting-started/index.md sent the router to /data-schema/index.md instead of /schema/data-schema/index.md. The crawler already resolves the target against currentURL() when it encounters the link — now it points the anchor at that resolved target before clicking, so the click exercises the real properLinks-and-router path with the production URL. 2. The navigation assertion compared against the link's #hash, which currentURL() never includes — page#hash links always read as failed navigations. Compare without the hash. 3. 'visited' was keyed on (current page, target) pairs, so every target was re-visited from every page that links it — shared nav links appear on every page, making the crawl quadratic in app size (~13k visits for a 265-page app; times out). Key on the target alone: one visit per reachable URL. Co-Authored-By: Claude Fable 5 --- test-app/app/router.ts | 5 +++ test-app/app/templates/application.gts | 4 +++ test-app/app/templates/docs/other.gts | 3 ++ test-app/app/templates/docs/page.gts | 10 ++++++ test-app/app/templates/hash-target.gts | 5 +++ .../tests/acceptance/visit-all-links-test.ts | 35 +++++++++++++++++++ test-support/src/routing/visit-all.ts | 29 ++++++++++++--- 7 files changed, 87 insertions(+), 4 deletions(-) create mode 100644 test-app/app/templates/docs/other.gts create mode 100644 test-app/app/templates/docs/page.gts create mode 100644 test-app/app/templates/hash-target.gts diff --git a/test-app/app/router.ts b/test-app/app/router.ts index a5eeddf..4ced252 100644 --- a/test-app/app/router.ts +++ b/test-app/app/router.ts @@ -12,4 +12,9 @@ export default class Router extends EmberRouter { Router.map(function () { this.route('foo'); + this.route('hash-target'); + this.route('docs', function () { + this.route('page'); + this.route('other'); + }); }); diff --git a/test-app/app/templates/application.gts b/test-app/app/templates/application.gts index a8daeb3..67b4bc2 100644 --- a/test-app/app/templates/application.gts +++ b/test-app/app/templates/application.gts @@ -7,6 +7,10 @@ here here / here + a nested page (with a relative link on it) + {{! a page+hash link: currentURL() after the click has no #hash, so the + navigation assertion must compare without it }} + hash link {{! non-SPA links: handled by the browser, not the router — visitAllLinks must skip them }} new tab diff --git a/test-app/app/templates/docs/other.gts b/test-app/app/templates/docs/other.gts new file mode 100644 index 0000000..ce4536b --- /dev/null +++ b/test-app/app/templates/docs/other.gts @@ -0,0 +1,3 @@ + diff --git a/test-app/app/templates/docs/page.gts b/test-app/app/templates/docs/page.gts new file mode 100644 index 0000000..d20ce6c --- /dev/null +++ b/test-app/app/templates/docs/page.gts @@ -0,0 +1,10 @@ + diff --git a/test-app/app/templates/hash-target.gts b/test-app/app/templates/hash-target.gts new file mode 100644 index 0000000..b94e1ed --- /dev/null +++ b/test-app/app/templates/hash-target.gts @@ -0,0 +1,5 @@ + diff --git a/test-app/tests/acceptance/visit-all-links-test.ts b/test-app/tests/acceptance/visit-all-links-test.ts index 1bfa216..d79c1c2 100644 --- a/test-app/tests/acceptance/visit-all-links-test.ts +++ b/test-app/tests/acceptance/visit-all-links-test.ts @@ -39,4 +39,39 @@ module('All Links', function (hooks) { assert.ok(visited.length > 0, 'the SPA links were still visited'); }); + + test('relative links navigate against the current route', async function (assert) { + // /docs/page renders ``, which from /docs/page must land + // on /docs/other. The browser resolves the anchor's element.href against + // the TEST PAGE's url (/tests) — which would send the router to /other — + // so the crawler points the anchor at the currentURL-resolved target + // before clicking. The navigation assertions inside visitAllLinks fail + // this test if the click lands anywhere else. + const visited: string[] = []; + + await visitAllLinks((url) => { + visited.push(url); + }); + + assert.true(visited.includes('/docs/other'), `visited: ${visited.join(', ')}`); + }); + + test('each target is visited once', async function (assert) { + // `visited` is keyed on the target path alone (not (page, target) pairs): + // shared links — like this app's application-template nav — appear on + // every page, and pair-keying makes the crawl quadratic in app size. + const visited: string[] = []; + + await visitAllLinks((url) => { + visited.push(url); + }); + + const paths = visited.map((url) => url.split('#')[0]); + + assert.strictEqual( + new Set(paths).size, + paths.length, + `no target is visited twice: ${paths.join(', ')}`, + ); + }); }); diff --git a/test-support/src/routing/visit-all.ts b/test-support/src/routing/visit-all.ts index 2a1febd..cb6f1eb 100644 --- a/test-support/src/routing/visit-all.ts +++ b/test-support/src/routing/visit-all.ts @@ -69,7 +69,7 @@ export async function visitAllLinks( knownRedirects?: Record, ) { /** - * string of "on::target" + * app-relative target paths (without hash) */ const visited = new Set(); let returnTo = '/'; @@ -110,7 +110,12 @@ export async function visitAllLinks( continue; } - const key = `${currentURL()}::${nonHashPart}`; + // Keyed on the target alone: this crawl answers "is every reachable URL + // visitable?", so one visit per target suffices. Keying on + // (current page, target) pairs re-visits every target from every page + // that links it — quadratic in the size of the app (shared nav links + // appear on every page), which times out on documentation-sized apps. + const key = nonHashPart; if (visited.has(key)) continue; @@ -130,16 +135,32 @@ export async function visitAllLinks( debugAssert(`link exists via selector \`${toVisit.selector}\``, link); + /** + * The click navigates by `element.href`, which the browser resolved + * against the test page's URL (e.g. `/tests`) — NOT against the app's + * current route the way a production visit would (there, the address bar + * is the current route). A relative href would therefore navigate + * somewhere the real app never goes. We already resolved the target + * against `currentURL()` when the link was encountered, so point the + * anchor at that; the click then exercises the real + * properLinks-and-router path with the production URL. + */ + if (!toVisit.original.startsWith('/')) { + link.setAttribute('href', toVisit.href); + } + await click(link); const current = rootURL.replace(/\/$/, '') + '/' + currentURL().replace(/^\//, ''); const expected = knownRedirects?.[toVisit.href] ?? toVisit.href; + // currentURL() never includes a #hash, so compare without it + const [expectedPath] = expected.split('#'); assert.pushResult({ - result: current.startsWith(expected), + result: current.startsWith(expectedPath ?? expected), actual: current, - expected: expected, + expected: expectedPath, message: `Navigation was successful: to:${toVisit.original}, from:${returnTo}`, }); visited.add(key); From 315f8e462b9c875bf4cf4bbf0472a8a98efe49ac Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Tue, 28 Jul 2026 16:15:57 -0400 Subject: [PATCH 2/2] Warn (with an action item) when the crawler rewrites a relative href Pointing the click at the resolved target keeps the crawl faithful, but a relative href is an authoring hazard the app author should fix: relative hrefs resolve against the browser's URL rather than the app's current route, so they only behave in a real full-page visit. The warning names the page, the href, and the exact root-absolute path to author instead. Co-Authored-By: Claude Fable 5 --- .../tests/acceptance/visit-all-links-test.ts | 29 +++++++++++++++---- test-support/src/routing/visit-all.ts | 8 +++++ 2 files changed, 31 insertions(+), 6 deletions(-) diff --git a/test-app/tests/acceptance/visit-all-links-test.ts b/test-app/tests/acceptance/visit-all-links-test.ts index d79c1c2..b799a8b 100644 --- a/test-app/tests/acceptance/visit-all-links-test.ts +++ b/test-app/tests/acceptance/visit-all-links-test.ts @@ -40,20 +40,37 @@ module('All Links', function (hooks) { assert.ok(visited.length > 0, 'the SPA links were still visited'); }); - test('relative links navigate against the current route', async function (assert) { + test('relative links navigate against the current route, and warn', async function (assert) { // /docs/page renders ``, which from /docs/page must land // on /docs/other. The browser resolves the anchor's element.href against // the TEST PAGE's url (/tests) — which would send the router to /other — // so the crawler points the anchor at the currentURL-resolved target - // before clicking. The navigation assertions inside visitAllLinks fail - // this test if the click lands anywhere else. + // before clicking (and warns: relative hrefs are an authoring hazard the + // app author should fix). The navigation assertions inside visitAllLinks + // fail this test if the click lands anywhere else. const visited: string[] = []; + const warnings: string[] = []; + const originalWarn = console.warn; - await visitAllLinks((url) => { - visited.push(url); - }); + console.warn = (...args: unknown[]) => warnings.push(args.join(' ')); + + try { + await visitAllLinks((url) => { + visited.push(url); + }); + } finally { + console.warn = originalWarn; + } assert.true(visited.includes('/docs/other'), `visited: ${visited.join(', ')}`); + + const warning = warnings.find((w) => w.includes('Relative href "other"')); + + assert.true(Boolean(warning), 'warned about the relative href'); + assert.true( + Boolean(warning?.includes('"/docs/other"')), + 'the warning names the resolved target to author instead', + ); }); test('each target is visited once', async function (assert) { diff --git a/test-support/src/routing/visit-all.ts b/test-support/src/routing/visit-all.ts index cb6f1eb..29ce70a 100644 --- a/test-support/src/routing/visit-all.ts +++ b/test-support/src/routing/visit-all.ts @@ -146,6 +146,14 @@ export async function visitAllLinks( * properLinks-and-router path with the production URL. */ if (!toVisit.original.startsWith('/')) { + console.warn( + `[visitAllLinks] Relative href "${toVisit.original}" found on ${returnTo}. ` + + `Relative hrefs resolve against the browser's URL rather than the app's current route, ` + + `so they only behave in a real full-page visit — they misresolve in this test harness ` + + `and under any mount where the address bar isn't the route (embeds, previews). ` + + `The crawler pointed this click at the resolved target instead. ` + + `Action: update the source document to link to "${toVisit.href}" directly.`, + ); link.setAttribute('href', toVisit.href); }