From bb14bf407434abb45e54a08f5b446ff34a0736e7 Mon Sep 17 00:00:00 2001 From: Roland Date: Wed, 5 Aug 2026 16:09:12 -0600 Subject: [PATCH] review: finish the sweep the first pass claimed The review was right that completeness was not done. Every finding held up. Three more pages mounted a `PageContainer` inside a route that already provides page chrome -- `ManageMap`, `MapPins`, `ManageZim` -- so they sat a further 1em down and 1em in from the tab bar than the viewers beside them. The same defect as ArchiveSettings, in a shape the scan could not see: it keyed on `PageContainer` route wrappers, and Map and Zim wrap their content in a `wrolpi-stack` div instead. It now locates routers first and asks whether the enclosing return provides chrome either way, resolves `element={}` as well as the bare form, and knows about `class` components -- `ManageZim` is one, and a scan that only knew functions reported it clean. Five top-level blocks still added their own margin to the stack's gap: the dashboard search row (52.8px against 17.6px, on the most-looked-at page in the app), the Playlists filter row, both Docs filter rows, and the Map pins filter. The extension page's hint panel had the same compound against a Mantine Stack's gap. ControllerPage held its four panel sections in a plain wrapper, so the admin page's stack saw one child and the panels kept sharing edges -- the exact hole Settings had. The dashboard was reported clean in the first pass and was not. The sweep measured wrapper-to-wrapper gaps, and that row's margin is INSIDE a Media wrapper, so it never appeared: the measurement, not the page, was at fault. Cypress gap assertions now derive from the stack's own `row-gap` instead of a 12-28px range that assumed the default interface scale and would have false-failed a correct layout at a larger one. Co-Authored-By: Claude Opus 5 (1M context) --- app/src/DashboardPage.js | 8 +- app/src/components/Common.test.js | 101 +++++++++++++-------- app/src/components/Docs.js | 6 +- app/src/components/Map.js | 22 +++-- app/src/components/Playlists.js | 4 +- app/src/components/Zim.js | 10 +- app/src/components/admin/ControllerPage.js | 4 +- app/src/components/admin/ExtensionPage.js | 4 +- app/src/components/ui/ui-layout.cy.js | 31 ++++--- 9 files changed, 125 insertions(+), 65 deletions(-) diff --git a/app/src/DashboardPage.js b/app/src/DashboardPage.js index a468b82c..3f60cd90 100644 --- a/app/src/DashboardPage.js +++ b/app/src/DashboardPage.js @@ -452,7 +452,13 @@ export function DashboardPage() { if (big) { inputProps.size = 'big'; } - return
+ /* + * No `marginBottom`. This row is a top-level block of the dashboard, so its own 2em was + * added to the page stack's gap -- 52.8px below the search box against 17.6px between + * every other pair of blocks, on the most-looked-at page in the app. One source of + * spacing, and it is the stack. + */ + return
{getSearchResultsInput(inputProps)} diff --git a/app/src/components/Common.test.js b/app/src/components/Common.test.js index 3d7ac75a..ca20148f 100644 --- a/app/src/components/Common.test.js +++ b/app/src/components/Common.test.js @@ -727,7 +727,7 @@ describe('PageContainer', () => { stacks.forEach(stack => expect(stack.textContent).toBe('Body')); }); - it('is never rendered by a page that a PageContainer already wraps', () => { + it('is never rendered by a page whose route already provides page chrome', () => { /* * `PageContainer` carries `margin-top: 1em` and `padding: 1em`, so nesting one inside * another applies both twice: the page is pushed 1em further down and inset 1em further @@ -736,10 +736,21 @@ describe('PageContainer', () => { * in another -- and read as "more margin than /archives/domains", which is exactly what * it was. * - * Nothing about a doubled margin throws, so this is a source scan. It reads each route - * wrapper (a `PageContainer` holding `` or ``), takes the components it - * routes to, and checks the ones defined in the same file -- which is where a route and - * its pages normally live together. + * Nothing about a doubled margin throws, so this is a source scan. It finds each router + * whose enclosing `return` provides page chrome, takes the components it routes to, and + * checks the ones declared in the same file -- which is where a route and its pages + * normally live together. + * + * "Chrome" is `PageContainer` OR a `wrolpi-stack` wrapper, not just the former. Keying on + * `PageContainer` alone missed `MapRoute` and `ZimRoute`, which wrap their own content in a + * stack instead -- and three pages under them (`ManageMap`, `MapPins`, `ManageZim`) mounted + * a `PageContainer` each, so they sat a further 1em down and 1em in from the tab bar than + * the viewers beside them. The same defect as Archive, in a shape the first scan could not + * see. + * + * Known limit: a layout that routes through `` has its `` list in App.js, + * naming components from other files, so the page cannot be resolved from one file's text. + * That is `VideosTabLayout`, whose pages are clean today but are not covered here. */ const src = path.join(__dirname, '..'); const files = (dir) => fs.readdirSync(dir, {withFileTypes: true}).flatMap(entry => { @@ -753,24 +764,9 @@ describe('PageContainer', () => { const offenders = []; for (const file of files(src)) { const text = fs.readFileSync(file, 'utf8'); - - /* - * EVERY `` in the file, not just the first. Checking only the first - * is what let this bug through a version of this test: in Archive.js the first one - * is `ArchiveSettingsPage`'s -- the offender -- and its block holds no ``, so - * the file was skipped before the route wrapper 400 lines below was ever looked at. - */ - for (const block of routeWrapperBlocks(text)) { - const routed = [...block.matchAll(/element=\{<(\w+)\s*\/>\}/g)].map(m => m[1]); - for (const name of routed) { - // The component's body, bounded by the next top-level declaration. - const declaration = new RegExp(`^(?:export\\s+)?(?:function|const)\\s+${name}\\b`, 'm'); - const found = declaration.exec(text); - if (!found) continue; // Defined elsewhere; out of this scan's reach. - const after = text.slice(found.index + 1); - const next = after.search(/^(?:export\s+)?(?:function|const|class)\s/m); - const body = next === -1 ? after : after.slice(0, next); - if (body.includes('')) { + for (const block of chromedRouterBlocks(text)) { + for (const name of routedComponents(block)) { + if (bodyOf(text, name).includes('')) { offenders.push(`${path.relative(src, file)}: ${name}`); } } @@ -778,33 +774,64 @@ describe('PageContainer', () => { } /* - * The premise: a scan that found no route wrappers, or routed to no components, would - * report no offenders and read exactly like a clean codebase. + * The premise: a scan that found no routers, or resolved no page components, would report + * no offenders and read exactly like a clean codebase. */ - const wrappers = files(src).flatMap(file => routeWrapperBlocks(fs.readFileSync(file, 'utf8'))); - expect(wrappers.length).toBeGreaterThan(2); - expect(wrappers.flatMap(b => [...b.matchAll(/element=\{<(\w+)\s*\/>\}/g)]).length) - .toBeGreaterThan(8); + const blocks = files(src).flatMap(file => chromedRouterBlocks(fs.readFileSync(file, 'utf8'))); + expect(blocks.length).toBeGreaterThan(3); + expect(blocks.flatMap(routedComponents).length).toBeGreaterThan(8); expect(offenders).toEqual([]); }); }); /** - * Every `...` in `text` that wraps a router -- so, the ones that - * wrap a whole section of the app rather than one page. All of them, because a file may hold both - * a route wrapper and the pages it routes to, in either order. + * Every `...` in `text` whose enclosing `return` provides page chrome -- a + * `PageContainer` or a `wrolpi-stack` wrapper. Those are the routers that have already spaced and + * inset the page, so a page under one must not do it again. + * + * All of them, and located from the router rather than from the chrome: keying on the first + * `` in a file missed this entirely in Archive.js, where the first one belongs to + * the offending page and holds no router at all. */ -const routeWrapperBlocks = (text) => { +const chromedRouterBlocks = (text) => { const blocks = []; let from = 0; for (;;) { - const start = text.indexOf('', from); + const start = text.indexOf('', from); if (start === -1) return blocks; - const end = text.indexOf('', start); + const end = text.indexOf('', start); if (end === -1) return blocks; - const block = text.slice(start, end); - if (/|/.test(block)) blocks.push(block); from = end + 1; + + const returned = text.lastIndexOf('return', start); + if (returned === -1) continue; + const chrome = text.slice(returned, start); + if (chrome.includes('') || chrome.includes('wrolpi-stack')) { + blocks.push(text.slice(start, end)); + } } }; + +/** + * The page components a router block routes to. Any capitalised tag that is not routing furniture, + * so `element={}` yields `Page` as well as the plain + * `element={}` form. + */ +const FURNITURE = new Set(['Route', 'Routes', 'ErrorBoundary', 'Suspense', 'Navigate', 'Outlet']); +const routedComponents = (block) => [...new Set( + [...block.matchAll(/<([A-Z]\w*)/g)].map(m => m[1]).filter(name => !FURNITURE.has(name)))]; + +/** + * The source of the component named `name`, bounded by the next top-level declaration. `class` is + * in both patterns because `ManageZim` is one, and a scan that only knew about functions would + * have reported it clean. + */ +const bodyOf = (text, name) => { + const declaration = new RegExp(`^(?:export\\s+)?(?:function|const|class)\\s+${name}\\b`, 'm'); + const found = declaration.exec(text); + if (!found) return ''; // Declared elsewhere; out of this scan's reach. + const after = text.slice(found.index + 1); + const next = after.search(/^(?:export\s+)?(?:function|const|class)\s/m); + return next === -1 ? after : after.slice(0, next); +}; diff --git a/app/src/components/Docs.js b/app/src/components/Docs.js index 881d6731..feff9329 100644 --- a/app/src/components/Docs.js +++ b/app/src/components/Docs.js @@ -469,7 +469,8 @@ function AuthorsPage() { const [searchStr, setSearchStr] = useOneQuery('author'); const searchInputRef = React.useRef(); - const header =
+ // No `marginBottom`: a top-level block of the page, spaced by the page's stack. + const header =
+ // No `marginBottom`: a top-level block of the page, spaced by the page's stack. + const header =
+ return <> Could not fetch map files - ; + ; } - return + /* + * A fragment, not a `PageContainer`. `MapRoute` already provides the page chrome -- a + * `wrolpi-stack` wrapper with its own top margin -- so a second one put this page a further + * 1em down and 1em in from the tab bar than the viewer beside it. + */ + return <>
Map Files
@@ -420,7 +424,7 @@ function ManageMap() {

You can also view the map at {MAP_VIEWER_URI}

 - + } const PIN_COLORS = ["red", "blue", "green", "yellow", "orange", "purple"]; @@ -605,13 +609,15 @@ function MapPins() { ) : pins; - return + // A fragment for the same reason as ManageMap above. The filter carries no `marginBottom` + // either: it is a top-level block of the page, and the route's stack spaces it. + return <> } placeholder='Filter by label or coordinates...' value={filter} onChange={(e) => setFilter(e.target.value)} - style={{marginBottom: '1em', width: '100%'}} + style={{width: '100%'}} /> - ; + ; } function MapPage() { diff --git a/app/src/components/Playlists.js b/app/src/components/Playlists.js index 158807e7..2e325367 100644 --- a/app/src/components/Playlists.js +++ b/app/src/components/Playlists.js @@ -128,7 +128,9 @@ export function PlaylistsPage() { } }; - const header = + // No `mb`: this is a top-level block of the page, and the page's stack spaces it. Its own + // margin was added to that gap -- the same fix as the twin filter rows on Channels/Domains. + const header = Could not fetch catalog; } - return + /* + * A fragment, not a `PageContainer`. `ZimRoute` already provides the page chrome -- a + * `wrolpi-stack` wrapper with its own top margin -- so a second one put this page a + * further 1em down and 1em in from the tab bar than the viewer beside it. + */ + return <>
Zim Files
{zimFilesBody} @@ -500,7 +504,7 @@ class ManageZim extends React.Component { - + } } diff --git a/app/src/components/admin/ControllerPage.js b/app/src/components/admin/ControllerPage.js index d559d97e..303bb2f8 100644 --- a/app/src/components/admin/ControllerPage.js +++ b/app/src/components/admin/ControllerPage.js @@ -1631,7 +1631,9 @@ function AdminControlsSection() { export function ControllerPage() { return ( -
+ // `wrolpi-stack`: these four sections are panels held in a wrapper of this page's own, + // so the stack on PageContainer does not reach them -- the same hole Settings had. +
diff --git a/app/src/components/admin/ExtensionPage.js b/app/src/components/admin/ExtensionPage.js index 52c11194..98589ce4 100644 --- a/app/src/components/admin/ExtensionPage.js +++ b/app/src/components/admin/ExtensionPage.js @@ -32,7 +32,9 @@ function DestinationHint() { // Fall through silently — the user can still copy manually. } }; - return + // No `marginBottom`: this Panel sits in a Mantine `Stack`, whose gap already spaces it. + // A margin here is added to that gap -- the compound the panel-margin approach caused. + return
This WROLPi's URL

After installing the extension, paste this URL into the diff --git a/app/src/components/ui/ui-layout.cy.js b/app/src/components/ui/ui-layout.cy.js index 8e9d1105..86c1e21a 100644 --- a/app/src/components/ui/ui-layout.cy.js +++ b/app/src/components/ui/ui-layout.cy.js @@ -142,15 +142,19 @@ describe('a page stacks its own blocks', () => { Calculators

); - cy.get('.wrolpi-panel').should(($panels) => { - const gaps = gapsBetween($panels); + cy.get('.wrolpi-stack').should(($stack) => { + /* + * Measured against the stack's own `row-gap` rather than a pixel range. The range was + * 12-28px, which assumes the default `--ui-scale`: at a larger scale the gap grows past + * 28 and a correct layout would have failed the assertion. A separate test covers the + * ratio; this one is about the panels really being that far apart. + */ + const declared = parseFloat(getComputedStyle($stack[0]).rowGap); + expect(declared, 'the stack declares a gap').to.be.greaterThan(0); + + const gaps = gapsBetween($stack[0].querySelectorAll('.wrolpi-panel')); expect(gaps, 'both pairs measured').to.have.length(2); - gaps.forEach((gap) => { - // Enough to read as a seam rather than a heavier border ... - expect(gap, 'gap between panels').to.be.at.least(12); - // ... and not so much that one page reads as unrelated cards. - expect(gap, 'gap between panels').to.be.at.most(28); - }); + gaps.forEach((gap) => expect(gap, 'gap between panels').to.be.closeTo(declared, 0.5)); }); }); @@ -243,9 +247,14 @@ describe('a page stacks its own blocks', () => { Also shown
); - cy.get('.wrolpi-panel:visible').should(($panels) => { - expect($panels, 'the hidden panel is not measured').to.have.length(2); - expect(gapsBetween($panels)[0], 'one gap, not two').to.be.at.most(28); + // One gap, not two: measured against the declared gap rather than a pixel ceiling, which + // would have false-failed at a larger interface scale instead of catching a doubled gap. + cy.get('.wrolpi-stack').should(($stack) => { + const declared = parseFloat(getComputedStyle($stack[0]).rowGap); + const shown = $stack[0].querySelectorAll('.wrolpi-panel:not([style*="none"])'); + const visible = [...shown].filter((el) => el.offsetParent !== null); + expect(visible, 'the hidden panel is not measured').to.have.length(2); + expect(gapsBetween(visible)[0], 'one gap, not two').to.be.closeTo(declared, 0.5); }); });