diff --git a/AGENTS.md b/AGENTS.md index 283a4c7..e011c62 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -73,7 +73,7 @@ npx eslint "lib/**/*.js" "app/**/*.{js,jsx}" "components/**/*" "tests/**/*" ``` app/ # Next.js App Router (static export) layout.jsx # CSP meta, header/footer, submission-target meta - not-found.jsx # 404 page + branch deep-link redirect (§6) + not-found.jsx # 404 page + in-place branch deep-link render (§6) error.jsx # global error boundary (+ per-route boundaries) page.jsx # landing (/) add-repo|about|api|.../page.jsx # static content pages @@ -139,8 +139,9 @@ coverage there when introducing new markup patterns. - **Static export constraints:** `dynamicParams = false` on dynamic routes; `generateStaticParams` enumerates `(username)`, `(username, repository)` and `(username, repository, main|master)` from `repositories.txt`; new repos render - client-side immediately thanks to the client-side listing refresh and the - `?branch=` deep-link redirect in `app/not-found.jsx`. + client-side immediately thanks to the client-side listing refresh, and + branch deep links (the branch is always a path parameter) are rendered + in place by the 404 shell (`app/not-found.jsx`) at their original URL. - **Widget loading:** CDN scripts load via `next/script` `lazyOnload` inside `components/report-view.jsx`, registered through `lib/widget-loader.js`); wasm/ESM bridges in `public/widgets/` run as native module scripts. diff --git a/README.md b/README.md index b7839e7..efac27e 100644 --- a/README.md +++ b/README.md @@ -268,7 +268,7 @@ GitLab Pages deploys from the `pages` job and serves ### 4. GitLab-specific considerations - **Client-side routing**: GitLab Pages serves `404.html` for unknown paths; the Next.js export already produces - `out/404.html` (from `app/not-found.jsx`), which handles branch deep-link recovery — no extra copy step needed. + `out/404.html` (from `app/not-found.jsx`), which renders branch deep links in place at their path URLs — no extra copy step needed. - **Submission processing**: set `submission-target` in `app/layout.jsx` to your `/`, extend the CSP `connect-src` with your GitLab base (`https://gitlab.com` or your self-managed host), and create a pipeline schedule (**CI/CD → Schedules**, e.g. every 10 minutes) — GitLab has no issue-triggered diff --git a/app/[username]/[repository]/[branch]/page.jsx b/app/[username]/[repository]/[branch]/page.jsx index 5253a70..579d36c 100644 --- a/app/[username]/[repository]/[branch]/page.jsx +++ b/app/[username]/[repository]/[branch]/page.jsx @@ -1,7 +1,7 @@ // /{username}/{repository}/{branch} — report shell pinned to a branch. -// Pre-generated for main/master; other branches reach this route via the -// not-found client redirect which conveys the branch through the ?branch= -// query parameter handled by ReportView. +// Pre-generated for main/master; other branches have no exported HTML and +// are rendered in place by the 404 shell (app/not-found.jsx) at the +// original path-parameter URL. import { Suspense } from 'react'; import { notFound } from 'next/navigation'; diff --git a/app/layout.jsx b/app/layout.jsx index e1f2f6a..ed721c5 100644 --- a/app/layout.jsx +++ b/app/layout.jsx @@ -54,11 +54,11 @@ export default function RootLayout({ children }) { vertically centered on one line. The row reuses the breadcrumbs' alignment formula (--width-content column, 0.8rem horizontal padding, border-box), so the first crumb's glyph sits at the menu - bar's left content edge and the toggle at its right content edge - — the two edges mirror each other (plans/css-only-dark-mode.md). - Suspense is required for useSearchParams (static export; the - fallback keeps prerendered HTML crumb-free and the toggle still - renders — an empty flex slot). */} + bar's left content edge and the toggle at its right content edge + — the two edges mirror each other (plans/css-only-dark-mode.md). + The Suspense boundary mirrors the fallback-free static-export + pattern (the prerendered HTML stays crumb-free and the toggle + still renders — an empty flex slot). */}
diff --git a/app/not-found.jsx b/app/not-found.jsx index fc44662..c07390d 100644 --- a/app/not-found.jsx +++ b/app/not-found.jsx @@ -1,23 +1,25 @@ 'use client'; -import { useEffect } from 'react'; +import { Suspense, lazy, useEffect } from 'react'; import Link from 'next/link'; import { usePathname } from 'next/navigation'; import { isValidGitHubName, STATIC_PAGES } from '../lib/routes.js'; import { getBasePath } from '../lib/base-path'; +// Lazy-loaded so the report-rendering bundle (renderer, table enhancement) +// stays out of the first-party budget of pages that never show a report — +// the chunk is only fetched when a branch deep link actually renders one. +const ReportView = lazy(() => import('../components/report-view')); + // Exported for the 404.html produced by the static export. GitHub Pages (and // similar static hosts) serve this page for any path that has no exported // HTML file. Two recovery behaviors: // - Branch deep links (/user/repo/) that are not pre-generated are -// redirected to the repo shell (/user/repo/) which loads the requested -// branch client-side (Technical Appendix §6). +// rendered in place by ReportView at the original URL — the branch is a +// path parameter, so no redirect is needed (Technical Appendix §6). // - Repos added after the last deploy get the "recently added" note // (Technical Appendix §5). -// -// `performRedirect` is injectable for tests (window.location is not -// configurable under jsdom). -export default function NotFound({ performRedirect }) { +export default function NotFound() { const pathname = usePathname() || ''; const basePath = getBasePath(); const path = basePath && pathname.startsWith(basePath) @@ -33,21 +35,32 @@ export default function NotFound({ performRedirect }) { isValidGitHubName(username) && isValidGitHubName(repository); - // Branch deep links are redirected to the statically generated repo shell; - // ReportView picks the requested branch back up from the query parameter - // (Technical Appendix §6). Branch names may contain slashes, so any - // segments beyond user/repo belong to the branch name. - const redirectTarget = - segments.length >= 3 && isRepoLike - ? `/${username}/${repository}/?branch=${encodeURIComponent(segments.slice(2).join('/'))}` - : null; + // Branch names may contain slashes, so any segments beyond user/repo + // belong to the branch name. Each segment must pass the same validation + // as classifyRoute's branch guard: the branch is interpolated into raw + // content URLs, so path-injection values must fall through to the 404 + // copy instead of being fetched. + const branchSegments = segments.slice(2); + const isBranchDeepLink = + segments.length >= 3 && + isRepoLike && + branchSegments.every(segment => isValidGitHubName(segment)); + // The exported 404.html statically carries the site title; report shells + // pre-generate `/@` titles, so match them here. useEffect(() => { - const redirect = performRedirect || (url => window.location.replace(url)); - if (redirectTarget) { - redirect(`${basePath}${redirectTarget}`); + if (isBranchDeepLink) { + document.title = `${username}/${repository}@${branchSegments.join('/')} - RefactorFirst`; } - }, [redirectTarget, basePath, performRedirect]); + }, [isBranchDeepLink, username, repository, branchSegments]); + + if (isBranchDeepLink) { + return ( + Loading report…

}> + + + ); + } return (
diff --git a/components/breadcrumbs.jsx b/components/breadcrumbs.jsx index 1981f95..f2cafda 100644 --- a/components/breadcrumbs.jsx +++ b/components/breadcrumbs.jsx @@ -10,7 +10,7 @@ // to the resolved branch so the trail identifies the displayed report. import { useEffect, useState } from 'react'; import Link from 'next/link'; -import { usePathname, useSearchParams } from 'next/navigation'; +import { usePathname } from 'next/navigation'; import { breadcrumbsFor, reportMatchesFor, @@ -25,9 +25,7 @@ import { */ export default function Breadcrumbs() { const pathname = usePathname(); - const searchParams = useSearchParams(); - const search = searchParams ? searchParams.toString() : ''; - const routeKey = `${pathname}?${search}`; + const routeKey = pathname || '/'; // The announced branch is keyed to the route it was announced on; a route // change resets it during render (the React-sanctioned alternative to a // setState call inside an effect). @@ -55,7 +53,6 @@ export default function Breadcrumbs() { const crumbs = breadcrumbsFor( pathname || '/', - search, resolved.key === routeKey ? resolved.branch : null ); if (crumbs.length === 0) return null; diff --git a/components/report-view.jsx b/components/report-view.jsx index 07c1b5b..4f22c7a 100644 --- a/components/report-view.jsx +++ b/components/report-view.jsx @@ -9,7 +9,6 @@ // lib/widget-loader.js. import { useCallback, useEffect, useRef, useState } from 'react'; -import { useSearchParams } from 'next/navigation'; import Script from 'next/script'; import { withBasePath } from '../lib/base-path'; import { detectHostingEnvironment, getPlatformBaseUrl, readMetaTag } from '../lib/host'; @@ -66,7 +65,6 @@ export default function ReportView({ enhanceTablesImpl = enhanceTables }) { const containerRef = useRef(null); - const searchParams = useSearchParams(); const [attempt, setAttempt] = useState(0); const [payload, setPayload] = useState(null); const [tableStates, setTableStates] = useState({}); @@ -80,7 +78,9 @@ export default function ReportView({ duration: toastDurationMs }); - const branch = branchProp || searchParams.get('branch') || undefined; + // The branch comes exclusively from the route (pre-generated shells pass + // it as a prop; the 404 shell renders unlisted branches in place). + const branch = branchProp || undefined; const handleTableAction = useCallback((tableId, patch, meta) => { if (meta?.restoreFocus) { diff --git a/lib/breadcrumbs.js b/lib/breadcrumbs.js index b017e9d..146d945 100644 --- a/lib/breadcrumbs.js +++ b/lib/breadcrumbs.js @@ -4,7 +4,7 @@ // when the only sensible link target would be the page itself (crumbs never // self-link). Routes that are not a user listing or repository report get no // trail (empty array), so landing and static content pages stay crumb-free. -import { classifyRoute, buildReportUrl, getQueryParam } from './routes.js'; +import { classifyRoute, buildReportUrl } from './routes.js'; // CustomEvent dispatched by ReportView once the report fetch resolves the // branch actually loaded (a `main` 404 falls back to `master`). Breadcrumbs @@ -79,13 +79,12 @@ export function reportMatchesFor(path, detail = {}) { * Builds the breadcrumb trail for a path. * * @param {string} path - Current pathname (basePath already stripped by Next). - * @param {string} [search] - Query string conveying ?branch= on deep links. * @param {string|null} [resolvedBranch] - Branch the report actually loaded * from (announced by ReportView after the fetch; null beforehand). * @returns {Array<{label: string, href?: string, current?: boolean}>} Crumbs * in hierarchical order; the last crumb is the current page. */ -export function breadcrumbsFor(path, search = '', resolvedBranch = null) { +export function breadcrumbsFor(path, resolvedBranch = null) { const route = classifyRoute(path); if (route.type === 'user') { @@ -97,9 +96,7 @@ export function breadcrumbsFor(path, search = '', resolvedBranch = null) { if (route.type === 'report') { const hasBranchSegment = path.split('/').filter(Boolean).length > 2; - const explicitBranch = hasBranchSegment - ? route.branch - : getQueryParam(search, 'branch'); + const explicitBranch = hasBranchSegment ? route.branch : null; // The resolved branch outranks any explicit/requested branch: when main // 404s and the fallback loads master, the label must say "master". const branch = resolvedBranch || explicitBranch || null; @@ -117,9 +114,9 @@ export function breadcrumbsFor(path, search = '', resolvedBranch = null) { return crumbs; } - // With an explicit branch (path segment or ?branch=), the repository - // crumb navigates back up to the default-branch report. Without one the - // only target would be the current page, so the crumb stays unlinked. + // With an explicit branch path segment, the repository crumb navigates + // back up to the default-branch report. Without one the only target + // would be the current page, so the crumb stays unlinked. crumbs.push( explicitBranch ? { label: route.repository, href: buildReportUrl(route.username, route.repository) } diff --git a/lib/routes.js b/lib/routes.js index 72690a4..319a23f 100644 --- a/lib/routes.js +++ b/lib/routes.js @@ -25,7 +25,9 @@ export function parseRoute(path) { return { username: parts[0] || null, repository: parts[1] || null, - branch: parts[2] || DEFAULT_BRANCH + // Join all branch segments, matching classifyRoute (paths such as + // /alice/repo/feature/login); default when none are present. + branch: parts.length > 2 ? parts.slice(2).join('/') : DEFAULT_BRANCH }; } @@ -44,7 +46,7 @@ export function isValidGitHubName(name) { // Classify a path into a route the application knows how to render. export function classifyRoute(path) { - const { username, repository, branch } = parseRoute(path); + const { username, repository } = parseRoute(path); if (!username) { return { type: 'landing' }; @@ -71,33 +73,35 @@ export function classifyRoute(path) { } const parts = path.split('/').filter(Boolean); - if (parts.length > 3) { - return { type: 'not-found' }; - } - // Path-injection guard: the branch segment is interpolated into raw - // content URLs; reject values like ".." that browsers would normalize - // into a different path. (Multi-segment branches are not expressible in - // classifyRoute paths — those arrive via ?branch= on the shell.) - if (!isValidGitHubName(branch)) { + // Branch names may contain slashes, so every segment beyond user/repo + // belongs to the branch name. Path-injection guard: each segment is + // interpolated into raw content URLs, so reject values like ".." that + // browsers would normalize into a different path. + const branchParts = parts.slice(2); + if (branchParts.some(part => !isValidGitHubName(part))) { return { type: 'not-found' }; } - return { type: 'report', username, repository, branch }; + return { + type: 'report', + username, + repository, + branch: branchParts.length ? branchParts.join('/') : DEFAULT_BRANCH + }; } export function buildReportUrl(username, repository, branch = DEFAULT_BRANCH) { const base = `/${encodeURIComponent(username)}/${encodeURIComponent(repository)}`; - return branch === DEFAULT_BRANCH ? base : `${base}/${encodeURIComponent(branch)}`; + if (branch === DEFAULT_BRANCH) return base; + // Encode each slash-delimited component while preserving the separators: + // a slashed branch like "feature/login" must stay real path segments so + // classifyRoute and the 404-shell deep-link render recognize the URL. + const branchPath = branch.split('/').map(encodeURIComponent).join('/'); + return `${base}/${branchPath}`; } export function buildRepositoryListUrl(fullName) { const [username, repository] = String(fullName).split('/'); return buildReportUrl(username, repository); } - -export function getQueryParam(search, name) { - const params = new URLSearchParams(search || ''); - const value = params.get(name); - return value === null ? null : value; -} diff --git a/lib/static-params.js b/lib/static-params.js index e81c62c..b982726 100644 --- a/lib/static-params.js +++ b/lib/static-params.js @@ -12,7 +12,7 @@ export function reportStaticParams(repositories) { } // Branch pages are pre-generated for the common default branches; any other -// branch deep link is handled client-side by the not-found redirect. +// branch deep link is rendered in place client-side by the 404 shell. export function branchStaticParams(repositories) { return repositories.flatMap(({ username, repository }) => ['main', 'master'].map(branch => ({ username, repository, branch })) diff --git a/tests/e2e/breadcrumbs.spec.js b/tests/e2e/breadcrumbs.spec.js index 422ef92..59d8564 100644 --- a/tests/e2e/breadcrumbs.spec.js +++ b/tests/e2e/breadcrumbs.spec.js @@ -71,8 +71,10 @@ test('a main 404 falls back to master and the trail relabels to the displayed br await expect(nav.locator('[aria-current="page"]')).toHaveText('master'); }); -test('branch deep link (?branch=) shows the deep-linked branch', async ({ page }) => { - await page.goto('/refactorfirst/refactorfirst/?branch=feature%2Flogin'); +test('branch deep link shows the deep-linked branch from the path', async ({ page }) => { + // Served by the 404 shell (feature/login is not pre-generated) — the + // branch path segments are joined into one crumb label. + await page.goto('/refactorfirst/refactorfirst/feature/login'); const nav = page.getByRole('navigation', { name: 'Breadcrumb' }); await expect(nav.locator('[aria-current="page"]')).toHaveText('feature/login'); }); diff --git a/tests/e2e/static-export-parity.spec.js b/tests/e2e/static-export-parity.spec.js index 67b4cd0..a62f3c3 100644 --- a/tests/e2e/static-export-parity.spec.js +++ b/tests/e2e/static-export-parity.spec.js @@ -3,8 +3,8 @@ import { readFileSync } from 'node:fs'; // Static-export parity (Technical Appendix §9): every repositories.txt entry // must resolve to generated HTML; unknown paths serve the app 404 with a -// real 404 status; branch deep links render the requested branch after the -// client-side redirect. +// real 404 status; branch deep links render the requested branch in place +// on the 404 shell, keeping the path-parameter URL. const repositories = readFileSync('repositories.txt', 'utf8') .split('\n') @@ -48,7 +48,7 @@ const SAMPLE_REPORT = { classCycles: { hasCycles: false, summary: [], largestCycle: { hasCycleMap: false } } }; -test('branch deep link redirects to the repo shell and renders that branch', async ({ page }) => { +test('branch deep link renders the requested branch in place, keeping the URL', async ({ page }) => { const requested = []; await page.route('**/raw.githubusercontent.com/**', route => { const url = route.request().url(); @@ -61,10 +61,29 @@ test('branch deep link redirects to the repo shell and renders that branch', asy }); await page.goto('/refactorfirst/refactorfirst/develop'); - // Client redirect rewrites the URL to the repo shell carrying the branch - await page.waitForURL('**/refactorfirst/refactorfirst/**?branch=develop**'); + // No redirect: the report renders on the 404 shell at the original URL + await expect(page).toHaveURL(/.*\/refactorfirst\/refactorfirst\/develop\/?$/); await expect(page.locator('h1').first()).toContainText('RefactorFirst Report for refactorfirst'); expect(requested.some(u => u.includes('/develop/.refactorfirst/refactor-first.json'))).toBe(true); + // The 404 copy must not co-render with the report + await expect(page.locator('.error-page')).toHaveCount(0); + // Title matches the pre-generated branch shells + await expect(page).toHaveTitle('refactorfirst/refactorfirst@develop - RefactorFirst'); +}); + +test('branch deep links with slashes render the full branch name', async ({ page }) => { + await page.route('**/raw.githubusercontent.com/**', route => { + const url = route.request().url(); + if (url.includes('/refs/heads/feature/login/.refactorfirst/refactor-first.json')) { + route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify(SAMPLE_REPORT) }); + } else { + route.fulfill({ status: 404 }); + } + }); + + await page.goto('/refactorfirst/refactorfirst/feature/login'); + await expect(page).toHaveURL(/.*\/refactorfirst\/refactorfirst\/feature\/login\/?$/); + await expect(page.locator('h1').first()).toContainText('RefactorFirst Report for refactorfirst'); }); test('recently added repo note shows for unlisted /user/repo 404s', async ({ page }) => { diff --git a/tests/integration/breadcrumbs.test.jsx b/tests/integration/breadcrumbs.test.jsx index c3b865f..59c2b93 100644 --- a/tests/integration/breadcrumbs.test.jsx +++ b/tests/integration/breadcrumbs.test.jsx @@ -1,17 +1,15 @@ // Breadcrumb navigation under the top menu: the trail mirrors the current // /// route with links back up the hierarchy. Pathname -// and query string are injected via mutable next/navigation mocks (same -// pattern as not-found.test.jsx). +// is injected via a mutable next/navigation mock (same pattern as +// not-found.test.jsx). import { describe, test, expect } from 'bun:test'; import { mock } from 'bun:test'; import { sharedNextNavigationMock } from './next-navigation-stub'; let currentPath = '/'; -let currentSearch = ''; mock.module('next/navigation', () => sharedNextNavigationMock({ - usePathname: () => currentPath, - useSearchParams: () => new URLSearchParams(currentSearch) + usePathname: () => currentPath })); import { render, installRtlDom } from './rtl'; @@ -25,12 +23,10 @@ installRtlDom(); * Sets the mocked route and mounts the breadcrumb component for assertions. * * @param {string} pathname - Path exposed by the navigation mock. - * @param {string} [search=''] - Query string exposed by the navigation mock. * @returns {import('@testing-library/react').RenderResult} Rendered test utilities. */ -function renderBreadcrumbs(pathname, search = '') { +function renderBreadcrumbs(pathname) { currentPath = pathname; - currentSearch = search; return render(_jsx(Breadcrumbs, {})); } @@ -58,7 +54,7 @@ describe('Breadcrumbs', () => { }); test('renders no breadcrumb nav on not-found paths', () => { - const { container } = renderBreadcrumbs('/alice/repo/main/extra'); + const { container } = renderBreadcrumbs('/alice/repo/