From 85eeb1cd8d6321dfe4b03fd4481aaea5bf9727b5 Mon Sep 17 00:00:00 2001 From: Roland Date: Wed, 5 Aug 2026 15:53:04 -0600 Subject: [PATCH] ui: stop /archives/settings wrapping itself in a second PageContainer `ArchiveRoute` wraps its `` in a PageContainer, and `ArchiveSettingsPage` wrapped itself in another. PageContainer carries `margin-top: 1em` and `padding: 1em`, so that page sat 1em lower and 1em further in on all four sides than /archives/domains beside it -- which is exactly what "seems to have more margin" was. It is a fragment now, so its panels are blocks of the route's own stack. Guarded by a source scan rather than a render, since a doubled margin throws nothing: it takes each PageContainer that wraps a router, reads the components it routes to, and fails if one of them renders a PageContainer of its own. The first version of that scan passed against the live bug. It looked at only the first `` in a file, and in Archive.js that is the offender itself, whose block holds no `` -- so the file was skipped before the route wrapper 400 lines below was reached. It reads every occurrence now, and the premise assertions count both the wrappers found and the routes within them. Two more top-level blocks gave up margins that were being added to the page's gap, both headers "matching" each other's pattern: the domain filter row (35.2px against 17.6px) and the channel filter row. /archives/settings and /archives/domains now measure identically -- tab bar bottom at 136.8px, first block 17.6px below it, content inset 17.6px -- as do the statistics and settings pages of every other tab group. Co-Authored-By: Claude Opus 5 (1M context) --- app/src/components/Archive.js | 15 ++++-- app/src/components/Channels.js | 5 +- app/src/components/Common.test.js | 83 +++++++++++++++++++++++++++++++ 3 files changed, 97 insertions(+), 6 deletions(-) diff --git a/app/src/components/Archive.js b/app/src/components/Archive.js index ec49bfe1..5a1edfc1 100644 --- a/app/src/components/Archive.js +++ b/app/src/components/Archive.js @@ -498,8 +498,9 @@ export function DomainsPage() { } }, {enableOnFormTags: false}); - // Header section matching ChannelsPage pattern - const header =
+ // Header section matching ChannelsPage pattern. No `marginBottom`: this is a top-level block + // of the page, and the page's stack spaces it -- its own margin was added to that gap. + const header =
navigator.clipboard.writeText('url')} />; - return + /* + * A fragment, not a `PageContainer`. `ArchiveRoute` already wraps its `` in one, and + * a second applies its `margin-top: 1em` and `padding: 1em` again -- so this page sat 1em + * lower and 1em further in on all four sides than /archives/domains beside it. As a fragment + * these panels become blocks of the route's own page stack, which spaces them. + */ + return <>
Archive Downloader Config
@@ -1081,7 +1088,7 @@ function ArchiveSettingsPage() { action={urlFieldNameClipboardButton} /> - + } function ArchivesPage() { diff --git a/app/src/components/Channels.js b/app/src/components/Channels.js index 0a9ed3df..b31f3c00 100644 --- a/app/src/components/Channels.js +++ b/app/src/components/Channels.js @@ -499,8 +499,9 @@ export function ChannelsPage() { } }, {enableOnFormTags: false}); - // Header section matching DomainsPage pattern - const header = + // Header section matching DomainsPage pattern. 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. + const header = { expect(stacks).toHaveLength(2); stacks.forEach(stack => expect(stack.textContent).toBe('Body')); }); + + it('is never rendered by a page that a PageContainer already wraps', () => { + /* + * `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 + * in on all four sides than every other page in the app. `/archives/settings` did this + * -- `ArchiveRoute` wraps its `` in one, and `ArchiveSettingsPage` wrapped itself + * 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. + */ + const src = path.join(__dirname, '..'); + const files = (dir) => fs.readdirSync(dir, {withFileTypes: true}).flatMap(entry => { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) return files(full); + if (!/\.jsx?$/.test(entry.name)) return []; + if (/\.(test|cy)\.jsx?$/.test(entry.name)) return []; + return [full]; + }); + + 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('')) { + offenders.push(`${path.relative(src, file)}: ${name}`); + } + } + } + } + + /* + * 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. + */ + 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); + + 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. + */ +const routeWrapperBlocks = (text) => { + const blocks = []; + let from = 0; + for (;;) { + const start = text.indexOf('', from); + if (start === -1) return blocks; + 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; + } +};